On Wed, May 20, 2020 at 10:09:54AM +0100, Daniel P. Berrangé wrote: > On Wed, May 20, 2020 at 10:32:05AM +0200, Erik Skultety wrote: > > On Mon, May 11, 2020 at 05:51:13PM +0100, Daniel P. Berrangé wrote: > > > On Mon, May 11, 2020 at 03:28:02PM +0200, Michal Privoznik wrote: > > > > On 4/30/20 11:42 PM, Jonathon Jongsma wrote: > > > > > This is the first portion of an effort to support persistent mediated devices > > > > > with libvirt. This first series simply enables creating and destroying > > > > > non-persistent mediated devices via the virNodeDeviceCreateXML() and > > > > > virNodeDeviceDestroy() functions. The 'mdevctl' utility[1] provides the backend > > > > > implementation. > > > > > > > > > > [1] https://github.com/mdevctl/mdevctl > > > > > > > > > > Jonathon Jongsma (6): > > > > > nodedev: factor out nodeDeviceHasCapability() > > > > > nodedev: add support for mdev attributes > > > > > nodedev: refactor nodeDeviceFindNewDevice() > > > > > nodedev: store mdev UUID in mdev caps > > > > > nodedev: add mdev support to virNodeDeviceCreateXML() > > > > > nodedev: add mdev support to virNodeDeviceDestroy() > > > > > > > > > > docs/schemas/nodedev.rng | 6 + > > > > > > docs/formatnode.html.in needs some documentation and examples > > > > > > > > libvirt.spec.in | 3 + > > > > > m4/virt-external-programs.m4 | 3 + > > > > > src/conf/node_device_conf.c | 59 ++++- > > > > > src/conf/node_device_conf.h | 3 + > > > > > src/conf/virnodedeviceobj.c | 34 +++ > > > > > src/conf/virnodedeviceobj.h | 3 + > > > > > src/libvirt_private.syms | 3 + > > > > > src/node_device/node_device_driver.c | 326 ++++++++++++++++++++++++--- > > > > > src/node_device/node_device_udev.c | 5 +- > > > > > src/util/virmdev.c | 12 + > > > > > src/util/virmdev.h | 11 + > > > > > 12 files changed, 425 insertions(+), 43 deletions(-) > > > > > > > > > > > > > > > > > Codewise, this looks good. I will let Erik review the semantics of creating > > > > mdevs. > > > > > > > > Reviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx> > > > > > > This is notably lacking any unit test coverage, so is not validating the > > > RNG schema or the XML parser or conversion of XML to mdevctl args. I think > > > that needs fixing before we accept it. > > > > Agreed. > > > > So I played with the series and found a want to make a few points. > > > > Apparently, this is the minimalistic XML that would work: > > <device> > > <parent>pci_0000_06_00_0</parent> > > <capability type='mdev'> > > <type id='nvidia-11'/> > > <iommuGroup number='71'/> > > </capability> > > </device> > > > > Which means we should make iommuGroup optional, because it's a readonly > > element, users are not supposed to specify the iommu group and as a setting > > it's also ignored, because it's figured out by the parent device driver (I > > think) when the mdev is created. > > > > Like Daniel mentioned, some documentation would be nice, especially clarifying > > that the <name> and <path> elements are also read only and any attempt to set > > them would be ignored - well, simply because we're reusing the XML structure > > we've been using for ages to only report results back, not consume them back > > ourselves. > > > > It's good to think ahead to the future with the additional attributes, but I > > don't know about any attributes that vGPUs would accept, so I can't comment on > > that really even if I wanted to, have you tried with some other non-vGPU type of > > mdev? > > > > I finally got to try the mdevctl utility directly and seeing what it can do and > > how it does things, I have to re-iterate the question what benefit does libvirt > > bring in terms of creating/defining the mdevs? > > Libvirt provides a consistent API to control the host, with authenticaton, > acess control and separation. In OpenStack case, Nova runs as a non-root > account and so anything where libvirt doesn't expose functionality would > force them to resort to sudo rules which is not attractive. In the OpenShift > demo installer, the libvirt client is running inside a container which is > permitted to connect to libvirt. They don't ave ability to run mdevctl > at all given the separate container image they're in. There's going to be > similar benefits for other applications. Okay, thanks for refreshing my memory, the OpenShift use case is new to me, but it makes sense.
