On Wed, Mar 25, 2015 at 10:43 PM, Andreas Färber <afaer...@suse.de> wrote:
> Am 25.03.2015 um 17:55 schrieb Bharata B Rao:
>> On Mon, Mar 23, 2015 at 11:02 PM, Andreas Färber <afaer...@suse.de> wrote:
>>> Inline realized=true from pc_new_cpu() so that the realization can be
>>> deferred, as it would otherwise create a device[n] node.
>>>
>>> Signed-off-by: Andreas Färber <afaer...@suse.de>
>>> ---
>>> 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_err);
>>> - object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>>>
>>> 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 = NULL;
>>> Error *error = NULL;
>>> unsigned long apic_id_limit;
>>> + int sockets, cpu_index = 0;
>>>
>>> /* init CPUs */
>>> if (cpu_model == NULL) {
>>> @@ -1087,14 +1104,41 @@ void pc_cpus_init(const char *cpu_model,
>>> DeviceState *icc_bridge)
>>> exit(1);
>>> }
>>>
>>> - for (i = 0; i < smp_cpus; i++) {
>>> - cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
>>> - icc_bridge, &error);
>>> + sockets = smp_cpus / smp_cores / smp_threads;
>>> + for (i = 0; i < sockets; i++) {
>>> + socket = g_malloc0(sizeof(*socket) + smp_cores *
>>> pc_cpu_core_size());
>>> + object_initialize(socket, sizeof(*socket), TYPE_X86_CPU_SOCKET);
>>> + OBJECT(socket)->free = g_free;
>>> +
>>> + for (j = 0; j < smp_cores; j++) {
>>> + core = 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 = 0; k < smp_threads; k++) {
>>> + cpu = 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", &error);
>>
>> So you do in-place initialization as part of an "atomic" socket object.
>
> (indivisible might've been a better term on my part)
>
>>
>> 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 ?
>>
>> + sockets = smp_cpus / smp_cores / smp_threads;
>> + for (i = 0; i < sockets; i++) {
>> + socket = object_new(TYPE_POWERPC_CPU_SOCKET);
>> + object_property_set_bool(socket, true, "realized", &error_abort);
>> }
>>
>> 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.
Ok, So the difference between what you are doing and the approach I
have taken is not that much.
You allocate a big socket object (which includes memory for cores and
threads objects) and then initialize them in a loop and finally
realize the socket object which iteratively realizes cores and
threads.
Instead of open g_malloc0, I resort to object_new for the socket
object. The instance_init of the socket object will create core
objects (via object_new again) and the instance_init of thread object
will create the CPU threads (using object_new). At the end of this I
do a realize on socket object which iteratively realizes cores and
threads similar to your implementation.
So unless I am missing the subtlety here, it is about one big malloc
vs multiple small mallocs. So the chance for failure exists in both
cases.
>
> Further, setting realized=true 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.
Please look at my v2 patchset, I am not setting realized=true from
instance _init.
Regards,
Bharata.
