| Message ID | 20220323072438.71815-5-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/virt: Fix CPU's default NUMA node ID | expand |
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. > 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 */
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. 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 */ >
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 */ > > >
Hi Igor, On 3/30/22 8:52 PM, Igor Mammedov wrote: > On Sat, 26 Mar 2022 03:08:19 +0800 > Gavin Shan <gshan@redhat.com> wrote: >> 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. > 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 <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 */ >>> >> >
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 */
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 Signed-off-by: Gavin Shan <gshan@redhat.com> --- hw/arm/virt-acpi-build.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)