Re: [libvirt] [PATCH 0/3] sample: vfio mdev display devices.

On Wed, 25 Apr 2018 21:00:39 +0530 Kirti Wankhede <kwankhede(a)nvidia.com&gt; wrote: > On 4/25/2018 4:29 AM, Alex Williamson wrote: > > On Wed, 25 Apr 2018 01:20:08 +0530 > > Kirti Wankhede <kwankhede(a)nvidia.com&gt; wrote: > > > >> On 4/24/2018 3:10 AM, Alex Williamson wrote: > >>> On Wed, 18 Apr 2018 12:31:53 -0600 > >>> Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > >>> > >>>> On Mon, 9 Apr 2018 12:35:10 +0200 > >>>> Gerd Hoffmann <kraxel(a)redhat.com&gt; wrote: > >>>> > >>>>> This little series adds three drivers, for demo-ing and testing vfio > >>>>> display interface code. There is one mdev device for each interface > >>>>> type (mdpy.ko for region and mbochs.ko for dmabuf). > >>>> > >>>> Erik Skultety brought up a good question today regarding how libvirt is > >>>> meant to handle these different flavors of display interfaces and > >>>> knowing whether a given mdev device has display support at all. It > >>>> seems that we cannot simply use the default display=auto because > >>>> libvirt needs to specifically configure gl support for a dmabuf type > >>>> interface versus not having such a requirement for a region interface, > >>>> perhaps even removing the emulated graphics in some cases (though I > >>>> don't think we have boot graphics through either solution yet). > >>>> Additionally, GVT-g seems to need the x-igd-opregion support > >>>> enabled(?), which is a non-starter for libvirt as it's an experimental > >>>> option! > >>>> > >>>> Currently the only way to determine display support is through the > >>>> VFIO_DEVICE_QUERY_GFX_PLANE ioctl, but for libvirt to probe that on > >>>> their own they'd need to get to the point where they could open the > >>>> vfio device and perform the ioctl. That means opening a vfio > >>>> container, adding the group, setting the iommu type, and getting the > >>>> device. I was initially a bit appalled at asking libvirt to do that, > >>>> but the alternative is to put this information in sysfs, but doing that > >>>> we risk that we need to describe every nuance of the mdev device > >>>> through sysfs and it becomes a dumping ground for every possible > >>>> feature an mdev device might have. ... > >>>> So I was ready to return and suggest that maybe libvirt should probe > >>>> the device to know about these ancillary configuration details, but > >>>> then I remembered that both mdev vGPU vendors had external dependencies > >>>> to even allow probing the device. KVMGT will fail to open the device > >>>> if it's not associated with an instance of KVM and NVIDIA vGPU, I > >>>> believe, will fail if the vGPU manager process cannot find the QEMU > >>>> instance to extract the VM UUID. (Both of these were bad ideas) > >>> > >>> Here's another proposal that's really growing on me: > >>> > >>> * Fix the vendor drivers! Allow devices to be opened and probed > >>> without these external dependencies. > >>> * Libvirt uses the existing vfio API to open the device and probe the > >>> necessary ioctls, if it can't probe the device, the feature is > >>> unavailable, ie. display=off, no migration. > >>> > >> > >> I'm trying to think simpler mechanism using sysfs that could work for > >> any feature and knowing source-destination migration compatibility check > >> by libvirt before initiating migration. > >> > >> I have another proposal: > >> * Add a ioctl VFIO_DEVICE_PROBE_FEATURES > >> struct vfio_device_features { > >> __u32 argsz; > >> __u32 features; > >> } > >> > >> Define bit for each feature: > >> #define VFIO_DEVICE_FEATURE_DISPLAY_REGION (1 << 0) > >> #define VFIO_DEVICE_FEATURE_DISPLAY_DMABUF (1 << 1) > >> #define VFIO_DEVICE_FEATURE_MIGRATION (1 << 2) > >> > >> * Vendor driver returns bitmask of supported features during > >> initialization phase. > >> > >> * In vfio core module, trap this ioctl for each device in > >> vfio_device_fops_unl_ioctl(), > > > > Whoops, chicken and egg problem, VFIO_GROUP_GET_DEVICE_FD is our > > blocking point with mdev drivers, we can't get a device fd, so we can't > > call an ioctl on the device fd. > > > > I'm sorry, I thought we could expose features when QEMU initialize, but > libvirt needs to know supported features before QEMU initialize. > > > >> check features bitmask returned by vendor > >> driver and add a sysfs file if feature is supported that device. This > >> sysfs file would return 0/1. > > > > I don't understand why we have an ioctl interface, if the user can get > > to the device fd then we have existing interfaces to probe these > > things, it seems like you're just wanting to pass a features bitmap > > through to vfio_add_group_dev() that vfio-core would expose through > > sysfs, but a list of feature bits doesn't convey enough info except for > > the most basic uses. > > > > Yes, vfio_add_group_dev() seems to be better way to convey features to > vfio core. > > >> For migration this bit will only indicate if host driver supports > >> migration feature. > >> > >> For source and destination compatibility check libvirt would need more > >> data/variables to check like, > >> * if same type of 'mdev_type' device create-able at destination, > >> i.e. if ('mdev_type'->available_instances > 0) > >> > >> * if host_driver_version at source and destination are compatible. > >> Host driver from same release branch should be mostly compatible, but if > >> there are major changes in structures or APIs, host drivers from > >> different branches might not be compatible, for example, if source and > >> destination are from different branches and one of the structure had > >> changed, then data collected at source might not be compatible with > >> structures at destination and typecasting it to changed structures would > >> mess up migrated data during restoration. > > > > Of course now you're asking that libvirt understand the release > > versioning scheme of every vendor driver and that it remain > > programatically consistent. We can't even do this with in-kernel > > drivers. And in the end, still the best we can do is guess. > > > > Libvirt doesn't need to understand the version, libvirt need to do > strcmp version string from source and destination. If those are equal, > then libvirt would understand that they are compatible. Who's to say that the driver version and migration compatibility have any relation at all? Some drivers might focus on designing their own migration interface that can maintain compatibility across versions (QEMU does this), some drivers may only allow identical version migration (which is going to frustrate upper level management tools and customers - RHEL goes to great extents to support cross version migration). We cannot have a one size fits all here that driver version defines completely the migration compatibility.

> >> * if guest_driver_version is compatible with host driver at destination. > >> For mdev devices, guest driver communicates with host driver in some > >> form. If there are changes in structures/APIs of such communication, > >> guest driver at source might not be compatible with host driver at > >> destination. > > > > And another guess plus now the guest driver is involved which libvirt > > has no visibility to. > > > > Like above libvirt need to do strcmp. Insufficient, imo > >> 'available_instances' sysfs already exist, later two should be added by > >> vendor driver which libvirt can use for migration compatibility check. > > > > As noted previously, display and migration are not necessarily > > mdev-only features, it's possible that vfio-pci or vfio-platform could > > also implement these, so the sysfs interface cannot be restricted to > > the mdev template and lifecycle interface. > > > > I agree. > Feature bitmask passed to vfio core is not mdev specific. But here > 'available_instances' for migration compatibility check is mdev > specific. If mdev device is not create-able at destination, there is no > point in initiating migration by libvirt. 'available_instances' for migration compatibility check...? We use available_instances to know whether we have the resources to create a given mdev type. It's certainly a prerequisite to have a device of the identical type at the migration target and how we define what is an identical device for a directly assigned PCI device is yet another overly complicated rat hole. But an identical device doesn't necessarily imply migration compatibility and I think that's the problem we're tackling. We cannot assume based only on the device type that migration is compatible, that's basically saying we're never going to have any bugs or oversights or new features in the migration stream.

Chatting with Laine, it may be worth a step back to include migration experts and people up the stack with more visibility to how openstack operates. The issue here is that if vfio gains migration support then we have a portion of the migration stream that is not under the control of QEMU, we cannot necessarily tie it to a QEMU machine type and we cannot necessarily dictate how the vfio bus driver (vendor driver) handles versioning and compatibility. My intent was to expose some sort of migration information through the vfio API so that upper level tools could determine source and target compatibility, but this in itself is I think something new that those tools need to agree how it might be done. How would something like openstack want to handle not only finding a migration target with a compatible device, but also verifying if the device supports the migration format of the source device? Alternatively, should we do anything? Is the problem too hard and we should let the driver return an error when it receives an incompatible migration stream, aborting the migration?

> > One more try... we have a vfio_group fd. This is created by the bus > > drivers calling vfio_add_group_dev() and registers a struct device, a > > struct vfio_device_ops, and private data. Typically we only wire the > > device_ops to the resulting file descriptor we get from > > VFIO_GROUP_GET_DEVICE_FD, but could we enable sort of a nested ioctl > > through the group fd? The ioctl would need to take a string arg to > > match to a device name, plus an ioctl cmd and arg for the device_ops > > ioctl. The group ioctl would need to filter cmds to known, benign > > queries. We'd also need to verify that the allowed ioctls have no > > dependencies on setup done in device_ops.open(). > > So these ioctls would be called without devices open() call, doesn't > this seem to be against file operations standard? vfio_device_ops is modeled largely after file operations, but I don't think we're bound by that for the interaction between vfio-core and the vfio bus drivers. We could make a separate callback for unprivileged ioctls, but that seems like more work per driver when we really want to maintain the identical API, we just want to provide a more limited interface and change the calling point. An issue I thought of for migration though is that this path wouldn't have access to the migration region and therefore if we place a header within that region containing the compatibility and versioning information, the user still couldn't access it. This doesn't seem to be a blocker though as we could put that information within the region capability that defines the region as used for migration. Possibly a device could have multiple migration regions with different formats for backwards compatibility, of course then we'd need a way to determine which to use and which combinations have been validated. Thanks, Alex