Re: [Qemu-devel] [PATCHv2 1/2] spapr: Allow some cases where we can't set VSMT mode in the kernel

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4304 bytes --]

On Tue, Jan 16, 2018 at 10:20:18AM +0100, Greg Kurz wrote:
> On Tue, 16 Jan 2018 15:47:13 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > At present if we require a vsmt mode that's not equal to the kernel's
> > default, and the kernel doesn't let us change it (e.g. because it's an old
> > kernel without support) then we always fail.
> > 
> > But in fact we can cope with the kernel having a different vsmt as long as
> >   a) it's >= the actual number of vthreads/vcore (so that guest threads
> >      that are supposed to be on the same core act like it)
> >   b) it's a submultiple of the requested vsmt mode (so that guest threads
> >      spaced by the vsmt value will act like they're on different cores)
> > 
> > Allowing this case gives us a bit more freedom to adjust the vsmt behaviour
> > without breaking existing cases.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> 
> I could check the following on a POWER9 host:
> 
> $ ./ppc64-softmmu/qemu-system-ppc64 -accel kvm -smp threads=1
> qemu-system-ppc64: warning: Failed to set KVM's VSMT mode to 8 (errno -22)
> 
> and the guest boots.
> 
> $ ./ppc64-softmmu/qemu-system-ppc64 -accel kvm -smp threads=2
> qemu-system-ppc64: Failed to set KVM's VSMT mode to 8 (errno -22)
> On PPC, a VM with 2 threads/core on a host with 1 threads/core requires the
>  use of VSMT mode 8.
> This KVM seems to be too old to support VSMT.
> 
> and QEMU exits.

I assume the above is with an old kernel that doesn't have the ability
to set the SMT cap?

> 
> Tested-by: Greg Kurz <groug@kaod.org>
> 
> Just one minor remark below but anyway:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> >  hw/ppc/spapr.c | 26 +++++++++++++++++++-------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index e35214bfc3..6d3613d934 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2314,17 +2314,29 @@ static void spapr_set_vsmt_mode(sPAPRMachineState *spapr, Error **errp)
> >      if (kvm_enabled() && (spapr->vsmt != kvm_smt)) {
> >          ret = kvmppc_set_smt_threads(spapr->vsmt);
> >          if (ret) {
> > +            /* Looks like KVM isn't able to change VSMT mode */
> >              error_setg(&local_err,
> >                         "Failed to set KVM's VSMT mode to %d (errno %d)",
> >                         spapr->vsmt, ret);
> > -            if (!vsmt_user) {
> > -                error_append_hint(&local_err, "On PPC, a VM with %d threads/"
> > -                             "core on a host with %d threads/core requires "
> > -                             " the use of VSMT mode %d.\n",
> > -                             smp_threads, kvm_smt, spapr->vsmt);
> > +            /* We can live with that if the default one is big enough
> > +             * for the number of threads, and a submultiple of the one
> > +             * we want.  In this case we'll waste some vcpu ids, but
> > +             * behaviour will be correct */
> > +            if ((kvm_smt >= smp_threads) && (spapr->vsmt % kvm_smt) == 0) {
> 
> Inconsistent use of parens in the left and right operands of &&
> 
> > +                warn_report_err(local_err);
> > +                local_err = NULL;
> > +                goto out;
> > +            } else {
> > +                if (!vsmt_user) {
> > +                    error_append_hint(&local_err,
> > +                                      "On PPC, a VM with %d threads/core"
> > +                                      " on a host with %d threads/core"
> > +                                      " requires the use of VSMT mode %d.\n",
> > +                                      smp_threads, kvm_smt, spapr->vsmt);
> > +                }
> > +                kvmppc_hint_smt_possible(&local_err);
> > +                goto out;
> >              }
> > -            kvmppc_hint_smt_possible(&local_err);
> > -            goto out;
> >          }
> >      }
> >      /* else TCG: nothing to do currently */
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]