On 8/11/2016 9:54 PM, Alex Williamson wrote: > On Thu, 11 Aug 2016 21:29:35 +0530 > Kirti Wankhede <kwankh...@nvidia.com> wrote: > >> On 8/11/2016 4:30 AM, Alex Williamson wrote: >>> On Thu, 11 Aug 2016 02:53:10 +0530 >>> Kirti Wankhede <kwankh...@nvidia.com> wrote: >>> >>>> On 8/10/2016 12:30 AM, Alex Williamson wrote: >>>>> On Thu, 4 Aug 2016 00:33:52 +0530 >>>>> Kirti Wankhede <kwankh...@nvidia.com> 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 >
