[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: |
Igor Mammedov |
|
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 16:54:49 +0100 |
On Thu, 3 Mar 2016 15:00:32 +0530
Bharata B Rao <address@hidden> wrote:
> 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 ?
I consider links as QOM interface to access Existing CPUs and
they links are more or less useless when it comes to possible CPUs.
If you really have to have links, you can associate them with
cores instead of slots, so it would look like:
/machine/
-> cpu-core[0]
-> cpu-core[1]
...
I'm not suggesting anything related to QOM interface or QOM
in general when we discussing QMP interface in this patch.
On contrary this QMP query should be QOM agnostic so each target
could implement it regardless of what QOM model it uses.
So CPUSlotProperties is a pure QMP construct not tied to QOM
and it's up to machine callback to return CPUs layout description
like it has been proposed in https://patchwork.ozlabs.org/patch/583036/
I think in that thread, I've succeed in convincing Eduardo
and David to set aside QOM interface and implement only QMP command.
(Perhaps looking through that thread will convince you too if you
re-read it). I'll repost that patch taking in account comments
on that and this mail-thread using CPUSlotProperties.
That way we separate interface issue from QOM interface, making
QOM modeling a target implementation detail. Then each target
can implement it's own QOM device model (including links if it wishes)
to play with but still use common QMP interface which will not
be affected by it.
So mgmt would get stable well described/documented interface,
and we could eventually move with device_add based CPU hotplug
from stalemate point. QOM interface would be left for future
exercise or as experimental thing until it became at parity
with QMP one and had time settle down more or less.
>
> Regards,
> Bharata.
>