From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43777) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abPbo-000448-Ee for qemu-devel@nongnu.org; Thu, 03 Mar 2016 04:31:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abPbj-0002jY-7E for qemu-devel@nongnu.org; Thu, 03 Mar 2016 04:31:48 -0500 Received: from e23smtp03.au.ibm.com ([202.81.31.145]:41219) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abPbi-0002hr-Lj for qemu-devel@nongnu.org; Thu, 03 Mar 2016 04:31:43 -0500 Received: from localhost by e23smtp03.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 3 Mar 2016 19:31:38 +1000 Date: Thu, 3 Mar 2016 15:00:32 +0530 From: Bharata B Rao Message-ID: <20160303093032.GA5968@in.ibm.com> References: <1456417362-20652-1-git-send-email-bharata@linux.vnet.ibm.com> <1456417362-20652-6-git-send-email-bharata@linux.vnet.ibm.com> <20160229114642.666a958f@nial.brq.redhat.com> <20160301090951.GJ5756@in.ibm.com> <20160301145502.03c58c4c@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160301145502.03c58c4c@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [RFC PATCH v0 5/6] qmp, spapr: Show hot-plugged/pluggable CPU slots in the Machine Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: mjrosato@linux.vnet.ibm.com, agraf@suse.de, thuth@redhat.com, pkrempa@redhat.com, ehabkost@redhat.com, aik@ozlabs.ru, qemu-devel@nongnu.org, armbru@redhat.com, borntraeger@de.ibm.com, qemu-ppc@nongnu.org, afaerber@suse.de, pbonzini@redhat.com, mdroth@linux.vnet.ibm.com, david@gibson.dropbear.id.au 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 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 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 > > > > --- > > > > 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.