> 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:[email protected]]
> >> On Behalf Of Yanan Wang
> >> Sent: Sunday, May 16, 2021 11:29 AM
> >> To: Peter Maydell <[email protected]>; Andrew Jones
> >> <[email protected]>; Michael S . Tsirkin <[email protected]>; Igor Mammedov
> >> <[email protected]>; Shannon Zhao <[email protected]>; Alistair
> >> Francis <[email protected]>; David Gibson
> >> <[email protected]>; [email protected]; [email protected]
> >> Cc: Song Bao Hua (Barry Song) <[email protected]>; zhukeqian
> >> <[email protected]>; yangyicong <[email protected]>; Zengtao (B)
> >> <[email protected]>; Wanghaibin (D) <[email protected]>;
> >> yuzenghui <[email protected]>; Paolo Bonzini <[email protected]>;
> >> Philippe Mathieu-Daudé <[email protected]>
> >> 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/[email protected]/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 <[email protected]>
> >> Signed-off-by: Andrew Jones <[email protected]>
> >> Co-developed-by: Ying Fang <[email protected]>
> >> Signed-off-by: Ying Fang <[email protected]>
> >> Co-developed-by: Yanan Wang <[email protected]>
> >> Signed-off-by: Yanan Wang <[email protected]>
> >> ---
> >> 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
> >>
> > .
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:[email protected]]
>>>> On Behalf Of Yanan Wang
>>>> Sent: Sunday, May 16, 2021 11:29 AM
>>>> To: Peter Maydell <[email protected]>; Andrew Jones
>>>> <[email protected]>; Michael S . Tsirkin <[email protected]>; Igor Mammedov
>>>> <[email protected]>; Shannon Zhao <[email protected]>; Alistair
>>>> Francis <[email protected]>; David Gibson
>>>> <[email protected]>; [email protected]; [email protected]
>>>> Cc: Song Bao Hua (Barry Song) <[email protected]>; zhukeqian
>>>> <[email protected]>; yangyicong <[email protected]>; Zengtao (B)
>>>> <[email protected]>; Wanghaibin (D) <[email protected]>;
>>>> yuzenghui <[email protected]>; Paolo Bonzini <[email protected]>;
>>>> Philippe Mathieu-Daudé <[email protected]>
>>>> 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/[email protected]/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 <[email protected]>
>>>> Signed-off-by: Andrew Jones <[email protected]>
>>>> Co-developed-by: Ying Fang <[email protected]>
>>>> Signed-off-by: Ying Fang <[email protected]>
>>>> Co-developed-by: Yanan Wang <[email protected]>
>>>> Signed-off-by: Yanan Wang <[email protected]>
>>>> ---
>>>> 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
>>>>
>>> .