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

[Philippe Mathieu-Daudé]

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:
6E514B4B-9185-424E-832E-01813DE8E83F@gmail.com/">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,