* Re: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
2020-07-08 9:24 [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource alvinx.zhang
@ 2020-07-08 9:38 ` David Marchand
2020-07-08 13:58 ` Xia, Chenbo
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: David Marchand @ 2020-07-08 9:38 UTC (permalink / raw)
To: Tal Shnaiderman, Burakov, Anatoly
Cc: dev, Beilei Xing, Jeff Guo, Ma, LihongX, Zhang, AlvinX
On Wed, Jul 8, 2020 at 11:26 AM <alvinx.zhang@intel.com> wrote:
>
> From: Alvin Zhang <alvinx.zhang@intel.com>
>
> When mapping a PCI BAR containing an MSI-X table, some devices do not
> need to actually map this BAR or only need to map part of them, which
> may cause the mapping to fail. Now some checks are added and a non-NULL
> initial value is set to the variable to avoid this situation.
>
> Fixes: 2fd3567e5425 ("pci: use OS generic memory mapping functions")
> Cc: talshn@mellanox.com
>
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
I guess this had to do with:
https://bugs.dpdk.org/show_bug.cgi?id=503
Review please.
--
David Marchand
> ---
> drivers/bus/pci/linux/pci_vfio.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index fdeb9a8..9143bfc 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -547,6 +547,14 @@
> bar_index,
> memreg[0].offset, memreg[0].size,
> memreg[1].offset, memreg[1].size);
> +
> + if (memreg[0].size == 0 && memreg[1].size == 0) {
> + /* No need to map this BAR */
> + RTE_LOG(DEBUG, EAL, "Skipping BAR%d\n", bar_index);
> + bar->size = 0;
> + bar->addr = 0;
> + return 0;
> + }
> } else {
> memreg[0].offset = bar->offset;
> memreg[0].size = bar->size;
> @@ -556,7 +564,9 @@
> bar_addr = mmap(bar->addr, bar->size, 0, MAP_PRIVATE |
> MAP_ANONYMOUS | additional_flags, -1, 0);
> if (bar_addr != MAP_FAILED) {
> - void *map_addr = NULL;
> + /* Set non NULL initial value for in case of no PCI mapping */
> + void *map_addr = bar_addr;
> +
> if (memreg[0].size) {
> /* actual map of first part */
> map_addr = pci_map_resource(bar_addr, vfio_dev_fd,
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
2020-07-08 9:24 [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource alvinx.zhang
2020-07-08 9:38 ` David Marchand
@ 2020-07-08 13:58 ` Xia, Chenbo
2020-07-09 5:13 ` Zhang, AlvinX
2020-07-09 2:25 ` Xiao, QimaiX
2020-07-10 9:54 ` David Marchand
3 siblings, 1 reply; 12+ messages in thread
From: Xia, Chenbo @ 2020-07-08 13:58 UTC (permalink / raw)
To: Zhang, AlvinX, dev
Cc: Xing, Beilei, Guo, Jia, David Marchand, Tal Shnaiderman, Burakov,
Anatoly, thomas
Hi Alvin,
CC the maintainers. Comments below.
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of alvinx.zhang@intel.com
> Sent: Wednesday, July 8, 2020 5:25 PM
> To: dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>
> Subject: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
>
> From: Alvin Zhang <alvinx.zhang@intel.com>
>
> When mapping a PCI BAR containing an MSI-X table, some devices do not need
> to actually map this BAR or only need to map part of them, which may cause the
> mapping to fail. Now some checks are added and a non-NULL initial value is set
> to the variable to avoid this situation.
>
> Fixes: 2fd3567e5425 ("pci: use OS generic memory mapping functions")
> Cc: talshn@mellanox.com
>
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> ---
> drivers/bus/pci/linux/pci_vfio.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index fdeb9a8..9143bfc 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -547,6 +547,14 @@
> bar_index,
> memreg[0].offset, memreg[0].size,
> memreg[1].offset, memreg[1].size);
> +
> + if (memreg[0].size == 0 && memreg[1].size == 0) {
> + /* No need to map this BAR */
> + RTE_LOG(DEBUG, EAL, "Skipping BAR%d\n", bar_index);
> + bar->size = 0;
> + bar->addr = 0;
> + return 0;
> + }
I'm not sure if this corner case will happen. If you confirmed it,
Just ignore this.
> } else {
> memreg[0].offset = bar->offset;
> memreg[0].size = bar->size;
> @@ -556,7 +564,9 @@
> bar_addr = mmap(bar->addr, bar->size, 0, MAP_PRIVATE |
> MAP_ANONYMOUS | additional_flags, -1, 0);
> if (bar_addr != MAP_FAILED) {
> - void *map_addr = NULL;
> + /* Set non NULL initial value for in case of no PCI mapping */
> + void *map_addr = bar_addr;
> +
I see the issue that this patch wants to fix is based on an old kernel.
In older vfio-pci kernel module, MSI related reg cannot be mmaped
in userspace while in newer kernel it can be. That's why sometimes
it cannot be reproduced (https://bugs.dpdk.org/show_bug.cgi?id=503)
So under the condition of old kernel, there could be an example that
Memreg 0 has size 0 but Memreg 1 has non-zero size, which leads to
Memreg 1 cannot be mmaped.
So I'm fine with this part of code change. As this issue is blocking test,
we should do fast confirm and review.
Thanks,
Chenbo
> if (memreg[0].size) {
> /* actual map of first part */
> map_addr = pci_map_resource(bar_addr, vfio_dev_fd,
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
2020-07-08 13:58 ` Xia, Chenbo
@ 2020-07-09 5:13 ` Zhang, AlvinX
2020-07-09 5:21 ` Xia, Chenbo
0 siblings, 1 reply; 12+ messages in thread
From: Zhang, AlvinX @ 2020-07-09 5:13 UTC (permalink / raw)
To: Xia, Chenbo, dev
Cc: David Marchand, Tal Shnaiderman, Burakov, Anatoly, thomas
Hi Chenbo,
Thanks your comments.
> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Wednesday, July 8, 2020 9:58 PM
> To: Zhang, AlvinX <alvinx.zhang@intel.com>; dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>; David
> Marchand <david.marchand@redhat.com>; Tal Shnaiderman
> <talshn@mellanox.com>; Burakov, Anatoly <anatoly.burakov@intel.com>;
> thomas@monjalon.net
> Subject: RE: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
>
> Hi Alvin,
>
> CC the maintainers. Comments below.
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of alvinx.zhang@intel.com
> > Sent: Wednesday, July 8, 2020 5:25 PM
> > To: dev@dpdk.org
> > Cc: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>
> > Subject: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
> >
> > From: Alvin Zhang <alvinx.zhang@intel.com>
> >
> > When mapping a PCI BAR containing an MSI-X table, some devices do not
> > need to actually map this BAR or only need to map part of them, which
> > may cause the mapping to fail. Now some checks are added and a
> > non-NULL initial value is set to the variable to avoid this situation.
> >
> > Fixes: 2fd3567e5425 ("pci: use OS generic memory mapping functions")
> > Cc: talshn@mellanox.com
> >
> > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > ---
> > drivers/bus/pci/linux/pci_vfio.c | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > b/drivers/bus/pci/linux/pci_vfio.c
> > index fdeb9a8..9143bfc 100644
> > --- a/drivers/bus/pci/linux/pci_vfio.c
> > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > @@ -547,6 +547,14 @@
> > bar_index,
> > memreg[0].offset, memreg[0].size,
> > memreg[1].offset, memreg[1].size);
> > +
> > + if (memreg[0].size == 0 && memreg[1].size == 0) {
> > + /* No need to map this BAR */
> > + RTE_LOG(DEBUG, EAL, "Skipping BAR%d\n", bar_index);
> > + bar->size = 0;
> > + bar->addr = 0;
> > + return 0;
> > + }
>
> I'm not sure if this corner case will happen. If you confirmed it, Just ignore this.
In theory, it is entirely possible if the misx-table size is equal to the bar size.
>
> > } else {
> > memreg[0].offset = bar->offset;
> > memreg[0].size = bar->size;
> > @@ -556,7 +564,9 @@
> > bar_addr = mmap(bar->addr, bar->size, 0, MAP_PRIVATE |
> > MAP_ANONYMOUS | additional_flags, -1, 0);
> > if (bar_addr != MAP_FAILED) {
> > - void *map_addr = NULL;
> > + /* Set non NULL initial value for in case of no PCI mapping */
> > + void *map_addr = bar_addr;
> > +
>
> I see the issue that this patch wants to fix is based on an old kernel.
> In older vfio-pci kernel module, MSI related reg cannot be mmaped in userspace
> while in newer kernel it can be. That's why sometimes it cannot be reproduced
> (https://bugs.dpdk.org/show_bug.cgi?id=503)
>
> So under the condition of old kernel, there could be an example that Memreg 0
> has size 0 but Memreg 1 has non-zero size, which leads to Memreg 1 cannot be
> mmaped.
Yes, it is.
>
> So I'm fine with this part of code change. As this issue is blocking test, we should
> do fast confirm and review.
>
> Thanks,
> Chenbo
>
> > if (memreg[0].size) {
> > /* actual map of first part */
> > map_addr = pci_map_resource(bar_addr, vfio_dev_fd,
> > --
> > 1.8.3.1
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
2020-07-09 5:13 ` Zhang, AlvinX
@ 2020-07-09 5:21 ` Xia, Chenbo
0 siblings, 0 replies; 12+ messages in thread
From: Xia, Chenbo @ 2020-07-09 5:21 UTC (permalink / raw)
To: Zhang, AlvinX, dev
Cc: David Marchand, Tal Shnaiderman, Burakov, Anatoly, thomas
> -----Original Message-----
> From: Zhang, AlvinX <alvinx.zhang@intel.com>
> Sent: Thursday, July 9, 2020 1:14 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> Cc: David Marchand <david.marchand@redhat.com>; Tal Shnaiderman
> <talshn@mellanox.com>; Burakov, Anatoly <anatoly.burakov@intel.com>;
> thomas@monjalon.net
> Subject: RE: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
>
> Hi Chenbo,
>
> Thanks your comments.
>
> > -----Original Message-----
> > From: Xia, Chenbo <chenbo.xia@intel.com>
> > Sent: Wednesday, July 8, 2020 9:58 PM
> > To: Zhang, AlvinX <alvinx.zhang@intel.com>; dev@dpdk.org
> > Cc: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia
> > <jia.guo@intel.com>; David Marchand <david.marchand@redhat.com>; Tal
> > Shnaiderman <talshn@mellanox.com>; Burakov, Anatoly
> > <anatoly.burakov@intel.com>; thomas@monjalon.net
> > Subject: RE: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
> >
> > Hi Alvin,
> >
> > CC the maintainers. Comments below.
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of alvinx.zhang@intel.com
> > > Sent: Wednesday, July 8, 2020 5:25 PM
> > > To: dev@dpdk.org
> > > Cc: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia
> > > <jia.guo@intel.com>
> > > Subject: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
> > >
> > > From: Alvin Zhang <alvinx.zhang@intel.com>
> > >
> > > When mapping a PCI BAR containing an MSI-X table, some devices do
> > > not need to actually map this BAR or only need to map part of them,
> > > which may cause the mapping to fail. Now some checks are added and a
> > > non-NULL initial value is set to the variable to avoid this situation.
> > >
> > > Fixes: 2fd3567e5425 ("pci: use OS generic memory mapping functions")
> > > Cc: talshn@mellanox.com
> > >
> > > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > > ---
> > > drivers/bus/pci/linux/pci_vfio.c | 12 +++++++++++-
> > > 1 file changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > > b/drivers/bus/pci/linux/pci_vfio.c
> > > index fdeb9a8..9143bfc 100644
> > > --- a/drivers/bus/pci/linux/pci_vfio.c
> > > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > > @@ -547,6 +547,14 @@
> > > bar_index,
> > > memreg[0].offset, memreg[0].size,
> > > memreg[1].offset, memreg[1].size);
> > > +
> > > + if (memreg[0].size == 0 && memreg[1].size == 0) {
> > > + /* No need to map this BAR */
> > > + RTE_LOG(DEBUG, EAL, "Skipping BAR%d\n", bar_index);
> > > + bar->size = 0;
> > > + bar->addr = 0;
> > > + return 0;
> > > + }
> >
> > I'm not sure if this corner case will happen. If you confirmed it, Just ignore this.
>
> In theory, it is entirely possible if the misx-table size is equal to the bar size.
>
> >
> > > } else {
> > > memreg[0].offset = bar->offset;
> > > memreg[0].size = bar->size;
> > > @@ -556,7 +564,9 @@
> > > bar_addr = mmap(bar->addr, bar->size, 0, MAP_PRIVATE |
> > > MAP_ANONYMOUS | additional_flags, -1, 0);
> > > if (bar_addr != MAP_FAILED) {
> > > - void *map_addr = NULL;
> > > + /* Set non NULL initial value for in case of no PCI mapping */
> > > + void *map_addr = bar_addr;
> > > +
> >
> > I see the issue that this patch wants to fix is based on an old kernel.
> > In older vfio-pci kernel module, MSI related reg cannot be mmaped in
> > userspace while in newer kernel it can be. That's why sometimes it
> > cannot be reproduced
> > (https://bugs.dpdk.org/show_bug.cgi?id=503)
> >
> > So under the condition of old kernel, there could be an example that
> > Memreg 0 has size 0 but Memreg 1 has non-zero size, which leads to
> > Memreg 1 cannot be mmaped.
>
> Yes, it is.
>
> >
> > So I'm fine with this part of code change. As this issue is blocking
> > test, we should do fast confirm and review.
> >
> > Thanks,
> > Chenbo
> >
> > > if (memreg[0].size) {
> > > /* actual map of first part */
> > > map_addr = pci_map_resource(bar_addr, vfio_dev_fd,
> > > --
> > > 1.8.3.1
Acked-by: Chenbo Xia <chenbo.xia@intel.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
2020-07-08 9:24 [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource alvinx.zhang
2020-07-08 9:38 ` David Marchand
2020-07-08 13:58 ` Xia, Chenbo
@ 2020-07-09 2:25 ` Xiao, QimaiX
2020-07-10 9:54 ` David Marchand
3 siblings, 0 replies; 12+ messages in thread
From: Xiao, QimaiX @ 2020-07-09 2:25 UTC (permalink / raw)
To: Zhang, AlvinX, dev; +Cc: Xing, Beilei, Guo, Jia
Tested-by: Xiao Qimai <qimaix.xiao@intel.com>
Regards,
Xiao Qimai
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of alvinx.zhang@intel.com
> Sent: Wednesday, July 8, 2020 5:25 PM
> To: dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>
> Subject: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
>
> From: Alvin Zhang <alvinx.zhang@intel.com>
>
> When mapping a PCI BAR containing an MSI-X table, some devices do not
> need to actually map this BAR or only need to map part of them, which may
> cause the mapping to fail. Now some checks are added and a non-NULL initial
> value is set to the variable to avoid this situation.
>
> Fixes: 2fd3567e5425 ("pci: use OS generic memory mapping functions")
> Cc: talshn@mellanox.com
>
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> ---
> drivers/bus/pci/linux/pci_vfio.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index fdeb9a8..9143bfc 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -547,6 +547,14 @@
> bar_index,
> memreg[0].offset, memreg[0].size,
> memreg[1].offset, memreg[1].size);
> +
> + if (memreg[0].size == 0 && memreg[1].size == 0) {
> + /* No need to map this BAR */
> + RTE_LOG(DEBUG, EAL, "Skipping BAR%d\n",
> bar_index);
> + bar->size = 0;
> + bar->addr = 0;
> + return 0;
> + }
> } else {
> memreg[0].offset = bar->offset;
> memreg[0].size = bar->size;
> @@ -556,7 +564,9 @@
> bar_addr = mmap(bar->addr, bar->size, 0, MAP_PRIVATE |
> MAP_ANONYMOUS | additional_flags, -1, 0);
> if (bar_addr != MAP_FAILED) {
> - void *map_addr = NULL;
> + /* Set non NULL initial value for in case of no PCI mapping */
> + void *map_addr = bar_addr;
> +
> if (memreg[0].size) {
> /* actual map of first part */
> map_addr = pci_map_resource(bar_addr,
> vfio_dev_fd,
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
2020-07-08 9:24 [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource alvinx.zhang
` (2 preceding siblings ...)
2020-07-09 2:25 ` Xiao, QimaiX
@ 2020-07-10 9:54 ` David Marchand
2020-07-10 10:07 ` Thomas Monjalon
2020-07-11 6:57 ` Zhang, AlvinX
3 siblings, 2 replies; 12+ messages in thread
From: David Marchand @ 2020-07-10 9:54 UTC (permalink / raw)
To: Zhang, AlvinX; +Cc: dev, Beilei Xing, Jeff Guo
On Wed, Jul 8, 2020 at 11:26 AM <alvinx.zhang@intel.com> wrote:
>
> From: Alvin Zhang <alvinx.zhang@intel.com>
>
> When mapping a PCI BAR containing an MSI-X table, some devices do not
> need to actually map this BAR or only need to map part of them, which
> may cause the mapping to fail. Now some checks are added and a non-NULL
> initial value is set to the variable to avoid this situation.
>
> Fixes: 2fd3567e5425 ("pci: use OS generic memory mapping functions")
> Cc: talshn@mellanox.com
>
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> ---
> drivers/bus/pci/linux/pci_vfio.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index fdeb9a8..9143bfc 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -547,6 +547,14 @@
> bar_index,
> memreg[0].offset, memreg[0].size,
> memreg[1].offset, memreg[1].size);
> +
> + if (memreg[0].size == 0 && memreg[1].size == 0) {
> + /* No need to map this BAR */
> + RTE_LOG(DEBUG, EAL, "Skipping BAR%d\n", bar_index);
> + bar->size = 0;
> + bar->addr = 0;
> + return 0;
> + }
We already have a check on bar size == 0.
Why would we have this condition?
Broken hw?
> } else {
> memreg[0].offset = bar->offset;
> memreg[0].size = bar->size;
> @@ -556,7 +564,9 @@
> bar_addr = mmap(bar->addr, bar->size, 0, MAP_PRIVATE |
> MAP_ANONYMOUS | additional_flags, -1, 0);
> if (bar_addr != MAP_FAILED) {
> - void *map_addr = NULL;
> + /* Set non NULL initial value for in case of no PCI mapping */
> + void *map_addr = bar_addr;
> +
It took me some time to understand this code...
Anyway, we have a regression in the librte_pci.
This is where the fix should be.
We can cleanup this code later.
> if (memreg[0].size) {
> /* actual map of first part */
> map_addr = pci_map_resource(bar_addr, vfio_dev_fd,
> --
> 1.8.3.1
>
Thanks.
--
David Marchand
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
2020-07-10 9:54 ` David Marchand
@ 2020-07-10 10:07 ` Thomas Monjalon
2020-07-10 12:31 ` Thomas Monjalon
` (2 more replies)
2020-07-11 6:57 ` Zhang, AlvinX
1 sibling, 3 replies; 12+ messages in thread
From: Thomas Monjalon @ 2020-07-10 10:07 UTC (permalink / raw)
To: Zhang, AlvinX
Cc: dev, Beilei Xing, Jeff Guo, David Marchand, anatoly.burakov, ci,
ferruh.yigit, bruce.richardson, qian.q.xu, john.mcnamara, talshn,
rasland, asafp
10/07/2020 11:54, David Marchand:
> On Wed, Jul 8, 2020 at 11:26 AM <alvinx.zhang@intel.com> wrote:
> > From: Alvin Zhang <alvinx.zhang@intel.com>
> >
> > When mapping a PCI BAR containing an MSI-X table, some devices do not
> > need to actually map this BAR or only need to map part of them, which
> > may cause the mapping to fail. Now some checks are added and a non-NULL
> > initial value is set to the variable to avoid this situation.
Note: this regression would not have happened if we had some CI tests
for simple device probing.
Please let's invest more in CI.
> > Fixes: 2fd3567e5425 ("pci: use OS generic memory mapping functions")
> > Cc: talshn@mellanox.com
No he was not Cc in the thread. Same for Anatoly.
Adding more people in Cc...
> > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > ---
> > --- a/drivers/bus/pci/linux/pci_vfio.c
> > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > @@ -547,6 +547,14 @@
> > bar_index,
> > memreg[0].offset, memreg[0].size,
> > memreg[1].offset, memreg[1].size);
> > +
> > + if (memreg[0].size == 0 && memreg[1].size == 0) {
> > + /* No need to map this BAR */
> > + RTE_LOG(DEBUG, EAL, "Skipping BAR%d\n", bar_index);
> > + bar->size = 0;
> > + bar->addr = 0;
> > + return 0;
> > + }
>
> We already have a check on bar size == 0.
> Why would we have this condition?
> Broken hw?
>
>
> > } else {
> > memreg[0].offset = bar->offset;
> > memreg[0].size = bar->size;
> > @@ -556,7 +564,9 @@
> > bar_addr = mmap(bar->addr, bar->size, 0, MAP_PRIVATE |
> > MAP_ANONYMOUS | additional_flags, -1, 0);
> > if (bar_addr != MAP_FAILED) {
> > - void *map_addr = NULL;
> > + /* Set non NULL initial value for in case of no PCI mapping */
> > + void *map_addr = bar_addr;
> > +
>
> It took me some time to understand this code...
> Anyway, we have a regression in the librte_pci.
> This is where the fix should be.
Yes, I am going to send a fix.
> We can cleanup this code later.
Yes please, this function isn't understandable and lack of comments.
Anatoly please?
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
2020-07-10 10:07 ` Thomas Monjalon
@ 2020-07-10 12:31 ` Thomas Monjalon
2020-07-10 12:43 ` [dpdk-dev] [dpdk-ci] " Lincoln Lavoie
2020-09-23 7:34 ` [dpdk-dev] " David Marchand
2 siblings, 0 replies; 12+ messages in thread
From: Thomas Monjalon @ 2020-07-10 12:31 UTC (permalink / raw)
To: Zhang, AlvinX, anatoly.burakov
Cc: dev, Beilei Xing, Jeff Guo, David Marchand, ci, ferruh.yigit,
bruce.richardson, talshn
10/07/2020 12:07, Thomas Monjalon:
> 10/07/2020 11:54, David Marchand:
> > On Wed, Jul 8, 2020 at 11:26 AM <alvinx.zhang@intel.com> wrote:
> > > From: Alvin Zhang <alvinx.zhang@intel.com>
> > >
> > > When mapping a PCI BAR containing an MSI-X table, some devices do not
> > > need to actually map this BAR or only need to map part of them, which
> > > may cause the mapping to fail. Now some checks are added and a non-NULL
> > > initial value is set to the variable to avoid this situation.
[...]
> > > --- a/drivers/bus/pci/linux/pci_vfio.c
> > > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > > @@ -547,6 +547,14 @@
> > > bar_index,
> > > memreg[0].offset, memreg[0].size,
> > > memreg[1].offset, memreg[1].size);
> > > +
> > > + if (memreg[0].size == 0 && memreg[1].size == 0) {
> > > + /* No need to map this BAR */
> > > + RTE_LOG(DEBUG, EAL, "Skipping BAR%d\n", bar_index);
> > > + bar->size = 0;
> > > + bar->addr = 0;
> > > + return 0;
> > > + }
> >
> > We already have a check on bar size == 0.
> > Why would we have this condition?
> > Broken hw?
> >
> >
> > > } else {
> > > memreg[0].offset = bar->offset;
> > > memreg[0].size = bar->size;
> > > @@ -556,7 +564,9 @@
> > > bar_addr = mmap(bar->addr, bar->size, 0, MAP_PRIVATE |
> > > MAP_ANONYMOUS | additional_flags, -1, 0);
> > > if (bar_addr != MAP_FAILED) {
> > > - void *map_addr = NULL;
> > > + /* Set non NULL initial value for in case of no PCI mapping */
> > > + void *map_addr = bar_addr;
> > > +
> >
> > It took me some time to understand this code...
> > Anyway, we have a regression in the librte_pci.
> > This is where the fix should be.
>
> Yes, I am going to send a fix.
Patch sent: https://patches.dpdk.org/patch/73741/
This patch is marked as rejected, but please follow-up on cleanup.
> > We can cleanup this code later.
>
> Yes please, this function isn't understandable and lack of comments.
> Anatoly please?
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [dpdk-ci] [PATCH] bus/pci: fix mmap PCI resource
2020-07-10 10:07 ` Thomas Monjalon
2020-07-10 12:31 ` Thomas Monjalon
@ 2020-07-10 12:43 ` Lincoln Lavoie
2020-09-23 7:34 ` [dpdk-dev] " David Marchand
2 siblings, 0 replies; 12+ messages in thread
From: Lincoln Lavoie @ 2020-07-10 12:43 UTC (permalink / raw)
To: Thomas Monjalon
Cc: Zhang, AlvinX, dev, Beilei Xing, Jeff Guo, David Marchand,
anatoly.burakov, ci, Ferruh Yigit, Richardson, Bruce, Xu, Qian Q,
john.mcnamara, talshn, Raslan Darawsheh, asafp
On Fri, Jul 10, 2020 at 6:08 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> 10/07/2020 11:54, David Marchand:
> > On Wed, Jul 8, 2020 at 11:26 AM <alvinx.zhang@intel.com> wrote:
> > > From: Alvin Zhang <alvinx.zhang@intel.com>
> > >
> > > When mapping a PCI BAR containing an MSI-X table, some devices do not
> > > need to actually map this BAR or only need to map part of them, which
> > > may cause the mapping to fail. Now some checks are added and a non-NULL
> > > initial value is set to the variable to avoid this situation.
>
> Note: this regression would not have happened if we had some CI tests
> for simple device probing.
> Please let's invest more in CI.
>
> Are you referring to adding tests to specifically check these conditions,
or would this have been caught just from the continued expansion of testing
on real hardware / NICs, or both? It seems like the issue is caused by a
combination of hardware behaviors and "broken code". My point is, without
having some of those behaviors in the CI, we might still not have caught
this issue, even with probing checks. Of course, more checks are always a
good thing.
>
> > > Fixes: 2fd3567e5425 ("pci: use OS generic memory mapping functions")
> > > Cc: talshn@mellanox.com
>
> No he was not Cc in the thread. Same for Anatoly.
> Adding more people in Cc...
>
> > > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > > ---
> > > --- a/drivers/bus/pci/linux/pci_vfio.c
> > > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > > @@ -547,6 +547,14 @@
> > > bar_index,
> > > memreg[0].offset, memreg[0].size,
> > > memreg[1].offset, memreg[1].size);
> > > +
> > > + if (memreg[0].size == 0 && memreg[1].size == 0) {
> > > + /* No need to map this BAR */
> > > + RTE_LOG(DEBUG, EAL, "Skipping BAR%d\n",
> bar_index);
> > > + bar->size = 0;
> > > + bar->addr = 0;
> > > + return 0;
> > > + }
> >
> > We already have a check on bar size == 0.
> > Why would we have this condition?
> > Broken hw?
> >
> >
> > > } else {
> > > memreg[0].offset = bar->offset;
> > > memreg[0].size = bar->size;
> > > @@ -556,7 +564,9 @@
> > > bar_addr = mmap(bar->addr, bar->size, 0, MAP_PRIVATE |
> > > MAP_ANONYMOUS | additional_flags, -1, 0);
> > > if (bar_addr != MAP_FAILED) {
> > > - void *map_addr = NULL;
> > > + /* Set non NULL initial value for in case of no PCI
> mapping */
> > > + void *map_addr = bar_addr;
> > > +
> >
> > It took me some time to understand this code...
> > Anyway, we have a regression in the librte_pci.
> > This is where the fix should be.
>
> Yes, I am going to send a fix.
>
> > We can cleanup this code later.
>
> Yes please, this function isn't understandable and lack of comments.
> Anatoly please?
>
>
>
--
*Lincoln Lavoie*
Senior Engineer, Broadband Technologies
21 Madbury Rd., Ste. 100, Durham, NH 03824
lylavoie@iol.unh.edu
https://www.iol.unh.edu
+1-603-674-2755 (m)
<https://www.iol.unh.edu/>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
2020-07-10 10:07 ` Thomas Monjalon
2020-07-10 12:31 ` Thomas Monjalon
2020-07-10 12:43 ` [dpdk-dev] [dpdk-ci] " Lincoln Lavoie
@ 2020-09-23 7:34 ` David Marchand
2 siblings, 0 replies; 12+ messages in thread
From: David Marchand @ 2020-09-23 7:34 UTC (permalink / raw)
To: Thomas Monjalon, Zhang, AlvinX, Burakov, Anatoly
Cc: dev, Beilei Xing, Jeff Guo, Yigit, Ferruh, Bruce Richardson,
Qian Xu, Mcnamara, John, Tal Shnaiderman, Raslan, Asaf Penso
On Fri, Jul 10, 2020 at 12:08 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > It took me some time to understand this code...
> > Anyway, we have a regression in the librte_pci.
> > This is where the fix should be.
>
> Yes, I am going to send a fix.
>
> > We can cleanup this code later.
>
> Yes please, this function isn't understandable and lack of comments.
> Anatoly please?
Now that the workaround on the pci mapping API has been reverted, we
are back with this issue.
https://bugs.dpdk.org/show_bug.cgi?id=539
Volunteer(s) to fix and clean this code?
Thanks.
--
David Marchand
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
2020-07-10 9:54 ` David Marchand
2020-07-10 10:07 ` Thomas Monjalon
@ 2020-07-11 6:57 ` Zhang, AlvinX
1 sibling, 0 replies; 12+ messages in thread
From: Zhang, AlvinX @ 2020-07-11 6:57 UTC (permalink / raw)
To: David Marchand; +Cc: dev
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Friday, July 10, 2020 5:54 PM
> To: Zhang, AlvinX <alvinx.zhang@intel.com>
> Cc: dev <dev@dpdk.org>; Xing, Beilei <beilei.xing@intel.com>; Guo, Jia
> <jia.guo@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
>
> On Wed, Jul 8, 2020 at 11:26 AM <alvinx.zhang@intel.com> wrote:
> >
> > From: Alvin Zhang <alvinx.zhang@intel.com>
> >
> > When mapping a PCI BAR containing an MSI-X table, some devices do not
> > need to actually map this BAR or only need to map part of them, which
> > may cause the mapping to fail. Now some checks are added and a
> > non-NULL initial value is set to the variable to avoid this situation.
> >
> > Fixes: 2fd3567e5425 ("pci: use OS generic memory mapping functions")
> > Cc: talshn@mellanox.com
> >
> > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > ---
> > drivers/bus/pci/linux/pci_vfio.c | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > b/drivers/bus/pci/linux/pci_vfio.c
> > index fdeb9a8..9143bfc 100644
> > --- a/drivers/bus/pci/linux/pci_vfio.c
> > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > @@ -547,6 +547,14 @@
> > bar_index,
> > memreg[0].offset, memreg[0].size,
> > memreg[1].offset, memreg[1].size);
> > +
> > + if (memreg[0].size == 0 && memreg[1].size == 0) {
> > + /* No need to map this BAR */
> > + RTE_LOG(DEBUG, EAL, "Skipping BAR%d\n",
> bar_index);
> > + bar->size = 0;
> > + bar->addr = 0;
> > + return 0;
> > + }
>
> We already have a check on bar size == 0.
> Why would we have this condition?
> Broken hw?
>
If the misx-table size is equal to the bar size, the memreg[0].size and memreg[1].size will be zero at same time although the bar size is not zero
>
> > } else {
> > memreg[0].offset = bar->offset;
> > memreg[0].size = bar->size; @@ -556,7 +564,9 @@
> > bar_addr = mmap(bar->addr, bar->size, 0, MAP_PRIVATE |
> > MAP_ANONYMOUS | additional_flags, -1, 0);
> > if (bar_addr != MAP_FAILED) {
> > - void *map_addr = NULL;
> > + /* Set non NULL initial value for in case of no PCI mapping
> */
> > + void *map_addr = bar_addr;
> > +
>
> It took me some time to understand this code...
> Anyway, we have a regression in the librte_pci.
> This is where the fix should be.
>
> We can cleanup this code later.
The key cause is the value of memreg[0].size is 0
>
> > if (memreg[0].size) {
> > /* actual map of first part */
> > map_addr = pci_map_resource(bar_addr,
> > vfio_dev_fd,
> > --
> > 1.8.3.1
> >
>
>
> Thanks.
>
> --
> David Marchand
^ permalink raw reply [flat|nested] 12+ messages in thread