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

[Thomas Huth <thuth@redhat.com>]

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.

  Thomas