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

[Alexander Graf]

On 04.11.2013, at 10:58, Alexey Kardashevskiy <address@hidden> wrote:

> On 11/04/2013 08:41 PM, Alexander Graf wrote:
>> 
>> 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.
> 
> 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