From mboxrd@z Thu Jan 1 00:00:00 1970 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) 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=3eRs90VaLBQqZllZvJwQJML5D05yehIbA0td+W1FIrU=; b=s68N9x8ulHl7xN4L87mAkbetHGJIEekLZoYhMKvwii3HhtrdV1d/0Ia1AkE5ECue05 q7untM8b0pvywPGiWxWl4hp3h5FeHLyzflHLD0ViPuKSG4kjPD7eGRJmy9zUzSXH1Ae4 GLo7k5dRGlvc5rUOXLAbRULepvg/nhtsxoxenX1YvvwDNy0hJVsFhdpMhaxOlhYmX2Ef noewC4PWk+ce6j3oFZnu8yIUy565hT4QBw6wsD5uwLggs3ba8JbU/CfKU4qKMw0Fs+6i UP6zPMImCx7/7sGoCUXakAKog1W90Djm8rqx9sKYPZt26AdwZdRP3kA4jy0v2V2ILhX9 t+Sw== X-Gm-Message-State: APzg51DMniZCTprmpm0948VN3kbI79e4bTCQfW4/raJMcBKhbfZHtGVZ KxCJzt8sWHKTAY4pWnj4hscKtBZr X-Google-Smtp-Source: ANB0VdZ5o6aUjlfHV62LJbOQvGDQMbU0meEMe5zE8v3y5ioqJpjPchnRbPr95VF33RzKnLLOcdhgjw== X-Received: by 2002:a5d:574b:: with SMTP id q11-v6mr9196747wrw.272.1535388778597; Mon, 27 Aug 2018 09:52:58 -0700 (PDT) Received: from localhost ([2a01:4b00:f419:6f00:8361:8946:ba2b:d556]) by smtp.gmail.com with ESMTPSA id s2-v6sm14183704wrn.83.2018.08.27.09.52.57 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 27 Aug 2018 09:52:57 -0700 (PDT) Message-ID: <1535388776.8636.15.camel@debian.org> From: Luca Boccassi To: Tiwei Bie Cc: dev@dpdk.org, maxime.coquelin@redhat.com, zhihong.wang@intel.com, bruce.richardson@intel.com, brian.russell@intl.att.com Date: Mon, 27 Aug 2018 17:52:56 +0100 In-Reply-To: <20180827052934.GA34061@fbsd.sh.intel.com> 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> 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 v4 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Aug 2018 16:52:59 -0000 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