From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2FBC7A052A for ; Fri, 10 Jul 2020 14:44:20 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CFD7B1DC6C; Fri, 10 Jul 2020 14:44:19 +0200 (CEST) Received: from mail-ej1-f67.google.com (mail-ej1-f67.google.com [209.85.218.67]) by dpdk.org (Postfix) with ESMTP id 9AB471D6A4 for ; Fri, 10 Jul 2020 14:44:17 +0200 (CEST) Received: by mail-ej1-f67.google.com with SMTP id dp18so5876457ejc.8 for ; Fri, 10 Jul 2020 05:44:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=3SWvRfUZD6F0Ltj+GAJJMJvjtCLZp5h7bA4yInMH1Ls=; b=hJ1/aku1rTz7ag7GU92ZRcmdWkEcg0TIP5uVRjab9RBKsJRlPwxQYP234V1PNbK0zd /+mWZIJogLCGNsO05DzALV67J9lHYBkyPKGDCOZfjLfMicgzeBlTE6hEZkgHV6XsUfwR P9GD/QtmfIJKDT4sZ6xNkJxXFoUW5I5kQqxXw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=3SWvRfUZD6F0Ltj+GAJJMJvjtCLZp5h7bA4yInMH1Ls=; b=Dt+3UKQwcAZKt0S0LS0Hx8KGGvFI+uOC987i3p97l1DDGzD+YNmmOa9Ni2WI5zweCA y+yv/qmW92MUBaFCxOHvjG/4+gR3v5xwkviVsDcyqGFYODECBqbwd7lXm8r6jlG/9XeQ 4ut3l0RfMG2tFibYA0zzZjSZyY9PPLtB38pURCy4eCrsv75RuSZgK07AudonM5MJ53u6 DZ+BIm+b2xWA8Ihkj1vfkWWEOeBrDWzgSfgBOgTEl8JdZ7Hq9RrmGy7U/AcAg1jL3VJp zvTJYr3aRgQ+yPRTFXsedzSDEHvZHCw7h/rZLoKVUdxfFHDXINJzpomNvYE7Xhh5YlCL z+vA== X-Gm-Message-State: AOAM533ZA3FiJLPlAAzXrpbWlToTTpJph/WPX+jZSvXuDJGHRI08UySY W5s2JVYGctufehAuYXllRxDJzAc1CPLAr8flhrFiAg== X-Google-Smtp-Source: ABdhPJxmlZA6KXfIWiXFVs9wvnstFu3xTDZ8YSxfCaRPohkkTXD3cAbjoE3uEzj6lSq1WZ6sYSjt17FmhO6SxwB6pDM= X-Received: by 2002:a17:906:d784:: with SMTP id pj4mr59297319ejb.405.1594385057284; Fri, 10 Jul 2020 05:44:17 -0700 (PDT) MIME-Version: 1.0 References: <20200708092435.9776-1-alvinx.zhang@intel.com> <3033441.08XpM1RNeG@thomas> In-Reply-To: <3033441.08XpM1RNeG@thomas> From: Lincoln Lavoie Date: Fri, 10 Jul 2020 08:43:37 -0400 Message-ID: To: Thomas Monjalon Cc: "Zhang, AlvinX" , dev@dpdk.org, Beilei Xing , Jeff Guo , David Marchand , anatoly.burakov@intel.com, ci@dpdk.org, Ferruh Yigit , "Richardson, Bruce" , "Xu, Qian Q" , john.mcnamara@intel.com, talshn@mellanox.com, Raslan Darawsheh , asafp@mellanox.com Content-Type: multipart/alternative; boundary="000000000000f9a9fa05aa15b21a" Subject: Re: [dpdk-ci] [dpdk-dev] [PATCH] bus/pci: fix mmap PCI resource X-BeenThere: ci@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK CI discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ci-bounces@dpdk.org Sender: "ci" --000000000000f9a9fa05aa15b21a Content-Type: text/plain; charset="UTF-8" On Fri, Jul 10, 2020 at 6:08 AM Thomas Monjalon wrote: > 10/07/2020 11:54, David Marchand: > > On Wed, Jul 8, 2020 at 11:26 AM wrote: > > > From: Alvin Zhang > > > > > > 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 > > > --- > > > --- 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) --000000000000f9a9fa05aa15b21a Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Fri, Jul 10, 2020 at 6:08 AM Thomas Monjalon <= ;thomas@monjalon.net> wrote:<= br>
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, w= hich
> > may cause the mapping to fail. Now some checks are added and a no= n-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 w= ould 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 combi= nation of hardware behaviors and "broken code".=C2=A0 My point is= , without having=C2=A0some of those behaviors in the CI, we might still not= have caught this issue, even with probing checks.=C2=A0 Of course, more ch= ecks are always a good thing.=C2=A0=C2=A0

> > Fixes: 2fd3567e5425 ("pci: use OS generic memory mapping fun= ctions")
> > Cc: tals= hn@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 @@
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0bar_index,
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0memreg[0].offset, memreg[0].size,
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0memreg[1].offset, memreg[1].size);
> > +
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (memre= g[0].size =3D=3D 0 && memreg[1].size =3D=3D 0) {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0/* No need to map this BAR */
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0RTE_LOG(DEBUG, EAL, "Skipping BAR%d\n", bar_i= ndex);
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0bar->size =3D 0;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0bar->addr =3D 0;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0return 0;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>
> We already have a check on bar size =3D=3D 0.
> Why would we have this condition?
> Broken hw?
>
>
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} else {
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memr= eg[0].offset =3D bar->offset;
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memr= eg[0].size =3D bar->size;
> > @@ -556,7 +564,9 @@
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0bar_addr =3D mmap(bar->addr, = bar->size, 0, MAP_PRIVATE |
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0MAP_ANONYMOUS | additional_flags, -1, 0);
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (bar_addr !=3D MAP_FAILED) {<= br> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0void *map= _addr =3D NULL;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Set no= n NULL initial value for in case of no PCI mapping */
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0void *map= _addr =3D 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. 10= 0, Durham, NH 03824
+1-603-674= -2755 (m)
--000000000000f9a9fa05aa15b21a--