From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51192) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adJNC-0000vW-5D for qemu-devel@nongnu.org; Tue, 08 Mar 2016 10:16:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adJN8-0003iA-QI for qemu-devel@nongnu.org; Tue, 08 Mar 2016 10:16:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55214) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adJN8-0003i2-JA for qemu-devel@nongnu.org; Tue, 08 Mar 2016 10:16:30 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 51DACD7925 for ; Tue, 8 Mar 2016 15:16:30 +0000 (UTC) References: <1456495168-144510-4-git-send-email-imammedo@redhat.com> <1457015336-288204-1-git-send-email-imammedo@redhat.com> <20160308132850.2fbe25bd@nial.brq.redhat.com> <56DEDBBF.9050203@redhat.com> <20160308154317.72a695a4@nial.brq.redhat.com> From: Marcel Apfelbaum Message-ID: <56DEECCB.6000402@redhat.com> Date: Tue, 8 Mar 2016 17:16:27 +0200 MIME-Version: 1.0 In-Reply-To: <20160308154317.72a695a4@nial.brq.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 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 list On 03/08/2016 04:43 PM, Igor Mammedov wrote: > On Tue, 8 Mar 2016 16:03:43 +0200 > Marcel Apfelbaum wrote: > >> On 03/08/2016 02:28 PM, Igor Mammedov wrote: >>> On Thu, 3 Mar 2016 15:28:56 +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 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 >>>> --- >>>> v5: >>>> s/CPUArchId cpus[1]/CPUArchId cpus[0]/ to make length >>>> calculation simpler. Marcel Apfelbaum >>> >>> Marcel, does v5 looks good to you? >> >> Definitely, thanks. >> >> Reviewed-by: Marcel Apfelbaum > Ops, I'm sorry but I've forgot to CC qemu-devel > could you reply to list list pls? > Sure, cc-ed qemu-devel. Thanks, Marcel >> >>> >>>> --- >>>> 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..1af95a1 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); >>>> + 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); >>>> + 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 b5d7eae..4b3fdbe 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[0]; >>>> +} 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; >>> >> >> >