* [PATCH] vfio: Fix overflow while assigning vfio BAR region offset and size
@ 2015-06-16 14:16 Rahul Lakkireddy
2015-06-17 12:09 ` Thomas Monjalon
2015-06-23 15:00 ` [PATCH v2] " Rahul Lakkireddy
0 siblings, 2 replies; 16+ messages in thread
From: Rahul Lakkireddy @ 2015-06-16 14:16 UTC (permalink / raw)
To: dev; +Cc: Felix Marti, Kumar Sanghvi, Nirranjan Kirubaharan
After the commit-id 90a1633b2 (eal/linux: allow to map BARs with MSI-X tables),
VFIO stopped working. On further debug, found that although BAR region
offset and size from vfio are read as u64, they are truncated when assigned to
uint32_t variables resulting in wrong offset being passed for mmap.
The fix is to use uint64_t for offset and size.
Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
Signed-off-by: Kumar Sanghvi <kumaras@chelsio.com>
---
lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 4 ++--
1 file changed, 2 insertions(+), 2 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..d0385ff 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;
+ uint64_t 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,
+ "%04lx:%04lx, %04lx:%04lx\n", i,
memreg[0].offset, memreg[0].size,
memreg[1].offset, memreg[1].size);
}
--
2.4.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio: Fix overflow while assigning vfio BAR region offset and size
2015-06-16 14:16 [PATCH] vfio: Fix overflow while assigning vfio BAR region offset and size Rahul Lakkireddy
@ 2015-06-17 12:09 ` Thomas Monjalon
2015-06-18 14:23 ` Rahul Lakkireddy
2015-06-23 15:00 ` [PATCH v2] " Rahul Lakkireddy
1 sibling, 1 reply; 16+ messages in thread
From: Thomas Monjalon @ 2015-06-17 12:09 UTC (permalink / raw)
To: Rahul Lakkireddy; +Cc: dev, Felix Marti, Nirranjan Kirubaharan, Kumar Sanghvi
Hi Rahul,
2015-06-16 19:46, Rahul Lakkireddy:
> After the commit-id 90a1633b2 (eal/linux: allow to map BARs with MSI-X tables),
Please show this information before the Signed-off-by lines:
Fixes: 90a1633b2347 ("eal/linux: allow to map BARs with MSI-X tables")
generated with this git alias:
fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")'
> VFIO stopped working. On further debug, found that although BAR region
I suppose the whole VFIO didn't stopped working.
Please precise the conditions of the failures (large offset, etc).
> offset and size from vfio are read as u64, they are truncated when assigned to
> uint32_t variables resulting in wrong offset being passed for mmap.
>
> The fix is to use uint64_t for offset and size.
Unfortunately, it doesn't build for 32-bit target.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio: Fix overflow while assigning vfio BAR region offset and size
2015-06-17 12:09 ` Thomas Monjalon
@ 2015-06-18 14:23 ` Rahul Lakkireddy
0 siblings, 0 replies; 16+ messages in thread
From: Rahul Lakkireddy @ 2015-06-18 14:23 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, Felix Marti, Nirranjan Kirubaharan, Kumar Sanghvi
Hi Thomas,
On Wed, Jun 17, 2015 at 14:09:35 +0200, Thomas Monjalon wrote:
> Hi Rahul,
>
> 2015-06-16 19:46, Rahul Lakkireddy:
> > After the commit-id 90a1633b2 (eal/linux: allow to map BARs with MSI-X tables),
>
> Please show this information before the Signed-off-by lines:
> Fixes: 90a1633b2347 ("eal/linux: allow to map BARs with MSI-X tables")
> generated with this git alias:
> fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")'
>
Ok. I'll add it in v2.
> > VFIO stopped working. On further debug, found that although BAR region
>
> I suppose the whole VFIO didn't stopped working.
> Please precise the conditions of the failures (large offset, etc).
When using vfio, the probe fails over Chelsio T5 after commit-id 90a1633b2
(eal/linux: allow to map BARs with MSI-X tables). While debugging further, found
that the region offset for BAR 2 returned by VFIO is too large to fit in uint32_t.
Also, before the above commit, the region offset was passed to mmap as it is and so
VFIO is working fine before the above commit.
>
> > offset and size from vfio are read as u64, they are truncated when assigned to
> > uint32_t variables resulting in wrong offset being passed for mmap.
> >
> > The fix is to use uint64_t for offset and size.
>
> Unfortunately, it doesn't build for 32-bit target.
I tried on RHEL-6.0 32-bit with T=i686-native-linuxapp-gcc and it built fine.
So, am I missing something here or some option may be?
I will also try out on newer distro like ubuntu 32-bit and confirm.
Thanks,
Rahul.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] vfio: Fix overflow while assigning vfio BAR region offset and size
2015-06-16 14:16 [PATCH] vfio: Fix overflow while assigning vfio BAR region offset and size Rahul Lakkireddy
2015-06-17 12:09 ` Thomas Monjalon
@ 2015-06-23 15:00 ` Rahul Lakkireddy
2015-06-30 21:12 ` Thomas Monjalon
2015-07-13 8:51 ` [PATCH v3] " Rahul Lakkireddy
1 sibling, 2 replies; 16+ messages in thread
From: Rahul Lakkireddy @ 2015-06-23 15:00 UTC (permalink / raw)
To: dev; +Cc: Felix Marti, Kumar Sanghvi, Nirranjan Kirubaharan
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 <rahul.lakkireddy@chelsio.com>
Signed-off-by: Kumar Sanghvi <kumaras@chelsio.com>
---
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] vfio: Fix overflow while assigning vfio BAR region offset and size
2015-06-23 15:00 ` [PATCH v2] " Rahul Lakkireddy
@ 2015-06-30 21:12 ` Thomas Monjalon
2015-07-01 8:34 ` Alejandro Lucero
2015-07-13 8:51 ` [PATCH v3] " Rahul Lakkireddy
1 sibling, 1 reply; 16+ messages in thread
From: Thomas Monjalon @ 2015-06-30 21:12 UTC (permalink / raw)
To: anatoly.burakov; +Cc: dev, Nirranjan Kirubaharan, Felix Marti, Kumar Sanghvi
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 <rahul.lakkireddy@chelsio.com>
> Signed-off-by: Kumar Sanghvi <kumaras@chelsio.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] vfio: Fix overflow while assigning vfio BAR region offset and size
2015-06-30 21:12 ` Thomas Monjalon
@ 2015-07-01 8:34 ` Alejandro Lucero
2015-07-01 10:00 ` Burakov, Anatoly
0 siblings, 1 reply; 16+ messages in thread
From: Alejandro Lucero @ 2015-07-01 8:34 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, Felix Marti, Nirranjan Kirubaharan, Kumar Sanghvi
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
<http://lxr.free-electrons.com/ident?v=3.13;i=VFIO_PCI_OFFSET_SHIFT>
40
#define VFIO_PCI_INDEX_TO_OFFSET
<http://lxr.free-electrons.com/ident?v=3.13;i=VFIO_PCI_INDEX_TO_OFFSET>(index
<http://lxr.free-electrons.com/ident?v=3.13;i=index>) ((u64
<http://lxr.free-electrons.com/ident?v=3.13;i=u64>)(index
<http://lxr.free-electrons.com/ident?v=3.13;i=index>) <<
VFIO_PCI_OFFSET_SHIFT
<http://lxr.free-electrons.com/ident?v=3.13;i=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 <thomas.monjalon@6wind.com
> 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 <rahul.lakkireddy@chelsio.com>
> > Signed-off-by: Kumar Sanghvi <kumaras@chelsio.com>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] vfio: Fix overflow while assigning vfio BAR region offset and size
2015-07-01 8:34 ` Alejandro Lucero
@ 2015-07-01 10:00 ` Burakov, Anatoly
2015-07-06 15:45 ` Alejandro Lucero
0 siblings, 1 reply; 16+ messages in thread
From: Burakov, Anatoly @ 2015-07-01 10:00 UTC (permalink / raw)
To: Alejandro Lucero, Thomas Monjalon
Cc: dev, Felix Marti, Nirranjan Kirubaharan, Kumar Sanghvi
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] vfio: Fix overflow while assigning vfio BAR region offset and size
2015-07-01 10:00 ` Burakov, Anatoly
@ 2015-07-06 15:45 ` Alejandro Lucero
2015-07-07 7:56 ` Rahul Lakkireddy
2015-07-07 9:08 ` Burakov, Anatoly
0 siblings, 2 replies; 16+ messages in thread
From: Alejandro Lucero @ 2015-07-06 15:45 UTC (permalink / raw)
To: Burakov, Anatoly; +Cc: dev, Felix Marti, Nirranjan Kirubaharan, Kumar Sanghvi
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] vfio: Fix overflow while assigning vfio BAR region offset and size
2015-07-06 15:45 ` Alejandro Lucero
@ 2015-07-07 7:56 ` Rahul Lakkireddy
2015-07-07 9:08 ` Burakov, Anatoly
1 sibling, 0 replies; 16+ messages in thread
From: Rahul Lakkireddy @ 2015-07-07 7:56 UTC (permalink / raw)
To: Alejandro Lucero; +Cc: dev, Felix Marti, Nirranjan Kirubaharan, Kumar Sanghvi
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] vfio: Fix overflow while assigning vfio BAR region offset and size
2015-07-06 15:45 ` Alejandro Lucero
2015-07-07 7:56 ` Rahul Lakkireddy
@ 2015-07-07 9:08 ` Burakov, Anatoly
2015-07-07 10:40 ` Rahul Lakkireddy
1 sibling, 1 reply; 16+ messages in thread
From: Burakov, Anatoly @ 2015-07-07 9:08 UTC (permalink / raw)
To: Alejandro Lucero; +Cc: dev, Felix Marti, Nirranjan Kirubaharan, Kumar Sanghvi
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] vfio: Fix overflow while assigning vfio BAR region offset and size
2015-07-07 9:08 ` Burakov, Anatoly
@ 2015-07-07 10:40 ` Rahul Lakkireddy
2015-07-07 10:50 ` Burakov, Anatoly
0 siblings, 1 reply; 16+ messages in thread
From: Rahul Lakkireddy @ 2015-07-07 10:40 UTC (permalink / raw)
To: Burakov, Anatoly; +Cc: dev, Felix Marti, Nirranjan Kirubaharan, Kumar Sanghvi
Hi Anatoly,
On Tue, Jul 07, 2015 at 09:08:08 +0000, Burakov, Anatoly wrote:
> 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?
>
Making offset off_t seems to fail for x86_x32-native-linuxapp-gcc target with
the following error in 64-bit gcc, but is working fine with i686 and
x86_64 targets:
/home/ubuntu/dpdk/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c: In function ‘pci_vfio_map_resource’:
/home/ubuntu/dpdk/x86_x32-native-linuxapp-gcc/include/rte_common.h:90:30: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
#define RTE_PTR_ADD(ptr, x) ((void*)((uintptr_t)(ptr) + (x)))
^
/home/ubuntu/dpdk/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c:816:25: note: in expansion of macro ‘RTE_PTR_ADD’
void *second_addr = RTE_PTR_ADD(bar_addr, memreg[1].offset);
^
cc1: all warnings being treated as errors
make[7]: *** [eal_pci_vfio.o] Error 1
make[6]: *** [eal] Error 2
make[5]: *** [linuxapp] Error 2
make[4]: *** [librte_eal] Error 2
make[3]: *** [lib] Error 2
make[2]: *** [all] Error 2
make[1]: *** [x86_x32-native-linuxapp-gcc_install] Error 2
make: *** [install] Error 2
However, unsigned long seems to be working fine for all builds.
Thanks,
Rahul
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] vfio: Fix overflow while assigning vfio BAR region offset and size
2015-07-07 10:40 ` Rahul Lakkireddy
@ 2015-07-07 10:50 ` Burakov, Anatoly
2015-07-10 9:54 ` Rahul Lakkireddy
0 siblings, 1 reply; 16+ messages in thread
From: Burakov, Anatoly @ 2015-07-07 10:50 UTC (permalink / raw)
To: Rahul Lakkireddy; +Cc: dev, Felix Marti, Nirranjan Kirubaharan, Kumar Sanghvi
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] vfio: Fix overflow while assigning vfio BAR region offset and size
2015-07-07 10:50 ` Burakov, Anatoly
@ 2015-07-10 9:54 ` Rahul Lakkireddy
2015-07-10 17:27 ` Alejandro Lucero
0 siblings, 1 reply; 16+ messages in thread
From: Rahul Lakkireddy @ 2015-07-10 9:54 UTC (permalink / raw)
To: Alejandro Lucero; +Cc: dev, Felix Marti, Nirranjan Kirubaharan, Kumar Sanghvi
On Tue, Jul 07, 2015 at 10:50:23 +0000, 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] vfio: Fix overflow while assigning vfio BAR region offset and size
2015-07-10 9:54 ` Rahul Lakkireddy
@ 2015-07-10 17:27 ` Alejandro Lucero
0 siblings, 0 replies; 16+ messages in thread
From: Alejandro Lucero @ 2015-07-10 17:27 UTC (permalink / raw)
To: Rahul Lakkireddy; +Cc: dev, Felix Marti, Nirranjan Kirubaharan, Kumar Sanghvi
Hi Rahul,
Go ahead. That's fine for me.
Thanks
On Fri, Jul 10, 2015 at 10:54 AM, Rahul Lakkireddy <
rahul.lakkireddy@chelsio.com> wrote:
> On Tue, Jul 07, 2015 at 10:50:23 +0000, 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
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3] vfio: Fix overflow while assigning vfio BAR region offset and size
2015-06-23 15:00 ` [PATCH v2] " Rahul Lakkireddy
2015-06-30 21:12 ` Thomas Monjalon
@ 2015-07-13 8:51 ` Rahul Lakkireddy
2015-07-14 8:56 ` Thomas Monjalon
1 sibling, 1 reply; 16+ messages in thread
From: Rahul Lakkireddy @ 2015-07-13 8:51 UTC (permalink / raw)
To: dev; +Cc: Felix Marti, Kumar Sanghvi, Nirranjan Kirubaharan
When using vfio, the probe fails for BAR > 0 after the
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.
The fix is to use unsigned long for the offset and size.
This is based on patch by Alejandro Lucero <alejandro.lucero@netronome.com>
posted at below:
http://dpdk.org/ml/archives/dev/2015-June/020201.html
and updated with diff from below to fix 32-bit compilation:
http://dpdk.org/ml/archives/dev/2015-July/020963.html
Fixes: 90a1633b2347 ("eal/linux: allow to map BARs with MSI-X tables")
Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
Signed-off-by: Kumar Sanghvi <kumaras@chelsio.com>
---
v3:
- Use unsigned long instead and updated the commit log.
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 | 4 ++--
1 file changed, 2 insertions(+), 2 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..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);
}
--
2.4.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3] vfio: Fix overflow while assigning vfio BAR region offset and size
2015-07-13 8:51 ` [PATCH v3] " Rahul Lakkireddy
@ 2015-07-14 8:56 ` Thomas Monjalon
0 siblings, 0 replies; 16+ messages in thread
From: Thomas Monjalon @ 2015-07-14 8:56 UTC (permalink / raw)
To: Rahul Lakkireddy; +Cc: dev, Felix Marti, Nirranjan Kirubaharan, Kumar Sanghvi
2015-07-13 14:21, Rahul Lakkireddy:
> When using vfio, the probe fails for BAR > 0 after the
> 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.
>
> The fix is to use unsigned long for the offset and size.
>
> This is based on patch by Alejandro Lucero <alejandro.lucero@netronome.com>
> posted at below:
>
> http://dpdk.org/ml/archives/dev/2015-June/020201.html
>
> and updated with diff from below to fix 32-bit compilation:
>
> http://dpdk.org/ml/archives/dev/2015-July/020963.html
>
> Fixes: 90a1633b2347 ("eal/linux: allow to map BARs with MSI-X tables")
> Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
> Signed-off-by: Kumar Sanghvi <kumaras@chelsio.com>
Applied, thanks
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2015-07-14 8:57 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-16 14:16 [PATCH] vfio: Fix overflow while assigning vfio BAR region offset and size Rahul Lakkireddy
2015-06-17 12:09 ` Thomas Monjalon
2015-06-18 14:23 ` Rahul Lakkireddy
2015-06-23 15:00 ` [PATCH v2] " Rahul Lakkireddy
2015-06-30 21:12 ` Thomas Monjalon
2015-07-01 8:34 ` Alejandro Lucero
2015-07-01 10:00 ` Burakov, Anatoly
2015-07-06 15:45 ` Alejandro Lucero
2015-07-07 7:56 ` Rahul Lakkireddy
2015-07-07 9:08 ` Burakov, Anatoly
2015-07-07 10:40 ` Rahul Lakkireddy
2015-07-07 10:50 ` Burakov, Anatoly
2015-07-10 9:54 ` Rahul Lakkireddy
2015-07-10 17:27 ` Alejandro Lucero
2015-07-13 8:51 ` [PATCH v3] " Rahul Lakkireddy
2015-07-14 8:56 ` Thomas Monjalon
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.