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

[Sam Bobroff]

On Fri, Jun 30, 2017 at 10:52:30AM +0200, Greg Kurz wrote:
> 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...

I'll rework this.

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

I did try this and it didn't seem to end up any better, but I'll take
your comments below into account and have anoter go :-) I agree that
it seems like a good idea.

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