Re: [RFC PATCH v4 07/10] vfio/pci: introduce a new irq type VFIO_IRQ_TYPE_REMAP_BAR_REGION

From: Alex Williamson <alex.williamson@redhat.com>
To: Yan Zhao <yan.y.zhao@intel.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	cohuck@redhat.com, zhenyuw@linux.intel.com, zhi.a.wang@intel.com,
	kevin.tian@intel.com, shaopeng.he@intel.com, yi.l.liu@intel.com,
	xin.zeng@intel.com, hang.yuan@intel.com
Subject: Re: [RFC PATCH v4 07/10] vfio/pci: introduce a new irq type VFIO_IRQ_TYPE_REMAP_BAR_REGION
Date: Wed, 3 Jun 2020 17:04:52 -0600	[thread overview]
Message-ID: <20200603170452.7f172baf@x1.home> (raw)
In-Reply-To: <20200603014058.GA12300@joy-OptiPlex-7040>

On Tue, 2 Jun 2020 21:40:58 -0400
Yan Zhao <yan.y.zhao@intel.com> wrote:

> On Tue, Jun 02, 2020 at 01:34:35PM -0600, Alex Williamson wrote:
> > I'm not at all happy with this.  Why do we need to hide the migration
> > sparse mmap from the user until migration time?  What if instead we
> > introduced a new VFIO_REGION_INFO_CAP_SPARSE_MMAP_SAVING capability
> > where the existing capability is the normal runtime sparse setup and
> > the user is required to use this new one prior to enabled device_state
> > with _SAVING.  The vendor driver could then simply track mmap vmas to
> > the region and refuse to change device_state if there are outstanding
> > mmaps conflicting with the _SAVING sparse mmap layout.  No new IRQs
> > required, no new irqfds, an incremental change to the protocol,
> > backwards compatible to the extent that a vendor driver requiring this
> > will automatically fail migration.
> >   
> right. looks we need to use this approach to solve the problem.
> thanks for your guide.
> so I'll abandon the current remap irq way for dirty tracking during live
> migration.
> but anyway, it demos how to customize irq_types in vendor drivers.
> then, what do you think about patches 1-5?

In broad strokes, I don't think we've found the right solution yet.  I
really question whether it's supportable to parcel out vfio-pci like
this and I don't know how I'd support unraveling whether we have a bug
in vfio-pci, the vendor driver, or how the vendor driver is making use
of vfio-pci.

Let me also ask, why does any of this need to be in the kernel?  We
spend 5 patches slicing up vfio-pci so that we can register a vendor
driver and have that vendor driver call into vfio-pci as it sees fit.
We have two patches creating device specific interrupts and a BAR
remapping scheme that we've decided we don't need.  That brings us to
the actual i40e vendor driver, where the first patch is simply making
the vendor driver work like vfio-pci already does, the second patch is
handling the migration region, and the third patch is implementing the
BAR remapping IRQ that we decided we don't need.  It's difficult to
actually find the small bit of code that's required to support
migration outside of just dealing with the protocol we've defined to
expose this from the kernel.  So why are we trying to do this in the
kernel?  We have quirk support in QEMU, we can easily flip
MemoryRegions on and off, etc.  What access to the device outside of
what vfio-pci provides to the user, and therefore QEMU, is necessary to
implement this migration support for i40e VFs?  Is this just an
exercise in making use of the migration interface?  Thanks,

Alex