Re: [libvirt PATCH 0/6] Add ability to create mediated devices in libvirt

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(a)redhat.com&gt; > > > > 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.