From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40763) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aRjGf-000060-1C for qemu-devel@nongnu.org; Fri, 05 Feb 2016 11:29:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aRjGb-000104-0l for qemu-devel@nongnu.org; Fri, 05 Feb 2016 11:29:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36139) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aRjGa-0000zy-PB for qemu-devel@nongnu.org; Fri, 05 Feb 2016 11:29:52 -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 4C7ED113718 for ; Fri, 5 Feb 2016 16:29:52 +0000 (UTC) Date: Fri, 5 Feb 2016 17:29:49 +0100 From: Igor Mammedov Message-ID: <20160205172949.171c36f0@nial.brq.redhat.com> In-Reply-To: <20160205155005.GU26314@thinpad.lan.raisama.net> References: <1454586455-10202-1-git-send-email-imammedo@redhat.com> <1454586455-10202-2-git-send-email-imammedo@redhat.com> <20160205150426.GB26447@thinpad.lan.raisama.net> <20160205163946.0422a7c3@nial.brq.redhat.com> <20160205155005.GU26314@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/9] machine: introduce MachineClass.possible_cpu_arch_ids() hook List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, mst@redhat.com On Fri, 5 Feb 2016 13:50:05 -0200 Eduardo Habkost wrote: > On Fri, Feb 05, 2016 at 04:39:46PM +0100, Igor Mammedov wrote: > > On Fri, 5 Feb 2016 13:04:26 -0200 > > Eduardo Habkost wrote: > > > > > On Thu, Feb 04, 2016 at 12:47:28PM +0100, Igor Mammedov wrote: > > > > on x86 currently range 0..max_cpus is used to generate > > > > architecture-dependent CPU ID (APIC Id) for each present > > > > and possible CPUs. However architecture-dependent CPU IDs > > > > list could be sparse and code that needs to enumerate > > > > all IDs (ACPI) ended up doing guess work enumerating all > > > > possible and impossible IDs up to > > > > apic_id_limit = x86_cpu_apic_id_from_index(max_cpus). > > > > > > > > That leads to creation of MADT entries and Processor > > > > objects in ACPI tables for not possible CPUs. > > > > Fix it by allowing board specify a concrete list of > > > > CPU IDs accourding its own rules (which for x86 depends > > > > on topology). So that code that needs this list could > > > > request it from board instead of trying to figure out > > > > what IDs are correct on its own. > > > > > > > > Signed-off-by: Igor Mammedov > > > > --- > > > > hw/i386/pc.c | 16 ++++++++++++++++ > > > > include/hw/boards.h | 18 ++++++++++++++++++ > > > > 2 files changed, 34 insertions(+) > > > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > > index d72246d..2fd8fc8 100644 > > > > --- a/hw/i386/pc.c > > > > +++ b/hw/i386/pc.c > > > > @@ -1946,6 +1946,21 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index) > > > > return topo.pkg_id; > > > > } > > > > > > > > +static GArray *pc_possible_cpu_arch_ids(void) > > > > +{ > > > > + int i; > > > > + GArray *list = g_array_new (FALSE, FALSE, sizeof (CPUArchId)); > > > > + > > > > + for (i = 0; i < max_cpus; i++) { > > > > + CPUArchId val; > > > > + > > > > + val.arch_id = x86_cpu_apic_id_from_index(i); > > > > + val.cpu = qemu_get_cpu_by_arch_id(val.arch_id); > > > > + g_array_append_val(list, val); > > > > > > What about letting callers call qemu_get_cpu_by_arch_id() only if > > > they really need it? > > > > > > If you do that, you just need to return an uint64_t array, and > > > there's no need for struct CPUArchId. > > So far all callers that would use it would need to call > > qemu_get_cpu_by_arch_id() so doing it in one place (here) > > seems better than to duplicating that call over the code. > > I see only one place using CPUArchId.cpu. All other callers don't > use the field. > > Simply replacing "id.cpu" with "qemu_get_cpu_by_arch_id(id)" in > one line seems worth it, if it's going to save us the trouble of > defining another struct and avoid lots of unnecessary calls to > qemu_get_cpu_by_arch_id() (that loops through all CPUs every time > it's called). id.cpu is going to be used at other places when I add xlapic entries and cpu Devices in new CPU hotplug interface later it will be used for similar purposes for virt-arm machine. Another reason for struct is to discourage usage of direct access to elements of array, while with uint64_t it's very tempting to do so and easy to get wrong. (In the first attempt I did uint64_t array and then were looking for bugs due to wrong type casting)