Re: [Qemu-devel] [PATCH v6 2/4] vfio: VFIO driver for mediated PCI device

[Alex Williamson]

On Thu, 11 Aug 2016 23:16:06 +0530
Kirti Wankhede <address@hidden> wrote:

> On 8/11/2016 9:54 PM, Alex Williamson wrote:
> > On Thu, 11 Aug 2016 21:29:35 +0530
> > Kirti Wankhede <address@hidden> wrote:
> >   
> >> On 8/11/2016 4:30 AM, Alex Williamson wrote:  
> >>> On Thu, 11 Aug 2016 02:53:10 +0530
> >>> Kirti Wankhede <address@hidden> wrote:
> >>>     
> >>>> On 8/10/2016 12:30 AM, Alex Williamson wrote:    
> >>>>> On Thu, 4 Aug 2016 00:33:52 +0530
> >>>>> Kirti Wankhede <address@hidden> wrote:
> >>>>>       
> >>>>
> >>>> ...  
> >>>>>>  #include "vfio_pci_private.h"
> >>>>>>  
> >>>>>> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> >>>>>> index 0ecae0b1cd34..431b824b0d3e 100644
> >>>>>> --- a/include/linux/vfio.h
> >>>>>> +++ b/include/linux/vfio.h
> >>>>>> @@ -18,6 +18,13 @@
> >>>>>>  #include <linux/poll.h>
> >>>>>>  #include <uapi/linux/vfio.h>
> >>>>>>  
> >>>>>> +#define VFIO_PCI_OFFSET_SHIFT   40
> >>>>>> +
> >>>>>> +#define VFIO_PCI_OFFSET_TO_INDEX(off)   (off >> VFIO_PCI_OFFSET_SHIFT)
> >>>>>> +#define VFIO_PCI_INDEX_TO_OFFSET(index) ((u64)(index) << 
> >>>>>> VFIO_PCI_OFFSET_SHIFT)
> >>>>>> +#define VFIO_PCI_OFFSET_MASK    (((u64)(1) << VFIO_PCI_OFFSET_SHIFT) 
> >>>>>> - 1)
> >>>>>> +
> >>>>>> +      
> >>>>>
> >>>>> Nak this, I'm not interested in making this any sort of ABI.
> >>>>>       
> >>>>
> >>>> These macros are used by drivers/vfio/pci/vfio_pci.c and
> >>>> drivers/vfio/mdev/vfio_mpci.c and to use those in both these modules,
> >>>> they should be moved to common place as you suggested in earlier
> >>>> reviews. I think this is better common place. Are there any other
> >>>> suggestion?    
> >>>
> >>> They're only used in ways that I objected to above and you've agreed
> >>> to.  These define implementation details that must not become part of
> >>> the mediated vendor driver ABI.  A vendor driver is free to redefine
> >>> this the same if they want, but as we can see with how easily they slip
> >>> into code where they don't belong, the only way to make sure they don't
> >>> become ABI is to keep them in private headers.
> >>>      
> >>
> >> Then I think, I can't use these macros in mdev modules, they are defined
> >> in drivers/vfio/pci/vfio_pci_private.h
> >> I have to define similar macros in drivers/vfio/mdev/mdev_private.h?
> >>
> >> parent->ops->get_region_info() is called from vfio_mpci_open() that is
> >> before PCI config space is setup. Main expectation from
> >> get_region_info() was to get flags and size. At this point of time
> >> vendor driver also don't know about the base addresses of regions.
> >>
> >>     case VFIO_DEVICE_GET_REGION_INFO:
> >> ...
> >>
> >>         info.offset = vmdev->vfio_region_info[info.index].offset;
> >>
> >> In that case, as suggested in previous reply, above is not going to work.
> >> I'll define such macros in drivers/vfio/mdev/mdev_private.h, set above
> >> offset according to these macros. Then on first access to any BAR
> >> region, i.e. after PCI config space is populated, call
> >> parent->ops->get_region_info() again so that
> >> vfio_region_info[index].offset for all regions are set by vendor driver.
> >> Then use these offsets to calculate 'pos' for
> >> read/write/validate_map_request(). Does this seems reasonable?  
> > 
> > This doesn't make any sense to me, there should be absolutely no reason
> > for the mid-layer mediated device infrastructure to impose region
> > offsets.  vfio-pci is a leaf driver, like the mediated vendor driver.
> > Only the leaf drivers can define how they layout the offsets within the
> > device file descriptor.  Being a VFIO_PCI device only defines region
> > indexes to resources, not offsets (ie. region 0 is BAR0, region 1 is
> > BAR1,... region 7 is PCI config space).  If this mid-layer even needs
> > to know region offsets, then caching them on opening the vendor device
> > is certainly sufficient.  Remember we're talking about the offset into
> > the vfio device file descriptor, how that potentially maps onto a
> > physical MMIO space later doesn't matter here.  It seems like maybe
> > we're confusing those points.  Anyway, the more I hear about needing to
> > reproduce these INDEX/OFFSET translation macros in places they
> > shouldn't be used, the more confident I am in keeping them private.  
> 
> If vendor driver defines the offsets into vfio device file descriptor,
> it will be vendor drivers responsibility that the ranges defined (offset
> to offset + size) are not overlapping with other regions ranges. There
> will be no validation in vfio-mpci, right?

Right, this seems like a pretty basic requirement of the vendor driver
to offer region ranges that do not overlap and there's plenty else
about the vendor driver that the mid-layer can't validate its
behavior...

> In current implementation there is a provision that if
> validate_map_request() callback is not provided, map it to physical
> device's region and start of physical device's BAR address is queried
> using pci_resource_start(). Since with the above change that you are
> proposing, index could not be extracted from offset. Then if vendor
> driver doesn't provide validate_map_request(), return SIGBUS from fault
> handler.
> So that impose indirect requirement that if vendor driver sets
> VFIO_REGION_INFO_FLAG_MMAP for any region, they should provide
> validate_map_request().

TBH, I don't see how providing a default implementation of
validate_map_request() is useful.  How many mediated devices are going
to want to identity map resources from the parent?  Even if they do, it
seems we can only support a single mediated device per parent device
since each will map the same parent resource offset. Let's not even try
to define a default.  If we get a fault and the vendor driver hasn't
provided a handler, send a SIGBUS.  I expect we should also allow
vendor drivers to fill the mapping at mmap() time rather than expecting
this map on fault scheme.  Maybe the mid-level driver should not even be
interacting with mmap() and should let the vendor driver entirely
determine the handling.

For the most part these mid-level drivers, like mediated pci, should be
as thin as possible, and to some extent I wonder if we need them at
all.  We mostly want user interaction with the vfio device file
descriptor to pass directly to the vendor driver and we should only be
adding logic to the mid-level driver when it actually provides some
useful and generic simplification to the vendor driver.  Things like
this default fault handling scheme don't appear to be generic at all,
it's actually a very unique use case I think.  For the most part
I think the mediated interface is just a shim to standardize the
lifecycle of a mediated device for management purposes,
integrate "fake/virtual" devices into the vfio infrastructure,
provide common page tracking, pinning and mapping services, but
the device interface itself should mostly just pass through the
vfio device API straight through to the vendor driver.  Thanks,

Alex