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

[Yicong Yang]

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:
> 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.
> 

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,
> 
> .