On 8/11/2016 9:54 PM, Alex Williamson wrote: > On Thu, 11 Aug 2016 21:29:35 +0530 > Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote: > >> On 8/11/2016 4:30 AM, Alex Williamson wrote: >>> On Thu, 11 Aug 2016 02:53:10 +0530 >>> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote: >>> >>>> On 8/10/2016 12:30 AM, Alex Williamson wrote: >>>>> On Thu, 4 Aug 2016 00:33:52 +0530 >>>>> Kirti Wankhede <kwankhede@xxxxxxxxxx> 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 > ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. ----------------------------------------------------------------------------------- -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
