* [PATCH v6 0/7] ARM virt: Support CPU cluster topology
@ 2022-01-03 8:46 Yanan Wang via
2022-01-03 8:46 ` [PATCH v6 1/7] hw/arm/virt: Support CPU cluster on ARM virt machine Yanan Wang via
` (6 more replies)
0 siblings, 7 replies; 21+ messages in thread
From: Yanan Wang via @ 2022-01-03 8:46 UTC (permalink / raw)
To: qemu-devel, qemu-arm
Cc: Peter Maydell, Andrew Jones, Michael S . Tsirkin, Igor Mammedov,
Shannon Zhao, Ani Sinha, Eric Auger, wanghaibin.wang, Yanan Wang
This v6 series enables the support for CPU cluster topology on
ARM virt machines. The generic infrastructure for CPU cluster
parameter now is in upstream.
Background and descriptions:
The new Cluster-Aware Scheduling support has landed in Linux 5.16,
which has been proved to benefit the scheduling performance (e.g.
load balance and wake_affine strategy) for both x86_64 and AArch64.
We can see the PR [1] or the actual patch series [2] for reference.
So since Linux 5.16 we have four-level arch-neutral CPU topology
definition like below and a new scheduler level for clusters.
struct cpu_topology {
int thread_id;
int core_id;
int cluster_id;
int package_id;
int llc_id;
cpumask_t thread_sibling;
cpumask_t core_sibling;
cpumask_t cluster_sibling;
cpumask_t llc_sibling;
}
A cluster generally means a group of CPU cores which share L2 cache
or other mid-level resources, and it is the shared resources that
is used to improve scheduler's behavior. From the point of view of
the size range, it's between CPU die and CPU core. For example, on
some ARM64 Kunpeng servers, we have 6 clusters in each NUMA node,
and 4 CPU cores in each cluster. The 4 CPU cores share a separate
L2 cache and a L3 cache tag, which brings cache affinity advantage.
[1] https://lore.kernel.org/lkml/163572864855.3357115.17938524897008353101.tglx@xen13/
[2] https://lkml.org/lkml/2021/9/24/178
In virtualization, on the Hosts which have physical clusters, if we
can design a vCPU topology with cluster level for guest kernel and
have a dedicated vCPU pinning. A Cluster-Aware Guest kernel can also
make use of the cache affinity of CPU clusters to gain similar
scheduling performance. This series only enables clusters support
in the vCPU topology on ARM virt machines. We can also enable it
for other machine types in the future if needed.
The patches in this series do:
- Enable CPU cluster support on ARM virt machines, so that users
can specify a 4-level CPU hierarchy sockets/clusters/cores/threads.
And the 4-level topology will be described to guest kernel through
ACPI PPTT and DT cpu-map.
Changelog:
v5->v6:
- drop the generic part which is in upstream now
- rebased on latest master
- v5: https://patchew.org/QEMU/20211228092221.21068-1-wangyanan55@huawei.com/
Yanan Wang (7):
hw/arm/virt: Support CPU cluster on ARM virt machine
hw/arm/virt: Support cluster level in DT cpu-map
hw/acpi/aml-build: Improve scalability of PPTT generation
hw/arm/virt-acpi-build: Make an ARM specific PPTT generator
tests/acpi/bios-tables-test: Allow changes to virt/PPTT file
hw/arm/virt-acpi-build: Support cluster level in PPTT generation
tests/acpi/bios-table-test: Update expected virt/PPTT file
hw/acpi/aml-build.c | 66 ++------------------------
hw/arm/virt-acpi-build.c | 92 +++++++++++++++++++++++++++++++++++-
hw/arm/virt.c | 16 ++++---
include/hw/acpi/aml-build.h | 5 +-
qemu-options.hx | 10 ++++
tests/data/acpi/virt/PPTT | Bin 76 -> 96 bytes
6 files changed, 115 insertions(+), 74 deletions(-)
--
2.27.0
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v6 1/7] hw/arm/virt: Support CPU cluster on ARM virt machine
2022-01-03 8:46 [PATCH v6 0/7] ARM virt: Support CPU cluster topology Yanan Wang via
@ 2022-01-03 8:46 ` Yanan Wang via
2022-01-03 11:24 ` Andrew Jones
2022-01-03 8:46 ` [PATCH v6 2/7] hw/arm/virt: Support cluster level in DT cpu-map Yanan Wang via
` (5 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Yanan Wang via @ 2022-01-03 8:46 UTC (permalink / raw)
To: qemu-devel, qemu-arm
Cc: Peter Maydell, Andrew Jones, Michael S . Tsirkin, Igor Mammedov,
Shannon Zhao, Ani Sinha, Eric Auger, wanghaibin.wang, Yanan Wang
ARM64 machines like Kunpeng Family Server Chips have a level
of hardware topology in which a group of CPU cores share L3
cache tag or L2 cache. For example, Kunpeng 920 typically
has 6 or 8 clusters in each NUMA node (also represent range
of CPU die), and each cluster has 4 CPU cores. All clusters
share L3 cache data, but CPU cores in each cluster share a
local L3 tag.
Running a guest kernel with Cluster-Aware Scheduling on the
Hosts which have physical clusters, if we can design a vCPU
topology with cluster level for guest kernel and then have
a dedicated vCPU pinning, the guest will gain scheduling
performance improvement from cache affinity of CPU cluster.
So let's enable the support for this new parameter on ARM
virt machines. After this patch, we can define a 4-level
CPU hierarchy like: cpus=*,maxcpus=*,sockets=*,clusters=*,
cores=*,threads=*.
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
hw/arm/virt.c | 1 +
qemu-options.hx | 10 ++++++++++
2 files changed, 11 insertions(+)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 6bce595aba..f413e146d9 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2700,6 +2700,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
hc->unplug_request = virt_machine_device_unplug_request_cb;
hc->unplug = virt_machine_device_unplug_cb;
mc->nvdimm_supported = true;
+ mc->smp_props.clusters_supported = true;
mc->auto_enable_numa_with_memhp = true;
mc->auto_enable_numa_with_memdev = true;
mc->default_ram_id = "mach-virt.ram";
diff --git a/qemu-options.hx b/qemu-options.hx
index fd1f8135fb..69ef1cdb85 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -277,6 +277,16 @@ SRST
-smp 16,sockets=2,dies=2,cores=2,threads=2,maxcpus=16
+ The following sub-option defines a CPU topology hierarchy (2 sockets
+ totally on the machine, 2 clusters per socket, 2 cores per cluster,
+ 2 threads per core) for ARM virt machines which support sockets/clusters
+ /cores/threads. Some members of the option can be omitted but their values
+ will be automatically computed:
+
+ ::
+
+ -smp 16,sockets=2,clusters=2,cores=2,threads=2,maxcpus=16
+
Historically preference was given to the coarsest topology parameters
when computing missing values (ie sockets preferred over cores, which
were preferred over threads), however, this behaviour is considered
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 2/7] hw/arm/virt: Support cluster level in DT cpu-map
2022-01-03 8:46 [PATCH v6 0/7] ARM virt: Support CPU cluster topology Yanan Wang via
2022-01-03 8:46 ` [PATCH v6 1/7] hw/arm/virt: Support CPU cluster on ARM virt machine Yanan Wang via
@ 2022-01-03 8:46 ` Yanan Wang via
2022-01-03 11:25 ` Andrew Jones
2022-01-03 8:46 ` [PATCH v6 3/7] hw/acpi/aml-build: Improve scalability of PPTT generation Yanan Wang via
` (4 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Yanan Wang via @ 2022-01-03 8:46 UTC (permalink / raw)
To: qemu-devel, qemu-arm
Cc: Peter Maydell, Andrew Jones, Michael S . Tsirkin, Igor Mammedov,
Shannon Zhao, Ani Sinha, Eric Auger, wanghaibin.wang, Yanan Wang
Support one cluster level between core and physical package in the
cpu-map of Arm/virt devicetree. This is also consistent with Linux
Doc "Documentation/devicetree/bindings/cpu/cpu-topology.txt".
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
hw/arm/virt.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f413e146d9..fc5eea8c8c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -430,9 +430,8 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
* can contain several layers of clustering within a single physical
* package and cluster nodes can be contained in parent cluster nodes.
*
- * Given that cluster is not yet supported in the vCPU topology,
- * we currently generate one cluster node within each socket node
- * by default.
+ * Note: currently we only support one layer of clustering within
+ * each physical package.
*/
qemu_fdt_add_subnode(ms->fdt, "/cpus/cpu-map");
@@ -442,14 +441,16 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
if (ms->smp.threads > 1) {
map_path = g_strdup_printf(
- "/cpus/cpu-map/socket%d/cluster0/core%d/thread%d",
- cpu / (ms->smp.cores * ms->smp.threads),
+ "/cpus/cpu-map/socket%d/cluster%d/core%d/thread%d",
+ cpu / (ms->smp.clusters * ms->smp.cores * ms->smp.threads),
+ (cpu / (ms->smp.cores * ms->smp.threads)) % ms->smp.clusters,
(cpu / ms->smp.threads) % ms->smp.cores,
cpu % ms->smp.threads);
} else {
map_path = g_strdup_printf(
- "/cpus/cpu-map/socket%d/cluster0/core%d",
- cpu / ms->smp.cores,
+ "/cpus/cpu-map/socket%d/cluster%d/core%d",
+ cpu / (ms->smp.clusters * ms->smp.cores),
+ (cpu / ms->smp.cores) % ms->smp.clusters,
cpu % ms->smp.cores);
}
qemu_fdt_add_path(ms->fdt, map_path);
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 3/7] hw/acpi/aml-build: Improve scalability of PPTT generation
2022-01-03 8:46 [PATCH v6 0/7] ARM virt: Support CPU cluster topology Yanan Wang via
2022-01-03 8:46 ` [PATCH v6 1/7] hw/arm/virt: Support CPU cluster on ARM virt machine Yanan Wang via
2022-01-03 8:46 ` [PATCH v6 2/7] hw/arm/virt: Support cluster level in DT cpu-map Yanan Wang via
@ 2022-01-03 8:46 ` Yanan Wang via
2022-01-03 11:24 ` Andrew Jones
2022-01-03 8:46 ` [PATCH v6 4/7] hw/arm/virt-acpi-build: Make an ARM specific PPTT generator Yanan Wang via
` (3 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Yanan Wang via @ 2022-01-03 8:46 UTC (permalink / raw)
To: qemu-devel, qemu-arm
Cc: Peter Maydell, Andrew Jones, Michael S . Tsirkin, Igor Mammedov,
Shannon Zhao, Ani Sinha, Eric Auger, wanghaibin.wang, Yanan Wang
Currently we generate a PPTT table of n-level processor hierarchy
with n-level loops in build_pptt(). It works fine as now there are
only three CPU topology parameters. But the code may become less
scalable with the processor hierarchy levels increasing.
This patch only improves the scalability of build_pptt by reducing
the loops, and intends to make no functional change.
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
hw/acpi/aml-build.c | 50 +++++++++++++++++++++++++++++----------------
1 file changed, 32 insertions(+), 18 deletions(-)
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index b3b3310df3..be3851be36 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2001,7 +2001,10 @@ static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
const char *oem_id, const char *oem_table_id)
{
- int pptt_start = table_data->len;
+ GQueue *list = g_queue_new();
+ guint pptt_start = table_data->len;
+ guint father_offset;
+ guint length, i;
int uid = 0;
int socket;
AcpiTable table = { .sig = "PPTT", .rev = 2,
@@ -2010,9 +2013,8 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
acpi_table_begin(&table, table_data);
for (socket = 0; socket < ms->smp.sockets; socket++) {
- uint32_t socket_offset = table_data->len - pptt_start;
- int core;
-
+ g_queue_push_tail(list,
+ GUINT_TO_POINTER(table_data->len - pptt_start));
build_processor_hierarchy_node(
table_data,
/*
@@ -2021,35 +2023,47 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
*/
(1 << 0),
0, socket, NULL, 0);
+ }
- for (core = 0; core < ms->smp.cores; core++) {
- uint32_t core_offset = table_data->len - pptt_start;
- int thread;
+ length = g_queue_get_length(list);
+ for (i = 0; i < length; i++) {
+ int core;
+ father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
+ for (core = 0; core < ms->smp.cores; core++) {
if (ms->smp.threads > 1) {
+ g_queue_push_tail(list,
+ GUINT_TO_POINTER(table_data->len - pptt_start));
build_processor_hierarchy_node(
table_data,
(0 << 0), /* not a physical package */
- socket_offset, core, NULL, 0);
-
- for (thread = 0; thread < ms->smp.threads; thread++) {
- build_processor_hierarchy_node(
- table_data,
- (1 << 1) | /* ACPI Processor ID valid */
- (1 << 2) | /* Processor is a Thread */
- (1 << 3), /* Node is a Leaf */
- core_offset, uid++, NULL, 0);
- }
+ father_offset, core, NULL, 0);
} else {
build_processor_hierarchy_node(
table_data,
(1 << 1) | /* ACPI Processor ID valid */
(1 << 3), /* Node is a Leaf */
- socket_offset, uid++, NULL, 0);
+ father_offset, uid++, NULL, 0);
}
}
}
+ length = g_queue_get_length(list);
+ for (i = 0; i < length; i++) {
+ int thread;
+
+ father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
+ for (thread = 0; thread < ms->smp.threads; thread++) {
+ build_processor_hierarchy_node(
+ table_data,
+ (1 << 1) | /* ACPI Processor ID valid */
+ (1 << 2) | /* Processor is a Thread */
+ (1 << 3), /* Node is a Leaf */
+ father_offset, uid++, NULL, 0);
+ }
+ }
+
+ g_queue_free(list);
acpi_table_end(linker, &table);
}
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 4/7] hw/arm/virt-acpi-build: Make an ARM specific PPTT generator
2022-01-03 8:46 [PATCH v6 0/7] ARM virt: Support CPU cluster topology Yanan Wang via
` (2 preceding siblings ...)
2022-01-03 8:46 ` [PATCH v6 3/7] hw/acpi/aml-build: Improve scalability of PPTT generation Yanan Wang via
@ 2022-01-03 8:46 ` Yanan Wang via
2022-01-03 11:30 ` Andrew Jones
2022-01-03 8:46 ` [PATCH v6 5/7] tests/acpi/bios-tables-test: Allow changes to virt/PPTT file Yanan Wang via
` (2 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Yanan Wang via @ 2022-01-03 8:46 UTC (permalink / raw)
To: qemu-devel, qemu-arm
Cc: Peter Maydell, Andrew Jones, Michael S . Tsirkin, Igor Mammedov,
Shannon Zhao, Ani Sinha, Eric Auger, wanghaibin.wang, Yanan Wang
We have a generic build_pptt() in hw/acpi/aml-build.c but it's
currently only used in ARM acpi initialization. Now we are going
to support the new CPU cluster parameter which is currently only
supported by ARM, it won't be a very good idea to add it to the
generic build_pptt() as it will make the code complex and hard
to maintain especially when we also support CPU cache topology
hierarchy in build_pptt() too. Note that the cache topology
design also varies between different CPU targets.
So an ARM specific PPTT generator becomes necessary now. Given
that the generic one is currently only used by ARM, let's just
move build_pptt() from aml-build.c to virt-acpi-build.c with
minor update.
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
hw/acpi/aml-build.c | 80 ++-----------------------------------
hw/arm/virt-acpi-build.c | 77 ++++++++++++++++++++++++++++++++++-
include/hw/acpi/aml-build.h | 5 ++-
3 files changed, 81 insertions(+), 81 deletions(-)
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index be3851be36..040fbc9b4b 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1968,10 +1968,9 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
* ACPI spec, Revision 6.3
* 5.2.29.1 Processor hierarchy node structure (Type 0)
*/
-static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
- uint32_t parent, uint32_t id,
- uint32_t *priv_rsrc,
- uint32_t priv_num)
+void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
+ uint32_t parent, uint32_t id,
+ uint32_t *priv_rsrc, uint32_t priv_num)
{
int i;
@@ -1994,79 +1993,6 @@ static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
}
}
-/*
- * ACPI spec, Revision 6.3
- * 5.2.29 Processor Properties Topology Table (PPTT)
- */
-void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
- const char *oem_id, const char *oem_table_id)
-{
- GQueue *list = g_queue_new();
- guint pptt_start = table_data->len;
- guint father_offset;
- guint length, i;
- int uid = 0;
- int socket;
- AcpiTable table = { .sig = "PPTT", .rev = 2,
- .oem_id = oem_id, .oem_table_id = oem_table_id };
-
- acpi_table_begin(&table, table_data);
-
- for (socket = 0; socket < ms->smp.sockets; socket++) {
- g_queue_push_tail(list,
- GUINT_TO_POINTER(table_data->len - pptt_start));
- build_processor_hierarchy_node(
- table_data,
- /*
- * Physical package - represents the boundary
- * of a physical package
- */
- (1 << 0),
- 0, socket, NULL, 0);
- }
-
- length = g_queue_get_length(list);
- for (i = 0; i < length; i++) {
- int core;
-
- father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
- for (core = 0; core < ms->smp.cores; core++) {
- if (ms->smp.threads > 1) {
- g_queue_push_tail(list,
- GUINT_TO_POINTER(table_data->len - pptt_start));
- build_processor_hierarchy_node(
- table_data,
- (0 << 0), /* not a physical package */
- father_offset, core, NULL, 0);
- } else {
- build_processor_hierarchy_node(
- table_data,
- (1 << 1) | /* ACPI Processor ID valid */
- (1 << 3), /* Node is a Leaf */
- father_offset, uid++, NULL, 0);
- }
- }
- }
-
- length = g_queue_get_length(list);
- for (i = 0; i < length; i++) {
- int thread;
-
- father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
- for (thread = 0; thread < ms->smp.threads; thread++) {
- build_processor_hierarchy_node(
- table_data,
- (1 << 1) | /* ACPI Processor ID valid */
- (1 << 2) | /* Processor is a Thread */
- (1 << 3), /* Node is a Leaf */
- father_offset, uid++, NULL, 0);
- }
- }
-
- g_queue_free(list);
- acpi_table_end(linker, &table);
-}
-
/* build rev1/rev3/rev5.1 FADT */
void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
const char *oem_id, const char *oem_table_id)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index d0f4867fdf..3ce7680393 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -808,6 +808,80 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
acpi_table_end(linker, &table);
}
+/*
+ * ACPI spec, Revision 6.3
+ * 5.2.29 Processor Properties Topology Table (PPTT)
+ */
+static void
+build_pptt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
+{
+ MachineState *ms = MACHINE(vms);
+ GQueue *list = g_queue_new();
+ guint pptt_start = table_data->len;
+ guint father_offset;
+ guint length, i;
+ int uid = 0;
+ int socket;
+ AcpiTable table = { .sig = "PPTT", .rev = 2, .oem_id = vms->oem_id,
+ .oem_table_id = vms->oem_table_id };
+
+ acpi_table_begin(&table, table_data);
+
+ for (socket = 0; socket < ms->smp.sockets; socket++) {
+ g_queue_push_tail(list,
+ GUINT_TO_POINTER(table_data->len - pptt_start));
+ build_processor_hierarchy_node(
+ table_data,
+ /*
+ * Physical package - represents the boundary
+ * of a physical package
+ */
+ (1 << 0),
+ 0, socket, NULL, 0);
+ }
+
+ length = g_queue_get_length(list);
+ for (i = 0; i < length; i++) {
+ int core;
+
+ father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
+ for (core = 0; core < ms->smp.cores; core++) {
+ if (ms->smp.threads > 1) {
+ g_queue_push_tail(list,
+ GUINT_TO_POINTER(table_data->len - pptt_start));
+ build_processor_hierarchy_node(
+ table_data,
+ (0 << 0), /* not a physical package */
+ father_offset, core, NULL, 0);
+ } else {
+ build_processor_hierarchy_node(
+ table_data,
+ (1 << 1) | /* ACPI Processor ID valid */
+ (1 << 3), /* Node is a Leaf */
+ father_offset, uid++, NULL, 0);
+ }
+ }
+ }
+
+ length = g_queue_get_length(list);
+ for (i = 0; i < length; i++) {
+ int thread;
+
+ father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
+ for (thread = 0; thread < ms->smp.threads; thread++) {
+ build_processor_hierarchy_node(
+ table_data,
+ (1 << 1) | /* ACPI Processor ID valid */
+ (1 << 2) | /* Processor is a Thread */
+ (1 << 3), /* Node is a Leaf */
+ father_offset, uid++, NULL, 0);
+ }
+ }
+
+ g_queue_free(list);
+ acpi_table_end(linker, &table);
+}
+
/* FADT */
static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
VirtMachineState *vms, unsigned dsdt_tbl_offset)
@@ -953,8 +1027,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
if (!vmc->no_cpu_topology) {
acpi_add_table(table_offsets, tables_blob);
- build_pptt(tables_blob, tables->linker, ms,
- vms->oem_id, vms->oem_table_id);
+ build_pptt(tables_blob, tables->linker, vms);
}
acpi_add_table(table_offsets, tables_blob);
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 8346003a22..2c457c8f17 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -489,8 +489,9 @@ void build_srat_memory(GArray *table_data, uint64_t base,
void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
const char *oem_id, const char *oem_table_id);
-void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
- const char *oem_id, const char *oem_table_id);
+void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
+ uint32_t parent, uint32_t id,
+ uint32_t *priv_rsrc, uint32_t priv_num);
void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
const char *oem_id, const char *oem_table_id);
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 5/7] tests/acpi/bios-tables-test: Allow changes to virt/PPTT file
2022-01-03 8:46 [PATCH v6 0/7] ARM virt: Support CPU cluster topology Yanan Wang via
` (3 preceding siblings ...)
2022-01-03 8:46 ` [PATCH v6 4/7] hw/arm/virt-acpi-build: Make an ARM specific PPTT generator Yanan Wang via
@ 2022-01-03 8:46 ` Yanan Wang via
2022-01-03 11:51 ` Ani Sinha
2022-01-03 8:46 ` [PATCH v6 6/7] hw/arm/virt-acpi-build: Support cluster level in PPTT generation Yanan Wang via
2022-01-03 8:46 ` [PATCH v6 7/7] tests/acpi/bios-table-test: Update expected virt/PPTT file Yanan Wang via
6 siblings, 1 reply; 21+ messages in thread
From: Yanan Wang via @ 2022-01-03 8:46 UTC (permalink / raw)
To: qemu-devel, qemu-arm
Cc: Peter Maydell, Andrew Jones, Michael S . Tsirkin, Igor Mammedov,
Shannon Zhao, Ani Sinha, Eric Auger, wanghaibin.wang, Yanan Wang
List test/data/acpi/virt/PPTT as the expected files allowed to
be changed in tests/qtest/bios-tables-test-allowed-diff.h
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
tests/qtest/bios-tables-test-allowed-diff.h | 1 +
1 file changed, 1 insertion(+)
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",
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 6/7] hw/arm/virt-acpi-build: Support cluster level in PPTT generation
2022-01-03 8:46 [PATCH v6 0/7] ARM virt: Support CPU cluster topology Yanan Wang via
` (4 preceding siblings ...)
2022-01-03 8:46 ` [PATCH v6 5/7] tests/acpi/bios-tables-test: Allow changes to virt/PPTT file Yanan Wang via
@ 2022-01-03 8:46 ` Yanan Wang via
2022-01-03 11:32 ` Andrew Jones
2022-01-03 8:46 ` [PATCH v6 7/7] tests/acpi/bios-table-test: Update expected virt/PPTT file Yanan Wang via
6 siblings, 1 reply; 21+ messages in thread
From: Yanan Wang via @ 2022-01-03 8:46 UTC (permalink / raw)
To: qemu-devel, qemu-arm
Cc: Peter Maydell, Andrew Jones, Michael S . Tsirkin, Igor Mammedov,
Shannon Zhao, Ani Sinha, Eric Auger, wanghaibin.wang, Yanan Wang
Support cluster level in generation of ACPI Processor Properties
Topology Table (PPTT) for ARM virt machines.
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
hw/arm/virt-acpi-build.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 3ce7680393..5f91969688 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -840,6 +840,21 @@ build_pptt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
0, socket, NULL, 0);
}
+ length = g_queue_get_length(list);
+ for (i = 0; i < length; i++) {
+ int cluster;
+
+ father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
+ for (cluster = 0; cluster < ms->smp.clusters; cluster++) {
+ g_queue_push_tail(list,
+ GUINT_TO_POINTER(table_data->len - pptt_start));
+ build_processor_hierarchy_node(
+ table_data,
+ (0 << 0), /* not a physical package */
+ father_offset, cluster, NULL, 0);
+ }
+ }
+
length = g_queue_get_length(list);
for (i = 0; i < length; i++) {
int core;
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 7/7] tests/acpi/bios-table-test: Update expected virt/PPTT file
2022-01-03 8:46 [PATCH v6 0/7] ARM virt: Support CPU cluster topology Yanan Wang via
` (5 preceding siblings ...)
2022-01-03 8:46 ` [PATCH v6 6/7] hw/arm/virt-acpi-build: Support cluster level in PPTT generation Yanan Wang via
@ 2022-01-03 8:46 ` Yanan Wang via
2022-01-03 12:01 ` Ani Sinha
6 siblings, 1 reply; 21+ messages in thread
From: Yanan Wang via @ 2022-01-03 8:46 UTC (permalink / raw)
To: qemu-devel, qemu-arm
Cc: Peter Maydell, Andrew Jones, Michael S . Tsirkin, Igor Mammedov,
Shannon Zhao, Ani Sinha, Eric Auger, wanghaibin.wang, Yanan Wang
Run ./tests/data/acpi/rebuild-expected-aml.sh from build directory
to update PPTT binary. Also empty bios-tables-test-allowed-diff.h.
The disassembled differences between actual and expected PPTT:
/*
* Intel ACPI Component Architecture
* AML/ASL+ Disassembler version 20180810 (64-bit version)
* Copyright (c) 2000 - 2018 Intel Corporation
*
- * Disassembly of tests/data/acpi/virt/PPTT, Mon Oct 25 20:24:53 2021
+ * Disassembly of /tmp/aml-BPI5B1, Mon Oct 25 20:24:53 2021
*
* ACPI Data Table [PPTT]
*
* Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue
*/
[000h 0000 4] Signature : "PPTT" [Processor Properties Topology Table]
-[004h 0004 4] Table Length : 0000004C
+[004h 0004 4] Table Length : 00000060
[008h 0008 1] Revision : 02
-[009h 0009 1] Checksum : A8
+[009h 0009 1] Checksum : 48
[00Ah 0010 6] Oem ID : "BOCHS "
[010h 0016 8] Oem Table ID : "BXPC "
[018h 0024 4] Oem Revision : 00000001
[01Ch 0028 4] Asl Compiler ID : "BXPC"
[020h 0032 4] Asl Compiler Revision : 00000001
[024h 0036 1] Subtable Type : 00 [Processor Hierarchy Node]
[025h 0037 1] Length : 14
[026h 0038 2] Reserved : 0000
[028h 0040 4] Flags (decoded below) : 00000001
Physical package : 1
ACPI Processor ID valid : 0
[02Ch 0044 4] Parent : 00000000
[030h 0048 4] ACPI Processor ID : 00000000
[034h 0052 4] Private Resource Number : 00000000
[038h 0056 1] Subtable Type : 00 [Processor Hierarchy Node]
[039h 0057 1] Length : 14
[03Ah 0058 2] Reserved : 0000
-[03Ch 0060 4] Flags (decoded below) : 0000000A
+[03Ch 0060 4] Flags (decoded below) : 00000000
Physical package : 0
- ACPI Processor ID valid : 1
+ ACPI Processor ID valid : 0
[040h 0064 4] Parent : 00000024
[044h 0068 4] ACPI Processor ID : 00000000
[048h 0072 4] Private Resource Number : 00000000
-Raw Table Data: Length 76 (0x4C)
+[04Ch 0076 1] Subtable Type : 00 [Processor Hierarchy Node]
+[04Dh 0077 1] Length : 14
+[04Eh 0078 2] Reserved : 0000
+[050h 0080 4] Flags (decoded below) : 0000000A
+ Physical package : 0
+ ACPI Processor ID valid : 1
+[054h 0084 4] Parent : 00000038
+[058h 0088 4] ACPI Processor ID : 00000000
+[05Ch 0092 4] Private Resource Number : 00000000
+
+Raw Table Data: Length 96 (0x60)
- 0000: 50 50 54 54 4C 00 00 00 02 A8 42 4F 43 48 53 20 // PPTTL.....BOCHS
+ 0000: 50 50 54 54 60 00 00 00 02 48 42 4F 43 48 53 20 // PPTT`....HBOCHS
0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
0020: 01 00 00 00 00 14 00 00 01 00 00 00 00 00 00 00 // ................
- 0030: 00 00 00 00 00 00 00 00 00 14 00 00 0A 00 00 00 // ................
- 0040: 24 00 00 00 00 00 00 00 00 00 00 00 // $...........
+ 0030: 00 00 00 00 00 00 00 00 00 14 00 00 00 00 00 00 // ................
+ 0040: 24 00 00 00 00 00 00 00 00 00 00 00 00 14 00 00 // $...............
+ 0050: 0A 00 00 00 38 00 00 00 00 00 00 00 00 00 00 00 // ....8...........
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
tests/data/acpi/virt/PPTT | Bin 76 -> 96 bytes
tests/qtest/bios-tables-test-allowed-diff.h | 1 -
2 files changed, 1 deletion(-)
diff --git a/tests/data/acpi/virt/PPTT b/tests/data/acpi/virt/PPTT
index 7a1258ecf123555b24462c98ccbb76b4ac1d0c2b..f56ea63b369a604877374ad696c396e796ab1c83 100644
GIT binary patch
delta 53
zcmV-50LuSNU<y!BR8(L90006=kqR;-00000Bme*a000000000002BZK3IG5AH~;_u
L0000000000uCW9Z
delta 32
qcmV+*0N?*$ObSp?R8&j=00080kqR=APy`Gl00000000000001OcLdh}
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index cb143a55a6..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,2 +1 @@
/* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/virt/PPTT",
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v6 3/7] hw/acpi/aml-build: Improve scalability of PPTT generation
2022-01-03 8:46 ` [PATCH v6 3/7] hw/acpi/aml-build: Improve scalability of PPTT generation Yanan Wang via
@ 2022-01-03 11:24 ` Andrew Jones
2022-01-04 2:05 ` wangyanan (Y) via
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Jones @ 2022-01-03 11:24 UTC (permalink / raw)
To: Yanan Wang
Cc: Peter Maydell, Michael S . Tsirkin, qemu-devel, Shannon Zhao,
qemu-arm, Eric Auger, wanghaibin.wang, Ani Sinha, Igor Mammedov
On Mon, Jan 03, 2022 at 04:46:32PM +0800, Yanan Wang wrote:
> Currently we generate a PPTT table of n-level processor hierarchy
> with n-level loops in build_pptt(). It works fine as now there are
> only three CPU topology parameters. But the code may become less
> scalable with the processor hierarchy levels increasing.
>
> This patch only improves the scalability of build_pptt by reducing
> the loops, and intends to make no functional change.
>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
> hw/acpi/aml-build.c | 50 +++++++++++++++++++++++++++++----------------
> 1 file changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index b3b3310df3..be3851be36 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2001,7 +2001,10 @@ static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
> void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> const char *oem_id, const char *oem_table_id)
> {
> - int pptt_start = table_data->len;
> + GQueue *list = g_queue_new();
> + guint pptt_start = table_data->len;
> + guint father_offset;
"parent_offset" would be more conventional.
> + guint length, i;
> int uid = 0;
> int socket;
> AcpiTable table = { .sig = "PPTT", .rev = 2,
> @@ -2010,9 +2013,8 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> acpi_table_begin(&table, table_data);
>
> for (socket = 0; socket < ms->smp.sockets; socket++) {
> - uint32_t socket_offset = table_data->len - pptt_start;
> - int core;
> -
> + g_queue_push_tail(list,
> + GUINT_TO_POINTER(table_data->len - pptt_start));
> build_processor_hierarchy_node(
> table_data,
> /*
> @@ -2021,35 +2023,47 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> */
> (1 << 0),
> 0, socket, NULL, 0);
> + }
>
> - for (core = 0; core < ms->smp.cores; core++) {
> - uint32_t core_offset = table_data->len - pptt_start;
> - int thread;
> + length = g_queue_get_length(list);
> + for (i = 0; i < length; i++) {
> + int core;
>
> + father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
> + for (core = 0; core < ms->smp.cores; core++) {
> if (ms->smp.threads > 1) {
> + g_queue_push_tail(list,
> + GUINT_TO_POINTER(table_data->len - pptt_start));
> build_processor_hierarchy_node(
> table_data,
> (0 << 0), /* not a physical package */
> - socket_offset, core, NULL, 0);
> -
> - for (thread = 0; thread < ms->smp.threads; thread++) {
> - build_processor_hierarchy_node(
> - table_data,
> - (1 << 1) | /* ACPI Processor ID valid */
> - (1 << 2) | /* Processor is a Thread */
> - (1 << 3), /* Node is a Leaf */
> - core_offset, uid++, NULL, 0);
> - }
> + father_offset, core, NULL, 0);
> } else {
> build_processor_hierarchy_node(
> table_data,
> (1 << 1) | /* ACPI Processor ID valid */
> (1 << 3), /* Node is a Leaf */
> - socket_offset, uid++, NULL, 0);
> + father_offset, uid++, NULL, 0);
> }
> }
> }
>
> + length = g_queue_get_length(list);
> + for (i = 0; i < length; i++) {
> + int thread;
> +
> + father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
> + for (thread = 0; thread < ms->smp.threads; thread++) {
> + build_processor_hierarchy_node(
> + table_data,
> + (1 << 1) | /* ACPI Processor ID valid */
> + (1 << 2) | /* Processor is a Thread */
> + (1 << 3), /* Node is a Leaf */
> + father_offset, uid++, NULL, 0);
> + }
> + }
> +
> + g_queue_free(list);
> acpi_table_end(linker, &table);
> }
This patch actually increases the number of loops, since we need to visit
higher hierarchical nodes twice (once to enqueue and once to dequeue). We
do reduce code indentation and it looks like we could more easily skip
hierarchy levels we don't want, though. While my impulse is to say we
should just keep this simple and add another nested loop for clusters, I
guess I'm OK with this too.
Reviewed-by: Andrew Jones <drjones@redhat.com>
Thanks,
drew
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 1/7] hw/arm/virt: Support CPU cluster on ARM virt machine
2022-01-03 8:46 ` [PATCH v6 1/7] hw/arm/virt: Support CPU cluster on ARM virt machine Yanan Wang via
@ 2022-01-03 11:24 ` Andrew Jones
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Jones @ 2022-01-03 11:24 UTC (permalink / raw)
To: Yanan Wang
Cc: Peter Maydell, Michael S . Tsirkin, qemu-devel, Shannon Zhao,
qemu-arm, Eric Auger, wanghaibin.wang, Ani Sinha, Igor Mammedov
On Mon, Jan 03, 2022 at 04:46:30PM +0800, Yanan Wang wrote:
> ARM64 machines like Kunpeng Family Server Chips have a level
> of hardware topology in which a group of CPU cores share L3
> cache tag or L2 cache. For example, Kunpeng 920 typically
> has 6 or 8 clusters in each NUMA node (also represent range
> of CPU die), and each cluster has 4 CPU cores. All clusters
> share L3 cache data, but CPU cores in each cluster share a
> local L3 tag.
>
> Running a guest kernel with Cluster-Aware Scheduling on the
> Hosts which have physical clusters, if we can design a vCPU
> topology with cluster level for guest kernel and then have
> a dedicated vCPU pinning, the guest will gain scheduling
> performance improvement from cache affinity of CPU cluster.
>
> So let's enable the support for this new parameter on ARM
> virt machines. After this patch, we can define a 4-level
> CPU hierarchy like: cpus=*,maxcpus=*,sockets=*,clusters=*,
> cores=*,threads=*.
>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
> hw/arm/virt.c | 1 +
> qemu-options.hx | 10 ++++++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 6bce595aba..f413e146d9 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2700,6 +2700,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
> hc->unplug_request = virt_machine_device_unplug_request_cb;
> hc->unplug = virt_machine_device_unplug_cb;
> mc->nvdimm_supported = true;
> + mc->smp_props.clusters_supported = true;
> mc->auto_enable_numa_with_memhp = true;
> mc->auto_enable_numa_with_memdev = true;
> mc->default_ram_id = "mach-virt.ram";
> diff --git a/qemu-options.hx b/qemu-options.hx
> index fd1f8135fb..69ef1cdb85 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -277,6 +277,16 @@ SRST
>
> -smp 16,sockets=2,dies=2,cores=2,threads=2,maxcpus=16
>
> + The following sub-option defines a CPU topology hierarchy (2 sockets
> + totally on the machine, 2 clusters per socket, 2 cores per cluster,
> + 2 threads per core) for ARM virt machines which support sockets/clusters
> + /cores/threads. Some members of the option can be omitted but their values
> + will be automatically computed:
> +
> + ::
> +
> + -smp 16,sockets=2,clusters=2,cores=2,threads=2,maxcpus=16
> +
> Historically preference was given to the coarsest topology parameters
> when computing missing values (ie sockets preferred over cores, which
> were preferred over threads), however, this behaviour is considered
> --
> 2.27.0
>
Reviewed-by: Andrew Jones <drjones@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 2/7] hw/arm/virt: Support cluster level in DT cpu-map
2022-01-03 8:46 ` [PATCH v6 2/7] hw/arm/virt: Support cluster level in DT cpu-map Yanan Wang via
@ 2022-01-03 11:25 ` Andrew Jones
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Jones @ 2022-01-03 11:25 UTC (permalink / raw)
To: Yanan Wang
Cc: Peter Maydell, Michael S . Tsirkin, qemu-devel, Shannon Zhao,
qemu-arm, Eric Auger, wanghaibin.wang, Ani Sinha, Igor Mammedov
On Mon, Jan 03, 2022 at 04:46:31PM +0800, Yanan Wang wrote:
> Support one cluster level between core and physical package in the
> cpu-map of Arm/virt devicetree. This is also consistent with Linux
> Doc "Documentation/devicetree/bindings/cpu/cpu-topology.txt".
>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
> hw/arm/virt.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f413e146d9..fc5eea8c8c 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -430,9 +430,8 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
> * can contain several layers of clustering within a single physical
> * package and cluster nodes can be contained in parent cluster nodes.
> *
> - * Given that cluster is not yet supported in the vCPU topology,
> - * we currently generate one cluster node within each socket node
> - * by default.
> + * Note: currently we only support one layer of clustering within
> + * each physical package.
> */
> qemu_fdt_add_subnode(ms->fdt, "/cpus/cpu-map");
>
> @@ -442,14 +441,16 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
>
> if (ms->smp.threads > 1) {
> map_path = g_strdup_printf(
> - "/cpus/cpu-map/socket%d/cluster0/core%d/thread%d",
> - cpu / (ms->smp.cores * ms->smp.threads),
> + "/cpus/cpu-map/socket%d/cluster%d/core%d/thread%d",
> + cpu / (ms->smp.clusters * ms->smp.cores * ms->smp.threads),
> + (cpu / (ms->smp.cores * ms->smp.threads)) % ms->smp.clusters,
> (cpu / ms->smp.threads) % ms->smp.cores,
> cpu % ms->smp.threads);
> } else {
> map_path = g_strdup_printf(
> - "/cpus/cpu-map/socket%d/cluster0/core%d",
> - cpu / ms->smp.cores,
> + "/cpus/cpu-map/socket%d/cluster%d/core%d",
> + cpu / (ms->smp.clusters * ms->smp.cores),
> + (cpu / ms->smp.cores) % ms->smp.clusters,
> cpu % ms->smp.cores);
> }
> qemu_fdt_add_path(ms->fdt, map_path);
> --
> 2.27.0
>
Reviewed-by: Andrew Jones <drjones@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 4/7] hw/arm/virt-acpi-build: Make an ARM specific PPTT generator
2022-01-03 8:46 ` [PATCH v6 4/7] hw/arm/virt-acpi-build: Make an ARM specific PPTT generator Yanan Wang via
@ 2022-01-03 11:30 ` Andrew Jones
2022-01-04 2:06 ` wangyanan (Y) via
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Jones @ 2022-01-03 11:30 UTC (permalink / raw)
To: Yanan Wang
Cc: Peter Maydell, Michael S . Tsirkin, qemu-devel, Shannon Zhao,
qemu-arm, Eric Auger, wanghaibin.wang, Ani Sinha, Igor Mammedov
On Mon, Jan 03, 2022 at 04:46:33PM +0800, Yanan Wang wrote:
> We have a generic build_pptt() in hw/acpi/aml-build.c but it's
> currently only used in ARM acpi initialization. Now we are going
> to support the new CPU cluster parameter which is currently only
> supported by ARM, it won't be a very good idea to add it to the
> generic build_pptt() as it will make the code complex and hard
> to maintain especially when we also support CPU cache topology
> hierarchy in build_pptt() too. Note that the cache topology
> design also varies between different CPU targets.
It's a shame to remove generic ACPI table. I'm not sure what
will be best when addressing the cache topology, but for the
clusters it seems like we should be able to easily skip them
when has_clusters is false.
>
> So an ARM specific PPTT generator becomes necessary now. Given
> that the generic one is currently only used by ARM, let's just
> move build_pptt() from aml-build.c to virt-acpi-build.c with
> minor update.
Please state what the minor update is? I see the oem parameter
change, but I want to be sure that's the one you're referring
to.
>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
> hw/acpi/aml-build.c | 80 ++-----------------------------------
> hw/arm/virt-acpi-build.c | 77 ++++++++++++++++++++++++++++++++++-
> include/hw/acpi/aml-build.h | 5 ++-
> 3 files changed, 81 insertions(+), 81 deletions(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index be3851be36..040fbc9b4b 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1968,10 +1968,9 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> * ACPI spec, Revision 6.3
> * 5.2.29.1 Processor hierarchy node structure (Type 0)
> */
> -static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
> - uint32_t parent, uint32_t id,
> - uint32_t *priv_rsrc,
> - uint32_t priv_num)
> +void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
> + uint32_t parent, uint32_t id,
> + uint32_t *priv_rsrc, uint32_t priv_num)
> {
> int i;
>
> @@ -1994,79 +1993,6 @@ static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
> }
> }
>
> -/*
> - * ACPI spec, Revision 6.3
> - * 5.2.29 Processor Properties Topology Table (PPTT)
> - */
> -void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> - const char *oem_id, const char *oem_table_id)
> -{
> - GQueue *list = g_queue_new();
> - guint pptt_start = table_data->len;
> - guint father_offset;
> - guint length, i;
> - int uid = 0;
> - int socket;
> - AcpiTable table = { .sig = "PPTT", .rev = 2,
> - .oem_id = oem_id, .oem_table_id = oem_table_id };
> -
> - acpi_table_begin(&table, table_data);
> -
> - for (socket = 0; socket < ms->smp.sockets; socket++) {
> - g_queue_push_tail(list,
> - GUINT_TO_POINTER(table_data->len - pptt_start));
> - build_processor_hierarchy_node(
> - table_data,
> - /*
> - * Physical package - represents the boundary
> - * of a physical package
> - */
> - (1 << 0),
> - 0, socket, NULL, 0);
> - }
> -
> - length = g_queue_get_length(list);
> - for (i = 0; i < length; i++) {
> - int core;
> -
> - father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
> - for (core = 0; core < ms->smp.cores; core++) {
> - if (ms->smp.threads > 1) {
> - g_queue_push_tail(list,
> - GUINT_TO_POINTER(table_data->len - pptt_start));
> - build_processor_hierarchy_node(
> - table_data,
> - (0 << 0), /* not a physical package */
> - father_offset, core, NULL, 0);
> - } else {
> - build_processor_hierarchy_node(
> - table_data,
> - (1 << 1) | /* ACPI Processor ID valid */
> - (1 << 3), /* Node is a Leaf */
> - father_offset, uid++, NULL, 0);
> - }
> - }
> - }
> -
> - length = g_queue_get_length(list);
> - for (i = 0; i < length; i++) {
> - int thread;
> -
> - father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
> - for (thread = 0; thread < ms->smp.threads; thread++) {
> - build_processor_hierarchy_node(
> - table_data,
> - (1 << 1) | /* ACPI Processor ID valid */
> - (1 << 2) | /* Processor is a Thread */
> - (1 << 3), /* Node is a Leaf */
> - father_offset, uid++, NULL, 0);
> - }
> - }
> -
> - g_queue_free(list);
> - acpi_table_end(linker, &table);
> -}
> -
> /* build rev1/rev3/rev5.1 FADT */
> void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
> const char *oem_id, const char *oem_table_id)
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index d0f4867fdf..3ce7680393 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -808,6 +808,80 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> acpi_table_end(linker, &table);
> }
>
> +/*
> + * ACPI spec, Revision 6.3
> + * 5.2.29 Processor Properties Topology Table (PPTT)
> + */
> +static void
> +build_pptt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> +{
> + MachineState *ms = MACHINE(vms);
> + GQueue *list = g_queue_new();
> + guint pptt_start = table_data->len;
> + guint father_offset;
> + guint length, i;
> + int uid = 0;
> + int socket;
> + AcpiTable table = { .sig = "PPTT", .rev = 2, .oem_id = vms->oem_id,
> + .oem_table_id = vms->oem_table_id };
> +
> + acpi_table_begin(&table, table_data);
> +
> + for (socket = 0; socket < ms->smp.sockets; socket++) {
> + g_queue_push_tail(list,
> + GUINT_TO_POINTER(table_data->len - pptt_start));
> + build_processor_hierarchy_node(
> + table_data,
> + /*
> + * Physical package - represents the boundary
> + * of a physical package
> + */
> + (1 << 0),
> + 0, socket, NULL, 0);
> + }
> +
> + length = g_queue_get_length(list);
> + for (i = 0; i < length; i++) {
> + int core;
> +
> + father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
> + for (core = 0; core < ms->smp.cores; core++) {
> + if (ms->smp.threads > 1) {
> + g_queue_push_tail(list,
> + GUINT_TO_POINTER(table_data->len - pptt_start));
> + build_processor_hierarchy_node(
> + table_data,
> + (0 << 0), /* not a physical package */
> + father_offset, core, NULL, 0);
> + } else {
> + build_processor_hierarchy_node(
> + table_data,
> + (1 << 1) | /* ACPI Processor ID valid */
> + (1 << 3), /* Node is a Leaf */
> + father_offset, uid++, NULL, 0);
> + }
> + }
> + }
> +
> + length = g_queue_get_length(list);
> + for (i = 0; i < length; i++) {
> + int thread;
> +
> + father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
> + for (thread = 0; thread < ms->smp.threads; thread++) {
> + build_processor_hierarchy_node(
> + table_data,
> + (1 << 1) | /* ACPI Processor ID valid */
> + (1 << 2) | /* Processor is a Thread */
> + (1 << 3), /* Node is a Leaf */
> + father_offset, uid++, NULL, 0);
> + }
> + }
> +
> + g_queue_free(list);
> + acpi_table_end(linker, &table);
> +}
> +
> /* FADT */
> static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms, unsigned dsdt_tbl_offset)
> @@ -953,8 +1027,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>
> if (!vmc->no_cpu_topology) {
> acpi_add_table(table_offsets, tables_blob);
> - build_pptt(tables_blob, tables->linker, ms,
> - vms->oem_id, vms->oem_table_id);
> + build_pptt(tables_blob, tables->linker, vms);
> }
>
> acpi_add_table(table_offsets, tables_blob);
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 8346003a22..2c457c8f17 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -489,8 +489,9 @@ void build_srat_memory(GArray *table_data, uint64_t base,
> void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> const char *oem_id, const char *oem_table_id);
>
> -void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> - const char *oem_id, const char *oem_table_id);
> +void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
> + uint32_t parent, uint32_t id,
> + uint32_t *priv_rsrc, uint32_t priv_num);
>
> void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
> const char *oem_id, const char *oem_table_id);
> --
> 2.27.0
>
Thanks,
drew
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 6/7] hw/arm/virt-acpi-build: Support cluster level in PPTT generation
2022-01-03 8:46 ` [PATCH v6 6/7] hw/arm/virt-acpi-build: Support cluster level in PPTT generation Yanan Wang via
@ 2022-01-03 11:32 ` Andrew Jones
2022-01-04 2:15 ` wangyanan (Y) via
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Jones @ 2022-01-03 11:32 UTC (permalink / raw)
To: Yanan Wang
Cc: Peter Maydell, Michael S . Tsirkin, qemu-devel, Shannon Zhao,
qemu-arm, Eric Auger, wanghaibin.wang, Ani Sinha, Igor Mammedov
On Mon, Jan 03, 2022 at 04:46:35PM +0800, Yanan Wang wrote:
> Support cluster level in generation of ACPI Processor Properties
> Topology Table (PPTT) for ARM virt machines.
>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
> hw/arm/virt-acpi-build.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 3ce7680393..5f91969688 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -840,6 +840,21 @@ build_pptt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> 0, socket, NULL, 0);
> }
>
> + length = g_queue_get_length(list);
> + for (i = 0; i < length; i++) {
> + int cluster;
> +
> + father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
> + for (cluster = 0; cluster < ms->smp.clusters; cluster++) {
> + g_queue_push_tail(list,
> + GUINT_TO_POINTER(table_data->len - pptt_start));
> + build_processor_hierarchy_node(
> + table_data,
> + (0 << 0), /* not a physical package */
> + father_offset, cluster, NULL, 0);
> + }
> + }
> +
> length = g_queue_get_length(list);
> for (i = 0; i < length; i++) {
> int core;
> --
> 2.27.0
>
Looks good except please do s/father_offset/parent_offset/ as I mentioned
in an earlier patch.
Reviewed-by: Andrew Jones <drjones@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 5/7] tests/acpi/bios-tables-test: Allow changes to virt/PPTT file
2022-01-03 8:46 ` [PATCH v6 5/7] tests/acpi/bios-tables-test: Allow changes to virt/PPTT file Yanan Wang via
@ 2022-01-03 11:51 ` Ani Sinha
0 siblings, 0 replies; 21+ messages in thread
From: Ani Sinha @ 2022-01-03 11:51 UTC (permalink / raw)
To: Yanan Wang
Cc: Peter Maydell, Andrew Jones, Michael S . Tsirkin, qemu-devel,
Shannon Zhao, qemu-arm, Eric Auger, wanghaibin.wang, Ani Sinha,
Igor Mammedov
On Mon, 3 Jan 2022, Yanan Wang wrote:
> List test/data/acpi/virt/PPTT as the expected files allowed to
> be changed in tests/qtest/bios-tables-test-allowed-diff.h
>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
Acked-by: Ani Sinha <ani@anisinha.ca>
> ---
> tests/qtest/bios-tables-test-allowed-diff.h | 1 +
> 1 file changed, 1 insertion(+)
>
> 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",
> --
> 2.27.0
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 7/7] tests/acpi/bios-table-test: Update expected virt/PPTT file
2022-01-03 8:46 ` [PATCH v6 7/7] tests/acpi/bios-table-test: Update expected virt/PPTT file Yanan Wang via
@ 2022-01-03 12:01 ` Ani Sinha
2022-01-04 2:28 ` wangyanan (Y) via
0 siblings, 1 reply; 21+ messages in thread
From: Ani Sinha @ 2022-01-03 12:01 UTC (permalink / raw)
To: Yanan Wang
Cc: Peter Maydell, Andrew Jones, Michael S . Tsirkin, qemu-devel,
Shannon Zhao, qemu-arm, Eric Auger, wanghaibin.wang, Ani Sinha,
Igor Mammedov
On Mon, 3 Jan 2022, Yanan Wang wrote:
> Run ./tests/data/acpi/rebuild-expected-aml.sh from build directory
> to update PPTT binary. Also empty bios-tables-test-allowed-diff.h.
>
> The disassembled differences between actual and expected PPTT:
>
> /*
> * Intel ACPI Component Architecture
> * AML/ASL+ Disassembler version 20180810 (64-bit version)
> * Copyright (c) 2000 - 2018 Intel Corporation
> *
> - * Disassembly of tests/data/acpi/virt/PPTT, Mon Oct 25 20:24:53 2021
> + * Disassembly of /tmp/aml-BPI5B1, Mon Oct 25 20:24:53 2021
> *
> * ACPI Data Table [PPTT]
> *
> * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue
> */
>
> [000h 0000 4] Signature : "PPTT" [Processor Properties Topology Table]
> -[004h 0004 4] Table Length : 0000004C
> +[004h 0004 4] Table Length : 00000060
> [008h 0008 1] Revision : 02
> -[009h 0009 1] Checksum : A8
> +[009h 0009 1] Checksum : 48
> [00Ah 0010 6] Oem ID : "BOCHS "
> [010h 0016 8] Oem Table ID : "BXPC "
> [018h 0024 4] Oem Revision : 00000001
> [01Ch 0028 4] Asl Compiler ID : "BXPC"
> [020h 0032 4] Asl Compiler Revision : 00000001
>
> [024h 0036 1] Subtable Type : 00 [Processor Hierarchy Node]
> [025h 0037 1] Length : 14
> [026h 0038 2] Reserved : 0000
> [028h 0040 4] Flags (decoded below) : 00000001
> Physical package : 1
> ACPI Processor ID valid : 0
> [02Ch 0044 4] Parent : 00000000
> [030h 0048 4] ACPI Processor ID : 00000000
> [034h 0052 4] Private Resource Number : 00000000
>
> [038h 0056 1] Subtable Type : 00 [Processor Hierarchy Node]
> [039h 0057 1] Length : 14
> [03Ah 0058 2] Reserved : 0000
> -[03Ch 0060 4] Flags (decoded below) : 0000000A
> +[03Ch 0060 4] Flags (decoded below) : 00000000
> Physical package : 0
> - ACPI Processor ID valid : 1
> + ACPI Processor ID valid : 0
I do not know this very well but does the above two changes (flags and
processor ID) makes sense?
> [040h 0064 4] Parent : 00000024
> [044h 0068 4] ACPI Processor ID : 00000000
> [048h 0072 4] Private Resource Number : 00000000
>
> -Raw Table Data: Length 76 (0x4C)
> +[04Ch 0076 1] Subtable Type : 00 [Processor Hierarchy Node]
> +[04Dh 0077 1] Length : 14
> +[04Eh 0078 2] Reserved : 0000
> +[050h 0080 4] Flags (decoded below) : 0000000A
> + Physical package : 0
> + ACPI Processor ID valid : 1
> +[054h 0084 4] Parent : 00000038
> +[058h 0088 4] ACPI Processor ID : 00000000
> +[05Ch 0092 4] Private Resource Number : 00000000
> +
> +Raw Table Data: Length 96 (0x60)
>
> - 0000: 50 50 54 54 4C 00 00 00 02 A8 42 4F 43 48 53 20 // PPTTL.....BOCHS
> + 0000: 50 50 54 54 60 00 00 00 02 48 42 4F 43 48 53 20 // PPTT`....HBOCHS
> 0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
> 0020: 01 00 00 00 00 14 00 00 01 00 00 00 00 00 00 00 // ................
> - 0030: 00 00 00 00 00 00 00 00 00 14 00 00 0A 00 00 00 // ................
> - 0040: 24 00 00 00 00 00 00 00 00 00 00 00 // $...........
> + 0030: 00 00 00 00 00 00 00 00 00 14 00 00 00 00 00 00 // ................
> + 0040: 24 00 00 00 00 00 00 00 00 00 00 00 00 14 00 00 // $...............
> + 0050: 0A 00 00 00 38 00 00 00 00 00 00 00 00 00 00 00 // ....8...........
>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
> tests/data/acpi/virt/PPTT | Bin 76 -> 96 bytes
> tests/qtest/bios-tables-test-allowed-diff.h | 1 -
> 2 files changed, 1 deletion(-)
>
> diff --git a/tests/data/acpi/virt/PPTT b/tests/data/acpi/virt/PPTT
> index 7a1258ecf123555b24462c98ccbb76b4ac1d0c2b..f56ea63b369a604877374ad696c396e796ab1c83 100644
> GIT binary patch
> delta 53
> zcmV-50LuSNU<y!BR8(L90006=kqR;-00000Bme*a000000000002BZK3IG5AH~;_u
> L0000000000uCW9Z
>
> delta 32
> qcmV+*0N?*$ObSp?R8&j=00080kqR=APy`Gl00000000000001OcLdh}
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index cb143a55a6..dfb8523c8b 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1,2 +1 @@
> /* List of comma-separated changed AML files to ignore */
> -"tests/data/acpi/virt/PPTT",
> --
> 2.27.0
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 3/7] hw/acpi/aml-build: Improve scalability of PPTT generation
2022-01-03 11:24 ` Andrew Jones
@ 2022-01-04 2:05 ` wangyanan (Y) via
0 siblings, 0 replies; 21+ messages in thread
From: wangyanan (Y) via @ 2022-01-04 2:05 UTC (permalink / raw)
To: Andrew Jones
Cc: qemu-devel, qemu-arm, Peter Maydell, Michael S . Tsirkin,
Igor Mammedov, Shannon Zhao, Ani Sinha, Eric Auger,
wanghaibin.wang
Hi Drew,
Thanks for your review.
On 2022/1/3 19:24, Andrew Jones wrote:
> On Mon, Jan 03, 2022 at 04:46:32PM +0800, Yanan Wang wrote:
>> Currently we generate a PPTT table of n-level processor hierarchy
>> with n-level loops in build_pptt(). It works fine as now there are
>> only three CPU topology parameters. But the code may become less
>> scalable with the processor hierarchy levels increasing.
>>
>> This patch only improves the scalability of build_pptt by reducing
>> the loops, and intends to make no functional change.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>> hw/acpi/aml-build.c | 50 +++++++++++++++++++++++++++++----------------
>> 1 file changed, 32 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index b3b3310df3..be3851be36 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -2001,7 +2001,10 @@ static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
>> void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>> const char *oem_id, const char *oem_table_id)
>> {
>> - int pptt_start = table_data->len;
>> + GQueue *list = g_queue_new();
>> + guint pptt_start = table_data->len;
>> + guint father_offset;
> "parent_offset" would be more conventional.
Apparently... I will rename it.
>> + guint length, i;
>> int uid = 0;
>> int socket;
>> AcpiTable table = { .sig = "PPTT", .rev = 2,
>> @@ -2010,9 +2013,8 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>> acpi_table_begin(&table, table_data);
>>
>> for (socket = 0; socket < ms->smp.sockets; socket++) {
>> - uint32_t socket_offset = table_data->len - pptt_start;
>> - int core;
>> -
>> + g_queue_push_tail(list,
>> + GUINT_TO_POINTER(table_data->len - pptt_start));
>> build_processor_hierarchy_node(
>> table_data,
>> /*
>> @@ -2021,35 +2023,47 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>> */
>> (1 << 0),
>> 0, socket, NULL, 0);
>> + }
>>
>> - for (core = 0; core < ms->smp.cores; core++) {
>> - uint32_t core_offset = table_data->len - pptt_start;
>> - int thread;
>> + length = g_queue_get_length(list);
>> + for (i = 0; i < length; i++) {
>> + int core;
>>
>> + father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
>> + for (core = 0; core < ms->smp.cores; core++) {
>> if (ms->smp.threads > 1) {
>> + g_queue_push_tail(list,
>> + GUINT_TO_POINTER(table_data->len - pptt_start));
>> build_processor_hierarchy_node(
>> table_data,
>> (0 << 0), /* not a physical package */
>> - socket_offset, core, NULL, 0);
>> -
>> - for (thread = 0; thread < ms->smp.threads; thread++) {
>> - build_processor_hierarchy_node(
>> - table_data,
>> - (1 << 1) | /* ACPI Processor ID valid */
>> - (1 << 2) | /* Processor is a Thread */
>> - (1 << 3), /* Node is a Leaf */
>> - core_offset, uid++, NULL, 0);
>> - }
>> + father_offset, core, NULL, 0);
>> } else {
>> build_processor_hierarchy_node(
>> table_data,
>> (1 << 1) | /* ACPI Processor ID valid */
>> (1 << 3), /* Node is a Leaf */
>> - socket_offset, uid++, NULL, 0);
>> + father_offset, uid++, NULL, 0);
>> }
>> }
>> }
>>
>> + length = g_queue_get_length(list);
>> + for (i = 0; i < length; i++) {
>> + int thread;
>> +
>> + father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
>> + for (thread = 0; thread < ms->smp.threads; thread++) {
>> + build_processor_hierarchy_node(
>> + table_data,
>> + (1 << 1) | /* ACPI Processor ID valid */
>> + (1 << 2) | /* Processor is a Thread */
>> + (1 << 3), /* Node is a Leaf */
>> + father_offset, uid++, NULL, 0);
>> + }
>> + }
>> +
>> + g_queue_free(list);
>> acpi_table_end(linker, &table);
>> }
> This patch actually increases the number of loops, since we need to visit
> higher hierarchical nodes twice (once to enqueue and once to dequeue).
Yes, we actually need to access the higher hierarchical node's offset twice.
But that may not be a problem since numbers of topology parameters are
not so huge that we need to consider the performance.
> We
> do reduce code indentation and it looks like we could more easily skip
> hierarchy levels we don't want, though.
Yes, just as you said. The commit message doesn't describe the motivation
well. This patch aims to reduce the increasing code indentation because of
increasing nested loops, and consequently it's a bit easier to extend with
new topology level.
> While my impulse is to say we
> should just keep this simple and add another nested loop for clusters, I
> guess I'm OK with this too.
Thank you!
> Reviewed-by: Andrew Jones <drjones@redhat.com>
>
Thanks,
Yanan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 4/7] hw/arm/virt-acpi-build: Make an ARM specific PPTT generator
2022-01-03 11:30 ` Andrew Jones
@ 2022-01-04 2:06 ` wangyanan (Y) via
0 siblings, 0 replies; 21+ messages in thread
From: wangyanan (Y) via @ 2022-01-04 2:06 UTC (permalink / raw)
To: Andrew Jones
Cc: qemu-devel, qemu-arm, Peter Maydell, Michael S . Tsirkin,
Igor Mammedov, Shannon Zhao, Ani Sinha, Eric Auger,
wanghaibin.wang
Hi Drew,
On 2022/1/3 19:30, Andrew Jones wrote:
> On Mon, Jan 03, 2022 at 04:46:33PM +0800, Yanan Wang wrote:
>> We have a generic build_pptt() in hw/acpi/aml-build.c but it's
>> currently only used in ARM acpi initialization. Now we are going
>> to support the new CPU cluster parameter which is currently only
>> supported by ARM, it won't be a very good idea to add it to the
>> generic build_pptt() as it will make the code complex and hard
>> to maintain especially when we also support CPU cache topology
>> hierarchy in build_pptt() too. Note that the cache topology
>> design also varies between different CPU targets.
> It's a shame to remove generic ACPI table. I'm not sure what
> will be best when addressing the cache topology, but for the
> clusters it seems like we should be able to easily skip them
> when has_clusters is false.
Hm yes. Currently for clusters alone, a check of
ms->smp_props.clusters_supported
would be enough. So I guess it's preferred that we should keep the
generic PPTT, right?
>> So an ARM specific PPTT generator becomes necessary now. Given
>> that the generic one is currently only used by ARM, let's just
>> move build_pptt() from aml-build.c to virt-acpi-build.c with
>> minor update.
> Please state what the minor update is? I see the oem parameter
> change, but I want to be sure that's the one you're referring
> to.
Yes, I changed the function parameter "ms" to "vms". That's the
whole update.
Thanks,
Yanan
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>> hw/acpi/aml-build.c | 80 ++-----------------------------------
>> hw/arm/virt-acpi-build.c | 77 ++++++++++++++++++++++++++++++++++-
>> include/hw/acpi/aml-build.h | 5 ++-
>> 3 files changed, 81 insertions(+), 81 deletions(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index be3851be36..040fbc9b4b 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -1968,10 +1968,9 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>> * ACPI spec, Revision 6.3
>> * 5.2.29.1 Processor hierarchy node structure (Type 0)
>> */
>> -static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
>> - uint32_t parent, uint32_t id,
>> - uint32_t *priv_rsrc,
>> - uint32_t priv_num)
>> +void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
>> + uint32_t parent, uint32_t id,
>> + uint32_t *priv_rsrc, uint32_t priv_num)
>> {
>> int i;
>>
>> @@ -1994,79 +1993,6 @@ static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
>> }
>> }
>>
>> -/*
>> - * ACPI spec, Revision 6.3
>> - * 5.2.29 Processor Properties Topology Table (PPTT)
>> - */
>> -void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>> - const char *oem_id, const char *oem_table_id)
>> -{
>> - GQueue *list = g_queue_new();
>> - guint pptt_start = table_data->len;
>> - guint father_offset;
>> - guint length, i;
>> - int uid = 0;
>> - int socket;
>> - AcpiTable table = { .sig = "PPTT", .rev = 2,
>> - .oem_id = oem_id, .oem_table_id = oem_table_id };
>> -
>> - acpi_table_begin(&table, table_data);
>> -
>> - for (socket = 0; socket < ms->smp.sockets; socket++) {
>> - g_queue_push_tail(list,
>> - GUINT_TO_POINTER(table_data->len - pptt_start));
>> - build_processor_hierarchy_node(
>> - table_data,
>> - /*
>> - * Physical package - represents the boundary
>> - * of a physical package
>> - */
>> - (1 << 0),
>> - 0, socket, NULL, 0);
>> - }
>> -
>> - length = g_queue_get_length(list);
>> - for (i = 0; i < length; i++) {
>> - int core;
>> -
>> - father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
>> - for (core = 0; core < ms->smp.cores; core++) {
>> - if (ms->smp.threads > 1) {
>> - g_queue_push_tail(list,
>> - GUINT_TO_POINTER(table_data->len - pptt_start));
>> - build_processor_hierarchy_node(
>> - table_data,
>> - (0 << 0), /* not a physical package */
>> - father_offset, core, NULL, 0);
>> - } else {
>> - build_processor_hierarchy_node(
>> - table_data,
>> - (1 << 1) | /* ACPI Processor ID valid */
>> - (1 << 3), /* Node is a Leaf */
>> - father_offset, uid++, NULL, 0);
>> - }
>> - }
>> - }
>> -
>> - length = g_queue_get_length(list);
>> - for (i = 0; i < length; i++) {
>> - int thread;
>> -
>> - father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
>> - for (thread = 0; thread < ms->smp.threads; thread++) {
>> - build_processor_hierarchy_node(
>> - table_data,
>> - (1 << 1) | /* ACPI Processor ID valid */
>> - (1 << 2) | /* Processor is a Thread */
>> - (1 << 3), /* Node is a Leaf */
>> - father_offset, uid++, NULL, 0);
>> - }
>> - }
>> -
>> - g_queue_free(list);
>> - acpi_table_end(linker, &table);
>> -}
>> -
>> /* build rev1/rev3/rev5.1 FADT */
>> void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>> const char *oem_id, const char *oem_table_id)
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index d0f4867fdf..3ce7680393 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -808,6 +808,80 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>> acpi_table_end(linker, &table);
>> }
>>
>> +/*
>> + * ACPI spec, Revision 6.3
>> + * 5.2.29 Processor Properties Topology Table (PPTT)
>> + */
>> +static void
>> +build_pptt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>> +{
>> + MachineState *ms = MACHINE(vms);
>> + GQueue *list = g_queue_new();
>> + guint pptt_start = table_data->len;
>> + guint father_offset;
>> + guint length, i;
>> + int uid = 0;
>> + int socket;
>> + AcpiTable table = { .sig = "PPTT", .rev = 2, .oem_id = vms->oem_id,
>> + .oem_table_id = vms->oem_table_id };
>> +
>> + acpi_table_begin(&table, table_data);
>> +
>> + for (socket = 0; socket < ms->smp.sockets; socket++) {
>> + g_queue_push_tail(list,
>> + GUINT_TO_POINTER(table_data->len - pptt_start));
>> + build_processor_hierarchy_node(
>> + table_data,
>> + /*
>> + * Physical package - represents the boundary
>> + * of a physical package
>> + */
>> + (1 << 0),
>> + 0, socket, NULL, 0);
>> + }
>> +
>> + length = g_queue_get_length(list);
>> + for (i = 0; i < length; i++) {
>> + int core;
>> +
>> + father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
>> + for (core = 0; core < ms->smp.cores; core++) {
>> + if (ms->smp.threads > 1) {
>> + g_queue_push_tail(list,
>> + GUINT_TO_POINTER(table_data->len - pptt_start));
>> + build_processor_hierarchy_node(
>> + table_data,
>> + (0 << 0), /* not a physical package */
>> + father_offset, core, NULL, 0);
>> + } else {
>> + build_processor_hierarchy_node(
>> + table_data,
>> + (1 << 1) | /* ACPI Processor ID valid */
>> + (1 << 3), /* Node is a Leaf */
>> + father_offset, uid++, NULL, 0);
>> + }
>> + }
>> + }
>> +
>> + length = g_queue_get_length(list);
>> + for (i = 0; i < length; i++) {
>> + int thread;
>> +
>> + father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
>> + for (thread = 0; thread < ms->smp.threads; thread++) {
>> + build_processor_hierarchy_node(
>> + table_data,
>> + (1 << 1) | /* ACPI Processor ID valid */
>> + (1 << 2) | /* Processor is a Thread */
>> + (1 << 3), /* Node is a Leaf */
>> + father_offset, uid++, NULL, 0);
>> + }
>> + }
>> +
>> + g_queue_free(list);
>> + acpi_table_end(linker, &table);
>> +}
>> +
>> /* FADT */
>> static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
>> VirtMachineState *vms, unsigned dsdt_tbl_offset)
>> @@ -953,8 +1027,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>
>> if (!vmc->no_cpu_topology) {
>> acpi_add_table(table_offsets, tables_blob);
>> - build_pptt(tables_blob, tables->linker, ms,
>> - vms->oem_id, vms->oem_table_id);
>> + build_pptt(tables_blob, tables->linker, vms);
>> }
>>
>> acpi_add_table(table_offsets, tables_blob);
>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index 8346003a22..2c457c8f17 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -489,8 +489,9 @@ void build_srat_memory(GArray *table_data, uint64_t base,
>> void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>> const char *oem_id, const char *oem_table_id);
>>
>> -void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>> - const char *oem_id, const char *oem_table_id);
>> +void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
>> + uint32_t parent, uint32_t id,
>> + uint32_t *priv_rsrc, uint32_t priv_num);
>>
>> void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>> const char *oem_id, const char *oem_table_id);
>> --
>> 2.27.0
>>
> Thanks,
> drew
>
> .
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 6/7] hw/arm/virt-acpi-build: Support cluster level in PPTT generation
2022-01-03 11:32 ` Andrew Jones
@ 2022-01-04 2:15 ` wangyanan (Y) via
0 siblings, 0 replies; 21+ messages in thread
From: wangyanan (Y) via @ 2022-01-04 2:15 UTC (permalink / raw)
To: Andrew Jones
Cc: qemu-devel, qemu-arm, Peter Maydell, Michael S . Tsirkin,
Igor Mammedov, Shannon Zhao, Ani Sinha, Eric Auger,
wanghaibin.wang
On 2022/1/3 19:32, Andrew Jones wrote:
> On Mon, Jan 03, 2022 at 04:46:35PM +0800, Yanan Wang wrote:
>> Support cluster level in generation of ACPI Processor Properties
>> Topology Table (PPTT) for ARM virt machines.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>> hw/arm/virt-acpi-build.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 3ce7680393..5f91969688 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -840,6 +840,21 @@ build_pptt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>> 0, socket, NULL, 0);
>> }
>>
>> + length = g_queue_get_length(list);
>> + for (i = 0; i < length; i++) {
>> + int cluster;
>> +
>> + father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
>> + for (cluster = 0; cluster < ms->smp.clusters; cluster++) {
>> + g_queue_push_tail(list,
>> + GUINT_TO_POINTER(table_data->len - pptt_start));
>> + build_processor_hierarchy_node(
>> + table_data,
>> + (0 << 0), /* not a physical package */
>> + father_offset, cluster, NULL, 0);
>> + }
>> + }
>> +
>> length = g_queue_get_length(list);
>> for (i = 0; i < length; i++) {
>> int core;
>> --
>> 2.27.0
>>
> Looks good except please do s/father_offset/parent_offset/ as I mentioned
> in an earlier patch.
Will do.
>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
>
Thanks,
Yanan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 7/7] tests/acpi/bios-table-test: Update expected virt/PPTT file
2022-01-03 12:01 ` Ani Sinha
@ 2022-01-04 2:28 ` wangyanan (Y) via
2022-01-04 4:27 ` Ani Sinha
0 siblings, 1 reply; 21+ messages in thread
From: wangyanan (Y) via @ 2022-01-04 2:28 UTC (permalink / raw)
To: Ani Sinha
Cc: qemu-devel, qemu-arm, Peter Maydell, Andrew Jones,
Michael S . Tsirkin, Igor Mammedov, Shannon Zhao, Eric Auger,
wanghaibin.wang
Hi Ani,
Thanks for your review.
On 2022/1/3 20:01, Ani Sinha wrote:
>
> On Mon, 3 Jan 2022, Yanan Wang wrote:
>
>> Run ./tests/data/acpi/rebuild-expected-aml.sh from build directory
>> to update PPTT binary. Also empty bios-tables-test-allowed-diff.h.
>>
>> The disassembled differences between actual and expected PPTT:
>>
>> /*
>> * Intel ACPI Component Architecture
>> * AML/ASL+ Disassembler version 20180810 (64-bit version)
>> * Copyright (c) 2000 - 2018 Intel Corporation
>> *
>> - * Disassembly of tests/data/acpi/virt/PPTT, Mon Oct 25 20:24:53 2021
>> + * Disassembly of /tmp/aml-BPI5B1, Mon Oct 25 20:24:53 2021
>> *
>> * ACPI Data Table [PPTT]
>> *
>> * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue
>> */
>>
>> [000h 0000 4] Signature : "PPTT" [Processor Properties Topology Table]
>> -[004h 0004 4] Table Length : 0000004C
>> +[004h 0004 4] Table Length : 00000060
>> [008h 0008 1] Revision : 02
>> -[009h 0009 1] Checksum : A8
>> +[009h 0009 1] Checksum : 48
>> [00Ah 0010 6] Oem ID : "BOCHS "
>> [010h 0016 8] Oem Table ID : "BXPC "
>> [018h 0024 4] Oem Revision : 00000001
>> [01Ch 0028 4] Asl Compiler ID : "BXPC"
>> [020h 0032 4] Asl Compiler Revision : 00000001
>>
>> [024h 0036 1] Subtable Type : 00 [Processor Hierarchy Node]
>> [025h 0037 1] Length : 14
>> [026h 0038 2] Reserved : 0000
>> [028h 0040 4] Flags (decoded below) : 00000001
>> Physical package : 1
>> ACPI Processor ID valid : 0
>> [02Ch 0044 4] Parent : 00000000
>> [030h 0048 4] ACPI Processor ID : 00000000
>> [034h 0052 4] Private Resource Number : 00000000
>>
>> [038h 0056 1] Subtable Type : 00 [Processor Hierarchy Node]
>> [039h 0057 1] Length : 14
>> [03Ah 0058 2] Reserved : 0000
>> -[03Ch 0060 4] Flags (decoded below) : 0000000A
>> +[03Ch 0060 4] Flags (decoded below) : 00000000
>> Physical package : 0
>> - ACPI Processor ID valid : 1
>> + ACPI Processor ID valid : 0
> I do not know this very well but does the above two changes (flags and
> processor ID) makes sense?
Yes. I think this is exactly what we expected.
Above flags is for the newly inserted cluster node which is between
socket node and core node. Flag "Physical package" is 0 because
it does not represent the boundary of physical package. Flag
"ACPI Processor ID valid" is 0, because we don't need a valid ID
for a container in QEMU (cluster is container of CPU core) just
like socket node.
"0000000A" originally comes from core node, which now is at
place [*] below.
We can also read the reason why we don't need a valid ID for a
container in 099f2df2e6b "hw/acpi/aml-build: Add PPTT table".
>
>> [040h 0064 4] Parent : 00000024
>> [044h 0068 4] ACPI Processor ID : 00000000
>> [048h 0072 4] Private Resource Number : 00000000
>>
>> -Raw Table Data: Length 76 (0x4C)
>> +[04Ch 0076 1] Subtable Type : 00 [Processor Hierarchy Node]
>> +[04Dh 0077 1] Length : 14
>> +[04Eh 0078 2] Reserved : 0000
>> +[050h 0080 4] Flags (decoded below) : 0000000A
>> + Physical package : 0
>> + ACPI Processor ID valid : 1
>> +[054h 0084 4] Parent : 00000038
>> +[058h 0088 4] ACPI Processor ID : 00000000
>> +[05Ch 0092 4] Private Resource Number : 00000000
[*] Information of core node.
Thanks,
Yanan
>> +
>> +Raw Table Data: Length 96 (0x60)
>>
>> - 0000: 50 50 54 54 4C 00 00 00 02 A8 42 4F 43 48 53 20 // PPTTL.....BOCHS
>> + 0000: 50 50 54 54 60 00 00 00 02 48 42 4F 43 48 53 20 // PPTT`....HBOCHS
>> 0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
>> 0020: 01 00 00 00 00 14 00 00 01 00 00 00 00 00 00 00 // ................
>> - 0030: 00 00 00 00 00 00 00 00 00 14 00 00 0A 00 00 00 // ................
>> - 0040: 24 00 00 00 00 00 00 00 00 00 00 00 // $...........
>> + 0030: 00 00 00 00 00 00 00 00 00 14 00 00 00 00 00 00 // ................
>> + 0040: 24 00 00 00 00 00 00 00 00 00 00 00 00 14 00 00 // $...............
>> + 0050: 0A 00 00 00 38 00 00 00 00 00 00 00 00 00 00 00 // ....8...........
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>> tests/data/acpi/virt/PPTT | Bin 76 -> 96 bytes
>> tests/qtest/bios-tables-test-allowed-diff.h | 1 -
>> 2 files changed, 1 deletion(-)
>>
>> diff --git a/tests/data/acpi/virt/PPTT b/tests/data/acpi/virt/PPTT
>> index 7a1258ecf123555b24462c98ccbb76b4ac1d0c2b..f56ea63b369a604877374ad696c396e796ab1c83 100644
>> GIT binary patch
>> delta 53
>> zcmV-50LuSNU<y!BR8(L90006=kqR;-00000Bme*a000000000002BZK3IG5AH~;_u
>> L0000000000uCW9Z
>>
>> delta 32
>> qcmV+*0N?*$ObSp?R8&j=00080kqR=APy`Gl00000000000001OcLdh}
>>
>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
>> index cb143a55a6..dfb8523c8b 100644
>> --- a/tests/qtest/bios-tables-test-allowed-diff.h
>> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
>> @@ -1,2 +1 @@
>> /* List of comma-separated changed AML files to ignore */
>> -"tests/data/acpi/virt/PPTT",
>> --
>> 2.27.0
>>
>>
> .
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 7/7] tests/acpi/bios-table-test: Update expected virt/PPTT file
2022-01-04 2:28 ` wangyanan (Y) via
@ 2022-01-04 4:27 ` Ani Sinha
2022-01-04 4:51 ` wangyanan (Y) via
0 siblings, 1 reply; 21+ messages in thread
From: Ani Sinha @ 2022-01-04 4:27 UTC (permalink / raw)
To: wangyanan (Y)
Cc: Peter Maydell, Andrew Jones, Michael S . Tsirkin, qemu-devel,
Shannon Zhao, qemu-arm, Eric Auger, wanghaibin.wang,
Igor Mammedov
On Tue, 4 Jan 2022, wangyanan (Y) wrote:
> Hi Ani,
> Thanks for your review.
>
> On 2022/1/3 20:01, Ani Sinha wrote:
> >
> > On Mon, 3 Jan 2022, Yanan Wang wrote:
> >
> > > Run ./tests/data/acpi/rebuild-expected-aml.sh from build directory
> > > to update PPTT binary. Also empty bios-tables-test-allowed-diff.h.
> > >
> > > The disassembled differences between actual and expected PPTT:
> > >
> > > /*
> > > * Intel ACPI Component Architecture
> > > * AML/ASL+ Disassembler version 20180810 (64-bit version)
> > > * Copyright (c) 2000 - 2018 Intel Corporation
> > > *
> > > - * Disassembly of tests/data/acpi/virt/PPTT, Mon Oct 25 20:24:53 2021
> > > + * Disassembly of /tmp/aml-BPI5B1, Mon Oct 25 20:24:53 2021
> > > *
> > > * ACPI Data Table [PPTT]
> > > *
> > > * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue
> > > */
> > >
> > > [000h 0000 4] Signature : "PPTT" [Processor
> > > Properties Topology Table]
> > > -[004h 0004 4] Table Length : 0000004C
> > > +[004h 0004 4] Table Length : 00000060
> > > [008h 0008 1] Revision : 02
> > > -[009h 0009 1] Checksum : A8
> > > +[009h 0009 1] Checksum : 48
> > > [00Ah 0010 6] Oem ID : "BOCHS "
> > > [010h 0016 8] Oem Table ID : "BXPC "
> > > [018h 0024 4] Oem Revision : 00000001
> > > [01Ch 0028 4] Asl Compiler ID : "BXPC"
> > > [020h 0032 4] Asl Compiler Revision : 00000001
> > >
> > > [024h 0036 1] Subtable Type : 00 [Processor Hierarchy
> > > Node]
> > > [025h 0037 1] Length : 14
> > > [026h 0038 2] Reserved : 0000
> > > [028h 0040 4] Flags (decoded below) : 00000001
> > > Physical package : 1
> > > ACPI Processor ID valid : 0
> > > [02Ch 0044 4] Parent : 00000000
> > > [030h 0048 4] ACPI Processor ID : 00000000
> > > [034h 0052 4] Private Resource Number : 00000000
> > >
> > > [038h 0056 1] Subtable Type : 00 [Processor Hierarchy
> > > Node]
> > > [039h 0057 1] Length : 14
> > > [03Ah 0058 2] Reserved : 0000
> > > -[03Ch 0060 4] Flags (decoded below) : 0000000A
> > > +[03Ch 0060 4] Flags (decoded below) : 00000000
> > > Physical package : 0
> > > - ACPI Processor ID valid : 1
> > > + ACPI Processor ID valid : 0
> > I do not know this very well but does the above two changes (flags and
> > processor ID) makes sense?
> Yes. I think this is exactly what we expected.
> Above flags is for the newly inserted cluster node which is between
> socket node and core node. Flag "Physical package" is 0 because
> it does not represent the boundary of physical package. Flag
> "ACPI Processor ID valid" is 0, because we don't need a valid ID
> for a container in QEMU (cluster is container of CPU core) just
> like socket node.
>
> "0000000A" originally comes from core node, which now is at
> place [*] below.
>
> We can also read the reason why we don't need a valid ID for a
> container in 099f2df2e6b "hw/acpi/aml-build: Add PPTT table".
Ok as long as we can explain it, I am good.
> >
> > > [040h 0064 4] Parent : 00000024
> > > [044h 0068 4] ACPI Processor ID : 00000000
> > > [048h 0072 4] Private Resource Number : 00000000
> > >
> > > -Raw Table Data: Length 76 (0x4C)
> > > +[04Ch 0076 1] Subtable Type : 00 [Processor Hierarchy
> > > Node]
> > > +[04Dh 0077 1] Length : 14
> > > +[04Eh 0078 2] Reserved : 0000
> > > +[050h 0080 4] Flags (decoded below) : 0000000A
> > > + Physical package : 0
> > > + ACPI Processor ID valid : 1
> > > +[054h 0084 4] Parent : 00000038
> > > +[058h 0088 4] ACPI Processor ID : 00000000
> > > +[05Ch 0092 4] Private Resource Number : 00000000
> [*] Information of core node.
>
> Thanks,
> Yanan
> > > +
> > > +Raw Table Data: Length 96 (0x60)
> > >
> > > - 0000: 50 50 54 54 4C 00 00 00 02 A8 42 4F 43 48 53 20 //
> > > PPTTL.....BOCHS
> > > + 0000: 50 50 54 54 60 00 00 00 02 48 42 4F 43 48 53 20 //
> > > PPTT`....HBOCHS
> > > 0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC
> > > ....BXPC
> > > 0020: 01 00 00 00 00 14 00 00 01 00 00 00 00 00 00 00 //
> > > ................
> > > - 0030: 00 00 00 00 00 00 00 00 00 14 00 00 0A 00 00 00 //
> > > ................
> > > - 0040: 24 00 00 00 00 00 00 00 00 00 00 00 //
> > > $...........
> > > + 0030: 00 00 00 00 00 00 00 00 00 14 00 00 00 00 00 00 //
> > > ................
> > > + 0040: 24 00 00 00 00 00 00 00 00 00 00 00 00 14 00 00 //
> > > $...............
> > > + 0050: 0A 00 00 00 38 00 00 00 00 00 00 00 00 00 00 00 //
> > > ....8...........
> > >
> > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > > ---
> > > tests/data/acpi/virt/PPTT | Bin 76 -> 96 bytes
> > > tests/qtest/bios-tables-test-allowed-diff.h | 1 -
> > > 2 files changed, 1 deletion(-)
> > >
> > > diff --git a/tests/data/acpi/virt/PPTT b/tests/data/acpi/virt/PPTT
> > > index
> > > 7a1258ecf123555b24462c98ccbb76b4ac1d0c2b..f56ea63b369a604877374ad696c396e796ab1c83
> > > 100644
> > > GIT binary patch
> > > delta 53
> > > zcmV-50LuSNU<y!BR8(L90006=kqR;-00000Bme*a000000000002BZK3IG5AH~;_u
> > > L0000000000uCW9Z
> > >
> > > delta 32
> > > qcmV+*0N?*$ObSp?R8&j=00080kqR=APy`Gl00000000000001OcLdh}
> > >
> > > diff --git a/tests/qtest/bios-tables-test-allowed-diff.h
> > > b/tests/qtest/bios-tables-test-allowed-diff.h
> > > index cb143a55a6..dfb8523c8b 100644
> > > --- a/tests/qtest/bios-tables-test-allowed-diff.h
> > > +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> > > @@ -1,2 +1 @@
> > > /* List of comma-separated changed AML files to ignore */
> > > -"tests/data/acpi/virt/PPTT",
> > > --
> > > 2.27.0
> > >
> > >
> > .
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 7/7] tests/acpi/bios-table-test: Update expected virt/PPTT file
2022-01-04 4:27 ` Ani Sinha
@ 2022-01-04 4:51 ` wangyanan (Y) via
0 siblings, 0 replies; 21+ messages in thread
From: wangyanan (Y) via @ 2022-01-04 4:51 UTC (permalink / raw)
To: Ani Sinha
Cc: qemu-devel, qemu-arm, Peter Maydell, Andrew Jones,
Michael S . Tsirkin, Igor Mammedov, Shannon Zhao, Eric Auger,
wanghaibin.wang
On 2022/1/4 12:27, Ani Sinha wrote:
>
> On Tue, 4 Jan 2022, wangyanan (Y) wrote:
>
>> Hi Ani,
>> Thanks for your review.
>>
>> On 2022/1/3 20:01, Ani Sinha wrote:
>>> On Mon, 3 Jan 2022, Yanan Wang wrote:
>>>
>>>> Run ./tests/data/acpi/rebuild-expected-aml.sh from build directory
>>>> to update PPTT binary. Also empty bios-tables-test-allowed-diff.h.
>>>>
>>>> The disassembled differences between actual and expected PPTT:
>>>>
>>>> /*
>>>> * Intel ACPI Component Architecture
>>>> * AML/ASL+ Disassembler version 20180810 (64-bit version)
>>>> * Copyright (c) 2000 - 2018 Intel Corporation
>>>> *
>>>> - * Disassembly of tests/data/acpi/virt/PPTT, Mon Oct 25 20:24:53 2021
>>>> + * Disassembly of /tmp/aml-BPI5B1, Mon Oct 25 20:24:53 2021
>>>> *
>>>> * ACPI Data Table [PPTT]
>>>> *
>>>> * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue
>>>> */
>>>>
>>>> [000h 0000 4] Signature : "PPTT" [Processor
>>>> Properties Topology Table]
>>>> -[004h 0004 4] Table Length : 0000004C
>>>> +[004h 0004 4] Table Length : 00000060
>>>> [008h 0008 1] Revision : 02
>>>> -[009h 0009 1] Checksum : A8
>>>> +[009h 0009 1] Checksum : 48
>>>> [00Ah 0010 6] Oem ID : "BOCHS "
>>>> [010h 0016 8] Oem Table ID : "BXPC "
>>>> [018h 0024 4] Oem Revision : 00000001
>>>> [01Ch 0028 4] Asl Compiler ID : "BXPC"
>>>> [020h 0032 4] Asl Compiler Revision : 00000001
>>>>
>>>> [024h 0036 1] Subtable Type : 00 [Processor Hierarchy
>>>> Node]
>>>> [025h 0037 1] Length : 14
>>>> [026h 0038 2] Reserved : 0000
>>>> [028h 0040 4] Flags (decoded below) : 00000001
>>>> Physical package : 1
>>>> ACPI Processor ID valid : 0
>>>> [02Ch 0044 4] Parent : 00000000
>>>> [030h 0048 4] ACPI Processor ID : 00000000
>>>> [034h 0052 4] Private Resource Number : 00000000
>>>>
>>>> [038h 0056 1] Subtable Type : 00 [Processor Hierarchy
>>>> Node]
>>>> [039h 0057 1] Length : 14
>>>> [03Ah 0058 2] Reserved : 0000
>>>> -[03Ch 0060 4] Flags (decoded below) : 0000000A
>>>> +[03Ch 0060 4] Flags (decoded below) : 00000000
>>>> Physical package : 0
>>>> - ACPI Processor ID valid : 1
>>>> + ACPI Processor ID valid : 0
>>> I do not know this very well but does the above two changes (flags and
>>> processor ID) makes sense?
>> Yes. I think this is exactly what we expected.
>> Above flags is for the newly inserted cluster node which is between
>> socket node and core node. Flag "Physical package" is 0 because
>> it does not represent the boundary of physical package. Flag
>> "ACPI Processor ID valid" is 0, because we don't need a valid ID
>> for a container in QEMU (cluster is container of CPU core) just
>> like socket node.
>>
>> "0000000A" originally comes from core node, which now is at
>> place [*] below.
>>
>> We can also read the reason why we don't need a valid ID for a
>> container in 099f2df2e6b "hw/acpi/aml-build: Add PPTT table".
> Ok as long as we can explain it, I am good.
>
>>>> [040h 0064 4] Parent : 00000024
>>>> [044h 0068 4] ACPI Processor ID : 00000000
>>>> [048h 0072 4] Private Resource Number : 00000000
>>>>
>>>> -Raw Table Data: Length 76 (0x4C)
>>>> +[04Ch 0076 1] Subtable Type : 00 [Processor Hierarchy
>>>> Node]
>>>> +[04Dh 0077 1] Length : 14
>>>> +[04Eh 0078 2] Reserved : 0000
>>>> +[050h 0080 4] Flags (decoded below) : 0000000A
>>>> + Physical package : 0
>>>> + ACPI Processor ID valid : 1
>>>> +[054h 0084 4] Parent : 00000038
>>>> +[058h 0088 4] ACPI Processor ID : 00000000
>>>> +[05Ch 0092 4] Private Resource Number : 00000000
>> [*] Information of core node.
>>
>> Thanks,
>> Yanan
>>>> +
>>>> +Raw Table Data: Length 96 (0x60)
>>>>
>>>> - 0000: 50 50 54 54 4C 00 00 00 02 A8 42 4F 43 48 53 20 //
>>>> PPTTL.....BOCHS
>>>> + 0000: 50 50 54 54 60 00 00 00 02 48 42 4F 43 48 53 20 //
>>>> PPTT`....HBOCHS
>>>> 0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC
>>>> ....BXPC
>>>> 0020: 01 00 00 00 00 14 00 00 01 00 00 00 00 00 00 00 //
>>>> ................
>>>> - 0030: 00 00 00 00 00 00 00 00 00 14 00 00 0A 00 00 00 //
>>>> ................
>>>> - 0040: 24 00 00 00 00 00 00 00 00 00 00 00 //
>>>> $...........
>>>> + 0030: 00 00 00 00 00 00 00 00 00 14 00 00 00 00 00 00 //
>>>> ................
>>>> + 0040: 24 00 00 00 00 00 00 00 00 00 00 00 00 14 00 00 //
>>>> $...............
>>>> + 0050: 0A 00 00 00 38 00 00 00 00 00 00 00 00 00 00 00 //
>>>> ....8...........
>>>>
>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> Reviewed-by: Ani Sinha <ani@anisinha.ca>
Thank you.
Yanan
>>>> ---
>>>> tests/data/acpi/virt/PPTT | Bin 76 -> 96 bytes
>>>> tests/qtest/bios-tables-test-allowed-diff.h | 1 -
>>>> 2 files changed, 1 deletion(-)
>>>>
>>>> diff --git a/tests/data/acpi/virt/PPTT b/tests/data/acpi/virt/PPTT
>>>> index
>>>> 7a1258ecf123555b24462c98ccbb76b4ac1d0c2b..f56ea63b369a604877374ad696c396e796ab1c83
>>>> 100644
>>>> GIT binary patch
>>>> delta 53
>>>> zcmV-50LuSNU<y!BR8(L90006=kqR;-00000Bme*a000000000002BZK3IG5AH~;_u
>>>> L0000000000uCW9Z
>>>>
>>>> delta 32
>>>> qcmV+*0N?*$ObSp?R8&j=00080kqR=APy`Gl00000000000001OcLdh}
>>>>
>>>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h
>>>> b/tests/qtest/bios-tables-test-allowed-diff.h
>>>> index cb143a55a6..dfb8523c8b 100644
>>>> --- a/tests/qtest/bios-tables-test-allowed-diff.h
>>>> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
>>>> @@ -1,2 +1 @@
>>>> /* List of comma-separated changed AML files to ignore */
>>>> -"tests/data/acpi/virt/PPTT",
>>>> --
>>>> 2.27.0
>>>>
>>>>
>>> .
>>
> .
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2022-01-04 4:52 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-03 8:46 [PATCH v6 0/7] ARM virt: Support CPU cluster topology Yanan Wang via
2022-01-03 8:46 ` [PATCH v6 1/7] hw/arm/virt: Support CPU cluster on ARM virt machine Yanan Wang via
2022-01-03 11:24 ` Andrew Jones
2022-01-03 8:46 ` [PATCH v6 2/7] hw/arm/virt: Support cluster level in DT cpu-map Yanan Wang via
2022-01-03 11:25 ` Andrew Jones
2022-01-03 8:46 ` [PATCH v6 3/7] hw/acpi/aml-build: Improve scalability of PPTT generation Yanan Wang via
2022-01-03 11:24 ` Andrew Jones
2022-01-04 2:05 ` wangyanan (Y) via
2022-01-03 8:46 ` [PATCH v6 4/7] hw/arm/virt-acpi-build: Make an ARM specific PPTT generator Yanan Wang via
2022-01-03 11:30 ` Andrew Jones
2022-01-04 2:06 ` wangyanan (Y) via
2022-01-03 8:46 ` [PATCH v6 5/7] tests/acpi/bios-tables-test: Allow changes to virt/PPTT file Yanan Wang via
2022-01-03 11:51 ` Ani Sinha
2022-01-03 8:46 ` [PATCH v6 6/7] hw/arm/virt-acpi-build: Support cluster level in PPTT generation Yanan Wang via
2022-01-03 11:32 ` Andrew Jones
2022-01-04 2:15 ` wangyanan (Y) via
2022-01-03 8:46 ` [PATCH v6 7/7] tests/acpi/bios-table-test: Update expected virt/PPTT file Yanan Wang via
2022-01-03 12:01 ` Ani Sinha
2022-01-04 2:28 ` wangyanan (Y) via
2022-01-04 4:27 ` Ani Sinha
2022-01-04 4:51 ` wangyanan (Y) via
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.