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

Date: Thu, 7 Mar 2024 07:22:10 +0100 From: Thomas Huth <thuth(a)redhat.com&gt; 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(a)intel.com&gt; > > 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(a)lists.libvirt.org > Signed-off-by: Zhao Liu <zhao1.liu(a)intel.com&gt; > --- > 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.