Eduardo Habkost <ehabkost@xxxxxxxxxx> writes:
> On Tue, Dec 13, 2016 at 08:51:34PM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@xxxxxxxxxx> writes:
>>
>> > On Tue, Dec 13, 2016 at 12:04:17PM +0100, Markus Armbruster wrote:
>> >> Quick interface review only:
>> >>
>> >> Eduardo Habkost <ehabkost@xxxxxxxxxx> 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@xxxxxxxxxx>
>> >> > ---
>> >> > 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).
I'm okay with doing (c) including all buses, with full information for
only some. As long as the case "no full information" is clearly
separate, and appropriately documented. Then we can tell libvirt to
always check query-device-slots, and if it has full information, use
that. Else, falling back to hardcoded strategies is okay, similar to
what's done when command query-device-slots doesn't exist.
>> > 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).
Yes.
> 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.
What if we ever run into a kind of slot where the full information is
"use this 'bus' argument" and "you can plug in multiple devices"?
Wouldn't that be indistinguishable from "full information isn't
available"?
Let's make "full information isn't available" impossible to confuse with
any conceivable set of full information.
> 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).
Let's call this one a non-slot, for brevity.
> 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).
Not sure there's much to document. For me a slot takes exactly one plug
by definition (if it can take more, it's a set of slots, isn't it?), and
non-slots take an unknown number of plugs by definition (if we knew,
we'd return suitable slots instead). But that's mincing words; I'm sure
we can come up with language that works for both of us.
> 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).
Works for me.
Additionally, let's replace 'devices': [ 'str' ] by '*device': 'str',
because a slot is either empty or has one device plugged in.
If we ever need to add a set-of-slots type, we'll have to extend the
schema, but then we'll be in a much better position to know what we
actually need.
>> >> > +#
>> >> > +# @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.
Should read code, not docs ;)
>> "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",
Does device_add device_add bus=/machine/q35/pcie.0 even work? I know
qbus_find() interprets /-separated paths, but their semantics are FUBAR
(I can find details if you're curious). We've long advised to use only
values without '/', like bus=pcie.0.
Now, QOM gives us a framework for interpreting paths sanely. Switching
the value of bus to QOM paths would be a compatibility break, though.
Matters only if the botched /-paths are actually used.
> "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.
>> >> [...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
