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

[Igor Mammedov]

On Sat, 26 Mar 2022 03:08:19 +0800
Gavin Shan <gshan@redhat.com> wrote:

> Hi Igor,
> 
> On 3/25/22 10:00 PM, Igor Mammedov wrote:
> > On Wed, 23 Mar 2022 15:24:38 +0800
> > Gavin Shan <gshan@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.

> 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 <gshan@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 */  
> >   
>