[PATCH v8 0/8] hw/arm/virt: Introduce cpu topology support

[PATCH v8 0/8] hw/arm/virt: Introduce cpu topology support

[Yanan Wang]

Hi,

This is the latest v8 with update in patch #6 and #8. Now only one
generic reference file for PPTT is added in tests/data/acpi/virt.
Machiel and Igor, please help to have a look, thanks!

Description of this series:
Once the view of an accurate virtual cpu topology is provided to guest,
with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
e.g., the scheduling performance improvement. See Dario Faggioli's
research and the related performance tests in [1] for reference.

This patch series introduces cpu topology support for Arm platform.
Both cpu-map in DT and ACPI PPTT table are introduced to store the
topology information. And we only describe the topology information
to 6.2 and newer virt machines, considering compatibility.

[1] https://kvmforum2020.sched.com/event/eE1y/virtual-topology-for-virtual-machines
-friend-or-foe-dario-faggioli-suse

Series tested locally on Arm64 machines kunpeng920.
After booting a Linux guest with "-smp 16,sockets=4,cores=4,threads=1,maxcpus=16",
through lscpu we will see the information about CPU topology like:
Architecture:        aarch64
Byte Order:          Little Endian
CPU(s):              16
On-line CPU(s) list: 0-15
Thread(s) per core:  1
Core(s) per socket:  4
Socket(s):           4
NUMA node(s):        1
Vendor ID:           0x48
Model:               0
Stepping:            0x1
BogoMIPS:            200.00
NUMA node0 CPU(s):   0-15

and with "-smp 16" we will see:
Architecture:        aarch64
Byte Order:          Little Endian
CPU(s):              16
On-line CPU(s) list: 0-15
Thread(s) per core:  1
Core(s) per socket:  16
Socket(s):           1
NUMA node(s):        1
Vendor ID:           0x48
Model:               0
Stepping:            0x1
BogoMIPS:            200.00
NUMA node0 CPU(s):   0-15

Changelog:
v7->v8:
- rebased on top of master (commit e5b2333f24)
- only add one generic expected file for PPTT instead of four, which works
  fine enough for now (patch #6 and #8 updated)
- v7: https://lore.kernel.org/qemu-devel/20211007030746.10420-1-wangyanan55@huawei.com/

v6->v7:
- rebased on top of master (commit ca61fa4b80)
- use newly introduced acpi_table_begin/acpi_table_end APIs to build PPTT
  (patch #5 updated)
- add reference files for PPTT to fix broken bios-table-test for Aarch64
  virt machine (patch #6-#8 added)
- v6: https://lore.kernel.org/qemu-devel/20210824122016.144364-1-wangyanan55@huawei.com/

Andrew Jones (2):
  hw/arm/virt: Add cpu-map to device tree
  hw/acpi/aml-build: Add PPTT table

Yanan Wang (6):
  hw/arm/virt: Only describe cpu topology since virt-6.2
  device_tree: Add qemu_fdt_add_path
  hw/acpi/aml-build: Add Processor hierarchy node structure
  tests/data/acpi/virt: Add an empty expected file for PPTT
  hw/arm/virt-acpi-build: Generate PPTT table
  tests/data/acpi/virt: Update the empty expected file for PPTT

 hw/acpi/aml-build.c          |  86 +++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c     |   8 +++-
 hw/arm/virt.c                |  71 +++++++++++++++++++++++++----
 include/hw/acpi/aml-build.h  |   7 +++
 include/hw/arm/virt.h        |   4 +-
 include/sysemu/device_tree.h |   1 +
 softmmu/device_tree.c        |  44 +++++++++++++++++-
 tests/data/acpi/virt/PPTT    | Bin 0 -> 76 bytes
 8 files changed, 207 insertions(+), 14 deletions(-)
 create mode 100644 tests/data/acpi/virt/PPTT

--
2.19.1

[PATCH v8 1/8] hw/arm/virt: Only describe cpu topology since virt-6.2

[Yanan Wang]

On existing older machine types, without cpu topology described
in ACPI or DT, the guest will populate one by default. With the
topology described, it will read the information and set up its
topology as instructed, but that may not be the same as what was
getting used by default. It's possible that an user application
has a dependency on the default topology and if the default one
gets changed it will probably behave differently.

Based on above consideration we'd better only describe topology
information to the guest on 6.2 and later machine types.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt.c         | 1 +
 include/hw/arm/virt.h | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7170aaacd5..d241516523 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2816,6 +2816,7 @@ static void virt_machine_6_1_options(MachineClass *mc)
     virt_machine_6_2_options(mc);
     compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
     mc->smp_props.prefer_sockets = true;
+    vmc->no_cpu_topology = true;
 
     /* qemu ITS was introduced with 6.2 */
     vmc->no_tcg_its = true;
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index b461b8d261..dc6b66ffc8 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -125,11 +125,13 @@ struct VirtMachineClass {
     bool claim_edge_triggered_timers;
     bool smbios_old_sys_ver;
     bool no_highmem_ecam;
-    bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
+    bool no_ged;   /* Machines < 4.2 have no support for ACPI GED device */
     bool kvm_no_adjvtime;
     bool no_kvm_steal_time;
     bool acpi_expose_flash;
     bool no_secure_gpio;
+    /* Machines < 6.2 have no support for describing cpu topology to guest */
+    bool no_cpu_topology;
 };
 
 struct VirtMachineState {
-- 
2.19.1

[PATCH v8 2/8] device_tree: Add qemu_fdt_add_path

[Yanan Wang]

qemu_fdt_add_path() works like qemu_fdt_add_subnode(), except it
also adds all missing subnodes from the given path. We'll use it
in a coming patch where we will add cpu-map to the device tree.

And we also tweak an error message of qemu_fdt_add_subnode().

Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Alistair Francis <alistair.francis@wdc.com>
Co-developed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 include/sysemu/device_tree.h |  1 +
 softmmu/device_tree.c        | 44 ++++++++++++++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 8a2fe55622..ef060a9759 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -121,6 +121,7 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
 uint32_t qemu_fdt_alloc_phandle(void *fdt);
 int qemu_fdt_nop_node(void *fdt, const char *node_path);
 int qemu_fdt_add_subnode(void *fdt, const char *name);
+int qemu_fdt_add_path(void *fdt, const char *path);
 
 #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
     do {                                                                      \
diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index b621f63fba..3965c834ca 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -540,8 +540,8 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
 
     retval = fdt_add_subnode(fdt, parent, basename);
     if (retval < 0) {
-        error_report("FDT: Failed to create subnode %s: %s", name,
-                     fdt_strerror(retval));
+        error_report("%s: Failed to create subnode %s: %s",
+                     __func__, name, fdt_strerror(retval));
         exit(1);
     }
 
@@ -549,6 +549,46 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
     return retval;
 }
 
+/*
+ * qemu_fdt_add_path: Like qemu_fdt_add_subnode(), but will add
+ * all missing subnodes from the given path.
+ */
+int qemu_fdt_add_path(void *fdt, const char *path)
+{
+    const char *name;
+    const char *p = path;
+    int namelen, retval;
+    int parent = 0;
+
+    if (path[0] != '/') {
+        return -1;
+    }
+
+    while (p) {
+        name = p + 1;
+        p = strchr(name, '/');
+        namelen = p != NULL ? p - name : strlen(name);
+
+        retval = fdt_subnode_offset_namelen(fdt, parent, name, namelen);
+        if (retval < 0 && retval != -FDT_ERR_NOTFOUND) {
+            error_report("%s: Unexpected error in finding subnode %.*s: %s",
+                         __func__, namelen, name, fdt_strerror(retval));
+            exit(1);
+        } else if (retval == -FDT_ERR_NOTFOUND) {
+            retval = fdt_add_subnode_namelen(fdt, parent, name, namelen);
+            if (retval < 0) {
+                error_report("%s: Failed to create subnode %.*s: %s",
+                             __func__, namelen, name, fdt_strerror(retval));
+                exit(1);
+            }
+        }
+
+        parent = retval;
+    }
+
+    return retval;
+}
+
 void qemu_fdt_dumpdtb(void *fdt, int size)
 {
     const char *dumpdtb = current_machine->dumpdtb;
-- 
2.19.1

[PATCH v8 3/8] hw/arm/virt: Add cpu-map to device tree

[Yanan Wang]

From: Andrew Jones <drjones@redhat.com>

Support device tree CPU topology descriptions.

In accordance with the Devicetree Specification, the Linux Doc
"arm/cpus.yaml" requires that cpus and cpu nodes in the DT are
present. And we have already met the requirement by generating
/cpus/cpu@* nodes for members within ms->smp.cpus. Accordingly,
we should also create subnodes in cpu-map for the present cpus,
each of which relates to an unique cpu node.

The Linux Doc "cpu/cpu-topology.txt" states that the hierarchy
of CPUs in a SMP system is defined through four entities and
they are socket/cluster/core/thread. It is also required that
a socket node's child nodes must be one or more cluster nodes.
Given that currently we are only provided with information of
socket/core/thread, we assume there is one cluster child node
in each socket node when creating cpu-map.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/arm/virt.c | 70 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 60 insertions(+), 10 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d241516523..f80af19cd3 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -351,20 +351,21 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
     int cpu;
     int addr_cells = 1;
     const MachineState *ms = MACHINE(vms);
+    const VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
     int smp_cpus = ms->smp.cpus;
 
     /*
-     * From Documentation/devicetree/bindings/arm/cpus.txt
-     *  On ARM v8 64-bit systems value should be set to 2,
-     *  that corresponds to the MPIDR_EL1 register size.
-     *  If MPIDR_EL1[63:32] value is equal to 0 on all CPUs
-     *  in the system, #address-cells can be set to 1, since
-     *  MPIDR_EL1[63:32] bits are not used for CPUs
-     *  identification.
+     * See Linux Documentation/devicetree/bindings/arm/cpus.yaml
+     * On ARM v8 64-bit systems value should be set to 2,
+     * that corresponds to the MPIDR_EL1 register size.
+     * If MPIDR_EL1[63:32] value is equal to 0 on all CPUs
+     * in the system, #address-cells can be set to 1, since
+     * MPIDR_EL1[63:32] bits are not used for CPUs
+     * identification.
      *
-     *  Here we actually don't know whether our system is 32- or 64-bit one.
-     *  The simplest way to go is to examine affinity IDs of all our CPUs. If
-     *  at least one of them has Aff3 populated, we set #address-cells to 2.
+     * Here we actually don't know whether our system is 32- or 64-bit one.
+     * The simplest way to go is to examine affinity IDs of all our CPUs. If
+     * at least one of them has Aff3 populated, we set #address-cells to 2.
      */
     for (cpu = 0; cpu < smp_cpus; cpu++) {
         ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(cpu));
@@ -407,8 +408,57 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
                 ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
         }
 
+        if (!vmc->no_cpu_topology) {
+            qemu_fdt_setprop_cell(ms->fdt, nodename, "phandle",
+                                  qemu_fdt_alloc_phandle(ms->fdt));
+        }
+
         g_free(nodename);
     }
+
+    if (!vmc->no_cpu_topology) {
+        /*
+         * Add vCPU topology description through fdt node cpu-map.
+         *
+         * See Linux Documentation/devicetree/bindings/cpu/cpu-topology.txt
+         * In a SMP system, the hierarchy of CPUs can be defined through
+         * four entities that are used to describe the layout of CPUs in
+         * the system: socket/cluster/core/thread.
+         *
+         * A socket node represents the boundary of system physical package
+         * and its child nodes must be one or more cluster nodes. A system
+         * 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.
+         */
+        qemu_fdt_add_subnode(ms->fdt, "/cpus/cpu-map");
+
+        for (cpu = smp_cpus - 1; cpu >= 0; cpu--) {
+            char *cpu_path = g_strdup_printf("/cpus/cpu@%d", cpu);
+            char *map_path;
+
+            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),
+                    (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,
+                    cpu % ms->smp.cores);
+            }
+            qemu_fdt_add_path(ms->fdt, map_path);
+            qemu_fdt_setprop_phandle(ms->fdt, map_path, "cpu", cpu_path);
+
+            g_free(map_path);
+            g_free(cpu_path);
+        }
+    }
 }
 
 static void fdt_add_its_gic_node(VirtMachineState *vms)
-- 
2.19.1

[PATCH v8 4/8] hw/acpi/aml-build: Add Processor hierarchy node structure

[Yanan Wang]

Add a generic API to build Processor hierarchy node structure (Type 0),
which is strictly consistent with descriptions in ACPI 6.2: 5.2.29.1.

This function will be used to build ACPI PPTT table for cpu topology.

Co-developed-by: Ying Fang <fangying1@huawei.com>
Co-developed-by: Henglong Fan <fanhenglong@huawei.com>
Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/acpi/aml-build.c         | 26 ++++++++++++++++++++++++++
 include/hw/acpi/aml-build.h |  4 ++++
 2 files changed, 30 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 76af0ebaf9..b7b9db6888 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1964,6 +1964,32 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
     acpi_table_end(linker, &table);
 }
 
+/* ACPI 6.2: 5.2.29.1 Processor hierarchy node structure (Type 0) */
+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;
+
+    build_append_byte(tbl, 0);                 /* Type 0 - processor */
+    build_append_byte(tbl, 20 + priv_num * 4); /* Length */
+    build_append_int_noprefix(tbl, 0, 2);      /* Reserved */
+    build_append_int_noprefix(tbl, flags, 4);  /* Flags */
+    build_append_int_noprefix(tbl, parent, 4); /* Parent */
+    build_append_int_noprefix(tbl, id, 4);     /* ACPI Processor ID */
+
+    /* Number of private resources */
+    build_append_int_noprefix(tbl, priv_num, 4);
+
+    /* Private resources[N] */
+    if (priv_num > 0) {
+        assert(priv_rsrc);
+        for (i = 0; i < priv_num; i++) {
+            build_append_int_noprefix(tbl, priv_rsrc[i], 4);
+        }
+    }
+}
+
 /* 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/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 3cf6f2c1b9..2c457c8f17 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -489,6 +489,10 @@ 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_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.19.1

[PATCH v8 5/8] hw/acpi/aml-build: Add PPTT table

[Yanan Wang]

From: Andrew Jones <drjones@redhat.com>

Add the Processor Properties Topology Table (PPTT) used to
describe CPU topology information to ACPI guests.

Note, a DT-boot Linux guest with a non-flat CPU topology will
see socket and core IDs being sequential integers starting
from zero, which is different from ACPI-boot Linux guest,
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 mandatorily 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.

So in summary, with QEMU as the vendor for the guests, we simply
use sequential integers starting from zero for the non-leaf nodes
but with ID-valid flag unset, so that guest will ignore them and
use table offsets as unique container IDs. And we use logical CPU
IDs for the leaf nodes with the ID-valid flag set, which will be
consistent with MADT.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/acpi/aml-build.c         | 60 +++++++++++++++++++++++++++++++++++++
 include/hw/acpi/aml-build.h |  3 ++
 2 files changed, 63 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index b7b9db6888..0d50e88e9d 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1990,6 +1990,66 @@ void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
     }
 }
 
+/* ACPI 6.2: 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)
+{
+    int pptt_start = table_data->len;
+    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++) {
+        uint32_t socket_offset = table_data->len - pptt_start;
+        int core;
+
+        build_processor_hierarchy_node(
+            table_data,
+            /*
+             * ACPI 6.2 - Physical package
+             * represents the boundary of a physical package
+             */
+            (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;
+
+            if (ms->smp.threads > 1) {
+                build_processor_hierarchy_node(
+                    table_data,
+                    /*
+                     * ACPI 6.2 - Physical package
+                     * doesn't represent the boundary of a physical package
+                     */
+                    (0 << 0),
+                    socket_offset, core, NULL, 0);
+
+                for (thread = 0; thread < ms->smp.threads; thread++) {
+                    build_processor_hierarchy_node(
+                        table_data,
+                        (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
+                        (1 << 2) | /* ACPI 6.3 - Processor is a Thread */
+                        (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
+                        core_offset, uid++, NULL, 0);
+                }
+            } else {
+                build_processor_hierarchy_node(
+                    table_data,
+                    (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
+                    (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
+                    socket_offset, uid++, NULL, 0);
+            }
+        }
+    }
+
+    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/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 2c457c8f17..b92706388c 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -493,6 +493,9 @@ 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_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
+                const char *oem_id, const char *oem_table_id);
+
 void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                 const char *oem_id, const char *oem_table_id);
 
-- 
2.19.1

[PATCH v8 6/8] tests/data/acpi/virt: Add an empty expected file for PPTT

[Yanan Wang]

Add a generic empty binary file for the new introduced PPTT table
under tests/data/acpi/virt, and list it as files to be changed in
tests/qtest/bios-tables-test-allowed-diff.h

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 tests/data/acpi/virt/PPTT                   | 0
 tests/qtest/bios-tables-test-allowed-diff.h | 1 +
 2 files changed, 1 insertion(+)
 create mode 100644 tests/data/acpi/virt/PPTT

diff --git a/tests/data/acpi/virt/PPTT b/tests/data/acpi/virt/PPTT
new file mode 100644
index 0000000000..e69de29bb2
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.19.1

[PATCH v8 7/8] hw/arm/virt-acpi-build: Generate PPTT table

[Yanan Wang]

Generate PPTT table for Arm virt machines.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt-acpi-build.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 6cec97352b..658a0a5d39 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -875,13 +875,19 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     dsdt = tables_blob->len;
     build_dsdt(tables_blob, tables->linker, vms);
 
-    /* FADT MADT GTDT MCFG SPCR pointed to by RSDT */
+    /* FADT MADT PPTT GTDT MCFG SPCR pointed to by RSDT */
     acpi_add_table(table_offsets, tables_blob);
     build_fadt_rev5(tables_blob, tables->linker, vms, dsdt);
 
     acpi_add_table(table_offsets, tables_blob);
     build_madt(tables_blob, tables->linker, vms);
 
+    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);
+    }
+
     acpi_add_table(table_offsets, tables_blob);
     build_gtdt(tables_blob, tables->linker, vms);
 
-- 
2.19.1

[PATCH v8 8/8] tests/data/acpi/virt: Update the empty expected file for PPTT

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

Disassembled output of the updated new file:

/*
 * 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, Fri Oct  8 10:12:32 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
[008h 0008   1]                     Revision : 02
[009h 0009   1]                     Checksum : A8
[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
                            Physical package : 0
                     ACPI Processor ID valid : 1
[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)

    0000: 50 50 54 54 4C 00 00 00 02 A8 42 4F 43 48 53 20  // PPTTL.....BOCHS
    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              // $...........

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 tests/data/acpi/virt/PPTT                   | Bin 0 -> 76 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 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..7a1258ecf123555b24462c98ccbb76b4ac1d0c2b 100644
GIT binary patch
literal 76
zcmV-S0JHy4P*hY*00002s6tOeNK+s}SWrVCARr(C0000&SWrU&000006aWAL00000
i0000000000000006aWAU0000a00000000000001^0tW#A

literal 0
KcmV+b0RR6000031

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.19.1

Re: [PATCH v8 0/8] hw/arm/virt: Introduce cpu topology support

[wangyanan (Y)]

Ping... for review of the newly added ACPI patches.

On 2021/10/14 21:21, Yanan Wang wrote:
> Hi,
>
> This is the latest v8 with update in patch #6 and #8. Now only one
> generic reference file for PPTT is added in tests/data/acpi/virt.
> Machiel and Igor, please help to have a look, thanks!
And sorry for the name typo, definitely should be Michael of course.

Thanks,
Yanan
> Description of this series:
> Once the view of an accurate virtual cpu topology is provided to guest,
> with a well-designed vCPU pinning to the pCPU we may get a huge benefit,
> e.g., the scheduling performance improvement. See Dario Faggioli's
> research and the related performance tests in [1] for reference.
>
> This patch series introduces cpu topology support for Arm platform.
> Both cpu-map in DT and ACPI PPTT table are introduced to store the
> topology information. And we only describe the topology information
> to 6.2 and newer virt machines, considering compatibility.
>
> [1] https://kvmforum2020.sched.com/event/eE1y/virtual-topology-for-virtual-machines
> -friend-or-foe-dario-faggioli-suse
>
> Series tested locally on Arm64 machines kunpeng920.
> After booting a Linux guest with "-smp 16,sockets=4,cores=4,threads=1,maxcpus=16",
> through lscpu we will see the information about CPU topology like:
> Architecture:        aarch64
> Byte Order:          Little Endian
> CPU(s):              16
> On-line CPU(s) list: 0-15
> Thread(s) per core:  1
> Core(s) per socket:  4
> Socket(s):           4
> NUMA node(s):        1
> Vendor ID:           0x48
> Model:               0
> Stepping:            0x1
> BogoMIPS:            200.00
> NUMA node0 CPU(s):   0-15
>
> and with "-smp 16" we will see:
> Architecture:        aarch64
> Byte Order:          Little Endian
> CPU(s):              16
> On-line CPU(s) list: 0-15
> Thread(s) per core:  1
> Core(s) per socket:  16
> Socket(s):           1
> NUMA node(s):        1
> Vendor ID:           0x48
> Model:               0
> Stepping:            0x1
> BogoMIPS:            200.00
> NUMA node0 CPU(s):   0-15
>
> Changelog:
> v7->v8:
> - rebased on top of master (commit e5b2333f24)
> - only add one generic expected file for PPTT instead of four, which works
>    fine enough for now (patch #6 and #8 updated)
> - v7: https://lore.kernel.org/qemu-devel/20211007030746.10420-1-wangyanan55@huawei.com/
>
> v6->v7:
> - rebased on top of master (commit ca61fa4b80)
> - use newly introduced acpi_table_begin/acpi_table_end APIs to build PPTT
>    (patch #5 updated)
> - add reference files for PPTT to fix broken bios-table-test for Aarch64
>    virt machine (patch #6-#8 added)
> - v6: https://lore.kernel.org/qemu-devel/20210824122016.144364-1-wangyanan55@huawei.com/
>
> Andrew Jones (2):
>    hw/arm/virt: Add cpu-map to device tree
>    hw/acpi/aml-build: Add PPTT table
>
> Yanan Wang (6):
>    hw/arm/virt: Only describe cpu topology since virt-6.2
>    device_tree: Add qemu_fdt_add_path
>    hw/acpi/aml-build: Add Processor hierarchy node structure
>    tests/data/acpi/virt: Add an empty expected file for PPTT
>    hw/arm/virt-acpi-build: Generate PPTT table
>    tests/data/acpi/virt: Update the empty expected file for PPTT
>
>   hw/acpi/aml-build.c          |  86 +++++++++++++++++++++++++++++++++++
>   hw/arm/virt-acpi-build.c     |   8 +++-
>   hw/arm/virt.c                |  71 +++++++++++++++++++++++++----
>   include/hw/acpi/aml-build.h  |   7 +++
>   include/hw/arm/virt.h        |   4 +-
>   include/sysemu/device_tree.h |   1 +
>   softmmu/device_tree.c        |  44 +++++++++++++++++-
>   tests/data/acpi/virt/PPTT    | Bin 0 -> 76 bytes
>   8 files changed, 207 insertions(+), 14 deletions(-)
>   create mode 100644 tests/data/acpi/virt/PPTT
>
> --
> 2.19.1
>
> .

Re: [PATCH v8 8/8] tests/data/acpi/virt: Update the empty expected file for PPTT

[wangyanan (Y)]

Ping...

On 2021/10/14 21:22, 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.
>
> Disassembled output of the updated new file:
>
> /*
>   * 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, Fri Oct  8 10:12:32 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
> [008h 0008   1]                     Revision : 02
> [009h 0009   1]                     Checksum : A8
> [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
>                              Physical package : 0
>                       ACPI Processor ID valid : 1
> [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)
>
>      0000: 50 50 54 54 4C 00 00 00 02 A8 42 4F 43 48 53 20  // PPTTL.....BOCHS
>      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              // $...........
>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>   tests/data/acpi/virt/PPTT                   | Bin 0 -> 76 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 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..7a1258ecf123555b24462c98ccbb76b4ac1d0c2b 100644
> GIT binary patch
> literal 76
> zcmV-S0JHy4P*hY*00002s6tOeNK+s}SWrVCARr(C0000&SWrU&000006aWAL00000
> i0000000000000006aWAU0000a00000000000001^0tW#A
>
> literal 0
> KcmV+b0RR6000031
>
> 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",

Re: [PATCH v8 6/8] tests/data/acpi/virt: Add an empty expected file for PPTT

[wangyanan (Y)]

Ping...

On 2021/10/14 21:22, Yanan Wang wrote:
> Add a generic empty binary file for the new introduced PPTT table
> under tests/data/acpi/virt, and list it as files to be changed in
> tests/qtest/bios-tables-test-allowed-diff.h
>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>   tests/data/acpi/virt/PPTT                   | 0
>   tests/qtest/bios-tables-test-allowed-diff.h | 1 +
>   2 files changed, 1 insertion(+)
>   create mode 100644 tests/data/acpi/virt/PPTT
>
> diff --git a/tests/data/acpi/virt/PPTT b/tests/data/acpi/virt/PPTT
> new file mode 100644
> index 0000000000..e69de29bb2
> 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",

Re: [PATCH v8 6/8] tests/data/acpi/virt: Add an empty expected file for PPTT

[Eric Auger]

Hi Yanan,
On 10/14/21 3:22 PM, Yanan Wang wrote:
> Add a generic empty binary file for the new introduced PPTT table
> under tests/data/acpi/virt, and list it as files to be changed in> tests/qtest/bios-tables-test-allowed-diff.h
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  tests/data/acpi/virt/PPTT                   | 0
>  tests/qtest/bios-tables-test-allowed-diff.h | 1 +
>  2 files changed, 1 insertion(+)
>  create mode 100644 tests/data/acpi/virt/PPTT
> 
> diff --git a/tests/data/acpi/virt/PPTT b/tests/data/acpi/virt/PPTT
> new file mode 100644
> index 0000000000..e69de29bb2
> 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",
> 

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric

Re: [PATCH v8 4/8] hw/acpi/aml-build: Add Processor hierarchy node structure

[Eric Auger]

Hi

On 10/14/21 3:22 PM, Yanan Wang wrote:
> Add a generic API to build Processor hierarchy node structure (Type 0),
> which is strictly consistent with descriptions in ACPI 6.2: 5.2.29.1.
> 
> This function will be used to build ACPI PPTT table for cpu topology.
> 
> Co-developed-by: Ying Fang <fangying1@huawei.com>
> Co-developed-by: Henglong Fan <fanhenglong@huawei.com>
> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/acpi/aml-build.c         | 26 ++++++++++++++++++++++++++
>  include/hw/acpi/aml-build.h |  4 ++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 76af0ebaf9..b7b9db6888 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1964,6 +1964,32 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>      acpi_table_end(linker, &table);
>  }
>  
> +/* ACPI 6.2: 5.2.29.1 Processor hierarchy node structure (Type 0) */
> +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;
> +
> +    build_append_byte(tbl, 0);                 /* Type 0 - processor */
nit: I think Igor would generally prefer
build_append_byte(tbl, 0 /* processor structure */);  /* Type */
although it generates checkpatch warnings

> +    build_append_byte(tbl, 20 + priv_num * 4); /* Length */
> +    build_append_int_noprefix(tbl, 0, 2);      /* Reserved */
> +    build_append_int_noprefix(tbl, flags, 4);  /* Flags */
> +    build_append_int_noprefix(tbl, parent, 4); /* Parent */
> +    build_append_int_noprefix(tbl, id, 4);     /* ACPI Processor ID */
> +
> +    /* Number of private resources */
> +    build_append_int_noprefix(tbl, priv_num, 4);
> +
> +    /* Private resources[N] */
> +    if (priv_num > 0) {
> +        assert(priv_rsrc);
> +        for (i = 0; i < priv_num; i++) {
> +            build_append_int_noprefix(tbl, priv_rsrc[i], 4);
> +        }
> +    }
> +}
> +
>  /* 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/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 3cf6f2c1b9..2c457c8f17 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -489,6 +489,10 @@ 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_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);
>  
> 
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>


Eric

Re: [PATCH v8 5/8] hw/acpi/aml-build: Add PPTT table

[Eric Auger]

Hi,

On 10/14/21 3:22 PM, Yanan Wang wrote:
> From: Andrew Jones <drjones@redhat.com>
> 
> Add the Processor Properties Topology Table (PPTT) used to
> describe CPU topology information to ACPI guests.
> 
> Note, a DT-boot Linux guest with a non-flat CPU topology will
> see socket and core IDs being sequential integers starting
> from zero, which is different from ACPI-boot Linux guest,
> 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 mandatorily 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.
> 
> So in summary, with QEMU as the vendor for the guests, we simply
> use sequential integers starting from zero for the non-leaf nodes
> but with ID-valid flag unset, so that guest will ignore them and
> use table offsets as unique container IDs. And we use logical CPU
> IDs for the leaf nodes with the ID-valid flag set, which will be
> consistent with MADT.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/acpi/aml-build.c         | 60 +++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/aml-build.h |  3 ++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index b7b9db6888..0d50e88e9d 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1990,6 +1990,66 @@ void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
>      }
>  }
>  
> +/* ACPI 6.2: 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)
> +{
> +    int pptt_start = table_data->len;
> +    int uid = 0;
> +    int socket;
> +    AcpiTable table = { .sig = "PPTT", .rev = 2,
> +                        .oem_id = oem_id, .oem_table_id = oem_table_id };
Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is
it an erratum somewhere I did miss?

I would also add the spec version in the commit msg.
> +
> +    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;
> +
> +        build_processor_hierarchy_node(
> +            table_data,
> +            /*
> +             * ACPI 6.2 - Physical package
> +             * represents the boundary of a physical package
> +             */
> +            (1 << 0),
> +            0, socket, NULL, 0);
I see we set an ACPI process ID but in the meantime the ACPI processor
ID valid flag is not set. I am not sure I fully catch the meaning of
this latter but just to double check if this is done on purpose. Maybe
wort a general comment as this also happens below.
> +
> +        for (core = 0; core < ms->smp.cores; core++) {
> +            uint32_t core_offset = table_data->len - pptt_start;
> +            int thread;
> +
> +            if (ms->smp.threads > 1) {
> +                build_processor_hierarchy_node(
> +                    table_data,
> +                    /*
> +                     * ACPI 6.2 - Physical package
> +                     * doesn't represent the boundary of a physical package
> +                     */
> +                    (0 << 0),
would rather say (0 << 0) /* not a physical package */ and same elsewhere
> +                    socket_offset, core, NULL, 0);
> +
> +                for (thread = 0; thread < ms->smp.threads; thread++) {
> +                    build_processor_hierarchy_node(
> +                        table_data,
> +                        (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
> +                        (1 << 2) | /* ACPI 6.3 - Processor is a Thread */
So the references look globaly confusing to me. Either it complies to
6.2 or to 6.3. Looks ir rather complies with 6.3. To me, this needs to
be clarified. I would also add the reference it complies to in the
commit msg.
> +                        (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
> +                        core_offset, uid++, NULL, 0);
> +                }
> +            } else {
> +                build_processor_hierarchy_node(
> +                    table_data,
> +                    (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
> +                    (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
> +                    socket_offset, uid++, NULL, 0);
> +            }
> +        }
> +    }
> +
> +    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/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 2c457c8f17..b92706388c 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -493,6 +493,9 @@ 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_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> +                const char *oem_id, const char *oem_table_id);
> +
>  void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>                  const char *oem_id, const char *oem_table_id);
>  
> 
Thanks

Eric

Re: [PATCH v8 7/8] hw/arm/virt-acpi-build: Generate PPTT table

[Eric Auger]

Hi

On 10/14/21 3:22 PM, Yanan Wang wrote:
> Generate PPTT table for Arm virt machines.
Generate the Processor Properties Topology Table (PPTT) for ARM virt
machines supporting it (>= 6.2)
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 6cec97352b..658a0a5d39 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -875,13 +875,19 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>      dsdt = tables_blob->len;
>      build_dsdt(tables_blob, tables->linker, vms);
>  
> -    /* FADT MADT GTDT MCFG SPCR pointed to by RSDT */
> +    /* FADT MADT PPTT GTDT MCFG SPCR pointed to by RSDT */
>      acpi_add_table(table_offsets, tables_blob);
>      build_fadt_rev5(tables_blob, tables->linker, vms, dsdt);
>  
>      acpi_add_table(table_offsets, tables_blob);
>      build_madt(tables_blob, tables->linker, vms);
>  
> +    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);
> +    }
> +
>      acpi_add_table(table_offsets, tables_blob);
>      build_gtdt(tables_blob, tables->linker, vms);
>  
> 

Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

Re: [PATCH v8 5/8] hw/acpi/aml-build: Add PPTT table

[wangyanan (Y)]

Hi Eric,

On 2021/10/20 16:02, Eric Auger wrote:
> Hi,
>
> On 10/14/21 3:22 PM, Yanan Wang wrote:
>> From: Andrew Jones <drjones@redhat.com>
>>
>> Add the Processor Properties Topology Table (PPTT) used to
>> describe CPU topology information to ACPI guests.
>>
>> Note, a DT-boot Linux guest with a non-flat CPU topology will
>> see socket and core IDs being sequential integers starting
>> from zero, which is different from ACPI-boot Linux guest,
>> 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 mandatorily 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.
>>
>> So in summary, with QEMU as the vendor for the guests, we simply
>> use sequential integers starting from zero for the non-leaf nodes
>> but with ID-valid flag unset, so that guest will ignore them and
>> use table offsets as unique container IDs. And we use logical CPU
>> IDs for the leaf nodes with the ID-valid flag set, which will be
>> consistent with MADT.
>>
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>   hw/acpi/aml-build.c         | 60 +++++++++++++++++++++++++++++++++++++
>>   include/hw/acpi/aml-build.h |  3 ++
>>   2 files changed, 63 insertions(+)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index b7b9db6888..0d50e88e9d 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -1990,6 +1990,66 @@ void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
>>       }
>>   }
>>   
>> +/* ACPI 6.2: 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)
>> +{
>> +    int pptt_start = table_data->len;
>> +    int uid = 0;
>> +    int socket;
>> +    AcpiTable table = { .sig = "PPTT", .rev = 2,
>> +                        .oem_id = oem_id, .oem_table_id = oem_table_id };
> Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is
> it an erratum somewhere I did miss?
Yes, the revision in 6.2 spec is 1. And it's 2 in spec 6.3.
So just to be sure, should I use the oldest revision ?
> I would also add the spec version in the commit msg.
>> +
>> +    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;
>> +
>> +        build_processor_hierarchy_node(
>> +            table_data,
>> +            /*
>> +             * ACPI 6.2 - Physical package
>> +             * represents the boundary of a physical package
>> +             */
>> +            (1 << 0),
>> +            0, socket, NULL, 0);
> I see we set an ACPI process ID but in the meantime the ACPI processor
> ID valid flag is not set. I am not sure I fully catch the meaning of
> this latter but just to double check if this is done on purpose.
Yes, it's on purpose.
> Maybe
> wort a general comment as this also happens below.
The ID of the container node is invalid and ID of the leaf node is valid.
The commit message by Andrew has explained why (reason 3). I think
it may be clear enough to explain there why we don't need a valid ID
for the container node.
>> +
>> +        for (core = 0; core < ms->smp.cores; core++) {
>> +            uint32_t core_offset = table_data->len - pptt_start;
>> +            int thread;
>> +
>> +            if (ms->smp.threads > 1) {
>> +                build_processor_hierarchy_node(
>> +                    table_data,
>> +                    /*
>> +                     * ACPI 6.2 - Physical package
>> +                     * doesn't represent the boundary of a physical package
>> +                     */
>> +                    (0 << 0),
> would rather say (0 << 0) /* not a physical package */ and same elsewhere
Ok, thanks.
>> +                    socket_offset, core, NULL, 0);
>> +
>> +                for (thread = 0; thread < ms->smp.threads; thread++) {
>> +                    build_processor_hierarchy_node(
>> +                        table_data,
>> +                        (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
>> +                        (1 << 2) | /* ACPI 6.3 - Processor is a Thread */
> So the references look globaly confusing to me. Either it complies to
> 6.2 or to 6.3. Looks ir rather complies with 6.3. To me, this needs to
> be clarified.
ACPI 6.2 in the comment means the flag is introduced in the spec since 6.2.
The same, ACPI 6.3 means the flag is introduced since 6.3. Maybe I should
just drop all the version-prefix in the comment ?
> I would also add the reference it complies to in the
> commit msg.
Ok, sure.

Thanks,
Yanan
.
>> +                        (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
>> +                        core_offset, uid++, NULL, 0);
>> +                }
>> +            } else {
>> +                build_processor_hierarchy_node(
>> +                    table_data,
>> +                    (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
>> +                    (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
>> +                    socket_offset, uid++, NULL, 0);
>> +            }
>> +        }
>> +    }
>> +
>> +    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/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index 2c457c8f17..b92706388c 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -493,6 +493,9 @@ 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_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>> +                const char *oem_id, const char *oem_table_id);
>> +
>>   void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>>                   const char *oem_id, const char *oem_table_id);
>>   
>>
> Thanks
>
> Eric
>
> .

Re: [PATCH v8 4/8] hw/acpi/aml-build: Add Processor hierarchy node structure

[wangyanan (Y)]

On 2021/10/20 15:43, Eric Auger wrote:
> Hi
>
> On 10/14/21 3:22 PM, Yanan Wang wrote:
>> Add a generic API to build Processor hierarchy node structure (Type 0),
>> which is strictly consistent with descriptions in ACPI 6.2: 5.2.29.1.
>>
>> This function will be used to build ACPI PPTT table for cpu topology.
>>
>> Co-developed-by: Ying Fang <fangying1@huawei.com>
>> Co-developed-by: Henglong Fan <fanhenglong@huawei.com>
>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>   hw/acpi/aml-build.c         | 26 ++++++++++++++++++++++++++
>>   include/hw/acpi/aml-build.h |  4 ++++
>>   2 files changed, 30 insertions(+)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 76af0ebaf9..b7b9db6888 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -1964,6 +1964,32 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>>       acpi_table_end(linker, &table);
>>   }
>>   
>> +/* ACPI 6.2: 5.2.29.1 Processor hierarchy node structure (Type 0) */
>> +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;
>> +
>> +    build_append_byte(tbl, 0);                 /* Type 0 - processor */
> nit: I think Igor would generally prefer
> build_append_byte(tbl, 0 /* processor structure */);  /* Type */
> although it generates checkpatch warnings
Ok.
>
>> +    build_append_byte(tbl, 20 + priv_num * 4); /* Length */
>> +    build_append_int_noprefix(tbl, 0, 2);      /* Reserved */
>> +    build_append_int_noprefix(tbl, flags, 4);  /* Flags */
>> +    build_append_int_noprefix(tbl, parent, 4); /* Parent */
>> +    build_append_int_noprefix(tbl, id, 4);     /* ACPI Processor ID */
>> +
>> +    /* Number of private resources */
>> +    build_append_int_noprefix(tbl, priv_num, 4);
>> +
>> +    /* Private resources[N] */
>> +    if (priv_num > 0) {
>> +        assert(priv_rsrc);
>> +        for (i = 0; i < priv_num; i++) {
>> +            build_append_int_noprefix(tbl, priv_rsrc[i], 4);
>> +        }
>> +    }
>> +}
>> +
>>   /* 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/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index 3cf6f2c1b9..2c457c8f17 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -489,6 +489,10 @@ 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_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);
>>   
>>
> Besides
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks for your review!

Yanan
.
>
> Eric
>
> .

Re: [PATCH v8 7/8] hw/arm/virt-acpi-build: Generate PPTT table

[wangyanan (Y)]

On 2021/10/20 16:05, Eric Auger wrote:
> Hi
>
> On 10/14/21 3:22 PM, Yanan Wang wrote:
>> Generate PPTT table for Arm virt machines.
> Generate the Processor Properties Topology Table (PPTT) for ARM virt
> machines supporting it (>= 6.2)
Ok, much clearer. I will add it to the commit msg.

Thanks,
Yanan
.
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>> ---
>>   hw/arm/virt-acpi-build.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 6cec97352b..658a0a5d39 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -875,13 +875,19 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>       dsdt = tables_blob->len;
>>       build_dsdt(tables_blob, tables->linker, vms);
>>   
>> -    /* FADT MADT GTDT MCFG SPCR pointed to by RSDT */
>> +    /* FADT MADT PPTT GTDT MCFG SPCR pointed to by RSDT */
>>       acpi_add_table(table_offsets, tables_blob);
>>       build_fadt_rev5(tables_blob, tables->linker, vms, dsdt);
>>   
>>       acpi_add_table(table_offsets, tables_blob);
>>       build_madt(tables_blob, tables->linker, vms);
>>   
>> +    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);
>> +    }
>> +
>>       acpi_add_table(table_offsets, tables_blob);
>>       build_gtdt(tables_blob, tables->linker, vms);
>>   
>>
> Besides
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>
> Eric
>
> .

Re: [PATCH v8 4/8] hw/acpi/aml-build: Add Processor hierarchy node structure

[Michael S. Tsirkin]

On Wed, Oct 20, 2021 at 05:51:46PM +0800, wangyanan (Y) wrote:
> 
> On 2021/10/20 15:43, Eric Auger wrote:
> > Hi
> > 
> > On 10/14/21 3:22 PM, Yanan Wang wrote:
> > > Add a generic API to build Processor hierarchy node structure (Type 0),
> > > which is strictly consistent with descriptions in ACPI 6.2: 5.2.29.1.
> > > 
> > > This function will be used to build ACPI PPTT table for cpu topology.
> > > 
> > > Co-developed-by: Ying Fang <fangying1@huawei.com>
> > > Co-developed-by: Henglong Fan <fanhenglong@huawei.com>
> > > Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >   hw/acpi/aml-build.c         | 26 ++++++++++++++++++++++++++
> > >   include/hw/acpi/aml-build.h |  4 ++++
> > >   2 files changed, 30 insertions(+)
> > > 
> > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > index 76af0ebaf9..b7b9db6888 100644
> > > --- a/hw/acpi/aml-build.c
> > > +++ b/hw/acpi/aml-build.c
> > > @@ -1964,6 +1964,32 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> > >       acpi_table_end(linker, &table);
> > >   }
> > > +/* ACPI 6.2: 5.2.29.1 Processor hierarchy node structure (Type 0) */
> > > +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;
> > > +
> > > +    build_append_byte(tbl, 0);                 /* Type 0 - processor */
> > nit: I think Igor would generally prefer
> > build_append_byte(tbl, 0 /* processor structure */);  /* Type */
> > although it generates checkpatch warnings
> Ok.

I think it's ok as is. Right Igor?

> > 
> > > +    build_append_byte(tbl, 20 + priv_num * 4); /* Length */
> > > +    build_append_int_noprefix(tbl, 0, 2);      /* Reserved */
> > > +    build_append_int_noprefix(tbl, flags, 4);  /* Flags */
> > > +    build_append_int_noprefix(tbl, parent, 4); /* Parent */
> > > +    build_append_int_noprefix(tbl, id, 4);     /* ACPI Processor ID */
> > > +
> > > +    /* Number of private resources */
> > > +    build_append_int_noprefix(tbl, priv_num, 4);
> > > +
> > > +    /* Private resources[N] */
> > > +    if (priv_num > 0) {
> > > +        assert(priv_rsrc);
> > > +        for (i = 0; i < priv_num; i++) {
> > > +            build_append_int_noprefix(tbl, priv_rsrc[i], 4);
> > > +        }
> > > +    }
> > > +}
> > > +
> > >   /* 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/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > > index 3cf6f2c1b9..2c457c8f17 100644
> > > --- a/include/hw/acpi/aml-build.h
> > > +++ b/include/hw/acpi/aml-build.h
> > > @@ -489,6 +489,10 @@ 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_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);
> > > 
> > Besides
> > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Thanks for your review!
> 
> Yanan
> .
> > 
> > Eric
> > 
> > .

Re: [PATCH v8 5/8] hw/acpi/aml-build: Add PPTT table

[Eric Auger]

Hi Yanan,
On 10/20/21 11:51 AM, wangyanan (Y) wrote:
> Hi Eric,
> 
> On 2021/10/20 16:02, Eric Auger wrote:
>> Hi,
>>
>> On 10/14/21 3:22 PM, Yanan Wang wrote:
>>> From: Andrew Jones <drjones@redhat.com>
>>>
>>> Add the Processor Properties Topology Table (PPTT) used to
>>> describe CPU topology information to ACPI guests.
>>>
>>> Note, a DT-boot Linux guest with a non-flat CPU topology will
>>> see socket and core IDs being sequential integers starting
>>> from zero, which is different from ACPI-boot Linux guest,
>>> 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 mandatorily 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.
>>>
>>> So in summary, with QEMU as the vendor for the guests, we simply
>>> use sequential integers starting from zero for the non-leaf nodes
>>> but with ID-valid flag unset, so that guest will ignore them and
>>> use table offsets as unique container IDs. And we use logical CPU
>>> IDs for the leaf nodes with the ID-valid flag set, which will be
>>> consistent with MADT.
>>>
>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>   hw/acpi/aml-build.c         | 60 +++++++++++++++++++++++++++++++++++++
>>>   include/hw/acpi/aml-build.h |  3 ++
>>>   2 files changed, 63 insertions(+)
>>>
>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>> index b7b9db6888..0d50e88e9d 100644
>>> --- a/hw/acpi/aml-build.c
>>> +++ b/hw/acpi/aml-build.c
>>> @@ -1990,6 +1990,66 @@ void build_processor_hierarchy_node(GArray
>>> *tbl, uint32_t flags,
>>>       }
>>>   }
>>>   +/* ACPI 6.2: 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)
>>> +{
>>> +    int pptt_start = table_data->len;
>>> +    int uid = 0;
>>> +    int socket;
>>> +    AcpiTable table = { .sig = "PPTT", .rev = 2,
>>> +                        .oem_id = oem_id, .oem_table_id =
>>> oem_table_id };
>> Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is
>> it an erratum somewhere I did miss?
> Yes, the revision in 6.2 spec is 1. And it's 2 in spec 6.3.
> So just to be sure, should I use the oldest revision ?
If you need (and use) features (such as flags) introduced in 6.3 then
you should say the code complies with 6.3 and update the above comment.
>> I would also add the spec version in the commit msg.
>>> +
>>> +    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;
>>> +
>>> +        build_processor_hierarchy_node(
>>> +            table_data,
>>> +            /*
>>> +             * ACPI 6.2 - Physical package
>>> +             * represents the boundary of a physical package
>>> +             */
>>> +            (1 << 0),
>>> +            0, socket, NULL, 0);
>> I see we set an ACPI process ID but in the meantime the ACPI processor
>> ID valid flag is not set. I am not sure I fully catch the meaning of
>> this latter but just to double check if this is done on purpose.
> Yes, it's on purpose.
>> Maybe
>> wort a general comment as this also happens below.
> The ID of the container node is invalid and ID of the leaf node is valid.
> The commit message by Andrew has explained why (reason 3). I think
> it may be clear enough to explain there why we don't need a valid ID
> for the container node.
>>> +
>>> +        for (core = 0; core < ms->smp.cores; core++) {
>>> +            uint32_t core_offset = table_data->len - pptt_start;
>>> +            int thread;
>>> +
>>> +            if (ms->smp.threads > 1) {
>>> +                build_processor_hierarchy_node(
>>> +                    table_data,
>>> +                    /*
>>> +                     * ACPI 6.2 - Physical package
>>> +                     * doesn't represent the boundary of a physical
>>> package
>>> +                     */
>>> +                    (0 << 0),
>> would rather say (0 << 0) /* not a physical package */ and same elsewhere
> Ok, thanks.
>>> +                    socket_offset, core, NULL, 0);
>>> +
>>> +                for (thread = 0; thread < ms->smp.threads; thread++) {
>>> +                    build_processor_hierarchy_node(
>>> +                        table_data,
>>> +                        (1 << 1) | /* ACPI 6.2 - ACPI Processor ID
>>> valid */
>>> +                        (1 << 2) | /* ACPI 6.3 - Processor is a
>>> Thread */
>> So the references look globaly confusing to me. Either it complies to
>> 6.2 or to 6.3. Looks ir rather complies with 6.3. To me, this needs to
>> be clarified.
> ACPI 6.2 in the comment means the flag is introduced in the spec since 6.2.
> The same, ACPI 6.3 means the flag is introduced since 6.3. Maybe I should
> just drop all the version-prefix in the comment ?
Yes I think you can drop those comments and just upgrade the global
compliance with 6.3

Thanks

Eric
>> I would also add the reference it complies to in the
>> commit msg.
> Ok, sure.
> 
> Thanks,
> Yanan
> .
>>> +                        (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
>>> +                        core_offset, uid++, NULL, 0);
>>> +                }
>>> +            } else {
>>> +                build_processor_hierarchy_node(
>>> +                    table_data,
>>> +                    (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
>>> +                    (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
>>> +                    socket_offset, uid++, NULL, 0);
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +    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/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>>> index 2c457c8f17..b92706388c 100644
>>> --- a/include/hw/acpi/aml-build.h
>>> +++ b/include/hw/acpi/aml-build.h
>>> @@ -493,6 +493,9 @@ 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_pptt(GArray *table_data, BIOSLinker *linker,
>>> MachineState *ms,
>>> +                const char *oem_id, const char *oem_table_id);
>>> +
>>>   void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData
>>> *f,
>>>                   const char *oem_id, const char *oem_table_id);
>>>  
>> Thanks
>>
>> Eric
>>
>> .
> 

Re: [PATCH v8 5/8] hw/acpi/aml-build: Add PPTT table

[wangyanan (Y)]

Hi Eric,

On 2021/10/20 19:11, Eric Auger wrote:
> Hi Yanan,
> On 10/20/21 11:51 AM, wangyanan (Y) wrote:
>> Hi Eric,
>>
>> On 2021/10/20 16:02, Eric Auger wrote:
>>> Hi,
>>>
>>> On 10/14/21 3:22 PM, Yanan Wang wrote:
>>>> From: Andrew Jones <drjones@redhat.com>
>>>>
>>>> Add the Processor Properties Topology Table (PPTT) used to
>>>> describe CPU topology information to ACPI guests.
>>>>
>>>> Note, a DT-boot Linux guest with a non-flat CPU topology will
>>>> see socket and core IDs being sequential integers starting
>>>> from zero, which is different from ACPI-boot Linux guest,
>>>> 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 mandatorily 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.
>>>>
>>>> So in summary, with QEMU as the vendor for the guests, we simply
>>>> use sequential integers starting from zero for the non-leaf nodes
>>>> but with ID-valid flag unset, so that guest will ignore them and
>>>> use table offsets as unique container IDs. And we use logical CPU
>>>> IDs for the leaf nodes with the ID-valid flag set, which will be
>>>> consistent with MADT.
>>>>
>>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>>    hw/acpi/aml-build.c         | 60 +++++++++++++++++++++++++++++++++++++
>>>>    include/hw/acpi/aml-build.h |  3 ++
>>>>    2 files changed, 63 insertions(+)
>>>>
>>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>>> index b7b9db6888..0d50e88e9d 100644
>>>> --- a/hw/acpi/aml-build.c
>>>> +++ b/hw/acpi/aml-build.c
>>>> @@ -1990,6 +1990,66 @@ void build_processor_hierarchy_node(GArray
>>>> *tbl, uint32_t flags,
>>>>        }
>>>>    }
>>>>    +/* ACPI 6.2: 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)
>>>> +{
>>>> +    int pptt_start = table_data->len;
>>>> +    int uid = 0;
>>>> +    int socket;
>>>> +    AcpiTable table = { .sig = "PPTT", .rev = 2,
>>>> +                        .oem_id = oem_id, .oem_table_id =
>>>> oem_table_id };
>>> Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is
>>> it an erratum somewhere I did miss?
>> Yes, the revision in 6.2 spec is 1. And it's 2 in spec 6.3.
>> So just to be sure, should I use the oldest revision ?
> If you need (and use) features (such as flags) introduced in 6.3 then
> you should say the code complies with 6.3 and update the above comment.
The comment /* ACPI 6.2: 5.2.29 Processor Properties Topology Table 
(PPTT) */
tried to explain the first spec version where PPTT is introduced is 6.2. 
But it may
adds some confusion. I think it's better to replace it with:
/*
  * ACPI spec 5.2.29 Processor Properties Topology Table (PPTT)
  * (Revision 6.2 or later)
  */

just like our build_slit().
>>> I would also add the spec version in the commit msg.
>>>> +
>>>> +    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;
>>>> +
>>>> +        build_processor_hierarchy_node(
>>>> +            table_data,
>>>> +            /*
>>>> +             * ACPI 6.2 - Physical package
>>>> +             * represents the boundary of a physical package
>>>> +             */
>>>> +            (1 << 0),
>>>> +            0, socket, NULL, 0);
>>> I see we set an ACPI process ID but in the meantime the ACPI processor
>>> ID valid flag is not set. I am not sure I fully catch the meaning of
>>> this latter but just to double check if this is done on purpose.
>> Yes, it's on purpose.
>>> Maybe
>>> wort a general comment as this also happens below.
>> The ID of the container node is invalid and ID of the leaf node is valid.
>> The commit message by Andrew has explained why (reason 3). I think
>> it may be clear enough to explain there why we don't need a valid ID
>> for the container node.
>>>> +
>>>> +        for (core = 0; core < ms->smp.cores; core++) {
>>>> +            uint32_t core_offset = table_data->len - pptt_start;
>>>> +            int thread;
>>>> +
>>>> +            if (ms->smp.threads > 1) {
>>>> +                build_processor_hierarchy_node(
>>>> +                    table_data,
>>>> +                    /*
>>>> +                     * ACPI 6.2 - Physical package
>>>> +                     * doesn't represent the boundary of a physical
>>>> package
>>>> +                     */
>>>> +                    (0 << 0),
>>> would rather say (0 << 0) /* not a physical package */ and same elsewhere
>> Ok, thanks.
>>>> +                    socket_offset, core, NULL, 0);
>>>> +
>>>> +                for (thread = 0; thread < ms->smp.threads; thread++) {
>>>> +                    build_processor_hierarchy_node(
>>>> +                        table_data,
>>>> +                        (1 << 1) | /* ACPI 6.2 - ACPI Processor ID
>>>> valid */
>>>> +                        (1 << 2) | /* ACPI 6.3 - Processor is a
>>>> Thread */
>>> So the references look globaly confusing to me. Either it complies to
>>> 6.2 or to 6.3. Looks ir rather complies with 6.3. To me, this needs to
>>> be clarified.
>> ACPI 6.2 in the comment means the flag is introduced in the spec since 6.2.
>> The same, ACPI 6.3 means the flag is introduced since 6.3. Maybe I should
>> just drop all the version-prefix in the comment ?
> Yes I think you can drop those comments and just upgrade the global
> compliance with 6.3
>
I will drop the prefix and keep the rest. And add a generic comment on
top of build_pptt() as I replied above.

Thanks,
Yanan
>>> I would also add the reference it complies to in the
>>> commit msg.
>> Ok, sure.
>>
>> Thanks,
>> Yanan
>> .
>>>> +                        (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
>>>> +                        core_offset, uid++, NULL, 0);
>>>> +                }
>>>> +            } else {
>>>> +                build_processor_hierarchy_node(
>>>> +                    table_data,
>>>> +                    (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
>>>> +                    (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
>>>> +                    socket_offset, uid++, NULL, 0);
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +
>>>> +    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/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>>>> index 2c457c8f17..b92706388c 100644
>>>> --- a/include/hw/acpi/aml-build.h
>>>> +++ b/include/hw/acpi/aml-build.h
>>>> @@ -493,6 +493,9 @@ 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_pptt(GArray *table_data, BIOSLinker *linker,
>>>> MachineState *ms,
>>>> +                const char *oem_id, const char *oem_table_id);
>>>> +
>>>>    void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData
>>>> *f,
>>>>                    const char *oem_id, const char *oem_table_id);
>>>>   
>>> Thanks
>>>
>>> Eric
>>>
>>> .
> .

Re: [PATCH v8 4/8] hw/acpi/aml-build: Add Processor hierarchy node structure

[wangyanan (Y)]

On 2021/10/20 18:04, Michael S. Tsirkin wrote:
> On Wed, Oct 20, 2021 at 05:51:46PM +0800, wangyanan (Y) wrote:
>> On 2021/10/20 15:43, Eric Auger wrote:
>>> Hi
>>>
>>> On 10/14/21 3:22 PM, Yanan Wang wrote:
>>>> Add a generic API to build Processor hierarchy node structure (Type 0),
>>>> which is strictly consistent with descriptions in ACPI 6.2: 5.2.29.1.
>>>>
>>>> This function will be used to build ACPI PPTT table for cpu topology.
>>>>
>>>> Co-developed-by: Ying Fang <fangying1@huawei.com>
>>>> Co-developed-by: Henglong Fan <fanhenglong@huawei.com>
>>>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>>    hw/acpi/aml-build.c         | 26 ++++++++++++++++++++++++++
>>>>    include/hw/acpi/aml-build.h |  4 ++++
>>>>    2 files changed, 30 insertions(+)
>>>>
>>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>>> index 76af0ebaf9..b7b9db6888 100644
>>>> --- a/hw/acpi/aml-build.c
>>>> +++ b/hw/acpi/aml-build.c
>>>> @@ -1964,6 +1964,32 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>>>>        acpi_table_end(linker, &table);
>>>>    }
>>>> +/* ACPI 6.2: 5.2.29.1 Processor hierarchy node structure (Type 0) */
>>>> +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;
>>>> +
>>>> +    build_append_byte(tbl, 0);                 /* Type 0 - processor */
>>> nit: I think Igor would generally prefer
>>> build_append_byte(tbl, 0 /* processor structure */);  /* Type */
>>> although it generates checkpatch warnings
>> Ok.
> I think it's ok as is. Right Igor?
Understood.

Thanks,
Yanan
.
>>>> +    build_append_byte(tbl, 20 + priv_num * 4); /* Length */
>>>> +    build_append_int_noprefix(tbl, 0, 2);      /* Reserved */
>>>> +    build_append_int_noprefix(tbl, flags, 4);  /* Flags */
>>>> +    build_append_int_noprefix(tbl, parent, 4); /* Parent */
>>>> +    build_append_int_noprefix(tbl, id, 4);     /* ACPI Processor ID */
>>>> +
>>>> +    /* Number of private resources */
>>>> +    build_append_int_noprefix(tbl, priv_num, 4);
>>>> +
>>>> +    /* Private resources[N] */
>>>> +    if (priv_num > 0) {
>>>> +        assert(priv_rsrc);
>>>> +        for (i = 0; i < priv_num; i++) {
>>>> +            build_append_int_noprefix(tbl, priv_rsrc[i], 4);
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>>    /* 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/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>>>> index 3cf6f2c1b9..2c457c8f17 100644
>>>> --- a/include/hw/acpi/aml-build.h
>>>> +++ b/include/hw/acpi/aml-build.h
>>>> @@ -489,6 +489,10 @@ 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_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);
>>>>
>>> Besides
>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> Thanks for your review!
>>
>> Yanan
>> .
>>> Eric
>>>
>>> .
> .

Re: [PATCH v8 5/8] hw/acpi/aml-build: Add PPTT table

[Eric Auger]

Hi,
On 10/20/21 2:27 PM, wangyanan (Y) wrote:
> Hi Eric,
> 
> On 2021/10/20 19:11, Eric Auger wrote:
>> Hi Yanan,
>> On 10/20/21 11:51 AM, wangyanan (Y) wrote:
>>> Hi Eric,
>>>
>>> On 2021/10/20 16:02, Eric Auger wrote:
>>>> Hi,
>>>>
>>>> On 10/14/21 3:22 PM, Yanan Wang wrote:
>>>>> From: Andrew Jones <drjones@redhat.com>
>>>>>
>>>>> Add the Processor Properties Topology Table (PPTT) used to
>>>>> describe CPU topology information to ACPI guests.
>>>>>
>>>>> Note, a DT-boot Linux guest with a non-flat CPU topology will
>>>>> see socket and core IDs being sequential integers starting
>>>>> from zero, which is different from ACPI-boot Linux guest,
>>>>> 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 mandatorily 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.
>>>>>
>>>>> So in summary, with QEMU as the vendor for the guests, we simply
>>>>> use sequential integers starting from zero for the non-leaf nodes
>>>>> but with ID-valid flag unset, so that guest will ignore them and
>>>>> use table offsets as unique container IDs. And we use logical CPU
>>>>> IDs for the leaf nodes with the ID-valid flag set, which will be
>>>>> consistent with MADT.
>>>>>
>>>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>>>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> ---
>>>>>    hw/acpi/aml-build.c         | 60
>>>>> +++++++++++++++++++++++++++++++++++++
>>>>>    include/hw/acpi/aml-build.h |  3 ++
>>>>>    2 files changed, 63 insertions(+)
>>>>>
>>>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>>>> index b7b9db6888..0d50e88e9d 100644
>>>>> --- a/hw/acpi/aml-build.c
>>>>> +++ b/hw/acpi/aml-build.c
>>>>> @@ -1990,6 +1990,66 @@ void build_processor_hierarchy_node(GArray
>>>>> *tbl, uint32_t flags,
>>>>>        }
>>>>>    }
>>>>>    +/* ACPI 6.2: 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)
>>>>> +{
>>>>> +    int pptt_start = table_data->len;
>>>>> +    int uid = 0;
>>>>> +    int socket;
>>>>> +    AcpiTable table = { .sig = "PPTT", .rev = 2,
>>>>> +                        .oem_id = oem_id, .oem_table_id =
>>>>> oem_table_id };
>>>> Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is
>>>> it an erratum somewhere I did miss?
>>> Yes, the revision in 6.2 spec is 1. And it's 2 in spec 6.3.
>>> So just to be sure, should I use the oldest revision ?
>> If you need (and use) features (such as flags) introduced in 6.3 then
>> you should say the code complies with 6.3 and update the above comment.
> The comment /* ACPI 6.2: 5.2.29 Processor Properties Topology Table
> (PPTT) */
> tried to explain the first spec version where PPTT is introduced is 6.2.
> But it may
> adds some confusion. I think it's better to replace it with:
> /*
>  * ACPI spec 5.2.29 Processor Properties Topology Table (PPTT)
>  * (Revision 6.2 or later)
>  */
> 
> just like our build_slit().
That's not my understanding. I think we usually point to the latest spec
revision you comply with, ie. in that case 6.3 chapter ...

Thanks

Eric
>>>> I would also add the spec version in the commit msg.
>>>>> +
>>>>> +    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;
>>>>> +
>>>>> +        build_processor_hierarchy_node(
>>>>> +            table_data,
>>>>> +            /*
>>>>> +             * ACPI 6.2 - Physical package
>>>>> +             * represents the boundary of a physical package
>>>>> +             */
>>>>> +            (1 << 0),
>>>>> +            0, socket, NULL, 0);
>>>> I see we set an ACPI process ID but in the meantime the ACPI processor
>>>> ID valid flag is not set. I am not sure I fully catch the meaning of
>>>> this latter but just to double check if this is done on purpose.
>>> Yes, it's on purpose.
>>>> Maybe
>>>> wort a general comment as this also happens below.
>>> The ID of the container node is invalid and ID of the leaf node is
>>> valid.
>>> The commit message by Andrew has explained why (reason 3). I think
>>> it may be clear enough to explain there why we don't need a valid ID
>>> for the container node.
>>>>> +
>>>>> +        for (core = 0; core < ms->smp.cores; core++) {
>>>>> +            uint32_t core_offset = table_data->len - pptt_start;
>>>>> +            int thread;
>>>>> +
>>>>> +            if (ms->smp.threads > 1) {
>>>>> +                build_processor_hierarchy_node(
>>>>> +                    table_data,
>>>>> +                    /*
>>>>> +                     * ACPI 6.2 - Physical package
>>>>> +                     * doesn't represent the boundary of a physical
>>>>> package
>>>>> +                     */
>>>>> +                    (0 << 0),
>>>> would rather say (0 << 0) /* not a physical package */ and same
>>>> elsewhere
>>> Ok, thanks.
>>>>> +                    socket_offset, core, NULL, 0);
>>>>> +
>>>>> +                for (thread = 0; thread < ms->smp.threads;
>>>>> thread++) {
>>>>> +                    build_processor_hierarchy_node(
>>>>> +                        table_data,
>>>>> +                        (1 << 1) | /* ACPI 6.2 - ACPI Processor ID
>>>>> valid */
>>>>> +                        (1 << 2) | /* ACPI 6.3 - Processor is a
>>>>> Thread */
>>>> So the references look globaly confusing to me. Either it complies to
>>>> 6.2 or to 6.3. Looks ir rather complies with 6.3. To me, this needs to
>>>> be clarified.
>>> ACPI 6.2 in the comment means the flag is introduced in the spec
>>> since 6.2.
>>> The same, ACPI 6.3 means the flag is introduced since 6.3. Maybe I
>>> should
>>> just drop all the version-prefix in the comment ?
>> Yes I think you can drop those comments and just upgrade the global
>> compliance with 6.3
>>
> I will drop the prefix and keep the rest. And add a generic comment on
> top of build_pptt() as I replied above.
> 
> Thanks,
> Yanan
>>>> I would also add the reference it complies to in the
>>>> commit msg.
>>> Ok, sure.
>>>
>>> Thanks,
>>> Yanan
>>> .
>>>>> +                        (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
>>>>> +                        core_offset, uid++, NULL, 0);
>>>>> +                }
>>>>> +            } else {
>>>>> +                build_processor_hierarchy_node(
>>>>> +                    table_data,
>>>>> +                    (1 << 1) | /* ACPI 6.2 - ACPI Processor ID
>>>>> valid */
>>>>> +                    (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
>>>>> +                    socket_offset, uid++, NULL, 0);
>>>>> +            }
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    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/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>>>>> index 2c457c8f17..b92706388c 100644
>>>>> --- a/include/hw/acpi/aml-build.h
>>>>> +++ b/include/hw/acpi/aml-build.h
>>>>> @@ -493,6 +493,9 @@ 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_pptt(GArray *table_data, BIOSLinker *linker,
>>>>> MachineState *ms,
>>>>> +                const char *oem_id, const char *oem_table_id);
>>>>> +
>>>>>    void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData
>>>>> *f,
>>>>>                    const char *oem_id, const char *oem_table_id);
>>>>>   
>>>> Thanks
>>>>
>>>> Eric
>>>>
>>>> .
>> .
> 

Re: [PATCH v8 5/8] hw/acpi/aml-build: Add PPTT table

[wangyanan (Y)]

On 2021/10/20 20:43, Eric Auger wrote:
> Hi,
> On 10/20/21 2:27 PM, wangyanan (Y) wrote:
>> Hi Eric,
>>
>> On 2021/10/20 19:11, Eric Auger wrote:
>>> Hi Yanan,
>>> On 10/20/21 11:51 AM, wangyanan (Y) wrote:
>>>> Hi Eric,
>>>>
>>>> On 2021/10/20 16:02, Eric Auger wrote:
>>>>> Hi,
>>>>>
>>>>> On 10/14/21 3:22 PM, Yanan Wang wrote:
>>>>>> From: Andrew Jones <drjones@redhat.com>
>>>>>>
>>>>>> Add the Processor Properties Topology Table (PPTT) used to
>>>>>> describe CPU topology information to ACPI guests.
>>>>>>
>>>>>> Note, a DT-boot Linux guest with a non-flat CPU topology will
>>>>>> see socket and core IDs being sequential integers starting
>>>>>> from zero, which is different from ACPI-boot Linux guest,
>>>>>> 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 mandatorily 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.
>>>>>>
>>>>>> So in summary, with QEMU as the vendor for the guests, we simply
>>>>>> use sequential integers starting from zero for the non-leaf nodes
>>>>>> but with ID-valid flag unset, so that guest will ignore them and
>>>>>> use table offsets as unique container IDs. And we use logical CPU
>>>>>> IDs for the leaf nodes with the ID-valid flag set, which will be
>>>>>> consistent with MADT.
>>>>>>
>>>>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>>>>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>>>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>> ---
>>>>>>     hw/acpi/aml-build.c         | 60
>>>>>> +++++++++++++++++++++++++++++++++++++
>>>>>>     include/hw/acpi/aml-build.h |  3 ++
>>>>>>     2 files changed, 63 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>>>>> index b7b9db6888..0d50e88e9d 100644
>>>>>> --- a/hw/acpi/aml-build.c
>>>>>> +++ b/hw/acpi/aml-build.c
>>>>>> @@ -1990,6 +1990,66 @@ void build_processor_hierarchy_node(GArray
>>>>>> *tbl, uint32_t flags,
>>>>>>         }
>>>>>>     }
>>>>>>     +/* ACPI 6.2: 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)
>>>>>> +{
>>>>>> +    int pptt_start = table_data->len;
>>>>>> +    int uid = 0;
>>>>>> +    int socket;
>>>>>> +    AcpiTable table = { .sig = "PPTT", .rev = 2,
>>>>>> +                        .oem_id = oem_id, .oem_table_id =
>>>>>> oem_table_id };
>>>>> Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is
>>>>> it an erratum somewhere I did miss?
>>>> Yes, the revision in 6.2 spec is 1. And it's 2 in spec 6.3.
>>>> So just to be sure, should I use the oldest revision ?
>>> If you need (and use) features (such as flags) introduced in 6.3 then
>>> you should say the code complies with 6.3 and update the above comment.
>> The comment /* ACPI 6.2: 5.2.29 Processor Properties Topology Table
>> (PPTT) */
>> tried to explain the first spec version where PPTT is introduced is 6.2.
>> But it may
>> adds some confusion. I think it's better to replace it with:
>> /*
>>   * ACPI spec 5.2.29 Processor Properties Topology Table (PPTT)
>>   * (Revision 6.2 or later)
>>   */
>>
>> just like our build_slit().
> That's not my understanding. I think we usually point to the latest spec
> revision you comply with, ie. in that case 6.3 chapter ...
Ok, I get it. Would
/*
  * ACPI spec, Revision 6.3
  * 5.2.29 Processor Properties Topology Table (PPTT)
  */
be ok ?

Thanks,
Yanan
> Thanks
>
> Eric
>>>>> I would also add the spec version in the commit msg.
>>>>>> +
>>>>>> +    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;
>>>>>> +
>>>>>> +        build_processor_hierarchy_node(
>>>>>> +            table_data,
>>>>>> +            /*
>>>>>> +             * ACPI 6.2 - Physical package
>>>>>> +             * represents the boundary of a physical package
>>>>>> +             */
>>>>>> +            (1 << 0),
>>>>>> +            0, socket, NULL, 0);
>>>>> I see we set an ACPI process ID but in the meantime the ACPI processor
>>>>> ID valid flag is not set. I am not sure I fully catch the meaning of
>>>>> this latter but just to double check if this is done on purpose.
>>>> Yes, it's on purpose.
>>>>> Maybe
>>>>> wort a general comment as this also happens below.
>>>> The ID of the container node is invalid and ID of the leaf node is
>>>> valid.
>>>> The commit message by Andrew has explained why (reason 3). I think
>>>> it may be clear enough to explain there why we don't need a valid ID
>>>> for the container node.
>>>>>> +
>>>>>> +        for (core = 0; core < ms->smp.cores; core++) {
>>>>>> +            uint32_t core_offset = table_data->len - pptt_start;
>>>>>> +            int thread;
>>>>>> +
>>>>>> +            if (ms->smp.threads > 1) {
>>>>>> +                build_processor_hierarchy_node(
>>>>>> +                    table_data,
>>>>>> +                    /*
>>>>>> +                     * ACPI 6.2 - Physical package
>>>>>> +                     * doesn't represent the boundary of a physical
>>>>>> package
>>>>>> +                     */
>>>>>> +                    (0 << 0),
>>>>> would rather say (0 << 0) /* not a physical package */ and same
>>>>> elsewhere
>>>> Ok, thanks.
>>>>>> +                    socket_offset, core, NULL, 0);
>>>>>> +
>>>>>> +                for (thread = 0; thread < ms->smp.threads;
>>>>>> thread++) {
>>>>>> +                    build_processor_hierarchy_node(
>>>>>> +                        table_data,
>>>>>> +                        (1 << 1) | /* ACPI 6.2 - ACPI Processor ID
>>>>>> valid */
>>>>>> +                        (1 << 2) | /* ACPI 6.3 - Processor is a
>>>>>> Thread */
>>>>> So the references look globaly confusing to me. Either it complies to
>>>>> 6.2 or to 6.3. Looks ir rather complies with 6.3. To me, this needs to
>>>>> be clarified.
>>>> ACPI 6.2 in the comment means the flag is introduced in the spec
>>>> since 6.2.
>>>> The same, ACPI 6.3 means the flag is introduced since 6.3. Maybe I
>>>> should
>>>> just drop all the version-prefix in the comment ?
>>> Yes I think you can drop those comments and just upgrade the global
>>> compliance with 6.3
>>>
>> I will drop the prefix and keep the rest. And add a generic comment on
>> top of build_pptt() as I replied above.
>>
>> Thanks,
>> Yanan
>>>>> I would also add the reference it complies to in the
>>>>> commit msg.
>>>> Ok, sure.
>>>>
>>>> Thanks,
>>>> Yanan
>>>> .
>>>>>> +                        (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
>>>>>> +                        core_offset, uid++, NULL, 0);
>>>>>> +                }
>>>>>> +            } else {
>>>>>> +                build_processor_hierarchy_node(
>>>>>> +                    table_data,
>>>>>> +                    (1 << 1) | /* ACPI 6.2 - ACPI Processor ID
>>>>>> valid */
>>>>>> +                    (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
>>>>>> +                    socket_offset, uid++, NULL, 0);
>>>>>> +            }
>>>>>> +        }
>>>>>> +    }
>>>>>> +
>>>>>> +    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/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>>>>>> index 2c457c8f17..b92706388c 100644
>>>>>> --- a/include/hw/acpi/aml-build.h
>>>>>> +++ b/include/hw/acpi/aml-build.h
>>>>>> @@ -493,6 +493,9 @@ 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_pptt(GArray *table_data, BIOSLinker *linker,
>>>>>> MachineState *ms,
>>>>>> +                const char *oem_id, const char *oem_table_id);
>>>>>> +
>>>>>>     void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData
>>>>>> *f,
>>>>>>                     const char *oem_id, const char *oem_table_id);
>>>>>>    
>>>>> Thanks
>>>>>
>>>>> Eric
>>>>>
>>>>> .
>>> .
> .

Re: [PATCH v8 5/8] hw/acpi/aml-build: Add PPTT table

[Andrew Jones]

On Wed, Oct 20, 2021 at 08:53:00PM +0800, wangyanan (Y) wrote:
> > > > > > Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is
> > > > > > it an erratum somewhere I did miss?
> > > > > Yes, the revision in 6.2 spec is 1. And it's 2 in spec 6.3.
> > > > > So just to be sure, should I use the oldest revision ?
> > > > If you need (and use) features (such as flags) introduced in 6.3 then
> > > > you should say the code complies with 6.3 and update the above comment.
> > > The comment /* ACPI 6.2: 5.2.29 Processor Properties Topology Table
> > > (PPTT) */
> > > tried to explain the first spec version where PPTT is introduced is 6.2.
> > > But it may
> > > adds some confusion. I think it's better to replace it with:
> > > /*
> > >   * ACPI spec 5.2.29 Processor Properties Topology Table (PPTT)
> > >   * (Revision 6.2 or later)
> > >   */
> > > 
> > > just like our build_slit().
> > That's not my understanding. I think we usually point to the latest spec
> > revision you comply with, ie. in that case 6.3 chapter ...
> Ok, I get it. Would
> /*
>  * ACPI spec, Revision 6.3
>  * 5.2.29 Processor Properties Topology Table (PPTT)
>  */
> be ok ?
>

Yes, we need to call out the oldest spec we can for the functionality
we're using. In this case, we're using flags first defined in the 6.3
spec, so we need to call out the 6.3 spec.

Thanks,
drew

Re: [PATCH v8 5/8] hw/acpi/aml-build: Add PPTT table

[wangyanan (Y)]

On 2021/10/21 17:08, Andrew Jones wrote:
> On Wed, Oct 20, 2021 at 08:53:00PM +0800, wangyanan (Y) wrote:
>>>>>>> Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is
>>>>>>> it an erratum somewhere I did miss?
>>>>>> Yes, the revision in 6.2 spec is 1. And it's 2 in spec 6.3.
>>>>>> So just to be sure, should I use the oldest revision ?
>>>>> If you need (and use) features (such as flags) introduced in 6.3 then
>>>>> you should say the code complies with 6.3 and update the above comment.
>>>> The comment /* ACPI 6.2: 5.2.29 Processor Properties Topology Table
>>>> (PPTT) */
>>>> tried to explain the first spec version where PPTT is introduced is 6.2.
>>>> But it may
>>>> adds some confusion. I think it's better to replace it with:
>>>> /*
>>>>    * ACPI spec 5.2.29 Processor Properties Topology Table (PPTT)
>>>>    * (Revision 6.2 or later)
>>>>    */
>>>>
>>>> just like our build_slit().
>>> That's not my understanding. I think we usually point to the latest spec
>>> revision you comply with, ie. in that case 6.3 chapter ...
>> Ok, I get it. Would
>> /*
>>   * ACPI spec, Revision 6.3
>>   * 5.2.29 Processor Properties Topology Table (PPTT)
>>   */
>> be ok ?
>>
> Yes, we need to call out the oldest spec we can for the functionality
> we're using. In this case, we're using flags first defined in the 6.3
> spec, so we need to call out the 6.3 spec.
>
Agreed.

I have updated these comments as suggested in v9. And I just noticed
that Richard has taken that v9 into the target-arm tree.

Thanks,
Yanan