[PATCH for-6.1 0/1] machine: Disallow specifying topology parameters as zero

[PATCH for-6.1 0/1] machine: Disallow specifying topology parameters as zero

[Yanan Wang]

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 in the smp parsers.

This patch originly comes form [1], and it was suggested that
this patch fixing the doc should be sent for 6.1 to avoid a
deprecation process in the future.

[1] https://lore.kernel.org/qemu-devel/YPWsThPiZa3mF+zp@redhat.com/

Yanan Wang (1):
  machine: Disallow specifying topology parameters as zero

 hw/core/machine.c | 30 ++++++++++++++++++++++--------
 hw/i386/pc.c      | 33 ++++++++++++++++++++++++---------
 qapi/machine.json |  6 +++---
 3 files changed; 49 insertions(+); 20 deletions(-)

--
2.19.1

[PATCH for-6.1 1/1] machine: Disallow specifying topology parameters as zero

[Yanan Wang]

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 in the smp parsers.

Reviewed-by: Andrew Jones <drjones@redhat.com>
Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/core/machine.c | 30 ++++++++++++++++++++++--------
 hw/i386/pc.c      | 33 ++++++++++++++++++++++++---------
 qapi/machine.json |  6 +++---
 3 files changed, 49 insertions(+), 20 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 775add0795..ada300542b 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -745,11 +745,24 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
 {
     unsigned cpus    = config->has_cpus ? config->cpus : 0;
     unsigned sockets = config->has_sockets ? config->sockets : 0;
+    unsigned dies    = config->has_dies ? config->dies : 0;
     unsigned cores   = config->has_cores ? config->cores : 0;
     unsigned threads = config->has_threads ? config->threads : 0;
+    unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
+
+    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, "parameters must be equal to or greater than one if provided");
+        return;
+    }
 
-    if (config->has_dies && config->dies != 0 && config->dies != 1) {
+    if (dies > 1) {
         error_setg(errp, "dies not supported by this machine's CPU topology");
+        return;
     }
 
     /* compute missing values, prefer sockets over cores over threads */
@@ -760,8 +773,8 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
             sockets = sockets > 0 ? sockets : 1;
             cpus = cores * threads * sockets;
         } else {
-            ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus;
-            sockets = ms->smp.max_cpus / (cores * threads);
+            maxcpus = maxcpus > 0 ? maxcpus : cpus;
+            sockets = maxcpus / (cores * threads);
         }
     } else if (cores == 0) {
         threads = threads > 0 ? threads : 1;
@@ -778,26 +791,27 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
         return;
     }
 
-    ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus;
+    maxcpus = maxcpus > 0 ? maxcpus : cpus;
 
-    if (ms->smp.max_cpus < cpus) {
+    if (maxcpus < cpus) {
         error_setg(errp, "maxcpus must be equal to or greater than smp");
         return;
     }
 
-    if (sockets * cores * threads != ms->smp.max_cpus) {
+    if (sockets * cores * threads != maxcpus) {
         error_setg(errp, "Invalid CPU topology: "
                    "sockets (%u) * cores (%u) * threads (%u) "
                    "!= maxcpus (%u)",
                    sockets, cores, threads,
-                   ms->smp.max_cpus);
+                   maxcpus);
         return;
     }
 
     ms->smp.cpus = cpus;
+    ms->smp.sockets = sockets;
     ms->smp.cores = cores;
     ms->smp.threads = threads;
-    ms->smp.sockets = sockets;
+    ms->smp.max_cpus = maxcpus;
 }
 
 static void machine_get_smp(Object *obj, Visitor *v, const char *name,
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c2b9d62a35..db93841097 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -716,9 +716,23 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
 {
     unsigned cpus    = config->has_cpus ? config->cpus : 0;
     unsigned sockets = config->has_sockets ? config->sockets : 0;
-    unsigned dies    = config->has_dies ? config->dies : 1;
+    unsigned dies    = config->has_dies ? config->dies : 0;
     unsigned cores   = config->has_cores ? config->cores : 0;
     unsigned threads = config->has_threads ? config->threads : 0;
+    unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
+
+    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, "parameters must be equal to or greater than one if provided");
+        return;
+    }
+
+    /* directly default dies to 1 if it's omitted */
+    dies = dies > 0 ? dies : 1;
 
     /* compute missing values, prefer sockets over cores over threads */
     if (cpus == 0 || sockets == 0) {
@@ -728,8 +742,8 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
             sockets = sockets > 0 ? sockets : 1;
             cpus = cores * threads * dies * sockets;
         } else {
-            ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus;
-            sockets = ms->smp.max_cpus / (cores * threads * dies);
+            maxcpus = maxcpus > 0 ? maxcpus : cpus;
+            sockets = maxcpus / (cores * threads * dies);
         }
     } else if (cores == 0) {
         threads = threads > 0 ? threads : 1;
@@ -746,27 +760,28 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
         return;
     }
 
-    ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus;
+    maxcpus = maxcpus > 0 ? maxcpus : cpus;
 
-    if (ms->smp.max_cpus < cpus) {
+    if (maxcpus < cpus) {
         error_setg(errp, "maxcpus must be equal to or greater than smp");
         return;
     }
 
-    if (sockets * dies * cores * threads != ms->smp.max_cpus) {
+    if (sockets * dies * cores * threads != maxcpus) {
         error_setg(errp, "Invalid CPU topology deprecated: "
                    "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
                    "!= maxcpus (%u)",
                    sockets, dies, cores, threads,
-                   ms->smp.max_cpus);
+                   maxcpus);
         return;
     }
 
     ms->smp.cpus = cpus;
-    ms->smp.cores = cores;
-    ms->smp.threads = threads;
     ms->smp.sockets = sockets;
     ms->smp.dies = dies;
+    ms->smp.cores = cores;
+    ms->smp.threads = threads;
+    ms->smp.max_cpus = maxcpus;
 }
 
 static
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
 #
-- 
2.19.1

Re: [PATCH for-6.1 0/1] machine: Disallow specifying topology parameters as zero

[Cornelia Huck]

On Thu, Jul 22 2021, Yanan Wang <wangyanan55@huawei.com> 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 in the smp parsers.

Are we expecting any real users to have used that 'parameter=0'
behaviour? I expect that libvirt and other management software already
did the right thing; unfortunately, sometimes weird configuration lines
tend to persist in search results.

>
> This patch originly comes form [1], and it was suggested that
> this patch fixing the doc should be sent for 6.1 to avoid a
> deprecation process in the future.
>
> [1] https://lore.kernel.org/qemu-devel/YPWsThPiZa3mF+zp@redhat.com/
>
> Yanan Wang (1):
>   machine: Disallow specifying topology parameters as zero
>
>  hw/core/machine.c | 30 ++++++++++++++++++++++--------
>  hw/i386/pc.c      | 33 ++++++++++++++++++++++++---------
>  qapi/machine.json |  6 +++---
>  3 files changed; 49 insertions(+); 20 deletions(-)

Re: [PATCH for-6.1 0/1] machine: Disallow specifying topology parameters as zero

[wangyanan (Y)]

On 2021/7/22 14:02, Cornelia Huck wrote:
> On Thu, Jul 22 2021, Yanan Wang <wangyanan55@huawei.com> 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 in the smp parsers.
> Are we expecting any real users to have used that 'parameter=0'
> behaviour? I expect that libvirt and other management software already
> did the right thing; unfortunately, sometimes weird configuration lines
> tend to persist in search results.
I think there may not any users who have already used a configuration
with explicit "parameter=0", instead it should have been just omitted.
Yes, libvirt now rejects "parameter=0" when parsing XML, but we now
still allows "parameter=0" in the direct QEMU cmdlines. If we hope to
disallow this kind of config thoroughly, we'd better also have a sanity
check in QEMU.

Thanks,
Yanan
>> This patch originly comes form [1], and it was suggested that
>> this patch fixing the doc should be sent for 6.1 to avoid a
>> deprecation process in the future.
>>
>> [1] https://lore.kernel.org/qemu-devel/YPWsThPiZa3mF+zp@redhat.com/
>>
>> Yanan Wang (1):
>>    machine: Disallow specifying topology parameters as zero
>>
>>   hw/core/machine.c | 30 ++++++++++++++++++++++--------
>>   hw/i386/pc.c      | 33 ++++++++++++++++++++++++---------
>>   qapi/machine.json |  6 +++---
>>   3 files changed; 49 insertions(+); 20 deletions(-)
> .

Re: [PATCH for-6.1 0/1] machine: Disallow specifying topology parameters as zero

[Andrew Jones]

On Thu, Jul 22, 2021 at 08:02:16AM +0200, Cornelia Huck wrote:
> On Thu, Jul 22 2021, Yanan Wang <wangyanan55@huawei.com> 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 in the smp parsers.
> 
> Are we expecting any real users to have used that 'parameter=0'
> behaviour? I expect that libvirt and other management software already
> did the right thing; unfortunately, sometimes weird configuration lines
> tend to persist in search results.

I understand this concern. I think the only documentation we had prior to
commit 1e63fe685804 was

DEF("smp", HAS_ARG, QEMU_OPTION_smp,
    "-smp [cpus=]n[,maxcpus=cpus][,cores=cores][,threads=threads][,dies=dies][,sockets=sockets]\n"
    "                set the number of CPUs to 'n' [default=1]\n"
    "                maxcpus= maximum number of total cpus, including\n"
    "                offline CPUs for hotplug, etc\n"
    "                cores= number of CPU cores on one socket (for PC, it's on one die)\n"
    "                threads= number of threads on one CPU core\n"
    "                dies= number of CPU dies on one socket (for PC only)\n"
    "                sockets= number of discrete sockets in the system\n",
        QEMU_ARCH_ALL)
SRST
``-smp [cpus=]n[,cores=cores][,threads=threads][,dies=dies][,sockets=sockets][,maxcpus=maxcpus]``
    Simulate an SMP system with n CPUs. On the PC target, up to 255 CPUs
    are supported. On Sparc32 target, Linux limits the number of usable
    CPUs to 4. For the PC target, the number of cores per die, the
    number of threads per cores, the number of dies per packages and the
    total number of sockets can be specified. Missing values will be
    computed. If any on the three values is given, the total number of
    CPUs n can be omitted. maxcpus specifies the maximum number of
    hotpluggable CPUs.
ERST

This doesn't mention zero inputs and even implies non-zero inputs.

I'm not sure if we need to worry about the odd command line that used zero
for some parameters. What do you think?

Thanks,
drew


> 
> >
> > This patch originly comes form [1], and it was suggested that
> > this patch fixing the doc should be sent for 6.1 to avoid a
> > deprecation process in the future.
> >
> > [1] https://lore.kernel.org/qemu-devel/YPWsThPiZa3mF+zp@redhat.com/
> >
> > Yanan Wang (1):
> >   machine: Disallow specifying topology parameters as zero
> >
> >  hw/core/machine.c | 30 ++++++++++++++++++++++--------
> >  hw/i386/pc.c      | 33 ++++++++++++++++++++++++---------
> >  qapi/machine.json |  6 +++---
> >  3 files changed; 49 insertions(+); 20 deletions(-)
> 

Re: [PATCH for-6.1 0/1] machine: Disallow specifying topology parameters as zero

[Cornelia Huck]

On Thu, Jul 22 2021, Andrew Jones <drjones@redhat.com> wrote:

> On Thu, Jul 22, 2021 at 08:02:16AM +0200, Cornelia Huck wrote:
>> On Thu, Jul 22 2021, Yanan Wang <wangyanan55@huawei.com> 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 in the smp parsers.
>> 
>> Are we expecting any real users to have used that 'parameter=0'
>> behaviour? I expect that libvirt and other management software already
>> did the right thing; unfortunately, sometimes weird configuration lines
>> tend to persist in search results.
>
> I understand this concern. I think the only documentation we had prior to
> commit 1e63fe685804 was
>
> DEF("smp", HAS_ARG, QEMU_OPTION_smp,
>     "-smp [cpus=]n[,maxcpus=cpus][,cores=cores][,threads=threads][,dies=dies][,sockets=sockets]\n"
>     "                set the number of CPUs to 'n' [default=1]\n"
>     "                maxcpus= maximum number of total cpus, including\n"
>     "                offline CPUs for hotplug, etc\n"
>     "                cores= number of CPU cores on one socket (for PC, it's on one die)\n"
>     "                threads= number of threads on one CPU core\n"
>     "                dies= number of CPU dies on one socket (for PC only)\n"
>     "                sockets= number of discrete sockets in the system\n",
>         QEMU_ARCH_ALL)
> SRST
> ``-smp [cpus=]n[,cores=cores][,threads=threads][,dies=dies][,sockets=sockets][,maxcpus=maxcpus]``
>     Simulate an SMP system with n CPUs. On the PC target, up to 255 CPUs
>     are supported. On Sparc32 target, Linux limits the number of usable
>     CPUs to 4. For the PC target, the number of cores per die, the
>     number of threads per cores, the number of dies per packages and the
>     total number of sockets can be specified. Missing values will be
>     computed. If any on the three values is given, the total number of
>     CPUs n can be omitted. maxcpus specifies the maximum number of
>     hotpluggable CPUs.
> ERST
>
> This doesn't mention zero inputs and even implies non-zero inputs.

Yes, hopefully that kept people away from using 0 magic, unless they
read the code.

>
> I'm not sure if we need to worry about the odd command line that used zero
> for some parameters. What do you think?

I did a cursory search for bad examples, and nothing popped up. So this
should be reasonably painless.

Re: [PATCH for-6.1 0/1] machine: Disallow specifying topology parameters as zero

[Paolo Bonzini]

On 22/07/21 15:37, Andrew Jones wrote:
> This doesn't mention zero inputs and even implies non-zero inputs.
> 
> I'm not sure if we need to worry about the odd command line that used zero
> for some parameters. What do you think?

I think I agree as well, however the patch that Yanan sent has 
unnecessary duplication between smp_parse and pc_smp_parse. 
machine_set_smp is a better place to implement this kind of check.

Paolo

Re: [PATCH for-6.1 0/1] machine: Disallow specifying topology parameters as zero

[wangyanan (Y)]

On 2021/7/22 21:55, Paolo Bonzini wrote:
> On 22/07/21 15:37, Andrew Jones wrote:
>> This doesn't mention zero inputs and even implies non-zero inputs.
>>
>> I'm not sure if we need to worry about the odd command line that used 
>> zero
>> for some parameters. What do you think?
>
> I think I agree as well, however the patch that Yanan sent has 
> unnecessary duplication between smp_parse and pc_smp_parse. 
> machine_set_smp is a better place to implement this kind of check.
>
The smp_parse and pc_smp_parse are going to be converted into a
generic parser, and the added sanity-check in this patch will also be
tested in an unit test. So is it probably better to keep the check in the
parser instead of the caller? The duplication will be eliminated anyway
when there is one single parser.

But I can also implement the check in machine_set_smp as you mentioned
if it's more reasonable and preferred. :)

Thanks,
Yanan
.

Re: [PATCH for-6.1 0/1] machine: Disallow specifying topology parameters as zero

[Paolo Bonzini]

On 22/07/21 16:12, wangyanan (Y) wrote:
> The smp_parse and pc_smp_parse are going to be converted into a
> generic parser, and the added sanity-check in this patch will also be
> tested in an unit test. So is it probably better to keep the check in the
> parser instead of the caller? The duplication will be eliminated anyway
> when there is one single parser.
> 
> But I can also implement the check in machine_set_smp as you mentioned
> if it's more reasonable and preferred. :)

Yes, I would prefer to avoid having duplicate code.  There are some 
common checks already in machine_set_smp, e.g. comparing ms->smp.cpus 
against mc->min_cpus and mc->max_cpus.

Paolo

Re: [PATCH for-6.1 0/1] machine: Disallow specifying topology parameters as zero

[wangyanan (Y)]

On 2021/7/22 22:38, Paolo Bonzini wrote:
> On 22/07/21 16:12, wangyanan (Y) wrote:
>> The smp_parse and pc_smp_parse are going to be converted into a
>> generic parser, and the added sanity-check in this patch will also be
>> tested in an unit test. So is it probably better to keep the check in 
>> the
>> parser instead of the caller? The duplication will be eliminated anyway
>> when there is one single parser.
>>
>> But I can also implement the check in machine_set_smp as you mentioned
>> if it's more reasonable and preferred. :)
>
> Yes, I would prefer to avoid having duplicate code.  There are some 
> common checks already in machine_set_smp, e.g. comparing ms->smp.cpus 
> against mc->min_cpus and mc->max_cpus.
>
>
Ok, I will send a v2.

Thanks,
Yanan
.