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 <drjo...@redhat.com> > Signed-off-by: Andrew Jones <drjo...@redhat.com> > Co-developed-by: Ying Fang <fangyi...@huawei.com> > Signed-off-by: Ying Fang <fangyi...@huawei.com> > Co-developed-by: Yanan Wang <wangyana...@huawei.com> > Signed-off-by: Yanan Wang <wangyana...@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