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

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

[Yanan Wang]

Hi,

This is v3 of the series [1] that I posted to introduce support of
generating cpu topology descriptions to guest.

Description:
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 [2] also shows the virtual topology could have impact on scheduling
performace. Thus this patch series introduces cpu topology support for
ARM platform.

In this series, both cpu-map in DT and ACPI PPTT table are introduced
to present cpu topology to the guest. And a new helper virt_smp_parse
not like the default one is introduced, which has more strict parsing
rules for the -smp command line.

[1] https://patchwork.kernel.org/project/qemu-devel/cover/20210413080745.33004-1-wangyanan55@huawei.com/
[2] https://kvmforum2020.sched.com/event/eE1y/virtual-topology-for-virtual-machines-friend-or-foe-dario-faggioli-suse

Test results about exposure of topology:
After applying this patch series, launch a guest with virt-6.1.

Cmdline: -smp 96, sockets=2, cores=48, threads=1
Output:
linux-atxcNc:~ # lscpu
Architecture:        aarch64
Byte Order:          Little Endian
CPU(s):              96
On-line CPU(s) list: 0-95
Thread(s) per core:  1
Core(s) per socket:  48
Socket(s):           2
NUMA node(s):        1
Vendor ID:           0x48

Cmdline: -smp 96
linux-atxcNc:~ # lscpu
Architecture:        aarch64
Byte Order:          Little Endian
CPU(s):              96
On-line CPU(s) list: 0-95
Thread(s) per core:  1
Core(s) per socket:  96
Socket(s):           1
NUMA node(s):        1
Vendor ID:           0x48

THINGS TO DO SOON:
1) Run some benchmark to test the scheduling improvement of guest kernel
   introduced by virtual cpu topology.
2) Add some QEMU tests about ARM vcpu topology, ACPI PPTT table, and DT
   cpu nodes. Will post in a separate patchset later.

---

Changelogs:
v2->v3:
- address comments from David, Philippe, and Andrew. Thanks!
- split some change into separate commits for ease of review
- adjust parsing rules of virt_smp_parse to be more strict
  (after discussion with Andrew)
- adjust author credit for the patches
- v2: https://patchwork.kernel.org/project/qemu-devel/cover/20210413080745.33004-1-wangyanan55@huawei.com/

v1->v2:
- Address Andrew Jones's comments
- Address Michael S. Tsirkin's comments
- https://patchwork.kernel.org/project/qemu-devel/cover/20210225085627.2263-1-fangying1@huawei.com/

---

Andrew Jones (3):
  device_tree: Add qemu_fdt_add_path
  hw/arm/virt: Add cpu-map to device tree
  hw/arm/virt-acpi-build: Generate PPTT table

Yanan Wang (6):
  hw/arm/virt: Disable cpu topology support on older machine types
  hw/arm/virt: Initialize the present cpu members
  hw/arm/virt-acpi-build: Use possible cpus in generation of DSDT
  hw/arm/virt-acpi-build: Use possible cpus in generation of MADT
  hw/acpi/aml-build: Add Processor hierarchy node structure
  hw/arm/virt: Add separate -smp parsing function for ARM machines

 hw/acpi/aml-build.c          |  26 ++++++
 hw/arm/virt-acpi-build.c     |  99 ++++++++++++++++++++---
 hw/arm/virt.c                | 148 ++++++++++++++++++++++++++++++++++-
 include/hw/acpi/aml-build.h  |   4 +
 include/hw/arm/virt.h        |   2 +
 include/sysemu/device_tree.h |   1 +
 qemu-options.hx              |   4 +
 softmmu/device_tree.c        |  44 ++++++++++-
 8 files changed, 316 insertions(+), 12 deletions(-)

-- 
2.19.1

[RFC PATCH v3 1/9] hw/arm/virt: Disable cpu topology support on older machine types

[Yanan Wang]

Add a compat variable "no_cpu_topology" used to determine if
a machine type has enabled support of generating cpu topology
description for the guest. Also, for compatibility we disable
this support on older machine types.

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

So in summary, we only enable support of this feature on the
latest machine type and disable it on the older ones (< 6.1)
to avoid possible problems.

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

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 0a78532018..c07841e3a4 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2761,6 +2761,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(6, 1)
 
 static void virt_machine_6_0_options(MachineClass *mc)
 {
+    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
+
+    virt_machine_6_1_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_6_0, hw_compat_6_0_len);
+    vmc->no_cpu_topology = true;
 }
 DEFINE_VIRT_MACHINE(6, 0)
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 921416f918..f546dd2023 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -129,6 +129,8 @@ struct VirtMachineClass {
     bool no_kvm_steal_time;
     bool acpi_expose_flash;
     bool no_secure_gpio;
+    /* Machines < 6.1 has no support of cpu topology description for guest */
+    bool no_cpu_topology;
 };
 
 struct VirtMachineState {
-- 
2.19.1

[RFC PATCH v3 2/9] device_tree: Add qemu_fdt_add_path

[Yanan Wang]

From: Andrew Jones <drjones@redhat.com>

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

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

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

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

[RFC PATCH v3 3/9] hw/arm/virt: Add cpu-map to device tree

[Yanan Wang]

From: Andrew Jones <drjones@redhat.com>

Support device tree CPU topology descriptions.

In accordance with the Devicetree Specification, the Linux Doc
"arm/cpus.yaml" requires that cpus and cpu nodes in the DT are
present. And we meet the requirement by creating /cpus/cpu@*
nodes for members within ms->smp.cpus.

Correspondingly, we should also create subnodes in cpu-map for
the present cpus, each of which relates to an unique cpu node.

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

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index c07841e3a4..e5dcdebdbc 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -349,10 +349,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
@@ -405,8 +406,46 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
                 ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
         }
 
+        if (!vmc->no_cpu_topology) {
+            qemu_fdt_setprop_cell(ms->fdt, nodename, "phandle",
+                                  qemu_fdt_alloc_phandle(ms->fdt));
+        }
+
         g_free(nodename);
     }
+
+    if (!vmc->no_cpu_topology) {
+        /*
+         * See Linux Documentation/devicetree/bindings/cpu/cpu-topology.txt
+         * In a SMP system, the hierarchy of CPUs is defined through four
+         * entities that are used to describe the layout of physical CPUs
+         * in the system: socket/cluster/core/thread.
+         */
+        qemu_fdt_add_subnode(ms->fdt, "/cpus/cpu-map");
+
+        for (cpu = smp_cpus - 1; cpu >= 0; cpu--) {
+            char *cpu_path = g_strdup_printf("/cpus/cpu@%d", cpu);
+            char *map_path;
+
+            if (ms->smp.threads > 1) {
+                map_path = g_strdup_printf(
+                    "/cpus/cpu-map/%s%d/%s%d/%s%d",
+                    "socket", 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",
+                    "socket", cpu / ms->smp.cores,
+                    "core", cpu % ms->smp.cores);
+            }
+            qemu_fdt_add_path(ms->fdt, map_path);
+            qemu_fdt_setprop_phandle(ms->fdt, map_path, "cpu", cpu_path);
+
+            g_free(map_path);
+            g_free(cpu_path);
+        }
+    }
 }
 
 static void fdt_add_its_gic_node(VirtMachineState *vms)
-- 
2.19.1

[RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members

[Yanan Wang]

We create and initialize a cpuobj for each present cpu in
machvirt_init(). Now we also initialize the cpu member of
structure CPUArchId for each present cpu in the function.

This will be used to determine whether a cpu is present
when generating ACPI tables in later patches.

Co-developed-by: Ying Fang <fangying1@huawei.com>
Signed-off-by: Ying Fang <fangying1@huawei.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/arm/virt.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index e5dcdebdbc..50e324975f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2061,6 +2061,13 @@ static void machvirt_init(MachineState *machine)
         }
 
         qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
+
+        /*
+         * As ARM cpu hotplug is not supported yet, we initialize
+         * the present cpu members here.
+         */
+        machine->possible_cpus->cpus[n].cpu = cpuobj;
+
         object_unref(cpuobj);
     }
     fdt_add_timer_nodes(vms);
-- 
2.19.1

[RFC PATCH v3 5/9] hw/arm/virt-acpi-build: Use possible cpus in generation of DSDT

[Yanan Wang]

When building ACPI tables regarding CPUs we should always build
them for the number of possible CPUs, not the number of present
CPUs. So we create cpu nodes in DSDT for possible cpus and then
ensure only the present CPUs are marked useful.

Co-developed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
Co-developed-by: Ying Fang <fangying1@huawei.com>
Signed-off-by: Ying Fang <fangying1@huawei.com>
Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/arm/virt-acpi-build.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 60fe2e65a7..a2d8e87616 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -59,15 +59,17 @@
 
 #define ACPI_BUILD_TABLE_SIZE             0x20000
 
-static void acpi_dsdt_add_cpus(Aml *scope, VirtMachineState *vms)
+static void acpi_dsdt_add_cpus(Aml *scope, const CPUArchIdList *possible_cpus)
 {
-    MachineState *ms = MACHINE(vms);
     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);
     }
 }
@@ -596,6 +598,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
     Aml *scope, *dsdt;
     MachineState *ms = MACHINE(vms);
+    MachineClass *mc = MACHINE_GET_CLASS(vms);
+    const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(ms);
     const MemMapEntry *memmap = vms->memmap;
     const int *irqmap = vms->irqmap;
 
@@ -609,7 +613,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
      * the RTC ACPI device at all when using UEFI.
      */
     scope = aml_scope("\\_SB");
-    acpi_dsdt_add_cpus(scope, vms);
+    acpi_dsdt_add_cpus(scope, possible_cpus);
     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
     if (vmc->acpi_expose_flash) {
-- 
2.19.1

[RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in generation of MADT

[Yanan Wang]

When building ACPI tables regarding CPUs we should always build
them for the number of possible CPUs, not the number of present
CPUs. So we create gicc nodes in MADT for possible cpus and then
ensure only the present CPUs are marked ENABLED. Furthermore, it
also needed if we are going to support CPU hotplug in the future.

Co-developed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
Co-developed-by: Ying Fang <fangying1@huawei.com>
Signed-off-by: Ying Fang <fangying1@huawei.com>
Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index a2d8e87616..4d64aeb865 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -481,6 +481,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     const int *irqmap = vms->irqmap;
     AcpiMadtGenericDistributor *gicd;
     AcpiMadtGenericMsiFrame *gic_msi;
+    MachineClass *mc = MACHINE_GET_CLASS(vms);
+    const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(MACHINE(vms));
+    bool pmu;
     int i;
 
     acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
@@ -491,11 +494,21 @@ 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));
 
+        /*
+         * PMU should have been either implemented for all CPUs or not,
+         * so we only get information from the first CPU, which could
+         * represent the others.
+         */
+        if (i == 0) {
+            pmu = arm_feature(&armcpu->env, ARM_FEATURE_PMU);
+        }
+        assert(!armcpu || arm_feature(&armcpu->env, ARM_FEATURE_PMU) == pmu);
+
         gicc->type = ACPI_APIC_GENERIC_CPU_INTERFACE;
         gicc->length = sizeof(*gicc);
         if (vms->gic_version == 2) {
@@ -504,11 +517,19 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
             gicc->gicv_base_address = cpu_to_le64(memmap[VIRT_GIC_VCPU].base);
         }
         gicc->cpu_interface_number = cpu_to_le32(i);
-        gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
+        gicc->arm_mpidr = cpu_to_le64(possible_cpus->cpus[i].arch_id);
         gicc->uid = cpu_to_le32(i);
-        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
 
-        if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
+        /*
+         * ACPI spec says that LAPIC entry for non present CPU may be
+         * omitted from MADT or it must be marked as disabled. Here we
+         * choose to also keep the disabled ones in MADT.
+         */
+        if (possible_cpus->cpus[i].cpu != NULL) {
+            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
+        }
+
+        if (pmu) {
             gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
         }
         if (vms->virt) {
-- 
2.19.1

[RFC PATCH v3 7/9] hw/acpi/aml-build: Add Processor hierarchy node structure

[Yanan Wang]

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

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

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

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index d33ce8954a..b2c71ab88a 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1916,6 +1916,32 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
                  table_data->len - slit_start, 1, oem_id, oem_table_id);
 }
 
+/* ACPI 6.2: 5.2.29.1 Processor hierarchy node structure (Type 0) */
+void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
+                                    uint32_t parent, uint32_t id,
+                                    uint32_t *priv_rsrc, uint32_t priv_num)
+{
+    int i;
+
+    build_append_byte(tbl, 0);                 /* Type 0 - processor */
+    build_append_byte(tbl, 20 + priv_num * 4); /* Length */
+    build_append_int_noprefix(tbl, 0, 2);      /* Reserved */
+    build_append_int_noprefix(tbl, flags, 4);  /* Flags */
+    build_append_int_noprefix(tbl, parent, 4); /* Parent */
+    build_append_int_noprefix(tbl, id, 4);     /* ACPI Processor ID */
+
+    /* Number of private resources */
+    build_append_int_noprefix(tbl, priv_num, 4);
+
+    /* Private resources[N] */
+    if (priv_num > 0) {
+        assert(priv_rsrc);
+        for (i = 0; i < priv_num; i++) {
+            build_append_int_noprefix(tbl, priv_rsrc[i], 4);
+        }
+    }
+}
+
 /* build rev1/rev3/rev5.1 FADT */
 void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                 const char *oem_id, const char *oem_table_id)
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 471266d739..ea74b8f6ed 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -462,6 +462,10 @@ 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_processor_hierarchy_node(GArray *tbl, uint32_t flags,
+                                    uint32_t parent, uint32_t id,
+                                    uint32_t *priv_rsrc, uint32_t priv_num);
+
 void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                 const char *oem_id, const char *oem_table_id);
 
-- 
2.19.1

[RFC PATCH v3 8/9] hw/arm/virt-acpi-build: Generate PPTT table

[Yanan Wang]

From: Andrew Jones <drjones@redhat.com>

Add the Processor Properties Topology Table (PPTT) to expose
CPU topology information defined by users to ACPI guests.

Note, a DT-boot Linux guest with a non-flat CPU topology will
see socket and core IDs being sequential integers starting
from zero, which is different from ACPI-boot Linux guest,
e.g. with -smp 4,sockets=2,cores=2,threads=1

a DT boot produces:

 cpu:  0 package_id:  0 core_id:  0
 cpu:  1 package_id:  0 core_id:  1
 cpu:  2 package_id:  1 core_id:  0
 cpu:  3 package_id:  1 core_id:  1

an ACPI boot produces:

 cpu:  0 package_id: 36 core_id:  0
 cpu:  1 package_id: 36 core_id:  1
 cpu:  2 package_id: 96 core_id:  2
 cpu:  3 package_id: 96 core_id:  3

This is due to several reasons:

 1) DT cpu nodes do not have an equivalent field to what the PPTT
    ACPI Processor ID must be, i.e. something equal to the MADT CPU
    UID or equal to the UID of an ACPI processor container. In both
    ACPI cases those are platform dependant IDs assigned by the
    vendor.

 2) While QEMU is the vendor for a guest, if the topology specifies
    SMT (> 1 thread), then, with ACPI, it is impossible to assign a
    core-id the same value as a package-id, thus it is not possible
    to have package-id=0 and core-id=0. This is because package and
    core containers must be in the same ACPI namespace and therefore
    must have unique UIDs.

 3) ACPI processor containers are not mandatorily required for PPTT
    tables to be used and, due to the limitations of which IDs are
    selected described above in (2), they are not helpful for QEMU,
    so we don't build them with this patch. In the absence of them,
    Linux assigns its own unique IDs. The maintainers have chosen not
    to use counters from zero, but rather ACPI table offsets, which
    explains why the numbers are so much larger than with DT.

 4) When there is no SMT (threads=1) the core IDs for ACPI boot guests
    match the logical CPU IDs, because these IDs must be equal to the
    MADT CPU UID (as no processor containers are present), and QEMU
    uses the logical CPU ID for these MADT IDs.

So in summary, with QEMU as vender for guest, we use sequential integers
starting from zero for non-leaf nodes without valid ID flag, so that the
guest will ignore them and use table offsets as the unique IDs. And we
also use logical CPU IDs for leaf nodes to be consistent with MADT.

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

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 4d64aeb865..b03d57745a 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -435,6 +435,57 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
                  vms->oem_table_id);
 }
 
+/* ACPI 6.2: 5.2.29 Processor Properties Topology Table (PPTT) */
+static void build_pptt(GArray *table_data, BIOSLinker *linker,
+                       VirtMachineState *vms)
+{
+    MachineState *ms = MACHINE(vms);
+    int pptt_start = table_data->len;
+    int uid = 0, socket;
+
+    acpi_data_push(table_data, sizeof(AcpiTableHeader));
+
+    for (socket = 0; socket < ms->smp.sockets; socket++) {
+        uint32_t socket_offset = table_data->len - pptt_start;
+        int core;
+
+        build_processor_hierarchy_node(
+            table_data,
+            (1 << 0), /* ACPI 6.2 - Physical package */
+            0, socket, NULL, 0);
+
+        for (core = 0; core < ms->smp.cores; core++) {
+            uint32_t core_offset = table_data->len - pptt_start;
+            int thread;
+
+            if (ms->smp.threads <= 1) {
+                build_processor_hierarchy_node(
+                    table_data,
+                    (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
+                    (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
+                    socket_offset, uid++, NULL, 0);
+            } else {
+                build_processor_hierarchy_node(table_data, 0, socket_offset,
+                                               core, NULL, 0);
+
+                for (thread = 0; thread < ms->smp.threads; thread++) {
+                    build_processor_hierarchy_node(
+                        table_data,
+                        (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
+                        (1 << 2) | /* ACPI 6.3 - Processor is a Thread */
+                        (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
+                        core_offset, uid++, NULL, 0);
+                }
+            }
+        }
+    }
+
+    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)
@@ -719,13 +770,18 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     dsdt = tables_blob->len;
     build_dsdt(tables_blob, tables->linker, vms);
 
-    /* FADT MADT GTDT MCFG SPCR pointed to by RSDT */
+    /* FADT MADT PPTT GTDT MCFG SPCR pointed to by RSDT */
     acpi_add_table(table_offsets, tables_blob);
     build_fadt_rev5(tables_blob, tables->linker, vms, dsdt);
 
     acpi_add_table(table_offsets, tables_blob);
     build_madt(tables_blob, tables->linker, vms);
 
+    if (!vmc->no_cpu_topology) {
+        acpi_add_table(table_offsets, tables_blob);
+        build_pptt(tables_blob, tables->linker, vms);
+    }
+
     acpi_add_table(table_offsets, tables_blob);
     build_gtdt(tables_blob, tables->linker, vms);
 
-- 
2.19.1

[RFC PATCH v3 9/9] hw/arm/virt: Add separate -smp parsing function for ARM machines

[Yanan Wang]

The cpu hierarchy topology information parsed out from QEMU -smp
command line will be exposed to guest kernel through ACPI and DT
since machine type 6.1, so we will expect more detailed topology
descriptions and will be more strict with the -smp cmdlines when
parsing them.

Compared with the default smp_parse() where all missing values
are automatically calculated in turn, there is some difference
in ARM specific virt_smp_parse(). The parsing logic is like:
At least one of cpus or maxcpus must be provided. Threads will
default to 1 if not provided (assume not support SMT). Sockets
and cores must be either both provided or both not.

Note, if neither sockets nor cores are provided, we calculate
all the missing values like smp_parse() did before, but will
disable support of exposing these auto-populated descriptions
to guest. Then guest will populate its topology by default.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/arm/virt.c   | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx |  4 +++
 2 files changed, 99 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 50e324975f..44e990e3be 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -76,6 +76,8 @@
 #include "hw/virtio/virtio-iommu.h"
 #include "hw/char/pl011.h"
 #include "qemu/guest-random.h"
+#include "qapi/qmp/qerror.h"
+#include "sysemu/replay.h"
 
 #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
     static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -2627,6 +2629,98 @@ static int virt_kvm_type(MachineState *ms, const char *type_str)
     return fixed_ipa ? 0 : requested_pa_size;
 }
 
+/*
+ * virt_smp_parse - Used to parse -smp command line for ARM machines.
+ *
+ * Compared with the default smp_parse where all the missing values
+ * are automatically calculated in turn, in this function, we expect
+ * more detailed topology information provided and are more strict
+ * with the -smp cmdlines when parsing them.
+ *
+ * We require that at least one of cpus or maxcpus must be provided.
+ * Threads will default to 1 if not provided. Sockets and cores must
+ * be either both provided or both not.
+ *
+ * Note, if neither sockets nor cores are specified, we will calculate
+ * all the missing values just like smp_parse() does, but will disable
+ * exposure of cpu topology descriptions to guest.
+ */
+static void virt_smp_parse(MachineState *ms, QemuOpts *opts)
+{
+    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(ms);
+
+    if (opts) {
+        unsigned cpus = qemu_opt_get_number(opts, "cpus", 0);
+        unsigned maxcpus = qemu_opt_get_number(opts, "maxcpus", 0);
+        unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
+        unsigned cores = qemu_opt_get_number(opts, "cores", 0);
+        unsigned threads = qemu_opt_get_number(opts, "threads", 0);
+
+        /* Default threads to 1 if not provided */
+        threads = threads > 0 ? threads : 1;
+
+        if (cpus == 0 && maxcpus == 0) {
+            error_report("at least one of cpus or maxcpus must be provided");
+            exit(1);
+        }
+
+        if (sockets == 0 && cores == 0) {
+            /* Disable exposure of topology descriptions to guest */
+            vmc->no_cpu_topology = true;
+
+            /* Compute missing values, prefer sockets over cores */
+            cores = 1;
+            if (cpus == 0) {
+                sockets = 1;
+                cpus = sockets * cores * threads;
+            } else {
+                maxcpus = maxcpus > 0 ? maxcpus : cpus;
+                sockets = maxcpus / (cores * threads);
+            }
+        } else if (sockets > 0 && cores > 0) {
+            cpus = cpus > 0 ? cpus : sockets * cores * threads;
+            maxcpus = maxcpus > 0 ? maxcpus : cpus;
+        } else {
+            error_report("sockets and cores must be both provided "
+                         "or both not");
+            exit(1);
+        }
+
+        if (maxcpus < cpus) {
+            error_report("maxcpus must be equal to or greater than smp");
+            exit(1);
+        }
+
+        if (sockets * cores * threads < cpus) {
+            error_report("cpu topology: "
+                         "sockets (%u) * cores (%u) * threads (%u) < "
+                         "smp_cpus (%u)",
+                         sockets, cores, threads, cpus);
+            exit(1);
+        }
+
+        if (sockets * cores * threads != maxcpus) {
+            error_report("cpu topology: "
+                         "sockets (%u) * cores (%u) * threads (%u) "
+                         "!= maxcpus (%u)",
+                         sockets, cores, threads, maxcpus);
+            exit(1);
+        }
+
+        ms->smp.cpus = cpus;
+        ms->smp.max_cpus = maxcpus;
+        ms->smp.sockets = sockets;
+        ms->smp.cores = cores;
+        ms->smp.threads = threads;
+    }
+
+    if (ms->smp.cpus > 1) {
+        Error *blocker = NULL;
+        error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED, "smp");
+        replay_add_blocker(blocker);
+    }
+}
+
 static void virt_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -2652,6 +2746,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
     mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
+    mc->smp_parse = virt_smp_parse;
     mc->kvm_type = virt_kvm_type;
     assert(!mc->get_hotplug_handler);
     mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
diff --git a/qemu-options.hx b/qemu-options.hx
index 635dc8a624..bd97086c21 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -203,6 +203,10 @@ SRST
     computed. If any on the three values is given, the total number of
     CPUs n can be omitted. maxcpus specifies the maximum number of
     hotpluggable CPUs.
+
+    For the ARM target, at least one of cpus or maxcpus must be provided.
+    Threads will default to 1 if not provided. Sockets and cores must be
+    either both provided or both not.
 ERST
 
 DEF("numa", HAS_ARG, QEMU_OPTION_numa,
-- 
2.19.1

Re: [RFC PATCH v3 2/9] device_tree: Add qemu_fdt_add_path

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3959 bytes --]

On Sun, May 16, 2021 at 06:28:53PM +0800, Yanan Wang wrote:
> From: Andrew Jones <drjones@redhat.com>
> 
> qemu_fdt_add_path() works like qemu_fdt_add_subnode(), except it
> also adds all missing subnodes from the given path. We'll use it
> in a coming patch where we will add cpu-map to the device tree.
> 
> And we also tweak an error message of qemu_fdt_add_subnode().
> 
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Wonder if I should integrate a function like this into libfdt.

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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH v3 2/9] device_tree: Add qemu_fdt_add_path

[Andrew Jones]

On Sun, May 16, 2021 at 06:28:53PM +0800, Yanan Wang wrote:
> From: Andrew Jones <drjones@redhat.com>

Hi Yanan,

This looks good, but the authorship is no longer correct. You've
completely rewritten it, so I think the most I deserve is a
Co-developed-by and maybe even just a Suggested-by. When changing
the authorship and tags, you can also add a

Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks,
drew


> 
> qemu_fdt_add_path() works like qemu_fdt_add_subnode(), except it
> also adds all missing subnodes from the given path. We'll use it
> in a coming patch where we will add cpu-map to the device tree.
> 
> And we also tweak an error message of qemu_fdt_add_subnode().
> 
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  include/sysemu/device_tree.h |  1 +
>  softmmu/device_tree.c        | 44 ++++++++++++++++++++++++++++++++++--
>  2 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index 8a2fe55622..ef060a9759 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -121,6 +121,7 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
>  uint32_t qemu_fdt_alloc_phandle(void *fdt);
>  int qemu_fdt_nop_node(void *fdt, const char *node_path);
>  int qemu_fdt_add_subnode(void *fdt, const char *name);
> +int qemu_fdt_add_path(void *fdt, const char *path);
>  
>  #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
>      do {                                                                      \
> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> index b621f63fba..3965c834ca 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -540,8 +540,8 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
>  
>      retval = fdt_add_subnode(fdt, parent, basename);
>      if (retval < 0) {
> -        error_report("FDT: Failed to create subnode %s: %s", name,
> -                     fdt_strerror(retval));
> +        error_report("%s: Failed to create subnode %s: %s",
> +                     __func__, name, fdt_strerror(retval));
>          exit(1);
>      }
>  
> @@ -549,6 +549,46 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
>      return retval;
>  }
>  
> +/*
> + * qemu_fdt_add_path: Like qemu_fdt_add_subnode(), but will add
> + * all missing subnodes from the given path.
> + */
> +int qemu_fdt_add_path(void *fdt, const char *path)
> +{
> +    const char *name;
> +    const char *p = path;
> +    int namelen, retval;
> +    int parent = 0;
> +
> +    if (path[0] != '/') {
> +        return -1;
> +    }
> +
> +    while (p) {
> +        name = p + 1;
> +        p = strchr(name, '/');
> +        namelen = p != NULL ? p - name : strlen(name);
> +
> +        retval = fdt_subnode_offset_namelen(fdt, parent, name, namelen);
> +        if (retval < 0 && retval != -FDT_ERR_NOTFOUND) {
> +            error_report("%s: Unexpected error in finding subnode %.*s: %s",
> +                         __func__, namelen, name, fdt_strerror(retval));
> +            exit(1);
> +        } else if (retval == -FDT_ERR_NOTFOUND) {
> +            retval = fdt_add_subnode_namelen(fdt, parent, name, namelen);
> +            if (retval < 0) {
> +                error_report("%s: Failed to create subnode %.*s: %s",
> +                             __func__, namelen, name, fdt_strerror(retval));
> +                exit(1);
> +            }
> +        }
> +
> +        parent = retval;
> +    }
> +
> +    return retval;
> +}
> +
>  void qemu_fdt_dumpdtb(void *fdt, int size)
>  {
>      const char *dumpdtb = current_machine->dumpdtb;
> -- 
> 2.19.1
> 

Re: [RFC PATCH v3 3/9] hw/arm/virt: Add cpu-map to device tree

[Andrew Jones]

On Sun, May 16, 2021 at 06:28:54PM +0800, Yanan Wang wrote:
> From: Andrew Jones <drjones@redhat.com>
> 
> Support device tree CPU topology descriptions.
> 
> In accordance with the Devicetree Specification, the Linux Doc
> "arm/cpus.yaml" requires that cpus and cpu nodes in the DT are
> present. And we meet the requirement by creating /cpus/cpu@*
> nodes for members within ms->smp.cpus.
> 
> Correspondingly, we should also create subnodes in cpu-map for
> the present cpus, each of which relates to an unique cpu node.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/arm/virt.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index c07841e3a4..e5dcdebdbc 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -349,10 +349,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

Rather than aligning the top line with the lower lines, we could remove
the extra space from the lower lines. Or, leave the formatting as it was,
by putting 'See' where 'From' was, like I did in my original patch.

>       *  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
> @@ -405,8 +406,46 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
>                  ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
>          }
>  
> +        if (!vmc->no_cpu_topology) {
> +            qemu_fdt_setprop_cell(ms->fdt, nodename, "phandle",
> +                                  qemu_fdt_alloc_phandle(ms->fdt));
> +        }
> +
>          g_free(nodename);
>      }
> +
> +    if (!vmc->no_cpu_topology) {
> +        /*
> +         * See Linux Documentation/devicetree/bindings/cpu/cpu-topology.txt
> +         * In a SMP system, the hierarchy of CPUs is defined through four
> +         * entities that are used to describe the layout of physical CPUs

s/entities/levels/

> +         * in the system: socket/cluster/core/thread.

The comment says there are four levels including 'cluster', but there's no
'cluster' below.

> +         */
> +        qemu_fdt_add_subnode(ms->fdt, "/cpus/cpu-map");
> +
> +        for (cpu = smp_cpus - 1; cpu >= 0; cpu--) {
> +            char *cpu_path = g_strdup_printf("/cpus/cpu@%d", cpu);
> +            char *map_path;
> +
> +            if (ms->smp.threads > 1) {
> +                map_path = g_strdup_printf(
> +                    "/cpus/cpu-map/%s%d/%s%d/%s%d",
> +                    "socket", 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",
> +                    "socket", cpu / ms->smp.cores,
> +                    "core", cpu % ms->smp.cores);
> +            }
> +            qemu_fdt_add_path(ms->fdt, map_path);
> +            qemu_fdt_setprop_phandle(ms->fdt, map_path, "cpu", cpu_path);
> +
> +            g_free(map_path);
> +            g_free(cpu_path);
> +        }
> +    }
>  }
>  
>  static void fdt_add_its_gic_node(VirtMachineState *vms)
> -- 
> 2.19.1
>

Thanks,
drew 

Re: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members

[Andrew Jones]

On Sun, May 16, 2021 at 06:28:55PM +0800, Yanan Wang wrote:
> We create and initialize a cpuobj for each present cpu in
> machvirt_init(). Now we also initialize the cpu member of
> structure CPUArchId for each present cpu in the function.
> 
> This will be used to determine whether a cpu is present
> when generating ACPI tables in later patches.
> 
> Co-developed-by: Ying Fang <fangying1@huawei.com>
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/arm/virt.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index e5dcdebdbc..50e324975f 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2061,6 +2061,13 @@ static void machvirt_init(MachineState *machine)
>          }
>  
>          qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> +
> +        /*
> +         * As ARM cpu hotplug is not supported yet, we initialize
> +         * the present cpu members here.
> +         */
> +        machine->possible_cpus->cpus[n].cpu = cpuobj;
> +
>          object_unref(cpuobj);
>      }
>      fdt_add_timer_nodes(vms);
> -- 
> 2.19.1
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in generation of MADT

[Andrew Jones]

On Sun, May 16, 2021 at 06:28:57PM +0800, Yanan Wang wrote:
> When building ACPI tables regarding CPUs we should always build
> them for the number of possible CPUs, not the number of present
> CPUs. So we create gicc nodes in MADT for possible cpus and then
> ensure only the present CPUs are marked ENABLED. Furthermore, it
> also needed if we are going to support CPU hotplug in the future.
> 
> Co-developed-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Co-developed-by: Ying Fang <fangying1@huawei.com>
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index a2d8e87616..4d64aeb865 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -481,6 +481,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      const int *irqmap = vms->irqmap;
>      AcpiMadtGenericDistributor *gicd;
>      AcpiMadtGenericMsiFrame *gic_msi;
> +    MachineClass *mc = MACHINE_GET_CLASS(vms);
> +    const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(MACHINE(vms));
> +    bool pmu;
>      int i;
>  
>      acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
> @@ -491,11 +494,21 @@ 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));
>  
> +        /*
> +         * PMU should have been either implemented for all CPUs or not,
> +         * so we only get information from the first CPU, which could
> +         * represent the others.
> +         */
> +        if (i == 0) {
> +            pmu = arm_feature(&armcpu->env, ARM_FEATURE_PMU);
> +        }
> +        assert(!armcpu || arm_feature(&armcpu->env, ARM_FEATURE_PMU) == pmu);

This doesn't belong in this patch. The commit message doesn't even mention
it. Also, I don't think we should do this here at all. If we want to
ensure that all cpus have a pmu when one does, then that should be done
somewhere like machvirt_init(), not in ACPI generation code which doesn't
even run for non-ACPI VMs.

> +
>          gicc->type = ACPI_APIC_GENERIC_CPU_INTERFACE;
>          gicc->length = sizeof(*gicc);
>          if (vms->gic_version == 2) {
> @@ -504,11 +517,19 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>              gicc->gicv_base_address = cpu_to_le64(memmap[VIRT_GIC_VCPU].base);
>          }
>          gicc->cpu_interface_number = cpu_to_le32(i);
> -        gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
> +        gicc->arm_mpidr = cpu_to_le64(possible_cpus->cpus[i].arch_id);

Hmm, I think we may have a problem. I don't think there's any guarantee
that possible_cpus->cpus[i].arch_id == armcpu->mp_affinity, because
arch_id comes from virt_cpu_mp_affinity(), which is arm_cpu_mp_affinity,
but with a variable cluster size, however mp_affinity comes from
arm_cpu_mp_affinity with a set cluster size. Also, when KVM is used,
then all bets are off as to what mp_affinity is.

We need to add some code that ensures arch_id == mp_affinity, and, for
now, we should stick with mp_affinity, since, at least when KVM is used,
that's the correct one.

>          gicc->uid = cpu_to_le32(i);
> -        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
>  
> -        if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
> +        /*
> +         * ACPI spec says that LAPIC entry for non present CPU may be

Why are we talking about LAPICs in a GICC generator?

> +         * omitted from MADT or it must be marked as disabled. Here we
> +         * choose to also keep the disabled ones in MADT.
> +         */
> +        if (possible_cpus->cpus[i].cpu != NULL) {
> +            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
> +        }
> +
> +        if (pmu) {
>              gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
>          }
>          if (vms->virt) {
> -- 
> 2.19.1
>

Thanks,
drew 

Re: [RFC PATCH v3 7/9] hw/acpi/aml-build: Add Processor hierarchy node structure

[Andrew Jones]

On Sun, May 16, 2021 at 06:28:58PM +0800, Yanan Wang wrote:
> Add a generic API to build Processor hierarchy node structure (Type 0),
> which is strictly consistent with descriptions in ACPI 6.2: 5.2.29.1.
> 
> This function will be used to build ACPI PPTT table for cpu topology.
> 
> Co-developed-by: Ying Fang <fangying1@huawei.com>
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> Co-developed-by: Henglong Fan <fanhenglong@huawei.com>
> Signed-off-by: Henglong Fan <fanhenglong@huawei.com>
> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/acpi/aml-build.c         | 26 ++++++++++++++++++++++++++
>  include/hw/acpi/aml-build.h |  4 ++++
>  2 files changed, 30 insertions(+)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [RFC PATCH v3 8/9] hw/arm/virt-acpi-build: Generate PPTT table

[Andrew Jones]

On Sun, May 16, 2021 at 06:28:59PM +0800, Yanan Wang wrote:
> From: Andrew Jones <drjones@redhat.com>
> 
> Add the Processor Properties Topology Table (PPTT) to expose
> CPU topology information defined by users to ACPI guests.
> 
> Note, a DT-boot Linux guest with a non-flat CPU topology will
> see socket and core IDs being sequential integers starting
> from zero, which is different from ACPI-boot Linux guest,
> e.g. with -smp 4,sockets=2,cores=2,threads=1
> 
> a DT boot produces:
> 
>  cpu:  0 package_id:  0 core_id:  0
>  cpu:  1 package_id:  0 core_id:  1
>  cpu:  2 package_id:  1 core_id:  0
>  cpu:  3 package_id:  1 core_id:  1
> 
> an ACPI boot produces:
> 
>  cpu:  0 package_id: 36 core_id:  0
>  cpu:  1 package_id: 36 core_id:  1
>  cpu:  2 package_id: 96 core_id:  2
>  cpu:  3 package_id: 96 core_id:  3
> 
> This is due to several reasons:
> 
>  1) DT cpu nodes do not have an equivalent field to what the PPTT
>     ACPI Processor ID must be, i.e. something equal to the MADT CPU
>     UID or equal to the UID of an ACPI processor container. In both
>     ACPI cases those are platform dependant IDs assigned by the
>     vendor.
> 
>  2) While QEMU is the vendor for a guest, if the topology specifies
>     SMT (> 1 thread), then, with ACPI, it is impossible to assign a
>     core-id the same value as a package-id, thus it is not possible
>     to have package-id=0 and core-id=0. This is because package and
>     core containers must be in the same ACPI namespace and therefore
>     must have unique UIDs.
> 
>  3) ACPI processor containers are not mandatorily required for PPTT
>     tables to be used and, due to the limitations of which IDs are
>     selected described above in (2), they are not helpful for QEMU,
>     so we don't build them with this patch. In the absence of them,
>     Linux assigns its own unique IDs. The maintainers have chosen not
>     to use counters from zero, but rather ACPI table offsets, which
>     explains why the numbers are so much larger than with DT.
> 
>  4) When there is no SMT (threads=1) the core IDs for ACPI boot guests
>     match the logical CPU IDs, because these IDs must be equal to the
>     MADT CPU UID (as no processor containers are present), and QEMU
>     uses the logical CPU ID for these MADT IDs.
> 
> So in summary, with QEMU as vender for guest, we use sequential integers
> starting from zero for non-leaf nodes without valid ID flag, so that the
> guest will ignore them and use table offsets as the unique IDs. And we
> also use logical CPU IDs for leaf nodes to be consistent with MADT.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c | 58 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)

Why aren't we adding build_pptt to aml-build.c, like my original patch
does? I don't see anything Arm specific below, at least not if you passed
MachineState instead of VirtMachineState, like my original patch did.

> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 4d64aeb865..b03d57745a 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -435,6 +435,57 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>                   vms->oem_table_id);
>  }
>  
> +/* ACPI 6.2: 5.2.29 Processor Properties Topology Table (PPTT) */
> +static void build_pptt(GArray *table_data, BIOSLinker *linker,
> +                       VirtMachineState *vms)
> +{
> +    MachineState *ms = MACHINE(vms);
> +    int pptt_start = table_data->len;
> +    int uid = 0, socket;
> +
> +    acpi_data_push(table_data, sizeof(AcpiTableHeader));
> +
> +    for (socket = 0; socket < ms->smp.sockets; socket++) {
> +        uint32_t socket_offset = table_data->len - pptt_start;
> +        int core;
> +
> +        build_processor_hierarchy_node(
> +            table_data,
> +            (1 << 0), /* ACPI 6.2 - Physical package */
> +            0, socket, NULL, 0);
> +
> +        for (core = 0; core < ms->smp.cores; core++) {
> +            uint32_t core_offset = table_data->len - pptt_start;
> +            int thread;
> +
> +            if (ms->smp.threads <= 1) {

We can't have threads < 1, so this condition should be == 1.

> +                build_processor_hierarchy_node(
> +                    table_data,
> +                    (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
> +                    (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
> +                    socket_offset, uid++, NULL, 0);
> +            } else {
> +                build_processor_hierarchy_node(table_data, 0, socket_offset,
> +                                               core, NULL, 0);
> +
> +                for (thread = 0; thread < ms->smp.threads; thread++) {
> +                    build_processor_hierarchy_node(
> +                        table_data,
> +                        (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
> +                        (1 << 2) | /* ACPI 6.3 - Processor is a Thread */
> +                        (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
> +                        core_offset, uid++, NULL, 0);
> +                }
> +            }
> +        }
> +    }
> +
> +    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)
> @@ -719,13 +770,18 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>      dsdt = tables_blob->len;
>      build_dsdt(tables_blob, tables->linker, vms);
>  
> -    /* FADT MADT GTDT MCFG SPCR pointed to by RSDT */
> +    /* FADT MADT PPTT GTDT MCFG SPCR pointed to by RSDT */
>      acpi_add_table(table_offsets, tables_blob);
>      build_fadt_rev5(tables_blob, tables->linker, vms, dsdt);
>  
>      acpi_add_table(table_offsets, tables_blob);
>      build_madt(tables_blob, tables->linker, vms);
>  
> +    if (!vmc->no_cpu_topology) {
> +        acpi_add_table(table_offsets, tables_blob);
> +        build_pptt(tables_blob, tables->linker, vms);
> +    }
> +
>      acpi_add_table(table_offsets, tables_blob);
>      build_gtdt(tables_blob, tables->linker, vms);
>  
> -- 
> 2.19.1
>

Thanks,
drew 

Re: [RFC PATCH v3 9/9] hw/arm/virt: Add separate -smp parsing function for ARM machines

[Andrew Jones]

On Sun, May 16, 2021 at 06:29:00PM +0800, Yanan Wang wrote:
> The cpu hierarchy topology information parsed out from QEMU -smp
> command line will be exposed to guest kernel through ACPI and DT
> since machine type 6.1, so we will expect more detailed topology
> descriptions and will be more strict with the -smp cmdlines when
> parsing them.
> 
> Compared with the default smp_parse() where all missing values
> are automatically calculated in turn, there is some difference
> in ARM specific virt_smp_parse(). The parsing logic is like:
> At least one of cpus or maxcpus must be provided. Threads will
> default to 1 if not provided (assume not support SMT). Sockets
> and cores must be either both provided or both not.
> 
> Note, if neither sockets nor cores are provided, we calculate
> all the missing values like smp_parse() did before, but will
> disable support of exposing these auto-populated descriptions
> to guest. Then guest will populate its topology by default.
> 
> Suggested-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/arm/virt.c   | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-options.hx |  4 +++
>  2 files changed, 99 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 50e324975f..44e990e3be 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -76,6 +76,8 @@
>  #include "hw/virtio/virtio-iommu.h"
>  #include "hw/char/pl011.h"
>  #include "qemu/guest-random.h"
> +#include "qapi/qmp/qerror.h"
> +#include "sysemu/replay.h"
>  
>  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> @@ -2627,6 +2629,98 @@ static int virt_kvm_type(MachineState *ms, const char *type_str)
>      return fixed_ipa ? 0 : requested_pa_size;
>  }
>  
> +/*
> + * virt_smp_parse - Used to parse -smp command line for ARM machines.
> + *
> + * Compared with the default smp_parse where all the missing values
> + * are automatically calculated in turn, in this function, we expect
> + * more detailed topology information provided and are more strict
> + * with the -smp cmdlines when parsing them.
> + *
> + * We require that at least one of cpus or maxcpus must be provided.
> + * Threads will default to 1 if not provided. Sockets and cores must
> + * be either both provided or both not.
> + *
> + * Note, if neither sockets nor cores are specified, we will calculate
> + * all the missing values just like smp_parse() does, but will disable
> + * exposure of cpu topology descriptions to guest.
> + */
> +static void virt_smp_parse(MachineState *ms, QemuOpts *opts)
> +{
> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(ms);
> +
> +    if (opts) {
> +        unsigned cpus = qemu_opt_get_number(opts, "cpus", 0);
> +        unsigned maxcpus = qemu_opt_get_number(opts, "maxcpus", 0);
> +        unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
> +        unsigned cores = qemu_opt_get_number(opts, "cores", 0);
> +        unsigned threads = qemu_opt_get_number(opts, "threads", 0);
> +
> +        /* Default threads to 1 if not provided */
> +        threads = threads > 0 ? threads : 1;

Can't do this yet, need to do it later, because...

> +
> +        if (cpus == 0 && maxcpus == 0) {
> +            error_report("at least one of cpus or maxcpus must be provided");
> +            exit(1);
> +        }
> +
> +        if (sockets == 0 && cores == 0) {
> +            /* Disable exposure of topology descriptions to guest */
> +            vmc->no_cpu_topology = true;

...we should do ensure threads == 0 here, otherwise provide another error
message.

> +
> +            /* Compute missing values, prefer sockets over cores */
> +            cores = 1;

Now threads = 1 is good here.

> +            if (cpus == 0) {
> +                sockets = 1;
> +                cpus = sockets * cores * threads;

This should be

  cpus = maxcpus;
  sockets = cpus;

> +            } else {
> +                maxcpus = maxcpus > 0 ? maxcpus : cpus;
> +                sockets = maxcpus / (cores * threads);

We know cores and threads should both be 1 here, so just do

 sockets = maxcpus;

> +            }
> +        } else if (sockets > 0 && cores > 0) {

Now 
        threads = threads > 0 ? threads : 1;

is good here.

> +            cpus = cpus > 0 ? cpus : sockets * cores * threads;
> +            maxcpus = maxcpus > 0 ? maxcpus : cpus;

We should calculate maxcpus first based on the topology,

  maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
  cpus = cpus > 0 ? cpus : maxcpus;

Please do comprehensive testing to ensure everything works as it
should. You can drop this function into a standalone executable
and run it for all possible inputs, maxcpus=0, maxcpus < cpus, maxcpus ==
cpus, maxcpus > cpus, sockets = 0, sockets < cpus, sockets == cpus, etc.

> +        } else {
> +            error_report("sockets and cores must be both provided "
> +                         "or both not");
> +            exit(1);
> +        }
> +
> +        if (maxcpus < cpus) {
> +            error_report("maxcpus must be equal to or greater than smp");
> +            exit(1);
> +        }
> +
> +        if (sockets * cores * threads < cpus) {
> +            error_report("cpu topology: "
> +                         "sockets (%u) * cores (%u) * threads (%u) < "
> +                         "smp_cpus (%u)",
> +                         sockets, cores, threads, cpus);
> +            exit(1);
> +        }
> +
> +        if (sockets * cores * threads != maxcpus) {
> +            error_report("cpu topology: "
> +                         "sockets (%u) * cores (%u) * threads (%u) "
> +                         "!= maxcpus (%u)",
> +                         sockets, cores, threads, maxcpus);
> +            exit(1);
> +        }
> +
> +        ms->smp.cpus = cpus;
> +        ms->smp.max_cpus = maxcpus;
> +        ms->smp.sockets = sockets;
> +        ms->smp.cores = cores;
> +        ms->smp.threads = threads;
> +    }
> +
> +    if (ms->smp.cpus > 1) {
> +        Error *blocker = NULL;
> +        error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED, "smp");
> +        replay_add_blocker(blocker);
> +    }
> +}
> +
>  static void virt_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -2652,6 +2746,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>      mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
>      mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
>      mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
> +    mc->smp_parse = virt_smp_parse;
>      mc->kvm_type = virt_kvm_type;
>      assert(!mc->get_hotplug_handler);
>      mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 635dc8a624..bd97086c21 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -203,6 +203,10 @@ SRST
>      computed. If any on the three values is given, the total number of
>      CPUs n can be omitted. maxcpus specifies the maximum number of
>      hotpluggable CPUs.
> +
> +    For the ARM target, at least one of cpus or maxcpus must be provided.
> +    Threads will default to 1 if not provided. Sockets and cores must be
> +    either both provided or both not.
>  ERST
>  
>  DEF("numa", HAS_ARG, QEMU_OPTION_numa,
> -- 
> 2.19.1
> 

Re: [RFC PATCH v3 2/9] device_tree: Add qemu_fdt_add_path

[wangyanan (Y)]

On 2021/5/17 11:11, David Gibson wrote:
> On Sun, May 16, 2021 at 06:28:53PM +0800, Yanan Wang wrote:
>> From: Andrew Jones<drjones@redhat.com>
>>
>> qemu_fdt_add_path() works like qemu_fdt_add_subnode(), except it
>> also adds all missing subnodes from the given path. We'll use it
>> in a coming patch where we will add cpu-map to the device tree.
>>
>> And we also tweak an error message of qemu_fdt_add_subnode().
>>
>> Cc: David Gibson<david@gibson.dropbear.id.au>
>> Cc: Alistair Francis<alistair.francis@wdc.com>
>> Signed-off-by: Andrew Jones<drjones@redhat.com>
>> Co-developed-by: Yanan Wang<wangyanan55@huawei.com>
>> Signed-off-by: Yanan Wang<wangyanan55@huawei.com>
Hi David,
> Reviewed-by: David Gibson<david@gibson.dropbear.id.au>
Thanks!
> Wonder if I should integrate a function like this into libfdt.

I think it's meaningful to add a function in libfdt that serves as helper
to add subpath but not subnode to a given parent node, like
fdt_add_subpath_namelen(...). This will be useful when we need to
frequently add different paths to a node.

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

Re: [RFC PATCH v3 2/9] device_tree: Add qemu_fdt_add_path

[wangyanan (Y)]

On 2021/5/17 14:27, Andrew Jones wrote:
> On Sun, May 16, 2021 at 06:28:53PM +0800, Yanan Wang wrote:
>> From: Andrew Jones <drjones@redhat.com>
> Hi Yanan,
>
> This looks good, but the authorship is no longer correct. You've
> completely rewritten it, so I think the most I deserve is a
> Co-developed-by and maybe even just a Suggested-by. When changing
> the authorship and tags, you can also add a
Hi Drew,

Thanks for pointing out this, I will correct it.
> Reviewed-by: Andrew Jones <drjones@redhat.com>
Thanks,
Yanan
> Thanks,
> drew
>
>
>> qemu_fdt_add_path() works like qemu_fdt_add_subnode(), except it
>> also adds all missing subnodes from the given path. We'll use it
>> in a coming patch where we will add cpu-map to the device tree.
>>
>> And we also tweak an error message of qemu_fdt_add_subnode().
>>
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> Cc: Alistair Francis <alistair.francis@wdc.com>
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   include/sysemu/device_tree.h |  1 +
>>   softmmu/device_tree.c        | 44 ++++++++++++++++++++++++++++++++++--
>>   2 files changed, 43 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
>> index 8a2fe55622..ef060a9759 100644
>> --- a/include/sysemu/device_tree.h
>> +++ b/include/sysemu/device_tree.h
>> @@ -121,6 +121,7 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
>>   uint32_t qemu_fdt_alloc_phandle(void *fdt);
>>   int qemu_fdt_nop_node(void *fdt, const char *node_path);
>>   int qemu_fdt_add_subnode(void *fdt, const char *name);
>> +int qemu_fdt_add_path(void *fdt, const char *path);
>>   
>>   #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
>>       do {                                                                      \
>> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
>> index b621f63fba..3965c834ca 100644
>> --- a/softmmu/device_tree.c
>> +++ b/softmmu/device_tree.c
>> @@ -540,8 +540,8 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
>>   
>>       retval = fdt_add_subnode(fdt, parent, basename);
>>       if (retval < 0) {
>> -        error_report("FDT: Failed to create subnode %s: %s", name,
>> -                     fdt_strerror(retval));
>> +        error_report("%s: Failed to create subnode %s: %s",
>> +                     __func__, name, fdt_strerror(retval));
>>           exit(1);
>>       }
>>   
>> @@ -549,6 +549,46 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
>>       return retval;
>>   }
>>   
>> +/*
>> + * qemu_fdt_add_path: Like qemu_fdt_add_subnode(), but will add
>> + * all missing subnodes from the given path.
>> + */
>> +int qemu_fdt_add_path(void *fdt, const char *path)
>> +{
>> +    const char *name;
>> +    const char *p = path;
>> +    int namelen, retval;
>> +    int parent = 0;
>> +
>> +    if (path[0] != '/') {
>> +        return -1;
>> +    }
>> +
>> +    while (p) {
>> +        name = p + 1;
>> +        p = strchr(name, '/');
>> +        namelen = p != NULL ? p - name : strlen(name);
>> +
>> +        retval = fdt_subnode_offset_namelen(fdt, parent, name, namelen);
>> +        if (retval < 0 && retval != -FDT_ERR_NOTFOUND) {
>> +            error_report("%s: Unexpected error in finding subnode %.*s: %s",
>> +                         __func__, namelen, name, fdt_strerror(retval));
>> +            exit(1);
>> +        } else if (retval == -FDT_ERR_NOTFOUND) {
>> +            retval = fdt_add_subnode_namelen(fdt, parent, name, namelen);
>> +            if (retval < 0) {
>> +                error_report("%s: Failed to create subnode %.*s: %s",
>> +                             __func__, namelen, name, fdt_strerror(retval));
>> +                exit(1);
>> +            }
>> +        }
>> +
>> +        parent = retval;
>> +    }
>> +
>> +    return retval;
>> +}
>> +
>>   void qemu_fdt_dumpdtb(void *fdt, int size)
>>   {
>>       const char *dumpdtb = current_machine->dumpdtb;
>> -- 
>> 2.19.1
>>
> .

Re: [RFC PATCH v3 8/9] hw/arm/virt-acpi-build: Generate PPTT table

[wangyanan (Y)]

Hi Drew,

On 2021/5/17 16:02, Andrew Jones wrote:
> On Sun, May 16, 2021 at 06:28:59PM +0800, Yanan Wang wrote:
>> From: Andrew Jones <drjones@redhat.com>
>>
>> Add the Processor Properties Topology Table (PPTT) to expose
>> CPU topology information defined by users to ACPI guests.
>>
>> Note, a DT-boot Linux guest with a non-flat CPU topology will
>> see socket and core IDs being sequential integers starting
>> from zero, which is different from ACPI-boot Linux guest,
>> e.g. with -smp 4,sockets=2,cores=2,threads=1
>>
>> a DT boot produces:
>>
>>   cpu:  0 package_id:  0 core_id:  0
>>   cpu:  1 package_id:  0 core_id:  1
>>   cpu:  2 package_id:  1 core_id:  0
>>   cpu:  3 package_id:  1 core_id:  1
>>
>> an ACPI boot produces:
>>
>>   cpu:  0 package_id: 36 core_id:  0
>>   cpu:  1 package_id: 36 core_id:  1
>>   cpu:  2 package_id: 96 core_id:  2
>>   cpu:  3 package_id: 96 core_id:  3
>>
>> This is due to several reasons:
>>
>>   1) DT cpu nodes do not have an equivalent field to what the PPTT
>>      ACPI Processor ID must be, i.e. something equal to the MADT CPU
>>      UID or equal to the UID of an ACPI processor container. In both
>>      ACPI cases those are platform dependant IDs assigned by the
>>      vendor.
>>
>>   2) While QEMU is the vendor for a guest, if the topology specifies
>>      SMT (> 1 thread), then, with ACPI, it is impossible to assign a
>>      core-id the same value as a package-id, thus it is not possible
>>      to have package-id=0 and core-id=0. This is because package and
>>      core containers must be in the same ACPI namespace and therefore
>>      must have unique UIDs.
>>
>>   3) ACPI processor containers are not mandatorily required for PPTT
>>      tables to be used and, due to the limitations of which IDs are
>>      selected described above in (2), they are not helpful for QEMU,
>>      so we don't build them with this patch. In the absence of them,
>>      Linux assigns its own unique IDs. The maintainers have chosen not
>>      to use counters from zero, but rather ACPI table offsets, which
>>      explains why the numbers are so much larger than with DT.
>>
>>   4) When there is no SMT (threads=1) the core IDs for ACPI boot guests
>>      match the logical CPU IDs, because these IDs must be equal to the
>>      MADT CPU UID (as no processor containers are present), and QEMU
>>      uses the logical CPU ID for these MADT IDs.
>>
>> So in summary, with QEMU as vender for guest, we use sequential integers
>> starting from zero for non-leaf nodes without valid ID flag, so that the
>> guest will ignore them and use table offsets as the unique IDs. And we
>> also use logical CPU IDs for leaf nodes to be consistent with MADT.
>>
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   hw/arm/virt-acpi-build.c | 58 +++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 57 insertions(+), 1 deletion(-)
> Why aren't we adding build_pptt to aml-build.c, like my original patch
> does? I don't see anything Arm specific below, at least not if you passed
> MachineState instead of VirtMachineState, like my original patch did.
I agree to move build_pptt to common code, so that other platforms
can also use it if they want. I will do it in next version.

BTW, it seems patch 1 and 5 were possibly missed for some review.
Any comments for them too? Thanks!
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 4d64aeb865..b03d57745a 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -435,6 +435,57 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>                    vms->oem_table_id);
>>   }
>>   
>> +/* ACPI 6.2: 5.2.29 Processor Properties Topology Table (PPTT) */
>> +static void build_pptt(GArray *table_data, BIOSLinker *linker,
>> +                       VirtMachineState *vms)
>> +{
>> +    MachineState *ms = MACHINE(vms);
>> +    int pptt_start = table_data->len;
>> +    int uid = 0, socket;
>> +
>> +    acpi_data_push(table_data, sizeof(AcpiTableHeader));
>> +
>> +    for (socket = 0; socket < ms->smp.sockets; socket++) {
>> +        uint32_t socket_offset = table_data->len - pptt_start;
>> +        int core;
>> +
>> +        build_processor_hierarchy_node(
>> +            table_data,
>> +            (1 << 0), /* ACPI 6.2 - Physical package */
>> +            0, socket, NULL, 0);
>> +
>> +        for (core = 0; core < ms->smp.cores; core++) {
>> +            uint32_t core_offset = table_data->len - pptt_start;
>> +            int thread;
>> +
>> +            if (ms->smp.threads <= 1) {
> We can't have threads < 1, so this condition should be == 1.
Right, I will fix it.

Thanks,
Yanan
>> +                build_processor_hierarchy_node(
>> +                    table_data,
>> +                    (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
>> +                    (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
>> +                    socket_offset, uid++, NULL, 0);
>> +            } else {
>> +                build_processor_hierarchy_node(table_data, 0, socket_offset,
>> +                                               core, NULL, 0);
>> +
>> +                for (thread = 0; thread < ms->smp.threads; thread++) {
>> +                    build_processor_hierarchy_node(
>> +                        table_data,
>> +                        (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */
>> +                        (1 << 2) | /* ACPI 6.3 - Processor is a Thread */
>> +                        (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
>> +                        core_offset, uid++, NULL, 0);
>> +                }
>> +            }
>> +        }
>> +    }
>> +
>> +    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)
>> @@ -719,13 +770,18 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>       dsdt = tables_blob->len;
>>       build_dsdt(tables_blob, tables->linker, vms);
>>   
>> -    /* FADT MADT GTDT MCFG SPCR pointed to by RSDT */
>> +    /* FADT MADT PPTT GTDT MCFG SPCR pointed to by RSDT */
>>       acpi_add_table(table_offsets, tables_blob);
>>       build_fadt_rev5(tables_blob, tables->linker, vms, dsdt);
>>   
>>       acpi_add_table(table_offsets, tables_blob);
>>       build_madt(tables_blob, tables->linker, vms);
>>   
>> +    if (!vmc->no_cpu_topology) {
>> +        acpi_add_table(table_offsets, tables_blob);
>> +        build_pptt(tables_blob, tables->linker, vms);
>> +    }
>> +
>>       acpi_add_table(table_offsets, tables_blob);
>>       build_gtdt(tables_blob, tables->linker, vms);
>>   
>> -- 
>> 2.19.1
>>
> Thanks,
> drew
>
> .

Re: [RFC PATCH v3 8/9] hw/arm/virt-acpi-build: Generate PPTT table

[Andrew Jones]

On Mon, May 17, 2021 at 09:43:34PM +0800, wangyanan (Y) wrote:
> BTW, it seems patch 1 and 5 were possibly missed for some review.
> Any comments for them too? Thanks!

I reviewed them and agreed with them, but you already provided my
s-o-b on them, so I didn't bother giving an r-b. Feel free to add
my r-b to both, if you'd like.

Thanks,
drew

Re: [RFC PATCH v3 3/9] hw/arm/virt: Add cpu-map to device tree

[wangyanan (Y)]

Hi Drew,

On 2021/5/17 14:41, Andrew Jones wrote:
> On Sun, May 16, 2021 at 06:28:54PM +0800, Yanan Wang wrote:
>> From: Andrew Jones <drjones@redhat.com>
>>
>> Support device tree CPU topology descriptions.
>>
>> In accordance with the Devicetree Specification, the Linux Doc
>> "arm/cpus.yaml" requires that cpus and cpu nodes in the DT are
>> present. And we meet the requirement by creating /cpus/cpu@*
>> nodes for members within ms->smp.cpus.
>>
>> Correspondingly, we should also create subnodes in cpu-map for
>> the present cpus, each of which relates to an unique cpu node.
>>
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   hw/arm/virt.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index c07841e3a4..e5dcdebdbc 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -349,10 +349,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
> Rather than aligning the top line with the lower lines, we could remove
> the extra space from the lower lines. Or, leave the formatting as it was,
> by putting 'See' where 'From' was, like I did in my original patch.
I think I prefer removing the extra space from the lower lines, which is
the right thing to do.
>>        *  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
>> @@ -405,8 +406,46 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
>>                   ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
>>           }
>>   
>> +        if (!vmc->no_cpu_topology) {
>> +            qemu_fdt_setprop_cell(ms->fdt, nodename, "phandle",
>> +                                  qemu_fdt_alloc_phandle(ms->fdt));
>> +        }
>> +
>>           g_free(nodename);
>>       }
>> +
>> +    if (!vmc->no_cpu_topology) {
>> +        /*
>> +         * See Linux Documentation/devicetree/bindings/cpu/cpu-topology.txt
>> +         * In a SMP system, the hierarchy of CPUs is defined through four
>> +         * entities that are used to describe the layout of physical CPUs
> s/entities/levels/
Above comment was completely from Linux Doc cpu-topology.txt. See [1].
I think entities may be more reasonable than levels here, since there can be
multiple levels of clusters in cpu-map which makes the total not four.

[1] 
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/cpu/cpu-topology.txt
>> +         * in the system: socket/cluster/core/thread.
> The comment says there are four levels including 'cluster', but there's no
> 'cluster' below.
According to Doc [1] (line 114), a socket node's child nodes must be
*one or more* cluster nodes which means cluster is mandatory to be
socket's child in DT.

So I think maybe we should just keep the comment as-is, and change
the map-path from /cpus/cpu-map/socket*/cores*/threads* to
/cpus/cpu-map/socket*/cluster0/cores*/threads* in this patch?

Thanks,
Yanan
>> +         */
>> +        qemu_fdt_add_subnode(ms->fdt, "/cpus/cpu-map");
>> +
>> +        for (cpu = smp_cpus - 1; cpu >= 0; cpu--) {
>> +            char *cpu_path = g_strdup_printf("/cpus/cpu@%d", cpu);
>> +            char *map_path;
>> +
>> +            if (ms->smp.threads > 1) {
>> +                map_path = g_strdup_printf(
>> +                    "/cpus/cpu-map/%s%d/%s%d/%s%d",
>> +                    "socket", 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",
>> +                    "socket", cpu / ms->smp.cores,
>> +                    "core", cpu % ms->smp.cores);
>> +            }
>> +            qemu_fdt_add_path(ms->fdt, map_path);
>> +            qemu_fdt_setprop_phandle(ms->fdt, map_path, "cpu", cpu_path);
>> +
>> +            g_free(map_path);
>> +            g_free(cpu_path);
>> +        }
>> +    }
>>   }
>>   
>>   static void fdt_add_its_gic_node(VirtMachineState *vms)
>> -- 
>> 2.19.1
>>
> Thanks,
> drew
>
> .

Re: [RFC PATCH v3 8/9] hw/arm/virt-acpi-build: Generate PPTT table

[wangyanan (Y)]

On 2021/5/17 22:45, Andrew Jones wrote:
> On Mon, May 17, 2021 at 09:43:34PM +0800, wangyanan (Y) wrote:
>> BTW, it seems patch 1 and 5 were possibly missed for some review.
>> Any comments for them too? Thanks!
> I reviewed them and agreed with them, but you already provided my
> s-o-b on them, so I didn't bother giving an r-b. Feel free to add
> my r-b to both, if you'd like.
I see, thanks !
> Thanks,
> drew
>
> .

Re: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in generation of MADT

[wangyanan (Y)]

Hi Drew,

On 2021/5/17 15:42, Andrew Jones wrote:
> On Sun, May 16, 2021 at 06:28:57PM +0800, Yanan Wang wrote:
>> When building ACPI tables regarding CPUs we should always build
>> them for the number of possible CPUs, not the number of present
>> CPUs. So we create gicc nodes in MADT for possible cpus and then
>> ensure only the present CPUs are marked ENABLED. Furthermore, it
>> also needed if we are going to support CPU hotplug in the future.
>>
>> Co-developed-by: Andrew Jones <drjones@redhat.com>
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>> Co-developed-by: Ying Fang <fangying1@huawei.com>
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++----
>>   1 file changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index a2d8e87616..4d64aeb865 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -481,6 +481,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>       const int *irqmap = vms->irqmap;
>>       AcpiMadtGenericDistributor *gicd;
>>       AcpiMadtGenericMsiFrame *gic_msi;
>> +    MachineClass *mc = MACHINE_GET_CLASS(vms);
>> +    const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(MACHINE(vms));
>> +    bool pmu;
>>       int i;
>>   
>>       acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
>> @@ -491,11 +494,21 @@ 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));
>>   
>> +        /*
>> +         * PMU should have been either implemented for all CPUs or not,
>> +         * so we only get information from the first CPU, which could
>> +         * represent the others.
>> +         */
>> +        if (i == 0) {
>> +            pmu = arm_feature(&armcpu->env, ARM_FEATURE_PMU);
>> +        }
>> +        assert(!armcpu || arm_feature(&armcpu->env, ARM_FEATURE_PMU) == pmu);
> This doesn't belong in this patch. The commit message doesn't even mention
> it. Also, I don't think we should do this here at all. If we want to
> ensure that all cpus have a pmu when one does, then that should be done
> somewhere like machvirt_init(), not in ACPI generation code which doesn't
> even run for non-ACPI VMs.
Sorry, I should have stated the reason of this change in the commit message.
Actually code change here and mp_affinity part below aim to make it correct
to create gicc entries for all possible cpus.

We only initialize and realize cpuobj for present cpus in machvirt_init,
so that we will get null ARMCPU pointer here for the non-present cpus,
and consequently we won't able to check from "armcpu->env" for the
non-present cpus. The same about "armcpu->mp_affinity".

That's the reason I use PMU configuration of the first cpu to represent the
others. I assume all cpus should have a pmu when one does here since it's
how armcpu->env is initialized. And the assert seems not needed here.

Is there any better alternative way about this?
>> +
>>           gicc->type = ACPI_APIC_GENERIC_CPU_INTERFACE;
>>           gicc->length = sizeof(*gicc);
>>           if (vms->gic_version == 2) {
>> @@ -504,11 +517,19 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>               gicc->gicv_base_address = cpu_to_le64(memmap[VIRT_GIC_VCPU].base);
>>           }
>>           gicc->cpu_interface_number = cpu_to_le32(i);
>> -        gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
>> +        gicc->arm_mpidr = cpu_to_le64(possible_cpus->cpus[i].arch_id);
> Hmm, I think we may have a problem. I don't think there's any guarantee
> that possible_cpus->cpus[i].arch_id == armcpu->mp_affinity, because
> arch_id comes from virt_cpu_mp_affinity(), which is arm_cpu_mp_affinity,
> but with a variable cluster size, however mp_affinity comes from
> arm_cpu_mp_affinity with a set cluster size. Also, when KVM is used,
> then all bets are off as to what mp_affinity is.
Right! Arch_id is initialized by virt_cpu_mp_affinity() in machvirt and then
mp_affinity is initialized by arch_id. Here they two have the same value.

But mp_affinity will be overridden in kvm_arch_init_vcpu() when KVM is
enabled. Here they two won't have the same value.
> We need to add some code that ensures arch_id == mp_affinity,
Can we also update the arch_id at the same time when we change mp_affinity?
> and, for
> now, we should stick with mp_affinity, since, at least when KVM is used,
> that's the correct one.
I also prefer sticking with mp_affinity, if the problem I explain about 
ARMCPU
above can be perfectly solved.
>>           gicc->uid = cpu_to_le32(i);
>> -        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
>>   
>> -        if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
>> +        /*
>> +         * ACPI spec says that LAPIC entry for non present CPU may be
> Why are we talking about LAPICs in a GICC generator?
Ah, sorry. This ought to be GICC entry. Will fix.

Thanks,
Yanan
>> +         * omitted from MADT or it must be marked as disabled. Here we
>> +         * choose to also keep the disabled ones in MADT.
>> +         */
>> +        if (possible_cpus->cpus[i].cpu != NULL) {
>> +            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
>> +        }
>> +
>> +        if (pmu) {
>>               gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
>>           }
>>           if (vms->virt) {
>> -- 
>> 2.19.1
>>
> Thanks,
> drew
>
> .

RE: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in generation of MADT

[Salil Mehta]

> From: Qemu-arm [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
> On Behalf Of Yanan Wang
> Sent: Sunday, May 16, 2021 11:29 AM
> To: Peter Maydell <peter.maydell@linaro.org>; Andrew Jones
> <drjones@redhat.com>; Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov
> <imammedo@redhat.com>; Shannon Zhao <shannon.zhaosl@gmail.com>; Alistair
> Francis <alistair.francis@wdc.com>; David Gibson
> <david@gibson.dropbear.id.au>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; zhukeqian
> <zhukeqian1@huawei.com>; yangyicong <yangyicong@huawei.com>; Zengtao (B)
> <prime.zeng@hisilicon.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>;
> yuzenghui <yuzenghui@huawei.com>; Paolo Bonzini <pbonzini@redhat.com>;
> Philippe Mathieu-Daudé <philmd@redhat.com>
> Subject: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in
> generation of MADT
> 
> When building ACPI tables regarding CPUs we should always build
> them for the number of possible CPUs, not the number of present
> CPUs. So we create gicc nodes in MADT for possible cpus and then
> ensure only the present CPUs are marked ENABLED. Furthermore, it
> also needed if we are going to support CPU hotplug in the future.

Hi Yanan,
Yes, these changes are part of the QEMU patch-set I floated last year.

Link: https://www.mail-archive.com/qemu-devel@nongnu.org/msg712018.html


Perhaps I am missing something, but how this patch is related to the vcpu
topology support?

Thanks

> 
> Co-developed-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Co-developed-by: Ying Fang <fangying1@huawei.com>
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index a2d8e87616..4d64aeb865 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -481,6 +481,9 @@ build_madt(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
>      const int *irqmap = vms->irqmap;
>      AcpiMadtGenericDistributor *gicd;
>      AcpiMadtGenericMsiFrame *gic_msi;
> +    MachineClass *mc = MACHINE_GET_CLASS(vms);
> +    const CPUArchIdList *possible_cpus =
> mc->possible_cpu_arch_ids(MACHINE(vms));
> +    bool pmu;
>      int i;
> 
>      acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
> @@ -491,11 +494,21 @@ 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));
> 
> +        /*
> +         * PMU should have been either implemented for all CPUs or not,
> +         * so we only get information from the first CPU, which could
> +         * represent the others.
> +         */
> +        if (i == 0) {
> +            pmu = arm_feature(&armcpu->env, ARM_FEATURE_PMU);
> +        }
> +        assert(!armcpu || arm_feature(&armcpu->env, ARM_FEATURE_PMU) == pmu);
> +
>          gicc->type = ACPI_APIC_GENERIC_CPU_INTERFACE;
>          gicc->length = sizeof(*gicc);
>          if (vms->gic_version == 2) {
> @@ -504,11 +517,19 @@ build_madt(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
>              gicc->gicv_base_address =
> cpu_to_le64(memmap[VIRT_GIC_VCPU].base);
>          }
>          gicc->cpu_interface_number = cpu_to_le32(i);
> -        gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
> +        gicc->arm_mpidr = cpu_to_le64(possible_cpus->cpus[i].arch_id);
>          gicc->uid = cpu_to_le32(i);
> -        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
> 
> -        if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
> +        /*
> +         * ACPI spec says that LAPIC entry for non present CPU may be
> +         * omitted from MADT or it must be marked as disabled. Here we
> +         * choose to also keep the disabled ones in MADT.
> +         */
> +        if (possible_cpus->cpus[i].cpu != NULL) {
> +            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
> +        }
> +
> +        if (pmu) {
>              gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
>          }
>          if (vms->virt) {
> --
> 2.19.1
> 

RE: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members

[Salil Mehta]

> From: Qemu-arm [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
> On Behalf Of Yanan Wang
> Sent: Sunday, May 16, 2021 11:29 AM
> To: Peter Maydell <peter.maydell@linaro.org>; Andrew Jones
> <drjones@redhat.com>; Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov
> <imammedo@redhat.com>; Shannon Zhao <shannon.zhaosl@gmail.com>; Alistair
> Francis <alistair.francis@wdc.com>; David Gibson
> <david@gibson.dropbear.id.au>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; zhukeqian
> <zhukeqian1@huawei.com>; yangyicong <yangyicong@huawei.com>; Zengtao (B)
> <prime.zeng@hisilicon.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>;
> yuzenghui <yuzenghui@huawei.com>; Paolo Bonzini <pbonzini@redhat.com>;
> Philippe Mathieu-Daudé <philmd@redhat.com>
> Subject: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members
> 
> We create and initialize a cpuobj for each present cpu in
> machvirt_init(). Now we also initialize the cpu member of
> structure CPUArchId for each present cpu in the function.

[...]

>          qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> +
> +        /*
> +         * As ARM cpu hotplug is not supported yet, we initialize
> +         * the present cpu members here.
> +         */
> +        machine->possible_cpus->cpus[n].cpu = cpuobj;


when vcpu Hotplug is not supported yet, what necessitates this change now?

Re: [RFC PATCH v3 9/9] hw/arm/virt: Add separate -smp parsing function for ARM machines

[wangyanan (Y)]

Hi Drew,

On 2021/5/17 16:24, Andrew Jones wrote:
> On Sun, May 16, 2021 at 06:29:00PM +0800, Yanan Wang wrote:
>> The cpu hierarchy topology information parsed out from QEMU -smp
>> command line will be exposed to guest kernel through ACPI and DT
>> since machine type 6.1, so we will expect more detailed topology
>> descriptions and will be more strict with the -smp cmdlines when
>> parsing them.
>>
>> Compared with the default smp_parse() where all missing values
>> are automatically calculated in turn, there is some difference
>> in ARM specific virt_smp_parse(). The parsing logic is like:
>> At least one of cpus or maxcpus must be provided. Threads will
>> default to 1 if not provided (assume not support SMT). Sockets
>> and cores must be either both provided or both not.
>>
>> Note, if neither sockets nor cores are provided, we calculate
>> all the missing values like smp_parse() did before, but will
>> disable support of exposing these auto-populated descriptions
>> to guest. Then guest will populate its topology by default.
>>
>> Suggested-by: Andrew Jones <drjones@redhat.com>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   hw/arm/virt.c   | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   qemu-options.hx |  4 +++
>>   2 files changed, 99 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 50e324975f..44e990e3be 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -76,6 +76,8 @@
>>   #include "hw/virtio/virtio-iommu.h"
>>   #include "hw/char/pl011.h"
>>   #include "qemu/guest-random.h"
>> +#include "qapi/qmp/qerror.h"
>> +#include "sysemu/replay.h"
>>   
>>   #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>>       static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
>> @@ -2627,6 +2629,98 @@ static int virt_kvm_type(MachineState *ms, const char *type_str)
>>       return fixed_ipa ? 0 : requested_pa_size;
>>   }
>>   
>> +/*
>> + * virt_smp_parse - Used to parse -smp command line for ARM machines.
>> + *
>> + * Compared with the default smp_parse where all the missing values
>> + * are automatically calculated in turn, in this function, we expect
>> + * more detailed topology information provided and are more strict
>> + * with the -smp cmdlines when parsing them.
>> + *
>> + * We require that at least one of cpus or maxcpus must be provided.
>> + * Threads will default to 1 if not provided. Sockets and cores must
>> + * be either both provided or both not.
>> + *
>> + * Note, if neither sockets nor cores are specified, we will calculate
>> + * all the missing values just like smp_parse() does, but will disable
>> + * exposure of cpu topology descriptions to guest.
>> + */
>> +static void virt_smp_parse(MachineState *ms, QemuOpts *opts)
>> +{
>> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(ms);
>> +
>> +    if (opts) {
>> +        unsigned cpus = qemu_opt_get_number(opts, "cpus", 0);
>> +        unsigned maxcpus = qemu_opt_get_number(opts, "maxcpus", 0);
>> +        unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
>> +        unsigned cores = qemu_opt_get_number(opts, "cores", 0);
>> +        unsigned threads = qemu_opt_get_number(opts, "threads", 0);
>> +
>> +        /* Default threads to 1 if not provided */
>> +        threads = threads > 0 ? threads : 1;
> Can't do this yet, need to do it later, because...
>
>> +
>> +        if (cpus == 0 && maxcpus == 0) {
>> +            error_report("at least one of cpus or maxcpus must be provided");
>> +            exit(1);
>> +        }
>> +
>> +        if (sockets == 0 && cores == 0) {
>> +            /* Disable exposure of topology descriptions to guest */
>> +            vmc->no_cpu_topology = true;
> ...we should do ensure threads == 0 here, otherwise provide another error
> message.
>
>> +
>> +            /* Compute missing values, prefer sockets over cores */
>> +            cores = 1;
> Now threads = 1 is good here.
>
>> +            if (cpus == 0) {
>> +                sockets = 1;
>> +                cpus = sockets * cores * threads;
> This should be
>
>    cpus = maxcpus;
>    sockets = cpus;
>
>> +            } else {
>> +                maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> +                sockets = maxcpus / (cores * threads);
> We know cores and threads should both be 1 here, so just do
>
>   sockets = maxcpus;
>
>> +            }
>> +        } else if (sockets > 0 && cores > 0) {
> Now
>          threads = threads > 0 ? threads : 1;
>
> is good here.
>
>> +            cpus = cpus > 0 ? cpus : sockets * cores * threads;
>> +            maxcpus = maxcpus > 0 ? maxcpus : cpus;
> We should calculate maxcpus first based on the topology,
>
>    maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
>    cpus = cpus > 0 ? cpus : maxcpus;
 From comments above, now I see something was missed by me in
previous discussion in v3. We won't allow that threads is provided
but sockets and cores are not. And cpus should default to maxcpus
if it is not explicitly specified.

I will fix as above suggests.
> Please do comprehensive testing to ensure everything works as it
> should. You can drop this function into a standalone executable
> and run it for all possible inputs, maxcpus=0, maxcpus < cpus, maxcpus ==
> cpus, maxcpus > cpus, sockets = 0, sockets < cpus, sockets == cpus, etc.
Of course, these tests are definitely necessary, will do it after the 
rework.

Thnaks,
Yanan
>> +        } else {
>> +            error_report("sockets and cores must be both provided "
>> +                         "or both not");
>> +            exit(1);
>> +        }
>> +
>> +        if (maxcpus < cpus) {
>> +            error_report("maxcpus must be equal to or greater than smp");
>> +            exit(1);
>> +        }
>> +
>> +        if (sockets * cores * threads < cpus) {
>> +            error_report("cpu topology: "
>> +                         "sockets (%u) * cores (%u) * threads (%u) < "
>> +                         "smp_cpus (%u)",
>> +                         sockets, cores, threads, cpus);
>> +            exit(1);
>> +        }
>> +
>> +        if (sockets * cores * threads != maxcpus) {
>> +            error_report("cpu topology: "
>> +                         "sockets (%u) * cores (%u) * threads (%u) "
>> +                         "!= maxcpus (%u)",
>> +                         sockets, cores, threads, maxcpus);
>> +            exit(1);
>> +        }
>> +
>> +        ms->smp.cpus = cpus;
>> +        ms->smp.max_cpus = maxcpus;
>> +        ms->smp.sockets = sockets;
>> +        ms->smp.cores = cores;
>> +        ms->smp.threads = threads;
>> +    }
>> +
>> +    if (ms->smp.cpus > 1) {
>> +        Error *blocker = NULL;
>> +        error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED, "smp");
>> +        replay_add_blocker(blocker);
>> +    }
>> +}
>> +
>>   static void virt_machine_class_init(ObjectClass *oc, void *data)
>>   {
>>       MachineClass *mc = MACHINE_CLASS(oc);
>> @@ -2652,6 +2746,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>>       mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
>>       mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
>>       mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
>> +    mc->smp_parse = virt_smp_parse;
>>       mc->kvm_type = virt_kvm_type;
>>       assert(!mc->get_hotplug_handler);
>>       mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 635dc8a624..bd97086c21 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -203,6 +203,10 @@ SRST
>>       computed. If any on the three values is given, the total number of
>>       CPUs n can be omitted. maxcpus specifies the maximum number of
>>       hotpluggable CPUs.
>> +
>> +    For the ARM target, at least one of cpus or maxcpus must be provided.
>> +    Threads will default to 1 if not provided. Sockets and cores must be
>> +    either both provided or both not.
>>   ERST
>>   
>>   DEF("numa", HAS_ARG, QEMU_OPTION_numa,
>> -- 
>> 2.19.1
>>
> .

Re: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members

[wangyanan (Y)]

Hi Salil,

On 2021/5/18 4:48, Salil Mehta wrote:
>> From: Qemu-arm [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
>> On Behalf Of Yanan Wang
>> Sent: Sunday, May 16, 2021 11:29 AM
>> To: Peter Maydell <peter.maydell@linaro.org>; Andrew Jones
>> <drjones@redhat.com>; Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov
>> <imammedo@redhat.com>; Shannon Zhao <shannon.zhaosl@gmail.com>; Alistair
>> Francis <alistair.francis@wdc.com>; David Gibson
>> <david@gibson.dropbear.id.au>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
>> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; zhukeqian
>> <zhukeqian1@huawei.com>; yangyicong <yangyicong@huawei.com>; Zengtao (B)
>> <prime.zeng@hisilicon.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>;
>> yuzenghui <yuzenghui@huawei.com>; Paolo Bonzini <pbonzini@redhat.com>;
>> Philippe Mathieu-Daudé <philmd@redhat.com>
>> Subject: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members
>>
>> We create and initialize a cpuobj for each present cpu in
>> machvirt_init(). Now we also initialize the cpu member of
>> structure CPUArchId for each present cpu in the function.
> [...]
>
>>           qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
>> +
>> +        /*
>> +         * As ARM cpu hotplug is not supported yet, we initialize
>> +         * the present cpu members here.
>> +         */
>> +        machine->possible_cpus->cpus[n].cpu = cpuobj;
>
> when vcpu Hotplug is not supported yet, what necessitates this change now?
>
The initialization will gives a way to determine whether a CPU is 
present or not.
At least, for now it will be used when generating ACPI tables, e.g. 
DSDT, MADT.
See patch 5 and 6.

Thanks,
Yanan
> .

Re: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in generation of MADT

[wangyanan (Y)]

Hi Salil,

On 2021/5/18 1:07, Salil Mehta wrote:
>> From: Qemu-arm [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
>> On Behalf Of Yanan Wang
>> Sent: Sunday, May 16, 2021 11:29 AM
>> To: Peter Maydell <peter.maydell@linaro.org>; Andrew Jones
>> <drjones@redhat.com>; Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov
>> <imammedo@redhat.com>; Shannon Zhao <shannon.zhaosl@gmail.com>; Alistair
>> Francis <alistair.francis@wdc.com>; David Gibson
>> <david@gibson.dropbear.id.au>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
>> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; zhukeqian
>> <zhukeqian1@huawei.com>; yangyicong <yangyicong@huawei.com>; Zengtao (B)
>> <prime.zeng@hisilicon.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>;
>> yuzenghui <yuzenghui@huawei.com>; Paolo Bonzini <pbonzini@redhat.com>;
>> Philippe Mathieu-Daudé <philmd@redhat.com>
>> Subject: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in
>> generation of MADT
>>
>> When building ACPI tables regarding CPUs we should always build
>> them for the number of possible CPUs, not the number of present
>> CPUs. So we create gicc nodes in MADT for possible cpus and then
>> ensure only the present CPUs are marked ENABLED. Furthermore, it
>> also needed if we are going to support CPU hotplug in the future.
> Hi Yanan,
> Yes, these changes are part of the QEMU patch-set I floated last year.
>
> Link: https://www.mail-archive.com/qemu-devel@nongnu.org/msg712018.html
Yes, I noticed this. Thanks!
>
> Perhaps I am missing something, but how this patch is related to the vcpu
> topology support?
No related actually. But this patch together with patch 5 aim to provide
complete information (all cpus including enabled and the others) to guest,
which will be more consistent with requirement in ACPI spec.

We don't consider cpu hotplug at all in this patch, but it indeed pave way
for cpu hotplug in the future.

Thanks,
Yanan
> Thanks
>
>> Co-developed-by: Andrew Jones <drjones@redhat.com>
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>> Co-developed-by: Ying Fang <fangying1@huawei.com>
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++----
>>   1 file changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index a2d8e87616..4d64aeb865 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -481,6 +481,9 @@ build_madt(GArray *table_data, BIOSLinker *linker,
>> VirtMachineState *vms)
>>       const int *irqmap = vms->irqmap;
>>       AcpiMadtGenericDistributor *gicd;
>>       AcpiMadtGenericMsiFrame *gic_msi;
>> +    MachineClass *mc = MACHINE_GET_CLASS(vms);
>> +    const CPUArchIdList *possible_cpus =
>> mc->possible_cpu_arch_ids(MACHINE(vms));
>> +    bool pmu;
>>       int i;
>>
>>       acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
>> @@ -491,11 +494,21 @@ 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));
>>
>> +        /*
>> +         * PMU should have been either implemented for all CPUs or not,
>> +         * so we only get information from the first CPU, which could
>> +         * represent the others.
>> +         */
>> +        if (i == 0) {
>> +            pmu = arm_feature(&armcpu->env, ARM_FEATURE_PMU);
>> +        }
>> +        assert(!armcpu || arm_feature(&armcpu->env, ARM_FEATURE_PMU) == pmu);
>> +
>>           gicc->type = ACPI_APIC_GENERIC_CPU_INTERFACE;
>>           gicc->length = sizeof(*gicc);
>>           if (vms->gic_version == 2) {
>> @@ -504,11 +517,19 @@ build_madt(GArray *table_data, BIOSLinker *linker,
>> VirtMachineState *vms)
>>               gicc->gicv_base_address =
>> cpu_to_le64(memmap[VIRT_GIC_VCPU].base);
>>           }
>>           gicc->cpu_interface_number = cpu_to_le32(i);
>> -        gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
>> +        gicc->arm_mpidr = cpu_to_le64(possible_cpus->cpus[i].arch_id);
>>           gicc->uid = cpu_to_le32(i);
>> -        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
>>
>> -        if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
>> +        /*
>> +         * ACPI spec says that LAPIC entry for non present CPU may be
>> +         * omitted from MADT or it must be marked as disabled. Here we
>> +         * choose to also keep the disabled ones in MADT.
>> +         */
>> +        if (possible_cpus->cpus[i].cpu != NULL) {
>> +            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
>> +        }
>> +
>> +        if (pmu) {
>>               gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
>>           }
>>           if (vms->virt) {
>> --
>> 2.19.1
>>
> .

RE: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in generation of MADT

[Salil Mehta]

> From: wangyanan (Y)
> Sent: Tuesday, May 18, 2021 6:03 AM
> 
> Hi Salil,
> 
> On 2021/5/18 1:07, Salil Mehta wrote:
> >> From: Qemu-arm
> [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
> >> On Behalf Of Yanan Wang
> >> Sent: Sunday, May 16, 2021 11:29 AM
> >> To: Peter Maydell <peter.maydell@linaro.org>; Andrew Jones
> >> <drjones@redhat.com>; Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov
> >> <imammedo@redhat.com>; Shannon Zhao <shannon.zhaosl@gmail.com>; Alistair
> >> Francis <alistair.francis@wdc.com>; David Gibson
> >> <david@gibson.dropbear.id.au>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> >> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; zhukeqian
> >> <zhukeqian1@huawei.com>; yangyicong <yangyicong@huawei.com>; Zengtao (B)
> >> <prime.zeng@hisilicon.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>;
> >> yuzenghui <yuzenghui@huawei.com>; Paolo Bonzini <pbonzini@redhat.com>;
> >> Philippe Mathieu-Daudé <philmd@redhat.com>
> >> Subject: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in
> >> generation of MADT
> >>
> >> When building ACPI tables regarding CPUs we should always build
> >> them for the number of possible CPUs, not the number of present
> >> CPUs. So we create gicc nodes in MADT for possible cpus and then
> >> ensure only the present CPUs are marked ENABLED. Furthermore, it
> >> also needed if we are going to support CPU hotplug in the future.
> > Hi Yanan,
> > Yes, these changes are part of the QEMU patch-set I floated last year.
> >
> > Link: https://www.mail-archive.com/qemu-devel@nongnu.org/msg712018.html
> Yes, I noticed this. Thanks!
> >
> > Perhaps I am missing something, but how this patch is related to the vcpu
> > topology support?
> No related actually. But this patch together with patch 5 aim to provide
> complete information (all cpus including enabled and the others) to guest,
> which will be more consistent with requirement in ACPI spec.


Well, if it is not related to the cpu topology support then this and other
similar patches included with the same line of thought should not be
part of this patch-set. 

I am already working with ARM folks in this regard.

Thanks

> 
> We don't consider cpu hotplug at all in this patch, but it indeed pave way
> for cpu hotplug in the future.
> 
> Thanks,
> Yanan
> > Thanks
> >
> >> Co-developed-by: Andrew Jones <drjones@redhat.com>
> >> Signed-off-by: Andrew Jones <drjones@redhat.com>
> >> Co-developed-by: Ying Fang <fangying1@huawei.com>
> >> Signed-off-by: Ying Fang <fangying1@huawei.com>
> >> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> >> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> >> ---
> >>   hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++----
> >>   1 file changed, 25 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >> index a2d8e87616..4d64aeb865 100644
> >> --- a/hw/arm/virt-acpi-build.c
> >> +++ b/hw/arm/virt-acpi-build.c
> >> @@ -481,6 +481,9 @@ build_madt(GArray *table_data, BIOSLinker *linker,
> >> VirtMachineState *vms)
> >>       const int *irqmap = vms->irqmap;
> >>       AcpiMadtGenericDistributor *gicd;
> >>       AcpiMadtGenericMsiFrame *gic_msi;
> >> +    MachineClass *mc = MACHINE_GET_CLASS(vms);
> >> +    const CPUArchIdList *possible_cpus =
> >> mc->possible_cpu_arch_ids(MACHINE(vms));
> >> +    bool pmu;
> >>       int i;
> >>
> >>       acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
> >> @@ -491,11 +494,21 @@ 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));
> >>
> >> +        /*
> >> +         * PMU should have been either implemented for all CPUs or not,
> >> +         * so we only get information from the first CPU, which could
> >> +         * represent the others.
> >> +         */
> >> +        if (i == 0) {
> >> +            pmu = arm_feature(&armcpu->env, ARM_FEATURE_PMU);
> >> +        }
> >> +        assert(!armcpu || arm_feature(&armcpu->env, ARM_FEATURE_PMU) ==
> pmu);
> >> +
> >>           gicc->type = ACPI_APIC_GENERIC_CPU_INTERFACE;
> >>           gicc->length = sizeof(*gicc);
> >>           if (vms->gic_version == 2) {
> >> @@ -504,11 +517,19 @@ build_madt(GArray *table_data, BIOSLinker *linker,
> >> VirtMachineState *vms)
> >>               gicc->gicv_base_address =
> >> cpu_to_le64(memmap[VIRT_GIC_VCPU].base);
> >>           }
> >>           gicc->cpu_interface_number = cpu_to_le32(i);
> >> -        gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
> >> +        gicc->arm_mpidr = cpu_to_le64(possible_cpus->cpus[i].arch_id);
> >>           gicc->uid = cpu_to_le32(i);
> >> -        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
> >>
> >> -        if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
> >> +        /*
> >> +         * ACPI spec says that LAPIC entry for non present CPU may be
> >> +         * omitted from MADT or it must be marked as disabled. Here we
> >> +         * choose to also keep the disabled ones in MADT.
> >> +         */
> >> +        if (possible_cpus->cpus[i].cpu != NULL) {
> >> +            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
> >> +        }
> >> +
> >> +        if (pmu) {
> >>               gicc->performance_interrupt =
> cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
> >>           }
> >>           if (vms->virt) {
> >> --
> >> 2.19.1
> >>
> > .

RE: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in generation of MADT

[Salil Mehta]

> From: wangyanan (Y)
> Sent: Tuesday, May 18, 2021 6:03 AM
> 
> Hi Salil,
> 
> On 2021/5/18 1:07, Salil Mehta wrote:
> >> From: Qemu-arm
> [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
> >> On Behalf Of Yanan Wang
> >> Sent: Sunday, May 16, 2021 11:29 AM
> >> To: Peter Maydell <peter.maydell@linaro.org>; Andrew Jones
> >> <drjones@redhat.com>; Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov
> >> <imammedo@redhat.com>; Shannon Zhao <shannon.zhaosl@gmail.com>; Alistair
> >> Francis <alistair.francis@wdc.com>; David Gibson
> >> <david@gibson.dropbear.id.au>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> >> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; zhukeqian
> >> <zhukeqian1@huawei.com>; yangyicong <yangyicong@huawei.com>; Zengtao (B)
> >> <prime.zeng@hisilicon.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>;
> >> yuzenghui <yuzenghui@huawei.com>; Paolo Bonzini <pbonzini@redhat.com>;
> >> Philippe Mathieu-Daudé <philmd@redhat.com>
> >> Subject: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in
> >> generation of MADT
> >>
> >> When building ACPI tables regarding CPUs we should always build
> >> them for the number of possible CPUs, not the number of present
> >> CPUs. So we create gicc nodes in MADT for possible cpus and then
> >> ensure only the present CPUs are marked ENABLED. Furthermore, it
> >> also needed if we are going to support CPU hotplug in the future.
> > Hi Yanan,
> > Yes, these changes are part of the QEMU patch-set I floated last year.
> >
> > Link: https://www.mail-archive.com/qemu-devel@nongnu.org/msg712018.html
> Yes, I noticed this. Thanks!
> >
> > Perhaps I am missing something, but how this patch is related to the vcpu
> > topology support?
> No related actually. But this patch together with patch 5 aim to provide
> complete information (all cpus including enabled and the others) to guest,
> which will be more consistent with requirement in ACPI spec.


Well, if it is not related to the cpu topology support then this and other
similar patches included with the same line of thought should not be
part of this patch-set. 

I am already working with ARM folks in this regard.

Thanks

> 
> We don't consider cpu hotplug at all in this patch, but it indeed pave way
> for cpu hotplug in the future.
> 
> Thanks,
> Yanan
> > Thanks
> >
> >> Co-developed-by: Andrew Jones <drjones@redhat.com>
> >> Signed-off-by: Andrew Jones <drjones@redhat.com>
> >> Co-developed-by: Ying Fang <fangying1@huawei.com>
> >> Signed-off-by: Ying Fang <fangying1@huawei.com>
> >> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> >> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> >> ---
> >>   hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++----
> >>   1 file changed, 25 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >> index a2d8e87616..4d64aeb865 100644
> >> --- a/hw/arm/virt-acpi-build.c
> >> +++ b/hw/arm/virt-acpi-build.c
> >> @@ -481,6 +481,9 @@ build_madt(GArray *table_data, BIOSLinker *linker,
> >> VirtMachineState *vms)
> >>       const int *irqmap = vms->irqmap;
> >>       AcpiMadtGenericDistributor *gicd;
> >>       AcpiMadtGenericMsiFrame *gic_msi;
> >> +    MachineClass *mc = MACHINE_GET_CLASS(vms);
> >> +    const CPUArchIdList *possible_cpus =
> >> mc->possible_cpu_arch_ids(MACHINE(vms));
> >> +    bool pmu;
> >>       int i;
> >>
> >>       acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
> >> @@ -491,11 +494,21 @@ 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));
> >>
> >> +        /*
> >> +         * PMU should have been either implemented for all CPUs or not,
> >> +         * so we only get information from the first CPU, which could
> >> +         * represent the others.
> >> +         */
> >> +        if (i == 0) {
> >> +            pmu = arm_feature(&armcpu->env, ARM_FEATURE_PMU);
> >> +        }
> >> +        assert(!armcpu || arm_feature(&armcpu->env, ARM_FEATURE_PMU) ==
> pmu);
> >> +
> >>           gicc->type = ACPI_APIC_GENERIC_CPU_INTERFACE;
> >>           gicc->length = sizeof(*gicc);
> >>           if (vms->gic_version == 2) {
> >> @@ -504,11 +517,19 @@ build_madt(GArray *table_data, BIOSLinker *linker,
> >> VirtMachineState *vms)
> >>               gicc->gicv_base_address =
> >> cpu_to_le64(memmap[VIRT_GIC_VCPU].base);
> >>           }
> >>           gicc->cpu_interface_number = cpu_to_le32(i);
> >> -        gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
> >> +        gicc->arm_mpidr = cpu_to_le64(possible_cpus->cpus[i].arch_id);
> >>           gicc->uid = cpu_to_le32(i);
> >> -        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
> >>
> >> -        if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
> >> +        /*
> >> +         * ACPI spec says that LAPIC entry for non present CPU may be
> >> +         * omitted from MADT or it must be marked as disabled. Here we
> >> +         * choose to also keep the disabled ones in MADT.
> >> +         */
> >> +        if (possible_cpus->cpus[i].cpu != NULL) {
> >> +            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
> >> +        }
> >> +
> >> +        if (pmu) {
> >>               gicc->performance_interrupt =
> cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
> >>           }
> >>           if (vms->virt) {
> >> --
> >> 2.19.1
> >>
> > .

RE: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members

[Salil Mehta]

> From: wangyanan (Y)
> Sent: Tuesday, May 18, 2021 5:43 AM
> 
> Hi Salil,
> 
> On 2021/5/18 4:48, Salil Mehta wrote:
> >> From: Qemu-arm
> [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
> >> On Behalf Of Yanan Wang
> >> Sent: Sunday, May 16, 2021 11:29 AM
> >> To: Peter Maydell <peter.maydell@linaro.org>; Andrew Jones
> >> <drjones@redhat.com>; Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov
> >> <imammedo@redhat.com>; Shannon Zhao <shannon.zhaosl@gmail.com>; Alistair
> >> Francis <alistair.francis@wdc.com>; David Gibson
> >> <david@gibson.dropbear.id.au>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> >> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; zhukeqian
> >> <zhukeqian1@huawei.com>; yangyicong <yangyicong@huawei.com>; Zengtao (B)
> >> <prime.zeng@hisilicon.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>;
> >> yuzenghui <yuzenghui@huawei.com>; Paolo Bonzini <pbonzini@redhat.com>;
> >> Philippe Mathieu-Daudé <philmd@redhat.com>
> >> Subject: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members
> >>
> >> We create and initialize a cpuobj for each present cpu in
> >> machvirt_init(). Now we also initialize the cpu member of
> >> structure CPUArchId for each present cpu in the function.
> > [...]
> >
> >>           qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> >> +
> >> +        /*
> >> +         * As ARM cpu hotplug is not supported yet, we initialize
> >> +         * the present cpu members here.
> >> +         */
> >> +        machine->possible_cpus->cpus[n].cpu = cpuobj;
> >
> > when vcpu Hotplug is not supported yet, what necessitates this change now?
> >
> The initialization will gives a way to determine whether a CPU is
> present or not.
> At least, for now it will be used when generating ACPI tables, e.g.
> DSDT, MADT.
> See patch 5 and 6.

yes,  but why do you require it now as part of the vcpu topology change?

As-far-as-i-can-see, PPTT table changes(part of patch 5/9) do not require
this change. Change in Patch 5/9 has also been done in anticipation of
some future requirement(vcpu Hotplug?).

Please correct me here if I am wrong?

Thanks

RE: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members

[Salil Mehta]

> From: wangyanan (Y)
> Sent: Tuesday, May 18, 2021 5:43 AM
> 
> Hi Salil,
> 
> On 2021/5/18 4:48, Salil Mehta wrote:
> >> From: Qemu-arm
> [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
> >> On Behalf Of Yanan Wang
> >> Sent: Sunday, May 16, 2021 11:29 AM
> >> To: Peter Maydell <peter.maydell@linaro.org>; Andrew Jones
> >> <drjones@redhat.com>; Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov
> >> <imammedo@redhat.com>; Shannon Zhao <shannon.zhaosl@gmail.com>; Alistair
> >> Francis <alistair.francis@wdc.com>; David Gibson
> >> <david@gibson.dropbear.id.au>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> >> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; zhukeqian
> >> <zhukeqian1@huawei.com>; yangyicong <yangyicong@huawei.com>; Zengtao (B)
> >> <prime.zeng@hisilicon.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>;
> >> yuzenghui <yuzenghui@huawei.com>; Paolo Bonzini <pbonzini@redhat.com>;
> >> Philippe Mathieu-Daudé <philmd@redhat.com>
> >> Subject: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members
> >>
> >> We create and initialize a cpuobj for each present cpu in
> >> machvirt_init(). Now we also initialize the cpu member of
> >> structure CPUArchId for each present cpu in the function.
> > [...]
> >
> >>           qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> >> +
> >> +        /*
> >> +         * As ARM cpu hotplug is not supported yet, we initialize
> >> +         * the present cpu members here.
> >> +         */
> >> +        machine->possible_cpus->cpus[n].cpu = cpuobj;
> >
> > when vcpu Hotplug is not supported yet, what necessitates this change now?
> >
> The initialization will gives a way to determine whether a CPU is
> present or not.
> At least, for now it will be used when generating ACPI tables, e.g.
> DSDT, MADT.
> See patch 5 and 6.

yes,  but why do you require it now as part of the vcpu topology change?

As-far-as-i-can-see, PPTT table changes(part of patch 5/9) do not require
this change. Change in Patch 5/9 has also been done in anticipation of
some future requirement(vcpu Hotplug?).

Please correct me here if I am wrong?

Thanks

Re: [RFC PATCH v3 3/9] hw/arm/virt: Add cpu-map to device tree

[Andrew Jones]

On Mon, May 17, 2021 at 11:00:07PM +0800, wangyanan (Y) wrote:
> Hi Drew,
> 
> On 2021/5/17 14:41, Andrew Jones wrote:
> > On Sun, May 16, 2021 at 06:28:54PM +0800, Yanan Wang wrote:
> > > From: Andrew Jones <drjones@redhat.com>
> > > 
> > > Support device tree CPU topology descriptions.
> > > 
> > > In accordance with the Devicetree Specification, the Linux Doc
> > > "arm/cpus.yaml" requires that cpus and cpu nodes in the DT are
> > > present. And we meet the requirement by creating /cpus/cpu@*
> > > nodes for members within ms->smp.cpus.
> > > 
> > > Correspondingly, we should also create subnodes in cpu-map for
> > > the present cpus, each of which relates to an unique cpu node.
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > > ---
> > >   hw/arm/virt.c | 41 ++++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 40 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index c07841e3a4..e5dcdebdbc 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -349,10 +349,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
> > Rather than aligning the top line with the lower lines, we could remove
> > the extra space from the lower lines. Or, leave the formatting as it was,
> > by putting 'See' where 'From' was, like I did in my original patch.
> I think I prefer removing the extra space from the lower lines, which is
> the right thing to do.

OK

> > >        *  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
> > > @@ -405,8 +406,46 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
> > >                   ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
> > >           }
> > > +        if (!vmc->no_cpu_topology) {
> > > +            qemu_fdt_setprop_cell(ms->fdt, nodename, "phandle",
> > > +                                  qemu_fdt_alloc_phandle(ms->fdt));
> > > +        }
> > > +
> > >           g_free(nodename);
> > >       }
> > > +
> > > +    if (!vmc->no_cpu_topology) {
> > > +        /*
> > > +         * See Linux Documentation/devicetree/bindings/cpu/cpu-topology.txt
> > > +         * In a SMP system, the hierarchy of CPUs is defined through four
> > > +         * entities that are used to describe the layout of physical CPUs
> > s/entities/levels/
> Above comment was completely from Linux Doc cpu-topology.txt. See [1].
> I think entities may be more reasonable than levels here, since there can be
> multiple levels of clusters in cpu-map which makes the total not four.

OK

> 
> [1] https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/cpu/cpu-topology.txt
> > > +         * in the system: socket/cluster/core/thread.
> > The comment says there are four levels including 'cluster', but there's no
> > 'cluster' below.
> According to Doc [1] (line 114), a socket node's child nodes must be
> *one or more* cluster nodes which means cluster is mandatory to be
> socket's child in DT.
> 
> So I think maybe we should just keep the comment as-is, and change
> the map-path from /cpus/cpu-map/socket*/cores*/threads* to
> /cpus/cpu-map/socket*/cluster0/cores*/threads* in this patch?

I agree. In fact, that's how I implemented it myself[1]

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

Thanks,
drew

Re: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members

[Andrew Jones]

On Tue, May 18, 2021 at 07:04:51AM +0000, Salil Mehta wrote:
> > From: wangyanan (Y)
> > Sent: Tuesday, May 18, 2021 5:43 AM
> > 
> > Hi Salil,
> > 
> > On 2021/5/18 4:48, Salil Mehta wrote:
> > >> From: Qemu-arm
> > [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
> > >> On Behalf Of Yanan Wang
> > >> Sent: Sunday, May 16, 2021 11:29 AM
> > >> To: Peter Maydell <peter.maydell@linaro.org>; Andrew Jones
> > >> <drjones@redhat.com>; Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov
> > >> <imammedo@redhat.com>; Shannon Zhao <shannon.zhaosl@gmail.com>; Alistair
> > >> Francis <alistair.francis@wdc.com>; David Gibson
> > >> <david@gibson.dropbear.id.au>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> > >> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; zhukeqian
> > >> <zhukeqian1@huawei.com>; yangyicong <yangyicong@huawei.com>; Zengtao (B)
> > >> <prime.zeng@hisilicon.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>;
> > >> yuzenghui <yuzenghui@huawei.com>; Paolo Bonzini <pbonzini@redhat.com>;
> > >> Philippe Mathieu-Daudé <philmd@redhat.com>
> > >> Subject: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members
> > >>
> > >> We create and initialize a cpuobj for each present cpu in
> > >> machvirt_init(). Now we also initialize the cpu member of
> > >> structure CPUArchId for each present cpu in the function.
> > > [...]
> > >
> > >>           qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> > >> +
> > >> +        /*
> > >> +         * As ARM cpu hotplug is not supported yet, we initialize
> > >> +         * the present cpu members here.
> > >> +         */
> > >> +        machine->possible_cpus->cpus[n].cpu = cpuobj;
> > >
> > > when vcpu Hotplug is not supported yet, what necessitates this change now?
> > >
> > The initialization will gives a way to determine whether a CPU is
> > present or not.
> > At least, for now it will be used when generating ACPI tables, e.g.
> > DSDT, MADT.
> > See patch 5 and 6.
> 
> yes,  but why do you require it now as part of the vcpu topology change?
> 
> As-far-as-i-can-see, PPTT table changes(part of patch 5/9) do not require
> this change. Change in Patch 5/9 has also been done in anticipation of
> some future requirement(vcpu Hotplug?).
> 
> Please correct me here if I am wrong?
>

Hi Salil,

The problem is that we've never required smp.cpus == smp.maxcpus, so
a user could have smp.cpus < smp.maxcpus. We want the topology to match
maxcpus, but only enable cpus. However, if you think we should just not
allow cpus < maxcpus until hot plug is sorted out, then we could discuss
a way of trying to enforce cpus == maxcpus, but I'm not sure how we can
without breaking existing command lines.

Thanks,
drew

Re: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members

[Andrew Jones]

On Tue, May 18, 2021 at 07:04:51AM +0000, Salil Mehta wrote:
> > From: wangyanan (Y)
> > Sent: Tuesday, May 18, 2021 5:43 AM
> > 
> > Hi Salil,
> > 
> > On 2021/5/18 4:48, Salil Mehta wrote:
> > >> From: Qemu-arm
> > [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
> > >> On Behalf Of Yanan Wang
> > >> Sent: Sunday, May 16, 2021 11:29 AM
> > >> To: Peter Maydell <peter.maydell@linaro.org>; Andrew Jones
> > >> <drjones@redhat.com>; Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov
> > >> <imammedo@redhat.com>; Shannon Zhao <shannon.zhaosl@gmail.com>; Alistair
> > >> Francis <alistair.francis@wdc.com>; David Gibson
> > >> <david@gibson.dropbear.id.au>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> > >> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; zhukeqian
> > >> <zhukeqian1@huawei.com>; yangyicong <yangyicong@huawei.com>; Zengtao (B)
> > >> <prime.zeng@hisilicon.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>;
> > >> yuzenghui <yuzenghui@huawei.com>; Paolo Bonzini <pbonzini@redhat.com>;
> > >> Philippe Mathieu-Daudé <philmd@redhat.com>
> > >> Subject: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members
> > >>
> > >> We create and initialize a cpuobj for each present cpu in
> > >> machvirt_init(). Now we also initialize the cpu member of
> > >> structure CPUArchId for each present cpu in the function.
> > > [...]
> > >
> > >>           qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> > >> +
> > >> +        /*
> > >> +         * As ARM cpu hotplug is not supported yet, we initialize
> > >> +         * the present cpu members here.
> > >> +         */
> > >> +        machine->possible_cpus->cpus[n].cpu = cpuobj;
> > >
> > > when vcpu Hotplug is not supported yet, what necessitates this change now?
> > >
> > The initialization will gives a way to determine whether a CPU is
> > present or not.
> > At least, for now it will be used when generating ACPI tables, e.g.
> > DSDT, MADT.
> > See patch 5 and 6.
> 
> yes,  but why do you require it now as part of the vcpu topology change?
> 
> As-far-as-i-can-see, PPTT table changes(part of patch 5/9) do not require
> this change. Change in Patch 5/9 has also been done in anticipation of
> some future requirement(vcpu Hotplug?).
> 
> Please correct me here if I am wrong?
>

Hi Salil,

The problem is that we've never required smp.cpus == smp.maxcpus, so
a user could have smp.cpus < smp.maxcpus. We want the topology to match
maxcpus, but only enable cpus. However, if you think we should just not
allow cpus < maxcpus until hot plug is sorted out, then we could discuss
a way of trying to enforce cpus == maxcpus, but I'm not sure how we can
without breaking existing command lines.

Thanks,
drew

Re: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in generation of MADT

[Andrew Jones]

On Tue, May 18, 2021 at 12:27:59AM +0800, wangyanan (Y) wrote:
> Hi Drew,
> 
> On 2021/5/17 15:42, Andrew Jones wrote:
> > On Sun, May 16, 2021 at 06:28:57PM +0800, Yanan Wang wrote:
> > > When building ACPI tables regarding CPUs we should always build
> > > them for the number of possible CPUs, not the number of present
> > > CPUs. So we create gicc nodes in MADT for possible cpus and then
> > > ensure only the present CPUs are marked ENABLED. Furthermore, it
> > > also needed if we are going to support CPU hotplug in the future.
> > > 
> > > Co-developed-by: Andrew Jones <drjones@redhat.com>
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > Co-developed-by: Ying Fang <fangying1@huawei.com>
> > > Signed-off-by: Ying Fang <fangying1@huawei.com>
> > > Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > > ---
> > >   hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++----
> > >   1 file changed, 25 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index a2d8e87616..4d64aeb865 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -481,6 +481,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > >       const int *irqmap = vms->irqmap;
> > >       AcpiMadtGenericDistributor *gicd;
> > >       AcpiMadtGenericMsiFrame *gic_msi;
> > > +    MachineClass *mc = MACHINE_GET_CLASS(vms);
> > > +    const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(MACHINE(vms));
> > > +    bool pmu;
> > >       int i;
> > >       acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
> > > @@ -491,11 +494,21 @@ 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));
> > > +        /*
> > > +         * PMU should have been either implemented for all CPUs or not,
> > > +         * so we only get information from the first CPU, which could
> > > +         * represent the others.
> > > +         */
> > > +        if (i == 0) {
> > > +            pmu = arm_feature(&armcpu->env, ARM_FEATURE_PMU);
> > > +        }
> > > +        assert(!armcpu || arm_feature(&armcpu->env, ARM_FEATURE_PMU) == pmu);
> > This doesn't belong in this patch. The commit message doesn't even mention
> > it. Also, I don't think we should do this here at all. If we want to
> > ensure that all cpus have a pmu when one does, then that should be done
> > somewhere like machvirt_init(), not in ACPI generation code which doesn't
> > even run for non-ACPI VMs.
> Sorry, I should have stated the reason of this change in the commit message.
> Actually code change here and mp_affinity part below aim to make it correct
> to create gicc entries for all possible cpus.
> 
> We only initialize and realize cpuobj for present cpus in machvirt_init,
> so that we will get null ARMCPU pointer here for the non-present cpus,
> and consequently we won't able to check from "armcpu->env" for the
> non-present cpus. The same about "armcpu->mp_affinity".
> 
> That's the reason I use PMU configuration of the first cpu to represent the
> others. I assume all cpus should have a pmu when one does here since it's
> how armcpu->env is initialized. And the assert seems not needed here.
> 
> Is there any better alternative way about this?

Move the

  if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
      gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
  }

into the if (possible_cpus->cpus[i].cpu != NULL) block?

> > > +
> > >           gicc->type = ACPI_APIC_GENERIC_CPU_INTERFACE;
> > >           gicc->length = sizeof(*gicc);
> > >           if (vms->gic_version == 2) {
> > > @@ -504,11 +517,19 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > >               gicc->gicv_base_address = cpu_to_le64(memmap[VIRT_GIC_VCPU].base);
> > >           }
> > >           gicc->cpu_interface_number = cpu_to_le32(i);
> > > -        gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
> > > +        gicc->arm_mpidr = cpu_to_le64(possible_cpus->cpus[i].arch_id);
> > Hmm, I think we may have a problem. I don't think there's any guarantee
> > that possible_cpus->cpus[i].arch_id == armcpu->mp_affinity, because
> > arch_id comes from virt_cpu_mp_affinity(), which is arm_cpu_mp_affinity,
> > but with a variable cluster size, however mp_affinity comes from
> > arm_cpu_mp_affinity with a set cluster size. Also, when KVM is used,
> > then all bets are off as to what mp_affinity is.
> Right! Arch_id is initialized by virt_cpu_mp_affinity() in machvirt and then
> mp_affinity is initialized by arch_id. Here they two have the same value.
> 
> But mp_affinity will be overridden in kvm_arch_init_vcpu() when KVM is
> enabled. Here they two won't have the same value.
> > We need to add some code that ensures arch_id == mp_affinity,
> Can we also update the arch_id at the same time when we change mp_affinity?

The proper fix is to send patches to KVM enabling userspace to control
MPIDR. Otherwise we can't be sure we don't have inconsistencies in QEMU,
since some user of possible_cpus could have made decisions or copied IDs
prior to KVM vcpu init time. Now, all that said, I think
virt_cpu_mp_affinity() should be generating the same ID as KVM does, so
maybe it doesn't matter in practice right now, but we're living with the
risk that KVM could change. For now, maybe we should just sanity check
that the KVM values match the possible_cpus values and emit warnings if
they don't?

Thanks,
drew

Re: [RFC PATCH v3 3/9] hw/arm/virt: Add cpu-map to device tree

[wangyanan (Y)]

On 2021/5/18 15:46, Andrew Jones wrote:
> On Mon, May 17, 2021 at 11:00:07PM +0800, wangyanan (Y) wrote:
>> Hi Drew,
>>
>> On 2021/5/17 14:41, Andrew Jones wrote:
>>> On Sun, May 16, 2021 at 06:28:54PM +0800, Yanan Wang wrote:
>>>> From: Andrew Jones <drjones@redhat.com>
>>>>
>>>> Support device tree CPU topology descriptions.
>>>>
>>>> In accordance with the Devicetree Specification, the Linux Doc
>>>> "arm/cpus.yaml" requires that cpus and cpu nodes in the DT are
>>>> present. And we meet the requirement by creating /cpus/cpu@*
>>>> nodes for members within ms->smp.cpus.
>>>>
>>>> Correspondingly, we should also create subnodes in cpu-map for
>>>> the present cpus, each of which relates to an unique cpu node.
>>>>
>>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>>> ---
>>>>    hw/arm/virt.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 40 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>> index c07841e3a4..e5dcdebdbc 100644
>>>> --- a/hw/arm/virt.c
>>>> +++ b/hw/arm/virt.c
>>>> @@ -349,10 +349,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
>>> Rather than aligning the top line with the lower lines, we could remove
>>> the extra space from the lower lines. Or, leave the formatting as it was,
>>> by putting 'See' where 'From' was, like I did in my original patch.
>> I think I prefer removing the extra space from the lower lines, which is
>> the right thing to do.
> OK
>
>>>>         *  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
>>>> @@ -405,8 +406,46 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
>>>>                    ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
>>>>            }
>>>> +        if (!vmc->no_cpu_topology) {
>>>> +            qemu_fdt_setprop_cell(ms->fdt, nodename, "phandle",
>>>> +                                  qemu_fdt_alloc_phandle(ms->fdt));
>>>> +        }
>>>> +
>>>>            g_free(nodename);
>>>>        }
>>>> +
>>>> +    if (!vmc->no_cpu_topology) {
>>>> +        /*
>>>> +         * See Linux Documentation/devicetree/bindings/cpu/cpu-topology.txt
>>>> +         * In a SMP system, the hierarchy of CPUs is defined through four
>>>> +         * entities that are used to describe the layout of physical CPUs
>>> s/entities/levels/
>> Above comment was completely from Linux Doc cpu-topology.txt. See [1].
>> I think entities may be more reasonable than levels here, since there can be
>> multiple levels of clusters in cpu-map which makes the total not four.
> OK
>
>> [1] https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/cpu/cpu-topology.txt
>>>> +         * in the system: socket/cluster/core/thread.
>>> The comment says there are four levels including 'cluster', but there's no
>>> 'cluster' below.
>> According to Doc [1] (line 114), a socket node's child nodes must be
>> *one or more* cluster nodes which means cluster is mandatory to be
>> socket's child in DT.
>>
>> So I think maybe we should just keep the comment as-is, and change
>> the map-path from /cpus/cpu-map/socket*/cores*/threads* to
>> /cpus/cpu-map/socket*/cluster0/cores*/threads* in this patch?
> I agree. In fact, that's how I implemented it myself[1]
>
> [1] https://github.com/rhdrjones/qemu/commit/35feecdd43475608c8f55973a0c159eac4aafefd
Ok, will fix. Thanks!
> Thanks,
> drew
>
> .

Re: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in generation of MADT

[wangyanan (Y)]

Hi Drew,

On 2021/5/18 16:15, Andrew Jones wrote:
> On Tue, May 18, 2021 at 12:27:59AM +0800, wangyanan (Y) wrote:
>> Hi Drew,
>>
>> On 2021/5/17 15:42, Andrew Jones wrote:
>>> On Sun, May 16, 2021 at 06:28:57PM +0800, Yanan Wang wrote:
>>>> When building ACPI tables regarding CPUs we should always build
>>>> them for the number of possible CPUs, not the number of present
>>>> CPUs. So we create gicc nodes in MADT for possible cpus and then
>>>> ensure only the present CPUs are marked ENABLED. Furthermore, it
>>>> also needed if we are going to support CPU hotplug in the future.
>>>>
>>>> Co-developed-by: Andrew Jones <drjones@redhat.com>
>>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>>> Co-developed-by: Ying Fang <fangying1@huawei.com>
>>>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>>>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>>> ---
>>>>    hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++----
>>>>    1 file changed, 25 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>>> index a2d8e87616..4d64aeb865 100644
>>>> --- a/hw/arm/virt-acpi-build.c
>>>> +++ b/hw/arm/virt-acpi-build.c
>>>> @@ -481,6 +481,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>>>        const int *irqmap = vms->irqmap;
>>>>        AcpiMadtGenericDistributor *gicd;
>>>>        AcpiMadtGenericMsiFrame *gic_msi;
>>>> +    MachineClass *mc = MACHINE_GET_CLASS(vms);
>>>> +    const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(MACHINE(vms));
>>>> +    bool pmu;
>>>>        int i;
>>>>        acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
>>>> @@ -491,11 +494,21 @@ 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));
>>>> +        /*
>>>> +         * PMU should have been either implemented for all CPUs or not,
>>>> +         * so we only get information from the first CPU, which could
>>>> +         * represent the others.
>>>> +         */
>>>> +        if (i == 0) {
>>>> +            pmu = arm_feature(&armcpu->env, ARM_FEATURE_PMU);
>>>> +        }
>>>> +        assert(!armcpu || arm_feature(&armcpu->env, ARM_FEATURE_PMU) == pmu);
>>> This doesn't belong in this patch. The commit message doesn't even mention
>>> it. Also, I don't think we should do this here at all. If we want to
>>> ensure that all cpus have a pmu when one does, then that should be done
>>> somewhere like machvirt_init(), not in ACPI generation code which doesn't
>>> even run for non-ACPI VMs.
>> Sorry, I should have stated the reason of this change in the commit message.
>> Actually code change here and mp_affinity part below aim to make it correct
>> to create gicc entries for all possible cpus.
>>
>> We only initialize and realize cpuobj for present cpus in machvirt_init,
>> so that we will get null ARMCPU pointer here for the non-present cpus,
>> and consequently we won't able to check from "armcpu->env" for the
>> non-present cpus. The same about "armcpu->mp_affinity".
>>
>> That's the reason I use PMU configuration of the first cpu to represent the
>> others. I assume all cpus should have a pmu when one does here since it's
>> how armcpu->env is initialized. And the assert seems not needed here.
>>
>> Is there any better alternative way about this?
> Move the
>
>    if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
>        gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
>    }
>
> into the if (possible_cpus->cpus[i].cpu != NULL) block?
We can. But this will only ensure that we initialize 
gicc->performance_interrupt
for enabled GICC entries but not the disabled ones.
>>>> +
>>>>            gicc->type = ACPI_APIC_GENERIC_CPU_INTERFACE;
>>>>            gicc->length = sizeof(*gicc);
>>>>            if (vms->gic_version == 2) {
>>>> @@ -504,11 +517,19 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>>>                gicc->gicv_base_address = cpu_to_le64(memmap[VIRT_GIC_VCPU].base);
>>>>            }
>>>>            gicc->cpu_interface_number = cpu_to_le32(i);
>>>> -        gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
>>>> +        gicc->arm_mpidr = cpu_to_le64(possible_cpus->cpus[i].arch_id);
>>> Hmm, I think we may have a problem. I don't think there's any guarantee
>>> that possible_cpus->cpus[i].arch_id == armcpu->mp_affinity, because
>>> arch_id comes from virt_cpu_mp_affinity(), which is arm_cpu_mp_affinity,
>>> but with a variable cluster size, however mp_affinity comes from
>>> arm_cpu_mp_affinity with a set cluster size. Also, when KVM is used,
>>> then all bets are off as to what mp_affinity is.
>> Right! Arch_id is initialized by virt_cpu_mp_affinity() in machvirt and then
>> mp_affinity is initialized by arch_id. Here they two have the same value.
>>
>> But mp_affinity will be overridden in kvm_arch_init_vcpu() when KVM is
>> enabled. Here they two won't have the same value.
>>> We need to add some code that ensures arch_id == mp_affinity,
>> Can we also update the arch_id at the same time when we change mp_affinity?
> The proper fix is to send patches to KVM enabling userspace to control
> MPIDR. Otherwise we can't be sure we don't have inconsistencies in QEMU,
> since some user of possible_cpus could have made decisions or copied IDs
> prior to KVM vcpu init time. Now, all that said, I think
> virt_cpu_mp_affinity() should be generating the same ID as KVM does, so
> maybe it doesn't matter in practice right now, but we're living with the
> risk that KVM could change. For now, maybe we should just sanity check
> that the KVM values match the possible_cpus values and emit warnings if
> they don't?
I think it may not so reasonable to emit warnings if they don't match, on
the contrary we should ensure they will match even when KVM changes.

Now virt_cpu_mp_affinity() is only called by virt_possible_cpu_arch_ids() to
initialize possible_cpus, so an idea is that we can move the stuff of 
resetting
"cpu->mp_affinity" from kvm_arch_init_vcpu() to virt_cpu_mp_affinity() to
initialize arch_id. So that we can ensure mp_affinity only comes from 
arch_id
and won't change later. Can it work?

BTW, I plan to pack patch 4-6 into a separate patchset and repost it until
we have a mature solution for the probelm, that's also Salil's suggestion.
Is it appropriate?

Thanks,
Yanan
> Thanks,
> drew
>
> .

Re: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in generation of MADT

[wangyanan (Y)]

On 2021/5/18 14:47, Salil Mehta wrote:
>> From: wangyanan (Y)
>> Sent: Tuesday, May 18, 2021 6:03 AM
>>
>> Hi Salil,
>>
>> On 2021/5/18 1:07, Salil Mehta wrote:
>>>> From: Qemu-arm
>> [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
>>>> On Behalf Of Yanan Wang
>>>> Sent: Sunday, May 16, 2021 11:29 AM
>>>> To: Peter Maydell <peter.maydell@linaro.org>; Andrew Jones
>>>> <drjones@redhat.com>; Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov
>>>> <imammedo@redhat.com>; Shannon Zhao <shannon.zhaosl@gmail.com>; Alistair
>>>> Francis <alistair.francis@wdc.com>; David Gibson
>>>> <david@gibson.dropbear.id.au>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
>>>> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; zhukeqian
>>>> <zhukeqian1@huawei.com>; yangyicong <yangyicong@huawei.com>; Zengtao (B)
>>>> <prime.zeng@hisilicon.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>;
>>>> yuzenghui <yuzenghui@huawei.com>; Paolo Bonzini <pbonzini@redhat.com>;
>>>> Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> Subject: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in
>>>> generation of MADT
>>>>
>>>> When building ACPI tables regarding CPUs we should always build
>>>> them for the number of possible CPUs, not the number of present
>>>> CPUs. So we create gicc nodes in MADT for possible cpus and then
>>>> ensure only the present CPUs are marked ENABLED. Furthermore, it
>>>> also needed if we are going to support CPU hotplug in the future.
>>> Hi Yanan,
>>> Yes, these changes are part of the QEMU patch-set I floated last year.
>>>
>>> Link: https://www.mail-archive.com/qemu-devel@nongnu.org/msg712018.html
>> Yes, I noticed this. Thanks!
>>> Perhaps I am missing something, but how this patch is related to the vcpu
>>> topology support?
>> No related actually. But this patch together with patch 5 aim to provide
>> complete information (all cpus including enabled and the others) to guest,
>> which will be more consistent with requirement in ACPI spec.
>
> Well, if it is not related to the cpu topology support then this and other
> similar patches included with the same line of thought should not be
> part of this patch-set.
>
> I am already working with ARM folks in this regard.
Hi Salil,

I'm planning to pack this part into a separate patchset and may repost
it another time, given that there are still some issues to solve.

Thanks,
Yanan
> Thanks
>
>> We don't consider cpu hotplug at all in this patch, but it indeed pave way
>> for cpu hotplug in the future.
>>
>> Thanks,
>> Yanan
>>> Thanks
>>>
>>>> Co-developed-by: Andrew Jones <drjones@redhat.com>
>>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>>> Co-developed-by: Ying Fang <fangying1@huawei.com>
>>>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>>>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>>> ---
>>>>    hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++----
>>>>    1 file changed, 25 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>>> index a2d8e87616..4d64aeb865 100644
>>>> --- a/hw/arm/virt-acpi-build.c
>>>> +++ b/hw/arm/virt-acpi-build.c
>>>> @@ -481,6 +481,9 @@ build_madt(GArray *table_data, BIOSLinker *linker,
>>>> VirtMachineState *vms)
>>>>        const int *irqmap = vms->irqmap;
>>>>        AcpiMadtGenericDistributor *gicd;
>>>>        AcpiMadtGenericMsiFrame *gic_msi;
>>>> +    MachineClass *mc = MACHINE_GET_CLASS(vms);
>>>> +    const CPUArchIdList *possible_cpus =
>>>> mc->possible_cpu_arch_ids(MACHINE(vms));
>>>> +    bool pmu;
>>>>        int i;
>>>>
>>>>        acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
>>>> @@ -491,11 +494,21 @@ 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));
>>>>
>>>> +        /*
>>>> +         * PMU should have been either implemented for all CPUs or not,
>>>> +         * so we only get information from the first CPU, which could
>>>> +         * represent the others.
>>>> +         */
>>>> +        if (i == 0) {
>>>> +            pmu = arm_feature(&armcpu->env, ARM_FEATURE_PMU);
>>>> +        }
>>>> +        assert(!armcpu || arm_feature(&armcpu->env, ARM_FEATURE_PMU) ==
>> pmu);
>>>> +
>>>>            gicc->type = ACPI_APIC_GENERIC_CPU_INTERFACE;
>>>>            gicc->length = sizeof(*gicc);
>>>>            if (vms->gic_version == 2) {
>>>> @@ -504,11 +517,19 @@ build_madt(GArray *table_data, BIOSLinker *linker,
>>>> VirtMachineState *vms)
>>>>                gicc->gicv_base_address =
>>>> cpu_to_le64(memmap[VIRT_GIC_VCPU].base);
>>>>            }
>>>>            gicc->cpu_interface_number = cpu_to_le32(i);
>>>> -        gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
>>>> +        gicc->arm_mpidr = cpu_to_le64(possible_cpus->cpus[i].arch_id);
>>>>            gicc->uid = cpu_to_le32(i);
>>>> -        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
>>>>
>>>> -        if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
>>>> +        /*
>>>> +         * ACPI spec says that LAPIC entry for non present CPU may be
>>>> +         * omitted from MADT or it must be marked as disabled. Here we
>>>> +         * choose to also keep the disabled ones in MADT.
>>>> +         */
>>>> +        if (possible_cpus->cpus[i].cpu != NULL) {
>>>> +            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
>>>> +        }
>>>> +
>>>> +        if (pmu) {
>>>>                gicc->performance_interrupt =
>> cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
>>>>            }
>>>>            if (vms->virt) {
>>>> --
>>>> 2.19.1
>>>>
>>> .

Re: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in generation of MADT

[wangyanan (Y)]

On 2021/5/18 14:47, Salil Mehta wrote:
>> From: wangyanan (Y)
>> Sent: Tuesday, May 18, 2021 6:03 AM
>>
>> Hi Salil,
>>
>> On 2021/5/18 1:07, Salil Mehta wrote:
>>>> From: Qemu-arm
>> [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
>>>> On Behalf Of Yanan Wang
>>>> Sent: Sunday, May 16, 2021 11:29 AM
>>>> To: Peter Maydell <peter.maydell@linaro.org>; Andrew Jones
>>>> <drjones@redhat.com>; Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov
>>>> <imammedo@redhat.com>; Shannon Zhao <shannon.zhaosl@gmail.com>; Alistair
>>>> Francis <alistair.francis@wdc.com>; David Gibson
>>>> <david@gibson.dropbear.id.au>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
>>>> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; zhukeqian
>>>> <zhukeqian1@huawei.com>; yangyicong <yangyicong@huawei.com>; Zengtao (B)
>>>> <prime.zeng@hisilicon.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>;
>>>> yuzenghui <yuzenghui@huawei.com>; Paolo Bonzini <pbonzini@redhat.com>;
>>>> Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> Subject: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in
>>>> generation of MADT
>>>>
>>>> When building ACPI tables regarding CPUs we should always build
>>>> them for the number of possible CPUs, not the number of present
>>>> CPUs. So we create gicc nodes in MADT for possible cpus and then
>>>> ensure only the present CPUs are marked ENABLED. Furthermore, it
>>>> also needed if we are going to support CPU hotplug in the future.
>>> Hi Yanan,
>>> Yes, these changes are part of the QEMU patch-set I floated last year.
>>>
>>> Link: https://www.mail-archive.com/qemu-devel@nongnu.org/msg712018.html
>> Yes, I noticed this. Thanks!
>>> Perhaps I am missing something, but how this patch is related to the vcpu
>>> topology support?
>> No related actually. But this patch together with patch 5 aim to provide
>> complete information (all cpus including enabled and the others) to guest,
>> which will be more consistent with requirement in ACPI spec.
>
> Well, if it is not related to the cpu topology support then this and other
> similar patches included with the same line of thought should not be
> part of this patch-set.
>
> I am already working with ARM folks in this regard.
Hi Salil,

I'm planning to pack this part into a separate patchset and may repost
it another time, given that there are still some issues to solve.

Thanks,
Yanan
> Thanks
>
>> We don't consider cpu hotplug at all in this patch, but it indeed pave way
>> for cpu hotplug in the future.
>>
>> Thanks,
>> Yanan
>>> Thanks
>>>
>>>> Co-developed-by: Andrew Jones <drjones@redhat.com>
>>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>>> Co-developed-by: Ying Fang <fangying1@huawei.com>
>>>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>>>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>>> ---
>>>>    hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++----
>>>>    1 file changed, 25 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>>> index a2d8e87616..4d64aeb865 100644
>>>> --- a/hw/arm/virt-acpi-build.c
>>>> +++ b/hw/arm/virt-acpi-build.c
>>>> @@ -481,6 +481,9 @@ build_madt(GArray *table_data, BIOSLinker *linker,
>>>> VirtMachineState *vms)
>>>>        const int *irqmap = vms->irqmap;
>>>>        AcpiMadtGenericDistributor *gicd;
>>>>        AcpiMadtGenericMsiFrame *gic_msi;
>>>> +    MachineClass *mc = MACHINE_GET_CLASS(vms);
>>>> +    const CPUArchIdList *possible_cpus =
>>>> mc->possible_cpu_arch_ids(MACHINE(vms));
>>>> +    bool pmu;
>>>>        int i;
>>>>
>>>>        acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
>>>> @@ -491,11 +494,21 @@ 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));
>>>>
>>>> +        /*
>>>> +         * PMU should have been either implemented for all CPUs or not,
>>>> +         * so we only get information from the first CPU, which could
>>>> +         * represent the others.
>>>> +         */
>>>> +        if (i == 0) {
>>>> +            pmu = arm_feature(&armcpu->env, ARM_FEATURE_PMU);
>>>> +        }
>>>> +        assert(!armcpu || arm_feature(&armcpu->env, ARM_FEATURE_PMU) ==
>> pmu);
>>>> +
>>>>            gicc->type = ACPI_APIC_GENERIC_CPU_INTERFACE;
>>>>            gicc->length = sizeof(*gicc);
>>>>            if (vms->gic_version == 2) {
>>>> @@ -504,11 +517,19 @@ build_madt(GArray *table_data, BIOSLinker *linker,
>>>> VirtMachineState *vms)
>>>>                gicc->gicv_base_address =
>>>> cpu_to_le64(memmap[VIRT_GIC_VCPU].base);
>>>>            }
>>>>            gicc->cpu_interface_number = cpu_to_le32(i);
>>>> -        gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
>>>> +        gicc->arm_mpidr = cpu_to_le64(possible_cpus->cpus[i].arch_id);
>>>>            gicc->uid = cpu_to_le32(i);
>>>> -        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
>>>>
>>>> -        if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
>>>> +        /*
>>>> +         * ACPI spec says that LAPIC entry for non present CPU may be
>>>> +         * omitted from MADT or it must be marked as disabled. Here we
>>>> +         * choose to also keep the disabled ones in MADT.
>>>> +         */
>>>> +        if (possible_cpus->cpus[i].cpu != NULL) {
>>>> +            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
>>>> +        }
>>>> +
>>>> +        if (pmu) {
>>>>                gicc->performance_interrupt =
>> cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
>>>>            }
>>>>            if (vms->virt) {
>>>> --
>>>> 2.19.1
>>>>
>>> .

Re: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in generation of MADT

[Andrew Jones]

On Tue, May 18, 2021 at 07:47:24PM +0800, wangyanan (Y) wrote:
> > > Is there any better alternative way about this?
> > Move the
> > 
> >    if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
> >        gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
> >    }
> > 
> > into the if (possible_cpus->cpus[i].cpu != NULL) block?
> We can. But this will only ensure that we initialize
> gicc->performance_interrupt
> for enabled GICC entries but not the disabled ones.

I'd write a comment and leave that problem for the hot plug support to
solve.

> > > Can we also update the arch_id at the same time when we change mp_affinity?
> > The proper fix is to send patches to KVM enabling userspace to control
> > MPIDR. Otherwise we can't be sure we don't have inconsistencies in QEMU,
> > since some user of possible_cpus could have made decisions or copied IDs
> > prior to KVM vcpu init time. Now, all that said, I think
> > virt_cpu_mp_affinity() should be generating the same ID as KVM does, so
> > maybe it doesn't matter in practice right now, but we're living with the
> > risk that KVM could change. For now, maybe we should just sanity check
> > that the KVM values match the possible_cpus values and emit warnings if
> > they don't?
> I think it may not so reasonable to emit warnings if they don't match, on
> the contrary we should ensure they will match even when KVM changes.
> 
> Now virt_cpu_mp_affinity() is only called by virt_possible_cpu_arch_ids() to
> initialize possible_cpus, so an idea is that we can move the stuff of
> resetting
> "cpu->mp_affinity" from kvm_arch_init_vcpu() to virt_cpu_mp_affinity() to
> initialize arch_id. So that we can ensure mp_affinity only comes from
> arch_id
> and won't change later. Can it work?

No. virt_cpu_mp_affinity can run way before cpu realize, but
kvm_arch_init_vcpu runs at cpu realize time.

> 
> BTW, I plan to pack patch 4-6 into a separate patchset and repost it until
> we have a mature solution for the probelm, that's also Salil's suggestion.
> Is it appropriate?

Well, you still need to set the enabled flag in the GICC table and you'll
still need to come up with a solution for user input of cpus < maxcpus,
but if your solution to the latter is good, then whatever you like.

Thanks,
drew

RE: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members

[Salil Mehta]

> From: Andrew Jones [mailto:drjones@redhat.com]
> Sent: Tuesday, May 18, 2021 8:50 AM
> 
> On Tue, May 18, 2021 at 07:04:51AM +0000, Salil Mehta wrote:
> > > From: wangyanan (Y)
> > > Sent: Tuesday, May 18, 2021 5:43 AM
> > >
> > > Hi Salil,
> > >
> > > On 2021/5/18 4:48, Salil Mehta wrote:
> > > >> From: Qemu-arm
> > > [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
> > > >> On Behalf Of Yanan Wang
> > > >> Sent: Sunday, May 16, 2021 11:29 AM
> > > >> To: Peter Maydell <peter.maydell@linaro.org>; Andrew Jones
> > > >> <drjones@redhat.com>; Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov
> > > >> <imammedo@redhat.com>; Shannon Zhao <shannon.zhaosl@gmail.com>; Alistair
> > > >> Francis <alistair.francis@wdc.com>; David Gibson
> > > >> <david@gibson.dropbear.id.au>; qemu-devel@nongnu.org;
> qemu-arm@nongnu.org
> > > >> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; zhukeqian
> > > >> <zhukeqian1@huawei.com>; yangyicong <yangyicong@huawei.com>; Zengtao (B)
> > > >> <prime.zeng@hisilicon.com>; Wanghaibin (D)
> <wanghaibin.wang@huawei.com>;
> > > >> yuzenghui <yuzenghui@huawei.com>; Paolo Bonzini <pbonzini@redhat.com>;
> > > >> Philippe Mathieu-Daudé <philmd@redhat.com>
> > > >> Subject: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members
> > > >>
> > > >> We create and initialize a cpuobj for each present cpu in
> > > >> machvirt_init(). Now we also initialize the cpu member of
> > > >> structure CPUArchId for each present cpu in the function.
> > > > [...]
> > > >
> > > >>           qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> > > >> +
> > > >> +        /*
> > > >> +         * As ARM cpu hotplug is not supported yet, we initialize
> > > >> +         * the present cpu members here.
> > > >> +         */
> > > >> +        machine->possible_cpus->cpus[n].cpu = cpuobj;
> > > >
> > > > when vcpu Hotplug is not supported yet, what necessitates this change now?
> > > >
> > > The initialization will gives a way to determine whether a CPU is
> > > present or not.
> > > At least, for now it will be used when generating ACPI tables, e.g.
> > > DSDT, MADT.
> > > See patch 5 and 6.
> >
> > yes,  but why do you require it now as part of the vcpu topology change?
> >
> > As-far-as-i-can-see, PPTT table changes(part of patch 5/9) do not require
> > this change. Change in Patch 5/9 has also been done in anticipation of
> > some future requirement(vcpu Hotplug?).
> >
> > Please correct me here if I am wrong?
> >
> 
> Hi Salil,
> 
> The problem is that we've never required smp.cpus == smp.maxcpus, so
> a user could have smp.cpus < smp.maxcpus. We want the topology to match
> maxcpus, but only enable cpus. However, if you think we should just not
> allow cpus < maxcpus until hot plug is sorted out, then we could discuss
> a way of trying to enforce cpus == maxcpus, but I'm not sure how we can
> without breaking existing command lines.


Hi Andrew,
Ideally, if the vcpu Hotplug is not supported the check in the smp_parse()
should impose (cpus == maxcpus). This as of now is just a warning of invalid
configuration I think. Beside this does not breaks any prior usages which you
suggested might happen?

Again, this is not a blocking issue from my side but just a humble suggestion.
You might want to take a call on this :)


Thanks
Salil.

RE: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members

[Salil Mehta]

> From: Andrew Jones [mailto:drjones@redhat.com]
> Sent: Tuesday, May 18, 2021 8:50 AM
> 
> On Tue, May 18, 2021 at 07:04:51AM +0000, Salil Mehta wrote:
> > > From: wangyanan (Y)
> > > Sent: Tuesday, May 18, 2021 5:43 AM
> > >
> > > Hi Salil,
> > >
> > > On 2021/5/18 4:48, Salil Mehta wrote:
> > > >> From: Qemu-arm
> > > [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
> > > >> On Behalf Of Yanan Wang
> > > >> Sent: Sunday, May 16, 2021 11:29 AM
> > > >> To: Peter Maydell <peter.maydell@linaro.org>; Andrew Jones
> > > >> <drjones@redhat.com>; Michael S . Tsirkin <mst@redhat.com>; Igor Mammedov
> > > >> <imammedo@redhat.com>; Shannon Zhao <shannon.zhaosl@gmail.com>; Alistair
> > > >> Francis <alistair.francis@wdc.com>; David Gibson
> > > >> <david@gibson.dropbear.id.au>; qemu-devel@nongnu.org;
> qemu-arm@nongnu.org
> > > >> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; zhukeqian
> > > >> <zhukeqian1@huawei.com>; yangyicong <yangyicong@huawei.com>; Zengtao (B)
> > > >> <prime.zeng@hisilicon.com>; Wanghaibin (D)
> <wanghaibin.wang@huawei.com>;
> > > >> yuzenghui <yuzenghui@huawei.com>; Paolo Bonzini <pbonzini@redhat.com>;
> > > >> Philippe Mathieu-Daudé <philmd@redhat.com>
> > > >> Subject: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members
> > > >>
> > > >> We create and initialize a cpuobj for each present cpu in
> > > >> machvirt_init(). Now we also initialize the cpu member of
> > > >> structure CPUArchId for each present cpu in the function.
> > > > [...]
> > > >
> > > >>           qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> > > >> +
> > > >> +        /*
> > > >> +         * As ARM cpu hotplug is not supported yet, we initialize
> > > >> +         * the present cpu members here.
> > > >> +         */
> > > >> +        machine->possible_cpus->cpus[n].cpu = cpuobj;
> > > >
> > > > when vcpu Hotplug is not supported yet, what necessitates this change now?
> > > >
> > > The initialization will gives a way to determine whether a CPU is
> > > present or not.
> > > At least, for now it will be used when generating ACPI tables, e.g.
> > > DSDT, MADT.
> > > See patch 5 and 6.
> >
> > yes,  but why do you require it now as part of the vcpu topology change?
> >
> > As-far-as-i-can-see, PPTT table changes(part of patch 5/9) do not require
> > this change. Change in Patch 5/9 has also been done in anticipation of
> > some future requirement(vcpu Hotplug?).
> >
> > Please correct me here if I am wrong?
> >
> 
> Hi Salil,
> 
> The problem is that we've never required smp.cpus == smp.maxcpus, so
> a user could have smp.cpus < smp.maxcpus. We want the topology to match
> maxcpus, but only enable cpus. However, if you think we should just not
> allow cpus < maxcpus until hot plug is sorted out, then we could discuss
> a way of trying to enforce cpus == maxcpus, but I'm not sure how we can
> without breaking existing command lines.


Hi Andrew,
Ideally, if the vcpu Hotplug is not supported the check in the smp_parse()
should impose (cpus == maxcpus). This as of now is just a warning of invalid
configuration I think. Beside this does not breaks any prior usages which you
suggested might happen?

Again, this is not a blocking issue from my side but just a humble suggestion.
You might want to take a call on this :)


Thanks
Salil.