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? You can already define multiple profiles in JSON for mdevctl even with the same UUID and activate them on demand. In terms of migration, in order to migrate persistent mdevs with libvirt we've already talked about pre-creating the configs on the destination with the correct parent address and mdev type which is exactly the same thing one would have to do with the JSON configs of mdevctl and editing the relevant bits. I'm asking because I've been away from this feature for a while, so I may have missed substantial information in this area. Regards, -- Erik Skultety
