DPDK CI discussions
 help / color / Atom feed
* Re: [dpdk-ci] [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
       [not found] ` <CAJFAV8xNh2XHS1YO4TwAi54_UAVaYj60jwAovnqgaPySS3yKbQ@mail.gmail.com>
@ 2020-07-10 10:07   ` Thomas Monjalon
  2020-07-10 12:31     ` Thomas Monjalon
  2020-07-10 12:43     ` Lincoln Lavoie
  0 siblings, 2 replies; 3+ 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] 3+ messages in thread

* Re: [dpdk-ci] [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
  2020-07-10 10:07   ` [dpdk-ci] [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource Thomas Monjalon
@ 2020-07-10 12:31     ` Thomas Monjalon
  2020-07-10 12:43     ` Lincoln Lavoie
  1 sibling, 0 replies; 3+ 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] 3+ messages in thread

* Re: [dpdk-ci] [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource
  2020-07-10 10:07   ` [dpdk-ci] [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource Thomas Monjalon
  2020-07-10 12:31     ` Thomas Monjalon
@ 2020-07-10 12:43     ` Lincoln Lavoie
  1 sibling, 0 replies; 3+ 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

[-- Attachment #1: Type: text/plain, Size: 3211 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 5316 bytes --]

<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jul 10, 2020 at 6:08 AM Thomas Monjalon &lt;<a href="mailto:thomas@monjalon.net">thomas@monjalon.net</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">10/07/2020 11:54, David Marchand:<br>
&gt; On Wed, Jul 8, 2020 at 11:26 AM &lt;<a href="mailto:alvinx.zhang@intel.com" target="_blank">alvinx.zhang@intel.com</a>&gt; wrote:<br>
&gt; &gt; From: Alvin Zhang &lt;<a href="mailto:alvinx.zhang@intel.com" target="_blank">alvinx.zhang@intel.com</a>&gt;<br>
&gt; &gt;<br>
&gt; &gt; When mapping a PCI BAR containing an MSI-X table, some devices do not<br>
&gt; &gt; need to actually map this BAR or only need to map part of them, which<br>
&gt; &gt; may cause the mapping to fail. Now some checks are added and a non-NULL<br>
&gt; &gt; initial value is set to the variable to avoid this situation.<br>
<br>
Note: this regression would not have happened if we had some CI tests<br>
for simple device probing.<br>
Please let&#39;s invest more in CI.<br>
<br></blockquote><div class="gmail_default" style="font-size:small">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 &quot;broken code&quot;.  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.  </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
&gt; &gt; Fixes: 2fd3567e5425 (&quot;pci: use OS generic memory mapping functions&quot;)<br>
&gt; &gt; Cc: <a href="mailto:talshn@mellanox.com" target="_blank">talshn@mellanox.com</a><br>
<br>
No he was not Cc in the thread. Same for Anatoly.<br>
Adding more people in Cc...<br>
<br>
&gt; &gt; Signed-off-by: Alvin Zhang &lt;<a href="mailto:alvinx.zhang@intel.com" target="_blank">alvinx.zhang@intel.com</a>&gt;<br>
&gt; &gt; ---<br>
&gt; &gt; --- a/drivers/bus/pci/linux/pci_vfio.c<br>
&gt; &gt; +++ b/drivers/bus/pci/linux/pci_vfio.c<br>
&gt; &gt; @@ -547,6 +547,14 @@<br>
&gt; &gt;                         bar_index,<br>
&gt; &gt;                         memreg[0].offset, memreg[0].size,<br>
&gt; &gt;                         memreg[1].offset, memreg[1].size);<br>
&gt; &gt; +<br>
&gt; &gt; +               if (memreg[0].size == 0 &amp;&amp; memreg[1].size == 0) {<br>
&gt; &gt; +                       /* No need to map this BAR */<br>
&gt; &gt; +                       RTE_LOG(DEBUG, EAL, &quot;Skipping BAR%d\n&quot;, bar_index);<br>
&gt; &gt; +                       bar-&gt;size = 0;<br>
&gt; &gt; +                       bar-&gt;addr = 0;<br>
&gt; &gt; +                       return 0;<br>
&gt; &gt; +               }<br>
&gt; <br>
&gt; We already have a check on bar size == 0.<br>
&gt; Why would we have this condition?<br>
&gt; Broken hw?<br>
&gt; <br>
&gt; <br>
&gt; &gt;         } else {<br>
&gt; &gt;                 memreg[0].offset = bar-&gt;offset;<br>
&gt; &gt;                 memreg[0].size = bar-&gt;size;<br>
&gt; &gt; @@ -556,7 +564,9 @@<br>
&gt; &gt;         bar_addr = mmap(bar-&gt;addr, bar-&gt;size, 0, MAP_PRIVATE |<br>
&gt; &gt;                         MAP_ANONYMOUS | additional_flags, -1, 0);<br>
&gt; &gt;         if (bar_addr != MAP_FAILED) {<br>
&gt; &gt; -               void *map_addr = NULL;<br>
&gt; &gt; +               /* Set non NULL initial value for in case of no PCI mapping */<br>
&gt; &gt; +               void *map_addr = bar_addr;<br>
&gt; &gt; +<br>
&gt; <br>
&gt; It took me some time to understand this code...<br>
&gt; Anyway, we have a regression in the librte_pci.<br>
&gt; This is where the fix should be.<br>
<br>
Yes, I am going to send a fix.<br>
<br>
&gt; We can cleanup this code later.<br>
<br>
Yes please, this function isn&#39;t understandable and lack of comments.<br>
Anatoly please?<br>
<br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><b>Lincoln Lavoie</b><br></div><div>Senior Engineer, Broadband Technologies</div><div>21 Madbury Rd., Ste. 100, Durham, NH 03824</div><div><a href="mailto:lylavoie@iol.unh.edu" target="_blank">lylavoie@iol.unh.edu</a></div><div><a href="https://www.iol.unh.edu" target="_blank">https://www.iol.unh.edu</a></div><div>+1-603-674-2755 (m)<br></div><div><a href="https://www.iol.unh.edu/" target="_blank"><img src="http://homeautomation.lavoieholdings.com/_/rsrc/1390068882701/unh-iol-logo.png"></a></div></div></div></div></div></div></div></div></div></div></div></div></div>

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200708092435.9776-1-alvinx.zhang@intel.com>
     [not found] ` <CAJFAV8xNh2XHS1YO4TwAi54_UAVaYj60jwAovnqgaPySS3yKbQ@mail.gmail.com>
2020-07-10 10:07   ` [dpdk-ci] [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource Thomas Monjalon
2020-07-10 12:31     ` Thomas Monjalon
2020-07-10 12:43     ` Lincoln Lavoie

DPDK CI discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/ci/0 ci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ci ci/ http://inbox.dpdk.org/ci \
		ci@dpdk.org
	public-inbox-index ci


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.ci


AGPL code for this site: git clone https://public-inbox.org/ public-inbox