From mboxrd@z Thu Jan 1 00:00:00 1970 From: Luca Boccassi Subject: Re: [PATCH v4 2/2] virtio: fix PCI config err handling Date: Mon, 27 Aug 2018 17:52:56 +0100 Message-ID: <1535388776.8636.15.camel@debian.org> References: <20180820164421.28763-1-bluca@debian.org> <20180824171420.31246-1-bluca@debian.org> <20180824171420.31246-2-bluca@debian.org> <20180827052934.GA34061@fbsd.sh.intel.com> 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-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by dpdk.org (Postfix) with ESMTP id DF08E5B20 for ; Mon, 27 Aug 2018 18:52:58 +0200 (CEST) Received: by mail-wr1-f65.google.com with SMTP id j26-v6so14296596wre.2 for ; Mon, 27 Aug 2018 09:52:58 -0700 (PDT) In-Reply-To: <20180827052934.GA34061@fbsd.sh.intel.com> 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 Mon, 2018-08-27 at 13:29 +0800, Tiwei Bie wrote: > On Fri, Aug 24, 2018 at 06:14:20PM +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 > > v3: do not handle rte_pci_read_config of virtio cap, added in v2, > > =C2=A0=C2=A0=C2=A0=C2=A0as it's less clear what the right thing to do t= here is > > v4: do a more robust check - first check what the vendor is, and > > =C2=A0=C2=A0=C2=A0=C2=A0skip the cap entirely if it's not what we are l= ooking for. > >=20 > > =C2=A0drivers/net/virtio/virtio_pci.c | 57 ++++++++++++++++++++++++----= - > > ---- > > =C2=A01 file changed, 42 insertions(+), 15 deletions(-) > >=20 > > diff --git a/drivers/net/virtio/virtio_pci.c > > b/drivers/net/virtio/virtio_pci.c > > index 6bd22e54a6..cfefa9789b 100644 > > --- a/drivers/net/virtio/virtio_pci.c > > +++ b/drivers/net/virtio/virtio_pci.c > > @@ -567,16 +567,30 @@ virtio_read_caps(struct rte_pci_device *dev, > > struct virtio_hw *hw) > > =C2=A0 } > > =C2=A0 > > =C2=A0 ret =3D rte_pci_read_config(dev, &pos, 1, > > PCI_CAPABILITY_LIST); > > - if (ret < 0) { > > - PMD_INIT_LOG(DEBUG, "failed to read pci capability > > list"); > > + if (ret !=3D 1) { > > + PMD_INIT_LOG(DEBUG, > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"failed to read pci capability list, > > ret %d", ret); > > =C2=A0 return -1; > > =C2=A0 } > > =C2=A0 > > =C2=A0 while (pos) { > > + 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; > > + } > > + > > =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 } > > =C2=A0 >=20 > It seems that I didn't make myself clear in my previous > comments. I mean it's better to handle MSIX cap and virtio > cap respectively in this function. Currently we're always > reading them as virtio caps. As we are strictly requiring > that _read_config() should return the required number of > bytes, it's not perfect to require it to return "virtio > cap size" of bytes while we're trying to read a MSIX cap. > So please change the code to something similar to this: Sorry, though you meant in the vtpci_msix_detect function, which I changed. Fixed in v5. > > @@ -689,25 +703,38 @@ enum virtio_msix_status > > =C2=A0vtpci_msix_detect(struct rte_pci_device *dev) > > =C2=A0{ > > =C2=A0 uint8_t pos; > > - struct virtio_pci_cap cap; > > =C2=A0 int ret; > > =C2=A0 > > =C2=A0 ret =3D rte_pci_read_config(dev, &pos, 1, > > PCI_CAPABILITY_LIST); > > - if (ret < 0) { > > - PMD_INIT_LOG(DEBUG, "failed to read pci capability > > list"); > > + if (ret !=3D 1) { > > + PMD_INIT_LOG(DEBUG, > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"failed to read pci capability list, > > ret %d", ret); > > =C2=A0 return VIRTIO_MSIX_NONE; > > =C2=A0 } > > =C2=A0 > > =C2=A0 while (pos) { > > - 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); > > + uint8_t cap[2]; > > + > > + 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); > > =C2=A0 break; > > =C2=A0 } > > =C2=A0 > > - if (cap.cap_vndr =3D=3D PCI_CAP_ID_MSIX) { > > - uint16_t flags =3D ((uint16_t *)&cap)[1]; > > + if (cap[0] =3D=3D PCI_CAP_ID_MSIX) { > > + uint16_t flags; > > + > > + ret =3D rte_pci_read_config(dev, &flags, > > sizeof(flags), > > + pos + sizeof(cap)); > > + if (ret !=3D sizeof(flags)) { > > + PMD_INIT_LOG(DEBUG, > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0"failed to read pci > > cap at pos:" > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0" %x ret %d", pos + > > sizeof(cap), >=20 > There is a build error: >=20 > In file included from drivers/net/virtio/virtio_pci.c:15: > drivers/net/virtio/virtio_pci.c: In function =E2=80=98vtpci_msix_detect= =E2=80=99: > drivers/net/virtio/virtio_logs.h:13:3: error: format =E2=80=98%x=E2=80=99= expects > argument of type =E2=80=98unsigned int=E2=80=99, but argument 5 has type = =E2=80=98long > unsigned int=E2=80=99 [-Werror=3Dformat=3D] > =C2=A0=C2=A0=C2=A0"%s(): " fmt "\n", __func__, ##args) > =C2=A0=C2=A0=C2=A0^~~~~~~~ > drivers/net/virtio/virtio_pci.c:732:5: note: in expansion of macro > =E2=80=98PMD_INIT_LOG=E2=80=99 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0PMD_INIT_LOG(DEBUG, > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0^~~~~~~~~~~~ > drivers/net/virtio/virtio_pci.c:734:14: note: format string is > defined here > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0" %x re= t %d", pos + sizeof(cap), > =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=A0~^ > =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=A0%lx Ok, changed to %lx - which GCC version was that with? Didn't get any warning with 6.3 on Debian 9. --=20 Kind regards, Luca Boccassi