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

[Greg Kurz]

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

> +
> +    /* 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.

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

pgpmYHBo7fLvJ.pgp
Description: OpenPGP digital signature