[dpdk-dev] [bug] dpdk-vfio: Invalid region/index assumption

[dpdk-dev] [bug] dpdk-vfio: Invalid region/index assumption

[Alex Williamson]

Hi,

I took a quick look at the dpdk vfio code and spotted an invalid
assumption that should probably be corrected ASAP.  That is:

lib/librte_eal/linuxapp/eal/eal_vfio.h:
#define VFIO_GET_REGION_ADDR(x) ((uint64_t) x << 40ULL)
#define VFIO_GET_REGION_IDX(x) (x >> 40)

Region offset to index is an implementation detail of the kernel, the
vfio API defines that the offset of a given region (BAR) is found via
the offset field of struct vfio_region_info returned via the
VFIO_DEVICE_GET_REGION_INFO ioctl.  You're free to cache the offset
into any sort of local variable you like, but the kernel may change the
implementation of region index to offset at any point in time.  This is
explicitly not part of the ABI.  Is there a place to file a bug, or is
this sufficient?  Thanks,

Alex

Re: [dpdk-dev] [bug] dpdk-vfio: Invalid region/index assumption

[Thomas Monjalon]

Hi,

2016-07-27 16:14, Alex Williamson:
> I took a quick look at the dpdk vfio code and spotted an invalid
> assumption that should probably be corrected ASAP.

It can theoretically be a bug but the value may never change in the kernel,
right?
So when you say ASAP, I feel it can wait the next DPDK release
(we plan to release today).
Do you agree?

> That is:
> 
> lib/librte_eal/linuxapp/eal/eal_vfio.h:
> #define VFIO_GET_REGION_ADDR(x) ((uint64_t) x << 40ULL)
> #define VFIO_GET_REGION_IDX(x) (x >> 40)
> 
> Region offset to index is an implementation detail of the kernel, the
> vfio API defines that the offset of a given region (BAR) is found via
> the offset field of struct vfio_region_info returned via the
> VFIO_DEVICE_GET_REGION_INFO ioctl.  You're free to cache the offset
> into any sort of local variable you like, but the kernel may change the
> implementation of region index to offset at any point in time.  This is
> explicitly not part of the ABI.  Is there a place to file a bug, or is
> this sufficient?  Thanks,

Thanks for the report. This email is sufficient :)

Re: [dpdk-dev] [bug] dpdk-vfio: Invalid region/index assumption

[Burakov, Anatoly]

> Hi,
> 
> 2016-07-27 16:14, Alex Williamson:
> > I took a quick look at the dpdk vfio code and spotted an invalid
> > assumption that should probably be corrected ASAP.
> 
> It can theoretically be a bug but the value may never change in the kernel,
> right?
> So when you say ASAP, I feel it can wait the next DPDK release (we plan to
> release today).
> Do you agree?

Unless there are imminent plans to change this in the kernel, I think it can wait for next release.

Thanks,
Anatoly

Re: [dpdk-dev] [bug] dpdk-vfio: Invalid region/index assumption

[Alex Williamson]

On Thu, 28 Jul 2016 09:42:13 +0000
"Burakov, Anatoly" <anatoly.burakov@intel.com> wrote:

> > Hi,
> > 
> > 2016-07-27 16:14, Alex Williamson:  
> > > I took a quick look at the dpdk vfio code and spotted an invalid
> > > assumption that should probably be corrected ASAP.  
> > 
> > It can theoretically be a bug but the value may never change in the kernel,
> > right?
> > So when you say ASAP, I feel it can wait the next DPDK release (we plan to
> > release today).
> > Do you agree?  
> 
> Unless there are imminent plans to change this in the kernel, I think it can wait for next release.

I don't have any plans to change it, but this relationship is not a
guaranteed part of the ABI.  I reserve the right to make such changes
in the future.  Thanks,

Alex

Re: [dpdk-dev] [bug] dpdk-vfio: Invalid region/index assumption

[Santosh Shukla]

On Thu, Jul 28, 2016 at 03:44:57AM +0530, Alex Williamson wrote:
> Hi,
> 
> I took a quick look at the dpdk vfio code and spotted an invalid
> assumption that should probably be corrected ASAP.  That is:
> 
> lib/librte_eal/linuxapp/eal/eal_vfio.h:
> #define VFIO_GET_REGION_ADDR(x) ((uint64_t) x << 40ULL)
> #define VFIO_GET_REGION_IDX(x) (x >> 40)

Yes. I agree. We need some way to carry essential vfio region info in pci_dev,
needed for pread/pwrite. currently, rte_intr_handle only has vfio_dev_fd but
thats not sufficient information. I stumbled while adding ioport support in vfio
and took a short path to define region_idx thatway. To get-rid of this, Possible
approach could be;
- add essential vfio region specific info (ie.. offset, idx, flag) in
  rte_intr_handle. 
- or pull dev_fd to rte_pci_device{}; and define region specific details.

Thanks.

> Region offset to index is an implementation detail of the kernel, the
> vfio API defines that the offset of a given region (BAR) is found via
> the offset field of struct vfio_region_info returned via the
> VFIO_DEVICE_GET_REGION_INFO ioctl.  You're free to cache the offset
> into any sort of local variable you like, but the kernel may change the
> implementation of region index to offset at any point in time.  This is
> explicitly not part of the ABI.  Is there a place to file a bug, or is
> this sufficient?  Thanks,
> 
> Alex