Re: [Qemu-devel] [PATCH v4 08/10] pc: acpi: drop cpu->found_cpus bitmap

[Igor Mammedov]

On Thu, 3 Mar 2016 13:33:29 +0200
Marcel Apfelbaum <address@hidden> wrote:

> On 02/26/2016 03:59 PM, Igor Mammedov wrote:
> > cpu->found_cpus bitmap is used for setting present
> > flag in CPON AML package. But it takes a bunch of code
> > to fill bitmap and could be simplified by getting
> > presense info from possible CPUs list directly.
> >
> > So drop cpu->found_cpus bitmap and unroll possible
> > CPUs list into APIC index array at the place where
> > CPUON AML package is created.
> >
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >   hw/i386/acpi-build.c | 53 
> > ++++++++++++++--------------------------------------
> >   1 file changed, 14 insertions(+), 39 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index df7e6df..48240e0 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -76,10 +76,6 @@
> >   #define ACPI_BUILD_DPRINTF(fmt, ...)
> >   #endif
> >
> > -typedef struct AcpiCpuInfo {
> > -    DECLARE_BITMAP(found_cpus, ACPI_CPU_HOTPLUG_ID_LIMIT);
> > -} AcpiCpuInfo;
> > -
> >   typedef struct AcpiMcfgInfo {
> >       uint64_t mcfg_base;
> >       uint32_t mcfg_size;
> > @@ -121,31 +117,6 @@ typedef struct AcpiBuildPciBusHotplugState {
> >       bool pcihp_bridge_en;
> >   } AcpiBuildPciBusHotplugState;
> >
> > -static
> > -int acpi_add_cpu_info(Object *o, void *opaque)
> > -{
> > -    AcpiCpuInfo *cpu = opaque;
> > -    uint64_t apic_id;
> > -
> > -    if (object_dynamic_cast(o, TYPE_CPU)) {
> > -        apic_id = object_property_get_int(o, "apic-id", NULL);
> > -        assert(apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
> > -
> > -        set_bit(apic_id, cpu->found_cpus);
> > -    }
> > -
> > -    object_child_foreach(o, acpi_add_cpu_info, opaque);
> > -    return 0;
> > -}
> > -
> > -static void acpi_get_cpu_info(AcpiCpuInfo *cpu)
> > -{
> > -    Object *root = object_get_root();
> > -
> > -    memset(cpu->found_cpus, 0, sizeof cpu->found_cpus);
> > -    object_child_foreach(root, acpi_add_cpu_info, cpu);
> > -}
> > -
> >   static void acpi_get_pm_info(AcpiPmInfo *pm)
> >   {
> >       Object *piix = piix4_pm_find();
> > @@ -966,9 +937,9 @@ static Aml *build_crs(PCIHostState *host,
> >   }
> >
> >   static void build_processor_devices(Aml *sb_scope, MachineState *machine,
> > -                                    AcpiCpuInfo *cpu, AcpiPmInfo *pm)
> > +                                    AcpiPmInfo *pm)
> >   {
> > -    int i;
> > +    int i, apic_idx;
> >       Aml *dev;
> >       Aml *crs;
> >       Aml *pkg;
> > @@ -1011,6 +982,7 @@ static void build_processor_devices(Aml *sb_scope, 
> > MachineState *machine,
> >           int apic_id = apic_ids->cpus[i].arch_id;
> >
> >           assert(apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
> > +  
> 
> Unneeded chunk, not really a big issue :)
assert has been separated from following statement originally,
so I've just kept the current style.

> 
> 
> Reviewed-by: Marcel Apfelbaum <address@hidden>
Thanks

> 
> Thanks,
> Marcel
> 
> >           dev = aml_processor(apic_id, 0, 0, "CP%.02X", apic_id);
> >
> >           method = aml_method("_MAT", 0, AML_NOTSERIALIZED);
> > @@ -1059,9 +1031,14 @@ static void build_processor_devices(Aml *sb_scope, 
> > MachineState *machine,
> >       pkg = pcms->apic_id_limit <= 255 ? aml_package(pcms->apic_id_limit) :
> >                                          
> > aml_varpackage(pcms->apic_id_limit);
> >
> > -    for (i = 0; i < pcms->apic_id_limit; i++) {
> > -        uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00;
> > -        aml_append(pkg, aml_int(b));
> > +    for (i = 0, apic_idx = 0; i < apic_ids->len; i++) {
> > +        int apic_id = apic_ids->cpus[i].arch_id;
> > +
> > +        for (; apic_idx < apic_id; apic_idx++) {
> > +            aml_append(pkg, aml_int(0));
> > +        }
> > +        aml_append(pkg, aml_int(apic_ids->cpus[i].cpu ? 1 : 0));
> > +        apic_idx = apic_id + 1;
> >       }
> >       aml_append(sb_scope, aml_name_decl(CPU_ON_BITMAP, pkg));
> >       g_free(apic_ids);
> > @@ -1952,7 +1929,7 @@ static Aml *build_q35_osc_method(void)
> >
> >   static void
> >   build_dsdt(GArray *table_data, GArray *linker,
> > -           AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc,
> > +           AcpiPmInfo *pm, AcpiMiscInfo *misc,
> >              PcPciInfo *pci, MachineState *machine)
> >   {
> >       CrsRangeEntry *entry;
> > @@ -2259,7 +2236,7 @@ build_dsdt(GArray *table_data, GArray *linker,
> >
> >       sb_scope = aml_scope("\\_SB");
> >       {
> > -        build_processor_devices(sb_scope, machine, cpu, pm);
> > +        build_processor_devices(sb_scope, machine, pm);
> >
> >           build_memory_devices(sb_scope, nr_mem, pm->mem_hp_io_base,
> >                                pm->mem_hp_io_len);
> > @@ -2613,7 +2590,6 @@ void acpi_build(AcpiBuildTables *tables, MachineState 
> > *machine)
> >       PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> >       GArray *table_offsets;
> >       unsigned facs, dsdt, rsdt, fadt;
> > -    AcpiCpuInfo cpu;
> >       AcpiPmInfo pm;
> >       AcpiMiscInfo misc;
> >       AcpiMcfgInfo mcfg;
> > @@ -2623,7 +2599,6 @@ void acpi_build(AcpiBuildTables *tables, MachineState 
> > *machine)
> >       GArray *tables_blob = tables->table_data;
> >       AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL };
> >
> > -    acpi_get_cpu_info(&cpu);
> >       acpi_get_pm_info(&pm);
> >       acpi_get_misc_info(&misc);
> >       acpi_get_pci_info(&pci);
> > @@ -2647,7 +2622,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState 
> > *machine)
> >
> >       /* DSDT is pointed to by FADT */
> >       dsdt = tables_blob->len;
> > -    build_dsdt(tables_blob, tables->linker, &cpu, &pm, &misc, &pci, 
> > machine);
> > +    build_dsdt(tables_blob, tables->linker, &pm, &misc, &pci, machine);
> >
> >       /* Count the size of the DSDT and SSDT, we will need it for legacy
> >        * sizing of ACPI tables.
> >  
>