Re: [Qemu-devel] [RFC PATCH v0 2/6] spapr: CPU core device

[Bharata B Rao]

On Mon, Feb 29, 2016 at 11:03:16AM +0100, Igor Mammedov wrote:
> On Mon, 29 Feb 2016 11:20:19 +0530
> Bharata B Rao <address@hidden> wrote:
> 
> > On Fri, Feb 26, 2016 at 12:13:39PM -0600, Michael Roth wrote:
> > > Quoting Bharata B Rao (2016-02-25 10:22:38)  
> > > > Add sPAPR specific CPU core device that is based on generic CPU core 
> > > > device.
> > > > Creating this core device will result in creation of all the CPU thread
> > > > devices that are part of this core.
> > > > 
> > > > Signed-off-by: Bharata B Rao <address@hidden>
> > > > ---
> > > >  hw/ppc/Makefile.objs            |   1 +
> > > >  hw/ppc/spapr_cpu_core.c         | 210 
> > > > ++++++++++++++++++++++++++++++++++++++++
> > > >  include/hw/ppc/spapr_cpu_core.h |  32 ++++++
> > > >  3 files changed, 243 insertions(+)
> > > >  create mode 100644 hw/ppc/spapr_cpu_core.c
> > > >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > > > 
> > > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > > index c1ffc77..5cc6608 100644
> > > > --- a/hw/ppc/Makefile.objs
> > > > +++ b/hw/ppc/Makefile.objs
> > > > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> > > >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> > > >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > > >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o 
> > > > spapr_rng.o
> > > > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> > > >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> > > >  obj-y += spapr_pci_vfio.o
> > > >  endif
> > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > new file mode 100644
> > > > index 0000000..c44eb61
> > > > --- /dev/null
> > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > @@ -0,0 +1,210 @@
> > > > +/*
> > > > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > > > + *
> > > > + * Copyright (C) 2016 Bharata B Rao <address@hidden>
> > > > + *
> > > > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > > > later.
> > > > + * See the COPYING file in the top-level directory.
> > > > + */
> > > > +#include "hw/cpu/core.h"
> > > > +#include "hw/ppc/spapr_cpu_core.h"
> > > > +#include "hw/ppc/spapr.h"
> > > > +#include "hw/boards.h"
> > > > +#include "qemu/error-report.h"
> > > > +#include "qapi/visitor.h"
> > > > +#include <sysemu/cpus.h>
> > > > +
> > > > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > > > +{
> > > > +    Error **errp = opaque;
> > > > +
> > > > +    object_property_set_bool(child, true, "realized", errp);
> > > > +    if (*errp) {
> > > > +        return 1;
> > > > +    }
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > > +{
> > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > > +    Error *local_err = NULL;
> > > > +
> > > > +    if (!core->nr_threads) {
> > > > +        error_setg(errp, "nr_threads property can't be 0");
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    if (!core->cpu_model) {
> > > > +        error_setg(errp, "cpu_model property isn't set");
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    /*
> > > > +     * TODO: If slot isn't specified, plug this core into
> > > > +     * an existing empty slot.
> > > > +     */
> > > > +    if (!core->slot) {
> > > > +        error_setg(errp, "slot property isn't set");
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    object_property_set_link(OBJECT(spapr), OBJECT(core), core->slot,
> > > > +                             &local_err);
> > > > +    if (local_err) {
> > > > +        error_propagate(errp, local_err);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, 
> > > > errp);
> > > > +}
> > > > +
> > > > +/*
> > > > + * This creates the CPU threads for a given @core.
> > > > + *
> > > > + * In order to create the threads, we need two inputs - number of
> > > > + * threads and the cpu_model. These are set as core object's 
> > > > properties.
> > > > + * When both of them become available/set, this routine will be called 
> > > > from
> > > > + * either property's set handler to create the threads.
> > > > + *
> > > > + * TODO: Dependence of threads creation on two properties is resulting
> > > > + * in this not-so-clean way of creating threads from either of the
> > > > + * property setters based on the order in which they get set. Check if
> > > > + * this can be handled in a better manner.
> > > > + */
> > > > +static void spapr_cpu_core_create_threads(sPAPRCPUCore *core, Error 
> > > > **errp)
> > > > +{
> > > > +    int i;
> > > > +
> > > > +    for (i = 0; i < core->nr_threads; i++) {
> > > > +        char id[32];
> > > > +        char type[32];
> > > > +
> > > > +        snprintf(type, sizeof(type), "%s-%s", core->cpu_model,
> > > > +                 TYPE_POWERPC_CPU);
> > > > +        object_initialize(&core->threads[i], sizeof(core->threads[i]), 
> > > > type);
> > > > +
> > > > +        snprintf(id, sizeof(id), "thread[%d]", i);
> > > > +        object_property_add_child(OBJECT(core), id, 
> > > > OBJECT(&core->threads[i]),
> > > > +                                  errp);
> > > > +    }
> > > > +}
> > > > +
> > > > +static char *spapr_cpu_core_prop_get_slot(Object *obj, Error **errp)
> > > > +{
> > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > +
> > > > +    return core->slot;
> > > > +}
> > > > +
> > > > +static void spapr_cpu_core_prop_set_slot(Object *obj, const char *val,
> > > > +                                              Error **errp)
> > > > +{
> > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > +
> > > > +    core->slot = g_strdup(val);
> > > > +}
> > > > +
> > > > +static char *spapr_cpu_core_prop_get_cpu_model(Object *obj, Error 
> > > > **errp)
> > > > +{
> > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > +
> > > > +    return core->cpu_model;
> > > > +}
> > > > +
> > > > +static void spapr_cpu_core_prop_set_cpu_model(Object *obj, const char 
> > > > *val,
> > > > +                                              Error **errp)
> > > > +{
> > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > +    MachineState *machine = MACHINE(qdev_get_machine());
> > > > +
> > > > +    /*
> > > > +     * cpu_model can't be different from what is specified with -cpu
> > > > +     */
> > > > +    if (strcmp(val, machine->cpu_model)) {
> > > > +       error_setg(errp, "cpu_model should be %s", machine->cpu_model);
> > > > +       return;
> > > > +    }
> > > > +
> > > > +    core->cpu_model = g_strdup(val);
> > > > +    if (core->nr_threads && core->cpu_model) {
> > > > +        spapr_cpu_core_create_threads(core, errp);
> > > > +    }
> > > > +}
> > > > +
> > > > +static void spapr_cpu_core_prop_get_nr_threads(Object *obj, Visitor *v,
> > > > +                                               const char *name, void 
> > > > *opaque,
> > > > +                                               Error **errp)
> > > > +{
> > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > +    int64_t value = core->nr_threads;
> > > > +
> > > > +    visit_type_int(v, name, &value, errp);
> > > > +}
> > > > +
> > > > +static void spapr_cpu_core_prop_set_nr_threads(Object *obj, Visitor *v,
> > > > +                                               const char *name, void 
> > > > *opaque,
> > > > +                                               Error **errp)
> > > > +{
> > > > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > +    Error *local_err = NULL;
> > > > +    int64_t value;
> > > > +
> > > > +    visit_type_int(v, name, &value, &local_err);
> > > > +    if (local_err) {
> > > > +        error_propagate(errp, local_err);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    /* Allow only homogeneous configuration */
> > > > +    if (value != smp_threads) {
> > > > +        error_setg(errp, "nr_threads should be %d", smp_threads);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    core->nr_threads = value;
> > > > +    core->threads = g_malloc0(core->nr_threads * sizeof(PowerPCCPU));
> > > > +
> > > > +    if (core->nr_threads && core->cpu_model) {
> > > > +        spapr_cpu_core_create_threads(core, errp);
> > > > +    }
> > > > +}
> > > > +
> > > > +static void spapr_cpu_core_instance_init(Object *obj)
> > > > +{
> > > > +    object_property_add(obj, "nr_threads", "int",
> > > > +                        spapr_cpu_core_prop_get_nr_threads,
> > > > +                        spapr_cpu_core_prop_set_nr_threads,
> > > > +                        NULL, NULL, NULL);
> > > > +    object_property_add_str(obj, "cpu_model",
> > > > +                            spapr_cpu_core_prop_get_cpu_model,
> > > > +                            spapr_cpu_core_prop_set_cpu_model,
> > > > +                            NULL);
> > > > +    object_property_add_str(obj, "slot",
> > > > +                            spapr_cpu_core_prop_get_slot,
> > > > +                            spapr_cpu_core_prop_set_slot,
> > > > +                            NULL);  
> > > 
> > > "slot" seems intended to be a machine-agnostic of mapping device
> > > types discovered from qmp_query_cpu_slots() to an appropriate
> > > "bus" location, but here's it a field specific to TYPE_SPAPR_CPU_CORE.
> > > It seems like maybe TYPE_CPU_CORE is a better place, but then on
> > > x86 I suppose it might be TYPE_CPU_SOCKET or something instead...  
> > 
> > Correct.
> > 
> > > 
> > > It almost seems like a TYPE_INTERFACE_SLOTABLE would be the
> > > right approach, but I don't know how we could expose that as
> > > a property. I guess it's somewhat implied that this "interface"
> > > exists if qmp_query_cpu_slots() returns the type, but I wonder
> > > if something a bit more formal should be modeled to make the
> > > implementation requirements a bit clearer.
> > > 
> > > Maybe have TYPE_CPU_{CORE,SOCKET} classes have a get_slot/set_slot
> > > class method, expose them via "slot" property, then have the
> > > defaults generate "not implemented" errors?  
> > 
> > Yes makes sense. In fact David has often times said that generic
> > properties/routines should be pushed to base class wherever possible.
> > 
> > I didn't do that in this first iteration to keep the generic changes
> > as minimum as possible, but yes slot should be a property of the
> > base class of core or socket.
> Then what will happen to slot if there isn't any core/socket device
> to query it, i.e. cpu hasn't been plugged in yet?
> To me slot looks like a machine belonged feature.

Yes slot belongs to the machine and it is represented by a link that
is created b/n the machine object and the core object that sits in
the slot.

In the context of this thread, slot is actually the slot name that
identifies the machine slot which the core occupies or will occupy after
hotplug. Thus slot name which is named slot here, it is a property of the
core device.

(qemu) device_add spapr-cpu-core,slot=core[2]
                                 ^
Regards,
Bharata.
> 
> > 
> > Regards,
> > Bharata.
> >