Re: [PATCH 02/14] hw/core/machine-smp: Deprecate unsupported "parameter=1" SMP configurations

[Zhao Liu <zhao1.liu@linux.intel.com>]

On Thu, Mar 07, 2024 at 07:22:10AM +0100, Thomas Huth wrote:
> Date: Thu, 7 Mar 2024 07:22:10 +0100
> From: Thomas Huth <thuth@redhat.com>
> Subject: Re: [PATCH 02/14] hw/core/machine-smp: Deprecate unsupported
>  "parameter=1" SMP configurations
> 
> On 06/03/2024 10.53, Zhao Liu wrote:
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > Currentlt, it was allowed for users to specify the unsupported
> 
> s/Currentlt/Currently/
> 
> > topology parameter as "1". For example, x86 PC machine doesn't
> > support drawer/book/cluster topology levels, but user could specify
> > "-smp drawers=1,books=1,clusters=1".
> > 
> > This is meaningless and confusing, so that the support for this kind of
> > configurations is marked depresated since 9.0. And report warning
> 
> s/depresated/deprecated/
> 
> > message for such case like:
> > 
> > qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
> >                      Unsupported clusters parameter mustn't be specified as 1
> > qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
> >                      Unsupported books parameter mustn't be specified as 1
> > qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
> >                      Unsupported drawers parameter mustn't be specified as 1
> > 
> > Users have to ensure that all the topology members described with -smp
> > are supported by the target machine.
> > 
> > Cc: devel@lists.libvirt.org
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >   docs/about/deprecated.rst | 14 +++++++++
> >   hw/core/machine-smp.c     | 63 +++++++++++++++++++++++++++++----------
> >   2 files changed, 61 insertions(+), 16 deletions(-)
> > 
> > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> > index 872974640252..2e782e83e952 100644
> > --- a/docs/about/deprecated.rst
> > +++ b/docs/about/deprecated.rst
> > @@ -47,6 +47,20 @@ as short-form boolean values, and passed to plugins as ``arg_name=on``.
> >   However, short-form booleans are deprecated and full explicit ``arg_name=on``
> >   form is preferred.
> > +``-smp`` (Unsopported "parameter=1" SMP configurations) (since 9.0)
> 
> s/Unsopported/Unsupported/
> 
> > +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> > +
> > +Specified CPU topology parameters must be supported by the machine.
> > +
> > +In the SMP configuration, users should provide the CPU topology parameters that
> > +are supported by the target machine.
> > +
> > +However, historically it was allowed for users to specify the unsupported
> > +topology parameter as "1", which is meaningless. So support for this kind of
> > +configurations (e.g. -smp drawers=1,books=1,clusters=1 for x86 PC machine) is
> > +marked depresated since 9.0, users have to ensure that all the topology members
> 
> s/depresated/deprecated/
> 
> > +described with -smp are supported by the target machine.
> > +
> >   QEMU Machine Protocol (QMP) commands
> >   ------------------------------------
> > diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> > index 96533886b14e..50a5a40dbc3d 100644
> > --- a/hw/core/machine-smp.c
> > +++ b/hw/core/machine-smp.c
> > @@ -112,30 +112,61 @@ void machine_parse_smp_config(MachineState *ms,
> >       /*
> >        * If not supported by the machine, a topology parameter must be
> > -     * omitted or specified equal to 1.
> > +     * omitted.
> >        */
> > -    if (!mc->smp_props.dies_supported && dies > 1) {
> > -        error_setg(errp, "dies not supported by this machine's CPU topology");
> > -        return;
> > -    }
> > -    if (!mc->smp_props.clusters_supported && clusters > 1) {
> > -        error_setg(errp, "clusters not supported by this machine's CPU topology");
> > -        return;
> > +    if (!mc->smp_props.clusters_supported && config->has_clusters) {
> > +        if (config->clusters > 1) {
> > +            error_setg(errp, "clusters not supported by this "
> > +                       "machine's CPU topology");
> > +            return;
> > +        } else {
> > +            /* Here clusters only equals 1 since we've checked zero case. */
> > +            warn_report("Deprecated CPU topology (considered invalid): "
> > +                        "Unsupported clusters parameter mustn't be "
> > +                        "specified as 1");
> > +        }
> >       }
> > +    clusters = clusters > 0 ? clusters : 1;
> > +    if (!mc->smp_props.dies_supported && config->has_dies) {
> > +        if (config->dies > 1) {
> > +            error_setg(errp, "dies not supported by this "
> > +                       "machine's CPU topology");
> > +            return;
> > +        } else {
> > +            /* Here dies only equals 1 since we've checked zero case. */
> > +            warn_report("Deprecated CPU topology (considered invalid): "
> > +                        "Unsupported dies parameter mustn't be "
> > +                        "specified as 1");
> > +        }
> > +    }
> >       dies = dies > 0 ? dies : 1;
> > -    clusters = clusters > 0 ? clusters : 1;
> > -    if (!mc->smp_props.books_supported && books > 1) {
> > -        error_setg(errp, "books not supported by this machine's CPU topology");
> > -        return;
> > +    if (!mc->smp_props.books_supported && config->has_books) {
> > +        if (config->books > 1) {
> > +            error_setg(errp, "books not supported by this "
> > +                       "machine's CPU topology");
> > +            return;
> > +        } else {
> > +            /* Here books only equals 1 since we've checked zero case. */
> > +            warn_report("Deprecated CPU topology (considered invalid): "
> > +                        "Unsupported books parameter mustn't be "
> > +                        "specified as 1");
> > +        }
> >       }
> >       books = books > 0 ? books : 1;
> > -    if (!mc->smp_props.drawers_supported && drawers > 1) {
> > -        error_setg(errp,
> > -                   "drawers not supported by this machine's CPU topology");
> > -        return;
> > +    if (!mc->smp_props.drawers_supported && config->has_drawers) {
> > +        if (config->drawers > 1) {
> > +            error_setg(errp, "drawers not supported by this "
> > +                       "machine's CPU topology");
> > +            return;
> > +        } else {
> > +            /* Here drawers only equals 1 since we've checked zero case. */
> > +            warn_report("Deprecated CPU topology (considered invalid): "
> > +                        "Unsupported drawers parameter mustn't be "
> > +                        "specified as 1");
> > +        }
> >       }
> >       drawers = drawers > 0 ? drawers : 1;
> 
> Apart from the typos, patch looks fine. I recommend to run "checkpath.pl"
> with the --codespell parameter, that helps to avoid those.
>

Oops...I didn't realize there were so many typos.

Maybe I'm relying too much on --codespell ;-), and these typos aren't in
the default dictionary used by --codespell so I didn't check them
before.

I'll refresh a new version (at Friday) for these typos.

Thanks,
Zhao