| Message ID | 20210723095731.4976-2-wangyanan55@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | machine: Disallow specifying topology parameters as zero | expand |
A gentle ping ... Will this patch be picked up soon for 6.1 or is there anything else to tweak? Thanks, Yanan On 2021/7/23 17:57, Yanan Wang wrote: > In the SMP configuration, we should either specify a topology > parameter with a reasonable value (equal to or greater than 1) > or just leave it omitted and QEMU will calculate its value. > Configurations which explicitly specify the topology parameters > as zero like "sockets=0" are meaningless, so disallow them. > > However, the commit 1e63fe685804d > (machine: pass QAPI struct to mc->smp_parse) has documented that > '0' has the same semantics as omitting a parameter in the qapi > comment for SMPConfiguration. So this patch fixes the doc and > also adds the corresponding sanity check. > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > Reviewed-by: Andrew Jones <drjones@redhat.com> > Reviewed-by: Daniel P. Berrange <berrange@redhat.com> > Tested-by: Daniel P. Berrange <berrange@redhat.com> > Suggested-by: Andrew Jones <drjones@redhat.com> > Signed-off-by: Yanan Wang <wangyanan55@huawei.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 775add0795..e1533dfc47 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -829,6 +829,20 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, > return; > } > > + /* > + * The CPU topology parameters must be specified greater than zero or > + * just omitted, 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 c3210ee1fb..9272cb3cf8 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 99ed5ec5f1..e077aa80bf 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -223,11 +223,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,
diff --git a/hw/core/machine.c b/hw/core/machine.c index 775add0795..e1533dfc47 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -829,6 +829,20 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, return; } + /* + * The CPU topology parameters must be specified greater than zero or + * just omitted, 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 c3210ee1fb..9272cb3cf8 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 99ed5ec5f1..e077aa80bf 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -223,11 +223,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,