From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 4C07210A3 for ; Fri, 24 Aug 2018 06:25:00 +0200 (CEST) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Aug 2018 21:24:59 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,280,1531810800"; d="scan'208";a="251521342" Received: from debian.sh.intel.com (HELO debian) ([10.67.104.194]) by orsmga005.jf.intel.com with ESMTP; 23 Aug 2018 21:24:42 -0700 Date: Fri, 24 Aug 2018 12:23:55 +0800 From: Tiwei Bie To: Luca Boccassi Cc: dev@dpdk.org, maxime.coquelin@redhat.com, zhihong.wang@intel.com, bruce.richardson@intel.com, brian.russell@intl.att.com Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1535028745.5764.134.camel@debian.org> User-Agent: Mutt/1.10.1 (2018-07-13) 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Aug 2018 04:25:01 -0000 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 > > > > > > > > > > > > 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. > > > > > > > > > > > > Fixes: 6ba1f63b5ab0 ("virtio: support specification 1.0") > > > > > > > > > > > > Signed-off-by: Brian Russell > > > > > > Signed-off-by: Luca Boccassi > > > > > > --- > > > > > > v2: handle additional rte_pci_read_config incomplete reads > > > > > > > > > > > >  drivers/net/virtio/virtio_pci.c | 35 +++++++++++++++++++++ > > > > > > ---- > > > > > > ---- > > > > > > ---- > > > > > >  1 file changed, 22 insertions(+), 13 deletions(-) > > > > > > > > > > > > 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 > > > > > > > > > > ... > > > > > > @@ -610,9 +613,13 @@ virtio_read_caps(struct rte_pci_device > > > > > > *dev, > > > > > > struct virtio_hw *hw) > > > > > >   hw->common_cfg = get_cfg_addr(dev, > > > > > > &cap); > > > > > >   break; > > > > > >   case VIRTIO_PCI_CAP_NOTIFY_CFG: > > > > > > - rte_pci_read_config(dev, &hw- > > > > > > > notify_off_multiplier, > > > > > > > > > > > > - 4, pos + > > > > > > sizeof(cap)); > > > > > > - hw->notify_base = get_cfg_addr(dev, > > > > > > &cap); > > > > > > + /* Avoid half-reads into hw */ > > > > > > + ret = rte_pci_read_config(dev, > > > > > > &multiplier, > > > > > > 4, > > > > > > + pos + sizeof(cap)); > > > > > > + if (ret == 4) { > > > > > > + hw->notify_off_multiplier = > > > > > > multiplier; > > > > > > + hw->notify_base = > > > > > > get_cfg_addr(dev, > > > > > > &cap); > > > > > > + } > > > > > >   break; > > > > > >   case VIRTIO_PCI_CAP_DEVICE_CFG: > > > > > >   hw->dev_cfg = get_cfg_addr(dev, > > > > > > &cap); > > > > > > > > > > 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. > > > > > > > > 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: > > > > > > > > https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/pci/ > > > > pci. > > > > c#L1491-L1497 > > > > > > 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 :-) > > > > > > I've sent a v3 that removes that individual change. > > > > My concern isn't about the above change (which handled the > > errors when reading multiplier). In fact, above change looks > > good to me! :-)  I mean the below changes in this patch: > > > >   while (pos) { > >   ret = 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 != sizeof(cap)) { > > + PMD_INIT_LOG(DEBUG, > > +      "failed to read pci cap at pos: > > %x ret %d", > > +      pos, ret); > >   break; > >   } > > > > 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: > > > > https://github.com/freebsd/freebsd/blob/72445a41b3ff/sys/dev/pci/pci. > > c#L1491-L1497 > > > > How do you think? > > > > Thanks > > Ah I see what you mean now, sorry. Would something like the following > be what you are looking for: > > ret = rte_pci_read_config(dev, &cap, 2, pos); > if (ret != 2) { > PMD_INIT_LOG(DEBUG, > "failed to read pci cap at pos: %x ret %d", > pos, ret); > break; > } > if (cap.cap_vndr != PCI_CAP_ID_MSIX && > cap.cap_vndr != PCI_CAP_ID_VNDR) { > goto next; > } > > ret = rte_pci_read_config(dev, &cap, sizeof(cap), pos); > if (ret != sizeof(cap)) { > PMD_INIT_LOG(DEBUG, > "failed to read pci cap at pos: %x ret %d", > pos, ret); > break; > } > 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: https://github.com/DPDK/dpdk/blob/7281cf520f89/drivers/net/virtio/virtio_pci.c#L584-L594 https://github.com/freebsd/freebsd/blob/59e4185a9358/sys/dev/pci/pci.c#L892 Thanks! > -- > Kind regards, > Luca Boccassi