From mboxrd@z Thu Jan 1 00:00:00 1970 From: Luca Boccassi Subject: Re: [PATCH 2/2] virtio: fix PCI config err handling Date: Thu, 23 Aug 2018 13:52:25 +0100 Message-ID: <1535028745.5764.134.camel@debian.org> 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> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Cc: dev@dpdk.org, maxime.coquelin@redhat.com, zhihong.wang@intel.com, bruce.richardson@intel.com, brian.russell@intl.att.com To: Tiwei Bie Return-path: Received: from mail-wm0-f68.google.com (mail-wm0-f68.google.com [74.125.82.68]) by dpdk.org (Postfix) with ESMTP id 0773A98 for ; Thu, 23 Aug 2018 14:52:28 +0200 (CEST) Received: by mail-wm0-f68.google.com with SMTP id t25-v6so5694990wmi.3 for ; Thu, 23 Aug 2018 05:52:27 -0700 (PDT) In-Reply-To: <20180821024030.GA17967@debian> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > > > > >=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 > > > > > Signed-off-by: Luca Boccassi > > > > > --- > > > > > 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 Ah I see what you mean now, sorry. Would something like the following be what you are looking for: ret =3D rte_pci_read_config(dev, &cap, 2, pos); if (ret !=3D 2) { PMD_INIT_LOG(DEBUG, "failed to read pci cap at pos: %x ret %d", pos, ret); break; } if (cap.cap_vndr !=3D PCI_CAP_ID_MSIX && cap.cap_vndr !=3D PCI_CAP_ID_VNDR) { goto next; } ret =3D rte_pci_read_config(dev, &cap, sizeof(cap), pos); if (ret !=3D sizeof(cap)) { PMD_INIT_LOG(DEBUG, "failed to read pci cap at pos: %x ret %d", pos, ret); break; } --=20 Kind regards, Luca Boccassi