Re: [Qemu-ppc] [PATCH RFC 3/4] PPC: KVM: Support machine option to set VSMT mode

[Greg Kurz]

On Thu, 29 Jun 2017 13:57:19 +1000
Sam Bobroff <address@hidden> wrote:
> On Wed, Jun 28, 2017 at 03:16:12PM +0200, Greg Kurz wrote:
> > On Tue, 27 Jun 2017 10:22:55 +1000
> > Sam Bobroff <address@hidden> wrote:
[...] 
> > > +    int vsmt = SPAPR_MACHINE(qdev_get_machine())->vsmt;  
> > 
> > Hmm... it doesn't look right for CPU code to rely on machine stuff.  
> 
> Yeah, I don't really like this either, see below.
> 

And also, this will cause non-sPAPR machines to abort...

> > >  #endif
> > >  
> > >      cpu_exec_realizefn(cs, &local_err);
> > > @@ -9851,14 +9840,14 @@ static void ppc_cpu_realizefn(DeviceState *dev, 
> > > Error **errp)
> > >      }
> > >  
> > >  #if !defined(CONFIG_USER_ONLY)
> > > -    cpu->cpu_dt_id = (cs->cpu_index / smp_threads) * max_smt
> > > +    cpu->cpu_dt_id = (cs->cpu_index / smp_threads) * vsmt
> > >          + (cs->cpu_index % smp_threads);
> > >  
> > >      if (kvm_enabled() && !kvm_vcpu_id_is_valid(cpu->cpu_dt_id)) {
> > >          error_setg(errp, "Can't create CPU with id %d in KVM", 
> > > cpu->cpu_dt_id);
> > >          error_append_hint(errp, "Adjust the number of cpus to %d "
> > >                            "or try to raise the number of threads per 
> > > core\n",
> > > -                          cpu->cpu_dt_id * smp_threads / max_smt);
> > > +                          cpu->cpu_dt_id * smp_threads / vsmt);  
> > 
> > Moreover vsmt is only needed to compute cpu_dt_id which is also a PAPR
> > abstraction actually. I remember David mentioning that he would rather
> > like cpu->cpu_dt_id to be dropped and replaced by a helper in the machine
> > code.
> >   
> > >          return;
> > >      }
> > >  #endif  
> >   
> 
> I agree, but I couldn't see a simple way to improve it. Do you have a
> suggestion for how (and where) to implement it in the machine,
> presumably in a way that allows us to use it from translate_init.c
> without including spapr.h?
> 
> I looked at removing cpu_dt_id but I thought it was likely that this
> code would change and I wanted to get that resolved first. I'll try
> adding it to the next version. (As an alternative I looked at putting
> the vsmt value into the CPU class but it didn't seem to be obviously the
> right place either, and it made it harder to set from the command line.)
> 

I suggest you get rid of cpu_dt_id as preliminary cleanup.

$ git grep cpu_dt_id
hw/ppc/e500.c:                 ppc_get_vcpu_dt_id(pcpu));
hw/ppc/e500.c:                              ppc_get_vcpu_dt_id(pcpu));

I don't know this platform but it doesn't seem to support multi-threaded
cores (the machine init code doesn't use smp_threads). So I'm not sure it
needs to rely on the funky vCPU id computation we currently have for sPAPR.

hw/ppc/ppc.c:int ppc_get_vcpu_dt_id(PowerPCCPU *cpu)
hw/ppc/ppc.c:    return cpu->cpu_dt_id;

Maybe the the computation should be open-coded here for sPAPR
machines and return cs->cpu_index for non-sPAPR machines ?

This could be achieved with an intermediary class for all
PPC machines.

hw/ppc/ppc.c:PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
hw/ppc/ppc.c:        if (cpu->cpu_dt_id == cpu_dt_id) {

And use ppc_get_vcpu_dt_id() here.

hw/ppc/spapr.c:    int index = ppc_get_vcpu_dt_id(cpu);
hw/ppc/spapr.c:    int index = ppc_get_vcpu_dt_id(cpu);
hw/ppc/spapr.c:        int index = ppc_get_vcpu_dt_id(cpu);
hw/ppc/spapr.c:    int index = ppc_get_vcpu_dt_id(cpu);
hw/ppc/spapr.c:        int index = ppc_get_vcpu_dt_id(cpu);
hw/ppc/spapr.c:    int id = ppc_get_vcpu_dt_id(cpu);
hw/ppc/spapr.c:static ICPState *spapr_icp_get(XICSFabric *xi, int cpu_dt_id)
hw/ppc/spapr.c:    PowerPCCPU *cpu = ppc_get_vcpu_by_dt_id(cpu_dt_id);
hw/ppc/spapr_hcall.c:            if (cpu->cpu_dt_id == target) {

Ditto.

target/ppc/cpu.h: * @cpu_dt_id: CPU index used in the device tree. KVM uses 
this index too
target/ppc/cpu.h:    int cpu_dt_id;
target/ppc/cpu.h: * ppc_get_vcpu_dt_id:
target/ppc/cpu.h:int ppc_get_vcpu_dt_id(PowerPCCPU *cpu);
target/ppc/cpu.h: * @cpu_dt_id: a device tree id
target/ppc/cpu.h: * Searches for a CPU by @cpu_dt_id.
target/ppc/cpu.h:PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id);
target/ppc/kvm.c:    return ppc_get_vcpu_dt_id(POWERPC_CPU(cpu));

^^ is kvm_arch_vcpu_id().

target/ppc/translate_init.c:    cpu->cpu_dt_id = (cs->cpu_index / smp_threads) 
* max_smt
target/ppc/translate_init.c:    if (kvm_enabled() && 
!kvm_vcpu_id_is_valid(cpu->cpu_dt_id)) {
target/ppc/translate_init.c:        error_setg(errp, "Can't create CPU with id 
%d in KVM", cpu->cpu_dt_id);
target/ppc/translate_init.c:                          cpu->cpu_dt_id * 
smp_threads / max_smt);

Cheers,

--
Greg

> Thanks,
> Sam.
> 
>

pgpbNed2kmVWx.pgp
Description: OpenPGP digital signature