Re: [Qemu-devel] [PATCH v2 1/1] ppc: spapr: Make VCPU ID handling private to SPAPR

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2661 bytes --]

On Wed, Aug 09, 2017 at 10:02:44AM +1000, Sam Bobroff wrote:
> On Tue, Aug 08, 2017 at 04:24:01PM +1000, David Gibson wrote:
> > On Mon, Aug 07, 2017 at 04:33:29PM +1000, Sam Bobroff wrote:
> > > The concept of a VCPU ID that differs from the CPU's index
> > > (cpu->cpu_index) exists only within SPAPR machines so, move the
> > > functions ppc_get_vcpu_id() and ppc_get_cpu_by_vcpu_id() into spapr.c
> > > and rename them appropriately.
> > > 
> > > Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> > > ---
> > > Changes in v2:
> > > 
> > > * Re-arranged so that spapr_vcpu_id() calls kvm_arch_vcpu_id() rather than the
> > > other way around.
> > 
> > Ah.. so close..
> > 
> > [snip]
> > > +int spapr_vcpu_id(PowerPCCPU *cpu)
> > > +{
> > > +    CPUState *cs = CPU(cpu);
> > > +
> > > +    if (kvm_enabled()) {
> > > +        return kvm_arch_vcpu_id(cs);
> > > +    } else {
> > > +        return cs->cpu_index;
> > > +    }
> > > +}
> > > +
> > > +PowerPCCPU *spapr_find_cpu(int vcpu_id)
> > > +{
> > > +    CPUState *cs;
> > > +
> > > +    CPU_FOREACH(cs) {
> > > +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > +
> > > +        if (cpu->vcpu_id == vcpu_id) {
> > 
> > This is still reaching into vcpu_id which should really be cpu
> > private, not in spapr.  You can use spapr_vcpu_id() which you already
> > defined to avoid this.
> 
> OK I'll post another version.
> 
> > With that done, I believe it should be possible to put cpu->vcpu_id in
> > an ifdef CONFIG_KVM and have things work.
> 
> That is indeed possible! ifdefs are needed around the initialization
> block in ppc_cpu_realizefn(), although here in spapr_vcpu_id() it doesn't seem
> necessary. (Presumably because kvm_enabled() becomes a constant 0 without
> CONFIG_KVM so the compiler can treat it like an ifdef.)
> 
> We don't want to actually patch it to use CONFIG_KVM though, do we?

Possibly not, not.

> I
> was intending (as part of the VSMT work) to make the vcpu_id a SPAPR
> concept, rather than a KVM one, so that machines could (in theory
> anyway) be migrated between KVM and non-KVM systems.

Uh.. the goal is good, I'm not sure that's the right way of going
about it, but I may just be misunderstanding your brief description.
I'll look at the actual patches once they're ready.

> 
> > > +            return cpu;
> > > +        }
> > > +    }
> > > +
> > > +    return NULL;
> > > +}
> > 
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]