[dpdk-dev] [PATCH 1/2] bus/pci: harmonize and document rte_pci_read_config return value

[dpdk-dev] [PATCH 1/2] bus/pci: harmonize and document rte_pci_read_config return value

[Luca Boccassi]

On Linux, rte_pci_read_config on success returns the number of read
bytes, but on BSD it returns 0.
Document the return values, and have BSD behave as Linux does.

At least one case (bnx2x PMD) treats 0 as an error, so the change
makes sense also for that.

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 drivers/bus/pci/bsd/pci.c     | 4 +++-
 drivers/bus/pci/rte_bus_pci.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
index 655b34b7e4..175d83cf1b 100644
--- a/drivers/bus/pci/bsd/pci.c
+++ b/drivers/bus/pci/bsd/pci.c
@@ -439,6 +439,8 @@ int rte_pci_read_config(const struct rte_pci_device *dev,
 {
 	int fd = -1;
 	int size;
+	/* Copy Linux implementation's behaviour */
+	const int return_len = len;
 	struct pci_io pi = {
 		.pi_sel = {
 			.pc_domain = dev->addr.domain,
@@ -469,7 +471,7 @@ int rte_pci_read_config(const struct rte_pci_device *dev,
 	}
 	close(fd);
 
-	return 0;
+	return return_len;
 
  error:
 	if (fd >= 0)
diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
index 0d1955ffe0..df8f64798d 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -219,6 +219,8 @@ void rte_pci_unregister(struct rte_pci_driver *driver);
  *   The length of the data buffer.
  * @param offset
  *   The offset into PCI config space
+ * @return
+ *  Number of bytes read on success, negative on error.
  */
 int rte_pci_read_config(const struct rte_pci_device *device,
 		void *buf, size_t len, off_t offset);
-- 
2.18.0

[dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling

[Luca Boccassi]

From: Brian Russell <brussell@brocade.com>

In virtio_read_caps, 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.

Signed-off-by: Brian Russell <brussell@brocade.com>
---

Follow-up from:
http://mails.dpdk.org/archives/dev/2017-June/067278.html
https://patches.dpdk.org/patch/25056/

 drivers/net/virtio/virtio_pci.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index 6bd22e54a6..a10698aed8 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -567,16 +567,18 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw)
 	}
 
 	ret = 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 != 1) {
+		PMD_INIT_LOG(DEBUG,
+			     "failed to read pci capability list, ret %d", ret);
 		return -1;
 	}
 
 	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;
 		}
 
-- 
2.18.0

Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling

[Tiwei Bie]

On Tue, Aug 14, 2018 at 03:30:35PM +0100, Luca Boccassi wrote:
> From: Brian Russell <brussell@brocade.com>
> 
> In virtio_read_caps, 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.

Is this a fix or an improvement?
Or did you see anything broken without this patch?
If so, we may need a fixes line and Cc stable.

> 
> Signed-off-by: Brian Russell <brussell@brocade.com>
> ---
> 
> Follow-up from:
> http://mails.dpdk.org/archives/dev/2017-June/067278.html
> https://patches.dpdk.org/patch/25056/
> 
>  drivers/net/virtio/virtio_pci.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
> index 6bd22e54a6..a10698aed8 100644
> --- a/drivers/net/virtio/virtio_pci.c
> +++ b/drivers/net/virtio/virtio_pci.c
> @@ -567,16 +567,18 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw)
>  	}
>  
>  	ret = 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 != 1) {
> +		PMD_INIT_LOG(DEBUG,
> +			     "failed to read pci capability list, ret %d", ret);
>  		return -1;
>  	}
>  
>  	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,

Why change the log level to DEBUG?

Thanks

> +				     "failed to read pci cap at pos: %x ret %d",
> +				     pos, ret);
>  			break;
>  		}
>  
> -- 
> 2.18.0
> 

Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling

[Luca Boccassi]

On Wed, 2018-08-15 at 11:11 +0800, Tiwei Bie wrote:
> On Tue, Aug 14, 2018 at 03:30:35PM +0100, Luca Boccassi wrote:
> > From: Brian Russell <brussell@brocade.com>
> > 
> > In virtio_read_caps, 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.
> 
> Is this a fix or an improvement?
> Or did you see anything broken without this patch?
> If so, we may need a fixes line and Cc stable.

It is a fix, as it was creating problems in production due to the
constant flux of errors in the logs.
But given patch 1/2 is effectively doing a small change in the BSD bus
API, and it's a requirement for 2/2, I don't think we can include it in
the stable releases unfortunately.

> > 
> > Signed-off-by: Brian Russell <brussell@brocade.com>
> > ---
> > 
> > Follow-up from:
> > http://mails.dpdk.org/archives/dev/2017-June/067278.html
> > https://patches.dpdk.org/patch/25056/
> > 
> >  drivers/net/virtio/virtio_pci.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/virtio/virtio_pci.c
> > b/drivers/net/virtio/virtio_pci.c
> > index 6bd22e54a6..a10698aed8 100644
> > --- a/drivers/net/virtio/virtio_pci.c
> > +++ b/drivers/net/virtio/virtio_pci.c
> > @@ -567,16 +567,18 @@ virtio_read_caps(struct rte_pci_device *dev,
> > struct virtio_hw *hw)
> >  	}
> >  
> >  	ret = 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 != 1) {
> > +		PMD_INIT_LOG(DEBUG,
> > +			     "failed to read pci capability list,
> > ret %d", ret);
> >  		return -1;
> >  	}
> >  
> >  	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,
> 
> Why change the log level to DEBUG?
> 
> Thanks

Beforehand reading less than the required amount of bytes caused
problems in the following code, so it warranted printing errors - but
now it will not go ahead without the right amount of data, so it's not
critical anymore to inform the user.
Main issue is, log will get very spammy with errors, and paying
customers don't like that :-)

-- 
Kind regards,
Luca Boccassi

Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling

[Tiwei Bie]

On Wed, Aug 15, 2018 at 10:50:57AM +0100, Luca Boccassi wrote:
> On Wed, 2018-08-15 at 11:11 +0800, Tiwei Bie wrote:
> > On Tue, Aug 14, 2018 at 03:30:35PM +0100, Luca Boccassi wrote:
> > > From: Brian Russell <brussell@brocade.com>
> > > 
> > > In virtio_read_caps, 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.
> > 
> > Is this a fix or an improvement?
> > Or did you see anything broken without this patch?
> > If so, we may need a fixes line and Cc stable.
> 
> It is a fix, as it was creating problems in production due to the
> constant flux of errors in the logs.

Could you be a bit more specific about which errors
were logged if possible?

If my understanding is correct, you mean the errors
were logged because less than the required amount of
bytes were read?

> But given patch 1/2 is effectively doing a small change in the BSD bus
> API, and it's a requirement for 2/2, I don't think we can include it in
> the stable releases unfortunately.

If it's a fix, we need a fixes line.

> 
> > > 
[...]
> > > @@ -567,16 +567,18 @@ virtio_read_caps(struct rte_pci_device *dev,
> > > struct virtio_hw *hw)
> > >  	}
> > >  
> > >  	ret = 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 != 1) {
> > > +		PMD_INIT_LOG(DEBUG,
> > > +			     "failed to read pci capability list, ret %d", ret);
> > >  		return -1;
> > >  	}
> > >  
> > >  	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)) {

Above code has to successfully read a full virtio
PCI capability during each read, otherwise it will
give up reading other capabilities and may fallback
to the legacy mode. In which case it will fail to
read the requested amount of bytes? Should we try
to read the generic PCI fields first?

Besides, you also need to update other calls to
rte_pci_read_config(), e.g.:

https://github.com/DPDK/dpdk/blob/76b9d9de5c7d/drivers/net/virtio/virtio_pci.c#L696

Thanks

> > > +			PMD_INIT_LOG(DEBUG,
> > 
> > Why change the log level to DEBUG?
> > 
> > Thanks
> 
> Beforehand reading less than the required amount of bytes caused
> problems in the following code, so it warranted printing errors - but
> now it will not go ahead without the right amount of data, so it's not
> critical anymore to inform the user.
> Main issue is, log will get very spammy with errors, and paying
> customers don't like that :-)
> 
> -- 
> Kind regards,
> Luca Boccassi

Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling

[Luca Boccassi]

On Thu, 2018-08-16 at 14:46 +0800, Tiwei Bie wrote:
> On Wed, Aug 15, 2018 at 10:50:57AM +0100, Luca Boccassi wrote:
> > On Wed, 2018-08-15 at 11:11 +0800, Tiwei Bie wrote:
> > > On Tue, Aug 14, 2018 at 03:30:35PM +0100, Luca Boccassi wrote:
> > > > From: Brian Russell <brussell@brocade.com>
> > > > 
> > > > In virtio_read_caps, 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.
> > > 
> > > Is this a fix or an improvement?
> > > Or did you see anything broken without this patch?
> > > If so, we may need a fixes line and Cc stable.
> > 
> > It is a fix, as it was creating problems in production due to the
> > constant flux of errors in the logs.
> 
> Could you be a bit more specific about which errors
> were logged if possible?
> 
> If my understanding is correct, you mean the errors
> were logged because less than the required amount of
> bytes were read?

Yes - rte_pci_read_config on Linux will return not just 0/-1, but the
actual number of bytes read. If it's less than the required amount, the
code then goes on and reads garbage, which causes errors later in the
execution. Checking that we actually got the amount of data we need
fixes this issue.

> > But given patch 1/2 is effectively doing a small change in the BSD
> > bus
> > API, and it's a requirement for 2/2, I don't think we can include
> > it in
> > the stable releases unfortunately.
> 
> If it's a fix, we need a fixes line.

Sure, will send a v2.

> > 
> > > > 
> 
> [...]
> > > > @@ -567,16 +567,18 @@ virtio_read_caps(struct rte_pci_device
> > > > *dev,
> > > > struct virtio_hw *hw)
> > > >  	}
> > > >  
> > > >  	ret = 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 != 1) {
> > > > +		PMD_INIT_LOG(DEBUG,
> > > > +			     "failed to read pci capability
> > > > list, ret %d", ret);
> > > >  		return -1;
> > > >  	}
> > > >  
> > > >  	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)) {
> 
> Above code has to successfully read a full virtio
> PCI capability during each read, otherwise it will
> give up reading other capabilities and may fallback
> to the legacy mode. In which case it will fail to
> read the requested amount of bytes? Should we try
> to read the generic PCI fields first?

I do not know what exactly causes less than required bytes to be read,
but we have seen it happen in production (not 100% of the times though
- so I think it's worth keeping the structure as-is). As you said in
that case it falls back to legacy mode which, in our experience in
production deployments, then succeeds. That's why the error level print
is undesired - because the code will actually work via the fallback,
but the customers will see scary errors in the logs and open
escalations :-)

> Besides, you also need to update other calls to
> rte_pci_read_config(), e.g.:
> 
> https://github.com/DPDK/dpdk/blob/76b9d9de5c7d/drivers/net/virtio/vir
> tio_pci.c#L696
> 
> Thanks

Sure I will apply the same changes in v2.

-- 
Kind regards,
Luca Boccassi

[dpdk-dev] [PATCH 1/2] bus/pci: harmonize and document rte_pci_read_config return value

[Luca Boccassi]

On Linux, rte_pci_read_config on success returns the number of read
bytes, but on BSD it returns 0.
Document the return values, and have BSD behave as Linux does.

At least one case (bnx2x PMD) treats 0 as an error, so the change
makes sense also for that.

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 drivers/bus/pci/bsd/pci.c     | 4 +++-
 drivers/bus/pci/rte_bus_pci.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
index 655b34b7e4..175d83cf1b 100644
--- a/drivers/bus/pci/bsd/pci.c
+++ b/drivers/bus/pci/bsd/pci.c
@@ -439,6 +439,8 @@ int rte_pci_read_config(const struct rte_pci_device *dev,
 {
 	int fd = -1;
 	int size;
+	/* Copy Linux implementation's behaviour */
+	const int return_len = len;
 	struct pci_io pi = {
 		.pi_sel = {
 			.pc_domain = dev->addr.domain,
@@ -469,7 +471,7 @@ int rte_pci_read_config(const struct rte_pci_device *dev,
 	}
 	close(fd);
 
-	return 0;
+	return return_len;
 
  error:
 	if (fd >= 0)
diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
index 0d1955ffe0..df8f64798d 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -219,6 +219,8 @@ void rte_pci_unregister(struct rte_pci_driver *driver);
  *   The length of the data buffer.
  * @param offset
  *   The offset into PCI config space
+ * @return
+ *  Number of bytes read on success, negative on error.
  */
 int rte_pci_read_config(const struct rte_pci_device *device,
 		void *buf, size_t len, off_t offset);
-- 
2.18.0

[dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling

[Luca Boccassi]

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
@@ -560,6 +560,7 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw)
 	uint8_t pos;
 	struct virtio_pci_cap cap;
 	int ret;
+	uint32_t multiplier;
 
 	if (rte_pci_map_device(dev)) {
 		PMD_INIT_LOG(DEBUG, "failed to map pci device!");
@@ -567,16 +568,18 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw)
 	}
 
 	ret = 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 != 1) {
+		PMD_INIT_LOG(DEBUG,
+			     "failed to read pci capability list, ret %d", ret);
 		return -1;
 	}
 
 	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;
 		}
 
@@ -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);
@@ -693,16 +700,18 @@ vtpci_msix_detect(struct rte_pci_device *dev)
 	int ret;
 
 	ret = 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 != 1) {
+		PMD_INIT_LOG(DEBUG,
+			     "failed to read pci capability list, ret %d", ret);
 		return VIRTIO_MSIX_NONE;
 	}
 
 	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;
 		}
 
-- 
2.18.0

Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling

[Luca Boccassi]

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.

-- 
Kind regards,
Luca Boccassi

Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling

[Tiwei Bie]

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

[dpdk-dev] [PATCH v3 1/2] bus/pci: harmonize and document rte_pci_read_config return value

[Luca Boccassi]

On Linux, rte_pci_read_config on success returns the number of read
bytes, but on BSD it returns 0.
Document the return values, and have BSD behave as Linux does.

At least one case (bnx2x PMD) treats 0 as an error, so the change
makes sense also for that.

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 drivers/bus/pci/bsd/pci.c     | 4 +++-
 drivers/bus/pci/rte_bus_pci.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
index 655b34b7e4..175d83cf1b 100644
--- a/drivers/bus/pci/bsd/pci.c
+++ b/drivers/bus/pci/bsd/pci.c
@@ -439,6 +439,8 @@ int rte_pci_read_config(const struct rte_pci_device *dev,
 {
 	int fd = -1;
 	int size;
+	/* Copy Linux implementation's behaviour */
+	const int return_len = len;
 	struct pci_io pi = {
 		.pi_sel = {
 			.pc_domain = dev->addr.domain,
@@ -469,7 +471,7 @@ int rte_pci_read_config(const struct rte_pci_device *dev,
 	}
 	close(fd);
 
-	return 0;
+	return return_len;
 
  error:
 	if (fd >= 0)
diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
index 0d1955ffe0..df8f64798d 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -219,6 +219,8 @@ void rte_pci_unregister(struct rte_pci_driver *driver);
  *   The length of the data buffer.
  * @param offset
  *   The offset into PCI config space
+ * @return
+ *  Number of bytes read on success, negative on error.
  */
 int rte_pci_read_config(const struct rte_pci_device *device,
 		void *buf, size_t len, off_t offset);
-- 
2.18.0

[dpdk-dev] [PATCH v3 2/2] virtio: fix PCI config err handling

[Luca Boccassi]

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
v3: do not handle rte_pci_read_config of virtio cap, added in v2,
    as it's less clear what the right thing to do there is

 drivers/net/virtio/virtio_pci.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index 6bd22e54a6..e1df2c3b4d 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -567,16 +567,18 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw)
 	}
 
 	ret = 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 != 1) {
+		PMD_INIT_LOG(DEBUG,
+			     "failed to read pci capability list, ret %d", ret);
 		return -1;
 	}
 
 	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;
 		}
 
@@ -693,16 +695,18 @@ vtpci_msix_detect(struct rte_pci_device *dev)
 	int ret;
 
 	ret = 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 != 1) {
+		PMD_INIT_LOG(DEBUG,
+			     "failed to read pci capability list, ret %d", ret);
 		return VIRTIO_MSIX_NONE;
 	}
 
 	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;
 		}
 
-- 
2.18.0

Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling

[Luca Boccassi]

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 <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

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.

-- 
Kind regards,
Luca Boccassi

Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling

[Tiwei Bie]

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 <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
> 
> 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

Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling

[Luca Boccassi]

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 <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
> > 
> > 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;
		}

-- 
Kind regards,
Luca Boccassi

Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling

[Tiwei Bie]

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 <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
> > > 
> > > 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

[dpdk-dev] [PATCH v4 1/2] bus/pci: harmonize and document rte_pci_read_config return value

[Luca Boccassi]

On Linux, rte_pci_read_config on success returns the number of read
bytes, but on BSD it returns 0.
Document the return values, and have BSD behave as Linux does.

At least one case (bnx2x PMD) treats 0 as an error, so the change
makes sense also for that.

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 drivers/bus/pci/bsd/pci.c     | 4 +++-
 drivers/bus/pci/rte_bus_pci.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
index 655b34b7e4..175d83cf1b 100644
--- a/drivers/bus/pci/bsd/pci.c
+++ b/drivers/bus/pci/bsd/pci.c
@@ -439,6 +439,8 @@ int rte_pci_read_config(const struct rte_pci_device *dev,
 {
 	int fd = -1;
 	int size;
+	/* Copy Linux implementation's behaviour */
+	const int return_len = len;
 	struct pci_io pi = {
 		.pi_sel = {
 			.pc_domain = dev->addr.domain,
@@ -469,7 +471,7 @@ int rte_pci_read_config(const struct rte_pci_device *dev,
 	}
 	close(fd);
 
-	return 0;
+	return return_len;
 
  error:
 	if (fd >= 0)
diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
index 0d1955ffe0..df8f64798d 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -219,6 +219,8 @@ void rte_pci_unregister(struct rte_pci_driver *driver);
  *   The length of the data buffer.
  * @param offset
  *   The offset into PCI config space
+ * @return
+ *  Number of bytes read on success, negative on error.
  */
 int rte_pci_read_config(const struct rte_pci_device *device,
 		void *buf, size_t len, off_t offset);
-- 
2.18.0

[dpdk-dev] [PATCH v4 2/2] virtio: fix PCI config err handling

[Luca Boccassi]

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
v3: do not handle rte_pci_read_config of virtio cap, added in v2,
    as it's less clear what the right thing to do there is
v4: do a more robust check - first check what the vendor is, and
    skip the cap entirely if it's not what we are looking for.

 drivers/net/virtio/virtio_pci.c | 57 ++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 15 deletions(-)

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)
 	}
 
 	ret = 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 != 1) {
+		PMD_INIT_LOG(DEBUG,
+			     "failed to read pci capability list, ret %d", ret);
 		return -1;
 	}
 
 	while (pos) {
+		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 < 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;
 		}
 
@@ -689,25 +703,38 @@ enum virtio_msix_status
 vtpci_msix_detect(struct rte_pci_device *dev)
 {
 	uint8_t pos;
-	struct virtio_pci_cap cap;
 	int ret;
 
 	ret = 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 != 1) {
+		PMD_INIT_LOG(DEBUG,
+			     "failed to read pci capability list, ret %d", ret);
 		return VIRTIO_MSIX_NONE;
 	}
 
 	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);
+		uint8_t cap[2];
+
+		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;
 		}
 
-		if (cap.cap_vndr == PCI_CAP_ID_MSIX) {
-			uint16_t flags = ((uint16_t *)&cap)[1];
+		if (cap[0] == PCI_CAP_ID_MSIX) {
+			uint16_t flags;
+
+			ret = rte_pci_read_config(dev, &flags, sizeof(flags),
+					pos + sizeof(cap));
+			if (ret != sizeof(flags)) {
+				PMD_INIT_LOG(DEBUG,
+					     "failed to read pci cap at pos:"
+					     " %x ret %d", pos + sizeof(cap),
+					     ret);
+				break;
+			}
 
 			if (flags & PCI_MSIX_ENABLE)
 				return VIRTIO_MSIX_ENABLED;
@@ -715,7 +742,7 @@ vtpci_msix_detect(struct rte_pci_device *dev)
 				return VIRTIO_MSIX_DISABLED;
 		}
 
-		pos = cap.cap_next;
+		pos = cap[1];
 	}
 
 	return VIRTIO_MSIX_NONE;
-- 
2.18.0

Re: [dpdk-dev] [PATCH 2/2] virtio: fix PCI config err handling

[Luca Boccassi]

On Fri, 2018-08-24 at 12:23 +0800, Tiwei Bie wrote:
> 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 <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
> > > > 
> > > > 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/vir
> tio_pci.c#L584-L594
> https://github.com/freebsd/freebsd/blob/59e4185a9358/sys/dev/pci/pci.
> c#L892
> 
> Thanks!

Ok, done, see v4.

-- 
Kind regards,
Luca Boccassi

Re: [dpdk-dev] [PATCH v4 2/2] virtio: fix PCI config err handling

[Tiwei Bie]

On Fri, Aug 24, 2018 at 06:14:20PM +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
> v3: do not handle rte_pci_read_config of virtio cap, added in v2,
>     as it's less clear what the right thing to do there is
> v4: do a more robust check - first check what the vendor is, and
>     skip the cap entirely if it's not what we are looking for.
> 
>  drivers/net/virtio/virtio_pci.c | 57 ++++++++++++++++++++++++---------
>  1 file changed, 42 insertions(+), 15 deletions(-)
> 
> 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)
>  	}
>  
>  	ret = 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 != 1) {
> +		PMD_INIT_LOG(DEBUG,
> +			     "failed to read pci capability list, ret %d", ret);
>  		return -1;
>  	}
>  
>  	while (pos) {
> +		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 < 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;
>  		}
>  

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:

	while (pos) {
		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) {
			/* Transitional devices would also have this capability,
			 * that's why we also check if msix is enabled.
			 * 1st byte is cap ID; 2nd byte is the position of next
			 * cap; next two bytes are the flags.
			 */
			uint16_t flags;

			ret = rte_pci_read_config(dev, &flags, sizeof(flags),
					pos + 2);
			if (ret != sizeof(flags)) {
				PMD_INIT_LOG(DEBUG,
					"failed to read pci cap at pos: %x ret %d",
					pos + 2, ret);
				break;
			}

			if (flags & PCI_MSIX_ENABLE)
				hw->use_msix = VIRTIO_MSIX_ENABLED;
			else
				hw->use_msix = VIRTIO_MSIX_DISABLED;
		}

		if (cap.cap_vndr != PCI_CAP_ID_VNDR) {
			PMD_INIT_LOG(DEBUG,
				"[%2x] skipping non VNDR cap id: %02x",
				pos, cap.cap_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;
		}

		PMD_INIT_LOG(DEBUG,
			"[%2x] cfg type: %u, bar: %u, offset: %04x, len: %u",
			pos, cap.cfg_type, cap.bar, cap.offset, cap.length);

		switch (cap.cfg_type) {
		......



> @@ -689,25 +703,38 @@ enum virtio_msix_status
>  vtpci_msix_detect(struct rte_pci_device *dev)
>  {
>  	uint8_t pos;
> -	struct virtio_pci_cap cap;
>  	int ret;
>  
>  	ret = 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 != 1) {
> +		PMD_INIT_LOG(DEBUG,
> +			     "failed to read pci capability list, ret %d", ret);
>  		return VIRTIO_MSIX_NONE;
>  	}
>  
>  	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);
> +		uint8_t cap[2];
> +
> +		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;
>  		}
>  
> -		if (cap.cap_vndr == PCI_CAP_ID_MSIX) {
> -			uint16_t flags = ((uint16_t *)&cap)[1];
> +		if (cap[0] == PCI_CAP_ID_MSIX) {
> +			uint16_t flags;
> +
> +			ret = rte_pci_read_config(dev, &flags, sizeof(flags),
> +					pos + sizeof(cap));
> +			if (ret != sizeof(flags)) {
> +				PMD_INIT_LOG(DEBUG,
> +					     "failed to read pci cap at pos:"
> +					     " %x ret %d", pos + sizeof(cap),

There is a build error:

In file included from drivers/net/virtio/virtio_pci.c:15:
drivers/net/virtio/virtio_pci.c: In function ‘vtpci_msix_detect’:
drivers/net/virtio/virtio_logs.h:13:3: error: format ‘%x’ expects argument of type ‘unsigned int’, but argument 5 has type ‘long unsigned int’ [-Werror=format=]
   "%s(): " fmt "\n", __func__, ##args)
   ^~~~~~~~
drivers/net/virtio/virtio_pci.c:732:5: note: in expansion of macro ‘PMD_INIT_LOG’
     PMD_INIT_LOG(DEBUG,
     ^~~~~~~~~~~~
drivers/net/virtio/virtio_pci.c:734:14: note: format string is defined here
           " %x ret %d", pos + sizeof(cap),
             ~^
             %lx


> +					     ret);
> +				break;
> +			}
>  
>  			if (flags & PCI_MSIX_ENABLE)
>  				return VIRTIO_MSIX_ENABLED;
> @@ -715,7 +742,7 @@ vtpci_msix_detect(struct rte_pci_device *dev)
>  				return VIRTIO_MSIX_DISABLED;
>  		}
>  
> -		pos = cap.cap_next;
> +		pos = cap[1];
>  	}
>  
>  	return VIRTIO_MSIX_NONE;
> -- 
> 2.18.0
> 

[dpdk-dev] [PATCH v5 1/2] bus/pci: harmonize and document rte_pci_read_config return value

[Luca Boccassi]

On Linux, rte_pci_read_config on success returns the number of read
bytes, but on BSD it returns 0.
Document the return values, and have BSD behave as Linux does.

At least one case (bnx2x PMD) treats 0 as an error, so the change
makes sense also for that.

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 drivers/bus/pci/bsd/pci.c     | 4 +++-
 drivers/bus/pci/rte_bus_pci.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
index 655b34b7e4..175d83cf1b 100644
--- a/drivers/bus/pci/bsd/pci.c
+++ b/drivers/bus/pci/bsd/pci.c
@@ -439,6 +439,8 @@ int rte_pci_read_config(const struct rte_pci_device *dev,
 {
 	int fd = -1;
 	int size;
+	/* Copy Linux implementation's behaviour */
+	const int return_len = len;
 	struct pci_io pi = {
 		.pi_sel = {
 			.pc_domain = dev->addr.domain,
@@ -469,7 +471,7 @@ int rte_pci_read_config(const struct rte_pci_device *dev,
 	}
 	close(fd);
 
-	return 0;
+	return return_len;
 
  error:
 	if (fd >= 0)
diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
index 0d1955ffe0..df8f64798d 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -219,6 +219,8 @@ void rte_pci_unregister(struct rte_pci_driver *driver);
  *   The length of the data buffer.
  * @param offset
  *   The offset into PCI config space
+ * @return
+ *  Number of bytes read on success, negative on error.
  */
 int rte_pci_read_config(const struct rte_pci_device *device,
 		void *buf, size_t len, off_t offset);
-- 
2.18.0

[dpdk-dev] [PATCH v5 2/2] virtio: fix PCI config err handling

[Luca Boccassi]

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
v3: do not handle rte_pci_read_config of virtio cap, added in v2,
    as it's less clear what the right thing to do there is
v4: do a more robust check - first check what the vendor is, and
    skip the cap entirely if it's not what we are looking for.
v5: fetch only 2 flags bytes if the vndr is PCI_CAP_ID_MSIX

 drivers/net/virtio/virtio_pci.c | 66 ++++++++++++++++++++++++---------
 1 file changed, 49 insertions(+), 17 deletions(-)

diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index 6bd22e54a6..e900254a12 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -567,16 +567,18 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw)
 	}
 
 	ret = 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 != 1) {
+		PMD_INIT_LOG(DEBUG,
+			     "failed to read pci capability list, ret %d", ret);
 		return -1;
 	}
 
 	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);
+		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;
 		}
 
@@ -586,7 +588,16 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw)
 			 * 1st byte is cap ID; 2nd byte is the position of next
 			 * cap; next two bytes are the flags.
 			 */
-			uint16_t flags = ((uint16_t *)&cap)[1];
+			uint16_t flags;
+
+			ret = rte_pci_read_config(dev, &flags, sizeof(flags),
+					pos + 2);
+			if (ret != sizeof(flags)) {
+				PMD_INIT_LOG(DEBUG,
+					     "failed to read pci cap at pos:"
+					     " %x ret %d", pos + 2, ret);
+				break;
+			}
 
 			if (flags & PCI_MSIX_ENABLE)
 				hw->use_msix = VIRTIO_MSIX_ENABLED;
@@ -601,6 +612,14 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw)
 			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;
+		}
+
 		PMD_INIT_LOG(DEBUG,
 			"[%2x] cfg type: %u, bar: %u, offset: %04x, len: %u",
 			pos, cap.cfg_type, cap.bar, cap.offset, cap.length);
@@ -689,25 +708,38 @@ enum virtio_msix_status
 vtpci_msix_detect(struct rte_pci_device *dev)
 {
 	uint8_t pos;
-	struct virtio_pci_cap cap;
 	int ret;
 
 	ret = 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 != 1) {
+		PMD_INIT_LOG(DEBUG,
+			     "failed to read pci capability list, ret %d", ret);
 		return VIRTIO_MSIX_NONE;
 	}
 
 	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);
+		uint8_t cap[2];
+
+		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;
 		}
 
-		if (cap.cap_vndr == PCI_CAP_ID_MSIX) {
-			uint16_t flags = ((uint16_t *)&cap)[1];
+		if (cap[0] == PCI_CAP_ID_MSIX) {
+			uint16_t flags;
+
+			ret = rte_pci_read_config(dev, &flags, sizeof(flags),
+					pos + sizeof(cap));
+			if (ret != sizeof(flags)) {
+				PMD_INIT_LOG(DEBUG,
+					     "failed to read pci cap at pos:"
+					     " %lx ret %d", pos + sizeof(cap),
+					     ret);
+				break;
+			}
 
 			if (flags & PCI_MSIX_ENABLE)
 				return VIRTIO_MSIX_ENABLED;
@@ -715,7 +747,7 @@ vtpci_msix_detect(struct rte_pci_device *dev)
 				return VIRTIO_MSIX_DISABLED;
 		}
 
-		pos = cap.cap_next;
+		pos = cap[1];
 	}
 
 	return VIRTIO_MSIX_NONE;
-- 
2.18.0

Re: [dpdk-dev] [PATCH v4 2/2] virtio: fix PCI config err handling

[Luca Boccassi]

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 <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
> > v3: do not handle rte_pci_read_config of virtio cap, added in v2,
> >     as it's less clear what the right thing to do there is
> > v4: do a more robust check - first check what the vendor is, and
> >     skip the cap entirely if it's not what we are looking for.
> > 
> >  drivers/net/virtio/virtio_pci.c | 57 ++++++++++++++++++++++++-----
> > ----
> >  1 file changed, 42 insertions(+), 15 deletions(-)
> > 
> > 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)
> >  	}
> >  
> >  	ret = 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 != 1) {
> > +		PMD_INIT_LOG(DEBUG,
> > +			     "failed to read pci capability list,
> > ret %d", ret);
> >  		return -1;
> >  	}
> >  
> >  	while (pos) {
> > +		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 < 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;
> >  		}
> >  
> 
> 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
> >  vtpci_msix_detect(struct rte_pci_device *dev)
> >  {
> >  	uint8_t pos;
> > -	struct virtio_pci_cap cap;
> >  	int ret;
> >  
> >  	ret = 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 != 1) {
> > +		PMD_INIT_LOG(DEBUG,
> > +			     "failed to read pci capability list,
> > ret %d", ret);
> >  		return VIRTIO_MSIX_NONE;
> >  	}
> >  
> >  	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);
> > +		uint8_t cap[2];
> > +
> > +		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;
> >  		}
> >  
> > -		if (cap.cap_vndr == PCI_CAP_ID_MSIX) {
> > -			uint16_t flags = ((uint16_t *)&cap)[1];
> > +		if (cap[0] == PCI_CAP_ID_MSIX) {
> > +			uint16_t flags;
> > +
> > +			ret = rte_pci_read_config(dev, &flags,
> > sizeof(flags),
> > +					pos + sizeof(cap));
> > +			if (ret != sizeof(flags)) {
> > +				PMD_INIT_LOG(DEBUG,
> > +					     "failed to read pci
> > cap at pos:"
> > +					     " %x ret %d", pos +
> > sizeof(cap),
> 
> There is a build error:
> 
> In file included from drivers/net/virtio/virtio_pci.c:15:
> drivers/net/virtio/virtio_pci.c: In function ‘vtpci_msix_detect’:
> drivers/net/virtio/virtio_logs.h:13:3: error: format ‘%x’ expects
> argument of type ‘unsigned int’, but argument 5 has type ‘long
> unsigned int’ [-Werror=format=]
>    "%s(): " fmt "\n", __func__, ##args)
>    ^~~~~~~~
> drivers/net/virtio/virtio_pci.c:732:5: note: in expansion of macro
> ‘PMD_INIT_LOG’
>      PMD_INIT_LOG(DEBUG,
>      ^~~~~~~~~~~~
> drivers/net/virtio/virtio_pci.c:734:14: note: format string is
> defined here
>            " %x ret %d", pos + sizeof(cap),
>              ~^
>              %lx

Ok, changed to %lx - which GCC version was that with? Didn't get any
warning with 6.3 on Debian 9.

-- 
Kind regards,
Luca Boccassi

Re: [dpdk-dev] [PATCH v5 2/2] virtio: fix PCI config err handling

[Tiwei Bie]

I just noticed the title. It should be "net/virtio: xxx",
instead of "virtio: xxx".

On Mon, Aug 27, 2018 at 05:52:40PM +0100, Luca Boccassi wrote:
[...]
> +			ret = rte_pci_read_config(dev, &flags, sizeof(flags),
> +					pos + sizeof(cap));
> +			if (ret != sizeof(flags)) {
> +				PMD_INIT_LOG(DEBUG,
> +					     "failed to read pci cap at pos:"
> +					     " %lx ret %d", pos + sizeof(cap),
> +					     ret);

In file included from drivers/net/virtio/virtio_pci.c:15:0:
drivers/net/virtio/virtio_pci.c: In function ‘vtpci_msix_detect’:
drivers/net/virtio/virtio_logs.h:13:3: error: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 5 has type ‘unsigned int’ [-Werror=format=]
   "%s(): " fmt "\n", __func__, ##args)
   ^
drivers/net/virtio/virtio_pci.c:737:5: note: in expansion of macro ‘PMD_INIT_LOG’
     PMD_INIT_LOG(DEBUG,
     ^
cc1: all warnings being treated as errors

I got above build issues in 32bit build.


Apart from that,

Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>

Thanks!

Re: [dpdk-dev] [PATCH v4 2/2] virtio: fix PCI config err handling

[Tiwei Bie]

On Mon, Aug 27, 2018 at 05:52:56PM +0100, Luca Boccassi wrote:
> 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 <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
> > > v3: do not handle rte_pci_read_config of virtio cap, added in v2,
> > >     as it's less clear what the right thing to do there is
> > > v4: do a more robust check - first check what the vendor is, and
> > >     skip the cap entirely if it's not what we are looking for.
> > > 
> > >  drivers/net/virtio/virtio_pci.c | 57 ++++++++++++++++++++++++-----
> > > ----
> > >  1 file changed, 42 insertions(+), 15 deletions(-)
> > > 
> > > 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)
> > >  	}
> > >  
> > >  	ret = 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 != 1) {
> > > +		PMD_INIT_LOG(DEBUG,
> > > +			     "failed to read pci capability list,
> > > ret %d", ret);
> > >  		return -1;
> > >  	}
> > >  
> > >  	while (pos) {
> > > +		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 < 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;
> > >  		}
> > >  
> > 
> > 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.

Thanks!

> 
> > > @@ -689,25 +703,38 @@ enum virtio_msix_status
> > >  vtpci_msix_detect(struct rte_pci_device *dev)
> > >  {
> > >  	uint8_t pos;
> > > -	struct virtio_pci_cap cap;
> > >  	int ret;
> > >  
> > >  	ret = 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 != 1) {
> > > +		PMD_INIT_LOG(DEBUG,
> > > +			     "failed to read pci capability list,
> > > ret %d", ret);
> > >  		return VIRTIO_MSIX_NONE;
> > >  	}
> > >  
> > >  	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);
> > > +		uint8_t cap[2];
> > > +
> > > +		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;
> > >  		}
> > >  
> > > -		if (cap.cap_vndr == PCI_CAP_ID_MSIX) {
> > > -			uint16_t flags = ((uint16_t *)&cap)[1];
> > > +		if (cap[0] == PCI_CAP_ID_MSIX) {
> > > +			uint16_t flags;
> > > +
> > > +			ret = rte_pci_read_config(dev, &flags,
> > > sizeof(flags),
> > > +					pos + sizeof(cap));
> > > +			if (ret != sizeof(flags)) {
> > > +				PMD_INIT_LOG(DEBUG,
> > > +					     "failed to read pci
> > > cap at pos:"
> > > +					     " %x ret %d", pos +
> > > sizeof(cap),
> > 
> > There is a build error:
> > 
> > In file included from drivers/net/virtio/virtio_pci.c:15:
> > drivers/net/virtio/virtio_pci.c: In function ‘vtpci_msix_detect’:
> > drivers/net/virtio/virtio_logs.h:13:3: error: format ‘%x’ expects
> > argument of type ‘unsigned int’, but argument 5 has type ‘long
> > unsigned int’ [-Werror=format=]
> >    "%s(): " fmt "\n", __func__, ##args)
> >    ^~~~~~~~
> > drivers/net/virtio/virtio_pci.c:732:5: note: in expansion of macro
> > ‘PMD_INIT_LOG’
> >      PMD_INIT_LOG(DEBUG,
> >      ^~~~~~~~~~~~
> > drivers/net/virtio/virtio_pci.c:734:14: note: format string is
> > defined here
> >            " %x ret %d", pos + sizeof(cap),
> >              ~^
> >              %lx
> 
> Ok, changed to %lx - which GCC version was that with? Didn't get any
> warning with 6.3 on Debian 9.

I saw this with gcc (Debian 8.2.0-4) 8.2.0 on Debian sid.
I also saw similar warnings with clang version 6.0.0
(tags/RELEASE_600/final 326565) on FreeBSD 11.2

> 
> -- 
> Kind regards,
> Luca Boccassi

[dpdk-dev] [PATCH v6 1/2] bus/pci: harmonize and document rte_pci_read_config return value

[Luca Boccassi]

On Linux, rte_pci_read_config on success returns the number of read
bytes, but on BSD it returns 0.
Document the return values, and have BSD behave as Linux does.

At least one case (bnx2x PMD) treats 0 as an error, so the change
makes sense also for that.

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 drivers/bus/pci/bsd/pci.c     | 4 +++-
 drivers/bus/pci/rte_bus_pci.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
index 655b34b7e4..175d83cf1b 100644
--- a/drivers/bus/pci/bsd/pci.c
+++ b/drivers/bus/pci/bsd/pci.c
@@ -439,6 +439,8 @@ int rte_pci_read_config(const struct rte_pci_device *dev,
 {
 	int fd = -1;
 	int size;
+	/* Copy Linux implementation's behaviour */
+	const int return_len = len;
 	struct pci_io pi = {
 		.pi_sel = {
 			.pc_domain = dev->addr.domain,
@@ -469,7 +471,7 @@ int rte_pci_read_config(const struct rte_pci_device *dev,
 	}
 	close(fd);
 
-	return 0;
+	return return_len;
 
  error:
 	if (fd >= 0)
diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
index 0d1955ffe0..df8f64798d 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -219,6 +219,8 @@ void rte_pci_unregister(struct rte_pci_driver *driver);
  *   The length of the data buffer.
  * @param offset
  *   The offset into PCI config space
+ * @return
+ *  Number of bytes read on success, negative on error.
  */
 int rte_pci_read_config(const struct rte_pci_device *device,
 		void *buf, size_t len, off_t offset);
-- 
2.18.0

[dpdk-dev] [PATCH v6 2/2] net/virtio: fix PCI config err handling

[Luca Boccassi]

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
v3: do not handle rte_pci_read_config of virtio cap, added in v2,
    as it's less clear what the right thing to do there is
v4: do a more robust check - first check what the vendor is, and
    skip the cap entirely if it's not what we are looking for.
v5: fetch only 2 flags bytes if the vndr is PCI_CAP_ID_MSIX
v6: fix 32bit build by changing the printf format specifier, fix patch title

 drivers/net/virtio/virtio_pci.c | 65 ++++++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 17 deletions(-)

diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index 6bd22e54a6..b6a3c80b4d 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -567,16 +567,18 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw)
 	}
 
 	ret = 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 != 1) {
+		PMD_INIT_LOG(DEBUG,
+			     "failed to read pci capability list, ret %d", ret);
 		return -1;
 	}
 
 	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);
+		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;
 		}
 
@@ -586,7 +588,16 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw)
 			 * 1st byte is cap ID; 2nd byte is the position of next
 			 * cap; next two bytes are the flags.
 			 */
-			uint16_t flags = ((uint16_t *)&cap)[1];
+			uint16_t flags;
+
+			ret = rte_pci_read_config(dev, &flags, sizeof(flags),
+					pos + 2);
+			if (ret != sizeof(flags)) {
+				PMD_INIT_LOG(DEBUG,
+					     "failed to read pci cap at pos:"
+					     " %x ret %d", pos + 2, ret);
+				break;
+			}
 
 			if (flags & PCI_MSIX_ENABLE)
 				hw->use_msix = VIRTIO_MSIX_ENABLED;
@@ -601,6 +612,14 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw)
 			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;
+		}
+
 		PMD_INIT_LOG(DEBUG,
 			"[%2x] cfg type: %u, bar: %u, offset: %04x, len: %u",
 			pos, cap.cfg_type, cap.bar, cap.offset, cap.length);
@@ -689,25 +708,37 @@ enum virtio_msix_status
 vtpci_msix_detect(struct rte_pci_device *dev)
 {
 	uint8_t pos;
-	struct virtio_pci_cap cap;
 	int ret;
 
 	ret = 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 != 1) {
+		PMD_INIT_LOG(DEBUG,
+			     "failed to read pci capability list, ret %d", ret);
 		return VIRTIO_MSIX_NONE;
 	}
 
 	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);
+		uint8_t cap[2];
+
+		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;
 		}
 
-		if (cap.cap_vndr == PCI_CAP_ID_MSIX) {
-			uint16_t flags = ((uint16_t *)&cap)[1];
+		if (cap[0] == PCI_CAP_ID_MSIX) {
+			uint16_t flags;
+
+			ret = rte_pci_read_config(dev, &flags, sizeof(flags),
+					pos + sizeof(cap));
+			if (ret != sizeof(flags)) {
+				PMD_INIT_LOG(DEBUG,
+					     "failed to read pci cap at pos:"
+					     " %x ret %d", pos + 2, ret);
+				break;
+			}
 
 			if (flags & PCI_MSIX_ENABLE)
 				return VIRTIO_MSIX_ENABLED;
@@ -715,7 +746,7 @@ vtpci_msix_detect(struct rte_pci_device *dev)
 				return VIRTIO_MSIX_DISABLED;
 		}
 
-		pos = cap.cap_next;
+		pos = cap[1];
 	}
 
 	return VIRTIO_MSIX_NONE;
-- 
2.18.0

Re: [dpdk-dev] [PATCH v5 2/2] virtio: fix PCI config err handling

[Luca Boccassi]

On Tue, 2018-08-28 at 14:43 +0800, Tiwei Bie wrote:
> I just noticed the title. It should be "net/virtio: xxx",
> instead of "virtio: xxx".

Fixed

> On Mon, Aug 27, 2018 at 05:52:40PM +0100, Luca Boccassi wrote:
> [...]
> > +			ret = rte_pci_read_config(dev, &flags,
> > sizeof(flags),
> > +					pos + sizeof(cap));
> > +			if (ret != sizeof(flags)) {
> > +				PMD_INIT_LOG(DEBUG,
> > +					     "failed to read pci
> > cap at pos:"
> > +					     " %lx ret %d", pos +
> > sizeof(cap),
> > +					     ret);
> 
> In file included from drivers/net/virtio/virtio_pci.c:15:0:
> drivers/net/virtio/virtio_pci.c: In function ‘vtpci_msix_detect’:
> drivers/net/virtio/virtio_logs.h:13:3: error: format ‘%lx’ expects
> argument of type ‘long unsigned int’, but argument 5 has type
> ‘unsigned int’ [-Werror=format=]
>    "%s(): " fmt "\n", __func__, ##args)
>    ^
> drivers/net/virtio/virtio_pci.c:737:5: note: in expansion of macro
> ‘PMD_INIT_LOG’
>      PMD_INIT_LOG(DEBUG,
>      ^
> cc1: all warnings being treated as errors
> 
> I got above build issues in 32bit build.
> 
> 
> Apart from that,
> 
> Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>
> 
> Thanks!

So the 32 and 64 bit builds want opposite things, due to the sizeof
type. I changed it to avoid sizeof and just calculate it like in every
other prints of these functions to avoid the issue.

-- 
Kind regards,
Luca Boccassi

Re: [dpdk-dev] [PATCH v6 2/2] net/virtio: fix PCI config err handling

[Thomas Monjalon]

28/08/2018 12:12, Luca Boccassi:
> 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
> v3: do not handle rte_pci_read_config of virtio cap, added in v2,
>     as it's less clear what the right thing to do there is
> v4: do a more robust check - first check what the vendor is, and
>     skip the cap entirely if it's not what we are looking for.
> v5: fetch only 2 flags bytes if the vndr is PCI_CAP_ID_MSIX
> v6: fix 32bit build by changing the printf format specifier, fix patch title

Tiwei did a comment on v5 and provided his Reviewed-by.
Is it OK to apply v6 with its tag?
All is fixed?

Re: [dpdk-dev] [PATCH v6 2/2] net/virtio: fix PCI config err handling

[Luca Boccassi]

On Thu, 2018-10-11 at 12:27 +0200, Thomas Monjalon wrote:
> 28/08/2018 12:12, Luca Boccassi:
> > 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
> > v3: do not handle rte_pci_read_config of virtio cap, added in v2,
> >     as it's less clear what the right thing to do there is
> > v4: do a more robust check - first check what the vendor is, and
> >     skip the cap entirely if it's not what we are looking for.
> > v5: fetch only 2 flags bytes if the vndr is PCI_CAP_ID_MSIX
> > v6: fix 32bit build by changing the printf format specifier, fix
> > patch title
> 
> Tiwei did a comment on v5 and provided his Reviewed-by.
> Is it OK to apply v6 with its tag?
> All is fixed?

Hi,

The title has been fixed (virtio -> net/virtio) and I just tried a
32bit build and it works fine after the fix from v6.

-- 
Kind regards,
Luca Boccassi

Re: [dpdk-dev] [PATCH v6 2/2] net/virtio: fix PCI config err handling

[Tiwei Bie]

On Thu, Oct 11, 2018 at 11:53:12AM +0100, Luca Boccassi wrote:
> On Thu, 2018-10-11 at 12:27 +0200, Thomas Monjalon wrote:
> > 28/08/2018 12:12, Luca Boccassi:
> > > 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
> > > v3: do not handle rte_pci_read_config of virtio cap, added in v2,
> > >     as it's less clear what the right thing to do there is
> > > v4: do a more robust check - first check what the vendor is, and
> > >     skip the cap entirely if it's not what we are looking for.
> > > v5: fetch only 2 flags bytes if the vndr is PCI_CAP_ID_MSIX
> > > v6: fix 32bit build by changing the printf format specifier, fix
> > > patch title
> > 
> > Tiwei did a comment on v5 and provided his Reviewed-by.
> > Is it OK to apply v6 with its tag?
> > All is fixed?
> 
> Hi,
> 
> The title has been fixed (virtio -> net/virtio) and I just tried a
> 32bit build and it works fine after the fix from v6.

Thanks Luca!

Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>

> 
> -- 
> Kind regards,
> Luca Boccassi

Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: harmonize and document rte_pci_read_config return value

[Luca Boccassi]

On Tue, 2018-08-28 at 11:12 +0100, Luca Boccassi wrote:
> On Linux, rte_pci_read_config on success returns the number of read
> bytes, but on BSD it returns 0.
> Document the return values, and have BSD behave as Linux does.
> 
> At least one case (bnx2x PMD) treats 0 as an error, so the change
> makes sense also for that.
> 
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> ---
>  drivers/bus/pci/bsd/pci.c     | 4 +++-
>  drivers/bus/pci/rte_bus_pci.h | 2 ++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
> index 655b34b7e4..175d83cf1b 100644
> --- a/drivers/bus/pci/bsd/pci.c
> +++ b/drivers/bus/pci/bsd/pci.c
> @@ -439,6 +439,8 @@ int rte_pci_read_config(const struct
> rte_pci_device *dev,
>  {
>  	int fd = -1;
>  	int size;
> +	/* Copy Linux implementation's behaviour */
> +	const int return_len = len;
>  	struct pci_io pi = {
>  		.pi_sel = {
>  			.pc_domain = dev->addr.domain,
> @@ -469,7 +471,7 @@ int rte_pci_read_config(const struct
> rte_pci_device *dev,
>  	}
>  	close(fd);
>  
> -	return 0;
> +	return return_len;
>  
>   error:
>  	if (fd >= 0)
> diff --git a/drivers/bus/pci/rte_bus_pci.h
> b/drivers/bus/pci/rte_bus_pci.h
> index 0d1955ffe0..df8f64798d 100644
> --- a/drivers/bus/pci/rte_bus_pci.h
> +++ b/drivers/bus/pci/rte_bus_pci.h
> @@ -219,6 +219,8 @@ void rte_pci_unregister(struct rte_pci_driver
> *driver);
>   *   The length of the data buffer.
>   * @param offset
>   *   The offset into PCI config space
> + * @return
> + *  Number of bytes read on success, negative on error.
>   */
>  int rte_pci_read_config(const struct rte_pci_device *device,
>  		void *buf, size_t len, off_t offset);

Hi Bruce,

Any chance you could please review the small BSD patch above when you
have a sec? Thanks!

-- 
Kind regards,
Luca Boccassi

Re: [dpdk-dev] [PATCH v6 1/2] bus/pci: harmonize and document rte_pci_read_config return value

[Bruce Richardson]

On Wed, Oct 17, 2018 at 10:58:58AM +0100, Luca Boccassi wrote:
> On Tue, 2018-08-28 at 11:12 +0100, Luca Boccassi wrote:
> > On Linux, rte_pci_read_config on success returns the number of read
> > bytes, but on BSD it returns 0.
> > Document the return values, and have BSD behave as Linux does.
> > 
> > At least one case (bnx2x PMD) treats 0 as an error, so the change
> > makes sense also for that.
> > 
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > ---

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH v6 2/2] net/virtio: fix PCI config err handling

[Thomas Monjalon]

11/10/2018 15:01, Tiwei Bie:
> On Thu, Oct 11, 2018 at 11:53:12AM +0100, Luca Boccassi wrote:
> > On Thu, 2018-10-11 at 12:27 +0200, Thomas Monjalon wrote:
> > > 28/08/2018 12:12, Luca Boccassi:
> > > > 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
> > > > v3: do not handle rte_pci_read_config of virtio cap, added in v2,
> > > >     as it's less clear what the right thing to do there is
> > > > v4: do a more robust check - first check what the vendor is, and
> > > >     skip the cap entirely if it's not what we are looking for.
> > > > v5: fetch only 2 flags bytes if the vndr is PCI_CAP_ID_MSIX
> > > > v6: fix 32bit build by changing the printf format specifier, fix
> > > > patch title
> > > 
> > > Tiwei did a comment on v5 and provided his Reviewed-by.
> > > Is it OK to apply v6 with its tag?
> > > All is fixed?
> > 
> > Hi,
> > 
> > The title has been fixed (virtio -> net/virtio) and I just tried a
> > 32bit build and it works fine after the fix from v6.
> 
> Thanks Luca!
> 
> Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>

Cc: stable@dpdk.org

Series applied, thanks