[PATCH net] i40e/iavf: Fix msg interface between VF and PF

[PATCH net] i40e/iavf: Fix msg interface between VF and PF

[Arkady Gilinsky]

From af5ab2aaa51882bb7111b026882fe217ed81c15b Mon Sep 17 00:00:00 2001
From: Arkady Gilinsky <arkady.gilinsky@harmonicinc
.com>
Date: Sun, 3 Nov 2019 20:37:21 +0200
Subject: [PATCH net] i40e/iavf: Fix msg interface between VF and PF

 * The original issue was that iavf driver passing TX/RX queues
   as bitmap in iavf_disable_queues and the i40e driver
   interprets this message as an absolute number in
   i40e_vc_disable_queues_msg, so the validation in the
   latter function always fail.
   This commit reorganize the msg interface between i40e and iavf
   between the iavf_disable_queues and i40e_vc_disable_queues_msg
   functions (also for iavf_enable_queues and i40e_vc_enable_queues_msg).
   Now both drivers operate with number of queues instead of
   bitmap. Also the commit introduces range check in
   i40e_vc_enable_queues_msg function.

Signed-off-by: Arkady Gilinsky <arkady.gilinsky@harmonicinc.com>
---
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 23 ++++++++++++++++------
 drivers/net/ethernet/intel/iavf/iavf_virtchnl.c    |  6 ++++--
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 3d2440838822..c650eb91982a 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -2352,13 +2352,22 @@ static int i40e_vc_enable_queues_msg(struct i40e_vf *vf, u8 *msg)
 		goto error_param;
 	}
 
-	/* Use the queue bit map sent by the VF */
-	if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx], vqs->rx_queues,
+	if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
+	    vqs->rx_queues > I40E_MAX_VF_QUEUES ||
+	    vqs->tx_queues > I40E_MAX_VF_QUEUES) {
+		aq_ret = I40E_ERR_PARAM;
+		goto error_param;
+	}
+
+	/* Build the queue bit map from value sent by the VF */
+	if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx],
+				  BIT(vqs->rx_queues) - 1,
 				  true)) {
 		aq_ret = I40E_ERR_TIMEOUT;
 		goto error_param;
 	}
-	if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx], vqs->tx_queues,
+	if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx],
+				  BIT(vqs->tx_queues) - 1,
 				  true)) {
 		aq_ret = I40E_ERR_TIMEOUT;
 		goto error_param;
@@ -2416,13 +2425,15 @@ static int i40e_vc_disable_queues_msg(struct i40e_vf *vf, u8 *msg)
 		goto error_param;
 	}
 
-	/* Use the queue bit map sent by the VF */
-	if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx], vqs->tx_queues,
+	/* Build the queue bit map from value sent by the VF */
+	if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx],
+				  BIT(vqs->tx_queues) - 1,
 				  false)) {
 		aq_ret = I40E_ERR_TIMEOUT;
 		goto error_param;
 	}
-	if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx], vqs->rx_queues,
+	if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx],
+				  BIT(vqs->rx_queues) - 1,
 				  false)) {
 		aq_ret = I40E_ERR_TIMEOUT;
 		goto error_param;
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index c46770eba320..271f144bf05b 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -308,7 +308,8 @@ void iavf_enable_queues(struct iavf_adapter *adapter)
 	}
 	adapter->current_op = VIRTCHNL_OP_ENABLE_QUEUES;
 	vqs.vsi_id = adapter->vsi_res->vsi_id;
-	vqs.tx_queues = BIT(adapter->num_active_queues) - 1;
+	/* Send the queues number to PF */
+	vqs.tx_queues = adapter->num_active_queues;
 	vqs.rx_queues = vqs.tx_queues;
 	adapter->aq_required &= ~IAVF_FLAG_AQ_ENABLE_QUEUES;
 	iavf_send_pf_msg(adapter, VIRTCHNL_OP_ENABLE_QUEUES,
@@ -333,7 +334,8 @@ void iavf_disable_queues(struct iavf_adapter *adapter)
 	}
 	adapter->current_op = VIRTCHNL_OP_DISABLE_QUEUES;
 	vqs.vsi_id = adapter->vsi_res->vsi_id;
-	vqs.tx_queues = BIT(adapter->num_active_queues) - 1;
+	/* Send the queues number to PF */
+	vqs.tx_queues = adapter->num_active_queues;
 	vqs.rx_queues = vqs.tx_queues;
 	adapter->aq_required &= ~IAVF_FLAG_AQ_DISABLE_QUEUES;
 	iavf_send_pf_msg(adapter, VIRTCHNL_OP_DISABLE_QUEUES,
-- 
2.11.0

RE: [PATCH net] i40e/iavf: Fix msg interface between VF and PF

[Creeley, Brett]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On Behalf Of Arkady Gilinsky
> Sent: Sunday, November 3, 2019 9:32 PM
> To: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: Arkady Gilinsky <arcadyg@gmail.com>
> Subject: [PATCH net] i40e/iavf: Fix msg interface between VF and PF
> 
> From af5ab2aaa51882bb7111b026882fe217ed81c15b Mon Sep 17 00:00:00 2001
> From: Arkady Gilinsky <arkady.gilinsky@harmonicinc
> .com>
> Date: Sun, 3 Nov 2019 20:37:21 +0200
> Subject: [PATCH net] i40e/iavf: Fix msg interface between VF and PF
> 
>  * The original issue was that iavf driver passing TX/RX queues
>    as bitmap in iavf_disable_queues and the i40e driver
>    interprets this message as an absolute number in
>    i40e_vc_disable_queues_msg, so the validation in the
>    latter function always fail.

The VIRTCHNL interface expects the tx_queues and rx_queues to be sent as
a bitmap so this should/can not be changed.

I think something like the following would be better becase the change is
completely contained inside the i40e driver and it should completely
address the issue you are seeing. Of course, this would have to be
in both the enable and disable queue flow. Also, with this change it might
be best to check and make sure the sizeof(vqs->[r|t]x_queues) equal
sizeof(u32) in case those members ever change from u32 to u64, which
will cause this check to always fail.

static bool i40e_vc_verify_vqs_bitmaps(struct virtchnl_queue_select *vqs)
{
	/* this will catch any changes made to the virtchnl_queue_select bitmap */
	if (sizeof(vqs->rx_queues) != sizeof(u32) ||
	     sizeof(vqs->tx_queues) != sizeof(u32))
		return false;

	if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
	      hweight32(vqs->rx_queues) > I40E_MAX_VF_QUEUES ||
	      hweight32(vqs->tx_queues) > I40E_MAX_VF_QUEUES)
		return false;

	return true;
}

if (i40e_vc_verify_vqs_bitmaps(vqs)) {
	aq_ret  = I40E_ERR_PARAM;
	goto error_param;
}

>    This commit reorganize the msg interface between i40e and iavf
>    between the iavf_disable_queues and i40e_vc_disable_queues_msg
>    functions (also for iavf_enable_queues and i40e_vc_enable_queues_msg).
>    Now both drivers operate with number of queues instead of
>    bitmap. Also the commit introduces range check in
>    i40e_vc_enable_queues_msg function.
> 
> Signed-off-by: Arkady Gilinsky <arkady.gilinsky@harmonicinc.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 23 ++++++++++++++++------
>  drivers/net/ethernet/intel/iavf/iavf_virtchnl.c    |  6 ++++--
>  2 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> index 3d2440838822..c650eb91982a 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> @@ -2352,13 +2352,22 @@ static int i40e_vc_enable_queues_msg(struct i40e_vf *vf, u8 *msg)
>  		goto error_param;
>  	}
> 
> -	/* Use the queue bit map sent by the VF */
> -	if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx], vqs->rx_queues,
> +	if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> +	    vqs->rx_queues > I40E_MAX_VF_QUEUES ||
> +	    vqs->tx_queues > I40E_MAX_VF_QUEUES) {
> +		aq_ret = I40E_ERR_PARAM;
> +		goto error_param;
> +	}
> +
> +	/* Build the queue bit map from value sent by the VF */
> +	if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx],
> +				  BIT(vqs->rx_queues) - 1,
>  				  true)) {
>  		aq_ret = I40E_ERR_TIMEOUT;
>  		goto error_param;
>  	}
> -	if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx], vqs->tx_queues,
> +	if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx],
> +				  BIT(vqs->tx_queues) - 1,
>  				  true)) {
>  		aq_ret = I40E_ERR_TIMEOUT;
>  		goto error_param;
> @@ -2416,13 +2425,15 @@ static int i40e_vc_disable_queues_msg(struct i40e_vf *vf, u8 *msg)
>  		goto error_param;
>  	}
> 
> -	/* Use the queue bit map sent by the VF */
> -	if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx], vqs->tx_queues,
> +	/* Build the queue bit map from value sent by the VF */
> +	if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx],
> +				  BIT(vqs->tx_queues) - 1,
>  				  false)) {
>  		aq_ret = I40E_ERR_TIMEOUT;
>  		goto error_param;
>  	}
> -	if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx], vqs->rx_queues,
> +	if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx],
> +				  BIT(vqs->rx_queues) - 1,
>  				  false)) {
>  		aq_ret = I40E_ERR_TIMEOUT;
>  		goto error_param;
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index c46770eba320..271f144bf05b 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> @@ -308,7 +308,8 @@ void iavf_enable_queues(struct iavf_adapter *adapter)
>  	}
>  	adapter->current_op = VIRTCHNL_OP_ENABLE_QUEUES;
>  	vqs.vsi_id = adapter->vsi_res->vsi_id;
> -	vqs.tx_queues = BIT(adapter->num_active_queues) - 1;
> +	/* Send the queues number to PF */
> +	vqs.tx_queues = adapter->num_active_queues;
>  	vqs.rx_queues = vqs.tx_queues;
>  	adapter->aq_required &= ~IAVF_FLAG_AQ_ENABLE_QUEUES;
>  	iavf_send_pf_msg(adapter, VIRTCHNL_OP_ENABLE_QUEUES,
> @@ -333,7 +334,8 @@ void iavf_disable_queues(struct iavf_adapter *adapter)
>  	}
>  	adapter->current_op = VIRTCHNL_OP_DISABLE_QUEUES;
>  	vqs.vsi_id = adapter->vsi_res->vsi_id;
> -	vqs.tx_queues = BIT(adapter->num_active_queues) - 1;
> +	/* Send the queues number to PF */
> +	vqs.tx_queues = adapter->num_active_queues;
>  	vqs.rx_queues = vqs.tx_queues;
>  	adapter->aq_required &= ~IAVF_FLAG_AQ_DISABLE_QUEUES;
>  	iavf_send_pf_msg(adapter, VIRTCHNL_OP_DISABLE_QUEUES,
> --
> 2.11.0

Re: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface between VF and PF

[Arkady Gilinsky]

On Mon, 2019-11-04 at 23:43 +0000, Creeley, Brett wrote:
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On Behalf Of Arkady Gilinsky
> > Sent: Sunday, November 3, 2019 9:32 PM
> > To: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> > Cc: Arkady Gilinsky <arcadyg@gmail.com>
> > Subject: [PATCH net] i40e/iavf: Fix msg interface between VF and PF
> > 
> > From af5ab2aaa51882bb7111b026882fe217ed81c15b Mon Sep 17 00:00:00 2001
> > From: Arkady Gilinsky <arkady.gilinsky@harmonicinc
> > .com>
> > Date: Sun, 3 Nov 2019 20:37:21 +0200
> > Subject: [PATCH net] i40e/iavf: Fix msg interface between VF and PF
> > 
> >  * The original issue was that iavf driver passing TX/RX queues
> >    as bitmap in iavf_disable_queues and the i40e driver
> >    interprets this message as an absolute number in
> >    i40e_vc_disable_queues_msg, so the validation in the
> >    latter function always fail.
> 
> The VIRTCHNL interface expects the tx_queues and rx_queues to be sent as
> a bitmap so this should/can not be changed.
The fields [t|r]x_queues through whole driver code contains the number
and not bitmap (as far as I understand), so I thought that keeping 
this convention is more important for better code readability,
saving in these fields something different is very confusing, IMHO, 
and potentially leads to such issues that this commit is trying to solve.
> 
> I think something like the following would be better becase the change is
> completely contained inside the i40e driver and it should completely
> address the issue you are seeing. Of course, this would have to be
> in both the enable and disable queue flow. Also, with this change it might
> be best to check and make sure the sizeof(vqs->[r|t]x_queues) equal
> sizeof(u32) in case those members ever change from u32 to u64, which
> will cause this check to always fail.
> 
> static bool i40e_vc_verify_vqs_bitmaps(struct virtchnl_queue_select *vqs)
> {
> 	/* this will catch any changes made to the virtchnl_queue_select bitmap */
> 	if (sizeof(vqs->rx_queues) != sizeof(u32) ||
> 	     sizeof(vqs->tx_queues) != sizeof(u32))
> 		return false;
If so, then is it better to check the type of the fields in compile-time rather than in runtime ?
Something like this:
BUILD_BUG_ON(sizeof(vqs->rx_queues) != sizeof(u32));
BUILD_BUG_ON(sizeof(vqs->tx_queues) != sizeof(u32));
This is not required comparison each time when function is called and made code more optimized.
> 
> 	if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> 	      hweight32(vqs->rx_queues) > I40E_MAX_VF_QUEUES ||
> 	      hweight32(vqs->tx_queues) > I40E_MAX_VF_QUEUES)
> 		return false;
Again, from optimization POV it is better to have constant changed than variable,
since it is compile time and not run time action:
	if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
	      vqs->rx_queues >= (BIT(I40E_MAX_VF_QUEUES)) ||
	
      vqs->tx_queues >= (BIT(I40E_MAX_VF_QUEUES)))
		return false;
> 	return true;
> }
> 
> if (i40e_vc_verify_vqs_bitmaps(vqs)) {
> 	aq_ret  = I40E_ERR_PARAM;
> 	goto error_param;
> }
> 
> >    This commit reorganize the msg interface between i40e and iavf
> >    between the iavf_disable_queues and i40e_vc_disable_queues_msg
> >    functions (also for iavf_enable_queues and i40e_vc_enable_queues_msg).
> >    Now both drivers operate with number of queues instead of
> >    bitmap. Also the commit introduces range check in
> >    i40e_vc_enable_queues_msg function.
> > 
> > Signed-off-by: Arkady Gilinsky <arkady.gilinsky@harmonicinc.com>
> > ---
> >  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 23 ++++++++++++++++------
> >  drivers/net/ethernet/intel/iavf/iavf_virtchnl.c    |  6 ++++--
> >  2 files changed, 21 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > index 3d2440838822..c650eb91982a 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > @@ -2352,13 +2352,22 @@ static int i40e_vc_enable_queues_msg(struct i40e_vf *vf, u8 *msg)
> >  		goto error_param;
> >  	}
> > 
> > -	/* Use the queue bit map sent by the VF */
> > -	if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx], vqs->rx_queues,
> > +	if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> > +	    vqs->rx_queues > I40E_MAX_VF_QUEUES ||
> > +	    vqs->tx_queues > I40E_MAX_VF_QUEUES) {
> > +		aq_ret = I40E_ERR_PARAM;
> > +		goto error_param;
> > +	}
> > +
> > +	/* Build the queue bit map from value sent by the VF */
> > +	if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx],
> > +				  BIT(vqs->rx_queues) - 1,
> >  				  true)) {
> >  		aq_ret = I40E_ERR_TIMEOUT;
> >  		goto error_param;
> >  	}
> > -	if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx], vqs->tx_queues,
> > +	if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx],
> > +				  BIT(vqs->tx_queues) - 1,
> >  				  true)) {
> >  		aq_ret = I40E_ERR_TIMEOUT;
> >  		goto error_param;
> > @@ -2416,13 +2425,15 @@ static int i40e_vc_disable_queues_msg(struct i40e_vf *vf, u8 *msg)
> >  		goto error_param;
> >  	}
> > 
> > -	/* Use the queue bit map sent by the VF */
> > -	if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx], vqs->tx_queues,
> > +	/* Build the queue bit map from value sent by the VF */
> > +	if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx],
> > +				  BIT(vqs->tx_queues) - 1,
> >  				  false)) {
> >  		aq_ret = I40E_ERR_TIMEOUT;
> >  		goto error_param;
> >  	}
> > -	if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx], vqs->rx_queues,
> > +	if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx],
> > +				  BIT(vqs->rx_queues) - 1,
> >  				  false)) {
> >  		aq_ret = I40E_ERR_TIMEOUT;
> >  		goto error_param;
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> > index c46770eba320..271f144bf05b 100644
> > --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> > @@ -308,7 +308,8 @@ void iavf_enable_queues(struct iavf_adapter *adapter)
> >  	}
> >  	adapter->current_op = VIRTCHNL_OP_ENABLE_QUEUES;
> >  	vqs.vsi_id = adapter->vsi_res->vsi_id;
> > -	vqs.tx_queues = BIT(adapter->num_active_queues) - 1;
> > +	/* Send the queues number to PF */
> > +	vqs.tx_queues = adapter->num_active_queues;
> >  	vqs.rx_queues = vqs.tx_queues;
> >  	adapter->aq_required &= ~IAVF_FLAG_AQ_ENABLE_QUEUES;
> >  	iavf_send_pf_msg(adapter, VIRTCHNL_OP_ENABLE_QUEUES,
> > @@ -333,7 +334,8 @@ void iavf_disable_queues(struct iavf_adapter *adapter)
> >  	}
> >  	adapter->current_op = VIRTCHNL_OP_DISABLE_QUEUES;
> >  	vqs.vsi_id = adapter->vsi_res->vsi_id;
> > -	vqs.tx_queues = BIT(adapter->num_active_queues) - 1;
> > +	/* Send the queues number to PF */
> > +	vqs.tx_queues = adapter->num_active_queues;
> >  	vqs.rx_queues = vqs.tx_queues;
> >  	adapter->aq_required &= ~IAVF_FLAG_AQ_DISABLE_QUEUES;
> >  	iavf_send_pf_msg(adapter, VIRTCHNL_OP_DISABLE_QUEUES,
> > --
> > 2.11.0
> 
-- 
Best regards,
Arkady Gilinsky

------------------------------------------

RE: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface between VF and PF

[Creeley, Brett]

> -----Original Message-----
> From: Arkady Gilinsky <arkady.gilinsky@harmonicinc.com>
> Sent: Monday, November 4, 2019 9:24 PM
> To: Creeley, Brett <brett.creeley@intel.com>; intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kirsher, Jeffrey T
> <jeffrey.t.kirsher@intel.com>
> Cc: Arkady Gilinsky <arcadyg@gmail.com>
> Subject: Re: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface between VF and PF
> > static bool i40e_vc_verify_vqs_bitmaps(struct virtchnl_queue_select *vqs)
> > {
> > 	/* this will catch any changes made to the virtchnl_queue_select bitmap */
> > 	if (sizeof(vqs->rx_queues) != sizeof(u32) ||
> > 	     sizeof(vqs->tx_queues) != sizeof(u32))
> > 		return false;
> If so, then is it better to check the type of the fields in compile-time rather than in runtime ?
> Something like this:
> BUILD_BUG_ON(sizeof(vqs->rx_queues) != sizeof(u32));
> BUILD_BUG_ON(sizeof(vqs->tx_queues) != sizeof(u32));
> This is not required comparison each time when function is called and made code more optimized.

I don't think this is required with the change you suggested below.

> >
> > 	if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> > 	      hweight32(vqs->rx_queues) > I40E_MAX_VF_QUEUES ||
> > 	      hweight32(vqs->tx_queues) > I40E_MAX_VF_QUEUES)
> > 		return false;
> Again, from optimization POV it is better to have constant changed than variable,
> since it is compile time and not run time action:
> 	if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> 	      vqs->rx_queues >= (BIT(I40E_MAX_VF_QUEUES)) ||
> 
>       vqs->tx_queues >= (BIT(I40E_MAX_VF_QUEUES)))
> 		return false;

This seems much better than my solution. It fixes the original issue, handles if the
vqs->[r|t]x_queues variables have changed in size, and the queue bitmap comparison
uses a constant. Thanks!

> > 	return true;
> > }

Re: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface between VF and PF

[Arkady Gilinsky]

On Tue, 2019-11-05 at 16:55 +0000, Creeley, Brett wrote:
> > -----Original Message-----
> > From: Arkady Gilinsky <arkady.gilinsky@harmonicinc.com>
> > Sent: Monday, November 4, 2019 9:24 PM
> > To: Creeley, Brett <brett.creeley@intel.com>; intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kirsher,
> > Jeffrey T
> > <jeffrey.t.kirsher@intel.com>
> > Cc: Arkady Gilinsky <arcadyg@gmail.com>
> > Subject: Re: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface between VF and PF
> > > static bool i40e_vc_verify_vqs_bitmaps(struct virtchnl_queue_select *vqs)
> > > {
> > > 	/* this will catch any changes made to the virtchnl_queue_select bitmap */
> > > 	if (sizeof(vqs->rx_queues) != sizeof(u32) ||
> > > 	     sizeof(vqs->tx_queues) != sizeof(u32))
> > > 		return false;
> > 
> > If so, then is it better to check the type of the fields in compile-time rather than in runtime ?
> > Something like this:
> > BUILD_BUG_ON(sizeof(vqs->rx_queues) != sizeof(u32));
> > BUILD_BUG_ON(sizeof(vqs->tx_queues) != sizeof(u32));
> > This is not required comparison each time when function is called and made code more optimized.
> 
> I don't think this is required with the change you suggested below.
Agree.
If other code in driver not need to be adjusted/verified, then this check is not needed.
> 
> > > 
> > > 	if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> > > 	      hweight32(vqs->rx_queues) > I40E_MAX_VF_QUEUES ||
> > > 	      hweight32(vqs->tx_queues) > I40E_MAX_VF_QUEUES)
> > > 		return false;
> > 
> > Again, from optimization POV it is better to have constant changed than variable,
> > since it is compile time and not run time action:
> > 	if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> > 	      vqs->rx_queues >= (BIT(I40E_MAX_VF_QUEUES)) ||
> > 
> >       vqs->tx_queues >= (BIT(I40E_MAX_VF_QUEUES)))
> > 		return false;
> 
> This seems much better than my solution. It fixes the original issue, handles if the
> vqs->[r|t]x_queues variables have changed in size, and the queue bitmap comparison
> uses a constant. Thanks!
Thanks to you for feedback.
I am trying to understand if this patch will enter into official kernel tree
and, not less important from my POV, to official Intel drivers.
Brett/Jeffrey, could you, please, assist to make sure that this fix, or fix suggested by Brett,
will be integrated into Intel i40e/iavf drivers ?
Or may be I should write mail straight to Intel support ?
> 
> > > 	return true;
> > > }
> 
> 
-- 
Best regards,
Arkady Gilinsky

------------------------------------------

Re: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface between VF and PF

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 2900 bytes --]

On Wed, 2019-11-06 at 05:30 +0000, Arkady Gilinsky wrote:
> On Tue, 2019-11-05 at 16:55 +0000, Creeley, Brett wrote:
> > > -----Original Message-----
> > > From: Arkady Gilinsky <arkady.gilinsky@harmonicinc.com>
> > > Sent: Monday, November 4, 2019 9:24 PM
> > > To: Creeley, Brett <brett.creeley@intel.com>; 
> > > intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kirsher,
> > > Jeffrey T
> > > <jeffrey.t.kirsher@intel.com>
> > > Cc: Arkady Gilinsky <arcadyg@gmail.com>
> > > Subject: Re: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface
> > > between VF and PF
> > > > static bool i40e_vc_verify_vqs_bitmaps(struct virtchnl_queue_select
> > > > *vqs)
> > > > {
> > > >    /* this will catch any changes made to the virtchnl_queue_select
> > > > bitmap */
> > > >    if (sizeof(vqs->rx_queues) != sizeof(u32) ||
> > > >         sizeof(vqs->tx_queues) != sizeof(u32))
> > > >            return false;
> > > 
> > > If so, then is it better to check the type of the fields in compile-
> > > time rather than in runtime ?
> > > Something like this:
> > > BUILD_BUG_ON(sizeof(vqs->rx_queues) != sizeof(u32));
> > > BUILD_BUG_ON(sizeof(vqs->tx_queues) != sizeof(u32));
> > > This is not required comparison each time when function is called and
> > > made code more optimized.
> > 
> > I don't think this is required with the change you suggested below.
> Agree.
> If other code in driver not need to be adjusted/verified, then this check
> is not needed.
> > > >    if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> > > >          hweight32(vqs->rx_queues) > I40E_MAX_VF_QUEUES ||
> > > >          hweight32(vqs->tx_queues) > I40E_MAX_VF_QUEUES)
> > > >            return false;
> > > 
> > > Again, from optimization POV it is better to have constant changed
> > > than variable,
> > > since it is compile time and not run time action:
> > >      if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> > >            vqs->rx_queues >= (BIT(I40E_MAX_VF_QUEUES)) ||
> > > 
> > >       vqs->tx_queues >= (BIT(I40E_MAX_VF_QUEUES)))
> > >              return false;
> > 
> > This seems much better than my solution. It fixes the original issue,
> > handles if the
> > vqs->[r|t]x_queues variables have changed in size, and the queue bitmap
> > comparison
> > uses a constant. Thanks!
> Thanks to you for feedback.
> I am trying to understand if this patch will enter into official kernel
> tree
> and, not less important from my POV, to official Intel drivers.
> Brett/Jeffrey, could you, please, assist to make sure that this fix, or
> fix suggested by Brett,
> will be integrated into Intel i40e/iavf drivers ?
> Or may be I should write mail straight to Intel support ?

As Brett pointed out, there are issues with this patch. Please make the
suggested changes and re-submit the patch to 
intel-wired-lan@lists.osuosl.org

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface between VF and PF

[Creeley, Brett]

> -----Original Message-----
> From: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Sent: Thursday, November 7, 2019 11:39 AM
> To: Arkady Gilinsky <arkady.gilinsky@harmonicinc.com>; Creeley, Brett <brett.creeley@intel.com>; intel-wired-lan@lists.osuosl.org;
> netdev@vger.kernel.org
> Cc: Arkady Gilinsky <arcadyg@gmail.com>
> Subject: Re: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface between VF and PF
> 
> On Wed, 2019-11-06 at 05:30 +0000, Arkady Gilinsky wrote:
> > On Tue, 2019-11-05 at 16:55 +0000, Creeley, Brett wrote:
> > > > -----Original Message-----
> > > > From: Arkady Gilinsky <arkady.gilinsky@harmonicinc.com>
> > > > Sent: Monday, November 4, 2019 9:24 PM
> > > > To: Creeley, Brett <brett.creeley@intel.com>;
> > > > intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kirsher,
> > > > Jeffrey T
> > > > <jeffrey.t.kirsher@intel.com>
> > > > Cc: Arkady Gilinsky <arcadyg@gmail.com>
> > > > Subject: Re: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface
> > > > between VF and PF
> > > > > static bool i40e_vc_verify_vqs_bitmaps(struct virtchnl_queue_select
> > > > > *vqs)
> > > > > {
> > > > >    /* this will catch any changes made to the virtchnl_queue_select
> > > > > bitmap */
> > > > >    if (sizeof(vqs->rx_queues) != sizeof(u32) ||
> > > > >         sizeof(vqs->tx_queues) != sizeof(u32))
> > > > >            return false;
> > > >
> > > > If so, then is it better to check the type of the fields in compile-
> > > > time rather than in runtime ?
> > > > Something like this:
> > > > BUILD_BUG_ON(sizeof(vqs->rx_queues) != sizeof(u32));
> > > > BUILD_BUG_ON(sizeof(vqs->tx_queues) != sizeof(u32));
> > > > This is not required comparison each time when function is called and
> > > > made code more optimized.
> > >
> > > I don't think this is required with the change you suggested below.
> > Agree.
> > If other code in driver not need to be adjusted/verified, then this check
> > is not needed.
> > > > >    if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> > > > >          hweight32(vqs->rx_queues) > I40E_MAX_VF_QUEUES ||
> > > > >          hweight32(vqs->tx_queues) > I40E_MAX_VF_QUEUES)
> > > > >            return false;
> > > >
> > > > Again, from optimization POV it is better to have constant changed
> > > > than variable,
> > > > since it is compile time and not run time action:
> > > >      if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> > > >            vqs->rx_queues >= (BIT(I40E_MAX_VF_QUEUES)) ||
> > > >
> > > >       vqs->tx_queues >= (BIT(I40E_MAX_VF_QUEUES)))
> > > >              return false;
> > >
> > > This seems much better than my solution. It fixes the original issue,
> > > handles if the
> > > vqs->[r|t]x_queues variables have changed in size, and the queue bitmap
> > > comparison
> > > uses a constant. Thanks!
> > Thanks to you for feedback.
> > I am trying to understand if this patch will enter into official kernel
> > tree
> > and, not less important from my POV, to official Intel drivers.
> > Brett/Jeffrey, could you, please, assist to make sure that this fix, or
> > fix suggested by Brett,
> > will be integrated into Intel i40e/iavf drivers ?
> > Or may be I should write mail straight to Intel support ?
> 
> As Brett pointed out, there are issues with this patch. Please make the
> suggested changes and re-submit the patch to
> intel-wired-lan@lists.osuosl.org

Jeff/Arkady: I have already submitted patches for this internally for
official Intel drivers. Apologies for the delayed response.

Re: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface between VF and PF

[Arkady Gilinsky]

Hi All,

Jeffrey/Brett: I did re-submit the patch as "[v2,net] i40e/iavf: Fix msg interface between VF and PF"
Please review.

On Fri, 2019-11-08 at 16:43 +0000, Creeley, Brett wrote:
> > -----Original Message-----
> > From: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> > Sent: Thursday, November 7, 2019 11:39 AM
> > To: Arkady Gilinsky <arkady.gilinsky@harmonicinc.com>; Creeley, Brett <brett.creeley@intel.com>; intel-wired-lan@lis
> > ts.osuosl.org;
> > netdev@vger.kernel.org
> > Cc: Arkady Gilinsky <arcadyg@gmail.com>
> > Subject: Re: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface between VF and PF
> > 
> > On Wed, 2019-11-06 at 05:30 +0000, Arkady Gilinsky wrote:
> > > On Tue, 2019-11-05 at 16:55 +0000, Creeley, Brett wrote:
> > > > > -----Original Message-----
> > > > > From: Arkady Gilinsky <arkady.gilinsky@harmonicinc.com>
> > > > > Sent: Monday, November 4, 2019 9:24 PM
> > > > > To: Creeley, Brett <brett.creeley@intel.com>;
> > > > > intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Kirsher,
> > > > > Jeffrey T
> > > > > <jeffrey.t.kirsher@intel.com>
> > > > > Cc: Arkady Gilinsky <arcadyg@gmail.com>
> > > > > Subject: Re: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface
> > > > > between VF and PF
> > > > > > static bool i40e_vc_verify_vqs_bitmaps(struct virtchnl_queue_select
> > > > > > *vqs)
> > > > > > {
> > > > > >    /* this will catch any changes made to the virtchnl_queue_select
> > > > > > bitmap */
> > > > > >    if (sizeof(vqs->rx_queues) != sizeof(u32) ||
> > > > > >         sizeof(vqs->tx_queues) != sizeof(u32))
> > > > > >            return false;
> > > > > 
> > > > > If so, then is it better to check the type of the fields in compile-
> > > > > time rather than in runtime ?
> > > > > Something like this:
> > > > > BUILD_BUG_ON(sizeof(vqs->rx_queues) != sizeof(u32));
> > > > > BUILD_BUG_ON(sizeof(vqs->tx_queues) != sizeof(u32));
> > > > > This is not required comparison each time when function is called and
> > > > > made code more optimized.
> > > > 
> > > > I don't think this is required with the change you suggested below.
> > > 
> > > Agree.
> > > If other code in driver not need to be adjusted/verified, then this check
> > > is not needed.
> > > > > >    if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> > > > > >          hweight32(vqs->rx_queues) > I40E_MAX_VF_QUEUES ||
> > > > > >          hweight32(vqs->tx_queues) > I40E_MAX_VF_QUEUES)
> > > > > >            return false;
> > > > > 
> > > > > Again, from optimization POV it is better to have constant changed
> > > > > than variable,
> > > > > since it is compile time and not run time action:
> > > > >      if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> > > > >            vqs->rx_queues >= (BIT(I40E_MAX_VF_QUEUES)) ||
> > > > > 
> > > > >       vqs->tx_queues >= (BIT(I40E_MAX_VF_QUEUES)))
> > > > >              return false;
> > > > 
> > > > This seems much better than my solution. It fixes the original issue,
> > > > handles if the
> > > > vqs->[r|t]x_queues variables have changed in size, and the queue bitmap
> > > > comparison
> > > > uses a constant. Thanks!
> > > 
> > > Thanks to you for feedback.
> > > I am trying to understand if this patch will enter into official kernel
> > > tree
> > > and, not less important from my POV, to official Intel drivers.
> > > Brett/Jeffrey, could you, please, assist to make sure that this fix, or
> > > fix suggested by Brett,
> > > will be integrated into Intel i40e/iavf drivers ?
> > > Or may be I should write mail straight to Intel support ?
> > 
> > As Brett pointed out, there are issues with this patch. Please make the
> > suggested changes and re-submit the patch to
> > intel-wired-lan@lists.osuosl.org
> 
> Jeff/Arkady: I have already submitted patches for this internally for
> official Intel drivers. Apologies for the delayed response.
-- 
Best regards,
Arkady Gilinsky

------------------------------------------

Re: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface between VF and PF

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 4508 bytes --]

On Tue, 2019-11-12 at 07:33 +0000, Arkady Gilinsky wrote:
> Hi All,
> 
> Jeffrey/Brett: I did re-submit the patch as "[v2,net] i40e/iavf: Fix msg
> interface between VF and PF"
> Please review.

Sorry, Brett is on vacation for a couple of weeks.  Before he left, he
provided an alternative patch, which I will be submitting later today.

> 
> On Fri, 2019-11-08 at 16:43 +0000, Creeley, Brett wrote:
> > > -----Original Message-----
> > > From: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> > > Sent: Thursday, November 7, 2019 11:39 AM
> > > To: Arkady Gilinsky <arkady.gilinsky@harmonicinc.com>; Creeley, Brett
> > > <brett.creeley@intel.com>; intel-wired-lan@lis
> > > ts.osuosl.org;
> > > netdev@vger.kernel.org
> > > Cc: Arkady Gilinsky <arcadyg@gmail.com>
> > > Subject: Re: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface
> > > between VF and PF
> > > 
> > > On Wed, 2019-11-06 at 05:30 +0000, Arkady Gilinsky wrote:
> > > > On Tue, 2019-11-05 at 16:55 +0000, Creeley, Brett wrote:
> > > > > > -----Original Message-----
> > > > > > From: Arkady Gilinsky <arkady.gilinsky@harmonicinc.com>
> > > > > > Sent: Monday, November 4, 2019 9:24 PM
> > > > > > To: Creeley, Brett <brett.creeley@intel.com>;
> > > > > > intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org;
> > > > > > Kirsher,
> > > > > > Jeffrey T
> > > > > > <jeffrey.t.kirsher@intel.com>
> > > > > > Cc: Arkady Gilinsky <arcadyg@gmail.com>
> > > > > > Subject: Re: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg
> > > > > > interface
> > > > > > between VF and PF
> > > > > > > static bool i40e_vc_verify_vqs_bitmaps(struct
> > > > > > > virtchnl_queue_select
> > > > > > > *vqs)
> > > > > > > {
> > > > > > >    /* this will catch any changes made to the
> > > > > > > virtchnl_queue_select
> > > > > > > bitmap */
> > > > > > >    if (sizeof(vqs->rx_queues) != sizeof(u32) ||
> > > > > > >         sizeof(vqs->tx_queues) != sizeof(u32))
> > > > > > >            return false;
> > > > > > 
> > > > > > If so, then is it better to check the type of the fields in
> > > > > > compile-
> > > > > > time rather than in runtime ?
> > > > > > Something like this:
> > > > > > BUILD_BUG_ON(sizeof(vqs->rx_queues) != sizeof(u32));
> > > > > > BUILD_BUG_ON(sizeof(vqs->tx_queues) != sizeof(u32));
> > > > > > This is not required comparison each time when function is
> > > > > > called and
> > > > > > made code more optimized.
> > > > > 
> > > > > I don't think this is required with the change you suggested
> > > > > below.
> > > > 
> > > > Agree.
> > > > If other code in driver not need to be adjusted/verified, then this
> > > > check
> > > > is not needed.
> > > > > > >    if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> > > > > > >          hweight32(vqs->rx_queues) > I40E_MAX_VF_QUEUES ||
> > > > > > >          hweight32(vqs->tx_queues) > I40E_MAX_VF_QUEUES)
> > > > > > >            return false;
> > > > > > 
> > > > > > Again, from optimization POV it is better to have constant
> > > > > > changed
> > > > > > than variable,
> > > > > > since it is compile time and not run time action:
> > > > > >      if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> > > > > >            vqs->rx_queues >= (BIT(I40E_MAX_VF_QUEUES)) ||
> > > > > > 
> > > > > >       vqs->tx_queues >= (BIT(I40E_MAX_VF_QUEUES)))
> > > > > >              return false;
> > > > > 
> > > > > This seems much better than my solution. It fixes the original
> > > > > issue,
> > > > > handles if the
> > > > > vqs->[r|t]x_queues variables have changed in size, and the queue
> > > > > bitmap
> > > > > comparison
> > > > > uses a constant. Thanks!
> > > > 
> > > > Thanks to you for feedback.
> > > > I am trying to understand if this patch will enter into official
> > > > kernel
> > > > tree
> > > > and, not less important from my POV, to official Intel drivers.
> > > > Brett/Jeffrey, could you, please, assist to make sure that this
> > > > fix, or
> > > > fix suggested by Brett,
> > > > will be integrated into Intel i40e/iavf drivers ?
> > > > Or may be I should write mail straight to Intel support ?
> > > 
> > > As Brett pointed out, there are issues with this patch. Please make
> > > the
> > > suggested changes and re-submit the patch to
> > > intel-wired-lan@lists.osuosl.org
> > 
> > Jeff/Arkady: I have already submitted patches for this internally for
> > official Intel drivers. Apologies for the delayed response.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]