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

[Sam Bobroff]

On Fri, Aug 04, 2017 at 01:10:19PM +1000, David Gibson wrote:
> On Thu, Aug 03, 2017 at 04:28:52PM +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 
> 
> Mostly good, but...
> 
> [snip]
> > +int spapr_vcpu_id(PowerPCCPU *cpu)
> > +{
> > +return cpu->vcpu_id;
> > +}
> > +
> > +PowerPCCPU *spapr_find_cpu(int vcpu_id)
> > +{
> > +CPUState *cs;
> > +
> > +CPU_FOREACH(cs) {
> > +PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +
> > +if (cpu->vcpu_id == vcpu_id) {
> > +return cpu;
> > +}
> > +}
> > +
> > +return NULL;
> > +}
> 
> [...]
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 7ccb350c5f..2bf2727860 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -512,7 +512,7 @@ bool kvmppc_is_mem_backend_page_size_ok(const char 
> > *obj_path)
> >  
> >  unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> >  {
> > -return ppc_get_vcpu_id(POWERPC_CPU(cpu));
> > +return spapr_vcpu_id(POWERPC_CPU(cpu));
> >  }
> 
> Here you've replaced an implicit dependency on spapr details in the
> generic code with an explicit dependency on spapr details.  That's the
> wrong direction.

Ah right, I'll flip it around.

> Instead _this_ one should directly reference vcpu_id, the spapr one
> should be something like:
> 
>   if (kvm)
>   return kvm_arch_vcpu_id(...)
>   else
>   return cpu_index;

OK.

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

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

[David Gibson]

On Thu, Aug 03, 2017 at 04:28:52PM +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 

Mostly good, but...

[snip]
> +int spapr_vcpu_id(PowerPCCPU *cpu)
> +{
> +return cpu->vcpu_id;
> +}
> +
> +PowerPCCPU *spapr_find_cpu(int vcpu_id)
> +{
> +CPUState *cs;
> +
> +CPU_FOREACH(cs) {
> +PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +if (cpu->vcpu_id == vcpu_id) {
> +return cpu;
> +}
> +}
> +
> +return NULL;
> +}

[...]
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 7ccb350c5f..2bf2727860 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -512,7 +512,7 @@ bool kvmppc_is_mem_backend_page_size_ok(const char 
> *obj_path)
>  
>  unsigned long kvm_arch_vcpu_id(CPUState *cpu)
>  {
> -return ppc_get_vcpu_id(POWERPC_CPU(cpu));
> +return spapr_vcpu_id(POWERPC_CPU(cpu));
>  }

Here you've replaced an implicit dependency on spapr details in the
generic code with an explicit dependency on spapr details.  That's the
wrong direction.

Instead _this_ one should directly reference vcpu_id, the spapr one
should be something like:

if (kvm)
return kvm_arch_vcpu_id(...)
else
return cpu_index;

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


signature.asc
Description: PGP signature