Re: [libvirt] [PATCH 00/22] support VFIO groups

On 06/24/2013 06:06 AM, Daniel P. Berrange wrote: > On Mon, Jun 24, 2013 at 05:54:49AM -0400, Laine Stump wrote: >> When I first put in support for VFIO device assignment, I didn't >> realize that groups of devices were quite as common as they actually >> are. In particular, I didn't know that often multiple >> seemingly-unrelated devices can end up in the same VFIO iommu group >> due to unlucky circumstances of hardware - they may share a dma >> controller which means that the devices can't truly be isolated from >> each other, and thus should not be simultaneously assigned to >> different guests (or even used by the host) - all of the devices in a >> group should be either assigned to the same guest or, if not assigned >> to the guest, should be isolated off in a driver to prevent them >> from being used by the host. >> >> The following set of patches makes setting that up easier to deal >> with. The end result of all the patches is the following: >> >> 1) The virNodeDevice API will be able to detach or re-attach all the >> devices in a particular group with a single API call. >> >> 2) <hostdev managed='yes'>, <interface type='hostdev' managed='yes'>, >> and <interface type='network' managed='yes'> devices (where the >> network is itself a pool of SRIOV Virtual Functions) can specify: >> >> <driver name='vfio' group='auto'/> >> >> and libvirt will automatically detach (and bind to the 'vfio-pci' >> driver for assignment/isolation) all devices in the same group as >> the device being assigned. Likewise, when the device it detached >> from the guest, a check will be made and, if none of the devices in >> the same group as the device being detach is still in use by a guest > I am concerned that group='auto' is a really incredibly dangerous > setting from the POV of operation of the host OS. > > I can just imagine forum postings / docs saying to use group=auto > and people blindly following it without much inclination as to > what will happen. They will be trying to assign a spare NIC to > their guest; they'll get an error saying it can't be done since it > is part of a group; they'll search google and find a recommendation > to use group=auto to "fix" the problem. libvirt will see that their > SATA controller & graphics card are part of the same group as the > NIC and automatically detach them both from the host OS. Kaboom, > the user is screwed. Yes, I understand (and share) your concern. These patches grew out of claims that there would be a regression in behavior if "managed='yes'" stopped working properly with devices that were in a group, and this was the only way I could see to make it work.

> > With traditional configs, even with managed=yes, you could be sure > that only the single device in the XML would ever be touched. If > there was a conflict due to other devices being on the same PCI > bridge without FLR, then the device would safely fail to be assigned > until the user had explicitly disconnected other devices from the > host. We never attempted to automatically disconnect anything that > was not part of the XML > > Following on from that, how does an application determine what > other devices are present in the group associated with the device > being assigned ? Are we exposing group membership info in the node > device XML anywhere ? Yes. nodedev-dumpxml now has the following information for every device that's in a group (this is added in Patch 20/22): <iommuGroup number='12'> <address domain='0x0000' bus='0x02' slot='0x00' function='0x0'/> <address domain='0x0000' bus='0x02' slot='0x00' function='0x1'/> </iommuGroup> > I'm not sure what else to suggest, other than to say we should not > add this attribute, and require that the application/user explicitly > disconnect any other devices in the same group from the host OS. Any > other option I can think of just sounds too dangerous. I would suggest adding some sort of "assignment white list" to libvirt's config, and requiring any device manipulated in any manner to be on that white list, but in a way it's already possible to create such a list - just manually detach all devices that will be assigned to a guest (and any devices in the same iommu groups) and stop using managed='yes'.

(Just doing that was my original plan, but it had opposition due to the perceived regression in behavior.)