Re: [Qemu-devel] [PATCH RFC 3/4] pc: Create sockets and cores for CPUs

["Andreas Färber" <afaerber@suse.de>]

Am 25.03.2015 um 17:55 schrieb Bharata B Rao:
> On Mon, Mar 23, 2015 at 11:02 PM, Andreas Färber <afaerber@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 <afaerber@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.

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.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)