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

[Sam Bobroff]

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:
> 
> > Currently, on Power9, the default value for KVM_CAP_PPC_SMT is one, so
> > VMs cannot currently use more than one thread/core.  Also, for Power7
> > and above, when creating VCPUs on PPC KVM and the VM uses less
> > threads/core than the host, QEMU must skip some VCPU IDs so that VCPUs
> > are grouped correctly in VCOREs.  This is because KVM divides the VCPU
> > ID by the number of host threads/core to determine the VCORE ID.
> > 
> > PPC KVM will soon allow writing to KVM_CAP_PPC_SMT to set the Virtual
> > SMT mode. If N is successfully written:
> > * KVM will use VCPU ID / N to determine the VCORE ID.
> > * KVM will support the VM using N threads/core.
> > 
> > To exploit this, allow the VSMT mode to be set on the command line.
> > If it is not given, choose a default value that is compatible with
> > hosts without this feature. This allows:
> > * VCPU IDs to be allocated without skipping.
> > * Power9 hosts to run VMs with more than one thread/core.
> > * Migration between hosts with different but compatible threads/core.
> > 
> > Signed-off-by: Sam Bobroff <address@hidden>
> > ---
> >  hw/ppc/spapr.c              | 84 
> > ++++++++++++++++++++++++++++++++++++++++++++-
> >  target/ppc/kvm.c            |  5 +++
> >  target/ppc/kvm_ppc.h        |  1 +
> >  target/ppc/translate_init.c | 23 ++++---------
> >  4 files changed, 95 insertions(+), 18 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index b33dc1b5c0..c9ec1e87a1 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -26,6 +26,7 @@
> >   */
> >  #include "qemu/osdep.h"
> >  #include "qapi/error.h"
> > +#include "qapi/visitor.h"
> >  #include "sysemu/sysemu.h"
> >  #include "sysemu/numa.h"
> >  #include "hw/hw.h"
> > @@ -2035,6 +2036,60 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
> >      g_free(type);
> >  }
> >  
> > +static void ppc_set_vsmt_mode(sPAPRMachineState *spapr)
> > +{
> > +    bool vsmt_user = !!spapr->vsmt;
> > +    int kvm_smt = (kvm_enabled() ? kvmppc_get_smt_threads() : 0);
> > +    int ret;
> > +
> > +    if (!is_power_of_2(smp_threads)) {
> > +        error_report("Cannot support %d threads/core on PPC,"
> > +                     " because it must be a power of 2.", smp_threads);
> > +        exit(1);
> > +    }
> > +    if (!kvm_enabled() && (smp_threads > 1)) {
> > +        error_report("Cannot support more than 1 thread/core on PPC with 
> > TCG.");
> > +        exit(1);
> > +    }
> 
> Cool to see these checks being moved to the machine code. :)

I thought it was better this way too, thanks :-)

> > +
> > +    /* Detemine the VSMT mode to use: */
> > +    if (vsmt_user) {
> > +        if (!is_power_of_2(spapr->vsmt)) {
> > +            error_report("Cannot support VSMT mode %d"
> > +                         " because it must be a power of 2.", spapr->vsmt);
> > +            goto error;
> > +        }
> > +        if (spapr->vsmt < smp_threads) {
> > +            error_report("Cannot support VSMT mode %d"
> > +                         " because it must be >= threads/core (%d).",
> > +                         spapr->vsmt, smp_threads);
> > +            goto error;
> > +        }
> > +    } else {
> > +        spapr->vsmt = MAX(kvm_smt, smp_threads);
> > +    }
> > +
> > +    if (kvm_enabled() && (spapr->vsmt != kvmppc_get_smt_threads())) {
> > +        ret = kvmppc_set_smt_threads(spapr->vsmt); 
> > +        if (ret) {
> > +            error_report("Failed to set KVM's VSMT mode to %d, errno %d.",
> > +                         spapr->vsmt, ret);
> > +            if (!vsmt_user) {
> > +                error_report("(On PPC, a VM with %d threads/core on a host"
> > +                             " with %d threads/core requires the use of a"
> > +                             " VSMT mode >= %d.)",
> > +                             smp_threads, kvm_smt, spapr->vsmt);
> > +            }
> > +            goto error;
> > +        }
> > +    }
> > +    /* else TCG: nothing to do currently */
> > +    return;
> > +error:
> > +    exit(1);
> > +
> > +}
> > +
> >  /* pSeries LPAR / sPAPR hardware init */
> >  static void ppc_spapr_init(MachineState *machine)
> >  {
> > @@ -2133,6 +2188,8 @@ static void ppc_spapr_init(MachineState *machine)
> >  
> >      ppc_cpu_parse_features(machine->cpu_model);
> >  
> > +    ppc_set_vsmt_mode(spapr);
> > +
> >      spapr_init_cpus(spapr);
> >  
> >      if (kvm_enabled()) {
> > @@ -2483,13 +2540,32 @@ static void spapr_set_modern_hotplug_events(Object 
> > *obj, bool value,
> >      spapr->use_hotplug_event_source = value;
> >  }
> >  
> > +static void spapr_get_vsmt(Object *obj, Visitor *v, const char *name,
> > +                                   void *opaque, Error **errp)
> > +{
> > +    visit_type_uint32(v, name, (uint32_t *)opaque, errp);
> > +}
> > +
> > +static void spapr_set_vsmt(Object *obj, Visitor *v, const char *name,
> > +                                   void *opaque, Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +
> > +    visit_type_uint32(v, name, (uint32_t *)opaque, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +out:
> > +    error_propagate(errp, local_err);
> > +}
> > +
> >  static void spapr_machine_initfn(Object *obj)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> >  
> >      spapr->htab_fd = -1;
> >      spapr->use_hotplug_event_source = true;
> > -    spapr->vsmt = (kvm_enabled() ? kvmppc_get_smt_threads() : 1);
> > +    spapr->vsmt = 0;
> >      object_property_add_str(obj, "kvm-type",
> >                              spapr_get_kvm_type, spapr_set_kvm_type, NULL);
> >      object_property_set_description(obj, "kvm-type",
> > @@ -2504,6 +2580,11 @@ static void spapr_machine_initfn(Object *obj)
> >                                      " place of standard EPOW events when 
> > possible"
> >                                      " (required for memory hot-unplug 
> > support)",
> >                                      NULL);
> > +    object_property_add(obj, "vsmt", "uint32", spapr_get_vsmt,
> > +                        spapr_set_vsmt, NULL, &spapr->vsmt, NULL);
> > +    object_property_set_description(obj, "vsmt",
> > +                                    "Virtual SMT: KVM behaves as if this 
> > were"
> > +                                    " the host's SMT mode", NULL);
> >  }
> >  
> >  static void spapr_machine_finalizefn(Object *obj)
> > @@ -3643,3 +3724,4 @@ static void spapr_machine_register_types(void)
> >  }
> >  
> >  type_init(spapr_machine_register_types)
> > +
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 74dfab95eb..da562fb100 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -2121,6 +2121,11 @@ int kvmppc_get_smt_threads(void)
> >      return kvm_vm_check_extension(kvm_state, KVM_CAP_PPC_SMT);
> >  }
> >  
> > +int kvmppc_set_smt_threads(int vsmt)
> > +{
> > +    return kvm_vm_enable_cap(kvm_state, KVM_CAP_PPC_SMT, 0, vsmt, 0);
> > +}
> > +
> >  #ifdef TARGET_PPC64
> >  off_t kvmppc_alloc_rma(void **rma)
> >  {
> > diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> > index cdc1407462..5c21ce668c 100644
> > --- a/target/ppc/kvm_ppc.h
> > +++ b/target/ppc/kvm_ppc.h
> > @@ -29,6 +29,7 @@ void kvmppc_set_papr(PowerPCCPU *cpu);
> >  int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr);
> >  void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
> >  int kvmppc_get_smt_threads(void);
> > +int kvmppc_set_smt_threads(int vsmt);
> >  int kvmppc_clear_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
> >  int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
> >  int kvmppc_set_tcr(PowerPCCPU *cpu);
> > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > index 0d3da9d746..51cc2cb031 100644
> > --- a/target/ppc/translate_init.c
> > +++ b/target/ppc/translate_init.c
> > @@ -33,6 +33,9 @@
> >  #include "hw/qdev-properties.h"
> >  #include "hw/ppc/ppc.h"
> >  #include "mmu-book3s-v3.h"
> > +#if !defined(CONFIG_USER_ONLY)
> > +#include "hw/ppc/spapr.h"
> > +#endif
> >  
> >  //#define PPC_DUMP_CPU
> >  //#define PPC_DEBUG_SPR
> > @@ -9827,21 +9830,7 @@ static void ppc_cpu_realizefn(DeviceState *dev, 
> > Error **errp)
> >      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> >      Error *local_err = NULL;
> >  #if !defined(CONFIG_USER_ONLY)
> > -    int max_smt = kvmppc_get_smt_threads();
> > -#endif
> > -
> > -#if !defined(CONFIG_USER_ONLY)
> > -    if (smp_threads > max_smt) {
> > -        error_setg(errp, "Cannot support more than %d threads on PPC with 
> > %s",
> > -                   max_smt, kvm_enabled() ? "KVM" : "TCG");
> > -        return;
> > -    }
> > -    if (!is_power_of_2(smp_threads)) {
> > -        error_setg(errp, "Cannot support %d threads on PPC with %s, "
> > -                   "threads count must be a power of 2.",
> > -                   smp_threads, kvm_enabled() ? "KVM" : "TCG");
> > -        return;
> > -    }
> > +    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.

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

Thanks,
Sam.