On Thu, Feb 25, 2021 at 04:56:25PM +0800, Ying Fang wrote: > When building ACPI tables regarding CPUs we should always build > them for the number of possible CPUs, not the number of present > CPUs. We then ensure only the present CPUs are enabled in madt. > Furthermore, it is also needed if we are going to support CPU > hotplug in the future. > > This patch is a rework based on Andrew Jones's contribution at > https://lists.gnu.org/archive/html/qemu-arm/2018-07/msg00076.html > > Signed-off-by: Ying Fang <fangyi...@huawei.com> > --- > hw/arm/virt-acpi-build.c | 14 ++++++++++---- > hw/arm/virt.c | 2 ++ > 2 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index f9c9df916c..bb91152fe2 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -61,13 +61,16 @@ > > static void acpi_dsdt_add_cpus(Aml *scope, VirtMachineState *vms) > { > - MachineState *ms = MACHINE(vms); > + CPUArchIdList *possible_cpus = MACHINE(vms)->possible_cpus; > uint16_t i; > > - for (i = 0; i < ms->smp.cpus; i++) { > + for (i = 0; i < possible_cpus->len; i++) {
Switching from ms->smp.cpus to possible_cpus->len makes some sense here, since we're going to be using possible_cpus->cpus[] inside the loop. Otherwise, I'd prefer switching to ms->smp.max_cpus. > Aml *dev = aml_device("C%.03X", i); > aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007"))); > aml_append(dev, aml_name_decl("_UID", aml_int(i))); > + if (possible_cpus->cpus[i].cpu == NULL) { > + aml_append(dev, aml_name_decl("_STA", aml_int(0))); > + } > aml_append(scope, dev); > } > } > @@ -479,6 +482,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, > VirtMachineState *vms) > const int *irqmap = vms->irqmap; > AcpiMadtGenericDistributor *gicd; > AcpiMadtGenericMsiFrame *gic_msi; > + CPUArchIdList *possible_cpus = MACHINE(vms)->possible_cpus; > int i; > > acpi_data_push(table_data, sizeof(AcpiMultipleApicTable)); > @@ -489,7 +493,7 @@ 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++) { Same comment as above. > AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data, > sizeof(*gicc)); > ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i)); > @@ -504,7 +508,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, > VirtMachineState *vms) > gicc->cpu_interface_number = cpu_to_le32(i); > gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity); > gicc->uid = cpu_to_le32(i); > - gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED); > + if (possible_cpus->cpus[i].cpu != NULL) { > + gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED); > + } > > if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) { > gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ)); > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index c133b342b8..75659502e2 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -2047,6 +2047,8 @@ static void machvirt_init(MachineState *machine) > > qdev_realize(DEVICE(cpuobj), NULL, &error_fatal); > object_unref(cpuobj); > + /* Initialize cpu member here since cpu hotplug is not supported yet > */ > + machine->possible_cpus->cpus[n].cpu = cpuobj; This feels like we're violating a possible_cpus abstraction. Igor? Also, it's using cpuobj after unrefing it. > } > fdt_add_timer_nodes(vms); > fdt_add_cpu_nodes(vms); > -- > 2.23.0 > > Thanks, drew