From: Lincoln Lavoie <lylavoie@iol.unh.edu>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: "Zhang, AlvinX" <alvinx.zhang@intel.com>,
dev@dpdk.org, Beilei Xing <beilei.xing@intel.com>,
Jeff Guo <jia.guo@intel.com>,
David Marchand <david.marchand@redhat.com>,
anatoly.burakov@intel.com, ci@dpdk.org,
Ferruh Yigit <ferruh.yigit@intel.com>,
"Richardson, Bruce" <bruce.richardson@intel.com>,
"Xu, Qian Q" <qian.q.xu@intel.com>,
john.mcnamara@intel.com, talshn@mellanox.com,
Raslan Darawsheh <rasland@mellanox.com>,
asafp@mellanox.com
Subject: Re: [dpdk-dev] [dpdk-ci] [PATCH] bus/pci: fix mmap PCI resource
Date: Fri, 10 Jul 2020 08:43:37 -0400 [thread overview]
Message-ID: <CAOE1vsPsgtDywVH1iJ0fYVa7E3-EyOyjDeM9-hahVRbBMS64Vw@mail.gmail.com> (raw)
In-Reply-To: <3033441.08XpM1RNeG@thomas>
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/>
next prev parent reply other threads:[~2020-07-10 12:44 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-08 9:24 [dpdk-dev] " 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 5:21 ` Xia, Chenbo
2020-07-09 2:25 ` Xiao, QimaiX
2020-07-10 9:54 ` David Marchand
2020-07-10 10:07 ` Thomas Monjalon
2020-07-10 12:31 ` Thomas Monjalon
2020-07-10 12:43 ` Lincoln Lavoie [this message]
2020-09-23 7:34 ` David Marchand
2020-07-11 6:57 ` Zhang, AlvinX
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOE1vsPsgtDywVH1iJ0fYVa7E3-EyOyjDeM9-hahVRbBMS64Vw@mail.gmail.com \
--to=lylavoie@iol.unh.edu \
--cc=alvinx.zhang@intel.com \
--cc=anatoly.burakov@intel.com \
--cc=asafp@mellanox.com \
--cc=beilei.xing@intel.com \
--cc=bruce.richardson@intel.com \
--cc=ci@dpdk.org \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=jia.guo@intel.com \
--cc=john.mcnamara@intel.com \
--cc=qian.q.xu@intel.com \
--cc=rasland@mellanox.com \
--cc=talshn@mellanox.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).