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

[Andrew Jones]

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 <ehabkost@redhat.com> ha scritto:
> 
> > From: Yanan Wang <wangyanan55@huawei.com>
> >
> > 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 <drjones@redhat.com>
> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> > Tested-by: Daniel P. Berrange <berrange@redhat.com>
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > Message-Id: <20210816024522.143124-2-wangyanan55@huawei.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  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
> >
> >