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>
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