From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50248) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaosL-0005rJ-4H for qemu-devel@nongnu.org; Wed, 25 Mar 2015 13:13:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YaosI-0001Lj-AF for qemu-devel@nongnu.org; Wed, 25 Mar 2015 13:13:53 -0400 Received: from cantor2.suse.de ([195.135.220.15]:38167 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaosI-0001Ki-0O for qemu-devel@nongnu.org; Wed, 25 Mar 2015 13:13:50 -0400 Message-ID: <5512ECCC.3050708@suse.de> Date: Wed, 25 Mar 2015 18:13:48 +0100 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1427131923-4670-1-git-send-email-afaerber@suse.de> <1427131923-4670-4-git-send-email-afaerber@suse.de> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH RFC 3/4] pc: Create sockets and cores for CPUs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bharata B Rao Cc: Paolo Bonzini , "Michael S. Tsirkin" , "qemu-devel@nongnu.org" , Richard Henderson Am 25.03.2015 um 17:55 schrieb Bharata B Rao: > On Mon, Mar 23, 2015 at 11:02 PM, Andreas F=C3=A4rber wrote: >> Inline realized=3Dtrue from pc_new_cpu() so that the realization can b= e >> deferred, as it would otherwise create a device[n] node. >> >> Signed-off-by: Andreas F=C3=A4rber >> --- >> hw/i386/pc.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++= +-------- >> 1 file changed, 58 insertions(+), 8 deletions(-) >> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 2c48277..492c262 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -54,11 +54,14 @@ >> #include "exec/memory.h" >> #include "exec/address-spaces.h" >> #include "sysemu/arch_init.h" >> +#include "sysemu/cpus.h" >> #include "qemu/bitmap.h" >> #include "qemu/config-file.h" >> #include "hw/acpi/acpi.h" >> #include "hw/acpi/cpu_hotplug.h" >> #include "hw/cpu/icc_bus.h" >> +#include "hw/i386/cpu-socket.h" >> +#include "hw/i386/cpu-core.h" >> #include "hw/boards.h" >> #include "hw/pci/pci_host.h" >> #include "acpi-build.h" >> @@ -990,6 +993,17 @@ void pc_acpi_smi_interrupt(void *opaque, int irq,= int level) >> } >> } >> >> +static inline size_t pc_cpu_core_size(void) >> +{ >> + return sizeof(X86CPUCore); >> +} >> + >> +static inline X86CPUCore *pc_cpu_socket_get_core(X86CPUSocket *socket= , >> + unsigned int index) >> +{ >> + return &socket->core[index]; >> +} >> + >> static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, >> DeviceState *icc_bridge, Error **errp) >> { >> @@ -1009,7 +1023,6 @@ static X86CPU *pc_new_cpu(const char *cpu_model,= int64_t apic_id, >> qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "= icc")); >> >> object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_e= rr); >> - object_property_set_bool(OBJECT(cpu), true, "realized", &local_er= r); >> >> out: >> if (local_err) { >> @@ -1060,15 +1073,19 @@ void pc_hot_add_cpu(const int64_t id, Error **= errp) >> error_propagate(errp, local_err); >> return; >> } >> + object_property_set_bool(OBJECT(cpu), true, "realized", errp); >> object_unref(OBJECT(cpu)); >> } >> >> void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge) >> { >> - int i; >> + int i, j, k; >> + X86CPUSocket *socket; >> + X86CPUCore *core; >> X86CPU *cpu =3D NULL; >> Error *error =3D NULL; >> unsigned long apic_id_limit; >> + int sockets, cpu_index =3D 0; >> >> /* init CPUs */ >> if (cpu_model =3D=3D NULL) { >> @@ -1087,14 +1104,41 @@ void pc_cpus_init(const char *cpu_model, Devic= eState *icc_bridge) >> exit(1); >> } >> >> - for (i =3D 0; i < smp_cpus; i++) { >> - cpu =3D pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), >> - icc_bridge, &error); >> + sockets =3D smp_cpus / smp_cores / smp_threads; >> + for (i =3D 0; i < sockets; i++) { >> + socket =3D g_malloc0(sizeof(*socket) + smp_cores * pc_cpu_cor= e_size()); >> + object_initialize(socket, sizeof(*socket), TYPE_X86_CPU_SOCKE= T); >> + OBJECT(socket)->free =3D g_free; >> + >> + for (j =3D 0; j < smp_cores; j++) { >> + core =3D pc_cpu_socket_get_core(socket, j); >> + object_initialize(core, sizeof(*core), TYPE_X86_CPU_CORE)= ; >> + object_property_add_child(OBJECT(socket), "core[*]", >> + OBJECT(core), &error); >> + if (error) { >> + goto error; >> + } >> + >> + for (k =3D 0; k < smp_threads; k++) { >> + cpu =3D pc_new_cpu(cpu_model, >> + x86_cpu_apic_id_from_index(cpu_index= ), >> + icc_bridge, &error); >> + if (error) { >> + goto error; >> + } >> + object_property_add_child(OBJECT(core), "thread[*]", >> + OBJECT(cpu), &error); >> + object_unref(OBJECT(cpu)); >> + if (error) { >> + goto error; >> + } >> + cpu_index++; >> + } >> + } >> + object_property_set_bool(OBJECT(socket), true, "realized", &e= rror); >=20 > So you do in-place initialization as part of an "atomic" socket object. (indivisible might've been a better term on my part) >=20 > I am not sure why cores and threads should be allocated as part of > socket object and initialized like above ? Do you see any problem with > just instantiating the socket object and then let the instance_init > routines of socket and cores to initialize the cores and threads like > how I am doing for sPAPR ? >=20 > + sockets =3D smp_cpus / smp_cores / smp_threads; > + for (i =3D 0; i < sockets; i++) { > + socket =3D object_new(TYPE_POWERPC_CPU_SOCKET); > + object_property_set_bool(socket, true, "realized", &error_abor= t); > } >=20 > Ref: http://lists.gnu.org/archive/html/qemu-ppc/2015-03/msg00492.html Yes, instance_init cannot fail. By making the allocation separate, we assure that at this stage we have sufficient memory to perform the initializations. This is easiest when we know how many child objects we have, then we can use proper fields or arrays to reserve the memory (e.g., ARM MPCore, PCI host bridges). Here, to cope with dynamic smp_cores, I am using inline helpers to do the size/offset calculations. Further, setting realized=3Dtrue from instance_init is a no-go. It's a two-step initialization, with realization being the second step and potentially failing. The alternative I pointed you to was creating objects in a property setter, like Alexey was asked for for xics. Regards, Andreas --=20 SUSE Linux GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Felix Imend=C3=B6rffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, Graham Norton; HRB 21284 (AG N=C3=BCrnberg)