Re: [RFC PATCH v2 2/4] hw/arm/virt: Parse -smp cluster parameter in virt_smp_parse

[Andrew Jones]

On Tue, Apr 13, 2021 at 04:31:45PM +0800, Yanan Wang wrote:
> There is a separate function virt_smp_parse() in hw/virt/arm.c used
> to parse cpu topology for the ARM machines. So add parsing of -smp
> cluster parameter in it, then total number of logical cpus will be
> calculated like: max_cpus = sockets * clusters * cores * threads.
> 
> In virt_smp_parse(), the computing logic of missing values prefers
> cores over sockets over threads. And for compatibility, the value
> of clusters will be set as default 1 if not explicitly specified.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/arm/virt.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 57ef961cb5..51797628db 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2639,35 +2639,38 @@ static void virt_smp_parse(MachineState *ms, QemuOpts 
> *opts)
>      if (opts) {
>          unsigned cpus = qemu_opt_get_number(opts, "cpus", 0);
>          unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
> +        unsigned clusters = qemu_opt_get_number(opts, "clusters", 1);
>          unsigned cores = qemu_opt_get_number(opts, "cores", 0);
>          unsigned threads = qemu_opt_get_number(opts, "threads", 0);
> +        VirtMachineState *vms = VIRT_MACHINE(ms);
>  
>          /*
> -         * Compute missing values; prefer cores over sockets and
> -         * sockets over threads.
> +         * Compute missing values; prefer cores over sockets and sockets
> +         * over threads. For compatibility, value of clusters will have
> +         * been set as default 1 if not explicitly specified.
>           */
>          if (cpus == 0 || cores == 0) {
>              sockets = sockets > 0 ? sockets : 1;
>              threads = threads > 0 ? threads : 1;
>              if (cpus == 0) {
>                  cores = cores > 0 ? cores : 1;
> -                cpus = cores * threads * sockets;
> +                cpus = sockets * clusters * cores * threads;
>              } else {
>                  ms->smp.max_cpus = qemu_opt_get_number(opts, "maxcpus", 
> cpus);
> -                cores = ms->smp.max_cpus / (sockets * threads);
> +                cores = ms->smp.max_cpus / (sockets * clusters * threads);
>              }
>          } else if (sockets == 0) {
>              threads = threads > 0 ? threads : 1;
> -            sockets = cpus / (cores * threads);
> +            sockets = cpus / (clusters * cores * threads);
>              sockets = sockets > 0 ? sockets : 1;

If we initialize clusters to zero instead of one and add lines in
'cpus == 0 || cores == 0' and 'sockets == 0' like
'clusters = clusters > 0 ? clusters : 1' as needed, then I think we can
add

 } else if (clusters == 0) {
     threads = threads > 0 ? threads : 1;
     clusters = cpus / (sockets * cores * thread);
     clusters = clusters > 0 ? clusters : 1;
 }

here.

>          } else if (threads == 0) {
> -            threads = cpus / (cores * sockets);
> +            threads = cpus / (sockets * clusters * cores);
>              threads = threads > 0 ? threads : 1;
> -        } else if (sockets * cores * threads < cpus) {
> +        } else if (sockets * clusters * cores * threads < cpus) {
>              error_report("cpu topology: "
> -                         "sockets (%u) * cores (%u) * threads (%u) < "
> -                         "smp_cpus (%u)",
> -                         sockets, cores, threads, cpus);
> +                         "sockets (%u) * clusters (%u) * cores (%u) * "
> +                         "threads (%u) < smp_cpus (%u)",
> +                         sockets, clusters, cores, threads, cpus);
>              exit(1);
>          }
>  
> @@ -2678,11 +2681,11 @@ static void virt_smp_parse(MachineState *ms, QemuOpts 
> *opts)
>              exit(1);
>          }
>  
> -        if (sockets * cores * threads != ms->smp.max_cpus) {
> +        if (sockets * clusters * cores * threads != ms->smp.max_cpus) {
>              error_report("cpu topology: "
> -                         "sockets (%u) * cores (%u) * threads (%u)"
> -                         "!= maxcpus (%u)",
> -                         sockets, cores, threads,
> +                         "sockets (%u) * clusters (%u) * cores (%u) * "
> +                         "threads (%u) != maxcpus (%u)",
> +                         sockets, clusters, cores, threads,
>                           ms->smp.max_cpus);
>              exit(1);
>          }
> @@ -2691,6 +2694,7 @@ static void virt_smp_parse(MachineState *ms, QemuOpts 
> *opts)
>          ms->smp.cores = cores;
>          ms->smp.threads = threads;
>          ms->smp.sockets = sockets;
> +        vms->smp_clusters = clusters;
>      }
>  
>      if (ms->smp.cpus > 1) {
> -- 
> 2.19.1
>

Thanks,
drew