| Message ID | 1572845537.13810.225.camel@harmonicinc.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Jeff Kirsher |
| Headers | show |
| Series | [net] i40e/iavf: Fix msg interface between VF and PF | expand |
> -----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
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 >
> -----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; > > }
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; > > > } > >
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
> -----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.
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.
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.
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,