[PULL,1/1] machine: Disallow specifying topology parameters as zero

Commit Message

Eduardo Habkost Aug. 16, 2021, 9:06 p.m. UTC

From: Yanan Wang <[e-mail]>

In the SMP configuration, we should either provide a topology
parameter with a reasonable value (greater than zero) or just
omit it and QEMU will compute the missing value. Users should
have never provided a configuration with parameters as zero
(e.g. -smp 8,sockets=0) which should be treated as invalid.

But commit 1e63fe68580 (machine: pass QAPI struct to mc->smp_parse)
has added some doc which implied that 'anything=0' is valid and
has the same semantics as omitting a parameter.

To avoid meaningless configurations possibly introduced by users
in the future and consequently a necessary deprecation process,
fix the doc and also add the corresponding sanity check.

Fixes: 1e63fe68580 (machine: pass QAPI struct to mc->smp_parse)
Suggested-by: Andrew Jones <[e-mail]>
Signed-off-by: Yanan Wang <[e-mail]>
Reviewed-by: Daniel P. Berrange <[e-mail]>
Tested-by: Daniel P. Berrange <[e-mail]>
Reviewed-by: Andrew Jones <[e-mail]>
Reviewed-by: Cornelia Huck <[e-mail]>
Message-Id: <[e-mail]>
Signed-off-by: Eduardo Habkost <[e-mail]>
---
 hw/core/machine.c | 14 ++++++++++++++
 qapi/machine.json |  6 +++---
 qemu-options.hx   | 12 +++++++-----
 3 files changed, 24 insertions(+), 8 deletions(-)

Comments

Paolo Bonzini Aug. 16, 2021, 9:37 p.m. UTC | #1

How do we know that no one has ever used such configuration? The conversion
was meant to be bug-compatible.

Paolo

Il lun 16 ago 2021, 23:06 Eduardo Habkost <[e-mail]> ha scritto:

> From: Yanan Wang <[e-mail]>
>
> In the SMP configuration, we should either provide a topology
> parameter with a reasonable value (greater than zero) or just
> omit it and QEMU will compute the missing value. Users should
> have never provided a configuration with parameters as zero
> (e.g. -smp 8,sockets=0) which should be treated as invalid.
>
> But commit 1e63fe68580 (machine: pass QAPI struct to mc->smp_parse)
> has added some doc which implied that 'anything=0' is valid and
> has the same semantics as omitting a parameter.
>
> To avoid meaningless configurations possibly introduced by users
> in the future and consequently a necessary deprecation process,
> fix the doc and also add the corresponding sanity check.
>
> Fixes: 1e63fe68580 (machine: pass QAPI struct to mc->smp_parse)
> Suggested-by: Andrew Jones <[e-mail]>
> Signed-off-by: Yanan Wang <[e-mail]>
> Reviewed-by: Daniel P. Berrange <[e-mail]>
> Tested-by: Daniel P. Berrange <[e-mail]>
> Reviewed-by: Andrew Jones <[e-mail]>
> Reviewed-by: Cornelia Huck <[e-mail]>
> Message-Id: <[e-mail]>
> Signed-off-by: Eduardo Habkost <[e-mail]>
> ---
>  hw/core/machine.c | 14 ++++++++++++++
>  qapi/machine.json |  6 +++---
>  qemu-options.hx   | 12 +++++++-----
>  3 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 54e040587dd..a7e119469aa 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -832,6 +832,20 @@ static void machine_set_smp(Object *obj, Visitor *v,
> const char *name,
>          return;
>      }
>
> +    /*
> +     * A specified topology parameter must be greater than zero,
> +     * explicit configuration like "cpus=0" is not allowed.
> +     */
> +    if ((config->has_cpus && config->cpus == 0) ||
> +        (config->has_sockets && config->sockets == 0) ||
> +        (config->has_dies && config->dies == 0) ||
> +        (config->has_cores && config->cores == 0) ||
> +        (config->has_threads && config->threads == 0) ||
> +        (config->has_maxcpus && config->maxcpus == 0)) {
> +        error_setg(errp, "CPU topology parameters must be greater than
> zero");
> +        goto out_free;
> +    }
> +
>      mc->smp_parse(ms, config, errp);
>      if (*errp) {
>          goto out_free;
> diff --git a/qapi/machine.json b/qapi/machine.json
> index c3210ee1fb2..9272cb3cf8b 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1288,8 +1288,8 @@
>  ##
>  # @SMPConfiguration:
>  #
> -# Schema for CPU topology configuration.  "0" or a missing value lets
> -# QEMU figure out a suitable value based on the ones that are provided.
> +# Schema for CPU topology configuration. A missing value lets QEMU
> +# figure out a suitable value based on the ones that are provided.
>  #
>  # @cpus: number of virtual CPUs in the virtual machine
>  #
> @@ -1297,7 +1297,7 @@
>  #
>  # @dies: number of dies per socket in the CPU topology
>  #
> -# @cores: number of cores per thread in the CPU topology
> +# @cores: number of cores per die in the CPU topology
>  #
>  # @threads: number of threads per core in the CPU topology
>  #
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 83aa59a920f..aee622f577d 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -227,11 +227,13 @@ SRST
>      of computing the CPU maximum count.
>
>      Either the initial CPU count, or at least one of the topology
> parameters
> -    must be specified. Values for any omitted parameters will be computed
> -    from those which are given. Historically preference was given to the
> -    coarsest topology parameters when computing missing values (ie sockets
> -    preferred over cores, which were preferred over threads), however,
> this
> -    behaviour is considered liable to change.
> +    must be specified. The specified parameters must be greater than zero,
> +    explicit configuration like "cpus=0" is not allowed. Values for any
> +    omitted parameters will be computed from those which are given.
> +    Historically preference was given to the coarsest topology parameters
> +    when computing missing values (ie sockets preferred over cores, which
> +    were preferred over threads), however, this behaviour is considered
> +    liable to change.
>  ERST
>
>  DEF("numa", HAS_ARG, QEMU_OPTION_numa,
> --
> 2.31.1
>
>

Andrew Jones Aug. 17, 2021, 12:02 p.m. UTC | #2

On Mon, Aug 16, 2021 at 11:37:21PM +0200, Paolo Bonzini wrote:
> How do we know that no one has ever used such configuration? The conversion
> was meant to be bug-compatible.

We don't. But we do know that a zero input value was never documented
prior to 1e63fe68580, which has not yet been released. Can we claim
that an undocumented input value has undefined behavior, giving us
freedom to modify that behavior until it is documented?

Thanks,
drew

> 
> Paolo
> 
> Il lun 16 ago 2021, 23:06 Eduardo Habkost <[e-mail]> ha scritto:
> 
> > From: Yanan Wang <[e-mail]>
> >
> > In the SMP configuration, we should either provide a topology
> > parameter with a reasonable value (greater than zero) or just
> > omit it and QEMU will compute the missing value. Users should
> > have never provided a configuration with parameters as zero
> > (e.g. -smp 8,sockets=0) which should be treated as invalid.
> >
> > But commit 1e63fe68580 (machine: pass QAPI struct to mc->smp_parse)
> > has added some doc which implied that 'anything=0' is valid and
> > has the same semantics as omitting a parameter.
> >
> > To avoid meaningless configurations possibly introduced by users
> > in the future and consequently a necessary deprecation process,
> > fix the doc and also add the corresponding sanity check.
> >
> > Fixes: 1e63fe68580 (machine: pass QAPI struct to mc->smp_parse)
> > Suggested-by: Andrew Jones <[e-mail]>
> > Signed-off-by: Yanan Wang <[e-mail]>
> > Reviewed-by: Daniel P. Berrange <[e-mail]>
> > Tested-by: Daniel P. Berrange <[e-mail]>
> > Reviewed-by: Andrew Jones <[e-mail]>
> > Reviewed-by: Cornelia Huck <[e-mail]>
> > Message-Id: <[e-mail]>
> > Signed-off-by: Eduardo Habkost <[e-mail]>
> > ---
> >  hw/core/machine.c | 14 ++++++++++++++
> >  qapi/machine.json |  6 +++---
> >  qemu-options.hx   | 12 +++++++-----
> >  3 files changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 54e040587dd..a7e119469aa 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -832,6 +832,20 @@ static void machine_set_smp(Object *obj, Visitor *v,
> > const char *name,
> >          return;
> >      }
> >
> > +    /*
> > +     * A specified topology parameter must be greater than zero,
> > +     * explicit configuration like "cpus=0" is not allowed.
> > +     */
> > +    if ((config->has_cpus && config->cpus == 0) ||
> > +        (config->has_sockets && config->sockets == 0) ||
> > +        (config->has_dies && config->dies == 0) ||
> > +        (config->has_cores && config->cores == 0) ||
> > +        (config->has_threads && config->threads == 0) ||
> > +        (config->has_maxcpus && config->maxcpus == 0)) {
> > +        error_setg(errp, "CPU topology parameters must be greater than
> > zero");
> > +        goto out_free;
> > +    }
> > +
> >      mc->smp_parse(ms, config, errp);
> >      if (*errp) {
> >          goto out_free;
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index c3210ee1fb2..9272cb3cf8b 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -1288,8 +1288,8 @@
> >  ##
> >  # @SMPConfiguration:
> >  #
> > -# Schema for CPU topology configuration.  "0" or a missing value lets
> > -# QEMU figure out a suitable value based on the ones that are provided.
> > +# Schema for CPU topology configuration. A missing value lets QEMU
> > +# figure out a suitable value based on the ones that are provided.
> >  #
> >  # @cpus: number of virtual CPUs in the virtual machine
> >  #
> > @@ -1297,7 +1297,7 @@
> >  #
> >  # @dies: number of dies per socket in the CPU topology
> >  #
> > -# @cores: number of cores per thread in the CPU topology
> > +# @cores: number of cores per die in the CPU topology
> >  #
> >  # @threads: number of threads per core in the CPU topology
> >  #
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 83aa59a920f..aee622f577d 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -227,11 +227,13 @@ SRST
> >      of computing the CPU maximum count.
> >
> >      Either the initial CPU count, or at least one of the topology
> > parameters
> > -    must be specified. Values for any omitted parameters will be computed
> > -    from those which are given. Historically preference was given to the
> > -    coarsest topology parameters when computing missing values (ie sockets
> > -    preferred over cores, which were preferred over threads), however,
> > this
> > -    behaviour is considered liable to change.
> > +    must be specified. The specified parameters must be greater than zero,
> > +    explicit configuration like "cpus=0" is not allowed. Values for any
> > +    omitted parameters will be computed from those which are given.
> > +    Historically preference was given to the coarsest topology parameters
> > +    when computing missing values (ie sockets preferred over cores, which
> > +    were preferred over threads), however, this behaviour is considered
> > +    liable to change.
> >  ERST
> >
> >  DEF("numa", HAS_ARG, QEMU_OPTION_numa,
> > --
> > 2.31.1
> >
> >

Peter Maydell Aug. 17, 2021, 12:06 p.m. UTC | #3

On Tue, 17 Aug 2021 at 13:02, Andrew Jones <[e-mail]> wrote:
>
> On Mon, Aug 16, 2021 at 11:37:21PM +0200, Paolo Bonzini wrote:
> > How do we know that no one has ever used such configuration? The conversion
> > was meant to be bug-compatible.
>
> We don't. But we do know that a zero input value was never documented
> prior to 1e63fe68580, which has not yet been released. Can we claim
> that an undocumented input value has undefined behavior, giving us
> freedom to modify that behavior until it is documented?

Dunno; I definitely don't want a behaviour-change patch at this
point in the release-cycle, though...

-- PMM

Andrew Jones Aug. 17, 2021, 12:22 p.m. UTC | #4

On Tue, Aug 17, 2021 at 01:06:19PM +0100, Peter Maydell wrote:
> On Tue, 17 Aug 2021 at 13:02, Andrew Jones <[e-mail]> wrote:
> >
> > On Mon, Aug 16, 2021 at 11:37:21PM +0200, Paolo Bonzini wrote:
> > > How do we know that no one has ever used such configuration? The conversion
> > > was meant to be bug-compatible.
> >
> > We don't. But we do know that a zero input value was never documented
> > prior to 1e63fe68580, which has not yet been released. Can we claim
> > that an undocumented input value has undefined behavior, giving us
> > freedom to modify that behavior until it is documented?
> 
> Dunno; I definitely don't want a behaviour-change patch at this
> point in the release-cycle, though...
>

Can we replace this patch with the following one for now? And then
discuss this further before committing to supporting a zero input?

Thanks,
drew

Author: Andrew Jones <[e-mail]>
Date:   Tue Aug 17 14:17:29 2021 +0200

    qapi/machine.json: Remove zero value reference from SMPConfiguration documentation
    
    Commit 1e63fe685804 ("machine: pass QAPI struct to mc->smp_parse")
    introduced documentation stating that a zero input value for an SMP
    parameter indicates that its value should be automatically configured.
    This is indeed how things work today, but we'd like to change that.
    Avoid documenting behaviors we want to leave undefined for the time
    being, giving us freedom to change it later.
    
    Fixes: 1e63fe685804 ("machine: pass QAPI struct to mc->smp_parse")
    Signed-off-by: Andrew Jones <[e-mail]>

diff --git a/qapi/machine.json b/qapi/machine.json
index c3210ee1fb24..157712f00614 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1288,7 +1288,7 @@
 ##
 # @SMPConfiguration:
 #
-# Schema for CPU topology configuration.  "0" or a missing value lets
+# Schema for CPU topology configuration.  A missing value lets
 # QEMU figure out a suitable value based on the ones that are provided.
 #
 # @cpus: number of virtual CPUs in the virtual machine

Peter Maydell Aug. 17, 2021, 12:37 p.m. UTC | #5

On Tue, 17 Aug 2021 at 13:22, Andrew Jones <[e-mail]> wrote:
>
> On Tue, Aug 17, 2021 at 01:06:19PM +0100, Peter Maydell wrote:
> > On Tue, 17 Aug 2021 at 13:02, Andrew Jones <[e-mail]> wrote:
> > >
> > > On Mon, Aug 16, 2021 at 11:37:21PM +0200, Paolo Bonzini wrote:
> > > > How do we know that no one has ever used such configuration? The conversion
> > > > was meant to be bug-compatible.
> > >
> > > We don't. But we do know that a zero input value was never documented
> > > prior to 1e63fe68580, which has not yet been released. Can we claim
> > > that an undocumented input value has undefined behavior, giving us
> > > freedom to modify that behavior until it is documented?
> >
> > Dunno; I definitely don't want a behaviour-change patch at this
> > point in the release-cycle, though...
> >
>
> Can we replace this patch with the following one for now? And then
> discuss this further before committing to supporting a zero input?

If you can agree on that and send out a patch and get it reviewed
within the next three hours or so...

-- PMM

Andrew Jones Aug. 17, 2021, 12:54 p.m. UTC | #6

On Tue, Aug 17, 2021 at 01:37:15PM +0100, Peter Maydell wrote:
> On Tue, 17 Aug 2021 at 13:22, Andrew Jones <[e-mail]> wrote:
> >
> > On Tue, Aug 17, 2021 at 01:06:19PM +0100, Peter Maydell wrote:
> > > On Tue, 17 Aug 2021 at 13:02, Andrew Jones <[e-mail]> wrote:
> > > >
> > > > On Mon, Aug 16, 2021 at 11:37:21PM +0200, Paolo Bonzini wrote:
> > > > > How do we know that no one has ever used such configuration? The conversion
> > > > > was meant to be bug-compatible.
> > > >
> > > > We don't. But we do know that a zero input value was never documented
> > > > prior to 1e63fe68580, which has not yet been released. Can we claim
> > > > that an undocumented input value has undefined behavior, giving us
> > > > freedom to modify that behavior until it is documented?
> > >
> > > Dunno; I definitely don't want a behaviour-change patch at this
> > > point in the release-cycle, though...
> > >
> >
> > Can we replace this patch with the following one for now? And then
> > discuss this further before committing to supporting a zero input?
> 
> If you can agree on that and send out a patch and get it reviewed
> within the next three hours or so...

I posted the patch. Agreement/disagreement can come in the form of
ack/nack. Hopefully in time...

Thanks,
drew

Yanan Wang Aug. 17, 2021, 1:29 p.m. UTC | #7

On 2021/8/17 20:22, Andrew Jones wrote:
> On Tue, Aug 17, 2021 at 01:06:19PM +0100, Peter Maydell wrote:
>> On Tue, 17 Aug 2021 at 13:02, Andrew Jones <[e-mail]> wrote:
>>> On Mon, Aug 16, 2021 at 11:37:21PM +0200, Paolo Bonzini wrote:
>>>> How do we know that no one has ever used such configuration? The conversion
>>>> was meant to be bug-compatible.
>>> We don't. But we do know that a zero input value was never documented
>>> prior to 1e63fe68580, which has not yet been released. Can we claim
>>> that an undocumented input value has undefined behavior, giving us
>>> freedom to modify that behavior until it is documented?
>> Dunno; I definitely don't want a behaviour-change patch at this
>> point in the release-cycle, though...
>>
> Can we replace this patch with the following one for now? And then
> discuss this further before committing to supporting a zero input?
Yeah, I think at least the doc needs to be fixed in 6.1.

Then we may think about deprecating this behavior since 6.2,
I have implemented the deprecation locally and it can be post
with the smp parsing improvement series.

Thanks,
Yanan
.
> Thanks,
> drew
>
> Author: Andrew Jones <[e-mail]>
> Date:   Tue Aug 17 14:17:29 2021 +0200
>
>      qapi/machine.json: Remove zero value reference from SMPConfiguration documentation
>      
>      Commit 1e63fe685804 ("machine: pass QAPI struct to mc->smp_parse")
>      introduced documentation stating that a zero input value for an SMP
>      parameter indicates that its value should be automatically configured.
>      This is indeed how things work today, but we'd like to change that.
>      Avoid documenting behaviors we want to leave undefined for the time
>      being, giving us freedom to change it later.
>      
>      Fixes: 1e63fe685804 ("machine: pass QAPI struct to mc->smp_parse")
>      Signed-off-by: Andrew Jones <[e-mail]>
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index c3210ee1fb24..157712f00614 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1288,7 +1288,7 @@
>   ##
>   # @SMPConfiguration:
>   #
> -# Schema for CPU topology configuration.  "0" or a missing value lets
> +# Schema for CPU topology configuration.  A missing value lets
>   # QEMU figure out a suitable value based on the ones that are provided.
>   #
>   # @cpus: number of virtual CPUs in the virtual machine
>
> .

Patch

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 54e040587dd..a7e119469aa 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -832,6 +832,20 @@  static void machine_set_smp(Object *obj, Visitor *v, const char *name,
         return;
     }
 
+    /*
+     * A specified topology parameter must be greater than zero,
+     * explicit configuration like "cpus=0" is not allowed.
+     */
+    if ((config->has_cpus && config->cpus == 0) ||
+        (config->has_sockets && config->sockets == 0) ||
+        (config->has_dies && config->dies == 0) ||
+        (config->has_cores && config->cores == 0) ||
+        (config->has_threads && config->threads == 0) ||
+        (config->has_maxcpus && config->maxcpus == 0)) {
+        error_setg(errp, "CPU topology parameters must be greater than zero");
+        goto out_free;
+    }
+
     mc->smp_parse(ms, config, errp);
     if (*errp) {
         goto out_free;
diff --git a/qapi/machine.json b/qapi/machine.json
index c3210ee1fb2..9272cb3cf8b 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1288,8 +1288,8 @@ 
 ##
 # @SMPConfiguration:
 #
-# Schema for CPU topology configuration.  "0" or a missing value lets
-# QEMU figure out a suitable value based on the ones that are provided.
+# Schema for CPU topology configuration. A missing value lets QEMU
+# figure out a suitable value based on the ones that are provided.
 #
 # @cpus: number of virtual CPUs in the virtual machine
 #
@@ -1297,7 +1297,7 @@ 
 #
 # @dies: number of dies per socket in the CPU topology
 #
-# @cores: number of cores per thread in the CPU topology
+# @cores: number of cores per die in the CPU topology
 #
 # @threads: number of threads per core in the CPU topology
 #
diff --git a/qemu-options.hx b/qemu-options.hx
index 83aa59a920f..aee622f577d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -227,11 +227,13 @@  SRST
     of computing the CPU maximum count.
 
     Either the initial CPU count, or at least one of the topology parameters
-    must be specified. Values for any omitted parameters will be computed
-    from those which are given. Historically preference was given to the
-    coarsest topology parameters when computing missing values (ie sockets
-    preferred over cores, which were preferred over threads), however, this
-    behaviour is considered liable to change.
+    must be specified. The specified parameters must be greater than zero,
+    explicit configuration like "cpus=0" is not allowed. Values for any
+    omitted parameters will be computed from those which are given.
+    Historically preference was given to the coarsest topology parameters
+    when computing missing values (ie sockets preferred over cores, which
+    were preferred over threads), however, this behaviour is considered
+    liable to change.
 ERST
 
 DEF("numa", HAS_ARG, QEMU_OPTION_numa,