On 2022/10/31 19:17, wangyanan (Y) wrote:
>
> On 2022/10/31 17:05, Yicong Yang 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 which cannot reflect the real topology on platforms have no
>> cluster and to avoid affecting the linux scheduling domains in the
>> VM.
> This sentence is a bit confusing, better to re-organize it.
>> So only generate it when user specified explicitly.
> "So only generate the cluster topology in ACPI PPTT when
> the user has specified it explicitly in -smp."
> Will this be more clear?
ok. seems better.
will also address the comments below.
thanks.
>>
>> 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>
>> Signed-off-by: Yicong Yang <yangyic...@hisilicon.com>
>> ---
>> hw/acpi/aml-build.c | 2 +-
>> hw/core/machine-smp.c | 3 +++
>> include/hw/boards.h | 3 +++
>> qemu-options.hx | 3 +++
>> tests/qtest/bios-tables-test-allowed-diff.h | 1 +
>> 5 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index e6bfac95c7..60c1acf3da 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..9c8950b2b0 100644
>> --- a/hw/core/machine-smp.c
>> +++ b/hw/core/machine-smp.c
>> @@ -158,6 +158,9 @@ void machine_parse_smp_config(MachineState *ms,
>> ms->smp.threads = threads;
>> ms->smp.max_cpus = maxcpus;
>> + if (config->has_clusters)
>> + mc->smp_props.has_clusters = true;
>> +
> why not "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/include/hw/boards.h b/include/hw/boards.h
>> index 311ed17e18..06ed66453f 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/qemu-options.hx b/qemu-options.hx
>> index eb38e5dc40..bbdbdef0af 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -349,6 +349,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,
>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h
>> b/tests/qtest/bios-tables-test-allowed-diff.h
>> index dfb8523c8b..cb143a55a6 100644
>> --- a/tests/qtest/bios-tables-test-allowed-diff.h
>> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
>> @@ -1 +1,2 @@
>> /* List of comma-separated changed AML files to ignore */
>> +"tests/data/acpi/virt/PPTT",
> This change in bios-tables-test-allowed-diff.h should be in a
> standalone patch before this patch.
>
> For your reference: see patch 4-6 in [1]:
> https://patchew.org/QEMU/20220107083232.16256-1-wangyana...@huawei.com/
>
> Thanks,
> Yanan
>
> .
