Re: [Qemu-devel] [PATCH v3] ppc: introduce CPUPPCState::cpu_dt_id
On Tue, 2013-11-05 at 07:00 +0100, Alexander Graf wrote: Am 05.11.2013 um 02:48 schrieb Scott Wood scottw...@freescale.com: On Tue, 2013-11-05 at 12:26 +1100, Alexey Kardashevskiy wrote: On 11/05/2013 06:42 AM, Scott Wood wrote: On Mon, 2013-11-04 at 10:41 +0100, Alexander Graf wrote: What we really have are 3 semantically separate entities: * QEMU internal cpu id * KVM internal cpu id * DT exposed cpu id As you have noted, it's a good idea to keep the QEMU internal cpu id linear, thus completely separate from the others. The DT exposed cpu id should be 100% local to hw/ppc/spapr*.c. I don't think any code outside of the DT generation and anything that accesses the Virtual Processor Number in sPAPR needs to care about the DT cpu id. All that code is 100% KVM agnostic. This patch isn't just for sPAPR... On e500 the DT cpu id is supposed to match the MPIC cpu id. At least is my patch correct for e500? I think so. I do not really know what is the difference between e500 and spapr in this part. e500 does not have sPAPR, and if the DT ID is 100% local to hw/ppc/spapr*.c then the MPIC code will have a problem. Currently we don't support smt on e500 in QEMU, but e6500 (an e500-derivative) does have smt, so it could happen in the future. Sure, at that point we add logic that syncs the kvm and dt ids to whatever the kernel expects from the e500 machine file. The current logic that says every Nth vcpu id is a core, the ones in between are threads is fairly implementation specific to the spapr hv code, no? It's the same on e6500, though I agree that in theory it should not be generic code specifying the scheme. My point relates to the consumer of this information -- device tree CPU ID is a concept that makes sense outside of sPAPR. Note that the connection between the interrupt controller and the CPU reg is an ePAPR requirement, not just a coincidence, or something we only do on MPIC. PIR is also supposed to match the device tree CPU ID -- especially under KVM, where PIR is not modifiable by the guest, in the absence of a device tree binding specifically for PIR which we do not have. -Scott
Re: [Qemu-devel] [PATCH v3] ppc: introduce CPUPPCState::cpu_dt_id
On 04.11.2013, at 02:10, Alexey Kardashevskiy a...@ozlabs.ru wrote: Normally CPUState::cpu_index is used to pick the right CPU for various operations. However default consecutive numbering does not always work for POWERPC. For example, on POWER7 (which supports 4 threads per core), -smp 8,threads=4 should create CPUs with indexes 0,1,2,3,4,5,6,7 and -smp 8,threads=1 should create CPUs with indexes 0,4,8,12,16,20,24,28. These indexes are reflected in /proc/device-tree/cpus/PowerPC,POWER7@XX and used to call KVM VCPU's ioctls. In order to achieve this, kvmppc_fixup_cpu() was introduced. Roughly speaking, it multiplies cpu_index by the number of threads per core. This approach has disadvantages such as: 1. NUMA configuration stays broken after the fixup; 2. CPU-related commands from QEMU Monitor do not work properly as the accept fixed CPU indexes and the user does not really know what they are after fixup as the number of threads per core changes between CPU versions and via QEMU command line. This introduces a new @cpu_dt_id field in the CPUPPCState struct which is set from @cpu_index by default but can be fixed later to the value which a hypervisor can accept. This also introduces two POWERPC-arch specific functions: 1. int ppc_get_vcpu_dt_id(CPUState *cs) - returns a device-tree ID for a CPU; 2. CPUState *ppc_get_vcpu_by_dt_id(int cpu_dt_id) - finds CPUState by a device-tree CPU ID. This uses the new functions to: 1. fix emulated XICS hypercall handlers as they receive fixed CPU indexes; 2. fix XICS-KVM to enable in-kernel XICS on right CPU; 3. compose correct device-tree. This removes @cpu_index fixup as @cpu_dt_id is used instead so QEMU monitor can accept command-line CPU indexes again. So this patch feels awkward. We use the dt fixup at random places in completely dt unrelated code paths. What we really have are 3 semantically separate entities: * QEMU internal cpu id * KVM internal cpu id * DT exposed cpu id As you have noted, it's a good idea to keep the QEMU internal cpu id linear, thus completely separate from the others. The DT exposed cpu id should be 100% local to hw/ppc/spapr*.c. I don't think any code outside of the DT generation and anything that accesses the Virtual Processor Number in sPAPR needs to care about the DT cpu id. All that code is 100% KVM agnostic. The KVM internal cpu id should probably be a new field in the generic CPUState that gets used by kvm specific code that needs to know the KVM internal cpu id a vcpu was created with. The flow should be that kvm_arch_vcpu_id() tells kvm_init_vcpu() the kvm internal id to use which then stores it in CPUState. That way you can always get the KVM intenal cpu id from a CPU struct. All the references to this ID should _only_ happen from KVM only code. Alex
Re: [Qemu-devel] [PATCH v3] ppc: introduce CPUPPCState::cpu_dt_id
On 11/04/2013 08:41 PM, Alexander Graf wrote: On 04.11.2013, at 02:10, Alexey Kardashevskiy a...@ozlabs.ru wrote: Normally CPUState::cpu_index is used to pick the right CPU for various operations. However default consecutive numbering does not always work for POWERPC. For example, on POWER7 (which supports 4 threads per core), -smp 8,threads=4 should create CPUs with indexes 0,1,2,3,4,5,6,7 and -smp 8,threads=1 should create CPUs with indexes 0,4,8,12,16,20,24,28. These indexes are reflected in /proc/device-tree/cpus/PowerPC,POWER7@XX and used to call KVM VCPU's ioctls. In order to achieve this, kvmppc_fixup_cpu() was introduced. Roughly speaking, it multiplies cpu_index by the number of threads per core. This approach has disadvantages such as: 1. NUMA configuration stays broken after the fixup; 2. CPU-related commands from QEMU Monitor do not work properly as the accept fixed CPU indexes and the user does not really know what they are after fixup as the number of threads per core changes between CPU versions and via QEMU command line. This introduces a new @cpu_dt_id field in the CPUPPCState struct which is set from @cpu_index by default but can be fixed later to the value which a hypervisor can accept. This also introduces two POWERPC-arch specific functions: 1. int ppc_get_vcpu_dt_id(CPUState *cs) - returns a device-tree ID for a CPU; 2. CPUState *ppc_get_vcpu_by_dt_id(int cpu_dt_id) - finds CPUState by a device-tree CPU ID. This uses the new functions to: 1. fix emulated XICS hypercall handlers as they receive fixed CPU indexes; 2. fix XICS-KVM to enable in-kernel XICS on right CPU; 3. compose correct device-tree. This removes @cpu_index fixup as @cpu_dt_id is used instead so QEMU monitor can accept command-line CPU indexes again. So this patch feels awkward. We use the dt fixup at random places in completely dt unrelated code paths. Yep. I called it smp_cpu_index in the first place :) What we really have are 3 semantically separate entities: * QEMU internal cpu id * KVM internal cpu id * DT exposed cpu id As you have noted, it's a good idea to keep the QEMU internal cpu id linear, thus completely separate from the others. The DT exposed cpu id should be 100% local to hw/ppc/spapr*.c. I don't think any code outside of the DT generation and anything that accesses the Virtual Processor Number in sPAPR needs to care about the DT cpu id. All that code is 100% KVM agnostic. The KVM internal cpu id should probably be a new field in the generic CPUState that gets used by kvm specific code that needs to know the KVM internal cpu id a vcpu was created with. The flow should be that kvm_arch_vcpu_id() tells kvm_init_vcpu() the kvm internal id to use which then stores it in CPUState. That way you can always get the KVM intenal cpu id from a CPU struct. All the references to this ID should _only_ happen from KVM only code. If DT id is local to spapr*, then how do I implement kvm_arch_vcpu_id()? Where will it get the fixed value? Do the same calculation in two different places (for device tree and for kvm)? -- Alexey
Re: [Qemu-devel] [PATCH v3] ppc: introduce CPUPPCState::cpu_dt_id
On 04.11.2013, at 10:58, Alexey Kardashevskiy a...@ozlabs.ru wrote: On 11/04/2013 08:41 PM, Alexander Graf wrote: On 04.11.2013, at 02:10, Alexey Kardashevskiy a...@ozlabs.ru wrote: Normally CPUState::cpu_index is used to pick the right CPU for various operations. However default consecutive numbering does not always work for POWERPC. For example, on POWER7 (which supports 4 threads per core), -smp 8,threads=4 should create CPUs with indexes 0,1,2,3,4,5,6,7 and -smp 8,threads=1 should create CPUs with indexes 0,4,8,12,16,20,24,28. These indexes are reflected in /proc/device-tree/cpus/PowerPC,POWER7@XX and used to call KVM VCPU's ioctls. In order to achieve this, kvmppc_fixup_cpu() was introduced. Roughly speaking, it multiplies cpu_index by the number of threads per core. This approach has disadvantages such as: 1. NUMA configuration stays broken after the fixup; 2. CPU-related commands from QEMU Monitor do not work properly as the accept fixed CPU indexes and the user does not really know what they are after fixup as the number of threads per core changes between CPU versions and via QEMU command line. This introduces a new @cpu_dt_id field in the CPUPPCState struct which is set from @cpu_index by default but can be fixed later to the value which a hypervisor can accept. This also introduces two POWERPC-arch specific functions: 1. int ppc_get_vcpu_dt_id(CPUState *cs) - returns a device-tree ID for a CPU; 2. CPUState *ppc_get_vcpu_by_dt_id(int cpu_dt_id) - finds CPUState by a device-tree CPU ID. This uses the new functions to: 1. fix emulated XICS hypercall handlers as they receive fixed CPU indexes; 2. fix XICS-KVM to enable in-kernel XICS on right CPU; 3. compose correct device-tree. This removes @cpu_index fixup as @cpu_dt_id is used instead so QEMU monitor can accept command-line CPU indexes again. So this patch feels awkward. We use the dt fixup at random places in completely dt unrelated code paths. Yep. I called it smp_cpu_index in the first place :) What we really have are 3 semantically separate entities: * QEMU internal cpu id * KVM internal cpu id * DT exposed cpu id As you have noted, it's a good idea to keep the QEMU internal cpu id linear, thus completely separate from the others. The DT exposed cpu id should be 100% local to hw/ppc/spapr*.c. I don't think any code outside of the DT generation and anything that accesses the Virtual Processor Number in sPAPR needs to care about the DT cpu id. All that code is 100% KVM agnostic. The KVM internal cpu id should probably be a new field in the generic CPUState that gets used by kvm specific code that needs to know the KVM internal cpu id a vcpu was created with. The flow should be that kvm_arch_vcpu_id() tells kvm_init_vcpu() the kvm internal id to use which then stores it in CPUState. That way you can always get the KVM intenal cpu id from a CPU struct. All the references to this ID should _only_ happen from KVM only code. If DT id is local to spapr*, then how do I implement kvm_arch_vcpu_id()? Where will it get the fixed value? Do the same calculation in two different places (for device tree and for kvm)? kvm_arch_vcpu_id() won't get called until qemu_init_vcpu() is issued from ppc_cpu_realizefn(). So if instead of calling cpu_ppc_init() you split up the function and set the kvm id property before realize from ppc_spapr_init(), that should work, no? Alex
Re: [Qemu-devel] [PATCH v3] ppc: introduce CPUPPCState::cpu_dt_id
On Mon, 2013-11-04 at 10:41 +0100, Alexander Graf wrote: What we really have are 3 semantically separate entities: * QEMU internal cpu id * KVM internal cpu id * DT exposed cpu id As you have noted, it's a good idea to keep the QEMU internal cpu id linear, thus completely separate from the others. The DT exposed cpu id should be 100% local to hw/ppc/spapr*.c. I don't think any code outside of the DT generation and anything that accesses the Virtual Processor Number in sPAPR needs to care about the DT cpu id. All that code is 100% KVM agnostic. This patch isn't just for sPAPR... On e500 the DT cpu id is supposed to match the MPIC cpu id. -Scott
Re: [Qemu-devel] [PATCH v3] ppc: introduce CPUPPCState::cpu_dt_id
On 11/05/2013 06:42 AM, Scott Wood wrote: On Mon, 2013-11-04 at 10:41 +0100, Alexander Graf wrote: What we really have are 3 semantically separate entities: * QEMU internal cpu id * KVM internal cpu id * DT exposed cpu id As you have noted, it's a good idea to keep the QEMU internal cpu id linear, thus completely separate from the others. The DT exposed cpu id should be 100% local to hw/ppc/spapr*.c. I don't think any code outside of the DT generation and anything that accesses the Virtual Processor Number in sPAPR needs to care about the DT cpu id. All that code is 100% KVM agnostic. This patch isn't just for sPAPR... On e500 the DT cpu id is supposed to match the MPIC cpu id. At least is my patch correct for e500? I do not really know what is the difference between e500 and spapr in this part. -- Alexey
Re: [Qemu-devel] [PATCH v3] ppc: introduce CPUPPCState::cpu_dt_id
On Tue, 2013-11-05 at 12:26 +1100, Alexey Kardashevskiy wrote: On 11/05/2013 06:42 AM, Scott Wood wrote: On Mon, 2013-11-04 at 10:41 +0100, Alexander Graf wrote: What we really have are 3 semantically separate entities: * QEMU internal cpu id * KVM internal cpu id * DT exposed cpu id As you have noted, it's a good idea to keep the QEMU internal cpu id linear, thus completely separate from the others. The DT exposed cpu id should be 100% local to hw/ppc/spapr*.c. I don't think any code outside of the DT generation and anything that accesses the Virtual Processor Number in sPAPR needs to care about the DT cpu id. All that code is 100% KVM agnostic. This patch isn't just for sPAPR... On e500 the DT cpu id is supposed to match the MPIC cpu id. At least is my patch correct for e500? I think so. I do not really know what is the difference between e500 and spapr in this part. e500 does not have sPAPR, and if the DT ID is 100% local to hw/ppc/spapr*.c then the MPIC code will have a problem. Currently we don't support smt on e500 in QEMU, but e6500 (an e500-derivative) does have smt, so it could happen in the future. -Scott
Re: [Qemu-devel] [PATCH v3] ppc: introduce CPUPPCState::cpu_dt_id
Am 05.11.2013 um 02:48 schrieb Scott Wood scottw...@freescale.com: On Tue, 2013-11-05 at 12:26 +1100, Alexey Kardashevskiy wrote: On 11/05/2013 06:42 AM, Scott Wood wrote: On Mon, 2013-11-04 at 10:41 +0100, Alexander Graf wrote: What we really have are 3 semantically separate entities: * QEMU internal cpu id * KVM internal cpu id * DT exposed cpu id As you have noted, it's a good idea to keep the QEMU internal cpu id linear, thus completely separate from the others. The DT exposed cpu id should be 100% local to hw/ppc/spapr*.c. I don't think any code outside of the DT generation and anything that accesses the Virtual Processor Number in sPAPR needs to care about the DT cpu id. All that code is 100% KVM agnostic. This patch isn't just for sPAPR... On e500 the DT cpu id is supposed to match the MPIC cpu id. At least is my patch correct for e500? I think so. I do not really know what is the difference between e500 and spapr in this part. e500 does not have sPAPR, and if the DT ID is 100% local to hw/ppc/spapr*.c then the MPIC code will have a problem. Currently we don't support smt on e500 in QEMU, but e6500 (an e500-derivative) does have smt, so it could happen in the future. Sure, at that point we add logic that syncs the kvm and dt ids to whatever the kernel expects from the e500 machine file. The current logic that says every Nth vcpu id is a core, the ones in between are threads is fairly implementation specific to the spapr hv code, no? Alex
[Qemu-devel] [PATCH v3] ppc: introduce CPUPPCState::cpu_dt_id
Normally CPUState::cpu_index is used to pick the right CPU for various
operations. However default consecutive numbering does not always work
for POWERPC.
For example, on POWER7 (which supports 4 threads per core),
-smp 8,threads=4 should create CPUs with indexes 0,1,2,3,4,5,6,7 and
-smp 8,threads=1 should create CPUs with indexes 0,4,8,12,16,20,24,28.
These indexes are reflected in /proc/device-tree/cpus/PowerPC,POWER7@XX
and used to call KVM VCPU's ioctls. In order to achieve this,
kvmppc_fixup_cpu() was introduced. Roughly speaking, it multiplies
cpu_index by the number of threads per core.
This approach has disadvantages such as:
1. NUMA configuration stays broken after the fixup;
2. CPU-related commands from QEMU Monitor do not work properly as
the accept fixed CPU indexes and the user does not really know
what they are after fixup as the number of threads per core changes
between CPU versions and via QEMU command line.
This introduces a new @cpu_dt_id field in the CPUPPCState struct which
is set from @cpu_index by default but can be fixed later to the value
which a hypervisor can accept. This also introduces two POWERPC-arch
specific functions:
1. int ppc_get_vcpu_dt_id(CPUState *cs) - returns a device-tree ID
for a CPU;
2. CPUState *ppc_get_vcpu_by_dt_id(int cpu_dt_id) - finds CPUState by
a device-tree CPU ID.
This uses the new functions to:
1. fix emulated XICS hypercall handlers as they receive fixed CPU indexes;
2. fix XICS-KVM to enable in-kernel XICS on right CPU;
3. compose correct device-tree.
This removes @cpu_index fixup as @cpu_dt_id is used instead so QEMU monitor
can accept command-line CPU indexes again.
Cc: Badari Pulavarty pbad...@linux.vnet.ibm.com
Cc: Paul Mackerras pau...@samba.org
Cc: David Gibson da...@gibson.dropbear.id.au
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Paolo Bonzini pbonz...@redhat.com
Cc: Bharat Bhushan bharat.bhus...@freescale.com
Cc: Scott Wood scottw...@freescale.com
Signed-off-by: Alexey Kardashevskiy a...@ozlabs.ru
---
Changes:
v3:
* althouth e500 does not tweak CPU indexes, e500 and openpic-kvm
are fixed too; hopefully it does not break anything.
v2:
* added PPC-specific ppc_get_vcpu_dt_id() and ppc_get_vcpu_by_dt_id()
* fixed kvm_arch_vcpu_id() to use ppc_get_vcpu_dt_id()
* fixed emulated XICS
* removed kvm_arch_vcpu_id() stub for non-KVM case
---
hw/intc/openpic_kvm.c | 2 +-
hw/intc/xics.c | 15 +--
hw/intc/xics_kvm.c | 15 +--
hw/ppc/e500.c | 5 +++--
hw/ppc/ppc.c| 25 +
hw/ppc/spapr.c | 9 +
hw/ppc/spapr_hcall.c| 2 +-
hw/ppc/spapr_rtas.c | 4 ++--
target-ppc/cpu.h| 6 ++
target-ppc/kvm.c| 4 ++--
target-ppc/translate_init.c | 1 +
11 files changed, 68 insertions(+), 20 deletions(-)
diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
index c7f7b84..a43d2be 100644
--- a/hw/intc/openpic_kvm.c
+++ b/hw/intc/openpic_kvm.c
@@ -228,7 +228,7 @@ int kvm_openpic_connect_vcpu(DeviceState *d, CPUState *cs)
encap.cap = KVM_CAP_IRQ_MPIC;
encap.args[0] = opp-fd;
-encap.args[1] = cs-cpu_index;
+encap.args[1] = ppc_get_vcpu_dt_id(cs);
return kvm_vcpu_ioctl(cs, KVM_ENABLE_CAP, encap);
}
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index a05..866ee08 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -33,6 +33,17 @@
#include qemu/error-report.h
#include qapi/visitor.h
+static int get_cpu_index_by_dt_id(int cpu_dt_id)
+{
+CPUState *cs = ppc_get_vcpu_by_dt_id(cpu_dt_id);
+
+if (cs) {
+return cs-cpu_index;
+}
+
+return -1;
+}
+
void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
{
CPUState *cs = CPU(cpu);
@@ -659,7 +670,7 @@ static target_ulong h_cppr(PowerPCCPU *cpu,
sPAPREnvironment *spapr,
static target_ulong h_ipi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
target_ulong opcode, target_ulong *args)
{
-target_ulong server = args[0];
+target_ulong server = get_cpu_index_by_dt_id(args[0]);
target_ulong mfrr = args[1];
if (server = spapr-icp-nr_servers) {
@@ -728,7 +739,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPREnvironment
*spapr,
}
nr = rtas_ld(args, 0);
-server = rtas_ld(args, 1);
+server = get_cpu_index_by_dt_id(rtas_ld(args, 1));
priority = rtas_ld(args, 2);
if (!ics_valid_irq(ics, nr) || (server = ics-icp-nr_servers)
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index c203646..091fcca 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -65,7 +65,8 @@ static void icp_get_kvm_state(ICPState *ss)
ret = kvm_vcpu_ioctl(ss-cs, KVM_GET_ONE_REG, reg);
if (ret != 0) {
error_report(Unable to retrieve KVM interrupt controller state
- for CPU %d: %s, ss-cs-cpu_index, strerror(errno));
+ for CPU %ld: %s, kvm_arch_vcpu_id(ss-cs),
+
