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

[Andrew Jones]

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