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

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.

>> * 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.

>> '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.

> 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?