DPDK patches and discussions
 help / color / mirror / Atom feed
From: Tiwei Bie <tiwei.bie@intel.com>
To: Luca Boccassi <bluca@debian.org>
Cc: dev@dpdk.org, maxime.coquelin@redhat.com, zhihong.wang@intel.com,
	bruce.richardson@intel.com, brian.russell@intl.att.com
Subject: Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling
Date: Mon, 20 Aug 2018 16:18:04 +0800
Message-ID: <20180820081804.GA29806@debian> (raw)
In-Reply-To: <1534445383.5764.56.camel@debian.org>

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>
> > 
> > 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 <brussell@brocade.com>
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > ---
> > 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

Thanks

  reply	other threads:[~2018-08-20  8:19 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-14 14:30 [dpdk-dev] [PATCH 1/2] bus/pci: harmonize and document rte_pci_read_config return value Luca Boccassi
2018-08-14 14:30 ` [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling Luca Boccassi
2018-08-15  3:11   ` Tiwei Bie
2018-08-15  9:50     ` Luca Boccassi
2018-08-16  6:46       ` Tiwei Bie
2018-08-16 10:27         ` Luca Boccassi
2018-08-16 18:47 ` [dpdk-dev] [PATCH 1/2] bus/pci: harmonize and document rte_pci_read_config return value Luca Boccassi
2018-08-16 18:47   ` [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling Luca Boccassi
2018-08-16 18:49     ` Luca Boccassi
2018-08-20  8:18       ` Tiwei Bie [this message]
2018-08-20 16:45         ` Luca Boccassi
2018-08-21  2:40           ` Tiwei Bie
2018-08-23 12:52             ` Luca Boccassi
2018-08-24  4:23               ` Tiwei Bie
2018-08-24 17:14                 ` Luca Boccassi
2018-08-20 16:44   ` [dpdk-dev] [PATCH v3 1/2] bus/pci: harmonize and document rte_pci_read_config return value Luca Boccassi
2018-08-20 16:44     ` [dpdk-dev] [PATCH v3 2/2] virtio: fix PCI config err handling Luca Boccassi
2018-08-24 17:14     ` [dpdk-dev] [PATCH v4 1/2] bus/pci: harmonize and document rte_pci_read_config return value Luca Boccassi
2018-08-24 17:14       ` [dpdk-dev] [PATCH v4 2/2] virtio: fix PCI config err handling Luca Boccassi
2018-08-27  5:29         ` Tiwei Bie
2018-08-27 16:52           ` Luca Boccassi
2018-08-28  6:47             ` Tiwei Bie
2018-08-27 16:52       ` [dpdk-dev] [PATCH v5 1/2] bus/pci: harmonize and document rte_pci_read_config return value Luca Boccassi
2018-08-27 16:52         ` [dpdk-dev] [PATCH v5 2/2] virtio: fix PCI config err handling Luca Boccassi
2018-08-28  6:43           ` Tiwei Bie
2018-08-28 10:14             ` Luca Boccassi
2018-08-28 10:12         ` [dpdk-dev] [PATCH v6 1/2] bus/pci: harmonize and document rte_pci_read_config return value Luca Boccassi
2018-08-28 10:12           ` [dpdk-dev] [PATCH v6 2/2] net/virtio: fix PCI config err handling Luca Boccassi
2018-10-11 10:27             ` Thomas Monjalon
2018-10-11 10:53               ` Luca Boccassi
2018-10-11 13:01                 ` Tiwei Bie
2018-10-28 23:55                   ` Thomas Monjalon
2018-10-17  9:58           ` [dpdk-dev] [PATCH v6 1/2] bus/pci: harmonize and document rte_pci_read_config return value Luca Boccassi
2018-10-17 10:57             ` Bruce Richardson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180820081804.GA29806@debian \
    --to=tiwei.bie@intel.com \
    --cc=bluca@debian.org \
    --cc=brian.russell@intl.att.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=maxime.coquelin@redhat.com \
    --cc=zhihong.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git