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

[Alejandro Lucero]

Hi Rahul,

Go ahead. That's fine for me.

Thanks

On Fri, Jul 10, 2015 at 10:54 AM, Rahul Lakkireddy <
rahul.lakkireddy at chelsio.com> wrote:

> On Tue, Jul 07, 2015 at 10:50:23 +, Burakov, Anatoly wrote:
> > Hi Rahul,
> >
> > > However, unsigned long seems to be working fine for all builds.
> >
> > unsigned long it is then, if there aren't any other objections.
> >
> > Thanks,
> > Anatoly
>
> Hi Alejandro,
>
> Are you planning to update the original patch as per below and re-submit:
> http://dpdk.org/ml/archives/dev/2015-July/020963.html
>
> Or, I can also submit it if you want.
> Please let me know.
>
> Thanks,
> Rahul
>
>

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

[Rahul Lakkireddy]

On Tue, Jul 07, 2015 at 10:50:23 +, Burakov, Anatoly wrote:
> Hi Rahul,
> 
> > However, unsigned long seems to be working fine for all builds.
> 
> unsigned long it is then, if there aren't any other objections.
> 
> Thanks,
> Anatoly

Hi Alejandro,

Are you planning to update the original patch as per below and re-submit:
http://dpdk.org/ml/archives/dev/2015-July/020963.html

Or, I can also submit it if you want.
Please let me know.

Thanks,
Rahul

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

[Rahul Lakkireddy]

Hi Alejandro,

On Mon, Jul 06, 2015 at 16:45:01 +0100, Alejandro Lucero wrote:
> 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.
> 

Thank you very much for the clarification.

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

Our Chelsio T5 cards can also have 4G bar size. So, it seems this won't work
on 32-bit with current state of kernel vfio driver.

Nevertheless, updating your patch with below diff works fine on 64-bit
for vfio testing on Chelsio T5 cards and it compiles for 32-bit targets
as well.


diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c 
b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index 426953a..6127f5f 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -728,7 +728,7 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
struct vfio_region_info reg = { .argsz = sizeof(reg) };
void *bar_addr;
struct memreg {
-   uint32_t offset, size;
+   unsigned long offset, size;
} memreg[2] = {};

reg.index = i;
@@ -771,7 +771,7 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
RTE_LOG(DEBUG, EAL,
"Trying to map BAR %d that contains the 
MSI-X "
"table. Trying offsets: "
-   "%04x:%04x, %04x:%04x\n", i,
+   "0x%04lx:0x%04lx, 0x%04lx:0x%04lx\n", i,
memreg[0].offset, memreg[0].size,
memreg[1].offset, memreg[1].size);
}


Thanks,
Rahul

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

[Burakov, Anatoly]

Hi Rahul,

> However, unsigned long seems to be working fine for all builds.

unsigned long it is then, if there aren't any other objections.

Thanks,
Anatoly

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

[Burakov, Anatoly]

Hi all,

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

...Or no one should try and run VFIO on a 32-bit system, which should be noted 
in documentation. I'm a bit wary of adding special handling in this case.

Does making the offset off_t fix the compile issues for 32 bit kernel?

Thanks,
Anatoly

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

[Alejandro Lucero]

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  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
>

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

[Burakov, Anatoly]

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

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

[Alejandro Lucero]

I submitted a patch for fixing this issue on the 25th of June. I did not
notice someone had reported this before. 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.

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). This worked for me but I did not realize this
has to be compiled for 32 bit systems as well. In that case unsigned long
will work for the mmap but not for the VFIO kernel API which expects
uint64_t for the offset and size inside the struct vfio_region_info.

The point is, the offset param from the vfio_region_info has the index BAR
to map. For this VFIO kernel code uses VFIO_PCI_INDEX_TO_OFFSET:

 #define VFIO_PCI_OFFSET_SHIFT

40
 #define VFIO_PCI_INDEX_TO_OFFSET
(index
) ((u64
)(index
) <<
VFIO_PCI_OFFSET_SHIFT
)

This index will be used by the VFIO mmap implementation when the DPDK code
tries to map the BARs. That code does the opposite for getting the index:

index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT);

In this case PAGE_SHIFT needs to be used because the mmap system call
modifies the offset previously.

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.




On Tue, Jun 30, 2015 at 10:12 PM, Thomas Monjalon  wrote:

> Hi Anatoly,
> Please could you review this fix to allow Chelsio using VFIO?
> Thanks
>
> 2015-06-23 20:30, Rahul Lakkireddy:
> > When using vfio, the probe fails over Chelsio T5 adapters after
> > commit-id 90a1633b2 (eal/linux: allow to map BARs with MSI-X tables).
> >
> > While debugging further, found that the BAR region offset and size read
> from
> > vfio are u64, but are assigned to uint32_t variables.  This results in
> the u64
> > value getting truncated to 0 and passing wrong offset and size to mmap
> for
> > subsequent BAR regions (i.e. trying to overwrite previously allocated
> BAR 0
> > region).
> >
> > The fix is to use these region offset and size directly rather than
> assigning
> > to uint32_t variables.
> >
> > Fixes: 90a1633b2347 ("eal/linux: allow to map BARs with MSI-X tables")
> > Signed-off-by: Rahul Lakkireddy 
> > Signed-off-by: Kumar Sanghvi 
>
>

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

[Thomas Monjalon]

Hi Anatoly,
Please could you review this fix to allow Chelsio using VFIO?
Thanks

2015-06-23 20:30, Rahul Lakkireddy:
> When using vfio, the probe fails over Chelsio T5 adapters after
> commit-id 90a1633b2 (eal/linux: allow to map BARs with MSI-X tables).
> 
> While debugging further, found that the BAR region offset and size read from
> vfio are u64, but are assigned to uint32_t variables.  This results in the u64
> value getting truncated to 0 and passing wrong offset and size to mmap for
> subsequent BAR regions (i.e. trying to overwrite previously allocated BAR 0
> region).
> 
> The fix is to use these region offset and size directly rather than assigning
> to uint32_t variables.
> 
> Fixes: 90a1633b2347 ("eal/linux: allow to map BARs with MSI-X tables")
> Signed-off-by: Rahul Lakkireddy 
> Signed-off-by: Kumar Sanghvi 

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

[Rahul Lakkireddy]

When using vfio, the probe fails over Chelsio T5 adapters after
commit-id 90a1633b2 (eal/linux: allow to map BARs with MSI-X tables).

While debugging further, found that the BAR region offset and size read from
vfio are u64, but are assigned to uint32_t variables.  This results in the u64
value getting truncated to 0 and passing wrong offset and size to mmap for
subsequent BAR regions (i.e. trying to overwrite previously allocated BAR 0
region).

The fix is to use these region offset and size directly rather than assigning
to uint32_t variables.

Fixes: 90a1633b2347 ("eal/linux: allow to map BARs with MSI-X tables")
Signed-off-by: Rahul Lakkireddy 
Signed-off-by: Kumar Sanghvi 
---
v2:
- For fixing 32-bit build failure, rather than converting uint32_t var to 
uint64_t
  as done in v1, taking a different approach instead to revert a part of above
  commit-id so as to use the original region offset and size directly.
- Add the commit-id that this patch fixes and update commit log.

 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c 
b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index 426953a..ff974f5 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -775,9 +775,6 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
memreg[0].offset, memreg[0].size,
memreg[1].offset, memreg[1].size);
}
-   } else {
-   memreg[0].offset = reg.offset;
-   memreg[0].size = reg.size;
}

/* try to figure out an address */
@@ -815,6 +812,15 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
MAP_FIXED);
}

+   if (!map_addr) {
+   /* Not a BAR containing MSI-X */
+   map_addr = pci_map_resource(bar_addr,
+   vfio_dev_fd,
+   reg.offset,
+   reg.size,
+   MAP_FIXED);
+   }
+
if (map_addr == MAP_FAILED || !map_addr) {
munmap(bar_addr, reg.size);
bar_addr = MAP_FAILED;
-- 
2.4.1