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.