Re: [PATCH v20 01/21] s390x/cpu topology: add s390 specifics to CPU topology

[Thomas Huth]

On 25/04/2023 18.14, Pierre Morel wrote:
> S390 adds two new SMP levels, drawers and books to the CPU
> topology.
> The S390 CPU have specific topology features like dedication
> and entitlement to give to the guest indications on the host
> vCPUs scheduling and help the guest take the best decisions
> on the scheduling of threads on the vCPUs.
>
> Let us provide the SMP properties with books and drawers levels
> and S390 CPU with dedication and entitlement,
>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
...> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index 2e267fa458..42a6a40333 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -342,3 +342,15 @@
>                      'TARGET_S390X',
>                      'TARGET_MIPS',
>                      'TARGET_LOONGARCH64' ] } }
> +
> +##
> +# @CpuS390Polarization:
> +#
> +# An enumeration of cpu polarization that can be assumed by a virtual
> +# S390 CPU
> +#
> +# Since: 8.1
> +##
> +{ 'enum': 'CpuS390Polarization',
> +  'prefix': 'S390_CPU_POLARIZATION',
> +  'data': [ 'horizontal', 'vertical' ] }

It seems like you don't need this here yet ... I think you likely could also 
introduce it in a later patch instead (patch 11 seems the first one that 
needs this?)

Also, would a " 'if': 'TARGET_S390X' " be possible here, too?

> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> index c3dab007da..77bee06304 100644
> --- a/hw/core/machine-smp.c
> +++ b/hw/core/machine-smp.c
> @@ -30,8 +30,19 @@ static char *cpu_hierarchy_to_string(MachineState *ms)
>   {
>       MachineClass *mc = MACHINE_GET_CLASS(ms);
>       GString *s = g_string_new(NULL);
> +    const char *multiply = " * ", *prefix = "";
>   
> -    g_string_append_printf(s, "sockets (%u)", ms->smp.sockets);
> +    if (mc->smp_props.drawers_supported) {
> +        g_string_append_printf(s, "drawers (%u)", ms->smp.drawers);
> +        prefix = multiply;

That "prefix" stuff looks complicated ... why don't you simply add the "*" 
at the end of the strings:

    g_string_append_printf(s, "drawers (%u) * ",
                           ms->smp.drawers);

?

> +    }
> +
> +    if (mc->smp_props.books_supported) {
> +        g_string_append_printf(s, "%sbooks (%u)", prefix, ms->smp.books);
> +        prefix = multiply;
> +    }
> +
> +    g_string_append_printf(s, "%ssockets (%u)", prefix, ms->smp.sockets);

... it's followed by "sockets" here anyway, so adding the " * " at the end 
of the strings above should be fine.

>   {
>       MachineClass *mc = MACHINE_GET_CLASS(ms);
>       unsigned cpus    = config->has_cpus ? config->cpus : 0;
> +    unsigned drawers = config->has_drawers ? config->drawers : 0;
> +    unsigned books   = config->has_books ? config->books : 0;
>       unsigned sockets = config->has_sockets ? config->sockets : 0;
>       unsigned dies    = config->has_dies ? config->dies : 0;
>       unsigned clusters = config->has_clusters ? config->clusters : 0;
> @@ -85,6 +98,8 @@ void machine_parse_smp_config(MachineState *ms,
>        * explicit configuration like "cpus=0" is not allowed.
>        */
>       if ((config->has_cpus && config->cpus == 0) ||
> +        (config->has_drawers && config->drawers == 0) ||
> +        (config->has_books && config->books == 0) ||
>           (config->has_sockets && config->sockets == 0) ||
>           (config->has_dies && config->dies == 0) ||
>           (config->has_clusters && config->clusters == 0) ||
> @@ -111,6 +126,19 @@ void machine_parse_smp_config(MachineState *ms,
>       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;
> +    }
> +    books = books > 0 ? books : 1;

Could be shortened to:  book = books ?: 1;

> +    if (!mc->smp_props.drawers_supported && drawers > 1) {
> +        error_setg(errp,
> +                   "drawers not supported by this machine's CPU topology");
> +        return;
> +    }
> +    drawers = drawers > 0 ? drawers : 1;

Could be shortened to:  drawers = drawers ?: 1;

>       /* compute missing values based on the provided ones */
>       if (cpus == 0 && maxcpus == 0) {
>           sockets = sockets > 0 ? sockets : 1;
> @@ -124,33 +152,41 @@ void machine_parse_smp_config(MachineState *ms,
>               if (sockets == 0) {
>                   cores = cores > 0 ? cores : 1;
>                   threads = threads > 0 ? threads : 1;
> -                sockets = maxcpus / (dies * clusters * cores * threads);
> +                sockets = maxcpus /
> +                          (drawers * books * dies * clusters * cores * 
> threads);
>               } else if (cores == 0) {
>                   threads = threads > 0 ? threads : 1;
> -                cores = maxcpus / (sockets * dies * clusters * threads);
> +                cores = maxcpus /
> +                        (drawers * books * sockets * dies * clusters * 
> threads);
>               }

(not very important, but I wonder whether we should maybe disallow 
"prefer_sockets" with drawers and books instead of updating the calculation 
here - since prefer_sockets should only occur on old machine types)

>           } else {
>               /* prefer cores over sockets since 6.2 */
>               if (cores == 0) {
>                   sockets = sockets > 0 ? sockets : 1;
>                   threads = threads > 0 ? threads : 1;
> -                cores = maxcpus / (sockets * dies * clusters * threads);
> +                cores = maxcpus /
> +                        (drawers * books * sockets * dies * clusters * 
> threads);
>               } else if (sockets == 0) {
>                   threads = threads > 0 ? threads : 1;
> -                sockets = maxcpus / (dies * clusters * cores * threads);
> +                sockets = maxcpus /
> +                          (drawers * books * dies * clusters * cores * 
> threads);
>               }
>           }

 Thomas