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

[Erik Skultety <eskultet@redhat.com>]

On Fri, Apr 27, 2018 at 12:15:01AM +0530, Kirti Wankhede wrote:
>
>
> On 4/26/2018 1:22 AM, Dr. David Alan Gilbert wrote:
> > * Alex Williamson (alex.williamson@redhat.com) wrote:
> >> On Wed, 25 Apr 2018 21:00:39 +0530
> >> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>
> >>> On 4/25/2018 4:29 AM, Alex Williamson wrote:
> >>>> On Wed, 25 Apr 2018 01:20:08 +0530
> >>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>>
> >>>>> On 4/24/2018 3:10 AM, Alex Williamson wrote:
> >>>>>> On Wed, 18 Apr 2018 12:31:53 -0600
> >>>>>> Alex Williamson <alex.williamson@redhat.com> wrote:
> >>>>>>
> >>>>>>> On Mon,  9 Apr 2018 12:35:10 +0200
> >>>>>>> Gerd Hoffmann <kraxel@redhat.com> 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.
> >
> > I'll agree; I don't know enough about these devices, but to give you
> > some example of things I'd expect to work:
> >    a) User adds new machines to their data centre with larger/newer
> > version of the same vendors GPU; in some cases that should work
> > (depending on vendor details etc)
> >    b) The same thing but with identical hardware but a newer driver on
> > the destination.
> >
> > Obviously there will be some cut offs that say some versions are
> > incompatible;  but for normal migration we jump through serious hoops
> > to make sure stuff works; customers will expect the same with some
> > VFIO devices.
> >
>
> How libvirt checks that cut off where some versions are incompatible?
>
>
> >>>>> * 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.
> >
> > Those things certainly happen; state that we forgot to transfer, new
> > features enables on devices, devices configured in different ways.
> >
>
> How libvirt checks migration compatibility for other devices across QEMU
> versions where source support a device and destination running with
> older QEMU version doesn't support that device or that device doesn't
> exist in that system?

We spoke about this on the call, but I'll write it down anyway so that we have
a track of it. Currently, libvirt doesn't support migration of a domain with
devices living outside of QEMU, therefore we'd need a completely new schema to
support this. The other thing I mentioned regarding probing of the migration
capabilities was that we should really consider openstack as both the consumer
and a commander of libvirt, because it's openstack that maintains a global
view of all the hosts in a cluster, so rather than poking libvirt to probe a
random host, I assume they'd already like to have this information beforehand
so that they can incorporate the logic in their scheduler, IOW at the point
where migration is about to happen, openstack should imho already know which
host is capable of hosting the VM to be migrated. That being said, it would
most probably be libvirt who provides openstack with this information just like
openstack probes libvirt for domain capabilities, but the idea stays the same,
ideally we'd need to have this information before users decide to migrate a VM.

Thanks,
Erik