[PATCH] net/iavf: fix abnormal disable HW interrupt

[PATCH] net/iavf: fix abnormal disable HW interrupt

[Mingjin Ye]

For command VIRTCHNL_OP_REQUEST_QUEUES, polling access to the admin
queue has the issue of access overruns after disabling interrupt. That
results in FW disabling HW interrupt for protection purposes.

The updates/changes in this patch:
1. Remove the polling admin queue processing and use the generic interrupt
processing instead.
2. Release redundant queue resource before stopping processing interrupt
events.

Fixes: 22b123a36d07 ("net/avf: initialize PMD")
Fixes: ef807926e148 ("net/iavf: support requesting additional queues from PF")
Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel message")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
 drivers/net/iavf/iavf_ethdev.c | 25 +++++++++---------
 drivers/net/iavf/iavf_vchnl.c  | 48 +++++++---------------------------
 2 files changed, 23 insertions(+), 50 deletions(-)

diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
index e6cf897293..ba5c88a1ec 100644
--- a/drivers/net/iavf/iavf_ethdev.c
+++ b/drivers/net/iavf/iavf_ethdev.c
@@ -2756,6 +2756,19 @@ iavf_dev_close(struct rte_eth_dev *dev)
 	}
 
 	ret = iavf_dev_stop(dev);
+
+	/*
+	 * Release redundant queue resource when close the dev
+	 * so that other vfs can re-use the queues.
+	 */
+	if (vf->lv_enabled) {
+		ret = iavf_request_queues(dev, IAVF_MAX_NUM_QUEUES_DFLT);
+		if (ret)
+			PMD_DRV_LOG(ERR, "Reset the num of queues failed");
+
+		vf->max_rss_qregion = IAVF_MAX_NUM_QUEUES_DFLT;
+	}
+
 	adapter->closed = true;
 
 	/* free iAVF security device context all related resources */
@@ -2772,18 +2785,6 @@ iavf_dev_close(struct rte_eth_dev *dev)
 	if (vf->promisc_unicast_enabled || vf->promisc_multicast_enabled)
 		iavf_config_promisc(adapter, false, false);
 
-	/*
-	 * Release redundant queue resource when close the dev
-	 * so that other vfs can re-use the queues.
-	 */
-	if (vf->lv_enabled) {
-		ret = iavf_request_queues(dev, IAVF_MAX_NUM_QUEUES_DFLT);
-		if (ret)
-			PMD_DRV_LOG(ERR, "Reset the num of queues failed");
-
-		vf->max_rss_qregion = IAVF_MAX_NUM_QUEUES_DFLT;
-	}
-
 	iavf_shutdown_adminq(hw);
 	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_WB_ON_ITR) {
 		/* disable uio intr before callback unregister */
diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index 8cc5377bcf..579c0d0d70 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -323,6 +323,7 @@ iavf_execute_vf_cmd(struct iavf_adapter *adapter, struct iavf_cmd_info *args,
 
 	switch (args->ops) {
 	case VIRTCHNL_OP_RESET_VF:
+	case VIRTCHNL_OP_REQUEST_QUEUES:
 		/*no need to wait for response */
 		_clear_cmd(vf);
 		break;
@@ -346,33 +347,6 @@ iavf_execute_vf_cmd(struct iavf_adapter *adapter, struct iavf_cmd_info *args,
 		}
 		_clear_cmd(vf);
 		break;
-	case VIRTCHNL_OP_REQUEST_QUEUES:
-		/*
-		 * ignore async reply, only wait for system message,
-		 * vf_reset = true if get VIRTCHNL_EVENT_RESET_IMPENDING,
-		 * if not, means request queues failed.
-		 */
-		do {
-			result = iavf_read_msg_from_pf(adapter, args->out_size,
-						   args->out_buffer);
-			if (result == IAVF_MSG_SYS && vf->vf_reset) {
-				break;
-			} else if (result == IAVF_MSG_CMD ||
-				result == IAVF_MSG_ERR) {
-				err = -1;
-				break;
-			}
-			iavf_msec_delay(ASQ_DELAY_MS);
-			/* If don't read msg or read sys event, continue */
-		} while (i++ < MAX_TRY_TIMES);
-		if (i >= MAX_TRY_TIMES ||
-			vf->cmd_retval != VIRTCHNL_STATUS_SUCCESS) {
-			err = -1;
-			PMD_DRV_LOG(ERR, "No response or return failure (%d)"
-				    " for cmd %d", vf->cmd_retval, args->ops);
-		}
-		_clear_cmd(vf);
-		break;
 	default:
 		/* For other virtchnl ops in running time,
 		 * wait for the cmd done flag.
@@ -2055,11 +2029,11 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
 	struct iavf_adapter *adapter =
 		IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 	struct iavf_info *vf =  IAVF_DEV_PRIVATE_TO_VF(adapter);
-	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct virtchnl_vf_res_request vfres;
 	struct iavf_cmd_info args;
 	uint16_t num_queue_pairs;
 	int err;
+	int i = 0;
 
 	if (!(vf->vf_res->vf_cap_flags &
 		VIRTCHNL_VF_OFFLOAD_REQ_QUEUES)) {
@@ -2080,16 +2054,7 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
 	args.out_size = IAVF_AQ_BUF_SZ;
 
 	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_WB_ON_ITR) {
-		/* disable interrupt to avoid the admin queue message to be read
-		 * before iavf_read_msg_from_pf.
-		 *
-		 * don't disable interrupt handler until ready to execute vf cmd.
-		 */
-		rte_spinlock_lock(&vf->aq_lock);
-		rte_intr_disable(pci_dev->intr_handle);
-		err = iavf_execute_vf_cmd(adapter, &args, 0);
-		rte_intr_enable(pci_dev->intr_handle);
-		rte_spinlock_unlock(&vf->aq_lock);
+		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	} else {
 		rte_eal_alarm_cancel(iavf_dev_alarm_handler, dev);
 		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
@@ -2102,6 +2067,13 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
 		return err;
 	}
 
+	/* wait for interrupt notification vf is resetting */
+	while (i++ < MAX_TRY_TIMES) {
+		if (vf->vf_reset)
+			break;
+		iavf_msec_delay(ASQ_DELAY_MS);
+	}
+
 	/* request queues succeeded, vf is resetting */
 	if (vf->vf_reset) {
 		PMD_DRV_LOG(INFO, "vf is resetting");
-- 
2.25.1

RE: [PATCH] net/iavf: fix abnormal disable HW interrupt

[Zhang, Qi Z]

> -----Original Message-----
> From: Mingjin Ye <mingjinx.ye@intel.com>
> Sent: Thursday, June 8, 2023 2:23 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>;
> stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>
> Subject: [PATCH] net/iavf: fix abnormal disable HW interrupt
> 
> For command VIRTCHNL_OP_REQUEST_QUEUES, polling access to the admin
> queue has the issue of access overruns after disabling interrupt. That results
> in FW disabling HW interrupt for protection purposes.
> 
> The updates/changes in this patch:
> 1. Remove the polling admin queue processing and use the generic interrupt
> processing instead.
> 2. Release redundant queue resource before stopping processing interrupt
> events.
> 
> Fixes: 22b123a36d07 ("net/avf: initialize PMD")
> Fixes: ef807926e148 ("net/iavf: support requesting additional queues from
> PF")
> Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel
> message")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---
>  drivers/net/iavf/iavf_ethdev.c | 25 +++++++++---------
> drivers/net/iavf/iavf_vchnl.c  | 48 +++++++---------------------------
>  2 files changed, 23 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
> index e6cf897293..ba5c88a1ec 100644
> --- a/drivers/net/iavf/iavf_ethdev.c
> +++ b/drivers/net/iavf/iavf_ethdev.c
> @@ -2756,6 +2756,19 @@ iavf_dev_close(struct rte_eth_dev *dev)
>  	}
> 
>  	ret = iavf_dev_stop(dev);
> +
> +	/*
> +	 * Release redundant queue resource when close the dev
> +	 * so that other vfs can re-use the queues.
> +	 */
> +	if (vf->lv_enabled) {
> +		ret = iavf_request_queues(dev,
> IAVF_MAX_NUM_QUEUES_DFLT);
> +		if (ret)
> +			PMD_DRV_LOG(ERR, "Reset the num of queues
> failed");
> +
> +		vf->max_rss_qregion = IAVF_MAX_NUM_QUEUES_DFLT;
> +	}
> +
>  	adapter->closed = true;
> 
>  	/* free iAVF security device context all related resources */ @@ -
> 2772,18 +2785,6 @@ iavf_dev_close(struct rte_eth_dev *dev)
>  	if (vf->promisc_unicast_enabled || vf->promisc_multicast_enabled)
>  		iavf_config_promisc(adapter, false, false);
> 
> -	/*
> -	 * Release redundant queue resource when close the dev
> -	 * so that other vfs can re-use the queues.
> -	 */
> -	if (vf->lv_enabled) {
> -		ret = iavf_request_queues(dev,
> IAVF_MAX_NUM_QUEUES_DFLT);
> -		if (ret)
> -			PMD_DRV_LOG(ERR, "Reset the num of queues
> failed");
> -
> -		vf->max_rss_qregion = IAVF_MAX_NUM_QUEUES_DFLT;
> -	}
> -
>  	iavf_shutdown_adminq(hw);
>  	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_WB_ON_ITR)
> {
>  		/* disable uio intr before callback unregister */ diff --git
> a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c index
> 8cc5377bcf..579c0d0d70 100644
> --- a/drivers/net/iavf/iavf_vchnl.c
> +++ b/drivers/net/iavf/iavf_vchnl.c
> @@ -323,6 +323,7 @@ iavf_execute_vf_cmd(struct iavf_adapter *adapter,
> struct iavf_cmd_info *args,
> 
>  	switch (args->ops) {
>  	case VIRTCHNL_OP_RESET_VF:
> +	case VIRTCHNL_OP_REQUEST_QUEUES:
>  		/*no need to wait for response */
>  		_clear_cmd(vf);
>  		break;

Have you tested the "large VF" case ( > 16 queue) with this patch.
I assume event VIRTCHNL_EVENT_RESET_IMPENDING need to be handled here then to trigger a device reset.
But I didn't see related part in your patch.

RE: [PATCH] net/iavf: fix abnormal disable HW interrupt

[Ye, MingjinX]

> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: 2023年6月9日 9:48
> To: Ye, MingjinX <mingjinx.ye@intel.com>; dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>;
> stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>
> Subject: RE: [PATCH] net/iavf: fix abnormal disable HW interrupt
> 
> 
> 
> > -----Original Message-----
> > From: Mingjin Ye <mingjinx.ye@intel.com>
> > Sent: Thursday, June 8, 2023 2:23 PM
> > To: dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> > <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>;
> > stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>
> > Subject: [PATCH] net/iavf: fix abnormal disable HW interrupt
> >
> > For command VIRTCHNL_OP_REQUEST_QUEUES, polling access to the
> admin
> > queue has the issue of access overruns after disabling interrupt. That
> > results in FW disabling HW interrupt for protection purposes.
> >
> > The updates/changes in this patch:
> > 1. Remove the polling admin queue processing and use the generic
> > interrupt processing instead.
> > 2. Release redundant queue resource before stopping processing
> > interrupt events.
> >
> > Fixes: 22b123a36d07 ("net/avf: initialize PMD")
> > Fixes: ef807926e148 ("net/iavf: support requesting additional queues
> > from
> > PF")
> > Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel
> > message")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> > ---
> >  drivers/net/iavf/iavf_ethdev.c | 25 +++++++++---------
> > drivers/net/iavf/iavf_vchnl.c  | 48 +++++++---------------------------
> >  2 files changed, 23 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/net/iavf/iavf_ethdev.c
> > b/drivers/net/iavf/iavf_ethdev.c index e6cf897293..ba5c88a1ec 100644
> > --- a/drivers/net/iavf/iavf_ethdev.c
> > +++ b/drivers/net/iavf/iavf_ethdev.c
> > @@ -2756,6 +2756,19 @@ iavf_dev_close(struct rte_eth_dev *dev)
> >  	}
> >
> >  	ret = iavf_dev_stop(dev);
> > +
> > +	/*
> > +	 * Release redundant queue resource when close the dev
> > +	 * so that other vfs can re-use the queues.
> > +	 */
> > +	if (vf->lv_enabled) {
> > +		ret = iavf_request_queues(dev,
> > IAVF_MAX_NUM_QUEUES_DFLT);
> > +		if (ret)
> > +			PMD_DRV_LOG(ERR, "Reset the num of queues
> > failed");
> > +
> > +		vf->max_rss_qregion = IAVF_MAX_NUM_QUEUES_DFLT;
> > +	}
> > +
> >  	adapter->closed = true;
> >
> >  	/* free iAVF security device context all related resources */ @@ -
> > 2772,18 +2785,6 @@ iavf_dev_close(struct rte_eth_dev *dev)
> >  	if (vf->promisc_unicast_enabled || vf->promisc_multicast_enabled)
> >  		iavf_config_promisc(adapter, false, false);
> >
> > -	/*
> > -	 * Release redundant queue resource when close the dev
> > -	 * so that other vfs can re-use the queues.
> > -	 */
> > -	if (vf->lv_enabled) {
> > -		ret = iavf_request_queues(dev,
> > IAVF_MAX_NUM_QUEUES_DFLT);
> > -		if (ret)
> > -			PMD_DRV_LOG(ERR, "Reset the num of queues
> > failed");
> > -
> > -		vf->max_rss_qregion = IAVF_MAX_NUM_QUEUES_DFLT;
> > -	}
> > -
> >  	iavf_shutdown_adminq(hw);
> >  	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_WB_ON_ITR)
> {
> >  		/* disable uio intr before callback unregister */ diff --git
> > a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c index
> > 8cc5377bcf..579c0d0d70 100644
> > --- a/drivers/net/iavf/iavf_vchnl.c
> > +++ b/drivers/net/iavf/iavf_vchnl.c
> > @@ -323,6 +323,7 @@ iavf_execute_vf_cmd(struct iavf_adapter *adapter,
> > struct iavf_cmd_info *args,
> >
> >  	switch (args->ops) {
> >  	case VIRTCHNL_OP_RESET_VF:
> > +	case VIRTCHNL_OP_REQUEST_QUEUES:
> >  		/*no need to wait for response */
> >  		_clear_cmd(vf);
> >  		break;
> 
> Have you tested the "large VF" case ( > 16 queue) with this patch.
> I assume event VIRTCHNL_EVENT_RESET_IMPENDING need to be handled
> here then to trigger a device reset.
> But I didn't see related part in your patch.

The processing related to the "big VF" is moved before "adapter->closed = true;"
 to make sure it can be handled correctly by the interrupt function.

RE: [PATCH] net/iavf: fix abnormal disable HW interrupt

[Zhang, Qi Z]

> -----Original Message-----
> From: Ye, MingjinX <mingjinx.ye@intel.com>
> Sent: Friday, June 9, 2023 11:20 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> <yidingx.zhou@intel.com>; stable@dpdk.org; Wu, Jingjing
> <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Subject: RE: [PATCH] net/iavf: fix abnormal disable HW interrupt
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Sent: 2023年6月9日 9:48
> > To: Ye, MingjinX <mingjinx.ye@intel.com>; dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> > <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>;
> > stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>
> > Subject: RE: [PATCH] net/iavf: fix abnormal disable HW interrupt
> >
> >
> >
> > > -----Original Message-----
> > > From: Mingjin Ye <mingjinx.ye@intel.com>
> > > Sent: Thursday, June 8, 2023 2:23 PM
> > > To: dev@dpdk.org
> > > Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> > > <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>;
> > > stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > > <beilei.xing@intel.com>
> > > Subject: [PATCH] net/iavf: fix abnormal disable HW interrupt
> > >
> > > For command VIRTCHNL_OP_REQUEST_QUEUES, polling access to the
> > admin
> > > queue has the issue of access overruns after disabling interrupt.
> > > That results in FW disabling HW interrupt for protection purposes.
> > >
> > > The updates/changes in this patch:
> > > 1. Remove the polling admin queue processing and use the generic
> > > interrupt processing instead.
> > > 2. Release redundant queue resource before stopping processing
> > > interrupt events.
> > >
> > > Fixes: 22b123a36d07 ("net/avf: initialize PMD")
> > > Fixes: ef807926e148 ("net/iavf: support requesting additional queues
> > > from
> > > PF")
> > > Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel
> > > message")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> > > ---
> > >  drivers/net/iavf/iavf_ethdev.c | 25 +++++++++---------
> > > drivers/net/iavf/iavf_vchnl.c  | 48
> > > +++++++---------------------------
> > >  2 files changed, 23 insertions(+), 50 deletions(-)
> > >
> > > diff --git a/drivers/net/iavf/iavf_ethdev.c
> > > b/drivers/net/iavf/iavf_ethdev.c index e6cf897293..ba5c88a1ec 100644
> > > --- a/drivers/net/iavf/iavf_ethdev.c
> > > +++ b/drivers/net/iavf/iavf_ethdev.c
> > > @@ -2756,6 +2756,19 @@ iavf_dev_close(struct rte_eth_dev *dev)
> > >  	}
> > >
> > >  	ret = iavf_dev_stop(dev);
> > > +
> > > +	/*
> > > +	 * Release redundant queue resource when close the dev
> > > +	 * so that other vfs can re-use the queues.
> > > +	 */
> > > +	if (vf->lv_enabled) {
> > > +		ret = iavf_request_queues(dev,
> > > IAVF_MAX_NUM_QUEUES_DFLT);
> > > +		if (ret)
> > > +			PMD_DRV_LOG(ERR, "Reset the num of queues
> > > failed");
> > > +
> > > +		vf->max_rss_qregion = IAVF_MAX_NUM_QUEUES_DFLT;
> > > +	}
> > > +
> > >  	adapter->closed = true;
> > >
> > >  	/* free iAVF security device context all related resources */ @@ -
> > > 2772,18 +2785,6 @@ iavf_dev_close(struct rte_eth_dev *dev)
> > >  	if (vf->promisc_unicast_enabled || vf->promisc_multicast_enabled)
> > >  		iavf_config_promisc(adapter, false, false);
> > >
> > > -	/*
> > > -	 * Release redundant queue resource when close the dev
> > > -	 * so that other vfs can re-use the queues.
> > > -	 */
> > > -	if (vf->lv_enabled) {
> > > -		ret = iavf_request_queues(dev,
> > > IAVF_MAX_NUM_QUEUES_DFLT);
> > > -		if (ret)
> > > -			PMD_DRV_LOG(ERR, "Reset the num of queues
> > > failed");
> > > -
> > > -		vf->max_rss_qregion = IAVF_MAX_NUM_QUEUES_DFLT;
> > > -	}
> > > -
> > >  	iavf_shutdown_adminq(hw);
> > >  	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_WB_ON_ITR)
> > {
> > >  		/* disable uio intr before callback unregister */ diff --git
> > > a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
> > > index
> > > 8cc5377bcf..579c0d0d70 100644
> > > --- a/drivers/net/iavf/iavf_vchnl.c
> > > +++ b/drivers/net/iavf/iavf_vchnl.c
> > > @@ -323,6 +323,7 @@ iavf_execute_vf_cmd(struct iavf_adapter
> > > *adapter, struct iavf_cmd_info *args,
> > >
> > >  	switch (args->ops) {
> > >  	case VIRTCHNL_OP_RESET_VF:
> > > +	case VIRTCHNL_OP_REQUEST_QUEUES:
> > >  		/*no need to wait for response */
> > >  		_clear_cmd(vf);
> > >  		break;
> >
> > Have you tested the "large VF" case ( > 16 queue) with this patch.

You didn't answer my question.

> > I assume event VIRTCHNL_EVENT_RESET_IMPENDING need to be handled
> here
> > then to trigger a device reset.
> > But I didn't see related part in your patch.
> 
> The processing related to the "big VF" is moved before "adapter->closed =
> true;"
>  to make sure it can be handled correctly by the interrupt function.
> 

I'm not asking what happened during dev_close but the init scenario in the large VF case, 

dev_configure-> iavf_queues_req_reset-> iavf_request_queues

this happens at the situation that is no interrupt is enabled and it need to handle event VIRTCHNL_EVENT_RESET_IMPENDING

RE: [PATCH] net/iavf: fix abnormal disable HW interrupt

[Zhang, Qi Z]

> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Monday, June 12, 2023 8:40 PM
> To: Ye, MingjinX <mingjinx.ye@intel.com>; dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> <yidingx.zhou@intel.com>; stable@dpdk.org; Wu, Jingjing
> <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Subject: RE: [PATCH] net/iavf: fix abnormal disable HW interrupt
> 
> 
> 
> > -----Original Message-----
> > From: Ye, MingjinX <mingjinx.ye@intel.com>
> > Sent: Friday, June 9, 2023 11:20 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> > <yidingx.zhou@intel.com>; stable@dpdk.org; Wu, Jingjing
> > <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> > Subject: RE: [PATCH] net/iavf: fix abnormal disable HW interrupt
> >
> >
> >
> > > -----Original Message-----
> > > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Sent: 2023年6月9日 9:48
> > > To: Ye, MingjinX <mingjinx.ye@intel.com>; dev@dpdk.org
> > > Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> > > <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>;
> > > stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > > <beilei.xing@intel.com>
> > > Subject: RE: [PATCH] net/iavf: fix abnormal disable HW interrupt
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Mingjin Ye <mingjinx.ye@intel.com>
> > > > Sent: Thursday, June 8, 2023 2:23 PM
> > > > To: dev@dpdk.org
> > > > Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> > > > <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>;
> > > > stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing,
> > > > Beilei <beilei.xing@intel.com>
> > > > Subject: [PATCH] net/iavf: fix abnormal disable HW interrupt
> > > >
> > > > For command VIRTCHNL_OP_REQUEST_QUEUES, polling access to the
> > > admin
> > > > queue has the issue of access overruns after disabling interrupt.
> > > > That results in FW disabling HW interrupt for protection purposes.
> > > >
> > > > The updates/changes in this patch:
> > > > 1. Remove the polling admin queue processing and use the generic
> > > > interrupt processing instead.
> > > > 2. Release redundant queue resource before stopping processing
> > > > interrupt events.
> > > >
> > > > Fixes: 22b123a36d07 ("net/avf: initialize PMD")
> > > > Fixes: ef807926e148 ("net/iavf: support requesting additional
> > > > queues from
> > > > PF")
> > > > Fixes: 84108425054a ("net/iavf: support asynchronous virtual
> > > > channel
> > > > message")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> > > > ---
> > > >  drivers/net/iavf/iavf_ethdev.c | 25 +++++++++---------
> > > > drivers/net/iavf/iavf_vchnl.c  | 48
> > > > +++++++---------------------------
> > > >  2 files changed, 23 insertions(+), 50 deletions(-)
> > > >
> > > > diff --git a/drivers/net/iavf/iavf_ethdev.c
> > > > b/drivers/net/iavf/iavf_ethdev.c index e6cf897293..ba5c88a1ec
> > > > 100644
> > > > --- a/drivers/net/iavf/iavf_ethdev.c
> > > > +++ b/drivers/net/iavf/iavf_ethdev.c
> > > > @@ -2756,6 +2756,19 @@ iavf_dev_close(struct rte_eth_dev *dev)
> > > >  	}
> > > >
> > > >  	ret = iavf_dev_stop(dev);
> > > > +
> > > > +	/*
> > > > +	 * Release redundant queue resource when close the dev
> > > > +	 * so that other vfs can re-use the queues.
> > > > +	 */
> > > > +	if (vf->lv_enabled) {
> > > > +		ret = iavf_request_queues(dev,
> > > > IAVF_MAX_NUM_QUEUES_DFLT);
> > > > +		if (ret)
> > > > +			PMD_DRV_LOG(ERR, "Reset the num of queues
> > > > failed");
> > > > +
> > > > +		vf->max_rss_qregion = IAVF_MAX_NUM_QUEUES_DFLT;
> > > > +	}
> > > > +
> > > >  	adapter->closed = true;
> > > >
> > > >  	/* free iAVF security device context all related resources */ @@
> > > > -
> > > > 2772,18 +2785,6 @@ iavf_dev_close(struct rte_eth_dev *dev)
> > > >  	if (vf->promisc_unicast_enabled || vf->promisc_multicast_enabled)
> > > >  		iavf_config_promisc(adapter, false, false);
> > > >
> > > > -	/*
> > > > -	 * Release redundant queue resource when close the dev
> > > > -	 * so that other vfs can re-use the queues.
> > > > -	 */
> > > > -	if (vf->lv_enabled) {
> > > > -		ret = iavf_request_queues(dev,
> > > > IAVF_MAX_NUM_QUEUES_DFLT);
> > > > -		if (ret)
> > > > -			PMD_DRV_LOG(ERR, "Reset the num of queues
> > > > failed");
> > > > -
> > > > -		vf->max_rss_qregion = IAVF_MAX_NUM_QUEUES_DFLT;
> > > > -	}
> > > > -
> > > >  	iavf_shutdown_adminq(hw);
> > > >  	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_WB_ON_ITR)
> > > {
> > > >  		/* disable uio intr before callback unregister */ diff --git
> > > > a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
> > > > index
> > > > 8cc5377bcf..579c0d0d70 100644
> > > > --- a/drivers/net/iavf/iavf_vchnl.c
> > > > +++ b/drivers/net/iavf/iavf_vchnl.c
> > > > @@ -323,6 +323,7 @@ iavf_execute_vf_cmd(struct iavf_adapter
> > > > *adapter, struct iavf_cmd_info *args,
> > > >
> > > >  	switch (args->ops) {
> > > >  	case VIRTCHNL_OP_RESET_VF:
> > > > +	case VIRTCHNL_OP_REQUEST_QUEUES:
> > > >  		/*no need to wait for response */
> > > >  		_clear_cmd(vf);
> > > >  		break;
> > >
> > > Have you tested the "large VF" case ( > 16 queue) with this patch.
> 
> You didn't answer my question.
> 
> > > I assume event VIRTCHNL_EVENT_RESET_IMPENDING need to be handled
> > here
> > > then to trigger a device reset.
> > > But I didn't see related part in your patch.
> >
> > The processing related to the "big VF" is moved before
> > "adapter->closed = true;"
> >  to make sure it can be handled correctly by the interrupt function.
> >
> 
> I'm not asking what happened during dev_close but the init scenario in the
> large VF case,
> 
> dev_configure-> iavf_queues_req_reset-> iavf_request_queues
> 
> this happens at the situation that is no interrupt is enabled and it need to
> handle event VIRTCHNL_EVENT_RESET_IMPENDING
> 

This is not correct, as the interrupt has already been enabled in dev_init.

But I still have question for other part of the implementation, will raise it in another thread.

[PATCH v2] net/iavf: fix abnormal disable HW interrupt

[Mingjin Ye]

For command VIRTCHNL_OP_REQUEST_QUEUES, polling access to the admin
queue has the issue of access overruns after disabling interrupt. That
results in FW disabling HW interrupt for protection purposes.

The updates/changes in this patch:
1. Remove the polling admin queue processing and use the generic interrupt
processing instead.
2. Release redundant queue resource before stopping processing interrupt
events.

Fixes: 22b123a36d07 ("net/avf: initialize PMD")
Fixes: ef807926e148 ("net/iavf: support requesting additional queues from PF")
Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel message")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
v2: Fix git version conflict.
---
 drivers/net/iavf/iavf_ethdev.c | 25 +++++++++---------
 drivers/net/iavf/iavf_vchnl.c  | 48 +++++++---------------------------
 2 files changed, 23 insertions(+), 50 deletions(-)

diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
index b72dbc8ceb..00b963128b 100644
--- a/drivers/net/iavf/iavf_ethdev.c
+++ b/drivers/net/iavf/iavf_ethdev.c
@@ -2757,6 +2757,19 @@ iavf_dev_close(struct rte_eth_dev *dev)
 	}
 
 	ret = iavf_dev_stop(dev);
+
+	/*
+	 * Release redundant queue resource when close the dev
+	 * so that other vfs can re-use the queues.
+	 */
+	if (vf->lv_enabled) {
+		ret = iavf_request_queues(dev, IAVF_MAX_NUM_QUEUES_DFLT);
+		if (ret)
+			PMD_DRV_LOG(ERR, "Reset the num of queues failed");
+
+		vf->max_rss_qregion = IAVF_MAX_NUM_QUEUES_DFLT;
+	}
+
 	adapter->closed = true;
 
 	/* free iAVF security device context all related resources */
@@ -2773,18 +2786,6 @@ iavf_dev_close(struct rte_eth_dev *dev)
 	if (vf->promisc_unicast_enabled || vf->promisc_multicast_enabled)
 		iavf_config_promisc(adapter, false, false);
 
-	/*
-	 * Release redundant queue resource when close the dev
-	 * so that other vfs can re-use the queues.
-	 */
-	if (vf->lv_enabled) {
-		ret = iavf_request_queues(dev, IAVF_MAX_NUM_QUEUES_DFLT);
-		if (ret)
-			PMD_DRV_LOG(ERR, "Reset the num of queues failed");
-
-		vf->max_rss_qregion = IAVF_MAX_NUM_QUEUES_DFLT;
-	}
-
 	iavf_shutdown_adminq(hw);
 	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_WB_ON_ITR) {
 		/* disable uio intr before callback unregister */
diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index 07eb358824..524732f67d 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -324,6 +324,7 @@ iavf_execute_vf_cmd(struct iavf_adapter *adapter, struct iavf_cmd_info *args,
 
 	switch (args->ops) {
 	case VIRTCHNL_OP_RESET_VF:
+	case VIRTCHNL_OP_REQUEST_QUEUES:
 		/*no need to wait for response */
 		_clear_cmd(vf);
 		break;
@@ -347,33 +348,6 @@ iavf_execute_vf_cmd(struct iavf_adapter *adapter, struct iavf_cmd_info *args,
 		}
 		_clear_cmd(vf);
 		break;
-	case VIRTCHNL_OP_REQUEST_QUEUES:
-		/*
-		 * ignore async reply, only wait for system message,
-		 * vf_reset = true if get VIRTCHNL_EVENT_RESET_IMPENDING,
-		 * if not, means request queues failed.
-		 */
-		do {
-			result = iavf_read_msg_from_pf(adapter, args->out_size,
-						   args->out_buffer);
-			if (result == IAVF_MSG_SYS && vf->vf_reset) {
-				break;
-			} else if (result == IAVF_MSG_CMD ||
-				result == IAVF_MSG_ERR) {
-				err = -1;
-				break;
-			}
-			iavf_msec_delay(ASQ_DELAY_MS);
-			/* If don't read msg or read sys event, continue */
-		} while (i++ < MAX_TRY_TIMES);
-		if (i >= MAX_TRY_TIMES ||
-			vf->cmd_retval != VIRTCHNL_STATUS_SUCCESS) {
-			err = -1;
-			PMD_DRV_LOG(ERR, "No response or return failure (%d)"
-				    " for cmd %d", vf->cmd_retval, args->ops);
-		}
-		_clear_cmd(vf);
-		break;
 	default:
 		if (rte_thread_is_intr()) {
 			/* For virtchnl ops were executed in eal_intr_thread,
@@ -2082,11 +2056,11 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
 	struct iavf_adapter *adapter =
 		IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 	struct iavf_info *vf =  IAVF_DEV_PRIVATE_TO_VF(adapter);
-	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct virtchnl_vf_res_request vfres;
 	struct iavf_cmd_info args;
 	uint16_t num_queue_pairs;
 	int err;
+	int i = 0;
 
 	if (!(vf->vf_res->vf_cap_flags &
 		VIRTCHNL_VF_OFFLOAD_REQ_QUEUES)) {
@@ -2107,16 +2081,7 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
 	args.out_size = IAVF_AQ_BUF_SZ;
 
 	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_WB_ON_ITR) {
-		/* disable interrupt to avoid the admin queue message to be read
-		 * before iavf_read_msg_from_pf.
-		 *
-		 * don't disable interrupt handler until ready to execute vf cmd.
-		 */
-		rte_spinlock_lock(&vf->aq_lock);
-		rte_intr_disable(pci_dev->intr_handle);
-		err = iavf_execute_vf_cmd(adapter, &args, 0);
-		rte_intr_enable(pci_dev->intr_handle);
-		rte_spinlock_unlock(&vf->aq_lock);
+		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	} else {
 		rte_eal_alarm_cancel(iavf_dev_alarm_handler, dev);
 		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
@@ -2129,6 +2094,13 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
 		return err;
 	}
 
+	/* wait for interrupt notification vf is resetting */
+	while (i++ < MAX_TRY_TIMES) {
+		if (vf->vf_reset)
+			break;
+		iavf_msec_delay(ASQ_DELAY_MS);
+	}
+
 	/* request queues succeeded, vf is resetting */
 	if (vf->vf_reset) {
 		PMD_DRV_LOG(INFO, "vf is resetting");
-- 
2.25.1

RE: [PATCH v2] net/iavf: fix abnormal disable HW interrupt

[Zhang, Qi Z]

> -----Original Message-----
> From: Mingjin Ye <mingjinx.ye@intel.com>
> Sent: Wednesday, June 14, 2023 5:53 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>;
> stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>
> Subject: [PATCH v2] net/iavf: fix abnormal disable HW interrupt
> 
> For command VIRTCHNL_OP_REQUEST_QUEUES, polling access to the admin
> queue has the issue of access overruns after disabling interrupt. That results
> in FW disabling HW interrupt for protection purposes.
> 
> The updates/changes in this patch:
> 1. Remove the polling admin queue processing and use the generic interrupt
> processing instead.
> 2. Release redundant queue resource before stopping processing interrupt
> events.
> 
> Fixes: 22b123a36d07 ("net/avf: initialize PMD")
> Fixes: ef807926e148 ("net/iavf: support requesting additional queues from
> PF")
> Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel
> message")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

Acked-by: Qi Zhang <qi.z.zhang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi

[PATCH] net/iavf: fix abnormal disable HW interrupt

[Mingjin Ye]

For command VIRTCHNL_OP_REQUEST_QUEUES, polling access to the admin
queue has the issue of access overruns after disabling interrupt. That
results in FW disabling HW interrupt for protection purposes.

This patch fixes the issue by removing the polling admin queue processing
and replacing it with a generic interrupt processing.

Fixes: 22b123a36d07 ("net/avf: initialize PMD")
Fixes: ef807926e148 ("net/iavf: support requesting additional queues from PF")
Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel message")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
 drivers/net/iavf/iavf_vchnl.c | 48 ++++++++---------------------------
 1 file changed, 10 insertions(+), 38 deletions(-)

diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index 8cc5377bcf..579c0d0d70 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -323,6 +323,7 @@ iavf_execute_vf_cmd(struct iavf_adapter *adapter, struct iavf_cmd_info *args,
 
 	switch (args->ops) {
 	case VIRTCHNL_OP_RESET_VF:
+	case VIRTCHNL_OP_REQUEST_QUEUES:
 		/*no need to wait for response */
 		_clear_cmd(vf);
 		break;
@@ -346,33 +347,6 @@ iavf_execute_vf_cmd(struct iavf_adapter *adapter, struct iavf_cmd_info *args,
 		}
 		_clear_cmd(vf);
 		break;
-	case VIRTCHNL_OP_REQUEST_QUEUES:
-		/*
-		 * ignore async reply, only wait for system message,
-		 * vf_reset = true if get VIRTCHNL_EVENT_RESET_IMPENDING,
-		 * if not, means request queues failed.
-		 */
-		do {
-			result = iavf_read_msg_from_pf(adapter, args->out_size,
-						   args->out_buffer);
-			if (result == IAVF_MSG_SYS && vf->vf_reset) {
-				break;
-			} else if (result == IAVF_MSG_CMD ||
-				result == IAVF_MSG_ERR) {
-				err = -1;
-				break;
-			}
-			iavf_msec_delay(ASQ_DELAY_MS);
-			/* If don't read msg or read sys event, continue */
-		} while (i++ < MAX_TRY_TIMES);
-		if (i >= MAX_TRY_TIMES ||
-			vf->cmd_retval != VIRTCHNL_STATUS_SUCCESS) {
-			err = -1;
-			PMD_DRV_LOG(ERR, "No response or return failure (%d)"
-				    " for cmd %d", vf->cmd_retval, args->ops);
-		}
-		_clear_cmd(vf);
-		break;
 	default:
 		/* For other virtchnl ops in running time,
 		 * wait for the cmd done flag.
@@ -2055,11 +2029,11 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
 	struct iavf_adapter *adapter =
 		IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 	struct iavf_info *vf =  IAVF_DEV_PRIVATE_TO_VF(adapter);
-	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct virtchnl_vf_res_request vfres;
 	struct iavf_cmd_info args;
 	uint16_t num_queue_pairs;
 	int err;
+	int i = 0;
 
 	if (!(vf->vf_res->vf_cap_flags &
 		VIRTCHNL_VF_OFFLOAD_REQ_QUEUES)) {
@@ -2080,16 +2054,7 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
 	args.out_size = IAVF_AQ_BUF_SZ;
 
 	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_WB_ON_ITR) {
-		/* disable interrupt to avoid the admin queue message to be read
-		 * before iavf_read_msg_from_pf.
-		 *
-		 * don't disable interrupt handler until ready to execute vf cmd.
-		 */
-		rte_spinlock_lock(&vf->aq_lock);
-		rte_intr_disable(pci_dev->intr_handle);
-		err = iavf_execute_vf_cmd(adapter, &args, 0);
-		rte_intr_enable(pci_dev->intr_handle);
-		rte_spinlock_unlock(&vf->aq_lock);
+		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	} else {
 		rte_eal_alarm_cancel(iavf_dev_alarm_handler, dev);
 		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
@@ -2102,6 +2067,13 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
 		return err;
 	}
 
+	/* wait for interrupt notification vf is resetting */
+	while (i++ < MAX_TRY_TIMES) {
+		if (vf->vf_reset)
+			break;
+		iavf_msec_delay(ASQ_DELAY_MS);
+	}
+
 	/* request queues succeeded, vf is resetting */
 	if (vf->vf_reset) {
 		PMD_DRV_LOG(INFO, "vf is resetting");
-- 
2.25.1