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

[PATCH 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".

Update the tests and test tables accordingly.

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 ++
 tests/data/acpi/virt/APIC.pxb      | Bin 0 -> 168 bytes
 tests/data/acpi/virt/APIC.topology | Bin 0 -> 700 bytes
 tests/data/acpi/virt/DBG2.memhp    | Bin 0 -> 87 bytes
 tests/data/acpi/virt/DBG2.numamem  | Bin 0 -> 87 bytes
 tests/data/acpi/virt/DBG2.pxb      | Bin 0 -> 87 bytes
 tests/data/acpi/virt/DBG2.topology | Bin 0 -> 87 bytes
 tests/data/acpi/virt/DSDT.topology | Bin 0 -> 5398 bytes
 tests/data/acpi/virt/FACP.pxb      | Bin 0 -> 268 bytes
 tests/data/acpi/virt/FACP.topology | Bin 0 -> 268 bytes
 tests/data/acpi/virt/GTDT.pxb      | Bin 0 -> 96 bytes
 tests/data/acpi/virt/GTDT.topology | Bin 0 -> 96 bytes
 tests/data/acpi/virt/IORT.topology | Bin 0 -> 128 bytes
 tests/data/acpi/virt/MCFG.pxb      | Bin 0 -> 60 bytes
 tests/data/acpi/virt/MCFG.topology | Bin 0 -> 60 bytes
 tests/data/acpi/virt/PPTT          | Bin 96 -> 76 bytes
 tests/data/acpi/virt/PPTT.memhp    | Bin 0 -> 76 bytes
 tests/data/acpi/virt/PPTT.numamem  | Bin 0 -> 76 bytes
 tests/data/acpi/virt/PPTT.pxb      | Bin 0 -> 76 bytes
 tests/data/acpi/virt/PPTT.topology | Bin 0 -> 336 bytes
 tests/data/acpi/virt/SPCR.pxb      | Bin 0 -> 80 bytes
 tests/data/acpi/virt/SPCR.topology | Bin 0 -> 80 bytes
 tests/qtest/bios-tables-test.c     |  22 ++++++++++++++++++++++
 25 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100644 tests/data/acpi/virt/APIC.pxb
 create mode 100644 tests/data/acpi/virt/APIC.topology
 create mode 100644 tests/data/acpi/virt/DBG2.memhp
 create mode 100644 tests/data/acpi/virt/DBG2.numamem
 create mode 100644 tests/data/acpi/virt/DBG2.pxb
 create mode 100644 tests/data/acpi/virt/DBG2.topology
 create mode 100644 tests/data/acpi/virt/DSDT.topology
 create mode 100644 tests/data/acpi/virt/FACP.pxb
 create mode 100644 tests/data/acpi/virt/FACP.topology
 create mode 100644 tests/data/acpi/virt/GTDT.pxb
 create mode 100644 tests/data/acpi/virt/GTDT.topology
 create mode 100644 tests/data/acpi/virt/IORT.topology
 create mode 100644 tests/data/acpi/virt/MCFG.pxb
 create mode 100644 tests/data/acpi/virt/MCFG.topology
 create mode 100644 tests/data/acpi/virt/PPTT.memhp
 create mode 100644 tests/data/acpi/virt/PPTT.numamem
 create mode 100644 tests/data/acpi/virt/PPTT.pxb
 create mode 100644 tests/data/acpi/virt/PPTT.topology
 create mode 100644 tests/data/acpi/virt/SPCR.pxb
 create mode 100644 tests/data/acpi/virt/SPCR.topology

-- 
2.24.0

[PATCH 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 it's unncessary to build it if user don't need. So only generate
it when user specify explicitly.

Also update the test ACPI tables.

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 ++
 3 files changed, 6 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 7b416c9787..24aafc213d 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;
 
 /**
-- 
2.24.0

[PATCH 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/APIC.pxb     | Bin 0 -> 168 bytes
 tests/data/acpi/virt/DBG2.memhp   | Bin 0 -> 87 bytes
 tests/data/acpi/virt/DBG2.numamem | Bin 0 -> 87 bytes
 tests/data/acpi/virt/DBG2.pxb     | Bin 0 -> 87 bytes
 tests/data/acpi/virt/FACP.pxb     | Bin 0 -> 268 bytes
 tests/data/acpi/virt/GTDT.pxb     | Bin 0 -> 96 bytes
 tests/data/acpi/virt/MCFG.pxb     | Bin 0 -> 60 bytes
 tests/data/acpi/virt/PPTT         | Bin 96 -> 76 bytes
 tests/data/acpi/virt/PPTT.memhp   | Bin 0 -> 76 bytes
 tests/data/acpi/virt/PPTT.numamem | Bin 0 -> 76 bytes
 tests/data/acpi/virt/PPTT.pxb     | Bin 0 -> 76 bytes
 tests/data/acpi/virt/SPCR.pxb     | Bin 0 -> 80 bytes
 12 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 tests/data/acpi/virt/APIC.pxb
 create mode 100644 tests/data/acpi/virt/DBG2.memhp
 create mode 100644 tests/data/acpi/virt/DBG2.numamem
 create mode 100644 tests/data/acpi/virt/DBG2.pxb
 create mode 100644 tests/data/acpi/virt/FACP.pxb
 create mode 100644 tests/data/acpi/virt/GTDT.pxb
 create mode 100644 tests/data/acpi/virt/MCFG.pxb
 create mode 100644 tests/data/acpi/virt/PPTT.memhp
 create mode 100644 tests/data/acpi/virt/PPTT.numamem
 create mode 100644 tests/data/acpi/virt/PPTT.pxb
 create mode 100644 tests/data/acpi/virt/SPCR.pxb

diff --git a/tests/data/acpi/virt/APIC.pxb b/tests/data/acpi/virt/APIC.pxb
new file mode 100644
index 0000000000000000000000000000000000000000..023f15f12e74fb9a3a6d3d9dc994541016947d6a
GIT binary patch
literal 168
zcmZ<^@N{0mz`(#9;N<V@5v<@85#X!<1dKp25F13p0FMNW#lQh$F##FeJ}@2w15CX*
gLI}uWgsNwO(#&xED9V5wUbr1hU^O7~92f!^05#AE0RR91

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/DBG2.memhp b/tests/data/acpi/virt/DBG2.memhp
new file mode 100644
index 0000000000000000000000000000000000000000..86e6314f7b0235ef8ed3e0221e09f996c41f5e98
GIT binary patch
literal 87
zcmZ>9ayJTR0D|*Q{>~o33QiFL&I&-l2owUbL9`AKgJ=eA21Zr}H4uw|p@A7lh%qQJ
TFmQk+Il-a=3=Gcxz6J~c3~mVl

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/DBG2.numamem b/tests/data/acpi/virt/DBG2.numamem
new file mode 100644
index 0000000000000000000000000000000000000000..86e6314f7b0235ef8ed3e0221e09f996c41f5e98
GIT binary patch
literal 87
zcmZ>9ayJTR0D|*Q{>~o33QiFL&I&-l2owUbL9`AKgJ=eA21Zr}H4uw|p@A7lh%qQJ
TFmQk+Il-a=3=Gcxz6J~c3~mVl

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/DBG2.pxb b/tests/data/acpi/virt/DBG2.pxb
new file mode 100644
index 0000000000000000000000000000000000000000..86e6314f7b0235ef8ed3e0221e09f996c41f5e98
GIT binary patch
literal 87
zcmZ>9ayJTR0D|*Q{>~o33QiFL&I&-l2owUbL9`AKgJ=eA21Zr}H4uw|p@A7lh%qQJ
TFmQk+Il-a=3=Gcxz6J~c3~mVl

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/FACP.pxb b/tests/data/acpi/virt/FACP.pxb
new file mode 100644
index 0000000000000000000000000000000000000000..1f764220f8533c427168e80ccf298604826a00b4
GIT binary patch
literal 268
ycmZ>BbPnKQWME(ob@F%i2v%^42yj*a0-z8Bhz+8t3j|P&V`iYf6{t24%>w}Cy9NOO

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/GTDT.pxb b/tests/data/acpi/virt/GTDT.pxb
new file mode 100644
index 0000000000000000000000000000000000000000..9408b71b59c0e0f2991c0053562280155b47bc0b
GIT binary patch
literal 96
zcmZ<{aS2IaU|?YCaq@Te2v%^42yj*a0!E-1hz+8Vfh?3G2c%hmSQ^Tgg3{;|04K!<
A0RR91

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/MCFG.pxb b/tests/data/acpi/virt/MCFG.pxb
new file mode 100644
index 0000000000000000000000000000000000000000..f4ae3203a4e9258a397c9912332178640209637b
GIT binary patch
literal 60
scmeZuc5}C3U|?W;<K*w`5v<@85#X!<1dKp25F11z0RaaH=Rb%706Iko0RR91

literal 0
HcmV?d00001

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

diff --git a/tests/data/acpi/virt/PPTT.memhp b/tests/data/acpi/virt/PPTT.memhp
new file mode 100644
index 0000000000000000000000000000000000000000..7a1258ecf123555b24462c98ccbb76b4ac1d0c2b
GIT binary patch
literal 76
zcmWFt2nq3FU|?Wc;pFe^5v<@85#X!<1dKp25F11@h%hjKX%HI*fMQ%gwhD|7qyeJ>
B2LS*8

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/PPTT.numamem b/tests/data/acpi/virt/PPTT.numamem
new file mode 100644
index 0000000000000000000000000000000000000000..7a1258ecf123555b24462c98ccbb76b4ac1d0c2b
GIT binary patch
literal 76
zcmWFt2nq3FU|?Wc;pFe^5v<@85#X!<1dKp25F11@h%hjKX%HI*fMQ%gwhD|7qyeJ>
B2LS*8

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/PPTT.pxb b/tests/data/acpi/virt/PPTT.pxb
new file mode 100644
index 0000000000000000000000000000000000000000..7a1258ecf123555b24462c98ccbb76b4ac1d0c2b
GIT binary patch
literal 76
zcmWFt2nq3FU|?Wc;pFe^5v<@85#X!<1dKp25F11@h%hjKX%HI*fMQ%gwhD|7qyeJ>
B2LS*8

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/SPCR.pxb b/tests/data/acpi/virt/SPCR.pxb
new file mode 100644
index 0000000000000000000000000000000000000000..24e0a579e7d73f432a614380e29aa95113344186
GIT binary patch
literal 80
zcmWFza1IJ!U|?W6?d0$55v<@85#X!<1dKp25F11@12F>!1Bl59VJZSCW(Gzk28RDY
I01|@%0RBM>0RR91

literal 0
HcmV?d00001

-- 
2.24.0

[PATCH 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 7c5f736b51..8579b43491 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -1531,6 +1531,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 = {
@@ -1831,6 +1852,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 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/DBG2.topology | Bin 0 -> 87 bytes
 tests/data/acpi/virt/DSDT.topology | Bin 0 -> 5398 bytes
 tests/data/acpi/virt/FACP.topology | Bin 0 -> 268 bytes
 tests/data/acpi/virt/GTDT.topology | Bin 0 -> 96 bytes
 tests/data/acpi/virt/IORT.topology | Bin 0 -> 128 bytes
 tests/data/acpi/virt/MCFG.topology | Bin 0 -> 60 bytes
 tests/data/acpi/virt/PPTT.topology | Bin 0 -> 336 bytes
 tests/data/acpi/virt/SPCR.topology | Bin 0 -> 80 bytes
 9 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 tests/data/acpi/virt/APIC.topology
 create mode 100644 tests/data/acpi/virt/DBG2.topology
 create mode 100644 tests/data/acpi/virt/DSDT.topology
 create mode 100644 tests/data/acpi/virt/FACP.topology
 create mode 100644 tests/data/acpi/virt/GTDT.topology
 create mode 100644 tests/data/acpi/virt/IORT.topology
 create mode 100644 tests/data/acpi/virt/MCFG.topology
 create mode 100644 tests/data/acpi/virt/PPTT.topology
 create mode 100644 tests/data/acpi/virt/SPCR.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/DBG2.topology b/tests/data/acpi/virt/DBG2.topology
new file mode 100644
index 0000000000000000000000000000000000000000..86e6314f7b0235ef8ed3e0221e09f996c41f5e98
GIT binary patch
literal 87
zcmZ>9ayJTR0D|*Q{>~o33QiFL&I&-l2owUbL9`AKgJ=eA21Zr}H4uw|p@A7lh%qQJ
TFmQk+Il-a=3=Gcxz6J~c3~mVl

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/FACP.topology b/tests/data/acpi/virt/FACP.topology
new file mode 100644
index 0000000000000000000000000000000000000000..1f764220f8533c427168e80ccf298604826a00b4
GIT binary patch
literal 268
ycmZ>BbPnKQWME(ob@F%i2v%^42yj*a0-z8Bhz+8t3j|P&V`iYf6{t24%>w}Cy9NOO

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/GTDT.topology b/tests/data/acpi/virt/GTDT.topology
new file mode 100644
index 0000000000000000000000000000000000000000..9408b71b59c0e0f2991c0053562280155b47bc0b
GIT binary patch
literal 96
zcmZ<{aS2IaU|?YCaq@Te2v%^42yj*a0!E-1hz+8Vfh?3G2c%hmSQ^Tgg3{;|04K!<
A0RR91

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/IORT.topology b/tests/data/acpi/virt/IORT.topology
new file mode 100644
index 0000000000000000000000000000000000000000..7efd0ce8a6b3928efa7e1373f688ab4c5f50543b
GIT binary patch
literal 128
zcmebD4+?2uU|?Y0?Bwt45v<@85#X!<1dKp25F11@0kHuPgMkDCNC*yK93~3}W)K^M
VRiHGGVg_O`aDdYP|3ers^8jQz3IPBB

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/virt/MCFG.topology b/tests/data/acpi/virt/MCFG.topology
new file mode 100644
index 0000000000000000000000000000000000000000..f4ae3203a4e9258a397c9912332178640209637b
GIT binary patch
literal 60
scmeZuc5}C3U|?W;<K*w`5v<@85#X!<1dKp25F11z0RaaH=Rb%706Iko0RR91

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

diff --git a/tests/data/acpi/virt/SPCR.topology b/tests/data/acpi/virt/SPCR.topology
new file mode 100644
index 0000000000000000000000000000000000000000..24e0a579e7d73f432a614380e29aa95113344186
GIT binary patch
literal 80
zcmWFza1IJ!U|?W6?d0$55v<@85#X!<1dKp25F11@12F>!1Bl59VJZSCW(Gzk28RDY
I01|@%0RBM>0RR91

literal 0
HcmV?d00001

-- 
2.24.0

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

[Jonathan Cameron via]

On Thu, 22 Sep 2022 21:11:39 +0800
Yicong Yang <yangyicong@huawei.com> 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".
> 
> Update the tests and test tables accordingly.
Hi Yicong,

Does this bring an issue with backwards compatibility?
I'm assuming we need to make this something controlled in
virt_machine_7_1_options()?

One of those things we probably should have noticed originally but now
we are stuck with the slightly messy backwards compatibility problem.

Also, looks like you picked up a bunch of tables that are unrelated
to the change.

Jonathan


> 
> 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 ++
>  tests/data/acpi/virt/APIC.pxb      | Bin 0 -> 168 bytes
>  tests/data/acpi/virt/APIC.topology | Bin 0 -> 700 bytes
>  tests/data/acpi/virt/DBG2.memhp    | Bin 0 -> 87 bytes
>  tests/data/acpi/virt/DBG2.numamem  | Bin 0 -> 87 bytes
>  tests/data/acpi/virt/DBG2.pxb      | Bin 0 -> 87 bytes
>  tests/data/acpi/virt/DBG2.topology | Bin 0 -> 87 bytes
>  tests/data/acpi/virt/DSDT.topology | Bin 0 -> 5398 bytes
>  tests/data/acpi/virt/FACP.pxb      | Bin 0 -> 268 bytes
>  tests/data/acpi/virt/FACP.topology | Bin 0 -> 268 bytes
>  tests/data/acpi/virt/GTDT.pxb      | Bin 0 -> 96 bytes
>  tests/data/acpi/virt/GTDT.topology | Bin 0 -> 96 bytes
>  tests/data/acpi/virt/IORT.topology | Bin 0 -> 128 bytes
>  tests/data/acpi/virt/MCFG.pxb      | Bin 0 -> 60 bytes
>  tests/data/acpi/virt/MCFG.topology | Bin 0 -> 60 bytes
>  tests/data/acpi/virt/PPTT          | Bin 96 -> 76 bytes
>  tests/data/acpi/virt/PPTT.memhp    | Bin 0 -> 76 bytes
>  tests/data/acpi/virt/PPTT.numamem  | Bin 0 -> 76 bytes
>  tests/data/acpi/virt/PPTT.pxb      | Bin 0 -> 76 bytes
>  tests/data/acpi/virt/PPTT.topology | Bin 0 -> 336 bytes
>  tests/data/acpi/virt/SPCR.pxb      | Bin 0 -> 80 bytes
>  tests/data/acpi/virt/SPCR.topology | Bin 0 -> 80 bytes

>  tests/qtest/bios-tables-test.c     |  22 ++++++++++++++++++++++
>  25 files changed, 28 insertions(+), 1 deletion(-)
>  create mode 100644 tests/data/acpi/virt/APIC.pxb
>  create mode 100644 tests/data/acpi/virt/APIC.topology
>  create mode 100644 tests/data/acpi/virt/DBG2.memhp
>  create mode 100644 tests/data/acpi/virt/DBG2.numamem
>  create mode 100644 tests/data/acpi/virt/DBG2.pxb
>  create mode 100644 tests/data/acpi/virt/DBG2.topology
>  create mode 100644 tests/data/acpi/virt/DSDT.topology
>  create mode 100644 tests/data/acpi/virt/FACP.pxb
>  create mode 100644 tests/data/acpi/virt/FACP.topology
>  create mode 100644 tests/data/acpi/virt/GTDT.pxb
>  create mode 100644 tests/data/acpi/virt/GTDT.topology
>  create mode 100644 tests/data/acpi/virt/IORT.topology
>  create mode 100644 tests/data/acpi/virt/MCFG.pxb
>  create mode 100644 tests/data/acpi/virt/MCFG.topology
>  create mode 100644 tests/data/acpi/virt/PPTT.memhp
>  create mode 100644 tests/data/acpi/virt/PPTT.numamem
>  create mode 100644 tests/data/acpi/virt/PPTT.pxb
>  create mode 100644 tests/data/acpi/virt/PPTT.topology
>  create mode 100644 tests/data/acpi/virt/SPCR.pxb
>  create mode 100644 tests/data/acpi/virt/SPCR.topology
> 

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

[Yicong Yang via]

On 2022/9/22 22:31, Jonathan Cameron wrote:
> On Thu, 22 Sep 2022 21:11:39 +0800
> Yicong Yang <yangyicong@huawei.com> 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".
>>
>> Update the tests and test tables accordingly.
> Hi Yicong,

Hi Jonathan,

Thanks for the comments!

> 
> Does this bring an issue with backwards compatibility?
> I'm assuming we need to make this something controlled in
> virt_machine_7_1_options()?
> 

I'm not sure but I cannot come up with one for now. Any hints for this?
Maybe Yanan can shed some light on this since they are using this cluster
feature.

> One of those things we probably should have noticed originally but now
> we are stuck with the slightly messy backwards compatibility problem.
> 
> Also, looks like you picked up a bunch of tables that are unrelated
> to the change.
> 

The tables are updated by tests/data/acpi/rebuild-expected-aml.sh so I think
it's doing the proper thing. These tables are introduced due to:

Patch 1 - Don't build the cluster node by default, so the "virt" test is influenced
Patch 2 - Update the expected PPTT table to get rid of the cluster node
Patch 3 - Since the cluster is not tested, add a new test for topology test of all
          level for aarch64, with command "-smp sockets=1,clusters=2,cores=2,threads=2"
Patch 4 - Update the expected tables for the topology test introduced in Patch 3


> Jonathan
> 
> 
>>
>> 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 ++
>>  tests/data/acpi/virt/APIC.pxb      | Bin 0 -> 168 bytes
>>  tests/data/acpi/virt/APIC.topology | Bin 0 -> 700 bytes
>>  tests/data/acpi/virt/DBG2.memhp    | Bin 0 -> 87 bytes
>>  tests/data/acpi/virt/DBG2.numamem  | Bin 0 -> 87 bytes
>>  tests/data/acpi/virt/DBG2.pxb      | Bin 0 -> 87 bytes
>>  tests/data/acpi/virt/DBG2.topology | Bin 0 -> 87 bytes
>>  tests/data/acpi/virt/DSDT.topology | Bin 0 -> 5398 bytes
>>  tests/data/acpi/virt/FACP.pxb      | Bin 0 -> 268 bytes
>>  tests/data/acpi/virt/FACP.topology | Bin 0 -> 268 bytes
>>  tests/data/acpi/virt/GTDT.pxb      | Bin 0 -> 96 bytes
>>  tests/data/acpi/virt/GTDT.topology | Bin 0 -> 96 bytes
>>  tests/data/acpi/virt/IORT.topology | Bin 0 -> 128 bytes
>>  tests/data/acpi/virt/MCFG.pxb      | Bin 0 -> 60 bytes
>>  tests/data/acpi/virt/MCFG.topology | Bin 0 -> 60 bytes
>>  tests/data/acpi/virt/PPTT          | Bin 96 -> 76 bytes
>>  tests/data/acpi/virt/PPTT.memhp    | Bin 0 -> 76 bytes
>>  tests/data/acpi/virt/PPTT.numamem  | Bin 0 -> 76 bytes
>>  tests/data/acpi/virt/PPTT.pxb      | Bin 0 -> 76 bytes
>>  tests/data/acpi/virt/PPTT.topology | Bin 0 -> 336 bytes
>>  tests/data/acpi/virt/SPCR.pxb      | Bin 0 -> 80 bytes
>>  tests/data/acpi/virt/SPCR.topology | Bin 0 -> 80 bytes
> 
>>  tests/qtest/bios-tables-test.c     |  22 ++++++++++++++++++++++
>>  25 files changed, 28 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/data/acpi/virt/APIC.pxb
>>  create mode 100644 tests/data/acpi/virt/APIC.topology
>>  create mode 100644 tests/data/acpi/virt/DBG2.memhp
>>  create mode 100644 tests/data/acpi/virt/DBG2.numamem
>>  create mode 100644 tests/data/acpi/virt/DBG2.pxb
>>  create mode 100644 tests/data/acpi/virt/DBG2.topology
>>  create mode 100644 tests/data/acpi/virt/DSDT.topology
>>  create mode 100644 tests/data/acpi/virt/FACP.pxb
>>  create mode 100644 tests/data/acpi/virt/FACP.topology
>>  create mode 100644 tests/data/acpi/virt/GTDT.pxb
>>  create mode 100644 tests/data/acpi/virt/GTDT.topology
>>  create mode 100644 tests/data/acpi/virt/IORT.topology
>>  create mode 100644 tests/data/acpi/virt/MCFG.pxb
>>  create mode 100644 tests/data/acpi/virt/MCFG.topology
>>  create mode 100644 tests/data/acpi/virt/PPTT.memhp
>>  create mode 100644 tests/data/acpi/virt/PPTT.numamem
>>  create mode 100644 tests/data/acpi/virt/PPTT.pxb
>>  create mode 100644 tests/data/acpi/virt/PPTT.topology
>>  create mode 100644 tests/data/acpi/virt/SPCR.pxb
>>  create mode 100644 tests/data/acpi/virt/SPCR.topology
>>
> 
> .
> 

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

[Michael S. Tsirkin]

On Thu, Sep 22, 2022 at 09:11:39PM +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".
> 
> Update the tests and test tables accordingly.

Given it's virt machine only, I'd like an ack from relevant maintainers.


> 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 ++
>  tests/data/acpi/virt/APIC.pxb      | Bin 0 -> 168 bytes
>  tests/data/acpi/virt/APIC.topology | Bin 0 -> 700 bytes
>  tests/data/acpi/virt/DBG2.memhp    | Bin 0 -> 87 bytes
>  tests/data/acpi/virt/DBG2.numamem  | Bin 0 -> 87 bytes
>  tests/data/acpi/virt/DBG2.pxb      | Bin 0 -> 87 bytes
>  tests/data/acpi/virt/DBG2.topology | Bin 0 -> 87 bytes
>  tests/data/acpi/virt/DSDT.topology | Bin 0 -> 5398 bytes
>  tests/data/acpi/virt/FACP.pxb      | Bin 0 -> 268 bytes
>  tests/data/acpi/virt/FACP.topology | Bin 0 -> 268 bytes
>  tests/data/acpi/virt/GTDT.pxb      | Bin 0 -> 96 bytes
>  tests/data/acpi/virt/GTDT.topology | Bin 0 -> 96 bytes
>  tests/data/acpi/virt/IORT.topology | Bin 0 -> 128 bytes
>  tests/data/acpi/virt/MCFG.pxb      | Bin 0 -> 60 bytes
>  tests/data/acpi/virt/MCFG.topology | Bin 0 -> 60 bytes
>  tests/data/acpi/virt/PPTT          | Bin 96 -> 76 bytes
>  tests/data/acpi/virt/PPTT.memhp    | Bin 0 -> 76 bytes
>  tests/data/acpi/virt/PPTT.numamem  | Bin 0 -> 76 bytes
>  tests/data/acpi/virt/PPTT.pxb      | Bin 0 -> 76 bytes
>  tests/data/acpi/virt/PPTT.topology | Bin 0 -> 336 bytes
>  tests/data/acpi/virt/SPCR.pxb      | Bin 0 -> 80 bytes
>  tests/data/acpi/virt/SPCR.topology | Bin 0 -> 80 bytes
>  tests/qtest/bios-tables-test.c     |  22 ++++++++++++++++++++++
>  25 files changed, 28 insertions(+), 1 deletion(-)
>  create mode 100644 tests/data/acpi/virt/APIC.pxb
>  create mode 100644 tests/data/acpi/virt/APIC.topology
>  create mode 100644 tests/data/acpi/virt/DBG2.memhp
>  create mode 100644 tests/data/acpi/virt/DBG2.numamem
>  create mode 100644 tests/data/acpi/virt/DBG2.pxb
>  create mode 100644 tests/data/acpi/virt/DBG2.topology
>  create mode 100644 tests/data/acpi/virt/DSDT.topology
>  create mode 100644 tests/data/acpi/virt/FACP.pxb
>  create mode 100644 tests/data/acpi/virt/FACP.topology
>  create mode 100644 tests/data/acpi/virt/GTDT.pxb
>  create mode 100644 tests/data/acpi/virt/GTDT.topology
>  create mode 100644 tests/data/acpi/virt/IORT.topology
>  create mode 100644 tests/data/acpi/virt/MCFG.pxb
>  create mode 100644 tests/data/acpi/virt/MCFG.topology
>  create mode 100644 tests/data/acpi/virt/PPTT.memhp
>  create mode 100644 tests/data/acpi/virt/PPTT.numamem
>  create mode 100644 tests/data/acpi/virt/PPTT.pxb
>  create mode 100644 tests/data/acpi/virt/PPTT.topology
>  create mode 100644 tests/data/acpi/virt/SPCR.pxb
>  create mode 100644 tests/data/acpi/virt/SPCR.topology
> 
> -- 
> 2.24.0

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

[Michael S. Tsirkin]

On Thu, Sep 22, 2022 at 09:11:40PM +0800, 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 it's unncessary to build it if user don't need. So only generate
> it when user specify explicitly.
> 
> Also update the test ACPI tables.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

This is an example of a commit log repeating what the patch does.
Which is ok but the important thing is to explain the motivation -
why is it a bug to generate a cluster node without '-smp clusters'?


> ---
>  hw/acpi/aml-build.c   | 2 +-
>  hw/core/machine-smp.c | 3 +++
>  include/hw/boards.h   | 2 ++
>  3 files changed, 6 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 7b416c9787..24aafc213d 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;
>  
>  /**
> -- 
> 2.24.0

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

[wangyanan (Y) via]

Hi Yicong,

On 2022/9/22 21:11, 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 it's unncessary to build it if user don't need. So only generate
> it when user specify explicitly.
>
> Also update the test ACPI tables.
It would be much more helpful to explain the problem you
have met in practice without this patch. (maybe have some
description or a link of the issue in the cover-letter if we
need a v2).

In qemu which behaves as like a firmware vendor for VM,
the ACPI PPTT is built based on the topology info produced
by machine_parse_smp_config(). And machine_parse_smp_config
will always calculate a complete topology hierarchy using its
algorithm, if the user gives an incomplete -smp CLI.

I think there are two options for us to chose:
1) approach described in this patch
2) qemu will always generate a full topology hierarchy in PPTT
with all the topo members it currently supports. While users
need to consider the necessity to use an incomplete -smp or
an complete one according to their specific scenario, and
should be aware of the kernel behavior resulted from the
config.

There is some Doc for users to explain how qemu will
parse user-specified -smp in [1].
[1] https://www.mankier.com/1/qemu#Options

Thanks,
Yanan
> 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 ++
>   3 files changed, 6 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 7b416c9787..24aafc213d 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;
>   
>   /**

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

[Yicong Yang via]

On 2022/10/9 14:46, wangyanan (Y) wrote:
> Hi Yicong,
> 
> On 2022/9/22 21:11, 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 it's unncessary to build it if user don't need. So only generate
>> it when user specify explicitly.
>>
>> Also update the test ACPI tables.
> It would be much more helpful to explain the problem you
> have met in practice without this patch. (maybe have some
> description or a link of the issue in the cover-letter if we
> need a v2).
> 

My problem is related to this but not fully caused by this.

I found my schedule domains are not built as expected with command
`-smp 8` and 4 NUMA nodes. The final schedule domains built look
like below with no NUMA domains built.

[    2.141316] CPU0 attaching sched-domain(s):
[    2.142558]  domain-0: span=0-7 level=MC
[    2.145364]   groups: 0:{ span=0 cap=964 }, 1:{ span=1 cap=914 }, 2:{ span=2 cap=921 }, 3:{ span=3 cap=964 }, 4:{ span=4 cap=925 }, 5:{ span=5 cap=964 }, 6:{ span=6 cap=967 }, 7:{ span=7 cap=967 }
[    2.158357] CPU1 attaching sched-domain(s):
[    2.158964]  domain-0: span=0-7 level=MC

should be:

[    2.008885] CPU0 attaching sched-domain(s):
[    2.009764]  domain-0: span=0-1 level=MC
[    2.012654]   groups: 0:{ span=0 cap=962 }, 1:{ span=1 cap=925 }
[    2.016532]   domain-1: span=0-3 level=NUMA
[    2.017444]    groups: 0:{ span=0-1 cap=1887 }, 2:{ span=2-3 cap=1871 }
[    2.019354]    domain-2: span=0-5 level=NUMA
[    2.019983]     groups: 0:{ span=0-3 cap=3758 }, 4:{ span=4-5 cap=1935 }
[    2.021527]     domain-3: span=0-7 level=NUMA
[    2.022516]      groups: 0:{ span=0-5 mask=0-1 cap=5693 }, 6:{ span=4-7 mask=6-7 cap=3978 }
[...]

It's because the MC level span extends to Cluster level which spans
all the cpus in the system, then the schedule domain building stops
at MC level since it already includes all the cpus.

It makes people confusing that cluster node is generated without
asking for it.

A discussion for the problem:
https://lore.kernel.org/lkml/2c079860-ee82-7719-d3d2-756192f41704@huawei.com/

> In qemu which behaves as like a firmware vendor for VM,
> the ACPI PPTT is built based on the topology info produced
> by machine_parse_smp_config(). And machine_parse_smp_config
> will always calculate a complete topology hierarchy using its
> algorithm, if the user gives an incomplete -smp CLI.
> 

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.
Also it may influences the build of schedule domains uncousiously
in some cases so...

> I think there are two options for us to chose:
> 1) approach described in this patch
> 2) qemu will always generate a full topology hierarchy in PPTT
> with all the topo members it currently supports. While users
> need to consider the necessity to use an incomplete -smp or
> an complete one according to their specific scenario, and
> should be aware of the kernel behavior resulted from the
> config.
> 

...I'd prefer 1) then users can generate this *only* when they
explicitly know what they want and what they'll get. A full
topology hierachy generation lacks flexibility. Any thought?

> There is some Doc for users to explain how qemu will
> parse user-specified -smp in [1].
> [1] https://www.mankier.com/1/qemu#Options
> 

Thanks!
Yicong

> Thanks,
> Yanan
>> 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 ++
>>   3 files changed, 6 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 7b416c9787..24aafc213d 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;
>>     /**
> 
> .

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

[Yicong Yang via]

On 2022/10/7 21:48, Michael S. Tsirkin wrote:
> On Thu, Sep 22, 2022 at 09:11:40PM +0800, 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 it's unncessary to build it if user don't need. So only generate
>> it when user specify explicitly.
>>
>> Also update the test ACPI tables.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> 
> This is an example of a commit log repeating what the patch does.
> Which is ok but the important thing is to explain the motivation -
> why is it a bug to generate a cluster node without '-smp clusters'?
> 

It may not be a bug but may build the unneeded topology unconsciously
and doesn't provide a way to inhibit this. So I thought the policy
can be improved.

Thanks.

> 
>> ---
>>  hw/acpi/aml-build.c   | 2 +-
>>  hw/core/machine-smp.c | 3 +++
>>  include/hw/boards.h   | 2 ++
>>  3 files changed, 6 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 7b416c9787..24aafc213d 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;
>>  
>>  /**
>> -- 
>> 2.24.0
> 
> .
> 

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

[Michael S. Tsirkin]

On Thu, Sep 22, 2022 at 09:11:39PM +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".
> 
> Update the tests and test tables accordingly.


This will need an ack from virt maintainers.

And I think what people are asking is about the impact
of this patch on guests.

> 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 ++
>  tests/data/acpi/virt/APIC.pxb      | Bin 0 -> 168 bytes
>  tests/data/acpi/virt/APIC.topology | Bin 0 -> 700 bytes
>  tests/data/acpi/virt/DBG2.memhp    | Bin 0 -> 87 bytes
>  tests/data/acpi/virt/DBG2.numamem  | Bin 0 -> 87 bytes
>  tests/data/acpi/virt/DBG2.pxb      | Bin 0 -> 87 bytes
>  tests/data/acpi/virt/DBG2.topology | Bin 0 -> 87 bytes
>  tests/data/acpi/virt/DSDT.topology | Bin 0 -> 5398 bytes
>  tests/data/acpi/virt/FACP.pxb      | Bin 0 -> 268 bytes
>  tests/data/acpi/virt/FACP.topology | Bin 0 -> 268 bytes
>  tests/data/acpi/virt/GTDT.pxb      | Bin 0 -> 96 bytes
>  tests/data/acpi/virt/GTDT.topology | Bin 0 -> 96 bytes
>  tests/data/acpi/virt/IORT.topology | Bin 0 -> 128 bytes
>  tests/data/acpi/virt/MCFG.pxb      | Bin 0 -> 60 bytes
>  tests/data/acpi/virt/MCFG.topology | Bin 0 -> 60 bytes
>  tests/data/acpi/virt/PPTT          | Bin 96 -> 76 bytes
>  tests/data/acpi/virt/PPTT.memhp    | Bin 0 -> 76 bytes
>  tests/data/acpi/virt/PPTT.numamem  | Bin 0 -> 76 bytes
>  tests/data/acpi/virt/PPTT.pxb      | Bin 0 -> 76 bytes
>  tests/data/acpi/virt/PPTT.topology | Bin 0 -> 336 bytes
>  tests/data/acpi/virt/SPCR.pxb      | Bin 0 -> 80 bytes
>  tests/data/acpi/virt/SPCR.topology | Bin 0 -> 80 bytes
>  tests/qtest/bios-tables-test.c     |  22 ++++++++++++++++++++++
>  25 files changed, 28 insertions(+), 1 deletion(-)
>  create mode 100644 tests/data/acpi/virt/APIC.pxb
>  create mode 100644 tests/data/acpi/virt/APIC.topology
>  create mode 100644 tests/data/acpi/virt/DBG2.memhp
>  create mode 100644 tests/data/acpi/virt/DBG2.numamem
>  create mode 100644 tests/data/acpi/virt/DBG2.pxb
>  create mode 100644 tests/data/acpi/virt/DBG2.topology
>  create mode 100644 tests/data/acpi/virt/DSDT.topology
>  create mode 100644 tests/data/acpi/virt/FACP.pxb
>  create mode 100644 tests/data/acpi/virt/FACP.topology
>  create mode 100644 tests/data/acpi/virt/GTDT.pxb
>  create mode 100644 tests/data/acpi/virt/GTDT.topology
>  create mode 100644 tests/data/acpi/virt/IORT.topology
>  create mode 100644 tests/data/acpi/virt/MCFG.pxb
>  create mode 100644 tests/data/acpi/virt/MCFG.topology
>  create mode 100644 tests/data/acpi/virt/PPTT.memhp
>  create mode 100644 tests/data/acpi/virt/PPTT.numamem
>  create mode 100644 tests/data/acpi/virt/PPTT.pxb
>  create mode 100644 tests/data/acpi/virt/PPTT.topology
>  create mode 100644 tests/data/acpi/virt/SPCR.pxb
>  create mode 100644 tests/data/acpi/virt/SPCR.topology
> 
> -- 
> 2.24.0

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

[Yicong Yang via]

Hi Michael,

On 2022/10/26 22:52, Michael S. Tsirkin wrote:
> On Thu, Sep 22, 2022 at 09:11:39PM +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".
>>
>> Update the tests and test tables accordingly.
> 
> 
> This will need an ack from virt maintainers.

Sure.

> 
> And I think what people are asking is about the impact
> of this patch on guests.

On the current kernel (tested on 6.1-rc1), there's no impact for the kernel
side, only the cluster node in PPTT is not generated. Booted with `-smp 8`

without this patch:

with 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

Will send a v2 version and update this information.

Thanks.

> 
>> 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 ++
>>  tests/data/acpi/virt/APIC.pxb      | Bin 0 -> 168 bytes
>>  tests/data/acpi/virt/APIC.topology | Bin 0 -> 700 bytes
>>  tests/data/acpi/virt/DBG2.memhp    | Bin 0 -> 87 bytes
>>  tests/data/acpi/virt/DBG2.numamem  | Bin 0 -> 87 bytes
>>  tests/data/acpi/virt/DBG2.pxb      | Bin 0 -> 87 bytes
>>  tests/data/acpi/virt/DBG2.topology | Bin 0 -> 87 bytes
>>  tests/data/acpi/virt/DSDT.topology | Bin 0 -> 5398 bytes
>>  tests/data/acpi/virt/FACP.pxb      | Bin 0 -> 268 bytes
>>  tests/data/acpi/virt/FACP.topology | Bin 0 -> 268 bytes
>>  tests/data/acpi/virt/GTDT.pxb      | Bin 0 -> 96 bytes
>>  tests/data/acpi/virt/GTDT.topology | Bin 0 -> 96 bytes
>>  tests/data/acpi/virt/IORT.topology | Bin 0 -> 128 bytes
>>  tests/data/acpi/virt/MCFG.pxb      | Bin 0 -> 60 bytes
>>  tests/data/acpi/virt/MCFG.topology | Bin 0 -> 60 bytes
>>  tests/data/acpi/virt/PPTT          | Bin 96 -> 76 bytes
>>  tests/data/acpi/virt/PPTT.memhp    | Bin 0 -> 76 bytes
>>  tests/data/acpi/virt/PPTT.numamem  | Bin 0 -> 76 bytes
>>  tests/data/acpi/virt/PPTT.pxb      | Bin 0 -> 76 bytes
>>  tests/data/acpi/virt/PPTT.topology | Bin 0 -> 336 bytes
>>  tests/data/acpi/virt/SPCR.pxb      | Bin 0 -> 80 bytes
>>  tests/data/acpi/virt/SPCR.topology | Bin 0 -> 80 bytes
>>  tests/qtest/bios-tables-test.c     |  22 ++++++++++++++++++++++
>>  25 files changed, 28 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/data/acpi/virt/APIC.pxb
>>  create mode 100644 tests/data/acpi/virt/APIC.topology
>>  create mode 100644 tests/data/acpi/virt/DBG2.memhp
>>  create mode 100644 tests/data/acpi/virt/DBG2.numamem
>>  create mode 100644 tests/data/acpi/virt/DBG2.pxb
>>  create mode 100644 tests/data/acpi/virt/DBG2.topology
>>  create mode 100644 tests/data/acpi/virt/DSDT.topology
>>  create mode 100644 tests/data/acpi/virt/FACP.pxb
>>  create mode 100644 tests/data/acpi/virt/FACP.topology
>>  create mode 100644 tests/data/acpi/virt/GTDT.pxb
>>  create mode 100644 tests/data/acpi/virt/GTDT.topology
>>  create mode 100644 tests/data/acpi/virt/IORT.topology
>>  create mode 100644 tests/data/acpi/virt/MCFG.pxb
>>  create mode 100644 tests/data/acpi/virt/MCFG.topology
>>  create mode 100644 tests/data/acpi/virt/PPTT.memhp
>>  create mode 100644 tests/data/acpi/virt/PPTT.numamem
>>  create mode 100644 tests/data/acpi/virt/PPTT.pxb
>>  create mode 100644 tests/data/acpi/virt/PPTT.topology
>>  create mode 100644 tests/data/acpi/virt/SPCR.pxb
>>  create mode 100644 tests/data/acpi/virt/SPCR.topology
>>
>> -- 
>> 2.24.0
> 
> .
>