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

[Sam Bobroff]

On Mon, Aug 14, 2017 at 05:52:09PM +0200, Greg Kurz wrote:
> Hi Sam,
> 
> It seems you forgot to Cc: qemu-devel, which is mandatory for patches to be
> merged in the master branch.

Oh, sorry! I knew that. Fixed.

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

Thanks, I'm always looking to improve :-)

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

Good idea.

> > +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:");

Thanks!

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

Yes it is, and I will.

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

OK.

> > +    }
> > +}
> > +
> > +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).

Sure, will do.

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

No, we don't have to. I wanted to change the previous message, which
just said "on PPC" because it actually doesn't apply to all PPC
machines, only the SPAPR ones. So I thought the message was misleading.
What do you think?

> > +    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, ...)

Ah, right!

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

OK!

> > +            }
> > +            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.

Thanks!

Just as an aside, you might have noticed that those two accessor
functions are completely generic code, and in fact some grep'ing shows
that the same thing (or something functionally equivalent) is done in
several other places. I'll have a go at improving that in a later patch
set :-)

> > +}
> > +
> >  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.

I had a look at the other properties added here, and one uses
error_fatal while the rest are all NULL.
How about I change the VSMT ones to error_abort in this patch and then
do a follow up patch to change the rest to error_abort as well?

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

Ouch. Fixed.

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

OK, good idea.

> >      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);
> 

Thanks so much for all the improvements!

I'll post a new version ASAP.

Cheers,
Sam.