[RFC PATCH 0/5] hw/arm/virt: Introduce cpu topology support

[RFC PATCH 0/5] hw/arm/virt: Introduce cpu topology support

[Ying Fang]

An accurate cpu topology may help improve the cpu scheduler's decision
making when dealing with multi-core system. So cpu topology description
is helpful to provide guest with the right view. Dario Faggioli's talk
in [0] also shows the virtual topology may has impact on sched performace.
Thus this patch series is posted to introduce cpu topology support for
arm platform.

Both fdt and ACPI are introduced to present the cpu topology. To describe
the cpu topology via ACPI, a PPTT table is introduced according to the
processor hierarchy node structure. This series is derived from [1], in
[1] we are trying to bring both cpu and cache topology support for arm
platform, but there is still some issues to solve to support the cache
hierarchy. So we split the cpu topology part out and send it seperately.
The patch series to support cache hierarchy will be send later since
Salil Mehta's cpu hotplug feature need the cpu topology enabled first and
he is waiting for it to be upstreamed.

This patch series was initially based on the patches posted by Andrew Jones [2].
I jumped in on it since some OS vendor cooperative partner are eager for it.
Thanks for Andrew's contribution.

After applying this patch series, launch a guest with virt-6.0 and cpu
topology configured with sockets:cores:threads = 2:4:2, you will get the
bellow messages with the lscpu command.

-----------------------------------------
Architecture:                    aarch64
CPU op-mode(s):                  64-bit
Byte Order:                      Little Endian
CPU(s):                          16
On-line CPU(s) list:             0-15
Thread(s) per core:              2
Core(s) per socket:              4
Socket(s):                       2
NUMA node(s):                    2
Vendor ID:                       HiSilicon
Model:                           0
Model name:                      Kunpeng-920
Stepping:                        0x1
BogoMIPS:                        200.00
NUMA node0 CPU(s):               0-7
NUMA node1 CPU(s):               8-15

[0] https://kvmforum2020.sched.com/event/eE1y/virtual-topology-for-virtual-machines-friend-or-foe-dario-faggioli-suse
[1] https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg02166.html
[2] https://patchwork.ozlabs.org/project/qemu-devel/cover/20180704124923.32483-1-drjones@redhat.com

Ying Fang (5):
  device_tree: Add qemu_fdt_add_path
  hw/arm/virt: Add cpu-map to device tree
  hw/arm/virt-acpi-build: distinguish possible and present cpus
  hw/acpi/aml-build: add processor hierarchy node structure
  hw/arm/virt-acpi-build: add PPTT table

 hw/acpi/aml-build.c          | 40 ++++++++++++++++++++++
 hw/arm/virt-acpi-build.c     | 64 +++++++++++++++++++++++++++++++++---
 hw/arm/virt.c                | 40 +++++++++++++++++++++-
 include/hw/acpi/acpi-defs.h  | 13 ++++++++
 include/hw/acpi/aml-build.h  |  7 ++++
 include/hw/arm/virt.h        |  1 +
 include/sysemu/device_tree.h |  1 +
 softmmu/device_tree.c        | 45 +++++++++++++++++++++++--
 8 files changed, 204 insertions(+), 7 deletions(-)

-- 
2.23.0

[RFC PATCH 1/5] device_tree: Add qemu_fdt_add_path

[Ying Fang]

qemu_fdt_add_path() works like qemu_fdt_add_subnode(), except
it also adds any missing parent nodes. We also tweak an error
message of qemu_fdt_add_subnode().

Signed-off-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 include/sysemu/device_tree.h |  1 +
 softmmu/device_tree.c        | 45 ++++++++++++++++++++++++++++++++++--
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 982c89345f..15fb98af98 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -104,6 +104,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 b9a3ddc518..1e3857ca0c 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -515,8 +515,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);
     }
 
@@ -524,6 +524,47 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
     return retval;
 }
 
+/*
+ * Like qemu_fdt_add_subnode(), but will add all missing
+ * subnodes in the path.
+ */
+int qemu_fdt_add_path(void *fdt, const char *path)
+{
+    char *dupname, *basename, *p;
+    int parent, retval = -1;
+
+    if (path[0] != '/') {
+        return retval;
+    }
+
+    parent = fdt_path_offset(fdt, "/");
+    p = dupname = g_strdup(path);
+
+    while (p) {
+        *p = '/';
+        basename = p + 1;
+        p = strchr(p + 1, '/');
+        if (p) {
+            *p = '\0';
+        }
+        retval = fdt_path_offset(fdt, dupname);
+        if (retval < 0 && retval != -FDT_ERR_NOTFOUND) {
+            error_report("%s: Invalid path %s: %s",
+                         __func__, path, fdt_strerror(retval));
+            exit(1);
+        } else if (retval == -FDT_ERR_NOTFOUND) {
+            retval = fdt_add_subnode(fdt, parent, basename);
+            if (retval < 0) {
+                break;
+            }
+        }
+        parent = retval;
+    }
+
+    g_free(dupname);
+    return retval;
+}
+
 void qemu_fdt_dumpdtb(void *fdt, int size)
 {
     const char *dumpdtb = current_machine->dumpdtb;
-- 
2.23.0

[RFC PATCH 2/5] hw/arm/virt: Add cpu-map to device tree

[Ying Fang]

Support device tree CPU topology descriptions.

Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 hw/arm/virt.c         | 38 +++++++++++++++++++++++++++++++++++++-
 include/hw/arm/virt.h |  1 +
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 371147f3ae..c133b342b8 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -351,10 +351,11 @@ 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
+     * 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
@@ -407,8 +408,42 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
                 ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
         }
 
+        if (ms->smp.cpus > 1 && !vmc->no_cpu_topology) {
+            qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle",
+                                  qemu_fdt_alloc_phandle(vms->fdt));
+        }
+
         g_free(nodename);
     }
+
+    if (ms->smp.cpus > 1 && !vmc->no_cpu_topology) {
+        /*
+         * See Linux Documentation/devicetree/bindings/cpu/cpu-topology.txt
+         */
+        qemu_fdt_add_subnode(vms->fdt, "/cpus/cpu-map");
+
+        for (cpu = ms->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/%s%d/%s%d/%s%d",
+                            "cluster", cpu / (ms->smp.cores * ms->smp.threads),
+                            "core", (cpu / ms->smp.threads) % ms->smp.cores,
+                            "thread", cpu % ms->smp.threads);
+            } else {
+                map_path = g_strdup_printf(
+                            "/cpus/cpu-map/%s%d/%s%d",
+                            "cluster", cpu / ms->smp.cores,
+                            "core", cpu % ms->smp.cores);
+            }
+            qemu_fdt_add_path(vms->fdt, map_path);
+            qemu_fdt_setprop_phandle(vms->fdt, map_path, "cpu", cpu_path);
+            g_free(map_path);
+            g_free(cpu_path);
+        }
+    }
 }
 
 static void fdt_add_its_gic_node(VirtMachineState *vms)
@@ -2742,6 +2777,7 @@ static void virt_machine_5_2_options(MachineClass *mc)
     virt_machine_6_0_options(mc);
     compat_props_add(mc->compat_props, hw_compat_5_2, hw_compat_5_2_len);
     vmc->no_secure_gpio = true;
+    vmc->no_cpu_topology = true;
 }
 DEFINE_VIRT_MACHINE(5, 2)
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index ee9a93101e..7ef6d08ac3 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -129,6 +129,7 @@ struct VirtMachineClass {
     bool no_kvm_steal_time;
     bool acpi_expose_flash;
     bool no_secure_gpio;
+    bool no_cpu_topology;
 };
 
 struct VirtMachineState {
-- 
2.23.0

[RFC PATCH 3/5] hw/arm/virt-acpi-build: distinguish possible and present cpus

[Ying Fang]

When building ACPI tables regarding CPUs we should always build
them for the number of possible CPUs, not the number of present
CPUs. We then ensure only the present CPUs are enabled in madt.
Furthermore, it is also needed if we are going to support CPU
hotplug in the future.

This patch is a rework based on Andrew Jones's contribution at
https://lists.gnu.org/archive/html/qemu-arm/2018-07/msg00076.html

Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 hw/arm/virt-acpi-build.c | 14 ++++++++++----
 hw/arm/virt.c            |  2 ++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index f9c9df916c..bb91152fe2 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -61,13 +61,16 @@
 
 static void acpi_dsdt_add_cpus(Aml *scope, VirtMachineState *vms)
 {
-    MachineState *ms = MACHINE(vms);
+    CPUArchIdList *possible_cpus = MACHINE(vms)->possible_cpus;
     uint16_t i;
 
-    for (i = 0; i < ms->smp.cpus; i++) {
+    for (i = 0; i < possible_cpus->len; i++) {
         Aml *dev = aml_device("C%.03X", i);
         aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
         aml_append(dev, aml_name_decl("_UID", aml_int(i)));
+        if (possible_cpus->cpus[i].cpu == NULL) {
+            aml_append(dev, aml_name_decl("_STA", aml_int(0)));
+        }
         aml_append(scope, dev);
     }
 }
@@ -479,6 +482,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     const int *irqmap = vms->irqmap;
     AcpiMadtGenericDistributor *gicd;
     AcpiMadtGenericMsiFrame *gic_msi;
+    CPUArchIdList *possible_cpus = MACHINE(vms)->possible_cpus;
     int i;
 
     acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
@@ -489,7 +493,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     gicd->base_address = cpu_to_le64(memmap[VIRT_GIC_DIST].base);
     gicd->version = vms->gic_version;
 
-    for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
+    for (i = 0; i < possible_cpus->len; i++) {
         AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
                                                            sizeof(*gicc));
         ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
@@ -504,7 +508,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         gicc->cpu_interface_number = cpu_to_le32(i);
         gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
         gicc->uid = cpu_to_le32(i);
-        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
+        if (possible_cpus->cpus[i].cpu != NULL) {
+            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
+        }
 
         if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
             gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index c133b342b8..75659502e2 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2047,6 +2047,8 @@ static void machvirt_init(MachineState *machine)
 
         qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
         object_unref(cpuobj);
+        /* Initialize cpu member here since cpu hotplug is not supported yet */
+        machine->possible_cpus->cpus[n].cpu = cpuobj;
     }
     fdt_add_timer_nodes(vms);
     fdt_add_cpu_nodes(vms);
-- 
2.23.0

[RFC PATCH 4/5] hw/acpi/aml-build: add processor hierarchy node structure

[Ying Fang]

Add the processor hierarchy node structures to build ACPI information
for CPU topology. Since the private resources may be used to describe
cache hierarchy and it is variable among different topology level,
three helpers are introduced to describe the hierarchy.

(1) build_socket_hierarchy for socket description
(2) build_processor_hierarchy for processor description
(3) build_smt_hierarchy for thread (logic processor) description

Signed-off-by: Ying Fang <fangying1@huawei.com>
Signed-off-by: Henglong Fan <fanhenglong@huawei.com>
---
 hw/acpi/aml-build.c         | 40 +++++++++++++++++++++++++++++++++++++
 include/hw/acpi/acpi-defs.h | 13 ++++++++++++
 include/hw/acpi/aml-build.h |  7 +++++++
 3 files changed, 60 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index a2cd7a5830..a0af3e9d73 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1888,6 +1888,46 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
                  table_data->len - slit_start, 1, oem_id, oem_table_id);
 }
 
+/*
+ * ACPI 6.3: 5.2.29.1 Processor hierarchy node structure (Type 0)
+ */
+void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
+{
+    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
+    build_append_byte(tbl, 20);         /* Length, no private resources */
+    build_append_int_noprefix(tbl, 0, 2);  /* Reserved */
+    build_append_int_noprefix(tbl, ACPI_PPTT_PHYSICAL_PACKAGE, 4);
+    build_append_int_noprefix(tbl, parent, 4); /* Parent */
+    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
+    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
+}
+
+void build_processor_hierarchy(GArray *tbl, uint32_t flags,
+                               uint32_t parent, uint32_t id)
+{
+    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR);  /* Type 0 - processor */
+    build_append_byte(tbl, 20);         /* Length, no private resources */
+    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 */
+    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
+}
+
+void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
+{
+    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
+    build_append_byte(tbl, 20);           /* Length, no private resources */
+    build_append_int_noprefix(tbl, 0, 2); /* Reserved */
+    build_append_int_noprefix(tbl,
+                              ACPI_PPTT_ACPI_PROCESSOR_ID_VALID |
+                              ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD |
+                              ACPI_PPTT_ACPI_LEAF_NODE, 4);  /* Flags */
+    build_append_int_noprefix(tbl, parent , 4); /* parent */
+    build_append_int_noprefix(tbl, id, 4);      /* ACPI processor ID */
+    build_append_int_noprefix(tbl, 0, 4);       /* Num of private resources */
+}
+
 /* 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/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index cf9f44299c..45e10d886f 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -618,4 +618,17 @@ struct AcpiIortRC {
 } QEMU_PACKED;
 typedef struct AcpiIortRC AcpiIortRC;
 
+enum {
+    ACPI_PPTT_TYPE_PROCESSOR = 0,
+    ACPI_PPTT_TYPE_CACHE,
+    ACPI_PPTT_TYPE_ID,
+    ACPI_PPTT_TYPE_RESERVED
+};
+
+#define ACPI_PPTT_PHYSICAL_PACKAGE          (1)
+#define ACPI_PPTT_ACPI_PROCESSOR_ID_VALID   (1 << 1)
+#define ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD  (1 << 2)      /* ACPI 6.3 */
+#define ACPI_PPTT_ACPI_LEAF_NODE            (1 << 3)      /* ACPI 6.3 */
+#define ACPI_PPTT_ACPI_IDENTICAL            (1 << 4)      /* ACPI 6.3 */
+
 #endif
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 380d3e3924..7f0ca1a198 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -462,6 +462,13 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
 void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
                 const char *oem_id, const char *oem_table_id);
 
+void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
+
+void build_processor_hierarchy(GArray *tbl, uint32_t flags,
+                               uint32_t parent, uint32_t id);
+
+void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
+
 void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                 const char *oem_id, const char *oem_table_id);
 
-- 
2.23.0

[RFC PATCH 5/5] hw/arm/virt-acpi-build: add PPTT table

[Ying Fang]

Add the Processor Properties Topology Table (PPTT) to present
CPU topology information to the guest. A three-level cpu
topology is built in accord with the linux kernel currently does.

Tested-by: Jiajie Li <lijiajie11@huawei.com>
Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 hw/arm/virt-acpi-build.c | 50 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index bb91152fe2..38d50ce66c 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -436,6 +436,50 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
                  vms->oem_table_id);
 }
 
+static void
+build_pptt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
+{
+    int pptt_start = table_data->len;
+    int uid = 0, cpus = 0, socket = 0;
+    MachineState *ms = MACHINE(vms);
+    unsigned int smp_cores = ms->smp.cores;
+    unsigned int smp_threads = ms->smp.threads;
+
+    acpi_data_push(table_data, sizeof(AcpiTableHeader));
+
+    for (socket = 0; cpus < ms->possible_cpus->len; socket++) {
+        uint32_t socket_offset = table_data->len - pptt_start;
+        int core;
+
+        build_socket_hierarchy(table_data, 0, socket);
+
+        for (core = 0; core < smp_cores; core++) {
+            uint32_t core_offset = table_data->len - pptt_start;
+            int thread;
+
+            if (smp_threads <= 1) {
+                build_processor_hierarchy(table_data,
+                                          ACPI_PPTT_ACPI_PROCESSOR_ID_VALID |
+                                          ACPI_PPTT_ACPI_LEAF_NODE,
+                                          socket_offset, uid++);
+             } else {
+                build_processor_hierarchy(table_data,
+                                          ACPI_PPTT_ACPI_PROCESSOR_ID_VALID,
+                                          socket_offset, core);
+                for (thread = 0; thread < smp_threads; thread++) {
+                    build_thread_hierarchy(table_data, core_offset, uid++);
+                }
+             }
+        }
+        cpus += smp_cores * smp_threads;
+    }
+
+    build_header(linker, table_data,
+                 (void *)(table_data->data + pptt_start), "PPTT",
+                 table_data->len - pptt_start, 2,
+                 vms->oem_id, vms->oem_table_id);
+}
+
 /* GTDT */
 static void
 build_gtdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
@@ -688,6 +732,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     unsigned dsdt, xsdt;
     GArray *tables_blob = tables->table_data;
     MachineState *ms = MACHINE(vms);
+    bool cpu_topology_enabled = !vmc->no_cpu_topology;
 
     table_offsets = g_array_new(false, true /* clear */,
                                         sizeof(uint32_t));
@@ -707,6 +752,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     acpi_add_table(table_offsets, tables_blob);
     build_madt(tables_blob, tables->linker, vms);
 
+    if (ms->smp.cpus > 1 && cpu_topology_enabled) {
+        acpi_add_table(table_offsets, tables_blob);
+        build_pptt(tables_blob, tables->linker, vms);
+    }
+
     acpi_add_table(table_offsets, tables_blob);
     build_gtdt(tables_blob, tables->linker, vms);
 
-- 
2.23.0

Re: [RFC PATCH 1/5] device_tree: Add qemu_fdt_add_path

[Andrew Jones]

Hi Ying Fang,

I don't see any change in this patch from what I have in my
tree, so this should be

 From: Andrew Jones <drjones@redhat.com>

Thanks,
drew

On Thu, Feb 25, 2021 at 04:56:23PM +0800, Ying Fang wrote:
> qemu_fdt_add_path() works like qemu_fdt_add_subnode(), except
> it also adds any missing parent nodes. We also tweak an error
> message of qemu_fdt_add_subnode().
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
>  include/sysemu/device_tree.h |  1 +
>  softmmu/device_tree.c        | 45 ++++++++++++++++++++++++++++++++++--
>  2 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index 982c89345f..15fb98af98 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -104,6 +104,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 b9a3ddc518..1e3857ca0c 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -515,8 +515,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);
>      }
>  
> @@ -524,6 +524,47 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
>      return retval;
>  }
>  
> +/*
> + * Like qemu_fdt_add_subnode(), but will add all missing
> + * subnodes in the path.
> + */
> +int qemu_fdt_add_path(void *fdt, const char *path)
> +{
> +    char *dupname, *basename, *p;
> +    int parent, retval = -1;
> +
> +    if (path[0] != '/') {
> +        return retval;
> +    }
> +
> +    parent = fdt_path_offset(fdt, "/");
> +    p = dupname = g_strdup(path);
> +
> +    while (p) {
> +        *p = '/';
> +        basename = p + 1;
> +        p = strchr(p + 1, '/');
> +        if (p) {
> +            *p = '\0';
> +        }
> +        retval = fdt_path_offset(fdt, dupname);
> +        if (retval < 0 && retval != -FDT_ERR_NOTFOUND) {
> +            error_report("%s: Invalid path %s: %s",
> +                         __func__, path, fdt_strerror(retval));
> +            exit(1);
> +        } else if (retval == -FDT_ERR_NOTFOUND) {
> +            retval = fdt_add_subnode(fdt, parent, basename);
> +            if (retval < 0) {
> +                break;
> +            }
> +        }
> +        parent = retval;
> +    }
> +
> +    g_free(dupname);
> +    return retval;
> +}
> +
>  void qemu_fdt_dumpdtb(void *fdt, int size)
>  {
>      const char *dumpdtb = current_machine->dumpdtb;
> -- 
> 2.23.0
> 
> 

Re: [RFC PATCH 2/5] hw/arm/virt: Add cpu-map to device tree

[Andrew Jones]

Hi Ying Fang,

The only difference between this and what I have in my tree[*]
is the removal of the socket node (which has been in the Linux
docs since June 2019). Any reason why you removed that node? In
any case, I think I deserve a bit more credit for this patch.

[*] https://github.com/rhdrjones/qemu/commit/35feecdd43475608c8f55973a0c159eac4aafefd

Thanks,
drew

On Thu, Feb 25, 2021 at 04:56:24PM +0800, Ying Fang wrote:
> Support device tree CPU topology descriptions.
> 
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
>  hw/arm/virt.c         | 38 +++++++++++++++++++++++++++++++++++++-
>  include/hw/arm/virt.h |  1 +
>  2 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 371147f3ae..c133b342b8 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -351,10 +351,11 @@ 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
> +     * 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
> @@ -407,8 +408,42 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
>                  ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
>          }
>  
> +        if (ms->smp.cpus > 1 && !vmc->no_cpu_topology) {
> +            qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle",
> +                                  qemu_fdt_alloc_phandle(vms->fdt));
> +        }
> +
>          g_free(nodename);
>      }
> +
> +    if (ms->smp.cpus > 1 && !vmc->no_cpu_topology) {
> +        /*
> +         * See Linux Documentation/devicetree/bindings/cpu/cpu-topology.txt
> +         */
> +        qemu_fdt_add_subnode(vms->fdt, "/cpus/cpu-map");
> +
> +        for (cpu = ms->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/%s%d/%s%d/%s%d",
> +                            "cluster", cpu / (ms->smp.cores * ms->smp.threads),
> +                            "core", (cpu / ms->smp.threads) % ms->smp.cores,
> +                            "thread", cpu % ms->smp.threads);
> +            } else {
> +                map_path = g_strdup_printf(
> +                            "/cpus/cpu-map/%s%d/%s%d",
> +                            "cluster", cpu / ms->smp.cores,
> +                            "core", cpu % ms->smp.cores);
> +            }
> +            qemu_fdt_add_path(vms->fdt, map_path);
> +            qemu_fdt_setprop_phandle(vms->fdt, map_path, "cpu", cpu_path);
> +            g_free(map_path);
> +            g_free(cpu_path);
> +        }
> +    }
>  }
>  
>  static void fdt_add_its_gic_node(VirtMachineState *vms)
> @@ -2742,6 +2777,7 @@ static void virt_machine_5_2_options(MachineClass *mc)
>      virt_machine_6_0_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_5_2, hw_compat_5_2_len);
>      vmc->no_secure_gpio = true;
> +    vmc->no_cpu_topology = true;
>  }
>  DEFINE_VIRT_MACHINE(5, 2)
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index ee9a93101e..7ef6d08ac3 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -129,6 +129,7 @@ struct VirtMachineClass {
>      bool no_kvm_steal_time;
>      bool acpi_expose_flash;
>      bool no_secure_gpio;
> +    bool no_cpu_topology;
>  };
>  
>  struct VirtMachineState {
> -- 
> 2.23.0
> 
> 

Re: [RFC PATCH 3/5] hw/arm/virt-acpi-build: distinguish possible and present cpus

[Andrew Jones]

On Thu, Feb 25, 2021 at 04:56:25PM +0800, Ying Fang wrote:
> When building ACPI tables regarding CPUs we should always build
> them for the number of possible CPUs, not the number of present
> CPUs. We then ensure only the present CPUs are enabled in madt.
> Furthermore, it is also needed if we are going to support CPU
> hotplug in the future.
> 
> This patch is a rework based on Andrew Jones's contribution at
> https://lists.gnu.org/archive/html/qemu-arm/2018-07/msg00076.html
> 
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c | 14 ++++++++++----
>  hw/arm/virt.c            |  2 ++
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index f9c9df916c..bb91152fe2 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -61,13 +61,16 @@
>  
>  static void acpi_dsdt_add_cpus(Aml *scope, VirtMachineState *vms)
>  {
> -    MachineState *ms = MACHINE(vms);
> +    CPUArchIdList *possible_cpus = MACHINE(vms)->possible_cpus;
>      uint16_t i;
>  
> -    for (i = 0; i < ms->smp.cpus; i++) {
> +    for (i = 0; i < possible_cpus->len; i++) {

Switching from ms->smp.cpus to possible_cpus->len makes some sense
here, since we're going to be using possible_cpus->cpus[] inside
the loop. Otherwise, I'd prefer switching to ms->smp.max_cpus.

>          Aml *dev = aml_device("C%.03X", i);
>          aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
>          aml_append(dev, aml_name_decl("_UID", aml_int(i)));
> +        if (possible_cpus->cpus[i].cpu == NULL) {
> +            aml_append(dev, aml_name_decl("_STA", aml_int(0)));
> +        }
>          aml_append(scope, dev);
>      }
>  }
> @@ -479,6 +482,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      const int *irqmap = vms->irqmap;
>      AcpiMadtGenericDistributor *gicd;
>      AcpiMadtGenericMsiFrame *gic_msi;
> +    CPUArchIdList *possible_cpus = MACHINE(vms)->possible_cpus;
>      int i;
>  
>      acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
> @@ -489,7 +493,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      gicd->base_address = cpu_to_le64(memmap[VIRT_GIC_DIST].base);
>      gicd->version = vms->gic_version;
>  
> -    for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
> +    for (i = 0; i < possible_cpus->len; i++) {

Same comment as above.

>          AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
>                                                             sizeof(*gicc));
>          ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
> @@ -504,7 +508,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          gicc->cpu_interface_number = cpu_to_le32(i);
>          gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
>          gicc->uid = cpu_to_le32(i);
> -        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
> +        if (possible_cpus->cpus[i].cpu != NULL) {
> +            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
> +        }
>  
>          if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
>              gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index c133b342b8..75659502e2 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2047,6 +2047,8 @@ static void machvirt_init(MachineState *machine)
>  
>          qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
>          object_unref(cpuobj);
> +        /* Initialize cpu member here since cpu hotplug is not supported yet */
> +        machine->possible_cpus->cpus[n].cpu = cpuobj;

This feels like we're violating a possible_cpus abstraction. Igor?
Also, it's using cpuobj after unrefing it.

>      }
>      fdt_add_timer_nodes(vms);
>      fdt_add_cpu_nodes(vms);
> -- 
> 2.23.0
> 
> 

Thanks,
drew

Re: [RFC PATCH 5/5] hw/arm/virt-acpi-build: add PPTT table

[Andrew Jones]

This is just [*] with some minor code changes

[*] https://github.com/rhdrjones/qemu/commit/439b38d67ca1f2cbfa5b9892a822b651ebd05c11

so it's disappointing that my name is nowhere to be found on it.

Also, the explanation of the DT and ACPI differences has been
dropped from the commit message of [*]. I'm not sure why.

Thanks,
drew

On Thu, Feb 25, 2021 at 04:56:27PM +0800, Ying Fang wrote:
> Add the Processor Properties Topology Table (PPTT) to present
> CPU topology information to the guest. A three-level cpu
> topology is built in accord with the linux kernel currently does.
> 
> Tested-by: Jiajie Li <lijiajie11@huawei.com>
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c | 50 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index bb91152fe2..38d50ce66c 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -436,6 +436,50 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>                   vms->oem_table_id);
>  }
>  
> +static void
> +build_pptt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> +{
> +    int pptt_start = table_data->len;
> +    int uid = 0, cpus = 0, socket = 0;
> +    MachineState *ms = MACHINE(vms);
> +    unsigned int smp_cores = ms->smp.cores;
> +    unsigned int smp_threads = ms->smp.threads;
> +
> +    acpi_data_push(table_data, sizeof(AcpiTableHeader));
> +
> +    for (socket = 0; cpus < ms->possible_cpus->len; socket++) {
> +        uint32_t socket_offset = table_data->len - pptt_start;
> +        int core;
> +
> +        build_socket_hierarchy(table_data, 0, socket);
> +
> +        for (core = 0; core < smp_cores; core++) {
> +            uint32_t core_offset = table_data->len - pptt_start;
> +            int thread;
> +
> +            if (smp_threads <= 1) {
> +                build_processor_hierarchy(table_data,
> +                                          ACPI_PPTT_ACPI_PROCESSOR_ID_VALID |
> +                                          ACPI_PPTT_ACPI_LEAF_NODE,
> +                                          socket_offset, uid++);
> +             } else {
> +                build_processor_hierarchy(table_data,
> +                                          ACPI_PPTT_ACPI_PROCESSOR_ID_VALID,
> +                                          socket_offset, core);
> +                for (thread = 0; thread < smp_threads; thread++) {
> +                    build_thread_hierarchy(table_data, core_offset, uid++);
> +                }
> +             }
> +        }
> +        cpus += smp_cores * smp_threads;
> +    }
> +
> +    build_header(linker, table_data,
> +                 (void *)(table_data->data + pptt_start), "PPTT",
> +                 table_data->len - pptt_start, 2,
> +                 vms->oem_id, vms->oem_table_id);
> +}
> +
>  /* GTDT */
>  static void
>  build_gtdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> @@ -688,6 +732,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>      unsigned dsdt, xsdt;
>      GArray *tables_blob = tables->table_data;
>      MachineState *ms = MACHINE(vms);
> +    bool cpu_topology_enabled = !vmc->no_cpu_topology;
>  
>      table_offsets = g_array_new(false, true /* clear */,
>                                          sizeof(uint32_t));
> @@ -707,6 +752,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>      acpi_add_table(table_offsets, tables_blob);
>      build_madt(tables_blob, tables->linker, vms);
>  
> +    if (ms->smp.cpus > 1 && cpu_topology_enabled) {
> +        acpi_add_table(table_offsets, tables_blob);
> +        build_pptt(tables_blob, tables->linker, vms);
> +    }
> +
>      acpi_add_table(table_offsets, tables_blob);
>      build_gtdt(tables_blob, tables->linker, vms);
>  
> -- 
> 2.23.0
> 
> 

Re: [RFC PATCH 4/5] hw/acpi/aml-build: add processor hierarchy node structure

[Andrew Jones]

On Thu, Feb 25, 2021 at 04:56:26PM +0800, Ying Fang wrote:
> Add the processor hierarchy node structures to build ACPI information
> for CPU topology. Since the private resources may be used to describe
> cache hierarchy and it is variable among different topology level,
> three helpers are introduced to describe the hierarchy.
> 
> (1) build_socket_hierarchy for socket description
> (2) build_processor_hierarchy for processor description
> (3) build_smt_hierarchy for thread (logic processor) description
> 
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> Signed-off-by: Henglong Fan <fanhenglong@huawei.com>
> ---
>  hw/acpi/aml-build.c         | 40 +++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/acpi-defs.h | 13 ++++++++++++
>  include/hw/acpi/aml-build.h |  7 +++++++
>  3 files changed, 60 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index a2cd7a5830..a0af3e9d73 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1888,6 +1888,46 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>                   table_data->len - slit_start, 1, oem_id, oem_table_id);
>  }
>  
> +/*
> + * ACPI 6.3: 5.2.29.1 Processor hierarchy node structure (Type 0)
> + */
> +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
> +{
> +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
> +    build_append_byte(tbl, 20);         /* Length, no private resources */
> +    build_append_int_noprefix(tbl, 0, 2);  /* Reserved */
> +    build_append_int_noprefix(tbl, ACPI_PPTT_PHYSICAL_PACKAGE, 4);

Missing '/* Flags */'

> +    build_append_int_noprefix(tbl, parent, 4); /* Parent */
> +    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
> +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
> +}
> +
> +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
> +                               uint32_t parent, uint32_t id)
> +{
> +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR);  /* Type 0 - processor */
> +    build_append_byte(tbl, 20);         /* Length, no private resources */
> +    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 */
> +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
> +}
> +
> +void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
> +{
> +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
> +    build_append_byte(tbl, 20);           /* Length, no private resources */
> +    build_append_int_noprefix(tbl, 0, 2); /* Reserved */
> +    build_append_int_noprefix(tbl,
> +                              ACPI_PPTT_ACPI_PROCESSOR_ID_VALID |
> +                              ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD |
> +                              ACPI_PPTT_ACPI_LEAF_NODE, 4);  /* Flags */
> +    build_append_int_noprefix(tbl, parent , 4); /* parent */

'parent' not capitalized. We want these comments to exactly match the text
in the spec.

> +    build_append_int_noprefix(tbl, id, 4);      /* ACPI processor ID */
> +    build_append_int_noprefix(tbl, 0, 4);       /* Num of private resources */
> +}
> +
>  /* 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/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index cf9f44299c..45e10d886f 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -618,4 +618,17 @@ struct AcpiIortRC {
>  } QEMU_PACKED;
>  typedef struct AcpiIortRC AcpiIortRC;
>  
> +enum {
> +    ACPI_PPTT_TYPE_PROCESSOR = 0,
> +    ACPI_PPTT_TYPE_CACHE,
> +    ACPI_PPTT_TYPE_ID,
> +    ACPI_PPTT_TYPE_RESERVED
> +};
> +
> +#define ACPI_PPTT_PHYSICAL_PACKAGE          (1)
> +#define ACPI_PPTT_ACPI_PROCESSOR_ID_VALID   (1 << 1)
> +#define ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD  (1 << 2)      /* ACPI 6.3 */
> +#define ACPI_PPTT_ACPI_LEAF_NODE            (1 << 3)      /* ACPI 6.3 */
> +#define ACPI_PPTT_ACPI_IDENTICAL            (1 << 4)      /* ACPI 6.3 */
> +
>  #endif
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 380d3e3924..7f0ca1a198 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -462,6 +462,13 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>  void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>                  const char *oem_id, const char *oem_table_id);
>  
> +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
> +
> +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
> +                               uint32_t parent, uint32_t id);
> +
> +void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);

Why does build_processor_hierarchy() take a flags argument, but the
others don't? Why not just have a single 'flags' taking function,
like [*] that works for all of them? I think that answer to that is
that when cache topology support is added it's better to break these
into separate functions, but should we do that now? It seems odd to
be introducing unused defines and this API before it's necessary.

[*] https://github.com/rhdrjones/qemu/commit/439b38d67ca1f2cbfa5b9892a822b651ebd05c11

Thanks,
drew

> +
>  void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>                  const char *oem_id, const char *oem_table_id);
>  
> -- 
> 2.23.0
> 
> 

Re: [RFC PATCH 0/5] hw/arm/virt: Introduce cpu topology support

[Andrew Jones]

On Thu, Feb 25, 2021 at 04:56:22PM +0800, Ying Fang wrote:
> An accurate cpu topology may help improve the cpu scheduler's decision
> making when dealing with multi-core system. So cpu topology description
> is helpful to provide guest with the right view. Dario Faggioli's talk
> in [0] also shows the virtual topology may has impact on sched performace.
> Thus this patch series is posted to introduce cpu topology support for
> arm platform.
> 
> Both fdt and ACPI are introduced to present the cpu topology. To describe
> the cpu topology via ACPI, a PPTT table is introduced according to the
> processor hierarchy node structure. This series is derived from [1], in
> [1] we are trying to bring both cpu and cache topology support for arm
> platform, but there is still some issues to solve to support the cache
> hierarchy. So we split the cpu topology part out and send it seperately.
> The patch series to support cache hierarchy will be send later since
> Salil Mehta's cpu hotplug feature need the cpu topology enabled first and
> he is waiting for it to be upstreamed.
> 
> This patch series was initially based on the patches posted by Andrew Jones [2].
> I jumped in on it since some OS vendor cooperative partner are eager for it.
> Thanks for Andrew's contribution.
> 
> After applying this patch series, launch a guest with virt-6.0 and cpu
> topology configured with sockets:cores:threads = 2:4:2, you will get the
> bellow messages with the lscpu command.
> 
> -----------------------------------------
> Architecture:                    aarch64
> CPU op-mode(s):                  64-bit
> Byte Order:                      Little Endian
> CPU(s):                          16
> On-line CPU(s) list:             0-15
> Thread(s) per core:              2

What CPU model was used? Did it actually support threads? If these were
KVM VCPUs, then I guess MPIDR.MT was not set on the CPUs. Apparently
that didn't confuse Linux? See [1] for how I once tried to deal with
threads.

[1] https://github.com/rhdrjones/qemu/commit/60218e0dd7b331031b644872d56f2aca42d0ff1e

> Core(s) per socket:              4
> Socket(s):                       2

Good, but what happens if you specify '-smp 16'? Do you get 16 sockets
each with 1 core? Or, do you get 1 socket with 16 cores? And, which do
we want and why? If you look at [2], then you'll see I was assuming we
want to prefer cores over sockets, since without topology descriptions
that's what the Linux guest kernel would do.

[2] https://github.com/rhdrjones/qemu/commit/c0670b1bccb4d08c7cf7c6957cc8878a2af131dd

> NUMA node(s):                    2

Why do we have two NUMA nodes in the guest? The two sockets in the
guest should not imply this.

Thanks,
drew

> Vendor ID:                       HiSilicon
> Model:                           0
> Model name:                      Kunpeng-920
> Stepping:                        0x1
> BogoMIPS:                        200.00
> NUMA node0 CPU(s):               0-7
> NUMA node1 CPU(s):               8-15
> 
> [0] https://kvmforum2020.sched.com/event/eE1y/virtual-topology-for-virtual-machines-friend-or-foe-dario-faggioli-suse
> [1] https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg02166.html
> [2] https://patchwork.ozlabs.org/project/qemu-devel/cover/20180704124923.32483-1-drjones@redhat.com
> 
> Ying Fang (5):
>   device_tree: Add qemu_fdt_add_path
>   hw/arm/virt: Add cpu-map to device tree
>   hw/arm/virt-acpi-build: distinguish possible and present cpus
>   hw/acpi/aml-build: add processor hierarchy node structure
>   hw/arm/virt-acpi-build: add PPTT table
> 
>  hw/acpi/aml-build.c          | 40 ++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c     | 64 +++++++++++++++++++++++++++++++++---
>  hw/arm/virt.c                | 40 +++++++++++++++++++++-
>  include/hw/acpi/acpi-defs.h  | 13 ++++++++
>  include/hw/acpi/aml-build.h  |  7 ++++
>  include/hw/arm/virt.h        |  1 +
>  include/sysemu/device_tree.h |  1 +
>  softmmu/device_tree.c        | 45 +++++++++++++++++++++++--
>  8 files changed, 204 insertions(+), 7 deletions(-)
> 
> -- 
> 2.23.0
> 

Re: [RFC PATCH 1/5] device_tree: Add qemu_fdt_add_path

[Ying Fang]

On 2/25/2021 7:03 PM, Andrew Jones wrote:
> Hi Ying Fang,
> 
> I don't see any change in this patch from what I have in my
> tree, so this should be
> 
>   From: Andrew Jones <drjones@redhat.com>
> 
> Thanks,
> drew
> 

Yes, I picked it from your qemu branch:
https://github.com/rhdrjones/qemu/commit/ecfc1565f22187d2c715a99bbcd35cf3a7e428fa

So what can I do to make it "From: Andrew Jones <drjones@redhat.com>" ?

Can I made it by using git commit --amend like below ?

git commit --amend --author "Andrew Jones <drjones@redhat.com>"

> On Thu, Feb 25, 2021 at 04:56:23PM +0800, Ying Fang wrote:
>> qemu_fdt_add_path() works like qemu_fdt_add_subnode(), except
>> it also adds any missing parent nodes. We also tweak an error
>> message of qemu_fdt_add_subnode().
>>
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>> ---
>>   include/sysemu/device_tree.h |  1 +
>>   softmmu/device_tree.c        | 45 ++++++++++++++++++++++++++++++++++--
>>   2 files changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
>> index 982c89345f..15fb98af98 100644
>> --- a/include/sysemu/device_tree.h
>> +++ b/include/sysemu/device_tree.h
>> @@ -104,6 +104,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 b9a3ddc518..1e3857ca0c 100644
>> --- a/softmmu/device_tree.c
>> +++ b/softmmu/device_tree.c
>> @@ -515,8 +515,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);
>>       }
>>   
>> @@ -524,6 +524,47 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
>>       return retval;
>>   }
>>   
>> +/*
>> + * Like qemu_fdt_add_subnode(), but will add all missing
>> + * subnodes in the path.
>> + */
>> +int qemu_fdt_add_path(void *fdt, const char *path)
>> +{
>> +    char *dupname, *basename, *p;
>> +    int parent, retval = -1;
>> +
>> +    if (path[0] != '/') {
>> +        return retval;
>> +    }
>> +
>> +    parent = fdt_path_offset(fdt, "/");
>> +    p = dupname = g_strdup(path);
>> +
>> +    while (p) {
>> +        *p = '/';
>> +        basename = p + 1;
>> +        p = strchr(p + 1, '/');
>> +        if (p) {
>> +            *p = '\0';
>> +        }
>> +        retval = fdt_path_offset(fdt, dupname);
>> +        if (retval < 0 && retval != -FDT_ERR_NOTFOUND) {
>> +            error_report("%s: Invalid path %s: %s",
>> +                         __func__, path, fdt_strerror(retval));
>> +            exit(1);
>> +        } else if (retval == -FDT_ERR_NOTFOUND) {
>> +            retval = fdt_add_subnode(fdt, parent, basename);
>> +            if (retval < 0) {
>> +                break;
>> +            }
>> +        }
>> +        parent = retval;
>> +    }
>> +
>> +    g_free(dupname);
>> +    return retval;
>> +}
>> +
>>   void qemu_fdt_dumpdtb(void *fdt, int size)
>>   {
>>       const char *dumpdtb = current_machine->dumpdtb;
>> -- 
>> 2.23.0
>>
>>
> 
> .
> 

Re: [RFC PATCH 2/5] hw/arm/virt: Add cpu-map to device tree

[Ying Fang]

On 2/25/2021 7:16 PM, Andrew Jones wrote:
> Hi Ying Fang,
> 
> The only difference between this and what I have in my tree[*]
> is the removal of the socket node (which has been in the Linux
> docs since June 2019). Any reason why you removed that node? In
> any case, I think I deserve a bit more credit for this patch.

Sorry, you surely deserve it. I forget to add it here.
Should I have a SOB of you here ?

The latest linux kernel use a four level cpu topology defined in

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/cpu/cpu-topology.txt?h=v5.11

ie. socket node, cluster node, core node, thread node.

The linux kernel 4.19 LTS use a three level cpu topology defined in
Documentation/devicetree/bindings/arm/topology.txt

ie. cluster node, core node, thread node.

Currently Qemu x86 has 4 level of cpu topology as: socket, die, core,
thread. Should arm64 active like it here ?

Further more, latest linux kernel define the cpu topology struct as.
So maybe it only cares about the socket, core, thread topology levels.

struct cpu_topology { 

     int thread_id; 

     int core_id; 

     int package_id; 

     int llc_id; 

     cpumask_t thread_sibling; 

     cpumask_t core_sibling; 

     cpumask_t llc_sibling; 

};

> 
> [*] https://github.com/rhdrjones/qemu/commit/35feecdd43475608c8f55973a0c159eac4aafefd
> 
> Thanks,
> drew
> 
> On Thu, Feb 25, 2021 at 04:56:24PM +0800, Ying Fang wrote:
>> Support device tree CPU topology descriptions.
>>
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>> ---
>>   hw/arm/virt.c         | 38 +++++++++++++++++++++++++++++++++++++-
>>   include/hw/arm/virt.h |  1 +
>>   2 files changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 371147f3ae..c133b342b8 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -351,10 +351,11 @@ 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
>> +     * 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
>> @@ -407,8 +408,42 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
>>                   ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
>>           }
>>   
>> +        if (ms->smp.cpus > 1 && !vmc->no_cpu_topology) {
>> +            qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle",
>> +                                  qemu_fdt_alloc_phandle(vms->fdt));
>> +        }
>> +
>>           g_free(nodename);
>>       }
>> +
>> +    if (ms->smp.cpus > 1 && !vmc->no_cpu_topology) {
>> +        /*
>> +         * See Linux Documentation/devicetree/bindings/cpu/cpu-topology.txt
>> +         */
>> +        qemu_fdt_add_subnode(vms->fdt, "/cpus/cpu-map");
>> +
>> +        for (cpu = ms->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/%s%d/%s%d/%s%d",
>> +                            "cluster", cpu / (ms->smp.cores * ms->smp.threads),

a cluster node may be replaced by socket to keep accord with the latest 
kernel.

>> +                            "core", (cpu / ms->smp.threads) % ms->smp.cores,
>> +                            "thread", cpu % ms->smp.threads);
>> +            } else {
>> +                map_path = g_strdup_printf(
>> +                            "/cpus/cpu-map/%s%d/%s%d",
>> +                            "cluster", cpu / ms->smp.cores,
>> +                            "core", cpu % ms->smp.cores);
>> +            }
>> +            qemu_fdt_add_path(vms->fdt, map_path);
>> +            qemu_fdt_setprop_phandle(vms->fdt, map_path, "cpu", cpu_path);
>> +            g_free(map_path);
>> +            g_free(cpu_path);
>> +        }
>> +    }
>>   }
>>   
>>   static void fdt_add_its_gic_node(VirtMachineState *vms)
>> @@ -2742,6 +2777,7 @@ static void virt_machine_5_2_options(MachineClass *mc)
>>       virt_machine_6_0_options(mc);
>>       compat_props_add(mc->compat_props, hw_compat_5_2, hw_compat_5_2_len);
>>       vmc->no_secure_gpio = true;
>> +    vmc->no_cpu_topology = true;
>>   }
>>   DEFINE_VIRT_MACHINE(5, 2)
>>   
>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>> index ee9a93101e..7ef6d08ac3 100644
>> --- a/include/hw/arm/virt.h
>> +++ b/include/hw/arm/virt.h
>> @@ -129,6 +129,7 @@ struct VirtMachineClass {
>>       bool no_kvm_steal_time;
>>       bool acpi_expose_flash;
>>       bool no_secure_gpio;
>> +    bool no_cpu_topology;
>>   };
>>   
>>   struct VirtMachineState {
>> -- 
>> 2.23.0
>>
>>
> 
> .
> 

Re: [RFC PATCH 1/5] device_tree: Add qemu_fdt_add_path

[Andrew Jones]

On Thu, Feb 25, 2021 at 08:54:40PM +0800, Ying Fang wrote:
> 
> 
> On 2/25/2021 7:03 PM, Andrew Jones wrote:
> > Hi Ying Fang,
> > 
> > I don't see any change in this patch from what I have in my
> > tree, so this should be
> > 
> >   From: Andrew Jones <drjones@redhat.com>
> > 
> > Thanks,
> > drew
> > 
> 
> Yes, I picked it from your qemu branch:
> https://github.com/rhdrjones/qemu/commit/ecfc1565f22187d2c715a99bbcd35cf3a7e428fa
> 
> So what can I do to make it "From: Andrew Jones <drjones@redhat.com>" ?
> 
> Can I made it by using git commit --amend like below ?
> 
> git commit --amend --author "Andrew Jones <drjones@redhat.com>"

That's one way to fix it now, but normally when you apply/cherry-pick
a patch it will keep the authorship. Then, all you have to do is
post like usual and the "From: ..." will show up automatically.

Thanks,
drew

> 
> > On Thu, Feb 25, 2021 at 04:56:23PM +0800, Ying Fang wrote:
> > > qemu_fdt_add_path() works like qemu_fdt_add_subnode(), except
> > > it also adds any missing parent nodes. We also tweak an error
> > > message of qemu_fdt_add_subnode().
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > Signed-off-by: Ying Fang <fangying1@huawei.com>
> > > ---
> > >   include/sysemu/device_tree.h |  1 +
> > >   softmmu/device_tree.c        | 45 ++++++++++++++++++++++++++++++++++--
> > >   2 files changed, 44 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> > > index 982c89345f..15fb98af98 100644
> > > --- a/include/sysemu/device_tree.h
> > > +++ b/include/sysemu/device_tree.h
> > > @@ -104,6 +104,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 b9a3ddc518..1e3857ca0c 100644
> > > --- a/softmmu/device_tree.c
> > > +++ b/softmmu/device_tree.c
> > > @@ -515,8 +515,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);
> > >       }
> > > @@ -524,6 +524,47 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
> > >       return retval;
> > >   }
> > > +/*
> > > + * Like qemu_fdt_add_subnode(), but will add all missing
> > > + * subnodes in the path.
> > > + */
> > > +int qemu_fdt_add_path(void *fdt, const char *path)
> > > +{
> > > +    char *dupname, *basename, *p;
> > > +    int parent, retval = -1;
> > > +
> > > +    if (path[0] != '/') {
> > > +        return retval;
> > > +    }
> > > +
> > > +    parent = fdt_path_offset(fdt, "/");
> > > +    p = dupname = g_strdup(path);
> > > +
> > > +    while (p) {
> > > +        *p = '/';
> > > +        basename = p + 1;
> > > +        p = strchr(p + 1, '/');
> > > +        if (p) {
> > > +            *p = '\0';
> > > +        }
> > > +        retval = fdt_path_offset(fdt, dupname);
> > > +        if (retval < 0 && retval != -FDT_ERR_NOTFOUND) {
> > > +            error_report("%s: Invalid path %s: %s",
> > > +                         __func__, path, fdt_strerror(retval));
> > > +            exit(1);
> > > +        } else if (retval == -FDT_ERR_NOTFOUND) {
> > > +            retval = fdt_add_subnode(fdt, parent, basename);
> > > +            if (retval < 0) {
> > > +                break;
> > > +            }
> > > +        }
> > > +        parent = retval;
> > > +    }
> > > +
> > > +    g_free(dupname);
> > > +    return retval;
> > > +}
> > > +
> > >   void qemu_fdt_dumpdtb(void *fdt, int size)
> > >   {
> > >       const char *dumpdtb = current_machine->dumpdtb;
> > > -- 
> > > 2.23.0
> > > 
> > > 
> > 
> > .
> > 
> 

Re: [RFC PATCH 1/5] device_tree: Add qemu_fdt_add_path

[Ying Fang]

On 2/25/2021 9:25 PM, Andrew Jones wrote:
> On Thu, Feb 25, 2021 at 08:54:40PM +0800, Ying Fang wrote:
>>
>>
>> On 2/25/2021 7:03 PM, Andrew Jones wrote:
>>> Hi Ying Fang,
>>>
>>> I don't see any change in this patch from what I have in my
>>> tree, so this should be
>>>
>>>    From: Andrew Jones <drjones@redhat.com>
>>>
>>> Thanks,
>>> drew
>>>
>>
>> Yes, I picked it from your qemu branch:
>> https://github.com/rhdrjones/qemu/commit/ecfc1565f22187d2c715a99bbcd35cf3a7e428fa
>>
>> So what can I do to make it "From: Andrew Jones <drjones@redhat.com>" ?
>>
>> Can I made it by using git commit --amend like below ?
>>
>> git commit --amend --author "Andrew Jones <drjones@redhat.com>"
> 
> That's one way to fix it now, but normally when you apply/cherry-pick
> a patch it will keep the authorship. Then, all you have to do is
> post like usual and the "From: ..." will show up automatically.
> 

Hmm, I know cherry-pick can do that. But sometimes there maybe
conflicts, so I have to backport it by hand and copy the commit
msg back, thus the authorship may be lost.


> Thanks,
> drew
> 
>>
>>> On Thu, Feb 25, 2021 at 04:56:23PM +0800, Ying Fang wrote:
>>>> qemu_fdt_add_path() works like qemu_fdt_add_subnode(), except
>>>> it also adds any missing parent nodes. We also tweak an error
>>>> message of qemu_fdt_add_subnode().
>>>>
>>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>>>> ---
>>>>    include/sysemu/device_tree.h |  1 +
>>>>    softmmu/device_tree.c        | 45 ++++++++++++++++++++++++++++++++++--
>>>>    2 files changed, 44 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
>>>> index 982c89345f..15fb98af98 100644
>>>> --- a/include/sysemu/device_tree.h
>>>> +++ b/include/sysemu/device_tree.h
>>>> @@ -104,6 +104,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 b9a3ddc518..1e3857ca0c 100644
>>>> --- a/softmmu/device_tree.c
>>>> +++ b/softmmu/device_tree.c
>>>> @@ -515,8 +515,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);
>>>>        }
>>>> @@ -524,6 +524,47 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
>>>>        return retval;
>>>>    }
>>>> +/*
>>>> + * Like qemu_fdt_add_subnode(), but will add all missing
>>>> + * subnodes in the path.
>>>> + */
>>>> +int qemu_fdt_add_path(void *fdt, const char *path)
>>>> +{
>>>> +    char *dupname, *basename, *p;
>>>> +    int parent, retval = -1;
>>>> +
>>>> +    if (path[0] != '/') {
>>>> +        return retval;
>>>> +    }
>>>> +
>>>> +    parent = fdt_path_offset(fdt, "/");
>>>> +    p = dupname = g_strdup(path);
>>>> +
>>>> +    while (p) {
>>>> +        *p = '/';
>>>> +        basename = p + 1;
>>>> +        p = strchr(p + 1, '/');
>>>> +        if (p) {
>>>> +            *p = '\0';
>>>> +        }
>>>> +        retval = fdt_path_offset(fdt, dupname);
>>>> +        if (retval < 0 && retval != -FDT_ERR_NOTFOUND) {
>>>> +            error_report("%s: Invalid path %s: %s",
>>>> +                         __func__, path, fdt_strerror(retval));
>>>> +            exit(1);
>>>> +        } else if (retval == -FDT_ERR_NOTFOUND) {
>>>> +            retval = fdt_add_subnode(fdt, parent, basename);
>>>> +            if (retval < 0) {
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +        parent = retval;
>>>> +    }
>>>> +
>>>> +    g_free(dupname);
>>>> +    return retval;
>>>> +}
>>>> +
>>>>    void qemu_fdt_dumpdtb(void *fdt, int size)
>>>>    {
>>>>        const char *dumpdtb = current_machine->dumpdtb;
>>>> -- 
>>>> 2.23.0
>>>>
>>>>
>>>
>>> .
>>>
>>
> 
> .
> 

Re: [RFC PATCH 2/5] hw/arm/virt: Add cpu-map to device tree

[Andrew Jones]

On Thu, Feb 25, 2021 at 09:18:22PM +0800, Ying Fang wrote:
> 
> 
> On 2/25/2021 7:16 PM, Andrew Jones wrote:
> > Hi Ying Fang,
> > 
> > The only difference between this and what I have in my tree[*]
> > is the removal of the socket node (which has been in the Linux
> > docs since June 2019). Any reason why you removed that node? In
> > any case, I think I deserve a bit more credit for this patch.
> 
> Sorry, you surely deserve it. I forget to add it here.
> Should I have a SOB of you here ?
> 
> The latest linux kernel use a four level cpu topology defined in
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/cpu/cpu-topology.txt?h=v5.11
> 
> ie. socket node, cluster node, core node, thread node.

Yes, this is why the code I wrote uses the 'socket' node.

> 
> The linux kernel 4.19 LTS use a three level cpu topology defined in
> Documentation/devicetree/bindings/arm/topology.txt

I don't think we want to target that guest kernel with this
new QEMU feature. If we must support that guest kernel, then
I would do it under a machine property, like compat_cpumap
or something.

> 
> ie. cluster node, core node, thread node.
> 
> Currently Qemu x86 has 4 level of cpu topology as: socket, die, core,
> thread. Should arm64 active like it here ?

Does the arm64 guest kernel support the concept of 'die'? (I don't
think so) Anyway, there's no such concept in the current cpu-map
definition. So, if the guest kernel does support die, then what
does it map that to from DT and ACPI?

> 
> Further more, latest linux kernel define the cpu topology struct as.
> So maybe it only cares about the socket, core, thread topology levels.
> 
> struct cpu_topology {
> 
>     int thread_id;
> 
>     int core_id;
> 
>     int package_id;
> 
>     int llc_id;
> 
>     cpumask_t thread_sibling;
> 
>     cpumask_t core_sibling;
> 
>     cpumask_t llc_sibling;
> 
> };
> 
> > 
> > [*] https://github.com/rhdrjones/qemu/commit/35feecdd43475608c8f55973a0c159eac4aafefd
> > 
> > Thanks,
> > drew
> > 
> > On Thu, Feb 25, 2021 at 04:56:24PM +0800, Ying Fang wrote:
> > > Support device tree CPU topology descriptions.
> > > 
> > > Signed-off-by: Ying Fang <fangying1@huawei.com>
> > > ---
> > >   hw/arm/virt.c         | 38 +++++++++++++++++++++++++++++++++++++-
> > >   include/hw/arm/virt.h |  1 +
> > >   2 files changed, 38 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index 371147f3ae..c133b342b8 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -351,10 +351,11 @@ 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
> > > +     * 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
> > > @@ -407,8 +408,42 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
> > >                   ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
> > >           }
> > > +        if (ms->smp.cpus > 1 && !vmc->no_cpu_topology) {
> > > +            qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle",
> > > +                                  qemu_fdt_alloc_phandle(vms->fdt));
> > > +        }
> > > +
> > >           g_free(nodename);
> > >       }
> > > +
> > > +    if (ms->smp.cpus > 1 && !vmc->no_cpu_topology) {
> > > +        /*
> > > +         * See Linux Documentation/devicetree/bindings/cpu/cpu-topology.txt
> > > +         */
> > > +        qemu_fdt_add_subnode(vms->fdt, "/cpus/cpu-map");
> > > +
> > > +        for (cpu = ms->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/%s%d/%s%d/%s%d",
> > > +                            "cluster", cpu / (ms->smp.cores * ms->smp.threads),
> 
> a cluster node may be replaced by socket to keep accord with the latest
> kernel.

Right, in which case this patch would be identical to the one in my
branch.

Thanks,
drew

> 
> > > +                            "core", (cpu / ms->smp.threads) % ms->smp.cores,
> > > +                            "thread", cpu % ms->smp.threads);
> > > +            } else {
> > > +                map_path = g_strdup_printf(
> > > +                            "/cpus/cpu-map/%s%d/%s%d",
> > > +                            "cluster", cpu / ms->smp.cores,
> > > +                            "core", cpu % ms->smp.cores);
> > > +            }
> > > +            qemu_fdt_add_path(vms->fdt, map_path);
> > > +            qemu_fdt_setprop_phandle(vms->fdt, map_path, "cpu", cpu_path);
> > > +            g_free(map_path);
> > > +            g_free(cpu_path);
> > > +        }
> > > +    }
> > >   }
> > >   static void fdt_add_its_gic_node(VirtMachineState *vms)
> > > @@ -2742,6 +2777,7 @@ static void virt_machine_5_2_options(MachineClass *mc)
> > >       virt_machine_6_0_options(mc);
> > >       compat_props_add(mc->compat_props, hw_compat_5_2, hw_compat_5_2_len);
> > >       vmc->no_secure_gpio = true;
> > > +    vmc->no_cpu_topology = true;
> > >   }
> > >   DEFINE_VIRT_MACHINE(5, 2)
> > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > > index ee9a93101e..7ef6d08ac3 100644
> > > --- a/include/hw/arm/virt.h
> > > +++ b/include/hw/arm/virt.h
> > > @@ -129,6 +129,7 @@ struct VirtMachineClass {
> > >       bool no_kvm_steal_time;
> > >       bool acpi_expose_flash;
> > >       bool no_secure_gpio;
> > > +    bool no_cpu_topology;
> > >   };
> > >   struct VirtMachineState {
> > > -- 
> > > 2.23.0
> > > 
> > > 
> > 
> > .
> > 
> 

Re: [RFC PATCH 4/5] hw/acpi/aml-build: add processor hierarchy node structure

[Ying Fang]

On 2/25/2021 7:47 PM, Andrew Jones wrote:
> On Thu, Feb 25, 2021 at 04:56:26PM +0800, Ying Fang wrote:
>> Add the processor hierarchy node structures to build ACPI information
>> for CPU topology. Since the private resources may be used to describe
>> cache hierarchy and it is variable among different topology level,
>> three helpers are introduced to describe the hierarchy.
>>
>> (1) build_socket_hierarchy for socket description
>> (2) build_processor_hierarchy for processor description
>> (3) build_smt_hierarchy for thread (logic processor) description
>>
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>> Signed-off-by: Henglong Fan <fanhenglong@huawei.com>
>> ---
>>   hw/acpi/aml-build.c         | 40 +++++++++++++++++++++++++++++++++++++
>>   include/hw/acpi/acpi-defs.h | 13 ++++++++++++
>>   include/hw/acpi/aml-build.h |  7 +++++++
>>   3 files changed, 60 insertions(+)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index a2cd7a5830..a0af3e9d73 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -1888,6 +1888,46 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>>                    table_data->len - slit_start, 1, oem_id, oem_table_id);
>>   }
>>   
>> +/*
>> + * ACPI 6.3: 5.2.29.1 Processor hierarchy node structure (Type 0)
>> + */
>> +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
>> +{
>> +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
>> +    build_append_byte(tbl, 20);         /* Length, no private resources */
>> +    build_append_int_noprefix(tbl, 0, 2);  /* Reserved */
>> +    build_append_int_noprefix(tbl, ACPI_PPTT_PHYSICAL_PACKAGE, 4);
> 
> Missing '/* Flags */'

Will fix.

> 
>> +    build_append_int_noprefix(tbl, parent, 4); /* Parent */
>> +    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
>> +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
>> +}
>> +
>> +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
>> +                               uint32_t parent, uint32_t id)
>> +{
>> +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR);  /* Type 0 - processor */
>> +    build_append_byte(tbl, 20);         /* Length, no private resources */
>> +    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 */
>> +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
>> +}
>> +
>> +void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
>> +{
>> +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
>> +    build_append_byte(tbl, 20);           /* Length, no private resources */
>> +    build_append_int_noprefix(tbl, 0, 2); /* Reserved */
>> +    build_append_int_noprefix(tbl,
>> +                              ACPI_PPTT_ACPI_PROCESSOR_ID_VALID |
>> +                              ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD |
>> +                              ACPI_PPTT_ACPI_LEAF_NODE, 4);  /* Flags */
>> +    build_append_int_noprefix(tbl, parent , 4); /* parent */
> 
> 'parent' not capitalized. We want these comments to exactly match the text
> in the spec.

Will fix.

> 
>> +    build_append_int_noprefix(tbl, id, 4);      /* ACPI processor ID */
>> +    build_append_int_noprefix(tbl, 0, 4);       /* Num of private resources */
>> +}
>> +
>>   /* 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/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>> index cf9f44299c..45e10d886f 100644
>> --- a/include/hw/acpi/acpi-defs.h
>> +++ b/include/hw/acpi/acpi-defs.h
>> @@ -618,4 +618,17 @@ struct AcpiIortRC {
>>   } QEMU_PACKED;
>>   typedef struct AcpiIortRC AcpiIortRC;
>>   
>> +enum {
>> +    ACPI_PPTT_TYPE_PROCESSOR = 0,
>> +    ACPI_PPTT_TYPE_CACHE,
>> +    ACPI_PPTT_TYPE_ID,
>> +    ACPI_PPTT_TYPE_RESERVED
>> +};
>> +
>> +#define ACPI_PPTT_PHYSICAL_PACKAGE          (1)
>> +#define ACPI_PPTT_ACPI_PROCESSOR_ID_VALID   (1 << 1)
>> +#define ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD  (1 << 2)      /* ACPI 6.3 */
>> +#define ACPI_PPTT_ACPI_LEAF_NODE            (1 << 3)      /* ACPI 6.3 */
>> +#define ACPI_PPTT_ACPI_IDENTICAL            (1 << 4)      /* ACPI 6.3 */
>> +
>>   #endif
>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index 380d3e3924..7f0ca1a198 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -462,6 +462,13 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>>   void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>>                   const char *oem_id, const char *oem_table_id);
>>   
>> +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
>> +
>> +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
>> +                               uint32_t parent, uint32_t id);
>> +
>> +void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
> 
> Why does build_processor_hierarchy() take a flags argument, but the
> others don't? Why not just have a single 'flags' taking function,
> like [*] that works for all of them? I think that answer to that is

Yes, you are right.

> that when cache topology support is added it's better to break these
> into separate functions, but should we do that now? It seems odd to
> be introducing unused defines and this API before it's necessary.
So it is better for us to keep just one common build_processor_hierarchy
API here in your opinion.

> 
> [*] https://github.com/rhdrjones/qemu/commit/439b38d67ca1f2cbfa5b9892a822b651ebd05c11
> 
> Thanks,
> drew
> 
>> +
>>   void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>>                   const char *oem_id, const char *oem_table_id);
>>   
>> -- 
>> 2.23.0
>>
>>
> 
> .
> 

Thanks,
Ying.

Re: [RFC PATCH 5/5] hw/arm/virt-acpi-build: add PPTT table

[Ying Fang]

On 2/25/2021 7:38 PM, Andrew Jones wrote:
> 
> This is just [*] with some minor code changes
> 
> [*] https://github.com/rhdrjones/qemu/commit/439b38d67ca1f2cbfa5b9892a822b651ebd05c11
> 
> so it's disappointing that my name is nowhere to be found on it.
> 
> Also, the explanation of the DT and ACPI differences has been
> dropped from the commit message of [*]. I'm not sure why.
> 

Will fix that. I will add SOB of you then you can help to comment on it.

> Thanks,
> drew
> 
> On Thu, Feb 25, 2021 at 04:56:27PM +0800, Ying Fang wrote:
>> Add the Processor Properties Topology Table (PPTT) to present
>> CPU topology information to the guest. A three-level cpu
>> topology is built in accord with the linux kernel currently does.
>>
>> Tested-by: Jiajie Li <lijiajie11@huawei.com>
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>> ---
>>   hw/arm/virt-acpi-build.c | 50 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 50 insertions(+)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index bb91152fe2..38d50ce66c 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -436,6 +436,50 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>                    vms->oem_table_id);
>>   }
>>   
>> +static void
>> +build_pptt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>> +{
>> +    int pptt_start = table_data->len;
>> +    int uid = 0, cpus = 0, socket = 0;
>> +    MachineState *ms = MACHINE(vms);
>> +    unsigned int smp_cores = ms->smp.cores;
>> +    unsigned int smp_threads = ms->smp.threads;
>> +
>> +    acpi_data_push(table_data, sizeof(AcpiTableHeader));
>> +
>> +    for (socket = 0; cpus < ms->possible_cpus->len; socket++) {
>> +        uint32_t socket_offset = table_data->len - pptt_start;
>> +        int core;
>> +
>> +        build_socket_hierarchy(table_data, 0, socket);
>> +
>> +        for (core = 0; core < smp_cores; core++) {
>> +            uint32_t core_offset = table_data->len - pptt_start;
>> +            int thread;
>> +
>> +            if (smp_threads <= 1) {
>> +                build_processor_hierarchy(table_data,
>> +                                          ACPI_PPTT_ACPI_PROCESSOR_ID_VALID |
>> +                                          ACPI_PPTT_ACPI_LEAF_NODE,
>> +                                          socket_offset, uid++);
>> +             } else {
>> +                build_processor_hierarchy(table_data,
>> +                                          ACPI_PPTT_ACPI_PROCESSOR_ID_VALID,
>> +                                          socket_offset, core);
>> +                for (thread = 0; thread < smp_threads; thread++) {
>> +                    build_thread_hierarchy(table_data, core_offset, uid++);
>> +                }
>> +             }
>> +        }
>> +        cpus += smp_cores * smp_threads;
>> +    }
>> +
>> +    build_header(linker, table_data,
>> +                 (void *)(table_data->data + pptt_start), "PPTT",
>> +                 table_data->len - pptt_start, 2,
>> +                 vms->oem_id, vms->oem_table_id);
>> +}
>> +
>>   /* GTDT */
>>   static void
>>   build_gtdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>> @@ -688,6 +732,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>       unsigned dsdt, xsdt;
>>       GArray *tables_blob = tables->table_data;
>>       MachineState *ms = MACHINE(vms);
>> +    bool cpu_topology_enabled = !vmc->no_cpu_topology;
>>   
>>       table_offsets = g_array_new(false, true /* clear */,
>>                                           sizeof(uint32_t));
>> @@ -707,6 +752,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>       acpi_add_table(table_offsets, tables_blob);
>>       build_madt(tables_blob, tables->linker, vms);
>>   
>> +    if (ms->smp.cpus > 1 && cpu_topology_enabled) {
>> +        acpi_add_table(table_offsets, tables_blob);
>> +        build_pptt(tables_blob, tables->linker, vms);
>> +    }
>> +
>>       acpi_add_table(table_offsets, tables_blob);
>>       build_gtdt(tables_blob, tables->linker, vms);
>>   
>> -- 
>> 2.23.0
>>
>>
> 
> .
> 

Re: [RFC PATCH 0/5] hw/arm/virt: Introduce cpu topology support

[Ying Fang]

On 2/25/2021 8:02 PM, Andrew Jones wrote:
> On Thu, Feb 25, 2021 at 04:56:22PM +0800, Ying Fang wrote:
>> An accurate cpu topology may help improve the cpu scheduler's decision
>> making when dealing with multi-core system. So cpu topology description
>> is helpful to provide guest with the right view. Dario Faggioli's talk
>> in [0] also shows the virtual topology may has impact on sched performace.
>> Thus this patch series is posted to introduce cpu topology support for
>> arm platform.
>>
>> Both fdt and ACPI are introduced to present the cpu topology. To describe
>> the cpu topology via ACPI, a PPTT table is introduced according to the
>> processor hierarchy node structure. This series is derived from [1], in
>> [1] we are trying to bring both cpu and cache topology support for arm
>> platform, but there is still some issues to solve to support the cache
>> hierarchy. So we split the cpu topology part out and send it seperately.
>> The patch series to support cache hierarchy will be send later since
>> Salil Mehta's cpu hotplug feature need the cpu topology enabled first and
>> he is waiting for it to be upstreamed.
>>
>> This patch series was initially based on the patches posted by Andrew Jones [2].
>> I jumped in on it since some OS vendor cooperative partner are eager for it.
>> Thanks for Andrew's contribution.
>>
>> After applying this patch series, launch a guest with virt-6.0 and cpu
>> topology configured with sockets:cores:threads = 2:4:2, you will get the
>> bellow messages with the lscpu command.
>>
>> -----------------------------------------
>> Architecture:                    aarch64
>> CPU op-mode(s):                  64-bit
>> Byte Order:                      Little Endian
>> CPU(s):                          16
>> On-line CPU(s) list:             0-15
>> Thread(s) per core:              2
> 
> What CPU model was used? Did it actually support threads? If these were

It's tested on Huawei Kunpeng 920 CPU model and vcpu host-passthrough.
It does not support threads for now, but the next version 930 may
support it. Here we emulate a virtual cpu topology, a virtual 2 threads
is used to do the test.


> KVM VCPUs, then I guess MPIDR.MT was not set on the CPUs. Apparently
> that didn't confuse Linux? See [1] for how I once tried to deal with
> threads.
> 
> [1] https://github.com/rhdrjones/qemu/commit/60218e0dd7b331031b644872d56f2aca42d0ff1e
> 

If ACPI PPTT table is specified, the linux kernel won't check the MPIDR
register to populate cpu topology. Moreover MPIDR does not ensure a
right cpu topology. So it won't be a problem if MPIDR.MT is not set.

>> Core(s) per socket:              4
>> Socket(s):                       2
> 
> Good, but what happens if you specify '-smp 16'? Do you get 16 sockets
> each with 1 core? Or, do you get 1 socket with 16 cores? And, which do
> we want and why? If you look at [2], then you'll see I was assuming we
> want to prefer cores over sockets, since without topology descriptions
> that's what the Linux guest kernel would do.
> 
> [2] https://github.com/rhdrjones/qemu/commit/c0670b1bccb4d08c7cf7c6957cc8878a2af131dd
> 
>> NUMA node(s):                    2
> 
> Why do we have two NUMA nodes in the guest? The two sockets in the
> guest should not imply this.

The two NUMA nodes are emulated by Qemu since we already have guest numa
topology feature. So the two sockets in the guest has nothing to do with
it. Actually even one socket may have two numa nodes in it in real cpu
model.

> 
> Thanks,
> drew
> 
>> Vendor ID:                       HiSilicon
>> Model:                           0
>> Model name:                      Kunpeng-920
>> Stepping:                        0x1
>> BogoMIPS:                        200.00
>> NUMA node0 CPU(s):               0-7
>> NUMA node1 CPU(s):               8-15
>>
>> [0] https://kvmforum2020.sched.com/event/eE1y/virtual-topology-for-virtual-machines-friend-or-foe-dario-faggioli-suse
>> [1] https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg02166.html
>> [2] https://patchwork.ozlabs.org/project/qemu-devel/cover/20180704124923.32483-1-drjones@redhat.com
>>
>> Ying Fang (5):
>>    device_tree: Add qemu_fdt_add_path
>>    hw/arm/virt: Add cpu-map to device tree
>>    hw/arm/virt-acpi-build: distinguish possible and present cpus
>>    hw/acpi/aml-build: add processor hierarchy node structure
>>    hw/arm/virt-acpi-build: add PPTT table
>>
>>   hw/acpi/aml-build.c          | 40 ++++++++++++++++++++++
>>   hw/arm/virt-acpi-build.c     | 64 +++++++++++++++++++++++++++++++++---
>>   hw/arm/virt.c                | 40 +++++++++++++++++++++-
>>   include/hw/acpi/acpi-defs.h  | 13 ++++++++
>>   include/hw/acpi/aml-build.h  |  7 ++++
>>   include/hw/arm/virt.h        |  1 +
>>   include/sysemu/device_tree.h |  1 +
>>   softmmu/device_tree.c        | 45 +++++++++++++++++++++++--
>>   8 files changed, 204 insertions(+), 7 deletions(-)
>>
>> -- 
>> 2.23.0
>>
> 
> .
> 

Re: [RFC PATCH 4/5] hw/acpi/aml-build: add processor hierarchy node structure

[Andrew Jones]

On Fri, Feb 26, 2021 at 10:23:03AM +0800, Ying Fang wrote:
> 
> 
> On 2/25/2021 7:47 PM, Andrew Jones wrote:
> > On Thu, Feb 25, 2021 at 04:56:26PM +0800, Ying Fang wrote:
> > > Add the processor hierarchy node structures to build ACPI information
> > > for CPU topology. Since the private resources may be used to describe
> > > cache hierarchy and it is variable among different topology level,
> > > three helpers are introduced to describe the hierarchy.
> > > 
> > > (1) build_socket_hierarchy for socket description
> > > (2) build_processor_hierarchy for processor description
> > > (3) build_smt_hierarchy for thread (logic processor) description
> > > 
> > > Signed-off-by: Ying Fang <fangying1@huawei.com>
> > > Signed-off-by: Henglong Fan <fanhenglong@huawei.com>
> > > ---
> > >   hw/acpi/aml-build.c         | 40 +++++++++++++++++++++++++++++++++++++
> > >   include/hw/acpi/acpi-defs.h | 13 ++++++++++++
> > >   include/hw/acpi/aml-build.h |  7 +++++++
> > >   3 files changed, 60 insertions(+)
> > > 
> > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > index a2cd7a5830..a0af3e9d73 100644
> > > --- a/hw/acpi/aml-build.c
> > > +++ b/hw/acpi/aml-build.c
> > > @@ -1888,6 +1888,46 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> > >                    table_data->len - slit_start, 1, oem_id, oem_table_id);
> > >   }
> > > +/*
> > > + * ACPI 6.3: 5.2.29.1 Processor hierarchy node structure (Type 0)
> > > + */
> > > +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
> > > +{
> > > +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
> > > +    build_append_byte(tbl, 20);         /* Length, no private resources */
> > > +    build_append_int_noprefix(tbl, 0, 2);  /* Reserved */
> > > +    build_append_int_noprefix(tbl, ACPI_PPTT_PHYSICAL_PACKAGE, 4);
> > 
> > Missing '/* Flags */'
> 
> Will fix.
> 
> > 
> > > +    build_append_int_noprefix(tbl, parent, 4); /* Parent */
> > > +    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
> > > +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
> > > +}
> > > +
> > > +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
> > > +                               uint32_t parent, uint32_t id)
> > > +{
> > > +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR);  /* Type 0 - processor */
> > > +    build_append_byte(tbl, 20);         /* Length, no private resources */
> > > +    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 */
> > > +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
> > > +}
> > > +
> > > +void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
> > > +{
> > > +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
> > > +    build_append_byte(tbl, 20);           /* Length, no private resources */
> > > +    build_append_int_noprefix(tbl, 0, 2); /* Reserved */
> > > +    build_append_int_noprefix(tbl,
> > > +                              ACPI_PPTT_ACPI_PROCESSOR_ID_VALID |
> > > +                              ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD |
> > > +                              ACPI_PPTT_ACPI_LEAF_NODE, 4);  /* Flags */
> > > +    build_append_int_noprefix(tbl, parent , 4); /* parent */
> > 
> > 'parent' not capitalized. We want these comments to exactly match the text
> > in the spec.
> 
> Will fix.
> 
> > 
> > > +    build_append_int_noprefix(tbl, id, 4);      /* ACPI processor ID */
> > > +    build_append_int_noprefix(tbl, 0, 4);       /* Num of private resources */
> > > +}
> > > +
> > >   /* 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/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > index cf9f44299c..45e10d886f 100644
> > > --- a/include/hw/acpi/acpi-defs.h
> > > +++ b/include/hw/acpi/acpi-defs.h
> > > @@ -618,4 +618,17 @@ struct AcpiIortRC {
> > >   } QEMU_PACKED;
> > >   typedef struct AcpiIortRC AcpiIortRC;
> > > +enum {
> > > +    ACPI_PPTT_TYPE_PROCESSOR = 0,
> > > +    ACPI_PPTT_TYPE_CACHE,
> > > +    ACPI_PPTT_TYPE_ID,
> > > +    ACPI_PPTT_TYPE_RESERVED
> > > +};
> > > +
> > > +#define ACPI_PPTT_PHYSICAL_PACKAGE          (1)
> > > +#define ACPI_PPTT_ACPI_PROCESSOR_ID_VALID   (1 << 1)
> > > +#define ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD  (1 << 2)      /* ACPI 6.3 */
> > > +#define ACPI_PPTT_ACPI_LEAF_NODE            (1 << 3)      /* ACPI 6.3 */
> > > +#define ACPI_PPTT_ACPI_IDENTICAL            (1 << 4)      /* ACPI 6.3 */
> > > +
> > >   #endif
> > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > > index 380d3e3924..7f0ca1a198 100644
> > > --- a/include/hw/acpi/aml-build.h
> > > +++ b/include/hw/acpi/aml-build.h
> > > @@ -462,6 +462,13 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
> > >   void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> > >                   const char *oem_id, const char *oem_table_id);
> > > +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
> > > +
> > > +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
> > > +                               uint32_t parent, uint32_t id);
> > > +
> > > +void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
> > 
> > Why does build_processor_hierarchy() take a flags argument, but the
> > others don't? Why not just have a single 'flags' taking function,
> > like [*] that works for all of them? I think that answer to that is
> 
> Yes, you are right.
> 
> > that when cache topology support is added it's better to break these
> > into separate functions, but should we do that now? It seems odd to
> > be introducing unused defines and this API before it's necessary.
> So it is better for us to keep just one common build_processor_hierarchy
> API here in your opinion.

Well, a consistent API without unused defines. Whether or not that's
a single common function or not isn't that important.

Thanks,
drew

> 
> > 
> > [*] https://github.com/rhdrjones/qemu/commit/439b38d67ca1f2cbfa5b9892a822b651ebd05c11
> > 
> > Thanks,
> > drew
> > 
> > > +
> > >   void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
> > >                   const char *oem_id, const char *oem_table_id);
> > > -- 
> > > 2.23.0
> > > 
> > > 
> > 
> > .
> > 
> 
> Thanks,
> Ying.
> 

Re: [RFC PATCH 0/5] hw/arm/virt: Introduce cpu topology support

[Andrew Jones]

On Fri, Feb 26, 2021 at 04:41:45PM +0800, Ying Fang wrote:
> 
> 
> On 2/25/2021 8:02 PM, Andrew Jones wrote:
> > On Thu, Feb 25, 2021 at 04:56:22PM +0800, Ying Fang wrote:
> > > An accurate cpu topology may help improve the cpu scheduler's decision
> > > making when dealing with multi-core system. So cpu topology description
> > > is helpful to provide guest with the right view. Dario Faggioli's talk
> > > in [0] also shows the virtual topology may has impact on sched performace.
> > > Thus this patch series is posted to introduce cpu topology support for
> > > arm platform.
> > > 
> > > Both fdt and ACPI are introduced to present the cpu topology. To describe
> > > the cpu topology via ACPI, a PPTT table is introduced according to the
> > > processor hierarchy node structure. This series is derived from [1], in
> > > [1] we are trying to bring both cpu and cache topology support for arm
> > > platform, but there is still some issues to solve to support the cache
> > > hierarchy. So we split the cpu topology part out and send it seperately.
> > > The patch series to support cache hierarchy will be send later since
> > > Salil Mehta's cpu hotplug feature need the cpu topology enabled first and
> > > he is waiting for it to be upstreamed.
> > > 
> > > This patch series was initially based on the patches posted by Andrew Jones [2].
> > > I jumped in on it since some OS vendor cooperative partner are eager for it.
> > > Thanks for Andrew's contribution.
> > > 
> > > After applying this patch series, launch a guest with virt-6.0 and cpu
> > > topology configured with sockets:cores:threads = 2:4:2, you will get the
> > > bellow messages with the lscpu command.
> > > 
> > > -----------------------------------------
> > > Architecture:                    aarch64
> > > CPU op-mode(s):                  64-bit
> > > Byte Order:                      Little Endian
> > > CPU(s):                          16
> > > On-line CPU(s) list:             0-15
> > > Thread(s) per core:              2
> > 
> > What CPU model was used? Did it actually support threads? If these were
> 
> It's tested on Huawei Kunpeng 920 CPU model and vcpu host-passthrough.
> It does not support threads for now, but the next version 930 may
> support it. Here we emulate a virtual cpu topology, a virtual 2 threads
> is used to do the test.
> 
> 
> > KVM VCPUs, then I guess MPIDR.MT was not set on the CPUs. Apparently
> > that didn't confuse Linux? See [1] for how I once tried to deal with
> > threads.
> > 
> > [1] https://github.com/rhdrjones/qemu/commit/60218e0dd7b331031b644872d56f2aca42d0ff1e
> > 
> 
> If ACPI PPTT table is specified, the linux kernel won't check the MPIDR
> register to populate cpu topology. Moreover MPIDR does not ensure a
> right cpu topology. So it won't be a problem if MPIDR.MT is not set.

OK, so Linux doesn't care about MPIDR.MT with ACPI. What happens with
DT?

> 
> > > Core(s) per socket:              4
> > > Socket(s):                       2
> > 
> > Good, but what happens if you specify '-smp 16'? Do you get 16 sockets
              ^^ You didn't answer this question.

> > each with 1 core? Or, do you get 1 socket with 16 cores? And, which do
> > we want and why? If you look at [2], then you'll see I was assuming we
> > want to prefer cores over sockets, since without topology descriptions
> > that's what the Linux guest kernel would do.
> > 
> > [2] https://github.com/rhdrjones/qemu/commit/c0670b1bccb4d08c7cf7c6957cc8878a2af131dd
> > 
> > > NUMA node(s):                    2
> > 
> > Why do we have two NUMA nodes in the guest? The two sockets in the
> > guest should not imply this.
> 
> The two NUMA nodes are emulated by Qemu since we already have guest numa
> topology feature.

That's what I suspected, and I presume only a single node is present when
you don't use QEMU's NUMA feature - even when you supply a VCPU topology
with multiple sockets?

Thanks,
drew

> So the two sockets in the guest has nothing to do with
> it. Actually even one socket may have two numa nodes in it in real cpu
> model.
> 
> > 
> > Thanks,
> > drew
> > 
> > > Vendor ID:                       HiSilicon
> > > Model:                           0
> > > Model name:                      Kunpeng-920
> > > Stepping:                        0x1
> > > BogoMIPS:                        200.00
> > > NUMA node0 CPU(s):               0-7
> > > NUMA node1 CPU(s):               8-15
> > > 
> > > [0] https://kvmforum2020.sched.com/event/eE1y/virtual-topology-for-virtual-machines-friend-or-foe-dario-faggioli-suse
> > > [1] https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg02166.html
> > > [2] https://patchwork.ozlabs.org/project/qemu-devel/cover/20180704124923.32483-1-drjones@redhat.com
> > > 
> > > Ying Fang (5):
> > >    device_tree: Add qemu_fdt_add_path
> > >    hw/arm/virt: Add cpu-map to device tree
> > >    hw/arm/virt-acpi-build: distinguish possible and present cpus
> > >    hw/acpi/aml-build: add processor hierarchy node structure
> > >    hw/arm/virt-acpi-build: add PPTT table
> > > 
> > >   hw/acpi/aml-build.c          | 40 ++++++++++++++++++++++
> > >   hw/arm/virt-acpi-build.c     | 64 +++++++++++++++++++++++++++++++++---
> > >   hw/arm/virt.c                | 40 +++++++++++++++++++++-
> > >   include/hw/acpi/acpi-defs.h  | 13 ++++++++
> > >   include/hw/acpi/aml-build.h  |  7 ++++
> > >   include/hw/arm/virt.h        |  1 +
> > >   include/sysemu/device_tree.h |  1 +
> > >   softmmu/device_tree.c        | 45 +++++++++++++++++++++++--
> > >   8 files changed, 204 insertions(+), 7 deletions(-)
> > > 
> > > -- 
> > > 2.23.0
> > > 
> > 
> > .
> > 
> 

Re: [RFC PATCH 4/5] hw/acpi/aml-build: add processor hierarchy node structure

[Michael S. Tsirkin]

On Mon, Mar 01, 2021 at 10:39:19AM +0100, Andrew Jones wrote:
> On Fri, Feb 26, 2021 at 10:23:03AM +0800, Ying Fang wrote:
> > 
> > 
> > On 2/25/2021 7:47 PM, Andrew Jones wrote:
> > > On Thu, Feb 25, 2021 at 04:56:26PM +0800, Ying Fang wrote:
> > > > Add the processor hierarchy node structures to build ACPI information
> > > > for CPU topology. Since the private resources may be used to describe
> > > > cache hierarchy and it is variable among different topology level,
> > > > three helpers are introduced to describe the hierarchy.
> > > > 
> > > > (1) build_socket_hierarchy for socket description
> > > > (2) build_processor_hierarchy for processor description
> > > > (3) build_smt_hierarchy for thread (logic processor) description
> > > > 
> > > > Signed-off-by: Ying Fang <fangying1@huawei.com>
> > > > Signed-off-by: Henglong Fan <fanhenglong@huawei.com>
> > > > ---
> > > >   hw/acpi/aml-build.c         | 40 +++++++++++++++++++++++++++++++++++++
> > > >   include/hw/acpi/acpi-defs.h | 13 ++++++++++++
> > > >   include/hw/acpi/aml-build.h |  7 +++++++
> > > >   3 files changed, 60 insertions(+)
> > > > 
> > > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > > index a2cd7a5830..a0af3e9d73 100644
> > > > --- a/hw/acpi/aml-build.c
> > > > +++ b/hw/acpi/aml-build.c
> > > > @@ -1888,6 +1888,46 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> > > >                    table_data->len - slit_start, 1, oem_id, oem_table_id);
> > > >   }
> > > > +/*
> > > > + * ACPI 6.3: 5.2.29.1 Processor hierarchy node structure (Type 0)
> > > > + */
> > > > +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
> > > > +{
> > > > +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
> > > > +    build_append_byte(tbl, 20);         /* Length, no private resources */
> > > > +    build_append_int_noprefix(tbl, 0, 2);  /* Reserved */
> > > > +    build_append_int_noprefix(tbl, ACPI_PPTT_PHYSICAL_PACKAGE, 4);
> > > 
> > > Missing '/* Flags */'
> > 
> > Will fix.
> > 
> > > 
> > > > +    build_append_int_noprefix(tbl, parent, 4); /* Parent */
> > > > +    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
> > > > +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
> > > > +}
> > > > +
> > > > +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
> > > > +                               uint32_t parent, uint32_t id)
> > > > +{
> > > > +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR);  /* Type 0 - processor */
> > > > +    build_append_byte(tbl, 20);         /* Length, no private resources */
> > > > +    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 */
> > > > +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
> > > > +}
> > > > +
> > > > +void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
> > > > +{
> > > > +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
> > > > +    build_append_byte(tbl, 20);           /* Length, no private resources */
> > > > +    build_append_int_noprefix(tbl, 0, 2); /* Reserved */
> > > > +    build_append_int_noprefix(tbl,
> > > > +                              ACPI_PPTT_ACPI_PROCESSOR_ID_VALID |
> > > > +                              ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD |
> > > > +                              ACPI_PPTT_ACPI_LEAF_NODE, 4);  /* Flags */
> > > > +    build_append_int_noprefix(tbl, parent , 4); /* parent */
> > > 
> > > 'parent' not capitalized. We want these comments to exactly match the text
> > > in the spec.
> > 
> > Will fix.
> > 
> > > 
> > > > +    build_append_int_noprefix(tbl, id, 4);      /* ACPI processor ID */
> > > > +    build_append_int_noprefix(tbl, 0, 4);       /* Num of private resources */
> > > > +}
> > > > +
> > > >   /* 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/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > > index cf9f44299c..45e10d886f 100644
> > > > --- a/include/hw/acpi/acpi-defs.h
> > > > +++ b/include/hw/acpi/acpi-defs.h
> > > > @@ -618,4 +618,17 @@ struct AcpiIortRC {
> > > >   } QEMU_PACKED;
> > > >   typedef struct AcpiIortRC AcpiIortRC;
> > > > +enum {
> > > > +    ACPI_PPTT_TYPE_PROCESSOR = 0,
> > > > +    ACPI_PPTT_TYPE_CACHE,
> > > > +    ACPI_PPTT_TYPE_ID,
> > > > +    ACPI_PPTT_TYPE_RESERVED
> > > > +};
> > > > +
> > > > +#define ACPI_PPTT_PHYSICAL_PACKAGE          (1)
> > > > +#define ACPI_PPTT_ACPI_PROCESSOR_ID_VALID   (1 << 1)
> > > > +#define ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD  (1 << 2)      /* ACPI 6.3 */
> > > > +#define ACPI_PPTT_ACPI_LEAF_NODE            (1 << 3)      /* ACPI 6.3 */
> > > > +#define ACPI_PPTT_ACPI_IDENTICAL            (1 << 4)      /* ACPI 6.3 */

You need to quote specific place in spec where this appeared, not
just version. and what about previous ones?


> > > > +
> > > >   #endif
> > > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > > > index 380d3e3924..7f0ca1a198 100644
> > > > --- a/include/hw/acpi/aml-build.h
> > > > +++ b/include/hw/acpi/aml-build.h
> > > > @@ -462,6 +462,13 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
> > > >   void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> > > >                   const char *oem_id, const char *oem_table_id);
> > > > +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
> > > > +
> > > > +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
> > > > +                               uint32_t parent, uint32_t id);
> > > > +
> > > > +void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
> > > 
> > > Why does build_processor_hierarchy() take a flags argument, but the
> > > others don't? Why not just have a single 'flags' taking function,
> > > like [*] that works for all of them? I think that answer to that is
> > 
> > Yes, you are right.
> > 
> > > that when cache topology support is added it's better to break these
> > > into separate functions, but should we do that now? It seems odd to
> > > be introducing unused defines and this API before it's necessary.
> > So it is better for us to keep just one common build_processor_hierarchy
> > API here in your opinion.
> 
> Well, a consistent API without unused defines. Whether or not that's
> a single common function or not isn't that important.
> 
> Thanks,
> drew

Yes, the preferred way is code comments:
E.g.

    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR);  /* Type 0 - processor */

should be

    build_append_byte(tbl, 0);  /* Type 0 - processor */


similar:

> > > > +    build_append_int_noprefix(tbl,
> > > > +                              ACPI_PPTT_ACPI_PROCESSOR_ID_VALID |
> > > > +                              ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD |
> > > > +                              ACPI_PPTT_ACPI_LEAF_NODE, 4);  /* Flags */

should be

 +    build_append_int_noprefix(tbl, /* Processor Structure Flags */
 +                              (1 << 1)  /* ACPI Processor ID valid */|
 +                              (1 << 2) /* Processor is a Thread */) |
 +                              (1 << 3) /* Node is a Leaf */, 4);

where you would make sure the text matches the spec verbatim.

also note how for multi-line code comments precede the code.
For single-line they can come after the code.

> > 
> > > 
> > > [*] https://github.com/rhdrjones/qemu/commit/439b38d67ca1f2cbfa5b9892a822b651ebd05c11
> > > 
> > > Thanks,
> > > drew
> > > 
> > > > +
> > > >   void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
> > > >                   const char *oem_id, const char *oem_table_id);
> > > > -- 
> > > > 2.23.0
> > > > 
> > > > 
> > > 
> > > .
> > > 
> > 
> > Thanks,
> > Ying.
> > 

Re: [RFC PATCH 4/5] hw/acpi/aml-build: add processor hierarchy node structure

[Ying Fang]

On 3/1/2021 11:50 PM, Michael S. Tsirkin wrote:
> On Mon, Mar 01, 2021 at 10:39:19AM +0100, Andrew Jones wrote:
>> On Fri, Feb 26, 2021 at 10:23:03AM +0800, Ying Fang wrote:
>>>
>>>
>>> On 2/25/2021 7:47 PM, Andrew Jones wrote:
>>>> On Thu, Feb 25, 2021 at 04:56:26PM +0800, Ying Fang wrote:
>>>>> Add the processor hierarchy node structures to build ACPI information
>>>>> for CPU topology. Since the private resources may be used to describe
>>>>> cache hierarchy and it is variable among different topology level,
>>>>> three helpers are introduced to describe the hierarchy.
>>>>>
>>>>> (1) build_socket_hierarchy for socket description
>>>>> (2) build_processor_hierarchy for processor description
>>>>> (3) build_smt_hierarchy for thread (logic processor) description
>>>>>
>>>>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>>>>> Signed-off-by: Henglong Fan <fanhenglong@huawei.com>
>>>>> ---
>>>>>    hw/acpi/aml-build.c         | 40 +++++++++++++++++++++++++++++++++++++
>>>>>    include/hw/acpi/acpi-defs.h | 13 ++++++++++++
>>>>>    include/hw/acpi/aml-build.h |  7 +++++++
>>>>>    3 files changed, 60 insertions(+)
>>>>>
>>>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>>>> index a2cd7a5830..a0af3e9d73 100644
>>>>> --- a/hw/acpi/aml-build.c
>>>>> +++ b/hw/acpi/aml-build.c
>>>>> @@ -1888,6 +1888,46 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>>>>>                     table_data->len - slit_start, 1, oem_id, oem_table_id);
>>>>>    }
>>>>> +/*
>>>>> + * ACPI 6.3: 5.2.29.1 Processor hierarchy node structure (Type 0)
>>>>> + */
>>>>> +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
>>>>> +{
>>>>> +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
>>>>> +    build_append_byte(tbl, 20);         /* Length, no private resources */
>>>>> +    build_append_int_noprefix(tbl, 0, 2);  /* Reserved */
>>>>> +    build_append_int_noprefix(tbl, ACPI_PPTT_PHYSICAL_PACKAGE, 4);
>>>>
>>>> Missing '/* Flags */'
>>>
>>> Will fix.
>>>
>>>>
>>>>> +    build_append_int_noprefix(tbl, parent, 4); /* Parent */
>>>>> +    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
>>>>> +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
>>>>> +}
>>>>> +
>>>>> +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
>>>>> +                               uint32_t parent, uint32_t id)
>>>>> +{
>>>>> +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR);  /* Type 0 - processor */
>>>>> +    build_append_byte(tbl, 20);         /* Length, no private resources */
>>>>> +    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 */
>>>>> +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
>>>>> +}
>>>>> +
>>>>> +void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
>>>>> +{
>>>>> +    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
>>>>> +    build_append_byte(tbl, 20);           /* Length, no private resources */
>>>>> +    build_append_int_noprefix(tbl, 0, 2); /* Reserved */
>>>>> +    build_append_int_noprefix(tbl,
>>>>> +                              ACPI_PPTT_ACPI_PROCESSOR_ID_VALID |
>>>>> +                              ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD |
>>>>> +                              ACPI_PPTT_ACPI_LEAF_NODE, 4);  /* Flags */
>>>>> +    build_append_int_noprefix(tbl, parent , 4); /* parent */
>>>>
>>>> 'parent' not capitalized. We want these comments to exactly match the text
>>>> in the spec.
>>>
>>> Will fix.
>>>
>>>>
>>>>> +    build_append_int_noprefix(tbl, id, 4);      /* ACPI processor ID */
>>>>> +    build_append_int_noprefix(tbl, 0, 4);       /* Num of private resources */
>>>>> +}
>>>>> +
>>>>>    /* 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/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>>>>> index cf9f44299c..45e10d886f 100644
>>>>> --- a/include/hw/acpi/acpi-defs.h
>>>>> +++ b/include/hw/acpi/acpi-defs.h
>>>>> @@ -618,4 +618,17 @@ struct AcpiIortRC {
>>>>>    } QEMU_PACKED;
>>>>>    typedef struct AcpiIortRC AcpiIortRC;
>>>>> +enum {
>>>>> +    ACPI_PPTT_TYPE_PROCESSOR = 0,
>>>>> +    ACPI_PPTT_TYPE_CACHE,
>>>>> +    ACPI_PPTT_TYPE_ID,
>>>>> +    ACPI_PPTT_TYPE_RESERVED
>>>>> +};
>>>>> +
>>>>> +#define ACPI_PPTT_PHYSICAL_PACKAGE          (1)
>>>>> +#define ACPI_PPTT_ACPI_PROCESSOR_ID_VALID   (1 << 1)
>>>>> +#define ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD  (1 << 2)      /* ACPI 6.3 */
>>>>> +#define ACPI_PPTT_ACPI_LEAF_NODE            (1 << 3)      /* ACPI 6.3 */
>>>>> +#define ACPI_PPTT_ACPI_IDENTICAL            (1 << 4)      /* ACPI 6.3 */
> 
> You need to quote specific place in spec where this appeared, not
> just version. and what about previous ones?

Thanks, Will fix.

> 
> 
>>>>> +
>>>>>    #endif
>>>>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>>>>> index 380d3e3924..7f0ca1a198 100644
>>>>> --- a/include/hw/acpi/aml-build.h
>>>>> +++ b/include/hw/acpi/aml-build.h
>>>>> @@ -462,6 +462,13 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>>>>>    void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>>>>>                    const char *oem_id, const char *oem_table_id);
>>>>> +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
>>>>> +
>>>>> +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
>>>>> +                               uint32_t parent, uint32_t id);
>>>>> +
>>>>> +void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
>>>>
>>>> Why does build_processor_hierarchy() take a flags argument, but the
>>>> others don't? Why not just have a single 'flags' taking function,
>>>> like [*] that works for all of them? I think that answer to that is
>>>
>>> Yes, you are right.
>>>
>>>> that when cache topology support is added it's better to break these
>>>> into separate functions, but should we do that now? It seems odd to
>>>> be introducing unused defines and this API before it's necessary.
>>> So it is better for us to keep just one common build_processor_hierarchy
>>> API here in your opinion.
>>
>> Well, a consistent API without unused defines. Whether or not that's
>> a single common function or not isn't that important.
>>
>> Thanks,
>> drew
> 
> Yes, the preferred way is code comments:
> E.g.
> 
>      build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR);  /* Type 0 - processor */
> 
> should be
> 
>      build_append_byte(tbl, 0);  /* Type 0 - processor */
> 
> 
> similar:
> 
>>>>> +    build_append_int_noprefix(tbl,
>>>>> +                              ACPI_PPTT_ACPI_PROCESSOR_ID_VALID |
>>>>> +                              ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD |
>>>>> +                              ACPI_PPTT_ACPI_LEAF_NODE, 4);  /* Flags */
> 
> should be
> 
>   +    build_append_int_noprefix(tbl, /* Processor Structure Flags */
>   +                              (1 << 1)  /* ACPI Processor ID valid */|
>   +                              (1 << 2) /* Processor is a Thread */) |
>   +                              (1 << 3) /* Node is a Leaf */, 4);
> 
> where you would make sure the text matches the spec verbatim.
> 
> also note how for multi-line code comments precede the code.
> For single-line they can come after the code.

Thanks, will fix it as your suggestions.

> 
>>>
>>>>
>>>> [*] https://github.com/rhdrjones/qemu/commit/439b38d67ca1f2cbfa5b9892a822b651ebd05c11
>>>>
>>>> Thanks,
>>>> drew
>>>>
>>>>> +
>>>>>    void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>>>>>                    const char *oem_id, const char *oem_table_id);
>>>>> -- 
>>>>> 2.23.0
>>>>>
>>>>>
>>>>
>>>> .
>>>>
>>>
>>> Thanks,
>>> Ying.
>>>
> 
> .
> 

Re: [RFC PATCH 0/5] hw/arm/virt: Introduce cpu topology support

[Ying Fang]

On 3/1/2021 5:48 PM, Andrew Jones wrote:
> On Fri, Feb 26, 2021 at 04:41:45PM +0800, Ying Fang wrote:
>>
>>
>> On 2/25/2021 8:02 PM, Andrew Jones wrote:
>>> On Thu, Feb 25, 2021 at 04:56:22PM +0800, Ying Fang wrote:
>>>> An accurate cpu topology may help improve the cpu scheduler's decision
>>>> making when dealing with multi-core system. So cpu topology description
>>>> is helpful to provide guest with the right view. Dario Faggioli's talk
>>>> in [0] also shows the virtual topology may has impact on sched performace.
>>>> Thus this patch series is posted to introduce cpu topology support for
>>>> arm platform.
>>>>
>>>> Both fdt and ACPI are introduced to present the cpu topology. To describe
>>>> the cpu topology via ACPI, a PPTT table is introduced according to the
>>>> processor hierarchy node structure. This series is derived from [1], in
>>>> [1] we are trying to bring both cpu and cache topology support for arm
>>>> platform, but there is still some issues to solve to support the cache
>>>> hierarchy. So we split the cpu topology part out and send it seperately.
>>>> The patch series to support cache hierarchy will be send later since
>>>> Salil Mehta's cpu hotplug feature need the cpu topology enabled first and
>>>> he is waiting for it to be upstreamed.
>>>>
>>>> This patch series was initially based on the patches posted by Andrew Jones [2].
>>>> I jumped in on it since some OS vendor cooperative partner are eager for it.
>>>> Thanks for Andrew's contribution.
>>>>
>>>> After applying this patch series, launch a guest with virt-6.0 and cpu
>>>> topology configured with sockets:cores:threads = 2:4:2, you will get the
>>>> bellow messages with the lscpu command.
>>>>
>>>> -----------------------------------------
>>>> Architecture:                    aarch64
>>>> CPU op-mode(s):                  64-bit
>>>> Byte Order:                      Little Endian
>>>> CPU(s):                          16
>>>> On-line CPU(s) list:             0-15
>>>> Thread(s) per core:              2
>>>
>>> What CPU model was used? Did it actually support threads? If these were
>>
>> It's tested on Huawei Kunpeng 920 CPU model and vcpu host-passthrough.
>> It does not support threads for now, but the next version 930 may
>> support it. Here we emulate a virtual cpu topology, a virtual 2 threads
>> is used to do the test.
>>
>>
>>> KVM VCPUs, then I guess MPIDR.MT was not set on the CPUs. Apparently
>>> that didn't confuse Linux? See [1] for how I once tried to deal with
>>> threads.
>>>
>>> [1] https://github.com/rhdrjones/qemu/commit/60218e0dd7b331031b644872d56f2aca42d0ff1e
>>>
>>
>> If ACPI PPTT table is specified, the linux kernel won't check the MPIDR
>> register to populate cpu topology. Moreover MPIDR does not ensure a
>> right cpu topology. So it won't be a problem if MPIDR.MT is not set.
> 
> OK, so Linux doesn't care about MPIDR.MT with ACPI. What happens with
> DT?

Behind the logical of Linux kernel, it tries to parse cpu topology in
smp_prepare_cpus (arch/arm64/kernel/topology.c). If cpu topology is
provided via DT, Linux kernel won't check MPIDR any more. This is the
same with ACPI enabled.

> 
>>
>>>> Core(s) per socket:              4
>>>> Socket(s):                       2
>>>
>>> Good, but what happens if you specify '-smp 16'? Do you get 16 sockets
>                ^^ You didn't answer this question.

The latest qemu use smp_parse the parse -smp command line, by default if
-smp 16 is given, arm64 virt machine will get 16 sockets.

> 
>>> each with 1 core? Or, do you get 1 socket with 16 cores? And, which do
>>> we want and why? If you look at [2], then you'll see I was assuming we
>>> want to prefer cores over sockets, since without topology descriptions
>>> that's what the Linux guest kernel would do.
>>>
>>> [2] https://github.com/rhdrjones/qemu/commit/c0670b1bccb4d08c7cf7c6957cc8878a2af131dd
>>>

Thanks, I'll check the default way Linux does.

>>>> NUMA node(s):                    2
>>>
>>> Why do we have two NUMA nodes in the guest? The two sockets in the
>>> guest should not imply this.
>>
>> The two NUMA nodes are emulated by Qemu since we already have guest numa
>> topology feature.
> 
> That's what I suspected, and I presume only a single node is present when
> you don't use QEMU's NUMA feature - even when you supply a VCPU topology
> with multiple sockets?

Agreed, I would like single numa node too if we do not use guest
numa feature. Here I provide the guest with two numa nodes and set the 
cpu affinity only to do a test.


> 
> Thanks,
> drew
> 
>> So the two sockets in the guest has nothing to do with
>> it. Actually even one socket may have two numa nodes in it in real cpu
>> model.
>>
>>>
>>> Thanks,
>>> drew
>>>
>>>> Vendor ID:                       HiSilicon
>>>> Model:                           0
>>>> Model name:                      Kunpeng-920
>>>> Stepping:                        0x1
>>>> BogoMIPS:                        200.00
>>>> NUMA node0 CPU(s):               0-7
>>>> NUMA node1 CPU(s):               8-15
>>>>
>>>> [0] https://kvmforum2020.sched.com/event/eE1y/virtual-topology-for-virtual-machines-friend-or-foe-dario-faggioli-suse
>>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg02166.html
>>>> [2] https://patchwork.ozlabs.org/project/qemu-devel/cover/20180704124923.32483-1-drjones@redhat.com
>>>>
>>>> Ying Fang (5):
>>>>     device_tree: Add qemu_fdt_add_path
>>>>     hw/arm/virt: Add cpu-map to device tree
>>>>     hw/arm/virt-acpi-build: distinguish possible and present cpus
>>>>     hw/acpi/aml-build: add processor hierarchy node structure
>>>>     hw/arm/virt-acpi-build: add PPTT table
>>>>
>>>>    hw/acpi/aml-build.c          | 40 ++++++++++++++++++++++
>>>>    hw/arm/virt-acpi-build.c     | 64 +++++++++++++++++++++++++++++++++---
>>>>    hw/arm/virt.c                | 40 +++++++++++++++++++++-
>>>>    include/hw/acpi/acpi-defs.h  | 13 ++++++++
>>>>    include/hw/acpi/aml-build.h  |  7 ++++
>>>>    include/hw/arm/virt.h        |  1 +
>>>>    include/sysemu/device_tree.h |  1 +
>>>>    softmmu/device_tree.c        | 45 +++++++++++++++++++++++--
>>>>    8 files changed, 204 insertions(+), 7 deletions(-)
>>>>
>>>> -- 
>>>> 2.23.0
>>>>
>>>
>>> .
>>>
>>
> 
> .
> 

答复: [RFC PATCH 0/5] hw/arm/virt: Introduce cpu topology support

[fangying]

-----邮件原件-----
发件人: Andrew Jones [mailto:drjones@redhat.com] 
发送时间: 2021年3月10日 17:21
收件人: fangying <fangying1@huawei.com>
主题: Re: [RFC PATCH 0/5] hw/arm/virt: Introduce cpu topology support


> Hi Ying Fang,
> 
> Do you plan to repost this soon? It'd be great if it got into 6.0.
>
> Thanks,
> drew


Hi Andrew
Thanks for your remind.
Yes, I will repost and update this series soon.
It seems 6.0 will be soft feature frozen at 3.16.
Deadline is close.

On Thu, Feb 25, 2021 at 04:56:22PM +0800, Ying Fang wrote:
> An accurate cpu topology may help improve the cpu scheduler's decision 
> making when dealing with multi-core system. So cpu topology 
> description is helpful to provide guest with the right view. Dario 
> Faggioli's talk in [0] also shows the virtual topology may has impact on sched performace.
> Thus this patch series is posted to introduce cpu topology support for 
> arm platform.
> 
> Both fdt and ACPI are introduced to present the cpu topology. To 
> describe the cpu topology via ACPI, a PPTT table is introduced 
> according to the processor hierarchy node structure. This series is 
> derived from [1], in [1] we are trying to bring both cpu and cache 
> topology support for arm platform, but there is still some issues to 
> solve to support the cache hierarchy. So we split the cpu topology part out and send it seperately.
> The patch series to support cache hierarchy will be send later since 
> Salil Mehta's cpu hotplug feature need the cpu topology enabled first 
> and he is waiting for it to be upstreamed.
> 
> This patch series was initially based on the patches posted by Andrew Jones [2].
> I jumped in on it since some OS vendor cooperative partner are eager for it.
> Thanks for Andrew's contribution.
> 
> After applying this patch series, launch a guest with virt-6.0 and cpu 
> topology configured with sockets:cores:threads = 2:4:2, you will get 
> the bellow messages with the lscpu command.
> 
> -----------------------------------------
> Architecture:                    aarch64
> CPU op-mode(s):                  64-bit
> Byte Order:                      Little Endian
> CPU(s):                          16
> On-line CPU(s) list:             0-15
> Thread(s) per core:              2
> Core(s) per socket:              4
> Socket(s):                       2
> NUMA node(s):                    2
> Vendor ID:                       HiSilicon
> Model:                           0
> Model name:                      Kunpeng-920
> Stepping:                        0x1
> BogoMIPS:                        200.00
> NUMA node0 CPU(s):               0-7
> NUMA node1 CPU(s):               8-15
> 
> [0] 
> https://kvmforum2020.sched.com/event/eE1y/virtual-topology-for-virtual
> -machines-friend-or-foe-dario-faggioli-suse
> [1] 
> https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg02166.html
> [2] 
> https://patchwork.ozlabs.org/project/qemu-devel/cover/20180704124923.3
> 2483-1-drjones@redhat.com
> 
> Ying Fang (5):
>   device_tree: Add qemu_fdt_add_path
>   hw/arm/virt: Add cpu-map to device tree
>   hw/arm/virt-acpi-build: distinguish possible and present cpus
>   hw/acpi/aml-build: add processor hierarchy node structure
>   hw/arm/virt-acpi-build: add PPTT table
> 
>  hw/acpi/aml-build.c          | 40 ++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c     | 64 +++++++++++++++++++++++++++++++++---
>  hw/arm/virt.c                | 40 +++++++++++++++++++++-
>  include/hw/acpi/acpi-defs.h  | 13 ++++++++  
> include/hw/acpi/aml-build.h  |  7 ++++
>  include/hw/arm/virt.h        |  1 +
>  include/sysemu/device_tree.h |  1 +
>  softmmu/device_tree.c        | 45 +++++++++++++++++++++++--
>  8 files changed, 204 insertions(+), 7 deletions(-)
> 
> --
> 2.23.0
>