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

[Andrew Jones <drjones@redhat.com>]

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(-)
>