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

[Kirti Wankhede]

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?

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().

Thanks,
Kirti.

> Thanks,
> 
> Alex
>