Re: [Qemu-ppc] [PATCH v2 1/1] PPC: KVM: Support machine option to set VSMT mode

[Greg Kurz]

Hi Sam,

It seems you forgot to Cc: qemu-devel, which is mandatory for patches to be
merged in the master branch.

On Mon, 14 Aug 2017 13:40:27 +1000
Sam Bobroff <address@hidden> wrote:

> KVM now allows writing to KVM_CAP_PPC_SMT which has previously been
> read only. Doing so causes KVM to act, for that VM, as if the host's
> SMT mode was the given value. This is particularly important on Power
> 9 systems because their default value is 1, but they are able to
> support values up to 8.
> 
> This patch introduces a way to control this capability via a new
> machine property called VSMT ("Virtual SMT"). If the value is not set
> on the command line a default is chosen that is, when possible,
> compatible with legacy systems.
> 
> Signed-off-by: Sam Bobroff <address@hidden>
> ---
> Changes in v2:
> 
> * Reworked to keep SPAPR dependencies out of KVM code.
> 

It looks really cool, but I have some comments anyway :)

>  hw/ppc/spapr.c              | 105 
> ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h      |   1 +
>  target/ppc/kvm.c            |  20 ++++++++-
>  target/ppc/kvm_ppc.h        |  12 +++++
>  target/ppc/translate_init.c |  14 ------
>  5 files changed, 137 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cd6eb2d4a9..1baf54a310 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"
> @@ -2140,6 +2141,83 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
>      g_free(type);
>  }
>  
> +/* Note: Only called when kvm_enabled() == true */

Maybe drop the comment then and assert(kvm_enabled()) instead ?

> +static void show_vsmt_possible(void)
> +{
> +    int i;
> +    GString *g;
> +    char *s;
> +
> +    if (kvmppc_smt_possible()) {
> +        g = g_string_new(NULL);
> +        g_string_append_printf(g, "Available VSMT modes:");

This could be:

           g = g_string_new("Available VSMT modes:");

> +        for (i = 63; i >= 0; i--) {
> +            if ((1UL << i) & kvmppc_smt_possible()) {
> +                g_string_append_printf(g, " %lu", (1UL << i));
> +            }
> +        }
> +        s = g_string_free(g, false);
> +        error_report("%s", s);

IIUC, the purpose of show_vsmt_possible() is to provide hints if
we failed to set VSMT in KVM, right ? If so, then you should use
error_printf(), with trailing punctuation and newline.

> +        g_free(s);
> +    } else {
> +        error_report("This KVM seems to be too old to support VSMT.");

Same here.

> +    }
> +}
> +
> +static void spapr_set_vsmt_mode(sPAPRMachineState *spapr)
> +{
> +    bool vsmt_user = !!spapr->vsmt;
> +    int kvm_smt = kvmppc_smt_threads();
> +    int ret;
> +    const char *machine_name;
> +
> +    machine_name = 
> object_class_get_name(OBJECT_CLASS(SPAPR_MACHINE_GET_CLASS(spapr)));

This makes ./script/checkpatch.pl unhappy:

WARNING: line over 80 characters
#79: FILE: hw/ppc/spapr.c:2174:

Personally, I'd use a intermediate 'smc' variable for
SPAPR_MACHINE_GET_CLASS(spapr).

This being said, do we really need to output the versioned machine type ?
Or even to output the machine type at all ?

> +    if (!kvm_enabled() && (smp_threads > 1)) {
> +        error_report("TCG cannot support more than 1 thread/core "
> +                     "on a %s.", machine_name);

Please drop the trailing period (same applies to most error_report() call
sites in this patch).

FYI, from util/qemu-error.c:

/*
 * Print an error message to current monitor if we have one, else to stderr.
 * Format arguments like sprintf().  The resulting message should be
 * a single phrase, with no newline or trailing punctuation.
 * Prepend the current location and append a newline.
 * It's wrong to call this in a QMP monitor.  Use error_setg() there.
 */
void error_report(const char *fmt, ...)

> +        exit(1);
> +    }
> +    if (!is_power_of_2(smp_threads)) {
> +        error_report("Cannot support %d threads/core on a %s because "
> +                     "it must be a power of 2.", smp_threads, machine_name);
> +        exit(1);
> +    }
> +
> +    /* Detemine the VSMT mode to use: */
> +    if (vsmt_user) {
> +        if (spapr->vsmt < smp_threads) {
> +            error_report("Cannot support VSMT mode %d"
> +                         " because it must be >= threads/core (%d).",
> +                         spapr->vsmt, smp_threads);
> +            exit(1);
> +        }
> +        /* In this case, spapr->vsmt has been set by the command line */
> +    } else {
> +        /* Choose a VSMT mode that may be higher than necessary but is
> +         * likely to be compatible with hosts that don't have VSMT. */
> +        spapr->vsmt = MAX(kvm_smt, smp_threads);
> +    }
> +
> +    /* KVM: If necessary, set the SMT mode: */
> +    if (kvm_enabled() && (spapr->vsmt != kvm_smt)) {
> +        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"
> +                             " VSMT mode %d.)",
> +                             smp_threads, kvm_smt, spapr->vsmt);

This should be error_printf(), just like in show_vsmt_possible(), and maybe
drop the *ugly* parens :)

> +            }
> +            show_vsmt_possible();
> +            exit(1);
> +        }
> +    }
> +    /* else TCG: nothing to do currently */
> +    return;
> +}
> +
>  /* pSeries LPAR / sPAPR hardware init */
>  static void ppc_spapr_init(MachineState *machine)
>  {
> @@ -2272,6 +2350,8 @@ static void ppc_spapr_init(MachineState *machine)
>  
>      spapr_cpu_parse_features(spapr);
>  
> +    spapr_set_vsmt_mode(spapr);
> +
>      spapr_init_cpus(spapr);
>  
>      if (kvm_enabled()) {
> @@ -2656,6 +2736,25 @@ static void spapr_set_resize_hpt(Object *obj, const 
> char *value, Error **errp)
>      }
>  }
>  
> +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);

It looks like you don't need local_err and you could pass errp
to visit_type_uint32(), just like in spapr_get_vsmt() above.

> +}
> +
>  static void spapr_machine_initfn(Object *obj)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> @@ -2686,6 +2785,11 @@ static void spapr_machine_initfn(Object *obj)
>      object_property_set_description(obj, "resize-hpt",
>                                      "Resizing of the Hash Page Table 
> (enabled, disabled, required)",
>                                      NULL);
> +    object_property_add(obj, "vsmt", "uint32", spapr_get_vsmt,
> +                        spapr_set_vsmt, NULL, &spapr->vsmt, NULL);

I know that we don't pass an Error ** to object_property_add() in a bunch
of places, but this is bad (for example, someone could inadvertently reuse
the name of an already existing property). I suggest you pass &error_abort.

> +    object_property_set_description(obj, "vsmt",
> +                                    "Virtual SMT: KVM behaves as if this 
> were"
> +                                    " the host's SMT mode", NULL);

Same here. If we cannot set the description of the property we've just
added then something is clearly wrong in QEMU.

>  }
>  
>  static void spapr_machine_finalizefn(Object *obj)
> @@ -3872,3 +3976,4 @@ static void spapr_machine_register_types(void)
>  }
>  
>  type_init(spapr_machine_register_types)
> +

empty line damage

> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 86c982cf2c..68c510339f 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -99,6 +99,7 @@ struct sPAPRMachineState {
>      uint64_t rtc_offset; /* Now used only during incoming migration */
>      struct PPCTimebase tb;
>      bool has_graphics;
> +    uint32_t vsmt;       /* Virtual SMT mode (KVM's "core stride") */
>  
>      Notifier epow_notifier;
>      QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 5fc7f238eb..0b9d5b83fa 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -74,6 +74,7 @@ static int cap_interrupt_level = false;
>  static int cap_segstate;
>  static int cap_booke_sregs;
>  static int cap_ppc_smt;
> +static int cap_ppc_smt_possible;
>  static int cap_ppc_rma;
>  static int cap_spapr_tce;
>  static int cap_spapr_tce_64;
> @@ -130,7 +131,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_interrupt_level = kvm_check_extension(s, KVM_CAP_PPC_IRQ_LEVEL);
>      cap_segstate = kvm_check_extension(s, KVM_CAP_PPC_SEGSTATE);
>      cap_booke_sregs = kvm_check_extension(s, KVM_CAP_PPC_BOOKE_SREGS);
> -    cap_ppc_smt = kvm_check_extension(s, KVM_CAP_PPC_SMT);
> +    cap_ppc_smt_possible = kvm_check_extension(s, KVM_CAP_PPC_SMT_POSSIBLE);
>      cap_ppc_rma = kvm_check_extension(s, KVM_CAP_PPC_RMA);
>      cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE);
>      cap_spapr_tce_64 = kvm_check_extension(s, KVM_CAP_SPAPR_TCE_64);
> @@ -144,6 +145,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>       * only activated after this by kvmppc_set_papr() */
>      cap_htab_fd = kvm_check_extension(s, KVM_CAP_PPC_HTAB_FD);
>      cap_fixup_hcalls = kvm_check_extension(s, KVM_CAP_PPC_FIXUP_HCALL);
> +    cap_ppc_smt = kvm_vm_check_extension(s, KVM_CAP_PPC_SMT);

Maybe move the explanation from the cover letter to this patch's changelog:

* The intialization of KVM_CAP_PPC_SMT has changed slightly because it has
  changed (in KVM) from a global capability to a VM-specific one. This won't
  cause a problem on older KVMs because VM capabilities fall back to global
  ones.

>      cap_htm = kvm_vm_check_extension(s, KVM_CAP_PPC_HTM);
>      cap_mmu_radix = kvm_vm_check_extension(s, KVM_CAP_PPC_MMU_RADIX);
>      cap_mmu_hash_v3 = kvm_vm_check_extension(s, KVM_CAP_PPC_MMU_HASH_V3);
> @@ -2134,6 +2136,22 @@ int kvmppc_smt_threads(void)
>      return cap_ppc_smt ? cap_ppc_smt : 1;
>  }
>  
> +int kvmppc_set_smt_threads(int smt)
> +{
> +    int ret;
> +
> +    ret = kvm_vm_enable_cap(kvm_state, KVM_CAP_PPC_SMT, 0, smt, 0);
> +    if (!ret) {
> +        cap_ppc_smt = smt;
> +    }
> +    return ret;
> +}
> +
> +int kvmppc_smt_possible(void)
> +{
> +    return cap_ppc_smt_possible;
> +}
> +
>  #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 381afe6240..7b2962eb86 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -29,6 +29,8 @@ 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_smt_threads(void);
> +int kvmppc_smt_possible(void);
> +int kvmppc_set_smt_threads(int smt);
>  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);
> @@ -148,6 +150,16 @@ static inline int kvmppc_smt_threads(void)
>      return 1;
>  }
>  
> +static inline int kvmppc_smt_possible(void)
> +{
> +    return 1;
> +}
> +
> +static inline int kvmppc_set_smt_threads(int smt)
> +{
> +    return 0;
> +}
> +
>  static inline int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits)
>  {
>      return 0;
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 0ce923be46..7f6a349e43 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -9907,20 +9907,6 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      int max_smt = kvmppc_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;
> -    }
> -#endif
> -

<Deja vu>
Cool to see these checks being moved to the machine code. :)
</Deja vu>

>      cpu_exec_realizefn(cs, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);

pgpCLvs4_rhvh.pgp
Description: OpenPGP digital signature