1. Patchew
  2. QEMU
  3. [PULL 00/51] virtio,pc,pci: features, cleanups, fixes
  4. View patch

[PULL 45/51] hw/acpi/aml-build: Only generate cluster node in PPTT when specified

Michael S. Tsirkin posted 51 patches 2 years, 11 months ago
Maintainers: Warner Losh <imp@bsdimp.com>, Kyle Evans <kevans@freebsd.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, "Daniel P. Berrangé" <berrange@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <ani@anisinha.ca>, Richard Henderson <richard.henderson@linaro.org>, Gerd Hoffmann <kraxel@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Yanan Wang <wangyanan55@huawei.com>, BALATON Zoltan <balaton@eik.bme.hu>, Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>, John Snow <jsnow@redhat.com>, Alberto Garcia <berto@igalia.com>, Corey Minyard <minyard@acm.org>, "Hervé Poussineau" <hpoussin@reactos.org>, Aurelien Jarno <aurelien@aurel32.net>, Pavel Pisa <pisa@cmp.felk.cvut.cz>, Vikram Garhwal <fnu.vikram@xilinx.com>, Jason Wang <jasowang@redhat.com>, Dmitry Fleytman <dmitry.fleytman@gmail.com>, Stefan Weil <sw@weilnetz.de>, Jiri Pirko <jiri@resnulli.us>, Sven Schnelle <svens@stackframe.org>, Keith Busch <kbusch@kernel.org>, Klaus Jensen <its@irrelevant.dk>, Huacai Chen <chenhuacai@kernel.org>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Helge Deller <deller@gmx.de>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Yoshinori Sato <ysato@users.sourceforge.jp>, Magnus Damm <magnus.damm@gmail.com>, Peter Maydell <peter.maydell@linaro.org>, Daniel Henrique Barboza <danielhb413@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, David Gibson <david@gibson.dropbear.id.au>, Greg Kurz <groug@kaod.org>, Yuval Shaia <yuval.shaia.ml@gmail.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Fam Zheng <fam@euphon.net>, Alex Williamson <alex.williamson@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Viresh Kumar <viresh.kumar@linaro.org>, "Gonglei (Arei)" <arei.gonglei@huawei.com>, Beniamino Galvani <b.galvani@gmail.com>, Laurent Vivier <laurent@vivier.eu>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Ben Widawsky <ben.widawsky@intel.com>, Jonathan Cameron <jonathan.cameron@huawei.com>, Alistair Francis <Alistair.Francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Palmer Dabbelt <palmer@dabbelt.com>, Xiaojuan Yang <yangxiaojuan@loongson.cn>, Song Gao <gaosong@loongson.cn>, Francisco Iglesias <francisco.iglesias@amd.com>, Andrey Smirnov <andrew.smirnov@gmail.com>, Paul Burton <paulburton@kernel.org>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Elena Ufimtseva <elena.ufimtseva@oracle.com>, Jagannathan Raman <jag.raman@oracle.com>, John G Johnson <john.g.johnson@oracle.com>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Riku Voipio <riku.voipio@iki.fi>, Michael Roth <michael.roth@amd.com>, Konstantin Kostiuk <kkostiuk@redhat.com>, Taylor Simpson <tsimpson@quicinc.com>, Alexander Bulekov <alxndr@bu.edu>, Bandan Das <bsd@redhat.com>, Darren Kenny <darren.kenny@oracle.com>, Qiuhao Li <Qiuhao.Li@outlook.com>
There is a newer version of this series
[PULL 45/51] hw/acpi/aml-build: Only generate cluster node in PPTT when specified
Posted by Michael S. Tsirkin 2 years, 11 months ago 
From: Yicong Yang <yangyicong@hisilicon.com>

Currently we'll always generate a cluster node no matter user has
specified '-smp clusters=X' or not. Cluster is an optional level
and will participant the building of Linux scheduling domains and
only appears on a few platforms. It's unncessary to always build
it when it cannot reflect the real topology on platforms having no
cluster implementation and to avoid affecting the linux scheduling
domains in the VM. So only generate the cluster topology in ACPI
PPTT when the user has specified it explicitly in -smp.

Tested qemu-system-aarch64 with `-smp 8` and linux 6.1-rc1, without
this patch:
estuary:/sys/devices/system/cpu/cpu0/topology$ cat cluster_*
ff	# cluster_cpus
0-7	# cluster_cpus_list
56	# cluster_id

with this patch:
estuary:/sys/devices/system/cpu/cpu0/topology$ cat cluster_*
ff	# cluster_cpus
0-7	# cluster_cpus_list
36	# cluster_id, with no cluster node kernel will make it to
	  physical package id

Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
Tested-by: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Message-Id: <20221229065513.55652-3-yangyicong@huawei.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/boards.h   | 3 +++
 hw/acpi/aml-build.c   | 2 +-
 hw/core/machine-smp.c | 2 ++
 qemu-options.hx       | 3 +++
 4 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index d18d6d0073..b0abbdd5dc 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -130,11 +130,14 @@ typedef struct {
  * @prefer_sockets - whether sockets are preferred over cores in smp parsing
  * @dies_supported - whether dies are supported by the machine
  * @clusters_supported - whether clusters are supported by the machine
+ * @has_clusters - whether clusters are explicitly specified in the user
+ *                 provided SMP configuration
  */
 typedef struct {
     bool prefer_sockets;
     bool dies_supported;
     bool clusters_supported;
+    bool has_clusters;
 } SMPCompatProps;
 
 /**
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 42feb4d4d7..ea331a20d1 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2030,7 +2030,7 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
                 0, socket_id, NULL, 0);
         }
 
-        if (mc->smp_props.clusters_supported) {
+        if (mc->smp_props.clusters_supported && mc->smp_props.has_clusters) {
             if (cpus->cpus[n].props.cluster_id != cluster_id) {
                 assert(cpus->cpus[n].props.cluster_id > cluster_id);
                 cluster_id = cpus->cpus[n].props.cluster_id;
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index b39ed21e65..c3dab007da 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -158,6 +158,8 @@ void machine_parse_smp_config(MachineState *ms,
     ms->smp.threads = threads;
     ms->smp.max_cpus = maxcpus;
 
+    mc->smp_props.has_clusters = config->has_clusters;
+
     /* sanity-check of the computed topology */
     if (sockets * dies * clusters * cores * threads != maxcpus) {
         g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
diff --git a/qemu-options.hx b/qemu-options.hx
index 7f99d15b23..8662568324 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -343,6 +343,9 @@ SRST
     ::
 
         -smp 2
+
+    Note: The cluster topology will only be generated in ACPI and exposed
+    to guest if it's explicitly specified in -smp.
 ERST
 
 DEF("numa", HAS_ARG, QEMU_OPTION_numa,
-- 
MST
Re: [PULL 45/51] hw/acpi/aml-build: Only generate cluster node in PPTT when specified
Posted by Philippe Mathieu-Daudé 2 years, 7 months ago 
Hi Yang,

On 5/1/23 10:16, Michael S. Tsirkin wrote:
> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> Currently we'll always generate a cluster node no matter user has
> specified '-smp clusters=X' or not. Cluster is an optional level
> and will participant the building of Linux scheduling domains and
> only appears on a few platforms. It's unncessary to always build
> it when it cannot reflect the real topology on platforms having no
> cluster implementation and to avoid affecting the linux scheduling
> domains in the VM. So only generate the cluster topology in ACPI
> PPTT when the user has specified it explicitly in -smp.
> 
> Tested qemu-system-aarch64 with `-smp 8` and linux 6.1-rc1, without
> this patch:
> estuary:/sys/devices/system/cpu/cpu0/topology$ cat cluster_*
> ff	# cluster_cpus
> 0-7	# cluster_cpus_list
> 56	# cluster_id
> 
> with this patch:
> estuary:/sys/devices/system/cpu/cpu0/topology$ cat cluster_*
> ff	# cluster_cpus
> 0-7	# cluster_cpus_list
> 36	# cluster_id, with no cluster node kernel will make it to
> 	  physical package id
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
> Tested-by: Yanan Wang <wangyanan55@huawei.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> Message-Id: <20221229065513.55652-3-yangyicong@huawei.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   include/hw/boards.h   | 3 +++
>   hw/acpi/aml-build.c   | 2 +-
>   hw/core/machine-smp.c | 2 ++
>   qemu-options.hx       | 3 +++
>   4 files changed, 9 insertions(+), 1 deletion(-)


> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 42feb4d4d7..ea331a20d1 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2030,7 +2030,7 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>                   0, socket_id, NULL, 0);
>           }
>   
> -        if (mc->smp_props.clusters_supported) {
> +        if (mc->smp_props.clusters_supported && mc->smp_props.has_clusters) {
>               if (cpus->cpus[n].props.cluster_id != cluster_id) {
>                   assert(cpus->cpus[n].props.cluster_id > cluster_id);
>                   cluster_id = cpus->cpus[n].props.cluster_id;
> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> index b39ed21e65..c3dab007da 100644
> --- a/hw/core/machine-smp.c
> +++ b/hw/core/machine-smp.c
> @@ -158,6 +158,8 @@ void machine_parse_smp_config(MachineState *ms,
>       ms->smp.threads = threads;
>       ms->smp.max_cpus = maxcpus;
>   
> +    mc->smp_props.has_clusters = config->has_clusters;

In another patch Bernhard noticed a QOM class field updated from
a QOM object, which is an anti-OOP pattern:
https://lore.kernel.org/qemu-devel/6E514B4B-9185-424E-832E-01813DE8E83F@gmail.com/

Looking at the codebase I noticed this is what you are doing here.
By doing so, updating the class field from this particular instance
results in all other instances being affected.

Currently this isn't really an issue because there are at most 2
MachineState instances (in a migration case, where we want the same
machine). However it would be nice to have this bad code example
cleaned. (Also eventually this could become an issue with heterogeneous
machines, but I'm not sure yet).

Do you mind sending a patch?

Thanks,

Phil.

>       /* sanity-check of the computed topology */
>       if (sockets * dies * clusters * cores * threads != maxcpus) {
>           g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 7f99d15b23..8662568324 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -343,6 +343,9 @@ SRST
>       ::
>   
>           -smp 2
> +
> +    Note: The cluster topology will only be generated in ACPI and exposed
> +    to guest if it's explicitly specified in -smp.
>   ERST
>   
>   DEF("numa", HAS_ARG, QEMU_OPTION_numa,
Re: [PULL 45/51] hw/acpi/aml-build: Only generate cluster node in PPTT when specified
Posted by Yicong Yang via 2 years, 7 months ago 
Hi Philippe,

On 2023/5/10 18:13, Philippe Mathieu-Daudé wrote:
> Hi Yang,
> 
> On 5/1/23 10:16, Michael S. Tsirkin wrote:
>> From: Yicong Yang <yangyicong@hisilicon.com>
>>
>> Currently we'll always generate a cluster node no matter user has
>> specified '-smp clusters=X' or not. Cluster is an optional level
>> and will participant the building of Linux scheduling domains and
>> only appears on a few platforms. It's unncessary to always build
>> it when it cannot reflect the real topology on platforms having no
>> cluster implementation and to avoid affecting the linux scheduling
>> domains in the VM. So only generate the cluster topology in ACPI
>> PPTT when the user has specified it explicitly in -smp.
>>
>> Tested qemu-system-aarch64 with `-smp 8` and linux 6.1-rc1, without
>> this patch:
>> estuary:/sys/devices/system/cpu/cpu0/topology$ cat cluster_*
>> ff    # cluster_cpus
>> 0-7    # cluster_cpus_list
>> 56    # cluster_id
>>
>> with this patch:
>> estuary:/sys/devices/system/cpu/cpu0/topology$ cat cluster_*
>> ff    # cluster_cpus
>> 0-7    # cluster_cpus_list
>> 36    # cluster_id, with no cluster node kernel will make it to
>>       physical package id
>>
>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
>> Tested-by: Yanan Wang <wangyanan55@huawei.com>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> Message-Id: <20221229065513.55652-3-yangyicong@huawei.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>   include/hw/boards.h   | 3 +++
>>   hw/acpi/aml-build.c   | 2 +-
>>   hw/core/machine-smp.c | 2 ++
>>   qemu-options.hx       | 3 +++
>>   4 files changed, 9 insertions(+), 1 deletion(-)
> 
> 
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 42feb4d4d7..ea331a20d1 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -2030,7 +2030,7 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>>                   0, socket_id, NULL, 0);
>>           }
>>   -        if (mc->smp_props.clusters_supported) {
>> +        if (mc->smp_props.clusters_supported && mc->smp_props.has_clusters) {
>>               if (cpus->cpus[n].props.cluster_id != cluster_id) {
>>                   assert(cpus->cpus[n].props.cluster_id > cluster_id);
>>                   cluster_id = cpus->cpus[n].props.cluster_id;
>> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
>> index b39ed21e65..c3dab007da 100644
>> --- a/hw/core/machine-smp.c
>> +++ b/hw/core/machine-smp.c
>> @@ -158,6 +158,8 @@ void machine_parse_smp_config(MachineState *ms,
>>       ms->smp.threads = threads;
>>       ms->smp.max_cpus = maxcpus;
>>   +    mc->smp_props.has_clusters = config->has_clusters;
> 
> In another patch Bernhard noticed a QOM class field updated from
> a QOM object, which is an anti-OOP pattern:
> https://lore.kernel.org/qemu-devel/6E514B4B-9185-424E-832E-01813DE8E83F@gmail.com/
> 
> Looking at the codebase I noticed this is what you are doing here.
> By doing so, updating the class field from this particular instance
> results in all other instances being affected.
> 

Is it suggested to move this to machinestat so we won't touch the machineclass members
in the initialization here? In the initial version the has_clusters is placed in the
ms->smp structure but it's suggested that's not a good place, see the discussion [1]

[1] https://lore.kernel.org/qemu-devel/6fa8a6ca-765a-8a55-76fb-91714b740fe7@huawei.com/

Thanks.

> Currently this isn't really an issue because there are at most 2
> MachineState instances (in a migration case, where we want the same
> machine). However it would be nice to have this bad code example
> cleaned. (Also eventually this could become an issue with heterogeneous
> machines, but I'm not sure yet).
> 
> Do you mind sending a patch?
> 
> Thanks,
> 
> Phil.
> 
>>       /* sanity-check of the computed topology */
>>       if (sockets * dies * clusters * cores * threads != maxcpus) {
>>           g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 7f99d15b23..8662568324 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -343,6 +343,9 @@ SRST
>>       ::
>>             -smp 2
>> +
>> +    Note: The cluster topology will only be generated in ACPI and exposed
>> +    to guest if it's explicitly specified in -smp.
>>   ERST
>>     DEF("numa", HAS_ARG, QEMU_OPTION_numa,
> 
> .

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.