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

On Tue, Dec 13, 2016 at 08:51:34PM +0100, Markus Armbruster wrote: > Eduardo Habkost <ehabkost(a)redhat.com&gt; writes: > > > On Tue, Dec 13, 2016 at 12:04:17PM +0100, Markus Armbruster wrote: > >> Quick interface review only: > >> > >> Eduardo Habkost <ehabkost(a)redhat.com&gt; writes: > >> > >> > 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 in the case of the other buses; > >> > >> Umm, that doesn't seem right. For instance, a SCSI bus has multiple > >> slots. The slot address is the SCSI ID. An IDE bus may have one (SATA) > >> or two (PATA). For more examples, see qdev-device-use.txt section > >> "Specifying Bus and Address on Bus". > > > > Yes, I should have clarified that: this version changes only PCI > > to expose multiple slots, but other buses also need be changed to > > implement BusClass::enumerate_slots() properly, too. > > > >> > >> > * One slot for each entry from query-hotpluggable-cpus. > >> > In the example below, I am not sure if the PCIe ports are all > >> > supposed to report all slots, but I didn't find any existing > >> > field in PCIBus that would help me figure out the actual number > >> > of slots in a given PCI bus. > >> > > >> > Git tree > >> > -------- > >> > > >> > This patch needs the previous query-machines series I am working > >> > on. The full tree can be found on the git tree at: > >> > > >> > git://github.com/ehabkost/qemu-hacks.git work/query-machines-bus-info > >> > > >> > Example output > >> > -------------- > >> > > >> > The following output was returned by QEMU when running it as: > >> > > >> > $ qemu-system-x86_64 -machine q35 \ > >> > -readconfig docs/q35-chipset.cfg \ > >> > -smp 4,maxcpus=8,sockets=2,cores=2,threads=2 > >> > > >> > { > >> > "return": [ > >> > >> Are you sure >3000 lines of example output make sense here? > > > > I'm not. :) > > > > That's why I need feedback from the PCI experts. I believe most > > of the PCI ports on q35 accept only one device, but I see no code > > implementing that restriction, and no obvious PCIBus or > > PCIBusClass field indicating that. > > > >> > >> [...] > >> > ] > >> > } > >> > > >> > Signed-off-by: Eduardo Habkost <ehabkost(a)redhat.com&gt; > >> > --- > >> > qapi-schema.json | 114 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> > include/hw/qdev-core.h | 6 +++ > >> > hw/core/bus.c | 49 +++++++++++++++++++++ > >> > hw/pci/pci.c | 106 +++++++++++++++++++++++++++++++++------------ > >> > qdev-monitor.c | 86 ++++++++++++++++++++++++++++++++++--- > >> > 5 files changed, 328 insertions(+), 33 deletions(-) > >> > > >> > diff --git a/qapi-schema.json b/qapi-schema.json > >> > index d48ff3f..484d91e 100644 > >> > --- a/qapi-schema.json > >> > +++ b/qapi-schema.json > >> > @@ -3166,6 +3166,120 @@ > >> > { 'command': 'closefd', 'data': {'fdname': 'str'} } > >> > > >> > ## > >> > +# @DeviceSlotType: > >> > +# > >> > +# Type of device slot > >> > +# > >> > +# @generic: Generic device slot, with no bus-specific information > >> > +# @pci: PCI device slot > >> > +# @cpu: CPU device slot > >> > +## > >> > +{ 'enum': 'DeviceSlotType', > >> > + 'data': ['generic', 'pci', 'cpu'] } > >> > + > >> > +## > >> > +# @DeviceSlotInfo: > >> > +# > >> > +# Information on a slot where devices can be plugged. Some buses > >> > +# are represented as a single slot that can support multiple devices, > >> > +# and some buses have multiple slots that are identified by arguments > >> > +# to @device_add. > >> > >> Okay, let me try to wrap my poor, ignorant mind around this. > >> > >> There are two kinds of buses: "single slot that can support multiple > >> devices", and "multiple slots that are identified by arguments". > >> > >> How are the two kinds related to @type? > > > > They are related to @type indirectly because different bus types > > will return different data. But I don't want to make this part of > > the specification: clients should be prepared to handle both > > cases. > > Well, color me confused :) > > > e.g. a QEMU version might return a single generic catch-all > > sysbus-device slot that support any number of devices. Future > > versions may return more detailed information, and return slots > > only for the sysbus devices that really work with the machine. > > Hmm. See below. > > >> > >> Examples for "multiple slots that are identified by arguments": > >> > >> -device edu,addr=06.0,bus=... > >> -device scsi-hd,scsi-hd=5,bus=... > >> -device ide-hd,unit=1,bus=... > >> -device virtserialport,nr=7,bus=... > >> > >> Note that each of these buses has its own way to specify a slot address, > >> namely a bus-specific option. > > > > Yes. > > > >> > >> Can you give examples for "single slot that can support multiple > >> devices"? > > > > I can't name any example except sysbus, right now. > > Sysbus is a bad example, because it's a hack, not a bus. > > Physical devices are wired up in some device-specific way. An important > special wiring case is plugging into a standard socket provided by a > bus. But not every device is connected only to a bus. Devices can also > be wired to other devices in ad hoc ways. > > In the initial qdev design, devices could only plug into buses. > Anything that didn't fit the mold was declared to plug into the "system > bus", which isn't a bus at all. > > The hack used to be fairly harmless, because you couldn't do much with > system bus devices anyway. Not true anymore: Alex created means to add > certain system bus devices to certain machines with -device. Has been a > thorn in my side ever since. I'm afraid we need to understand how > exactly it works before we can finalize the design for this command. I agree it is a hack, I just wanted to be able to cover it too (and possibly other cases where there's no obvious "address" parameter to devices). The other options I had were: a) Omitting the bus from the output; b) Waiting until we fix sysbus before implementing the interface. I avoided (a) because I would like to tell libvirt "always check query-device-slots before plugging anything. if it's not there, it means you can't plug it". To do that, I would need to ensure all buses are present in the list (even sysbus). (But as I say below: I don't love this solution and I am open to suggestions).

> > There are two cases that I tried to cover by reporting > > multiple-device no-argument slots: > > > > 1) Buses that were not converted (yet) to implement > > enumerate_buses() (in the current version: everything except > > PCI) > > 2) Buses that really do not require any extra slot address > > argument (sysbus? others?) > > > > I'm proposing this interface because I don't want (1) or (2) to > > be missing from the returned data (otherwise management software > > can't differentiate "this bus is not available" from "this bus > > simply doesn't implement slot enumeration yet"). > > > > We won't need the special multi-device-slot case if we eliminate > > all instances of (1) and (2). Can we do that in 2.9? I'm not > > sure. > > If we can't do a complete job, and we don't want to hide slots affected > by that, the data we return for them should unequivocally state that > it's incomplete because computing complete data hasn't been implemented, > yet. OK, so we agree we need a way to tell the client the data is incomplete (or not as detailed as we would like).

I tried to solve that by providing a mechanism to tell the client "look, I can tell you that there's a bus called <bus name>, and it is valid to use it as the 'bus' argument for -device and device_add, but I don't know the rest of the rules (sorry!)". We can do that by simply including the bus on the list as if it was a multi-device slot.

But I don't love this solution, so I am open to other suggestions on how to tell the client that slot information is incomplete for a bus. Maybe the following? 1) Add a new DeviceSlotType value meaning "this entry represents a bus/device-type that can't enumerate its slots" (let's call it "bus-with-no-slot-info" by now).

2) Remove DeviceSlotType "generic". 3) Remove max-devices field, and simply document the device-limit semantics for each DeviceSlotType (cpu: 1 device, pci: 1 device, bus-with-no-slot-info: undefined limit).

This looks like a trade-off between moving data to specific union types (and representing type-specific limits/rules in the schema documentation), or moving data to the base union type (and representing type-specific limits as data in the base union type).

> >> > +# > >> > +# @bus: ID of the bus object where the device can be plugged. Optional, > >> > +# as some devices don't need a bus to be plugged (e.g. CPUs). > >> > +# Can be copied to the "bus" argument to @device_add. > >> > >> Suggest something like "Can be used as value for @device_add's bus > >> option". > > > > Will do. > > > >> > >> Should we give similar information on the slot address? The option name > >> is obvious. What about acceptable values? > > > > Actually, this part of the documentation was a leftover from a > > previous version where @bus was inside DeviceSlotInfo. Now I > > moved @bus inside @props for all device types, and it should be > > documented the same way as the other fields inside @props. > > > >> > >> > +# > >> > +# @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. > >> > +# > >> > +# @max-devices: #optional maximum number of devices that can be plugged > >> > +# to the slot. > >> > >> What does it mean when @max-devices isn't given? > > > > It means there is no hard limit on the number of pluggable > > devices. sysbus is one of such cases. > > Sysbus certainly has limits, but they're more complicated than just "at > most @max-devices devices". > > >> > +# > >> > +# @devices: List of QOM paths for devices that are already plugged. > >> > +# > >> > +# @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 device becomes full, or if the machine > >> > +# was already initialized and the slot doesn't support > >> > +# hotplug). > >> > +# > >> > +# @hotpluggable: If true, the slot accepts hotplugged devices. > >> > +# > >> > +# DeviceSlotInfo structs always have a @props member, whose members > >> > +# can be directly copied to the arguments to @device_add. > >> > >> Do you mean names of properties common to all @accepted-device-types? > > > > I mean that it should be safe to simply use the keys+values in > > @props as arguments to device_add or -device, for any slot @type. > > Some clients may want to extract some information from @props (if > > they already manage PCI addresses, for example), but some of them > > may simply choose any available slot and copy @props blindly. > > > > I didn't want to state anything about QOM properties, because I > > only want @props to represent device_add/-device arguments. Some > > of those arguments are consumed by the device_add code itself > > (e.g. "bus") and others are translated to QOM properties. I don't > > want the interface to impose any restrictions on how this is > > implemented. > > Hmm, is this meant to work as follows? To plug a device into this slot, > you use > > { "execute": "device_add", > "arguments": { "driver": TYPE, ... PROPS ... } > > where TYPE is a (concrete subtype of a) member of > @accepted-device-types, and PROPS is the value of @props spliced in. > Example: the data for slot 06.0 of PCI bus pci.0 would be something like > > { > "bus": "pci.0", There's no "bus" field in DeviceSlotInfo. It was a documentation bug.

> "props": { > "bus": "pci.0", > "addr": "06.0" > } > } > > Doesn't match your example output; I guess I'm still misunderstanding > something. This is a busy PCI slot, from my example: { "available": false, "devices": [ "/machine/q35/mch" ], "accepted-device-types": [ "legacy-pci-device", "pci-express-device" ], "props": { "bus": "/machine/q35/pcie.0",

"addr": 0 }, "hotpluggable": false, "type": "pci", "max-devices": 1 }, This is an empty PCI slot from my example. It has available=false because hotplug is not supported on the root bus: { "available": false, "devices": [], "accepted-device-types": [ "legacy-pci-device", "pci-express-device" ], "props": { "bus": "/machine/q35/pcie.0", "addr": 32 }, "hotpluggable": false, "type": "pci", "max-devices": 1 }, This is an empty PCI slot on pc_piix (not on my example): { "available": true, "devices": [], "accepted-device-types": [ "legacy-pci-device" ], "props": { "bus": "/machine/i440fx/pci.0", "addr": 32 }, "hotpluggable": true, "type": "pci", "max-devices": 1 }, This is a free PCIe slot, from my example: { "available": true, "devices": [], "accepted-device-types": [ "pci-express-device" ], "props": { "bus": "/machine/peripheral/ich9-pcie-port-3/ich9-pcie-port-3", "addr": 0 }, "hotpluggable": true, "type": "pci", "max-devices": 1 }, Note that it returns the uint32 value of "addr" (there's some magic on the devfn setter to make it accept both "<slot>[.<function>]" strings and <devfn> integers. On the next version, I plan to fix this insanity and replace "addr" with proper "slot" and "function" integer properties. (See FIXME below) > > > > >> > >> > +## > >> > +{ 'union': 'DeviceSlotInfo', > >> > + 'base': { 'type': 'DeviceSlotType', > >> > + 'accepted-device-types': [ 'str' ], > >> > + '*max-devices': 'int', 'devices': [ 'str' ], > >> > + 'available': 'bool', 'hotpluggable': 'bool' }, > >> > + 'discriminator': 'type', > >> > + 'data': { 'generic': 'GenericSlotInfo', > >> > + 'pci': 'PCISlotInfo', > >> > + 'cpu': 'CPUSlotInfo' } } > >> > + > >> > +## > >> > +# @GenericSlotProperties: > >> > +## > >> > +{ 'struct': 'GenericSlotProperties', > >> > + 'data': { 'bus': 'str' } } > >> > + > >> > + > >> > +## > >> > +# @GenericSlotInfo: > >> > +# > >> > +# Generic slot information, with no bus-specific information > >> > +## > >> > +{ 'struct': 'GenericSlotInfo', > >> > + 'data': { 'props': 'GenericSlotProperties' } } > >> > + > >> > +## > >> > +# @PCIDeviceSlotProperties: > >> > +# > >> > +# Properties that can be set when plugging a PCI device. > >> > +# > >> > +# @addr: "addr" argument to @device_add. > >> > +# > >> > +#FIXME: replace @addr with slot and function properties. > >> > +## > >> > +{ 'struct': 'PCIDeviceSlotProperties', > >> > + 'data': { 'bus': 'str', 'addr': 'int' } } > >> > + > >> > +## > >> > +# @PCISlotInfo: > >> > +# > >> > +# Information on a PCI device slot. > >> > +# > >> > +# @props: The @device_add arguments that can be used when plugging > >> > +# the device. > >> > +## > >> > +{ 'struct': 'PCISlotInfo', > >> > + 'data': { 'props': 'PCIDeviceSlotProperties' } } > >> > + > >> > +## > >> > +# @CPUSlotInfo: > >> > +# > >> > +# Information on a CPU device slot. > >> > +# > >> > +# @props: The @device_add arguments that can be used when plugging > >> > +# the device. > >> > +## > >> > +{ 'struct': 'CPUSlotInfo', > >> > + 'data': { 'props': 'CpuInstanceProperties' } } > >> > + > >> > +## > >> > +# @query-device-slots: > >> > +# > >> > +# Return the list of possible slots for plugging devices using > >> > +# @device_add. > >> > +## > >> > +{ 'command': 'query-device-slots', > >> > + 'returns': [ 'DeviceSlotInfo' ] } > >> > + > >> > +## > >> > # @MachineBusInfo > >> > # > >> > # Information about a bus present on a machine. > >> [...]