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 <yangyic...@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 <m...@redhat.com>
>> Reviewed-by: Yanan Wang <wangyana...@huawei.com>
>> Tested-by: Yanan Wang <wangyana...@huawei.com>
>> Signed-off-by: Yicong Yang <yangyic...@hisilicon.com>
>> Message-Id: <20221229065513.55652-3-yangyic...@huawei.com>
>> Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <m...@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-01813de8e...@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-91714b740...@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,
>
> .
