From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38147) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTtqI-0002oy-O0 for qemu-devel@nongnu.org; Thu, 11 Feb 2016 11:11:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTtqD-0002EP-Bf for qemu-devel@nongnu.org; Thu, 11 Feb 2016 11:11:42 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42399) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTtqC-0002EI-Uy for qemu-devel@nongnu.org; Thu, 11 Feb 2016 11:11:37 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 86D77804E1 for ; Thu, 11 Feb 2016 16:11:36 +0000 (UTC) Date: Thu, 11 Feb 2016 14:11:34 -0200 From: Eduardo Habkost Message-ID: <20160211161134.GC6239@thinpad.lan.raisama.net> References: <1454586455-10202-1-git-send-email-imammedo@redhat.com> <1454586455-10202-6-git-send-email-imammedo@redhat.com> <20160205152831.GR26314@thinpad.lan.raisama.net> <20160205171441.4d5e8b12@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160205171441.4d5e8b12@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [PATCH 6/9] pc: acpi: create MADT.lapic entries only for valid lapics List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, mst@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 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 > > > > Reviewed-by: Eduardo Habkost > > > > 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