On 04/24/2013 10:01 AM, Daniel P. Berrange wrote:
> 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.
Yes, you're right.
>> 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.
Okay. I didn't really see a need for it either, but wanted to bring it
up in case I was missing something.
> In fact we don't
> really need a new API at all, unless we have some need for the
> flags parameter.
Good point. Guess I got carried away :-)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list