| Message ID | 20221031090523.34146-2-yangyicong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Only generate cluster node in PPTT when specified | expand |
On 2022/10/31 17:05, Yicong Yang 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 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? > > 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> > Signed-off-by: Yicong Yang <yangyicong@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-wangyanan55@huawei.com/ Thanks, Yanan
On 2022/10/31 19:17, wangyanan (Y) wrote: > > On 2022/10/31 17:05, Yicong Yang 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 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 <mst@redhat.com> >> Signed-off-by: Yicong Yang <yangyicong@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-wangyanan55@huawei.com/ > > Thanks, > Yanan > > .
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; + /* 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",