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

[Sam Bobroff]

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 <address@hidden>
> > ---
> > 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? 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.

> > +            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