From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47757) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abRPq-0004Qw-Rd for qemu-devel@nongnu.org; Thu, 03 Mar 2016 06:27:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abRPm-0001vM-RD for qemu-devel@nongnu.org; Thu, 03 Mar 2016 06:27:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45005) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abRPm-0001ut-Gt for qemu-devel@nongnu.org; Thu, 03 Mar 2016 06:27:30 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 966C37EBA1 for ; Thu, 3 Mar 2016 11:27:29 +0000 (UTC) References: <1456495168-144510-1-git-send-email-imammedo@redhat.com> <1456495168-144510-4-git-send-email-imammedo@redhat.com> From: Marcel Apfelbaum Message-ID: <56D81F9F.9020007@redhat.com> Date: Thu, 3 Mar 2016 13:27:27 +0200 MIME-Version: 1.0 In-Reply-To: <1456495168-144510-4-git-send-email-imammedo@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 03/10] machine: introduce MachineClass.possible_cpu_arch_ids() hook List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov , qemu-devel@nongnu.org Cc: ehabkost@redhat.com, mst@redhat.com On 02/26/2016 03:59 PM, 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 guess > what IDs are correct on its own. > > This interface will also allow to help making AML > part of CPU hotplug target independent so it could > be reused for ARM target. > > Signed-off-by: Igor Mammedov > --- > hw/i386/pc.c | 44 ++++++++++++++++++++++++++++++++++++++++---- > include/hw/boards.h | 26 ++++++++++++++++++++++++++ > include/hw/i386/pc.h | 1 + > 3 files changed, 67 insertions(+), 4 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 151a64c..5ce9295 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1132,10 +1132,17 @@ void pc_cpus_init(PCMachineState *pcms) > exit(1); > } > > - for (i = 0; i < smp_cpus; i++) { > - cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i), > - &error_fatal); > - object_unref(OBJECT(cpu)); > + pcms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) + > + sizeof(CPUArchId) * (max_cpus - 1)); > + for (i = 0; i < max_cpus; i++) { > + pcms->possible_cpus->cpus[i].arch_id = x86_cpu_apic_id_from_index(i); > + pcms->possible_cpus->len++; > + if (i < smp_cpus) { > + cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i), > + &error_fatal); > + pcms->possible_cpus->cpus[i].cpu = CPU(cpu); > + object_unref(OBJECT(cpu)); > + } > } > > /* tell smbios about cpuid version and features */ > @@ -1658,9 +1665,19 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev, > error_propagate(errp, local_err); > } > > +static int pc_apic_cmp(const void *a, const void *b) > +{ > + CPUArchId *apic_a = (CPUArchId *)a; > + CPUArchId *apic_b = (CPUArchId *)b; > + > + return apic_a->arch_id - apic_b->arch_id; > +} > + > static void pc_cpu_plug(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > + CPUClass *cc = CPU_GET_CLASS(dev); > + CPUArchId apic_id, *found_cpu; > HotplugHandlerClass *hhc; > Error *local_err = NULL; > PCMachineState *pcms = PC_MACHINE(hotplug_dev); > @@ -1683,6 +1700,13 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev, > > /* increment the number of CPUs */ > rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1); > + > + apic_id.arch_id = cc->get_arch_id(CPU(dev)); > + found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus, > + pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus), > + pc_apic_cmp); > + assert(found_cpu); > + found_cpu->cpu = CPU(dev); > out: > error_propagate(errp, local_err); > } > @@ -1925,6 +1949,17 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index) > return topo.pkg_id; > } > > +static CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *machine) > +{ > + PCMachineState *pcms = PC_MACHINE(machine); > + int len = sizeof(CPUArchIdList) + > + sizeof(CPUArchId) * (pcms->possible_cpus->len - 1); > + CPUArchIdList *list = g_malloc(len); > + > + memcpy(list, pcms->possible_cpus, len); > + return list; > +} > + > static void pc_machine_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > @@ -1947,6 +1982,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > pcmc->save_tsc_khz = true; > mc->get_hotplug_handler = pc_get_hotpug_handler; > mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id; > + mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids; > mc->default_boot_order = "cad"; > mc->hot_add_cpu = pc_hot_add_cpu; > mc->max_cpus = 255; > diff --git a/include/hw/boards.h b/include/hw/boards.h > index de3b3bd..c9b11d4 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -8,6 +8,7 @@ > #include "sysemu/accel.h" > #include "hw/qdev.h" > #include "qom/object.h" > +#include "qom/cpu.h" > > void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner, > const char *name, > @@ -42,6 +43,26 @@ bool machine_dump_guest_core(MachineState *machine); > bool machine_mem_merge(MachineState *machine); > > /** > + * CPUArchId: > + * @arch_id - architecture-dependent CPU ID of present or possible CPU > + * @cpu - pointer to corresponding CPU object if it's present on NULL otherwise > + */ > +typedef struct { > + uint64_t arch_id; > + struct CPUState *cpu; > +} CPUArchId; > + > +/** > + * CPUArchIdList: > + * @len - number of @CPUArchId items in @cpus array > + * @cpus - array of present or possible CPUs for current machine configuration > + */ > +typedef struct { > + int len; > + CPUArchId cpus[1]; Hi Igor, The patch looks good to me, I just have one question. Why do you have cpus[1] in CPUArchIdList and not cpus[0]? I suppose is to say that every machine has at least one cpu..., on the other hand that leads to "not standard" usage like when allocating cpus you need a "-1" and so on. Thanks, Marcel > +} CPUArchIdList; > + > +/** > * MachineClass: > * @get_hotplug_handler: this function is called during bus-less > * device hotplug. If defined it returns pointer to an instance > @@ -57,6 +78,10 @@ bool machine_mem_merge(MachineState *machine); > * Set only by old machines because they need to keep > * compatibility on code that exposed QEMU_VERSION to guests in > * the past (and now use qemu_hw_version()). > + * @possible_cpu_arch_ids: > + * Returns an array of @CPUArchId architecture-dependent CPU IDs > + * which includes CPU IDs for present and possible to hotplug CPUs. > + * Caller is responsible for freeing returned list. > */ > struct MachineClass { > /*< private >*/ > @@ -98,6 +123,7 @@ struct MachineClass { > HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > DeviceState *dev); > unsigned (*cpu_index_to_socket_id)(unsigned cpu_index); > + CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine); > }; > > /** > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 8b3546e..3e09232 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -65,6 +65,7 @@ struct PCMachineState { > /* CPU and apic information: */ > bool apic_xrupt_override; > unsigned apic_id_limit; > + CPUArchIdList *possible_cpus; > > /* NUMA information: */ > uint64_t numa_nodes; >