qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Igor Mammedov
Subject: Re: [Qemu-ppc] [RFC PATCH v0 2/6] spapr: CPU core device
Date: Tue, 1 Mar 2016 10:16:18 +0100

On Tue, 1 Mar 2016 13:47:27 +0530
Bharata B Rao <address@hidden> wrote:

> On Mon, Feb 29, 2016 at 04:15:25PM +0100, Igor Mammedov wrote:
> > On Mon, 29 Feb 2016 18:25:25 +0530
> > Bharata B Rao <address@hidden> wrote:
> >   
> > > 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]
> > >                                  ^  
> > Is 'slot' a term used by SPAPR on real hardware?
> > I'd thought that it's 'core', that's why I suggested to use
> > 'core' for POWER as that matched real world concept, see
> > my other reply in "[RFC PATCH v0 4/6] spapr: CPU hotplug support" thread
> > of this series.  
> 
> I don't see any reply from you to that thread. I guess you mean
> "[RFC PATCH v0 3/6] spapr: Represent boot CPUs as spapr-cpu-core devices"
no it's another thread, sorry for mixing things up.
it was thread about QMP interface:
http://lists.gnu.org/archive/html/qemu-devel/2016-02/msg06540.html

> thread where you suggest to use CORE type rather than TYPE_CPU and
> enumerate threads under CORE type.
> 
> In case, it didn't come out clearly, I am indeed using core object and
> hotplugging at core granularity for sPAPR. The core sits in the machine slot.
There I was suggesting to handle only CORE at spapr_machine_device_plug,
making core.realize() to complete underlying threads initialization if possible,
and if it you have to do some wiring at machine level for threads then
do it while handling CORE branch.

That way hotplug call-chain would look like:
 device_add(core)
    -> device_realize(core)
        -> core_realize()-> {realize threads}
        -> spapr_machine_device_plug(core)
              connect to board internals for core (maybe threads) 

vs current way:
  device_add(core)
    -> device_realize(core)
        -> core_realize()-> {realize threads}
                             -> device_realize(thread)
                                   -> spapr_machine_device_plug(thread)
                                         some board specific wiring
                             ...
        -> spapr_machine_device_plug(core)
              connect to board internals 

the former is easier to read/follow
> 
> Regards,
> Bharata.
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]