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@xxxxxxxxxx>
> 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@xxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxx
> > Signed-off-by: Zhao Liu <zhao1.liu@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
