Re: [dpdk-dev] [PATCH v2] vfio: Fix overflow while assigning vfio BAR region offset and size

[Alejandro Lucero <alejandro.lucero@netronome.com>]

Hi all,

>From the kernel VFIO maintainer:

"I suppose in the short term, mmap should not be advertised as available
on 32bit hosts.  Thanks,"

So, as VFIO support for 32bit systems is broken, DPDK should not configure
VFIO in that case.

This is the complete email sent to the kernel maintainer and his answer:

On Thu, 2015-07-02 at 14:42 +0100, Alejandro Lucero wrote:
> Hi Alex,
>
> is VFIO expected to work in 32 bit systems?
>
> I know VFIO initial goal was a better control of device assignment to
> virtual machines and it is based on IOMMU hardware support. I do not know
> how likely is to have a 32 bit system with IOMMU but it seems to be
> possible such hardware configuration.
>
> The problem with VFIO and 32 bit systems is the VFIO kernel code uses the
> upper bits (VFIO_PCI_OFFSET_SHIFT) of a __u64 variable, offset field in
> struct vfio_region_info, for saving info about the PCI BAR index to work
> with. This is done inside the ioctl command VFIO_DEVICE_GET_REGION_INFO.
> That vfio_region_info is got by the process doing the system call and the
> offset is used as a parameter for mmap system call which expects such a
> parameter as unsigned long. If I am not wrong, unsigned long in 32 bit
> linux systems is a 32 bit type, so when the vfio_pci_mmap function is
> executed, the index BAR to work with is obtained from the offset, which
> turns to be always 0 as the value was "lost in translation". There is a
> chance current implementation can work if all the PCI BARs are equal in
> terms of size, but obviously this is not acceptable.
>
> So, if VFIO needs to work in 32 bits systems another way to map the device
> PCI BARs is needed.

Not necessarily, VFIO_PCI_OFFSET_SHIFT is an internal implementation
detail, userspace should always use  vfio_region_info.offset.  We're
therefore free to come up with other algorithms for handling this
limitation.  If we want to continue using a single device file
descriptor, we could simply choose a smaller shift on 32bit systems.  A
29 bit shift would give us 512MB regions support, which is sufficient
for the vast majority of devices.

We could also replace the macros with functions such that we pack
regions as tightly as possible within the device file descriptor.  It's
reasonable to expect that we could support up to 2G BARs using such a
method.  A hybrid approach is also possible, for instance the config
space region could also contain the ROM and VGA areas (or we could
simply choose not to support VGA on 32bit hosts).

If we need to support 4G BARs, our only choice is really to extend the
vfio region support for a separate file descriptor per region.  The only
devices I'm aware of with 4G BARs are Nvidia Tesla.  This is possible,
but I would expect such devices would be extremely rare on 32bit hosts.

I suppose in the short term, mmap should not be advertised as available
on 32bit hosts.  Thanks,

Alex

On Wed, Jul 1, 2015 at 11:00 AM, Burakov, Anatoly <anatoly.burakov@intel.com
> wrote:

> Hi all,
>
> > The last patch from Rahul does not solve the problem. For those cases
> where the MSI-X table is in one of the BARs to map, the memreg array is
> still in use.
>
> Rahul's initial patch was pretty much what you have submitted, it just
> didn't build on a 32-bit system.
>
> > My fix was using unsigned long instead of uint32_t for the memreg array
> as this is used as  a parameter for mmap system call which expects such a
> type for the offset (and size).
>
> Maybe use off_t? That would at least be guaranteed to compile on any
> system...
>
> > In a 32-bit system mmap system call and VFIO mmap implementation will
> get an unsigned long offset, as it does the struct vma_area_struct for
> vm_pgoff.
> > VFIO will not be able to map the right BAR except for BAR 0.
> >
> > So, basically, VFIO kernel code does not work for 32 bit systems.
> >
> > I think we should define memreg as unsigned long and to report this
> problem to the VFIO kernel maintainer.
>
> If that's the case, this should indeed be taken up with the kernel
> maintainers. I don't have a 32-bit system handy to test it, unfortunately.
>
> Thanks,
> Anatoly
>