From: wangyanan (Y)

Hi Salil,

On 2021/5/17 23:17, Salil Mehta wrote:

...

...

From: Qemu-devel [mailto:qemu-devel-bounces+salil.mehta=huawei.com@nongnu.org] On Behalf Of Yanan Wang Sent: Sunday, May 16, 2021 11:32 AM To: Peter Maydell <peter.maydell@linaro.org>; Paolo Bonzini <pbonzini@redhat.com>; Andrew Jones <drjones@redhat.com>; Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov <imammedo@redhat.com>; Shannon Zhao <shannon.zhaosl@gmail.com>; qemu-devel@nongnu.org; qemu-arm@nongnu.org Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; wangyanan (Y) <wangyanan55@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>; yuzenghui <yuzenghui@huawei.com>; yangyicong <yangyicong@huawei.com>; zhukeqian <zhukeqian1@huawei.com> Subject: [RFC PATCH v3 4/4] hw/arm/virt: Parse -smp cluster parameter in virt_smp_parse

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.

Note, we will assume multi-cluster in one socket is not supported and default the value of clusters to 1, if it's not explicitly specified in -smp cmdline.

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 7de822e491..678d5ef36c 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -2642,8 +2642,8 @@ static int virt_kvm_type(MachineState *ms, const char *type_str) * with the -smp cmdlines when parsing them. * * We require that at least one of cpus or maxcpus must be provided. - * Threads will default to 1 if not provided. Sockets and cores must - * be either both provided or both not. + * Clusters and threads will default to 1 if they are not provided. + * Sockets and cores must be either both provided or both not. * * Note, if neither sockets nor cores are specified, we will calculate * all the missing values just like smp_parse() does, but will disable @@ -2652,15 +2652,18 @@ static int virt_kvm_type(MachineState *ms, const char *type_str) static void virt_smp_parse(MachineState *ms, QemuOpts *opts) { VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(ms); + VirtMachineState *vms = VIRT_MACHINE(ms);

if (opts) { unsigned cpus = qemu_opt_get_number(opts, "cpus", 0); unsigned maxcpus = qemu_opt_get_number(opts, "maxcpus", 0); unsigned sockets = qemu_opt_get_number(opts, "sockets", 0); + unsigned clusters = qemu_opt_get_number(opts, "clusters", 0); unsigned cores = qemu_opt_get_number(opts, "cores", 0); unsigned threads = qemu_opt_get_number(opts, "threads", 0);

- /* Default threads to 1 if not provided */ + /* Default clusters and threads to 1 if not provided */ + clusters = clusters > 0 ? clusters : 1; threads = threads > 0 ? threads : 1;

if (cpus == 0 && maxcpus == 0) { @@ -2676,13 +2679,13 @@ static void virt_smp_parse(MachineState *ms, QemuOpts *opts) cores = 1; if (cpus == 0) { sockets = 1; - cpus = sockets * cores * threads; + cpus = sockets * clusters * cores * threads; } else { maxcpus = maxcpus > 0 ? maxcpus : cpus; - sockets = maxcpus / (cores * threads); + sockets = maxcpus / (clusters * cores * threads); } } else if (sockets > 0 && cores > 0) { - cpus = cpus > 0 ? cpus : sockets * cores * threads; + cpus = cpus > 0 ? cpus : sockets * clusters * cores * threads; maxcpus = maxcpus > 0 ? maxcpus : cpus; } else { error_report("sockets and cores must be both provided " @@ -2695,25 +2698,26 @@ static void virt_smp_parse(MachineState *ms, QemuOpts *opts) exit(1); }

- if (sockets * cores * threads < cpus) { + 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); }

- if (sockets * cores * threads != maxcpus) { + if (sockets * clusters * cores * threads != maxcpus) { error_report("cpu topology: " - "sockets (%u) * cores (%u) * threads (%u) " - "!= maxcpus (%u)", - sockets, cores, threads, maxcpus); + "sockets (%u) * clusters (%u) * cores (%u) * " + "threads (%u) != maxcpus (%u)", + sockets, clusters, cores, threads, maxcpus); exit(1); }

ms->smp.cpus = cpus; ms->smp.max_cpus = maxcpus; ms->smp.sockets = sockets; + vms->smp_clusters = clusters;

This variable naming *smp_clusters* looks out-of-sorts. I thought a similar variable *smp_cpus* was destined to be removed for the reason given in below link - a patch by Andrew Jones?

Link: https://lists.gnu.org/archive/html/qemu-arm/2020-12/msg00418.html

Am I missing anything here? The smp_clusters is added in VirtMachineState and nowhere else because it's currently only used for ARM. But I think maybe I should also move it to CpuTopology structure like [1] is doing to move dies to CpuTopology.

yes, that’s the idea. It is always good to have right place holders so that the code comprehension/usage(in this case) becomes easy and obvious.

Move clusters to CpuTopology won't affect other architectures that don't support it yet, and will also make it easy if they want to in the future.

[1] From Paolo: https://patchwork.kernel.org/project/qemu-devel/patch/20210513162901.131023 9-10-pbonzini@redhat.com/

sure.

Thanks, Yanan

...

Salil.

...

ms->smp.cores = cores; ms->smp.threads = threads; }