Re: [Qemu-ppc] [PATCH v3] ppc: introduce CPUPPCState::cpu_dt_id

[Alexander Graf]

On 04.11.2013, at 02:10, Alexey Kardashevskiy <address@hidden> 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,address@hidden
> 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