diff mbox series

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

Message ID 20210516102900.28036-7-wangyanan55@huawei.com
State New
Headers show
Series hw/arm/virt: Introduce cpu topology support | expand

Commit Message

wangyanan (Y) May 16, 2021, 10:28 a.m. UTC 
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(-)

Comments

Andrew Jones May 17, 2021, 7:42 a.m. UTC | #1 
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
wangyanan (Y) May 17, 2021, 4:27 p.m. UTC | #2 
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
>
> .
Salil Mehta May 17, 2021, 5:07 p.m. UTC | #3 
> 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
>
wangyanan (Y) May 18, 2021, 5:02 a.m. UTC | #4 
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
>>
> .
Salil Mehta May 18, 2021, 6:47 a.m. UTC | #5 
> 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
> >>
> > .
Andrew Jones May 18, 2021, 8:15 a.m. UTC | #6 
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
wangyanan (Y) May 18, 2021, 11:47 a.m. UTC | #7 
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
>
> .
wangyanan (Y) May 18, 2021, 11:58 a.m. UTC | #8 
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
>>>>
>>> .
Andrew Jones May 18, 2021, 1:40 p.m. UTC | #9 
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
diff mbox series

Patch

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) {