Re: [Qemu-devel] [RFC v2] qmp: query-device-slots command

[Eduardo Habkost]

On Thu, Dec 15, 2016 at 10:36:17AM +0100, Igor Mammedov wrote:
> On Wed, 14 Dec 2016 17:39:08 -0200
> Eduardo Habkost <address@hidden> wrote:
> 
> > This adds a new command to QMP: query-device-slots. It will allow
> > management software to query possible slots where devices can be
> > plugged.
> > 
> > This implementation of the command will return:
> > 
> > * Multiple PCI slots per bus, in the case of PCI buses;
> > * One slot per bus for the other buses (that don't
> >   implement slot enumeration yet);
> > * One slot for each entry from query-hotpluggable-cpus.
> Perhaps command should handle slots enumeration for DIMM/NV-DIMM
> devices as well which are also bus-less as cpus but don't have
> dedicated command for possible slots enumeration.

Good idea. I will take a look.

> 
> [...]
> > +##
> > +# @DeviceSlotInfo:
> > +#
> > +# Information on a slot where devices can be plugged.
> > +#
> > +# @type: type of device slot.
> > +#
> > +# @accepted-device-types: List of device types accepted by the slot.
> > +#                         Any device plugged to the slot should implement
> > +#                         one of the accepted device types.
> > +#
> > +# @available: If false, the slot is not available for plugging any device.
> > +#             This value can change at runtime if condition changes
> > +#             (e.g. if the slot becomes full, or if the machine
> > +#             was already initialized and the slot doesn't support
> > +#             hotplug).
> or slot has been freed as result of unplug action

True. I will update it.

> 
> 
> > +# @hotpluggable: If true, the slot accepts hotplugged devices.
> > +#
> > +# @props: The arguments that should be given to @device_add if plugging
> > +#         a device to this slot.
> Could it be a list of arbitrary properties?
> What do we gain making it a union of fixed types which are in the end just 
> property lists?

For the same reason HotpluggableCPU.props is
CpuInstanceProperties instead of an arbitrary list. It forces us
to explicitly document how the returned data behaves, while still
allowing clients to simply use the field like an opaque
dictionary (if they want to).

> 
> [...]
> > +{
> > +    SlotListState s = { };
> > +    MachineState *ms = MACHINE(qdev_get_machine());
> > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> > +
> > +    s.machine = ms;
> > +    s.next = &s.result;
> > +
> > +    /* We build the device slot list from two sources:
> > +     * 1) Calling the BusClass::enumerate_slots() method on all buses;
> > +     * 2) The return value of MachineClass::query_hotpluggable_cpus()
> > +     */
> > +
> > +
> > +    object_child_foreach_recursive(qdev_get_machine(), walk_bus, &s);
> > +    if (s.err) {
> > +        goto out;
> > +    }
> > +
> > +    if (mc->query_hotpluggable_cpus) {
> > +        HotpluggableCPUList *hcl = mc->query_hotpluggable_cpus(ms);
> > +        HotpluggableCPUList *i;
> > +
> > +        for (i = hcl; i; i = i->next) {
> > +            DeviceSlotInfoList *r = g_new0(DeviceSlotInfoList, 1);
> > +            HotpluggableCPU *hc = i->value;
> > +            r->value = g_new0(DeviceSlotInfo, 1);
> > +            r->value->type = DEVICE_SLOT_TYPE_CPU;
> > +            r->value->accepted_device_types = g_new0(strList, 1);
> > +            r->value->accepted_device_types->value = g_strdup(hc->type);
> > +            r->value->available = !hc->has_qom_path;
> > +            /*TODO: should it be always true? */
> > +            r->value->hotpluggable = true;
> it shouldn't be true for BSP, and for the rest of x86 cpus it's true
> provided machine versions supports cpu hotplug.
> 
> But it might be another story for SPAPR or s390,
> CCing maintainers.

Thanks!

> 
> > +
> > +            r->value->u.cpu.props = QAPI_CLONE(CpuInstanceProperties,
> > +                                               hc->props);
> > +            *s.next = r;
> > +            s.next = & r->next;
> > +        }
> > +
> > +        qapi_free_HotpluggableCPUList(hcl);
> > +    }
> > +
> > +out:
> > +    error_propagate(errp, s.err);
> > +    return s.result;
> > +}
> > +
> >  
> >  #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", 
> > ## __VA_ARGS__)
> >  static void qbus_print(Monitor *mon, BusState *bus, int indent);
> 

-- 
Eduardo