[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH v0 5/6] qmp, spapr: Show hot-plugged/pluggab
From: |
Bharata B Rao |
Subject: |
Re: [Qemu-devel] [RFC PATCH v0 5/6] qmp, spapr: Show hot-plugged/pluggable CPU slots in the Machine |
Date: |
Thu, 3 Mar 2016 15:00:32 +0530 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Tue, Mar 01, 2016 at 02:55:02PM +0100, Igor Mammedov wrote:
> On Tue, 1 Mar 2016 14:39:51 +0530
> Bharata B Rao <address@hidden> wrote:
>
> > On Mon, Feb 29, 2016 at 11:46:42AM +0100, Igor Mammedov wrote:
> > > On Thu, 25 Feb 2016 21:52:41 +0530
> > > Bharata B Rao <address@hidden> wrote:
> > >
> > > > Implement query cpu-slots that provides information about hot-plugged
> > > > as well as hot-pluggable CPU slots that the machine supports.
> > > >
> > > > TODO: As Eric suggested use enum for type instead of str.
> > > > TODO: @hotplug-granularity probably isn't required.
> > > >
> > > > Signed-off-by: Bharata B Rao <address@hidden>
> > > > ---
> > > > hw/core/machine.c | 19 +++++++++
> > > > hw/ppc/spapr.c | 112
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > include/hw/boards.h | 4 ++
> > > > qapi-schema.json | 85 +++++++++++++++++++++++++++++++++++++++
> > > > qmp-commands.hx | 47 ++++++++++++++++++++++
> > > > 5 files changed, 267 insertions(+)
> > > >
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index 6d1a0d8..3055ef8 100644
> > > > --- a/hw/core/machine.c
> > > > +++ b/hw/core/machine.c
> > > > @@ -17,6 +17,25 @@
> > > > #include "hw/sysbus.h"
> > > > #include "sysemu/sysemu.h"
> > > > #include "qemu/error-report.h"
> > > > +#include "qmp-commands.h"
> > > > +
> > > > +/*
> > > > + * QMP: query-cpu-slots
> > > > + *
> > > > + * TODO: Ascertain if this is the right place to for this arch-neutral
> > > > routine.
> > > > + */
> > > > +CPUSlotInfoList *qmp_query_cpu_slots(Error **errp)
> > > > +{
> > > > + MachineState *ms = MACHINE(qdev_get_machine());
> > > > + MachineClass *mc = MACHINE_GET_CLASS(ms);
> > > > +
> > > > + if (!mc->cpu_slots) {
> > > > + error_setg(errp, QERR_UNSUPPORTED);
> > > > + return NULL;
> > > > + }
> > > > +
> > > > + return mc->cpu_slots(ms);
> > > > +}
> > > >
> > > > static char *machine_get_accel(Object *obj, Error **errp)
> > > > {
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 780cd00..b76ed85 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -2453,6 +2453,117 @@ static unsigned
> > > > spapr_cpu_index_to_socket_id(unsigned cpu_index)
> > > > return cpu_index / smp_threads / smp_cores;
> > > > }
> > > >
> > > > +static int spapr_cpuinfo_list(Object *obj, void *opaque)
> > > > +{
> > > > + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> > > > + CPUInfoList ***prev = opaque;
> > > > +
> > > > + if (object_dynamic_cast(obj, TYPE_CPU)) {
> > > > + CPUInfoList *elem = g_new0(CPUInfoList, 1);
> > > > + CPUInfo *s = g_new0(CPUInfo, 1);
> > > > + CPUState *cpu = CPU(obj);
> > > > + PowerPCCPU *pcpu = POWERPC_CPU(cpu);
> > > > +
> > > > + s->arch_id = ppc_get_vcpu_dt_id(pcpu);
> > > > + s->type = g_strdup(object_get_typename(obj));
> > > > + s->thread = cpu->cpu_index;
> > > > + s->has_thread = true;
> > > > + s->core = cpu->cpu_index / smp_threads;
> > > > + s->has_core = true;
> > > > + if (mc->cpu_index_to_socket_id) {
> > > > + s->socket = mc->cpu_index_to_socket_id(cpu->cpu_index);
> > > > + } else {
> > > > + s->socket = cpu->cpu_index / smp_threads / smp_cores;
> > > > + }
> > > > + s->has_socket = true;
> > > > + s->node = cpu->numa_node;
> > > > + s->has_node = true;
> > > > + s->qom_path = object_get_canonical_path(obj);
> > > > + s->has_qom_path = true;
> > > > +
> > > > + elem->value = s;
> > > > + elem->next = NULL;
> > > > + **prev = elem;
> > > > + *prev = &elem->next;
> > > > + }
> > > > + object_child_foreach(obj, spapr_cpuinfo_list, opaque);
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static CPUSlotInfoList *spapr_cpu_slots(MachineState *machine)
> > > > +{
> > > > + CPUSlotInfoList *head = NULL;
> > > > + CPUSlotInfoList **prev = &head;
> > > > + Object *root_container;
> > > > + ObjectProperty *prop;
> > > > + ObjectPropertyIterator iter;
> > > > +
> > > > + /*
> > > > + * TODO: There surely must be a better/easier way to walk all
> > > > + * the link properties of an object ?
> > > > + */
> > > > + root_container = container_get(object_get_root(), "/machine");
> > > > + object_property_iter_init(&iter, root_container);
> > > > +
> > > > + while ((prop = object_property_iter_next(&iter))) {
> > > > + Object *obj;
> > > > + DeviceState *dev;
> > > > + CPUSlotInfoList *elem;
> > > > + CPUSlotInfo *s;
> > > > + CPUInfoList *cpu_head = NULL;
> > > > + CPUInfoList **cpu_prev = &cpu_head;
> > > > + sPAPRCPUCore *core;
> > > > +
> > > > + if (!strstart(prop->type, "link<", NULL)) {
> > > > + continue;
> > > > + }
> > > > +
> > > > + if (!strstart(prop->name, SPAPR_MACHINE_CPU_CORE_PROP, NULL)) {
> > > > + continue;
> > > > + }
> > > > +
> > > > + elem = g_new0(CPUSlotInfoList, 1);
> > > > + s = g_new0(CPUSlotInfo, 1);
> > > > +
> > > > + obj = object_property_get_link(root_container, prop->name,
> > > > NULL);
> > > > + if (obj) {
> > > > + /* Slot populated */
> > > > + dev = DEVICE(obj);
> > > > + core = SPAPR_CPU_CORE(obj);
> > > > +
> > > > + if (dev->id) {
> > > > + s->has_id = true;
> > > > + s->id = g_strdup(dev->id);
> > > > + }
> > > > + s->realized = object_property_get_bool(obj, "realized",
> > > > NULL);
> > > > + s->nr_cpus = core->nr_threads;
> > > > + s->has_nr_cpus = true;
> > > > + s->qom_path = object_get_canonical_path(obj);
> > > > + s->has_qom_path = true;
> > > > + if (s->realized) {
> > > > + spapr_cpuinfo_list(obj, &cpu_prev);
> > > > + }
> > > > + s->has_cpus = true;
> > > > + } else {
> > > > + /* Slot empty */
> > > > + s->has_id = false;
> > > > + s->has_nr_cpus = false;
> > > > + s->has_qom_path = false;
> > > > + s->has_cpus = false;
> > > > + s->realized = false;
> > > > + }
> > > > + s->type = g_strdup(TYPE_SPAPR_CPU_CORE);
> > > > + s->hotplug_granularity = g_strdup(SPAPR_MACHINE_CPU_CORE_PROP);
> > > > + s->slot_id = g_strdup(prop->name);
> > > > + s->cpus = cpu_head;
> > > > + elem->value = s;
> > > > + elem->next = NULL;
> > > > + *prev = elem;
> > > > + prev = &elem->next;
> > > > + }
> > > > + return head;
> > > > +}
> > > > +
> > > > static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > > > {
> > > > MachineClass *mc = MACHINE_CLASS(oc);
> > > > @@ -2482,6 +2593,7 @@ static void spapr_machine_class_init(ObjectClass
> > > > *oc, void *data)
> > > > hc->plug = spapr_machine_device_plug;
> > > > hc->unplug = spapr_machine_device_unplug;
> > > > mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
> > > > + mc->cpu_slots = spapr_cpu_slots;
> > > >
> > > > smc->dr_lmb_enabled = true;
> > > > smc->dr_cpu_enabled = true;
> > > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > > index 0f30959..d888a02 100644
> > > > --- a/include/hw/boards.h
> > > > +++ b/include/hw/boards.h
> > > > @@ -57,6 +57,9 @@ bool machine_mem_merge(MachineState *machine);
> > > > * Set only by old machines because they need to keep
> > > > * compatibility on code that exposed QEMU_VERSION to guests in
> > > > * the past (and now use qemu_hw_version()).
> > > > + * @cpu_slots:
> > > > + * Provides information about populated and yet-to-be populated
> > > > + * CPU slots in the machine. Used by QMP query-cpu-slots.
> > > > */
> > > > struct MachineClass {
> > > > /*< private >*/
> > > > @@ -99,6 +102,7 @@ struct MachineClass {
> > > > HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > > > DeviceState *dev);
> > > > unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
> > > > + CPUSlotInfoList *(*cpu_slots)(MachineState *machine);
> > > > };
> > > >
> > > > /**
> > > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > > index 8d04897..e9a52a2 100644
> > > > --- a/qapi-schema.json
> > > > +++ b/qapi-schema.json
> > > > @@ -4083,3 +4083,88 @@
> > > > ##
> > > > { 'enum': 'ReplayMode',
> > > > 'data': [ 'none', 'record', 'play' ] }
> > > > +
> > > > +##
> > > > +# @CPUInfo:
> > > > +#
> > > > +# Information about CPUs
> > > > +#
> > > > +# @arch-id: Arch-specific ID for the CPU.
> > > > +#
> > > > +# @type: QOM type of the CPU.
> > > > +#
> > > > +# @thread: Thread ID of the CPU.
> > > > +#
> > > > +# @core: Core ID of the CPU.
> > > > +#
> > > > +# @socket: Socket ID of the CPU.
> > > > +#
> > > > +# @node: NUMA node to which the CPU belongs.
> > > > +#
> > > > +# @qom-path: QOM path of the CPU object
> > > > +#
> > > > +# Since: 2.6
> > > > +##
> > > > +
> > > > +{ 'struct': 'CPUInfo',
> > > > + 'data': { 'arch-id': 'int',
> > > > + 'type': 'str',
> > > > + '*thread': 'int',
> > > > + '*core': 'int',
> > > > + '*socket' : 'int',
> > > > + '*node' : 'int',
> > > > + '*qom-path': 'str'
> > > > + }
> > > > +}
> > > > +
> > > > +##
> > > > +# @CPUSlotInfo:
> > > > +#
> > > > +# Information about CPU Slots
> > > > +#
> > > > +# @id: Device ID of the CPU composite object that occupies the slot.
> > > > +#
> > > > +# @type: QOM type of the CPU composite object that occupies the slot.
> > > > +#
> > > > +# @hotplug-granularity: Granularity of CPU composite hotplug for this
> > > > slot,
> > > > +# can be thread, core or socket.
> > > > +#
> > > > +# @slot-id: Slot's identifier.
> > > > +#
> > > > +# @qom-path: QOM path of the CPU composite object that occupies the
> > > > slot.
> > > > +#
> > > > +# @realized: A boolean that indicates whether the slot is filled or
> > > > empty.
> > > > +#
> > > > +# @nr-cpus: Number of CPUs that are part of CPU composite object that
> > > > occupies
> > > > +# this slot.
> > > > +#
> > > > +# @cpus: An array of @CPUInfo elements where each element describes one
> > > > +# CPU that is part of this slot's CPU composite object.
> > > > +#
> > > > +# @type: QOM type
> > > > +#
> > > > +# Since: 2.6
> > > > +##
> > > > +
> > > > +{ 'struct': 'CPUSlotInfo',
> > > > + 'data': { '*id': 'str',
> > > > + 'type': 'str',
> > > > + 'hotplug-granularity' : 'str',
> > > Does it convey any useful info, if yes how it will be used by mgmt?
> >
> > As I noted in the patch desc, this field is useless, will remove.
> >
> > >
> > > > + 'slot-id' : 'str',
> > > > + '*qom-path': 'str',
> > > > + 'realized': 'bool',
> > > field's redundant, presence of qom-path answers this question
> >
> > Ok, makes sense.
> >
> > >
> > > > + '*nr-cpus': 'int',
> > > > + '*cpus' : ['CPUInfo']
> > > I'd suggest to drop above 2 fields as it's impl dependent,
> > > qom-path already point's to socket/core/thread object and
> > > its internal composition can be explored by other means if needed.
> > >
> > > Moreover 'CPUInfo' is confusing and sort of conflicts with existing
> > > 'CpuInfo'.
> >
> > Ah I see, should change the naming if we eventually stick with this
> > implementation.
> >
> > > I'd drop CPUInfo altogether and introduce only 'CPUSlotInfo' here,
> > > existing thread enumeration's already implemented query-cpus.
> >
> > Ok.
> >
> > >
> > > > + }
> > > > +}
> > > What I miss here is that CPUSlotInfo doesn't provide any
> > > information to about where CPU might be hotplugged to.
> > >
> > > Maybe use following tuple instead of slot-id?
> > >
> > > { 'struct': 'CPUSlotProperties',
> > > 'data': { '*node': 'int',
> > > '*socket': 'int',
> > > '*core': 'int',
> > > '*thread': 'int'
> > > }
> > > }
> >
> > Hmm not sure. If I undestand Andreas' proposal correctly, slot is the
> > place where the CPU sits. Machine determines the type of the slot and it
> > can be socket slot, core slot or thread slot based on the granularity
> > of the hotplug supported by the machine. With this I don't see why
> > anything else apart from slot-id/slot-name is required to figure out where
> > to hoplug CPU.
> Of cause it's possible to create and keep at board level map of
> slot-names to whatever other info needed to create a CPU object
> at machine init time, and then make board somehow to apply it
> to the new CPU object before realizing it.
>
> But it doesn't give mgmt any information whatsoever about where CPU
> is being hotplugged. So for x86/arm we would end up with adding
> yet another interface that would tell it. If CPUSlotProperties is used
> instead of slot-name, it could convey that information while
> keeping interface compatible with a range targets we care about
> and extendable to other targets in future.
>
> > In the current implementation, sPAPR hotplug is at core granularity.
> > CPUSlotInfo.type advertises the type as spapr-cpu-core. The number of
> > threads in the core and the CPU model of the threads are either machine
> > default or specified by user during hotplug time.
> now imagine extending in future sPAPR to support NUMA, which way would
> you extend interface, how would mgmt know which CPU belongs to what node?
>
> As you may note in CPUSlotProperties fields are optional so target would
> use only ones it needs. For current sPAPR, query result could be
> something like this:
>
> {
> {
> type: spapr-cpu-core
> properties: { core: 0 }
> qom-path: /machine/unattached/device[xxx]
> }
> {
> type: spapr-cpu-core
> properties: { core: 1 }
> }
> ...
> {
> type: spapr-cpu-core
> properties: { core: 0 }
> }
> }
>
> Later if numa was needed, the board could set 'numa' property as well.
> Would that work for sPAPR?
It could work, however I am finding it difficult to reconcile this with
the machine object to core links that I have in this implementation which
I believe is what Andreas originally suggested. Such a link essentially
represents a slot and it has a name associated with it which identifies the
location where the core is plugged.
Now in order to incorporate CPUSlotProperties, these properties must
be tied to a slot/location object which in the present implementation
doesn't exist. So are you suggesting that we create slot/location
object (abstract ?) that has CPUSlotProperties ? If so, then I guess we
don't need machine object to core object links ?
Regards,
Bharata.