docs/schemas/nodedev.rng | 6 + 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(-)
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 + 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(-) -- 2.21.1
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 + > 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@redhat.com> Michal
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@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. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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@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
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@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. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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@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.
© 2016 - 2024 Red Hat, Inc.