On Tue, Apr 13, 2021 at 04:07:44PM +0800, Yanan Wang wrote:
Add the Processor Properties Topology Table (PPTT) to present
CPU topology information to ACPI guests. Note, while a DT boot
Linux guest with a non-flat CPU topology will see socket and
core IDs being sequential integers starting from zero, e.g.
with -smp 4,sockets=2,cores=2,threads=1
a DT boot produces
cpu: 0 package_id: 0 core_id: 0
cpu: 1 package_id: 0 core_id: 1
cpu: 2 package_id: 1 core_id: 0
cpu: 3 package_id: 1 core_id: 1
an ACPI boot produces
cpu: 0 package_id: 36 core_id: 0
cpu: 1 package_id: 36 core_id: 1
cpu: 2 package_id: 96 core_id: 2
cpu: 3 package_id: 96 core_id: 3
This is due to several reasons:
1) DT cpu nodes do not have an equivalent field to what the PPTT
ACPI Processor ID must be, i.e. something equal to the MADT CPU
UID or equal to the UID of an ACPI processor container. In both
ACPI cases those are platform dependant IDs assigned by the
vendor.
2) While QEMU is the vendor for a guest, if the topology specifies
SMT (> 1 thread), then, with ACPI, it is impossible to assign a
core-id the same value as a package-id, thus it is not possible
to have package-id=0 and core-id=0. This is because package and
core containers must be in the same ACPI namespace and therefore
must have unique UIDs.
3) ACPI processor containers are not required for PPTT tables to
be used and, due to the limitations of which IDs are selected
described above in (2), they are not helpful for QEMU, so we
don't build them with this patch. In the absence of them, Linux
assigns its own unique IDs. The maintainers have chosen not to use
counters from zero, but rather ACPI table offsets, which explains
why the numbers are so much larger than with DT.
4) When there is no SMT (threads=1) the core IDs for ACPI boot guests
match the logical CPU IDs, because these IDs must be equal to the
MADT CPU UID (as no processor containers are present), and QEMU
uses the logical CPU ID for these MADT IDs.
Tested-by: Jiajie Li <lijiajie11@huawei.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Ying Fang <fangying1@huawei.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
hw/arm/virt-acpi-build.c | 63 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 63 insertions(+)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 2ad5dad1bf..03fd812d5a 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -436,6 +436,64 @@ build_srat(GArray *table_data, BIOSLinker *linker,
VirtMachineState *vms)
vms->oem_table_id);
}
+/* PPTT */
Please point out the ACPI spec section "5.2.29 Processor Properties
Topology Table"
+static void
+build_pptt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
QEMU doesn't do this style, please write as
static void build_pptt(GArray *table_data, BIOSLinker *linker,
VirtMachineState *vms)
+{
+ int pptt_start = table_data->len;
+ int uid = 0, cpus = 0, socket = 0;
+ MachineState *ms = MACHINE(vms);
+ unsigned int smp_cores = ms->smp.cores;
+ unsigned int smp_threads = ms->smp.threads;
+
+ acpi_data_push(table_data, sizeof(AcpiTableHeader));
+
+ for (socket = 0; cpus < ms->possible_cpus->len; socket++) {
Why not iterate from zero to ms->smp.sockets? With this type of loop if
the number of sockets doesn't correctly fit the number of possible cpus,
then you'll magically create new sockets that the user didn't want. That
case shouldn't be able to happen, though, because the smp parsing should
catch it. In any case, iterating sockets between zero it's number would
make more sense.
+ uint32_t socket_offset = table_data->len - pptt_start;
+ int core;
+
+ build_processor_hierarchy_node(
+ table_data, 1, /* Physical package */
If we want to pass the flags with in-argument-list comments, then please
make sure the flags are on separate lines. See below.
+ 0, socket, /* No parent */
+ NULL, 0); /* No private resources */
We don't need the 'No parent' and 'No private resources' comments.
build_processor_hierarchy_node(table_data,
(1 << 0), /* ACPI 6.2: Physical package */
0, socket, NULL, 0);
+
+ for (core = 0; core < smp_cores; core++) {
+ uint32_t core_offset = table_data->len - pptt_start;
+ int thread;
+
+ if (smp_threads <= 1) {
+ build_processor_hierarchy_node(
+ table_data,
+ (1 << 1) | /* ACPI Processor ID valid */
+ (1 << 3), /* ACPI 6.3 - Node is a Leaf */
+ socket_offset, uid++, /* Parent is a Socket */
+ NULL, 0); /* No private resources */
Now I see why you were calling out 6.3 in the previous patch. I suggest
still keeping the function of the previous patch referencing 6.2, but
also keep referencing 6.3 here, like you already do
build_processor_hierarchy_node(table_data,
(1 << 1) | /* ACPI Processor ID valid */
(1 << 3), /* ACPI 6.3 - Node is a Leaf */
socket_offset, uid++, NULL, 0);
+ } else {
+ build_processor_hierarchy_node(
+ table_data, 0,
+ socket_offset, core, /* Parent is a Socket */
+ NULL, 0); /* No private resources */
No need for these in-argument-comments that don't match up with the spec.
+
+ for (thread = 0; thread < smp_threads; thread++) {
+ build_processor_hierarchy_node(
+ table_data,
+ (1 << 1) | /* ACPI Processor ID valid */
+ (1 << 2) | /* ACPI 6.3 - Processor is a Thread */
+ (1 << 3), /* ACPI 6.3 - Node is a Leaf */
This looks good.
+ core_offset, uid++, /* Parent is a Core */
+ NULL, 0); /* No private resources */
Don't need these comments.
+ }
+ }
+ }
+ cpus += smp_cores * smp_threads;
As stated above, we don't want this.
+ }
+
+ build_header(linker, table_data,
+ (void *)(table_data->data + pptt_start), "PPTT",
+ table_data->len - pptt_start, 2,
+ vms->oem_id, vms->oem_table_id);
+}
+
/* GTDT */
static void
build_gtdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
@@ -707,6 +765,11 @@ void virt_acpi_build(VirtMachineState *vms,
AcpiBuildTables *tables)
acpi_add_table(table_offsets, tables_blob);
build_madt(tables_blob, tables->linker, vms);
+ if (ms->smp.cpus > 1 && !vmc->no_cpu_topology) {
+ acpi_add_table(table_offsets, tables_blob);
+ build_pptt(tables_blob, tables->linker, vms);
+ }
+
acpi_add_table(table_offsets, tables_blob);
build_gtdt(tables_blob, tables->linker, vms);
--
2.19.1
Besides some changes that I think should be changed back and the 6.3
flags, this patch looks very similar to [1], so I'd prefer my
authorship be maintained. However, if my authorship is dropped, then
my s-o-b should be replaced with a Co-developed-by.