diff mbox series

common/sfc_efx/base: remove VQ index check during VQ start

Message ID 20220708073702.29391-1-asaini@xilinx.com (mailing list archive)
State Superseded, archived
Delegated to: Andrew Rybchenko
Headers
Series common/sfc_efx/base: remove VQ index check during VQ start |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/intel-Testing success Testing PASS

Commit Message

abhimanyu.saini@xilinx.com July 8, 2022, 7:37 a.m. UTC 
From: Abhimanyu Saini <absaini@amd.com>

The used/avail queue indexes are not bound by queue
size, because the descriptor entry index is calculated
by a simple modulo between queue index and queue_size

So, do not check initial used and avail queue indexes
against queue size because it is possible for these
indexes to be greater than queue size in the
following cases:
1) The queue is created to be migrated into, or
2) The client issues a qstop/qstart after running datapath

Fixes: 4dda72dbdeab3 ("common/sfc_efx/base: add base virtio support for vDPA")
Cc: stable@dpdk.org

Signed-off-by: Abhimanyu Saini <absaini@amd.com>
---
 drivers/common/sfc_efx/base/rhead_virtio.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

Comments

Srivastava, Vijay July 8, 2022, 9:17 a.m. UTC | #1 
>From: Abhimanyu Saini <absaini@amd.com>
>
>The used/avail queue indexes are not bound by queue size, because the
>descriptor entry index is calculated by a simple modulo between queue index
>and queue_size
>
>So, do not check initial used and avail queue indexes against queue size
>because it is possible for these indexes to be greater than queue size in the
>following cases:
>1) The queue is created to be migrated into, or
>2) The client issues a qstop/qstart after running datapath
>
>Fixes: 4dda72dbdeab3 ("common/sfc_efx/base: add base virtio support for
>vDPA")
>Cc: stable@dpdk.org
>
>Signed-off-by: Abhimanyu Saini <absaini@amd.com>
>---
> drivers/common/sfc_efx/base/rhead_virtio.c | 12 +-----------
> 1 file changed, 1 insertion(+), 11 deletions(-)
>
>diff --git a/drivers/common/sfc_efx/base/rhead_virtio.c
>b/drivers/common/sfc_efx/base/rhead_virtio.c
>index 335cb74..7f08717 100644
>--- a/drivers/common/sfc_efx/base/rhead_virtio.c
>+++ b/drivers/common/sfc_efx/base/rhead_virtio.c
>@@ -47,14 +47,6 @@
> 		goto fail2;
> 	}
>
>-	if (evvdp != NULL) {
>-		if ((evvdp->evvd_vq_cidx > evvcp->evvc_vq_size) ||
>-		    (evvdp->evvd_vq_pidx > evvcp->evvc_vq_size)) {
>-			rc = EINVAL;
>-			goto fail3;
>-		}
>-	}
>-
> 	req.emr_cmd = MC_CMD_VIRTIO_INIT_QUEUE;
> 	req.emr_in_buf = payload;
> 	req.emr_in_length = MC_CMD_VIRTIO_INIT_QUEUE_REQ_LEN; @@ -
>116,15 +108,13 @@
>
> 	if (req.emr_rc != 0) {
> 		rc = req.emr_rc;
>-		goto fail4;
>+		goto fail3;
> 	}
>
> 	evvp->evv_vi_index = vi_index;
>
> 	return (0);
>
>-fail4:
>-	EFSYS_PROBE(fail4);
> fail3:
> 	EFSYS_PROBE(fail3);
> fail2:
>--
>1.8.3.1

Acked-by: Vijay Srivastava <vijays@amd.com>
Andrew Rybchenko July 11, 2022, 8:25 a.m. UTC | #2 
Hi Vijay,

On 7/8/22 12:17, Srivastava, Vijay wrote:

> Acked-by: Vijay Srivastava <vijays@amd.com>

Please, send a patch to MAINTAINERS to update your E-mail
address from Xilinx to AMD domain.

Andrew.
David Marchand July 11, 2022, 8:31 a.m. UTC | #3 
Hello,

On Mon, Jul 11, 2022 at 10:26 AM Andrew Rybchenko
<andrew.rybchenko@oktetlabs.ru> wrote:
>
> On 7/8/22 12:17, Srivastava, Vijay wrote:
>
> > Acked-by: Vijay Srivastava <vijays@amd.com>
>
> Please, send a patch to MAINTAINERS to update your E-mail
> address from Xilinx to AMD domain.

Please register this mail address to the mailing list, too.
Your recent messages from @amd.com were waiting in the moderation queue.
Andrew Rybchenko July 11, 2022, 8:36 a.m. UTC | #4 
On 7/8/22 10:37, abhimanyu.saini@xilinx.com wrote:
> From: Abhimanyu Saini <absaini@amd.com>
> 
> The used/avail queue indexes are not bound by queue
> size, because the descriptor entry index is calculated
> by a simple modulo between queue index and queue_size

"is calculated" is a bit vague since looking at the code
I've failed to find the place where modulo operation is
done. Don't we need to apply it these values are put
into MCDI message?

> 
> So, do not check initial used and avail queue indexes
> against queue size because it is possible for these
> indexes to be greater than queue size in the
> following cases:
> 1) The queue is created to be migrated into, or
> 2) The client issues a qstop/qstart after running datapath
> 
> Fixes: 4dda72dbdeab3 ("common/sfc_efx/base: add base virtio support for vDPA")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Abhimanyu Saini <absaini@amd.com>
> ---
>   drivers/common/sfc_efx/base/rhead_virtio.c | 12 +-----------
>   1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/drivers/common/sfc_efx/base/rhead_virtio.c b/drivers/common/sfc_efx/base/rhead_virtio.c
> index 335cb74..7f08717 100644
> --- a/drivers/common/sfc_efx/base/rhead_virtio.c
> +++ b/drivers/common/sfc_efx/base/rhead_virtio.c
> @@ -47,14 +47,6 @@
>   		goto fail2;
>   	}
>   
> -	if (evvdp != NULL) {
> -		if ((evvdp->evvd_vq_cidx > evvcp->evvc_vq_size) ||
> -		    (evvdp->evvd_vq_pidx > evvcp->evvc_vq_size)) {
> -			rc = EINVAL;
> -			goto fail3;
> -		}
> -	}
> -
>   	req.emr_cmd = MC_CMD_VIRTIO_INIT_QUEUE;
>   	req.emr_in_buf = payload;
>   	req.emr_in_length = MC_CMD_VIRTIO_INIT_QUEUE_REQ_LEN;
> @@ -116,15 +108,13 @@
>   
>   	if (req.emr_rc != 0) {
>   		rc = req.emr_rc;
> -		goto fail4;
> +		goto fail3;
>   	}
>   
>   	evvp->evv_vi_index = vi_index;
>   
>   	return (0);
>   
> -fail4:
> -	EFSYS_PROBE(fail4);
>   fail3:
>   	EFSYS_PROBE(fail3);
>   fail2:
Saini, Abhimanyu July 12, 2022, 5 a.m. UTC | #5 
[AMD Official Use Only - General]

On 7/8/22 10:37, abhimanyu.saini@xilinx.com wrote:
> > From: Abhimanyu Saini <absaini@amd.com>
> >
> > The used/avail queue indexes are not bound by queue
> > size, because the descriptor entry index is calculated
> > by a simple modulo between queue index and queue_size
> 
> "is calculated" is a bit vague since looking at the code
> I've failed to find the place where modulo operation is
> done. Don't we need to apply it these values are put
> into MCDI message?
> 

The values are added to the MCDI as is and,
the modulo is performed by the hardware.

I can append the commit message to reflect the same?

> >
> > So, do not check initial used and avail queue indexes
> > against queue size because it is possible for these
> > indexes to be greater than queue size in the
> > following cases:
> > 1) The queue is created to be migrated into, or
> > 2) The client issues a qstop/qstart after running datapath
> >
> > Fixes: 4dda72dbdeab3 ("common/sfc_efx/base: add base virtio support for
> vDPA")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Abhimanyu Saini <absaini@amd.com>
> > ---
> >   drivers/common/sfc_efx/base/rhead_virtio.c | 12 +-----------
> >   1 file changed, 1 insertion(+), 11 deletions(-)
> >
> > diff --git a/drivers/common/sfc_efx/base/rhead_virtio.c
> b/drivers/common/sfc_efx/base/rhead_virtio.c
> > index 335cb74..7f08717 100644
> > --- a/drivers/common/sfc_efx/base/rhead_virtio.c
> > +++ b/drivers/common/sfc_efx/base/rhead_virtio.c
> > @@ -47,14 +47,6 @@
> >               goto fail2;
> >       }
> >
> > -     if (evvdp != NULL) {
> > -             if ((evvdp->evvd_vq_cidx > evvcp->evvc_vq_size) ||
> > -                 (evvdp->evvd_vq_pidx > evvcp->evvc_vq_size)) {
> > -                     rc = EINVAL;
> > -                     goto fail3;
> > -             }
> > -     }
> > -
> >       req.emr_cmd = MC_CMD_VIRTIO_INIT_QUEUE;
> >       req.emr_in_buf = payload;
> >       req.emr_in_length = MC_CMD_VIRTIO_INIT_QUEUE_REQ_LEN;
> > @@ -116,15 +108,13 @@
> >
> >       if (req.emr_rc != 0) {
> >               rc = req.emr_rc;
> > -             goto fail4;
> > +             goto fail3;
> >       }
> >
> >       evvp->evv_vi_index = vi_index;
> >
> >       return (0);
> >
> > -fail4:
> > -     EFSYS_PROBE(fail4);
> >   fail3:
> >       EFSYS_PROBE(fail3);
> >   fail2:
diff mbox series

Patch

diff --git a/drivers/common/sfc_efx/base/rhead_virtio.c b/drivers/common/sfc_efx/base/rhead_virtio.c
index 335cb74..7f08717 100644
--- a/drivers/common/sfc_efx/base/rhead_virtio.c
+++ b/drivers/common/sfc_efx/base/rhead_virtio.c
@@ -47,14 +47,6 @@ 
 		goto fail2;
 	}
 
-	if (evvdp != NULL) {
-		if ((evvdp->evvd_vq_cidx > evvcp->evvc_vq_size) ||
-		    (evvdp->evvd_vq_pidx > evvcp->evvc_vq_size)) {
-			rc = EINVAL;
-			goto fail3;
-		}
-	}
-
 	req.emr_cmd = MC_CMD_VIRTIO_INIT_QUEUE;
 	req.emr_in_buf = payload;
 	req.emr_in_length = MC_CMD_VIRTIO_INIT_QUEUE_REQ_LEN;
@@ -116,15 +108,13 @@ 
 
 	if (req.emr_rc != 0) {
 		rc = req.emr_rc;
-		goto fail4;
+		goto fail3;
 	}
 
 	evvp->evv_vi_index = vi_index;
 
 	return (0);
 
-fail4:
-	EFSYS_PROBE(fail4);
 fail3:
 	EFSYS_PROBE(fail3);
 fail2: