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>
---
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) {
+ 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 */
--
2.14.3
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.
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 */
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
On Tue, 16 Jan 2018 23:24:32 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> 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?
>
Yes this was tested with a 4.11 kernel (setting of SMT came with 4.12).
> >
> > 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 */
> >
>
On 16/01/2018 05:47, David Gibson 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>
> ---
> 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) {
I agree with Greg: inconsistent use of parenthesis, should be
if (kvm_smt >= smp_threads && (spapr->vsmt % kvm_smt) == 0) {
Anyway:
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Thanks,
Laurent
On Tue, Jan 16, 2018 at 11:34:52AM +0100, Laurent Vivier wrote:
> On 16/01/2018 05:47, David Gibson 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>
> > ---
> > 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) {
>
> I agree with Greg: inconsistent use of parenthesis, should be
>
> if (kvm_smt >= smp_threads && (spapr->vsmt % kvm_smt) == 0) {
Corrected in my tree.
> Anyway:
>
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
>
> Thanks,
> Laurent
>
--
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
© 2016 - 2025 Red Hat, Inc.