[PATCH v2 0/4] Only generate cluster node in PPTT when specified

[PATCH v2 0/4] Only generate cluster node in PPTT when specified

[Yicong Yang via]

From: Yicong Yang <yangyicong@hisilicon.com>

This series mainly change the policy for building a cluster topology node
in PPTT. Previously we'll always build a cluster node in PPTT without
asking the user, after this set the cluster node will be built only the
the user specify through "-smp clusters=X".

One problem is related to this but not fully caused by this, see the
discussion in [*]. When booting the VM with `-smp 8` and 4 numa nodes,
the linux scheduling domains in the VM misses the NUMA domains. It's
because the MC level span extends to Cluster level (which is generated
by the Qemu by default) that spans all the cpus in the system, then the
scheduling domain building stops at MC level since it already includes all
the cpus.

Considering cluster is an optional level and most platforms don't have it,
they may even don't realize this is built and a always build policy cannot
emulate the topology on these platforms. So in this series improve the
policy to only generate cluster when the user explicitly want.

Update the tests and test tables accordingly.

[*] https://lore.kernel.org/lkml/2c079860-ee82-7719-d3d2-756192f41704@huawei.com/

Change since v1:
- Only includes the test tables which is really needed
- Enrich the commit
Link: https://lore.kernel.org/qemu-devel/20220922131143.58003-1-yangyicong@huawei.com/

Yicong Yang (4):
  hw/acpi/aml-build: Only generate cluster node in PPTT when specified
  tests: virt: update expected ACPI tables for virt test
  tests: acpi: aarch64: add topology test for aarch64
  tests: acpi: aarch64: add *.topology tables

 hw/acpi/aml-build.c                |   2 +-
 hw/core/machine-smp.c              |   3 +++
 include/hw/boards.h                |   2 ++
 qemu-options.hx                    |   2 ++
 tests/data/acpi/virt/APIC.topology | Bin 0 -> 700 bytes
 tests/data/acpi/virt/DSDT.topology | Bin 0 -> 5398 bytes
 tests/data/acpi/virt/PPTT          | Bin 96 -> 76 bytes
 tests/data/acpi/virt/PPTT.topology | Bin 0 -> 336 bytes
 tests/qtest/bios-tables-test.c     |  22 ++++++++++++++++++++++
 9 files changed, 30 insertions(+), 1 deletion(-)
 create mode 100644 tests/data/acpi/virt/APIC.topology
 create mode 100644 tests/data/acpi/virt/DSDT.topology
 create mode 100644 tests/data/acpi/virt/PPTT.topology

-- 
2.24.0

[PATCH v2 1/4] hw/acpi/aml-build: Only generate cluster node in PPTT when specified

[Yicong Yang via]

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. So only generate it when user specified explicitly.

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

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 hw/acpi/aml-build.c   | 2 +-
 hw/core/machine-smp.c | 3 +++
 include/hw/boards.h   | 2 ++
 qemu-options.hx       | 2 ++
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index e6bfac95c7..aab73af66d 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 && ms->smp.build_cluster) {
             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..5d37e8d07a 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)
+        ms->smp.build_cluster = 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..c53f047b90 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -305,6 +305,7 @@ typedef struct DeviceMemoryState {
  * @cores: the number of cores in one cluster
  * @threads: the number of threads in one core
  * @max_cpus: the maximum number of logical processors on the machine
+ * @build_cluster: build cluster topology or not
  */
 typedef struct CpuTopology {
     unsigned int cpus;
@@ -314,6 +315,7 @@ typedef struct CpuTopology {
     unsigned int cores;
     unsigned int threads;
     unsigned int max_cpus;
+    bool build_cluster;
 } CpuTopology;
 
 /**
diff --git a/qemu-options.hx b/qemu-options.hx
index eb38e5dc40..0a710e7be3 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -342,6 +342,8 @@ SRST
     were preferred over threads), however, this behaviour is considered
     liable to change. Prior to 6.2 the preference was sockets over cores
     over threads. Since 6.2 the preference is cores over sockets over threads.
+    The cluster topology will only be generated if explicitly specified
+    by the "-cluster" option.
 
     For example, the following option defines a machine board with 2 sockets
     of 1 core before 6.2 and 1 socket of 2 cores after 6.2:
-- 
2.24.0

[PATCH v2 2/4] tests: virt: update expected ACPI tables for virt test

[Yicong Yang via]

From: Yicong Yang <yangyicong@hisilicon.com>

Update the ACPI tables according to the acpi aml_build change.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 tests/data/acpi/virt/PPTT | Bin 96 -> 76 bytes
 1 file changed, 0 insertions(+), 0 deletions(-)

diff --git a/tests/data/acpi/virt/PPTT b/tests/data/acpi/virt/PPTT
index f56ea63b369a604877374ad696c396e796ab1c83..7a1258ecf123555b24462c98ccbb76b4ac1d0c2b 100644
GIT binary patch
delta 32
fcmYfB;R*-{3GrcIU|?D?k;`ae01J-_kOKn%ZFdCM

delta 53
pcmeZC;0g!`2}xjJU|{l?$YrDgWH5jU5Ca567#O&Klm(arApowi1QY-O

-- 
2.24.0

[PATCH v2 3/4] tests: acpi: aarch64: add topology test for aarch64

[Yicong Yang via]

From: Yicong Yang <yangyicong@hisilicon.com>

Add test for aarch64's ACPI topology building for all the supported
levels.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 tests/qtest/bios-tables-test.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index e6096e7f73..099b723444 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -1533,6 +1533,27 @@ static void test_acpi_virt_tcg(void)
     free_test_data(&data);
 }
 
+static void test_acpi_virt_tcg_topology(void)
+{
+    test_data data = {
+        .machine = "virt",
+        .variant = ".topology",
+        .tcg_only = true,
+        .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
+        .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
+        .cd = "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2",
+        .ram_start = 0x40000000ULL,
+        .scan_len = 128ULL * 1024 * 1024,
+    };
+
+    data.smbios_cpu_max_speed = 2900;
+    data.smbios_cpu_curr_speed = 2700;
+    test_acpi_one("-cpu cortex-a57 "
+                  "-smbios type=4,max-speed=2900,current-speed=2700 "
+                  "-smp sockets=1,clusters=2,cores=2,threads=2", &data);
+    free_test_data(&data);
+}
+
 static void test_acpi_q35_viot(void)
 {
     test_data data = {
@@ -1864,6 +1885,7 @@ int main(int argc, char *argv[])
     } else if (strcmp(arch, "aarch64") == 0) {
         if (has_tcg) {
             qtest_add_func("acpi/virt", test_acpi_virt_tcg);
+            qtest_add_func("acpi/virt/topology", test_acpi_virt_tcg_topology);
             qtest_add_func("acpi/virt/numamem", test_acpi_virt_tcg_numamem);
             qtest_add_func("acpi/virt/memhp", test_acpi_virt_tcg_memhp);
             qtest_add_func("acpi/virt/pxb", test_acpi_virt_tcg_pxb);
-- 
2.24.0

[PATCH v2 4/4] tests: acpi: aarch64: add *.topology tables

[Yicong Yang via]

From: Yicong Yang <yangyicong@hisilicon.com>

Add *.topology tables for the aarch64's topology test.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 tests/data/acpi/virt/APIC.topology | Bin 0 -> 700 bytes
 tests/data/acpi/virt/DSDT.topology | Bin 0 -> 5398 bytes
 tests/data/acpi/virt/PPTT.topology | Bin 0 -> 336 bytes
 3 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 tests/data/acpi/virt/APIC.topology
 create mode 100644 tests/data/acpi/virt/DSDT.topology
 create mode 100644 tests/data/acpi/virt/PPTT.topology

diff --git a/tests/data/acpi/virt/APIC.topology b/tests/data/acpi/virt/APIC.topology
new file mode 100644
index 0000000000000000000000000000000000000000..558a1856981531b0e18e6d4aa12569cd80aa0015
GIT binary patch
literal 700
zcmbV~OA5k35JW4P_<@+UE_0Na*(d}Y$i~BZHNk$8j<`%06xH*o$1LVr?)g<q={-L3
zZSKcs$-SwP#7w$Q7oT+W$*O86UrB!d{M)jrTJASXrnUcf%@(j=xH;d-@;AWZec1Q5
zvgjgM$r49dbP=q^5=U8d5v-jhj<V<?SbIwxWzj{j4wg8|qKmrq-__rL18-2#2XL1S
A0RR91

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/DSDT.topology b/tests/data/acpi/virt/DSDT.topology
new file mode 100644
index 0000000000000000000000000000000000000000..501314c91be01d927fd125e0c72e919fdd85592e
GIT binary patch
literal 5398
zcmZvg%WoT16o>EFlh__VVmr?J;S@^6vl`n?la>}@kDbINPK+mQkX*@?5QvgZB`Ty+
zA*ERq=#EBW9i&M78%V6!v17rS4gUZ;%(-)ClHYO9NEy$Wd(SuX%^YWrr|CEMr>B&P
zoiz5mZGWZlN!MGU#ZpS?ZT*>lwrAZR_>DpTc;0heH#yjDH?wuG+ooVmB?ougO=ZR^
z(wNmhUZA|HH0H$2U`-s1o55@1plt?M#lbN%cwHPEH-l^9V4{C~)7$Grmc7ol>sBhE
zWpd#4{KC95^E{>WrAev0Qa_9<%eq9-6S@lPn+M*e0e{@;+@&j2rCfi%?xZQ%t6K(9
zaB>C_OU;Ivb^Bf~y0|;Ly*)}@y*TW7=EcDs6$=mUA|kv89H9^U3L>U15S0+o&}R|e
zDvoes62k^Y6&c|j9bv>J#yBu)$Ov!z2*Z{bNnl(<Mpz#sj4_Gf0Am#yVHu4u#wA7u
z7}t>zR@(?8Au)2mSVP9TDXbAjQexzRv5t%zA|oX+iom!5j7s?B7&#9|Vw8Y!6B%Ne
z@-InL>eIk@9~p9;W~B3&1;#C8$aR{P81ulmjSRU?a}r|_7#|=*uG0yLu?&n4ks;S<
zUSg~OV*?p-ofag<Yrv=@L$1@J#JCKM1~TM2os<|?fZ+k7D%WXAV!R2ACNktYost-D
z1EYluxlX4g#=F4SM21|aGZNz}Ft(5(*XgXpaDlOn47pC{Bt{h&ZDh!GIxjJ<0pkub
z<T_oD7}tUE5i;aDU6dH>z}P{CT&GJC<0ddVz^KV}x-2nn0b>^#a-EhX#s|RI3mLn=
zbiH<X9^KupTX)x~`S7UGGf_=<F|93HHyXR=ZHd3%E0mqZuJTk{eWq5FOMgw;`dU3y
zpVFt&kf8DC_Vy=tzH*L=X*)d}sx80mDzk0Tc10C4dcPB+pc(~n3TmpDwKKz^rF0I>
z3nQIH6LV%P$fK!Is56Nl%%v{L%nc)*8BL`YNFR}=2ALG<%;+fbATv6HxYC)?)VRr{
zsX-=%I+M;QIEo!)MrU9LnbA~gnL^7TlS1?yW1eF{X5=|$GNY5H5Ix74CpD#XKG9Ta
zvCxx3^h_|%1oKRAPYTg9$vl(HlUg$Lq!2w*%#+$_bM=BtlH#5eqNl?=9p*_b9C}iS
zo@wTpW}a#8Ng;Y>m}iE0Qp<;)6ryLAd1jesmU~i&o;l{3W1iF^q9=vuIl(+9nCArd
zq!2yn=ZvZGpo;U%lUhpjq!2v|%(K8e3*3`J^ei&ZBJ-pc6g??K&q?Mv$vh{yCxz%)
zVxA@DNi8dSQiz`PW0|f{^dDl1c}{Up3ej_#c}_D=YH`t%LiC(ro-@pIhI>+op7i4q
z?&mD?q?Q;xDMZgX<~hea=eQ?@=sC|k=b0z9(CA4adM+@}1?IWHJt;)bMdrE4JgMbI
zPYTg<iFqzD&n50jA$l$|&t>LGEjoHqh@NHUS!SMP?n$BX>>syneJjn+H~mod+|Ba`
zahG08<eYTyD&qCvkxtLuSN4_02Y%0|_b~w~>=+n|-V-3|vVb!C&QW*tS%nQQL+SSg
z$a+IynSGoUHoBZe?+uW3MPQkIA*+-hc#XO`qyM2Qzd<W=Ikpqd<L|R7M*q%f8S0hw
z9eukp)LjHiemMM3|16_rc$G%14D|qJp{9kFA&pw<#XFD_3?Jz+y#&$4O7DN7lK$Op
zS0%mu-i|75rUrYyXTLa9Uh|-Gx}7-rqA=;?`<=gP|CSdwemZzu|Mm8tpT9VCY?@G|
z&m?`;9_c`H^hQmip6ZoT*6Y*!%ae!Jw=_}-W>-$9U!Fws%<jA%e55Dq{bz?i=gfY6
zkjmL%>AgYI@7Sl8%-Q_0_WR%d>NlMqXa4ET{pNK}Qzu`lvqIdm^om||b?jctXVs`*
zbm^L_IqoahC%6Z6b;=tTmqu^V^Txb4Yb5Sp)$bU$TFrqear1()q8m?o!I-6ikZ<Zd
zZoOqvk6JzIOX-d#Q;yw#me!%y@>@GArKLgZ-hS$l4j!E5Po6$-bien!d(dk*NB!eD
My@B5+&m2qr5A>`*Jpcdz

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/PPTT.topology b/tests/data/acpi/virt/PPTT.topology
new file mode 100644
index 0000000000000000000000000000000000000000..3fbcae5ff08aaf16fedf4da45e941661d79c1174
GIT binary patch
literal 336
zcmWFt2nh*bWME*baq@Te2v%^42yj*a0-z8Bhz+6{L>L&rG>8oYKrs+dflv?<DrSKu
z#s}p4;1GkGi=-D>45YUMh?!vef$Csl%t&G&Cde(wdO>1GKm-gx_1*yTS+Iz)B8h>R
aAic=uf$S9l3b27BK>%tVNQ@mK!T<mOd=3Es

literal 0
HcmV?d00001

-- 
2.24.0

Re: [PATCH v2 0/4] Only generate cluster node in PPTT when specified

[Michael S. Tsirkin]

On Thu, Oct 27, 2022 at 11:26:09AM +0800, Yicong Yang wrote:
> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> This series mainly change the policy for building a cluster topology node
> in PPTT. Previously we'll always build a cluster node in PPTT without
> asking the user, after this set the cluster node will be built only the
> the user specify through "-smp clusters=X".
> 
> One problem is related to this but not fully caused by this, see the
> discussion in [*]. When booting the VM with `-smp 8` and 4 numa nodes,
> the linux scheduling domains in the VM misses the NUMA domains. It's
> because the MC level span extends to Cluster level (which is generated
> by the Qemu by default) that spans all the cpus in the system, then the
> scheduling domain building stops at MC level since it already includes all
> the cpus.
> 
> Considering cluster is an optional level and most platforms don't have it,
> they may even don't realize this is built and a always build policy cannot
> emulate the topology on these platforms. So in this series improve the
> policy to only generate cluster when the user explicitly want.
> 
> Update the tests and test tables accordingly.
> 
> [*] https://lore.kernel.org/lkml/2c079860-ee82-7719-d3d2-756192f41704@huawei.com/
> 
> Change since v1:
> - Only includes the test tables which is really needed
> - Enrich the commit
> Link: https://lore.kernel.org/qemu-devel/20220922131143.58003-1-yangyicong@huawei.com/


Looks superficually ok. ARM maintainers? Ack? Will you take this
or want me to?

Acked-by: Michael S. Tsirkin <mst@redhat.com>



> Yicong Yang (4):
>   hw/acpi/aml-build: Only generate cluster node in PPTT when specified
>   tests: virt: update expected ACPI tables for virt test
>   tests: acpi: aarch64: add topology test for aarch64
>   tests: acpi: aarch64: add *.topology tables
> 
>  hw/acpi/aml-build.c                |   2 +-
>  hw/core/machine-smp.c              |   3 +++
>  include/hw/boards.h                |   2 ++
>  qemu-options.hx                    |   2 ++
>  tests/data/acpi/virt/APIC.topology | Bin 0 -> 700 bytes
>  tests/data/acpi/virt/DSDT.topology | Bin 0 -> 5398 bytes
>  tests/data/acpi/virt/PPTT          | Bin 96 -> 76 bytes
>  tests/data/acpi/virt/PPTT.topology | Bin 0 -> 336 bytes
>  tests/qtest/bios-tables-test.c     |  22 ++++++++++++++++++++++
>  9 files changed, 30 insertions(+), 1 deletion(-)
>  create mode 100644 tests/data/acpi/virt/APIC.topology
>  create mode 100644 tests/data/acpi/virt/DSDT.topology
>  create mode 100644 tests/data/acpi/virt/PPTT.topology
> 
> -- 
> 2.24.0

Re: [PATCH v2 2/4] tests: virt: update expected ACPI tables for virt test

[Michael S. Tsirkin]

On Thu, Oct 27, 2022 at 11:26:11AM +0800, Yicong Yang wrote:
> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> Update the ACPI tables according to the acpi aml_build change.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

OK nice but if patch 1 is applied alone that will break make check
won't it? And this means the patchset breaks git bisect.
Pls look at top of tests/qtest/bios-tables-test.c to see
how to organize a patchset changing expected tables.



> ---
>  tests/data/acpi/virt/PPTT | Bin 96 -> 76 bytes
>  1 file changed, 0 insertions(+), 0 deletions(-)
> 
> diff --git a/tests/data/acpi/virt/PPTT b/tests/data/acpi/virt/PPTT
> index f56ea63b369a604877374ad696c396e796ab1c83..7a1258ecf123555b24462c98ccbb76b4ac1d0c2b 100644
> GIT binary patch
> delta 32
> fcmYfB;R*-{3GrcIU|?D?k;`ae01J-_kOKn%ZFdCM
> 
> delta 53
> pcmeZC;0g!`2}xjJU|{l?$YrDgWH5jU5Ca567#O&Klm(arApowi1QY-O
> 
> -- 
> 2.24.0

Re: [PATCH v2 3/4] tests: acpi: aarch64: add topology test for aarch64

[Michael S. Tsirkin]

On Thu, Oct 27, 2022 at 11:26:12AM +0800, Yicong Yang wrote:
> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> Add test for aarch64's ACPI topology building for all the supported
> levels.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

And same comment here.
Pls follow the process in bios-tables-test

> ---
>  tests/qtest/bios-tables-test.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index e6096e7f73..099b723444 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -1533,6 +1533,27 @@ static void test_acpi_virt_tcg(void)
>      free_test_data(&data);
>  }
>  
> +static void test_acpi_virt_tcg_topology(void)
> +{
> +    test_data data = {
> +        .machine = "virt",
> +        .variant = ".topology",
> +        .tcg_only = true,
> +        .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
> +        .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
> +        .cd = "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2",
> +        .ram_start = 0x40000000ULL,
> +        .scan_len = 128ULL * 1024 * 1024,
> +    };
> +
> +    data.smbios_cpu_max_speed = 2900;
> +    data.smbios_cpu_curr_speed = 2700;
> +    test_acpi_one("-cpu cortex-a57 "
> +                  "-smbios type=4,max-speed=2900,current-speed=2700 "
> +                  "-smp sockets=1,clusters=2,cores=2,threads=2", &data);
> +    free_test_data(&data);
> +}
> +
>  static void test_acpi_q35_viot(void)
>  {
>      test_data data = {
> @@ -1864,6 +1885,7 @@ int main(int argc, char *argv[])
>      } else if (strcmp(arch, "aarch64") == 0) {
>          if (has_tcg) {
>              qtest_add_func("acpi/virt", test_acpi_virt_tcg);
> +            qtest_add_func("acpi/virt/topology", test_acpi_virt_tcg_topology);
>              qtest_add_func("acpi/virt/numamem", test_acpi_virt_tcg_numamem);
>              qtest_add_func("acpi/virt/memhp", test_acpi_virt_tcg_memhp);
>              qtest_add_func("acpi/virt/pxb", test_acpi_virt_tcg_pxb);
> -- 
> 2.24.0

Re: [PATCH v2 1/4] hw/acpi/aml-build: Only generate cluster node in PPTT when specified

[wangyanan (Y) via]

Hi Yicong,

On 2022/10/27 11:26, 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. So only generate it when user specified explicitly.
>
> 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
>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>   hw/acpi/aml-build.c   | 2 +-
>   hw/core/machine-smp.c | 3 +++
>   include/hw/boards.h   | 2 ++
>   qemu-options.hx       | 2 ++
>   4 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index e6bfac95c7..aab73af66d 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 && ms->smp.build_cluster) {
>               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..5d37e8d07a 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)
> +        ms->smp.build_cluster = 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..c53f047b90 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -305,6 +305,7 @@ typedef struct DeviceMemoryState {
>    * @cores: the number of cores in one cluster
>    * @threads: the number of threads in one core
>    * @max_cpus: the maximum number of logical processors on the machine
> + * @build_cluster: build cluster topology or not
>    */
>   typedef struct CpuTopology {
>       unsigned int cpus;
> @@ -314,6 +315,7 @@ typedef struct CpuTopology {
>       unsigned int cores;
>       unsigned int threads;
>       unsigned int max_cpus;
> +    bool build_cluster;
build_cluster seems a variable defined specifically for ACPI PPTT
generation. It may not be proper to place it in the generic struct
CpuTopology which only holds topo members.

What about a more generic variable in struct SMPCompatProps
together with @clusters_supported. Something like below:

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 1f57ee8ca2..8db0706d5d 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 is explicitly specified in the user
+ *    provided SMP configuration.
   */
  typedef struct {
      bool prefer_sockets;
      bool dies_supported;
      bool clusters_supported;
+    bool has_clusters;
  } SMPCompatProps;

>   } CpuTopology;
>   
>   /**
> diff --git a/qemu-options.hx b/qemu-options.hx
> index eb38e5dc40..0a710e7be3 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -342,6 +342,8 @@ SRST
>       were preferred over threads), however, this behaviour is considered
>       liable to change. Prior to 6.2 the preference was sockets over cores
>       over threads. Since 6.2 the preference is cores over sockets over threads.
> +    The cluster topology will only be generated if explicitly specified
> +    by the "-cluster" option.
no "-cluster" option, only "-smp" :)
>       For example, the following option defines a machine board with 2 sockets
>       of 1 core before 6.2 and 1 socket of 2 cores after 6.2:
Maybe better to add a note at *end* of the doc about -smp like:

Note: The cluster topology will only be generated in ACPI and exposed
to guest if it's explicitly specified in -smp.

Thanks,
Yanan

Re: [PATCH v2 2/4] tests: virt: update expected ACPI tables for virt test

[Yicong Yang via]

On 2022/10/29 15:53, Michael S. Tsirkin wrote:
> On Thu, Oct 27, 2022 at 11:26:11AM +0800, Yicong Yang wrote:
>> From: Yicong Yang <yangyicong@hisilicon.com>
>>
>> Update the ACPI tables according to the acpi aml_build change.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> 
> OK nice but if patch 1 is applied alone that will break make check
> won't it? And this means the patchset breaks git bisect.
> Pls look at top of tests/qtest/bios-tables-test.c to see
> how to organize a patchset changing expected tables.
> 

Will follow the guidance and correctly mentioned the changes in
tests/qtest/bios-tables-test-allowed-diff.h. So is the rest patches
in the series.

Thanks.

> 
>> ---
>>  tests/data/acpi/virt/PPTT | Bin 96 -> 76 bytes
>>  1 file changed, 0 insertions(+), 0 deletions(-)
>>
>> diff --git a/tests/data/acpi/virt/PPTT b/tests/data/acpi/virt/PPTT
>> index f56ea63b369a604877374ad696c396e796ab1c83..7a1258ecf123555b24462c98ccbb76b4ac1d0c2b 100644
>> GIT binary patch
>> delta 32
>> fcmYfB;R*-{3GrcIU|?D?k;`ae01J-_kOKn%ZFdCM
>>
>> delta 53
>> pcmeZC;0g!`2}xjJU|{l?$YrDgWH5jU5Ca567#O&Klm(arApowi1QY-O
>>
>> -- 
>> 2.24.0
> 
> .
> 

Re: [PATCH v2 1/4] hw/acpi/aml-build: Only generate cluster node in PPTT when specified

[Yicong Yang via]

Hi Yanan,

On 2022/10/31 14:56, wangyanan (Y) wrote:
> Hi Yicong,
> 
> On 2022/10/27 11:26, 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. So only generate it when user specified explicitly.
>>
>> 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
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>   hw/acpi/aml-build.c   | 2 +-
>>   hw/core/machine-smp.c | 3 +++
>>   include/hw/boards.h   | 2 ++
>>   qemu-options.hx       | 2 ++
>>   4 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index e6bfac95c7..aab73af66d 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 && ms->smp.build_cluster) {
>>               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..5d37e8d07a 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)
>> +        ms->smp.build_cluster = 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..c53f047b90 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -305,6 +305,7 @@ typedef struct DeviceMemoryState {
>>    * @cores: the number of cores in one cluster
>>    * @threads: the number of threads in one core
>>    * @max_cpus: the maximum number of logical processors on the machine
>> + * @build_cluster: build cluster topology or not
>>    */
>>   typedef struct CpuTopology {
>>       unsigned int cpus;
>> @@ -314,6 +315,7 @@ typedef struct CpuTopology {
>>       unsigned int cores;
>>       unsigned int threads;
>>       unsigned int max_cpus;
>> +    bool build_cluster;
> build_cluster seems a variable defined specifically for ACPI PPTT
> generation. It may not be proper to place it in the generic struct
> CpuTopology which only holds topo members.
> 
> What about a more generic variable in struct SMPCompatProps
> together with @clusters_supported. Something like below:
> 

ok. will follow the suggestion. Thanks for the snippet :)

> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 1f57ee8ca2..8db0706d5d 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 is explicitly specified in the user
> + *    provided SMP configuration.
>   */
>  typedef struct {
>      bool prefer_sockets;
>      bool dies_supported;
>      bool clusters_supported;
> +    bool has_clusters;
>  } SMPCompatProps;
> 
>>   } CpuTopology;
>>     /**
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index eb38e5dc40..0a710e7be3 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -342,6 +342,8 @@ SRST
>>       were preferred over threads), however, this behaviour is considered
>>       liable to change. Prior to 6.2 the preference was sockets over cores
>>       over threads. Since 6.2 the preference is cores over sockets over threads.
>> +    The cluster topology will only be generated if explicitly specified
>> +    by the "-cluster" option.
> no "-cluster" option, only "-smp" :)

ok.

>>       For example, the following option defines a machine board with 2 sockets
>>       of 1 core before 6.2 and 1 socket of 2 cores after 6.2:
> Maybe better to add a note at *end* of the doc about -smp like:
> 
> Note: The cluster topology will only be generated in ACPI and exposed
> to guest if it's explicitly specified in -smp.
> 

will do.

Thanks.