Yesterday for the first time I consciously noticed the virNodeDeviceDettach and virNodeDeviceReAttach APIs, and found that they are hardcoded to bind to/unbind from the pci-stub driver for qemu, and the "pciback" driver for Xen. If we want these APIs to be useful for VFIO, they will need to bind to the vfio driver instead, but there is currently no method in those APIs to specify which driver to bind to. I guess we could do this with new virNodeDeviceDetachFlags() and virNodeDeviceReAttachFlags() APIs which have a flag to indicate vfio, but before going to that trouble I'd like to know if these APIs are actually used or if they are deprecated (they don't seem to be of any use if the hostdev devices you're assigning have "managed='yes'" - as far as I can see, setting managed='yes' just makes the bind/unbind from the stub driver an automatic part of assigning/un-assigning the device to a guest).

On 04/23/2013 11:46 AM, Daniel P. Berrange wrote: > On Tue, Apr 23, 2013 at 10:52:09AM -0400, Laine Stump wrote: >> Yesterday for the first time I consciously noticed the >> virNodeDeviceDettach and virNodeDeviceReAttach APIs, and found that they >> are hardcoded to bind to/unbind from the pci-stub driver for qemu, and >> the "pciback" driver for Xen. If we want these APIs to be useful for >> VFIO, they will need to bind to the vfio driver instead, but there is >> currently no method in those APIs to specify which driver to bind to. >> >> I guess we could do this with new virNodeDeviceDetachFlags() and >> virNodeDeviceReAttachFlags() APIs which have a flag to indicate vfio, >> but before going to that trouble I'd like to know if these APIs are >> actually used or if they are deprecated (they don't seem to be of any >> use if the hostdev devices you're assigning have "managed='yes'" - as >> far as I can see, setting managed='yes' just makes the bind/unbind from >> the stub driver an automatic part of assigning/un-assigning the device >> to a guest). > The rationale for managed="no", is that in more security paranoid/locked > down environments, admins will not want libvirtd screwing around with > kernel module drivers. The admin would manually setup "pciback" binding > ahead of time when configuring the host. I think we need to continue to > support this scenario in the VFIO world. Okay. But then in that case, since they don't want libvirtd screwing around with kernel module drivers when starting a domain, they also wouldn't want libvirtd screwing around with kernel module drivers at any other time either (i.e. via virNodeDeviceDetach(), right? (I suppose once we have more fine-grained authorization of the libvirt API, it might be possible for one user to call virNodeDeviceDetach() and another could only start a domain with <hostdev managed='no'>, but is the "fine grainedness" really going to be down to the level of which attributes are allowed in certain domain XML elements?) > If we go down the route of extending <hostdev> to have support for > > <driver name="vfio|qemu"/> Hehe. That's *almost" exactly the patchset I'm working on (I was thinking of sending out just the config-change patch now to get it reviewed early and avoid the last minute rush before the freeze). However, since the legacy backend in this case is handled by the kernel KVM module, I had named them "vfio" and "kvm" (I originally thought to use "qemu", but then considered it might confuse people into thinking that it was something done by qemu in usermode). > Then, this says that the virNodeDevice{Detach,ReAttach} methods need > to have new variants which take a 'const char *drivername' parameter. > > NB, the values of 'drivername' would match those used in the domain > XML <driver> element - they would *not* be kernel module names. > > So we'd call > > virNodeDeviceDetachDriver(dev, "qemu"); > virNodeDeviceDetachDriver(dev, "vfio"); (of course with a "flags" argument at the end :-) That's a bit confusing, since "detach" in this case really means "detach it from whatever driver it's currently bound to, and bind it to the 'vfio' driver". But just from reading the function name and args, it *seems* to mean "detach this device from the 'vfio' driver". On the other hand, I don't see a better alternative. And then what about the converse - virNodeDeviceAttachDriver(dev, "vfio") - that really means "detach it from vfio, and attach it to.... well actually *nowhere*! (that's what virNodeDeviceReAttach does - it's not necessarily the inverse of virNodeDeviceDettach) Maybe we need a totally generic virNodeDeviceAttach() that allows us to specify a stub driver name, a "real" driver name, or NULL (in which case it would just be unbound from the current driver)? But there again you get back to the problem of specifying the actual name of a driver vs. an abstracted name like "kvm" or "vfio" (the actual device to bind to for vfio is called "vfio-pci").

Oh, and to top it all off, vfio has a concept of device "groups"; some groups of devices must either be all assigned to the same guest, or if not all are wanted, the others must be held in isolation (to preclude their use by other guests or even by the host). I guess we could handle that with a flag.

On Wed, Apr 24, 2013 at 09:56:34AM -0400, Laine Stump wrote: > On 04/23/2013 12:34 PM, Laine Stump wrote: >> On 04/23/2013 11:46 AM, Daniel P. Berrange wrote: >>> On Tue, Apr 23, 2013 at 10:52:09AM -0400, Laine Stump wrote: >>>> Yesterday for the first time I consciously noticed the >>>> virNodeDeviceDettach and virNodeDeviceReAttach APIs, and found that they >>>> are hardcoded to bind to/unbind from the pci-stub driver for qemu, and >>>> the "pciback" driver for Xen. If we want these APIs to be useful for >>>> VFIO, they will need to bind to the vfio driver instead, but there is >>>> currently no method in those APIs to specify which driver to bind to. >>>> >>>> I guess we could do this with new virNodeDeviceDetachFlags() and >>>> virNodeDeviceReAttachFlags() APIs which have a flag to indicate vfio, >>>> but before going to that trouble I'd like to know if these APIs are >>>> actually used or if they are deprecated (they don't seem to be of any >>>> use if the hostdev devices you're assigning have "managed='yes'" - as >>>> far as I can see, setting managed='yes' just makes the bind/unbind from >>>> the stub driver an automatic part of assigning/un-assigning the device >>>> to a guest). >>> The rationale for managed="no", is that in more security paranoid/locked >>> down environments, admins will not want libvirtd screwing around with >>> kernel module drivers. The admin would manually setup "pciback" binding >>> ahead of time when configuring the host. I think we need to continue to >>> support this scenario in the VFIO world. >> Okay. But then in that case, since they don't want libvirtd screwing >> around with kernel module drivers when starting a domain, they also >> wouldn't want libvirtd screwing around with kernel module drivers at any >> other time either (i.e. via virNodeDeviceDetach(), right? >> >> >> (I suppose once we have more fine-grained authorization of the libvirt >> API, it might be possible for one user to call virNodeDeviceDetach() and >> another could only start a domain with <hostdev managed='no'>, but is >> the "fine grainedness" really going to be down to the level of which >> attributes are allowed in certain domain XML elements?) >> >> >>> If we go down the route of extending <hostdev> to have support for >>> >>> <driver name="vfio|qemu"/> >> Hehe. That's *almost" exactly the patchset I'm working on (I was >> thinking of sending out just the config-change patch now to get it >> reviewed early and avoid the last minute rush before the freeze). >> However, since the legacy backend in this case is handled by the kernel >> KVM module, I had named them "vfio" and "kvm" (I originally thought to >> use "qemu", but then considered it might confuse people into thinking >> that it was something done by qemu in usermode). >> >> >>> Then, this says that the virNodeDevice{Detach,ReAttach} methods need >>> to have new variants which take a 'const char *drivername' parameter. >>> >>> NB, the values of 'drivername' would match those used in the domain >>> XML <driver> element - they would *not* be kernel module names. >>> >>> So we'd call >>> >>> virNodeDeviceDetachDriver(dev, "qemu"); >>> virNodeDeviceDetachDriver(dev, "vfio"); >> (of course with a "flags" argument at the end :-) >> >> That's a bit confusing, since "detach" in this case really means "detach >> it from whatever driver it's currently bound to, and bind it to the >> 'vfio' driver". But just from reading the function name and args, it >> *seems* to mean "detach this device from the 'vfio' driver". On the >> other hand, I don't see a better alternative. >> >> And then what about the converse - virNodeDeviceAttachDriver(dev, >> "vfio") - that really means "detach it from vfio, and attach it to.... >> well actually *nowhere*! (that's what virNodeDeviceReAttach does - it's >> not necessarily the inverse of virNodeDeviceDettach) >> >> Maybe we need a totally generic virNodeDeviceAttach() that allows us to >> specify a stub driver name, a "real" driver name, or NULL (in which case >> it would just be unbound from the current driver)? But there again you >> get back to the problem of specifying the actual name of a driver vs. an >> abstracted name like "kvm" or "vfio" (the actual device to bind to for >> vfio is called "vfio-pci"). > How about this: > > int virNodeDeviceDetachFlags(virNodeDevicePtr dev, > const char *stubDriver, > unsigned int flags); > > stubDriver will be exactly equal to whatever is in the <driver > name='x'/>, and can also be NULL, in which case the "hypervisor default" > will be used. 'stubDriver' implies that you're passing the kernel module name, which this is not. It should be called 'driverName' to reflect the XML attribute name.

> In the meantime, the virPCIDevice object (used internally for the list > of active and inactice PCI devices) will be extended to keep track of > both which stub driver is used, as well as the original driver (if any), > so that we can then have: > > > int virNodeDeviceReAttachFlags(virNodeDEvicePtr dev, > unsigned int flags); > > (it won't need a driver/stubDriver arg, since that will be recorded when > it's detached.) > > OR - we could make it more general-purpose by adding a const char > *driver which can be NULL (in which case it's just restored to its > original state), but also allows naming a specific driver. Nah, we don't need to know the driver name here.

In fact we don't really need a new API at all, unless we have some need for the flags parameter.