Re: [PATCH v3 4/4] hw/arm/virt: Unify ACPI processor ID in MADT and SRAT table

[Gavin Shan]

Hi Igor,

On 3/30/22 8:52 PM, Igor Mammedov wrote:
On Sat, 26 Mar 2022 03:08:19 +0800
Gavin Shan <gs...@redhat.com> wrote:
On 3/25/22 10:00 PM, Igor Mammedov wrote:
On Wed, 23 Mar 2022 15:24:38 +0800
Gavin Shan <gs...@redhat.com> wrote:
   
The value of the following field has been used in ACPI PPTT table
to identify the corresponding processor. This takes the same field
as the ACPI processor ID in MADT and SRAT tables.
    ms->possible_cpus->cpus[i].props.thread_id

thread-id could be something different than ACPI processor ID
(to be discussed in the first patch).

I'd prefer to keep both decoupled, i.e. use [0 - possible_cpus->len)
for ACPI processor ID as it's done with x86 madt/srat and ACPI CPU
hotplug code. So we could reuse at least the later when implementing
CPU hotplug for arm/virt and it's good from consistency point of view.
   

Yeah, I think so, thread-id and ACPI processor UID could be different.
thread IDs could be overlapped on multiple CPUs, who belongs to different
socket/die/core. However, ACPI processor UID should be unique for the
CPU in the whole system.

If you agree, Lets use [0 - possible_cpus->len] in next respin.

Agreed.


Thanks for your confirm and the changes, as I mentioned before, will be
included in v4.
 
What I
need to do is dropping PATCH[4/4] and replacing @thread_id with @n in
build_pptt() of PATCH[3/4].


Thanks,
Gavin


Signed-off-by: Gavin Shan <gs...@redhat.com>
---
   hw/arm/virt-acpi-build.c | 12 +++++++++---
   1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 449fab0080..7fedb56eea 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -534,13 +534,16 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
VirtMachineState *vms)
   
       for (i = 0; i < cpu_list->len; ++i) {
           uint32_t nodeid = cpu_list->cpus[i].props.node_id;
+        uint32_t thread_id = cpu_list->cpus[i].props.thread_id;
+
           /*
            * 5.2.16.4 GICC Affinity Structure
            */
           build_append_int_noprefix(table_data, 3, 1);      /* Type */
           build_append_int_noprefix(table_data, 18, 1);     /* Length */
           build_append_int_noprefix(table_data, nodeid, 4); /* Proximity 
Domain */
-        build_append_int_noprefix(table_data, i, 4); /* ACPI Processor UID */
+        build_append_int_noprefix(table_data,
+                                  thread_id, 4); /* ACPI Processor UID */
           /* Flags, Table 5-76 */
           build_append_int_noprefix(table_data, 1 /* Enabled */, 4);
           build_append_int_noprefix(table_data, 0, 4); /* Clock Domain */
@@ -704,6 +707,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, 
VirtMachineState *vms)
   {
       int i;
       VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
+    MachineState *ms = MACHINE(vms);
       const MemMapEntry *memmap = vms->memmap;
       AcpiTable table = { .sig = "APIC", .rev = 3, .oem_id = vms->oem_id,
                           .oem_table_id = vms->oem_table_id };
@@ -725,8 +729,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, 
VirtMachineState *vms)
       build_append_int_noprefix(table_data, vms->gic_version, 1);
       build_append_int_noprefix(table_data, 0, 3);   /* Reserved */
   
-    for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
+    for (i = 0; i < ms->smp.cpus; i++) {
           ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
+        uint32_t thread_id = ms->possible_cpus->cpus[i].props.thread_id;
           uint64_t physical_base_address = 0, gich = 0, gicv = 0;
           uint32_t vgic_interrupt = vms->virt ? PPI(ARCH_GIC_MAINT_IRQ) : 0;
           uint32_t pmu_interrupt = arm_feature(&armcpu->env, ARM_FEATURE_PMU) ?
@@ -743,7 +748,8 @@ build_madt(GArray *table_data, BIOSLinker *linker, 
VirtMachineState *vms)
           build_append_int_noprefix(table_data, 76, 1);   /* Length */
           build_append_int_noprefix(table_data, 0, 2);    /* Reserved */
           build_append_int_noprefix(table_data, i, 4);    /* GIC ID */
-        build_append_int_noprefix(table_data, i, 4);    /* ACPI Processor UID 
*/
+        build_append_int_noprefix(table_data,
+                                  thread_id, 4);        /* ACPI Processor UID 
*/
           /* Flags */
           build_append_int_noprefix(table_data, 1, 4);    /* Enabled */
           /* Parking Protocol Version */