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

[Erik Skultety]

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 at redhat.com>
> > > 
> > > 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.