Re: [Qemu-devel] [PATCH 6/9] pc: acpi: create MADT.lapic entries only for valid lapics

[Eduardo Habkost <ehabkost@redhat.com>]

On Fri, Feb 05, 2016 at 05:14:41PM +0100, Igor Mammedov wrote:
> On Fri, 5 Feb 2016 13:28:31 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Feb 04, 2016 at 12:47:32PM +0100, Igor Mammedov wrote:
> > > do not assume that all lapics in range 0..apic_id_limit
> > > are valid and do not create lapic entries for not
> > > possible lapics in MADT.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > 
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > But there's one minor suggestion below:
> > 
> > > ---
> > >  hw/i386/acpi-build.c | 21 ++++++++++++++-------
> > >  1 file changed, 14 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index df13c7d..9eeeffa 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -361,9 +361,11 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm,
> > >  }
> > >  
> > >  static void
> > > -build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
> > > -           PcGuestInfo *guest_info)
> > > +build_madt(GArray *table_data, GArray *linker,
> > > +           MachineState *machine, PcGuestInfo *guest_info)
> > >  {
> > > +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> > > +    GArray *apic_id_list = mc->possible_cpu_arch_ids();
> > >      int madt_start = table_data->len;
> > >  
> > >      AcpiMultipleApicTable *madt;
> > > @@ -376,18 +378,23 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
> > >      madt->local_apic_address = cpu_to_le32(APIC_DEFAULT_ADDRESS);
> > >      madt->flags = cpu_to_le32(1);
> > >  
> > > -    for (i = 0; i < guest_info->apic_id_limit; i++) {
> > > +    for (i = 0; i < apic_id_list->len; i++) {
> > >          AcpiMadtProcessorApic *apic = acpi_data_push(table_data, sizeof *apic);
> > > +        CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i);
> > > +        int apic_id = id.arch_id;
> > > +
> > >          apic->type = ACPI_APIC_PROCESSOR;
> > >          apic->length = sizeof(*apic);
> > > -        apic->processor_id = i;
> > > -        apic->local_apic_id = i;
> > > -        if (test_bit(i, cpu->found_cpus)) {
> > > +        apic->processor_id = apic_id;
> > > +        apic->local_apic_id = apic_id;
> > > +        if (id.cpu != NULL) {  
> > 
> > This seems to be the only place where CPUArchId.cpu is being used
> > (see my previous suggestion about making possible_cpu_arch_ids()
> > return just an uint64_t list).
> > 
> > Also, using the existing found_cpus bitmap is more efficient than
> > making multiple calls to qemu_get_cpu_by_arch_id(). I wouldn't
> > mind keeping the bitmap.
> found_cpus bitmap is not better than (id.cpu != NULL) check
> the cost of filling both is about the same.

The cost doesn't look the same. Populating found_cpus should be
O(smp_cpus)[1], and is being done only once. Filling id.cpu in
pc_possible_cpu_arch_ids() is O(max_cpus*smp_cpus), and it is
called multiple times.

[1] I just noticed it is actually O(size_of_qom_tree), but it
    is still linear, and could be changed to O(smp_cpus).

> The issue I have with bitmap is that it's harder to generalize
> CPU hotplug code with it, while with possible_cpu_arch_ids()
> returned array I have a list of CPUs to work with without any
> assumptions on position in bitmap or array.
> Also bitmap scales worse than a list of CPUs if ID space
> is sparse and if ID is quite big.

Yes, I agree that a list is better depending on how the arch ID
space is used.

> That's why I'm dropping bitmap and switching to a list of
> IDs which in worst case is upto max_cpus.

I think the possible_cpu_arch_ids() interface looks good, maybe
we just need to optimize it.

But I'm not sure if we need to optimize it now, or if we can live
with the inefficient code and optimize it later. I won't complain
if we do it later, if we warn about it in the commit message or
comments.

-- 
Eduardo