Re: [Qemu-devel] [RFC PATCH v0 3/6] spapr: Represent boot CPUs as spapr-cpu-core devices

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Fri, Feb 26, 2016 at 04:18:57PM +0100, Igor Mammedov wrote:
> On Thu, 25 Feb 2016 21:52:39 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > Initialize boot CPUs as spapr-cpu-core devices and create links from
> > machine object to these core devices. These links can be considered
> > as CPU slots in which core devices will get hot-plugged. spapr-cpu-core
> > device's slot property indicates the slot where it is plugged. Information
> > about all the CPU slots can be obtained by walking these links.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c         | 65 +++++++++++++++++++++++++++++++++++++++++++-------
> >  include/hw/ppc/spapr.h |  3 +++
> >  2 files changed, 60 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index e214a34..1f0d232 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -64,6 +64,7 @@
> >  
> >  #include "hw/compat.h"
> >  #include "qemu-common.h"
> > +#include "hw/ppc/spapr_cpu_core.h"
> >  
> >  #include <libfdt.h>
> >  
> > @@ -1720,6 +1721,21 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp)
> >      }
> >  }
> >  
> > +/*
> > + * Check to see if core is being hot-plugged into an already populated slot.
> > + */
> > +static void spapr_cpu_core_allow_set_link(Object *obj, const char *name,
> > +                                          Object *val, Error **errp)
> > +{
> > +    Object *core = object_property_get_link(qdev_get_machine(), name, NULL);
> > +
> > +    if (core) {
> > +        char *path = object_get_canonical_path(core);
> > +        error_setg(errp, "Slot %s already populated with %s", name, path);
> > +        g_free(path);
> > +    }
> > +}
> > +
> >  /* pSeries LPAR / sPAPR hardware init */
> >  static void ppc_spapr_init(MachineState *machine)
> >  {
> > @@ -1728,7 +1744,6 @@ static void ppc_spapr_init(MachineState *machine)
> >      const char *kernel_filename = machine->kernel_filename;
> >      const char *kernel_cmdline = machine->kernel_cmdline;
> >      const char *initrd_filename = machine->initrd_filename;
> > -    PowerPCCPU *cpu;
> >      PCIHostState *phb;
> >      int i;
> >      MemoryRegion *sysmem = get_system_memory();
> > @@ -1742,6 +1757,8 @@ static void ppc_spapr_init(MachineState *machine)
> >      long load_limit, fw_size;
> >      bool kernel_le = false;
> >      char *filename;
> > +    int spapr_cores = smp_cpus / smp_threads;
> > +    int spapr_max_cores = max_cpus / smp_threads;
> >  
> >      msi_supported = true;
> >  
> > @@ -1800,13 +1817,38 @@ static void ppc_spapr_init(MachineState *machine)
> >      if (machine->cpu_model == NULL) {
> >          machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
> >      }
> > -    for (i = 0; i < smp_cpus; i++) {
> > -        cpu = cpu_ppc_init(machine->cpu_model);
> > -        if (cpu == NULL) {
> > -            error_report("Unable to find PowerPC CPU definition");
> > -            exit(1);
> > +
> > +    spapr->cores = g_malloc0(spapr_max_cores * sizeof(Object));
> souldn't it be sizeof(Object *) 

Yes, I meant to store the pointers to Object here, will change.

> 
> > +
> > +    for (i = 0; i < spapr_max_cores; i++) {
> > +        Object *spapr_cpu_core  = object_new(TYPE_SPAPR_CPU_CORE);
> you allocate spapr_max_cores cores but set links to only to spapr_cores only,
> it looks like all spapr_cpu_core objects are leaked for range spapr_cores..spapr_max_cores
> 

Yes, as I replied in an ealier thread, the intention is to create links
for all possible cores, but create only those many cores required for CPUs
specified with -smp from machine init. The links will be set only for those
cores. Hotplugged cores will get created and the links will be set for them
during device_add. So the changed code now looks like this:

    for (i = 0; i < spapr_max_cores; i++) {
        char name[32];

        /*
         * Create links from machine objects to all possible cores.
         */
        snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
        object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
                                 (Object **)&spapr->cores[i],
                                 spapr_cpu_core_allow_set_link, 0,
                                 &error_fatal);

        /*
         * Create cores and set link from machine object to core object for
         * boot time CPUs and realize them.
         */
        if (i < spapr_cores) {
            Object *core  = object_new(TYPE_SPAPR_CPU_CORE);

            object_property_set_str(core, machine->cpu_model, "cpu_model",
                                    &error_fatal);
            object_property_set_int(core, smp_threads, "nr_threads",
                                    &error_fatal);
            object_property_set_str(core, name, SPAPR_CPU_CORE_SLOT_PROP,
                                    &error_fatal);
            object_property_set_bool(core, true, "realized", &error_fatal);
        }
    }

> 
> > +        char name[32];
> > +
> > +        object_property_set_str(spapr_cpu_core, machine->cpu_model, "cpu_model",
> > +                                &error_fatal);
> > +        object_property_set_int(spapr_cpu_core, smp_threads, "nr_threads",
> > +                                &error_fatal);
> > +        /*
> > +         * Create links from machine objects to all possible cores.
> > +         */
> > +        snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> > +        object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
> > +                                 (Object **)&spapr->cores[i],
> > +                                 spapr_cpu_core_allow_set_link, 0,
> > +                                 &error_fatal);
> > +
> > +        /*
> > +         * Set the link from machine object to core object for all
> > +         * boot time CPUs specified with -smp and realize them.
> > +         * For rest of the hotpluggable cores this is happens from
> > +         * the core hotplug/realization path.
> > +         */
> > +        if (i < spapr_cores) {
> > +            object_property_set_str(spapr_cpu_core, name,
> > +                                    SPAPR_CPU_CORE_SLOT_PROP, &error_fatal);
> > +            object_property_set_bool(spapr_cpu_core, true, "realized",
> > +                                     &error_fatal);
> >          }
> > -        spapr_cpu_init(spapr, cpu, &error_fatal);
> >      }
> >  
> >      if (kvm_enabled()) {
> > @@ -2209,6 +2251,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> >                                        DeviceState *dev, Error **errp)
> >  {
> >      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
> > +    sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> >  
> >      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> >          int node;
> > @@ -2245,6 +2288,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> >          }
> >  
> >          spapr_memory_plug(hotplug_dev, dev, node, errp);
> > +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> here probably should be CORE and not TYPE_CPU,
> then if board needs to set some state for child threads it
> could ennumerate child of core here and do the required job.

Hmm, we have things to set for CPUs and cores as well from hotplug
handler. So the above code is for CPUs and I have spapr_core_plug()
which is called conditionally when device is of type TYPE_SPAPR_CPU_CORE.

Given that ->plug() is called during realization of both individual CPU
threads as well as their parent core, I thought handling the setup for
them separately like the above is simpler, no ?

Regards,
Bharata.