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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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).

> 
> > 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.
> >> [...]

-- 
Eduardo

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux