From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <luca.boccassi@gmail.com>
Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66])
by dpdk.org (Postfix) with ESMTP id 6550D1B001
for <dev@dpdk.org>; Fri, 24 Aug 2018 19:14:40 +0200 (CEST)
Received: by mail-wm0-f66.google.com with SMTP id c14-v6so2235007wmb.4
for <dev@dpdk.org>; Fri, 24 Aug 2018 10:14:40 -0700 (PDT)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to
:references:content-transfer-encoding:mime-version;
bh=BCbpik2q6fdwnLegc1/atX2XqDiDR4D8PgGk8DjImoI=;
b=Yu0oIvdGFL3Cxfd35CTcXda/I3dvWzeY42utmtJNDhGeGGiWY5WuAUAElw9BVm5Nz0
yPddkt2HqYZu7C4c/Wud8HX56PPRNvlTO9s1daiFOg6bwpFq7hZ0OoNWxqAwcHW26p3U
Q3uJ6IHWzHuhT9RZxD4U52rSYzjggm7VeYbZ4QQ0Ggov/KcGkGyaNJr4+Gl4FHVvahZa
Zceucp+scL6oq0y26ev9vWdTR4xQ3xg0PaS6EnEfHwUsxkWHY4JUwJgMLWe2ATiRdIPt
/wIM2ehPBDfil0X5vh8bGh/D3wmrMINoMlmhXW8x6eo5lPSh1hTkR+xjMOkj9G8QqSOS
euMg==
X-Gm-Message-State: APzg51B3v8YSog5PP1mnhI3aC54zBBW7WxNKIio8VR1tXKPZhzQl7PYZ
0AGSqRcszhYfpGnukHeMh8A=
X-Google-Smtp-Source: ANB0VdbggmqBS101VoWMQBtQPAQZUUInBXk8Vem8XB0/rxSRJvzcnZqTHiAXmd/f0hKaxNgD3A8GTw==
X-Received: by 2002:a1c:8406:: with SMTP id g6-v6mr1868641wmd.18.1535130880098;
Fri, 24 Aug 2018 10:14:40 -0700 (PDT)
Received: from localhost ([2a01:4b00:f419:6f00:8361:8946:ba2b:d556])
by smtp.gmail.com with ESMTPSA id w15-v6sm6038012wrs.8.2018.08.24.10.14.38
(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);
Fri, 24 Aug 2018 10:14:38 -0700 (PDT)
Message-ID: <1535130878.5764.148.camel@debian.org>
From: Luca Boccassi <bluca@debian.org>
To: Tiwei Bie <tiwei.bie@intel.com>
Cc: dev@dpdk.org, maxime.coquelin@redhat.com, zhihong.wang@intel.com,
bruce.richardson@intel.com, brian.russell@intl.att.com
Date: Fri, 24 Aug 2018 18:14:38 +0100
In-Reply-To: <20180824042355.GA18154@debian>
References: <20180814143035.19640-1-bluca@debian.org>
<20180816184750.30843-1-bluca@debian.org>
<20180816184750.30843-2-bluca@debian.org>
<1534445383.5764.56.camel@debian.org> <20180820081804.GA29806@debian>
<1534783535.5764.93.camel@debian.org> <20180821024030.GA17967@debian>
<1535028745.5764.134.camel@debian.org> <20180824042355.GA18154@debian>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
X-Mailer: Evolution 3.22.6-1+deb9u1
Mime-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Fri, 24 Aug 2018 17:14:40 -0000
On Fri, 2018-08-24 at 12:23 +0800, Tiwei Bie wrote:
> On Thu, Aug 23, 2018 at 01:52:25PM +0100, Luca Boccassi wrote:
> > On Tue, 2018-08-21 at 10:40 +0800, Tiwei Bie wrote:
> > > On Mon, Aug 20, 2018 at 05:45:35PM +0100, Luca Boccassi wrote:
> > > > On Mon, 2018-08-20 at 16:18 +0800, Tiwei Bie wrote:
> > > > > On Thu, Aug 16, 2018 at 07:49:43PM +0100, Luca Boccassi
> > > > > wrote:
> > > > > > On Thu, 2018-08-16 at 19:47 +0100, Luca Boccassi wrote:
> > > > > > > From: Brian Russell <brussell@brocade.com>
> > > > > > >=20
> > > > > > > In virtio_read_caps and vtpci_msix_detect,
> > > > > > > rte_pci_read_config
> > > > > > > returns
> > > > > > > the number of bytes read from PCI config or < 0 on error.
> > > > > > > If less than the expected number of bytes are read then
> > > > > > > log
> > > > > > > the
> > > > > > > failure and return rather than carrying on with garbage.
> > > > > > >=20
> > > > > > > Fixes: 6ba1f63b5ab0 ("virtio: support specification 1.0")
> > > > > > >=20
> > > > > > > Signed-off-by: Brian Russell <brussell@brocade.com>
> > > > > > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > > > > > > ---
> > > > > > > v2: handle additional rte_pci_read_config incomplete
> > > > > > > reads
> > > > > > >=20
> > > > > > > =C2=A0drivers/net/virtio/virtio_pci.c | 35
> > > > > > > +++++++++++++++++++++
> > > > > > > ----
> > > > > > > ----
> > > > > > > ----
> > > > > > > =C2=A01 file changed, 22 insertions(+), 13 deletions(-)
> > > > > > >=20
> > > > > > > diff --git a/drivers/net/virtio/virtio_pci.c
> > > > > > > b/drivers/net/virtio/virtio_pci.c
> > > > > > > index 6bd22e54a6..ff6f96f361 100644
> > > > > > > --- a/drivers/net/virtio/virtio_pci.c
> > > > > > > +++ b/drivers/net/virtio/virtio_pci.c
> > > > > >=20
> > > > > > ...
> > > > > > > @@ -610,9 +613,13 @@ virtio_read_caps(struct
> > > > > > > rte_pci_device
> > > > > > > *dev,
> > > > > > > struct virtio_hw *hw)
> > > > > > > =C2=A0 hw->common_cfg =3D
> > > > > > > get_cfg_addr(dev,
> > > > > > > &cap);
> > > > > > > =C2=A0 break;
> > > > > > > =C2=A0 case VIRTIO_PCI_CAP_NOTIFY_CFG:
> > > > > > > - rte_pci_read_config(dev, &hw-
> > > > > > > > notify_off_multiplier,
> > > > > > >=20
> > > > > > > - 4, pos +
> > > > > > > sizeof(cap));
> > > > > > > - hw->notify_base =3D
> > > > > > > get_cfg_addr(dev,
> > > > > > > &cap);
> > > > > > > + /* Avoid half-reads into hw */
> > > > > > > + ret =3D rte_pci_read_config(dev,
> > > > > > > &multiplier,
> > > > > > > 4,
> > > > > > > + pos +
> > > > > > > sizeof(cap));
> > > > > > > + if (ret =3D=3D 4) {
> > > > > > > + hw-
> > > > > > > >notify_off_multiplier =3D
> > > > > > > multiplier;
> > > > > > > + hw->notify_base =3D
> > > > > > > get_cfg_addr(dev,
> > > > > > > &cap);
> > > > > > > + }
> > > > > > > =C2=A0 break;
> > > > > > > =C2=A0 case VIRTIO_PCI_CAP_DEVICE_CFG:
> > > > > > > =C2=A0 hw->dev_cfg =3D get_cfg_addr(dev,
> > > > > > > &cap);
> > > > > >=20
> > > > > > Tiwei: not 100% sure what's the best way to handle a
> > > > > > failure
> > > > > > here,
> > > > > > this
> > > > > > will avoid writing to *hw in case of errors. Let me know if
> > > > > > this is
> > > > > > OK.
> > > > >=20
> > > > > My concern is about reading the virtio capability directly.
> > > > > With this patch, it will give up reading other capabilities
> > > > > when failed to read a full virtio PCI capability structure
> > > > > (the returned bytes are less than the expected bytes) even
> > > > > though when the capability it's reading isn't a virtio vendor
> > > > > specific capability. I'm not quite sure whether it will bring
> > > > > any bad consequence. How about just reading the first two
> > > > > bytes first? Something like this:
> > > > >=20
> > > > > https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/
> > > > > pci/
> > > > > pci.
> > > > > c#L1491-L1497
> > > >=20
> > > > I'm not sure, to be honest. That bit didn't give me trouble at
> > > > all,
> > > > so
> > > > at this point I'd much rather leave it alone so that the
> > > > maintainers
> > > > can take care of it how they see fit, if necessary :-)
> > > >=20
> > > > I've sent a v3 that removes that individual change.
> > >=20
> > > My concern isn't about the above change (which handled the
> > > errors when reading multiplier). In fact, above change looks
> > > good to me! :-)=C2=A0=C2=A0I mean the below changes in this patch:
> > >=20
> > > =C2=A0 while (pos) {
> > > =C2=A0 ret =3D rte_pci_read_config(dev, &cap,
> > > sizeof(cap),
> > > pos);
> > > - if (ret < 0) {
> > > - PMD_INIT_LOG(ERR,
> > > - "failed to read pci cap at pos:
> > > %x",
> > > pos);
> > > + if (ret !=3D sizeof(cap)) {
> > > + PMD_INIT_LOG(DEBUG,
> > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"failed to read pci cap at
> > > pos:
> > > %x ret %d",
> > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0pos, ret);
> > > =C2=A0 break;
> > > =C2=A0 }
> > >=20
> > > With this change, it will give up reading other capabilities
> > > when failed to read a full virtio PCI capability structure
> > > (the returned bytes are less than the expected bytes) even
> > > though when the capability it's reading isn't a virtio vendor
> > > specific capability. Maybe it would be better to read the
> > > first two bytes first and check whether it's the capability
> > > we want to parse (i.e. vendor capability and MSIX capability).
> > > Something like this:
> > >=20
> > > https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/pci/
> > > pci.
> > > c#L1491-L1497
> > >=20
> > > How do you think?
> > >=20
> > > Thanks
> >=20
> > Ah I see what you mean now, sorry. Would something like the
> > following
> > be what you are looking for:
> >=20
> > ret =3D rte_pci_read_config(dev, &cap, 2, pos);
> > if (ret !=3D 2) {
> > PMD_INIT_LOG(DEBUG,
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"failed to read pci cap at pos: %x
> > ret %d",
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0pos, ret);
> > break;
> > }
> > if (cap.cap_vndr !=3D PCI_CAP_ID_MSIX &&
> > cap.cap_vndr !=3D PCI_CAP_ID_VNDR) {
> > goto next;
> > }
> >=20
> > ret =3D rte_pci_read_config(dev, &cap, sizeof(cap), pos);
> > if (ret !=3D sizeof(cap)) {
> > PMD_INIT_LOG(DEBUG,
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"failed to read pci cap at pos: %x
> > ret %d",
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0pos, ret);
> > break;
> > }
> >=20
>=20
> Yeah, that's what I want. :-)
> Just one nit, we don't need to read it as a
> virtio cap when dealing with the MSIX cap.
> When dealing with the MSIX cap, we just need
> to read the next two bytes:
>=20
> https://github.com/DPDK/dpdk/blob/7281cf520f89/drivers/net/virtio/vir
> tio_pci.c#L584-L594
> https://github.com/freebsd/freebsd/blob/59e4185a9358/sys/dev/pci/pci.
> c#L892
>=20
> Thanks!
Ok, done, see v4.
--=20
Kind regards,
Luca Boccassi