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

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?