patches for DPDK stable branches
 help / color / mirror / Atom feed
* [PATCH] net/iavf: add lock for vf commands
@ 2022-12-19 23:23 Mike Pattrick
  2022-12-20 15:02 ` David Marchand
  0 siblings, 1 reply; 8+ messages in thread
From: Mike Pattrick @ 2022-12-19 23:23 UTC (permalink / raw)
  To: dev; +Cc: yidingx.zhou, ktraynor, Mike Pattrick, stable

iavf admin queue commands aren't thread-safe. Bugs surrounding this
issue can manifest in a variety of ways but frequently pend_cmd is
over written. Simultaneously executing commands can result in a
misconfigured device or DPDK sleeping in a thread for 2 second.

Despite this limitation, vf commands may be executed from both
iavf_dev_alarm_handler() in a control thread and the applications main
thread. This is trivial to simulate in the testpmd application by
creating a bond of vf's in active backup mode, and then turning the
bond off and then on again repeatedly.

Previously [1] was proposed as a potential solution, but this commit
has been reverted. I propose adding locks until a more complete
solution is available.

[1] commit cb5c1b91f76f ("net/iavf: add thread for event callbacks")

Fixes: 48de41ca11f0 ("net/avf: enable link status update")
Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel message")
Cc: stable@dpdk.org

Signed-off-by: Mike Pattrick <mkp@redhat.com>
---
 drivers/net/iavf/iavf.h       |  1 +
 drivers/net/iavf/iavf_vchnl.c | 76 +++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+)

diff --git a/drivers/net/iavf/iavf.h b/drivers/net/iavf/iavf.h
index 1edebab8dc..aa18650ffa 100644
--- a/drivers/net/iavf/iavf.h
+++ b/drivers/net/iavf/iavf.h
@@ -262,6 +262,7 @@ struct iavf_info {
 	struct iavf_qv_map *qv_map; /* queue vector mapping */
 	struct iavf_flow_list flow_list;
 	rte_spinlock_t flow_ops_lock;
+	rte_spinlock_t aq_lock;
 	struct iavf_parser_list rss_parser_list;
 	struct iavf_parser_list dist_parser_list;
 	struct iavf_parser_list ipsec_crypto_parser_list;
diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index f92daf97f2..ca435dee2e 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -554,7 +554,9 @@ iavf_enable_vlan_strip(struct iavf_adapter *adapter)
 	args.in_args_size = 0;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (ret)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " OP_ENABLE_VLAN_STRIPPING");
@@ -575,7 +577,9 @@ iavf_disable_vlan_strip(struct iavf_adapter *adapter)
 	args.in_args_size = 0;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (ret)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " OP_DISABLE_VLAN_STRIPPING");
@@ -604,7 +608,9 @@ iavf_check_api_version(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_INIT_LOG(ERR, "Fail to execute command of OP_VERSION");
 		return err;
@@ -665,7 +671,9 @@ iavf_get_vf_resource(struct iavf_adapter *adapter)
 	args.in_args = (uint8_t *)&caps;
 	args.in_args_size = sizeof(caps);
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 
 	if (err) {
 		PMD_DRV_LOG(ERR,
@@ -710,7 +718,9 @@ iavf_get_supported_rxdid(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (ret) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_GET_SUPPORTED_RXDIDS");
@@ -754,7 +764,9 @@ iavf_config_vlan_strip_v2(struct iavf_adapter *adapter, bool enable)
 	args.in_args_size = sizeof(vlan_strip);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (ret)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    enable ? "VIRTCHNL_OP_ENABLE_VLAN_STRIPPING_V2" :
@@ -794,7 +806,9 @@ iavf_config_vlan_insert_v2(struct iavf_adapter *adapter, bool enable)
 	args.in_args_size = sizeof(vlan_insert);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (ret)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    enable ? "VIRTCHNL_OP_ENABLE_VLAN_INSERTION_V2" :
@@ -837,7 +851,9 @@ iavf_add_del_vlan_v2(struct iavf_adapter *adapter, uint16_t vlanid, bool add)
 	args.in_args_size = sizeof(vlan_filter);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    add ? "OP_ADD_VLAN_V2" :  "OP_DEL_VLAN_V2");
@@ -858,7 +874,9 @@ iavf_get_vlan_offload_caps_v2(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (ret) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of VIRTCHNL_OP_GET_OFFLOAD_VLAN_V2_CAPS");
@@ -889,7 +907,9 @@ iavf_enable_queues(struct iavf_adapter *adapter)
 	args.in_args_size = sizeof(queue_select);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_ENABLE_QUEUES");
@@ -917,7 +937,9 @@ iavf_disable_queues(struct iavf_adapter *adapter)
 	args.in_args_size = sizeof(queue_select);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_DISABLE_QUEUES");
@@ -953,7 +975,9 @@ iavf_switch_queue(struct iavf_adapter *adapter, uint16_t qid,
 	args.in_args_size = sizeof(queue_select);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of %s",
 			    on ? "OP_ENABLE_QUEUES" : "OP_DISABLE_QUEUES");
@@ -995,7 +1019,9 @@ iavf_enable_queues_lv(struct iavf_adapter *adapter)
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_ENABLE_QUEUES_V2");
@@ -1039,7 +1065,9 @@ iavf_disable_queues_lv(struct iavf_adapter *adapter)
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_DISABLE_QUEUES_V2");
@@ -1085,7 +1113,9 @@ iavf_switch_queue_lv(struct iavf_adapter *adapter, uint16_t qid,
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of %s",
 			    on ? "OP_ENABLE_QUEUES_V2" : "OP_DISABLE_QUEUES_V2");
@@ -1117,7 +1147,9 @@ iavf_configure_rss_lut(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_CONFIG_RSS_LUT");
@@ -1149,7 +1181,9 @@ iavf_configure_rss_key(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_CONFIG_RSS_KEY");
@@ -1247,7 +1281,9 @@ iavf_configure_queues(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " VIRTCHNL_OP_CONFIG_VSI_QUEUES");
@@ -1288,7 +1324,9 @@ iavf_config_irq_map(struct iavf_adapter *adapter)
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command OP_CONFIG_IRQ_MAP");
 
@@ -1329,7 +1367,9 @@ iavf_config_irq_map_lv(struct iavf_adapter *adapter, uint16_t num,
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command OP_MAP_QUEUE_VECTOR");
 
@@ -1389,7 +1429,9 @@ iavf_add_del_all_mac_addr(struct iavf_adapter *adapter, bool add)
 		args.in_args_size = len;
 		args.out_buffer = vf->aq_resp;
 		args.out_size = IAVF_AQ_BUF_SZ;
+		rte_spinlock_lock(&vf->aq_lock);
 		err = iavf_execute_vf_cmd(adapter, &args, 0);
+		rte_spinlock_unlock(&vf->aq_lock);
 		if (err)
 			PMD_DRV_LOG(ERR, "fail to execute command %s",
 				    add ? "OP_ADD_ETHER_ADDRESS" :
@@ -1419,7 +1461,9 @@ iavf_query_stats(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_GET_STATS");
 		*pstats = NULL;
@@ -1457,7 +1501,9 @@ iavf_config_promisc(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 
 	if (err) {
 		PMD_DRV_LOG(ERR,
@@ -1500,7 +1546,9 @@ iavf_add_del_eth_addr(struct iavf_adapter *adapter, struct rte_ether_addr *addr,
 	args.in_args_size = sizeof(cmd_buffer);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    add ? "OP_ADD_ETH_ADDR" :  "OP_DEL_ETH_ADDR");
@@ -1527,7 +1575,9 @@ iavf_add_del_vlan(struct iavf_adapter *adapter, uint16_t vlanid, bool add)
 	args.in_args_size = sizeof(cmd_buffer);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    add ? "OP_ADD_VLAN" :  "OP_DEL_VLAN");
@@ -1554,7 +1604,9 @@ iavf_fdir_add(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_ADD_FDIR_FILTER");
 		return err;
@@ -1614,7 +1666,9 @@ iavf_fdir_del(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_DEL_FDIR_FILTER");
 		return err;
@@ -1661,7 +1715,9 @@ iavf_fdir_check(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to check flow director rule");
 		return err;
@@ -1838,7 +1894,9 @@ iavf_add_del_rss_cfg(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of %s",
@@ -1861,7 +1919,9 @@ iavf_get_hena_caps(struct iavf_adapter *adapter, uint64_t *caps)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_GET_RSS_HENA_CAPS");
@@ -1887,7 +1947,9 @@ iavf_set_hena(struct iavf_adapter *adapter, uint64_t hena)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_SET_RSS_HENA");
@@ -1908,7 +1970,9 @@ iavf_get_qos_cap(struct iavf_adapter *adapter)
 	args.in_args_size = 0;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 
 	if (err) {
 		PMD_DRV_LOG(ERR,
@@ -1941,7 +2005,9 @@ int iavf_set_q_tc_map(struct rte_eth_dev *dev,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " VIRTCHNL_OP_CONFIG_TC_MAP");
@@ -2009,7 +2075,9 @@ iavf_add_del_mc_addr_list(struct iavf_adapter *adapter,
 		i * sizeof(struct virtchnl_ether_addr);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
@@ -2054,12 +2122,16 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
 		/* disable interrupt to avoid the admin queue message to be read
 		 * before iavf_read_msg_from_pf.
 		 */
+		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);
 	} else {
 		rte_eal_alarm_cancel(iavf_dev_alarm_handler, dev);
+		rte_spinlock_lock(&vf->aq_lock);
 		err = iavf_execute_vf_cmd(adapter, &args, 0);
+		rte_spinlock_unlock(&vf->aq_lock);
 		rte_eal_alarm_set(IAVF_ALARM_INTERVAL,
 				  iavf_dev_alarm_handler, dev);
 	}
@@ -2098,7 +2170,9 @@ iavf_get_max_rss_queue_region(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to execute command of VIRTCHNL_OP_GET_MAX_RSS_QREGION");
 		return err;
@@ -2129,7 +2203,9 @@ iavf_ipsec_crypto_request(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 1);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 				"OP_INLINE_IPSEC_CRYPTO");
-- 
2.31.1


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] net/iavf: add lock for vf commands
  2022-12-19 23:23 [PATCH] net/iavf: add lock for vf commands Mike Pattrick
@ 2022-12-20 15:02 ` David Marchand
  2022-12-20 15:54   ` [PATCH v2] " Mike Pattrick
  0 siblings, 1 reply; 8+ messages in thread
From: David Marchand @ 2022-12-20 15:02 UTC (permalink / raw)
  To: Mike Pattrick
  Cc: dev, yidingx.zhou, ktraynor, stable, Qi Zhang, Ferruh Yigit,
	Thomas Monjalon

Hello Mike,

On Tue, Dec 20, 2022 at 12:24 AM Mike Pattrick <mkp@redhat.com> wrote:
>
> iavf admin queue commands aren't thread-safe. Bugs surrounding this
> issue can manifest in a variety of ways but frequently pend_cmd is
> over written. Simultaneously executing commands can result in a
> misconfigured device or DPDK sleeping in a thread for 2 second.
>
> Despite this limitation, vf commands may be executed from both
> iavf_dev_alarm_handler() in a control thread and the applications main
> thread. This is trivial to simulate in the testpmd application by
> creating a bond of vf's in active backup mode, and then turning the
> bond off and then on again repeatedly.
>
> Previously [1] was proposed as a potential solution, but this commit
> has been reverted. I propose adding locks until a more complete
> solution is available.

- Hum, this commit is still in the main branch, and I did reproduce
the issue with a E810 nic.
[1] is not enough for the race on pend_cmd (probably some alarm is
still racing with the control thread).

So I think a fix is needed regardless of [1] fate.

- Your patch does make the race disappear, but then, about using a
lock on pend_cmd, I wonder what the point to set it atomically is.

- The same issue probably affects the recently copy/pasted^Wintroduced
driver idpf.


>
> [1] commit cb5c1b91f76f ("net/iavf: add thread for event callbacks")
>
> Fixes: 48de41ca11f0 ("net/avf: enable link status update")
> Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel message")
> Cc: stable@dpdk.org
>
> Signed-off-by: Mike Pattrick <mkp@redhat.com>
> ---

[snip]

Doing a git grep -nC 1 iavf_execute_vf_cmd, I see some unprotected calls.
This is probably a rebase issue if you tested on 21.11.



-- 
David Marchand


^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH v2] net/iavf: add lock for vf commands
  2022-12-20 15:02 ` David Marchand
@ 2022-12-20 15:54   ` Mike Pattrick
  2022-12-26  6:01     ` Zhang, Qi Z
                       ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Mike Pattrick @ 2022-12-20 15:54 UTC (permalink / raw)
  To: dev
  Cc: qi.z.zhang, thomas, david.marchand, yidingx.zhou, ktraynor,
	Mike Pattrick, stable

iavf admin queue commands aren't thread-safe. Bugs surrounding this
issue can manifest in a variety of ways but frequently pend_cmd is
over written. Simultaneously executing commands can result in a
misconfigured device or DPDK sleeping in a thread for 2 second.

Despite this limitation, vf commands may be executed from both
iavf_dev_alarm_handler() in a control thread and the applications main
thread. This is trivial to simulate in the testpmd application by
creating a bond of vf's in active backup mode, and then turning the
bond off and then on again repeatedly.

Previously [1] was proposed as a potential solution, but this commit did
not resolve all potential issues concerning the admin queue and has been
reverted from the stable branch. I propose adding locks until a more
complete solution is available.

[1] commit cb5c1b91f76f ("net/iavf: add thread for event callbacks")

Fixes: 48de41ca11f0 ("net/avf: enable link status update")
Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel message")
Cc: stable@dpdk.org

Signed-off-by: Mike Pattrick <mkp@redhat.com>

---

v2:
 - Added locks around some iavf_execute_vf_cmd calls which were missed
 in v1
 - Changed description to indicate that [1] was only reverted out of the
 stable branch.
---
 drivers/net/iavf/iavf.h       |  1 +
 drivers/net/iavf/iavf_vchnl.c | 90 +++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+)

diff --git a/drivers/net/iavf/iavf.h b/drivers/net/iavf/iavf.h
index 1edebab8dc..aa18650ffa 100644
--- a/drivers/net/iavf/iavf.h
+++ b/drivers/net/iavf/iavf.h
@@ -262,6 +262,7 @@ struct iavf_info {
 	struct iavf_qv_map *qv_map; /* queue vector mapping */
 	struct iavf_flow_list flow_list;
 	rte_spinlock_t flow_ops_lock;
+	rte_spinlock_t aq_lock;
 	struct iavf_parser_list rss_parser_list;
 	struct iavf_parser_list dist_parser_list;
 	struct iavf_parser_list ipsec_crypto_parser_list;
diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index f92daf97f2..edb74edcac 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -554,7 +554,9 @@ iavf_enable_vlan_strip(struct iavf_adapter *adapter)
 	args.in_args_size = 0;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (ret)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " OP_ENABLE_VLAN_STRIPPING");
@@ -575,7 +577,9 @@ iavf_disable_vlan_strip(struct iavf_adapter *adapter)
 	args.in_args_size = 0;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (ret)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " OP_DISABLE_VLAN_STRIPPING");
@@ -604,7 +608,9 @@ iavf_check_api_version(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_INIT_LOG(ERR, "Fail to execute command of OP_VERSION");
 		return err;
@@ -665,7 +671,9 @@ iavf_get_vf_resource(struct iavf_adapter *adapter)
 	args.in_args = (uint8_t *)&caps;
 	args.in_args_size = sizeof(caps);
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 
 	if (err) {
 		PMD_DRV_LOG(ERR,
@@ -710,7 +718,9 @@ iavf_get_supported_rxdid(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (ret) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_GET_SUPPORTED_RXDIDS");
@@ -754,7 +764,9 @@ iavf_config_vlan_strip_v2(struct iavf_adapter *adapter, bool enable)
 	args.in_args_size = sizeof(vlan_strip);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (ret)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    enable ? "VIRTCHNL_OP_ENABLE_VLAN_STRIPPING_V2" :
@@ -794,7 +806,9 @@ iavf_config_vlan_insert_v2(struct iavf_adapter *adapter, bool enable)
 	args.in_args_size = sizeof(vlan_insert);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (ret)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    enable ? "VIRTCHNL_OP_ENABLE_VLAN_INSERTION_V2" :
@@ -837,7 +851,9 @@ iavf_add_del_vlan_v2(struct iavf_adapter *adapter, uint16_t vlanid, bool add)
 	args.in_args_size = sizeof(vlan_filter);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    add ? "OP_ADD_VLAN_V2" :  "OP_DEL_VLAN_V2");
@@ -858,7 +874,9 @@ iavf_get_vlan_offload_caps_v2(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (ret) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of VIRTCHNL_OP_GET_OFFLOAD_VLAN_V2_CAPS");
@@ -889,7 +907,9 @@ iavf_enable_queues(struct iavf_adapter *adapter)
 	args.in_args_size = sizeof(queue_select);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_ENABLE_QUEUES");
@@ -917,7 +937,9 @@ iavf_disable_queues(struct iavf_adapter *adapter)
 	args.in_args_size = sizeof(queue_select);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_DISABLE_QUEUES");
@@ -953,7 +975,9 @@ iavf_switch_queue(struct iavf_adapter *adapter, uint16_t qid,
 	args.in_args_size = sizeof(queue_select);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of %s",
 			    on ? "OP_ENABLE_QUEUES" : "OP_DISABLE_QUEUES");
@@ -995,7 +1019,9 @@ iavf_enable_queues_lv(struct iavf_adapter *adapter)
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_ENABLE_QUEUES_V2");
@@ -1039,7 +1065,9 @@ iavf_disable_queues_lv(struct iavf_adapter *adapter)
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_DISABLE_QUEUES_V2");
@@ -1085,7 +1113,9 @@ iavf_switch_queue_lv(struct iavf_adapter *adapter, uint16_t qid,
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of %s",
 			    on ? "OP_ENABLE_QUEUES_V2" : "OP_DISABLE_QUEUES_V2");
@@ -1117,7 +1147,9 @@ iavf_configure_rss_lut(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_CONFIG_RSS_LUT");
@@ -1149,7 +1181,9 @@ iavf_configure_rss_key(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_CONFIG_RSS_KEY");
@@ -1247,7 +1281,9 @@ iavf_configure_queues(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " VIRTCHNL_OP_CONFIG_VSI_QUEUES");
@@ -1288,7 +1324,9 @@ iavf_config_irq_map(struct iavf_adapter *adapter)
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command OP_CONFIG_IRQ_MAP");
 
@@ -1329,7 +1367,9 @@ iavf_config_irq_map_lv(struct iavf_adapter *adapter, uint16_t num,
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command OP_MAP_QUEUE_VECTOR");
 
@@ -1389,7 +1429,9 @@ iavf_add_del_all_mac_addr(struct iavf_adapter *adapter, bool add)
 		args.in_args_size = len;
 		args.out_buffer = vf->aq_resp;
 		args.out_size = IAVF_AQ_BUF_SZ;
+		rte_spinlock_lock(&vf->aq_lock);
 		err = iavf_execute_vf_cmd(adapter, &args, 0);
+		rte_spinlock_unlock(&vf->aq_lock);
 		if (err)
 			PMD_DRV_LOG(ERR, "fail to execute command %s",
 				    add ? "OP_ADD_ETHER_ADDRESS" :
@@ -1419,7 +1461,9 @@ iavf_query_stats(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_GET_STATS");
 		*pstats = NULL;
@@ -1457,7 +1501,9 @@ iavf_config_promisc(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 
 	if (err) {
 		PMD_DRV_LOG(ERR,
@@ -1500,7 +1546,9 @@ iavf_add_del_eth_addr(struct iavf_adapter *adapter, struct rte_ether_addr *addr,
 	args.in_args_size = sizeof(cmd_buffer);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    add ? "OP_ADD_ETH_ADDR" :  "OP_DEL_ETH_ADDR");
@@ -1527,7 +1575,9 @@ iavf_add_del_vlan(struct iavf_adapter *adapter, uint16_t vlanid, bool add)
 	args.in_args_size = sizeof(cmd_buffer);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    add ? "OP_ADD_VLAN" :  "OP_DEL_VLAN");
@@ -1554,7 +1604,9 @@ iavf_fdir_add(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_ADD_FDIR_FILTER");
 		return err;
@@ -1614,7 +1666,9 @@ iavf_fdir_del(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_DEL_FDIR_FILTER");
 		return err;
@@ -1661,7 +1715,9 @@ iavf_fdir_check(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to check flow director rule");
 		return err;
@@ -1704,7 +1760,9 @@ iavf_flow_sub(struct iavf_adapter *adapter, struct iavf_fsub_conf *filter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to execute command of "
 				 "OP_FLOW_SUBSCRIBE");
@@ -1755,7 +1813,9 @@ iavf_flow_unsub(struct iavf_adapter *adapter, struct iavf_fsub_conf *filter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to execute command of "
 				 "OP_FLOW_UNSUBSCRIBE");
@@ -1798,7 +1858,9 @@ iavf_flow_sub_check(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to check flow subscription rule");
 		return err;
@@ -1838,7 +1900,9 @@ iavf_add_del_rss_cfg(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of %s",
@@ -1861,7 +1925,9 @@ iavf_get_hena_caps(struct iavf_adapter *adapter, uint64_t *caps)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_GET_RSS_HENA_CAPS");
@@ -1887,7 +1953,9 @@ iavf_set_hena(struct iavf_adapter *adapter, uint64_t hena)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_SET_RSS_HENA");
@@ -1908,7 +1976,9 @@ iavf_get_qos_cap(struct iavf_adapter *adapter)
 	args.in_args_size = 0;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 
 	if (err) {
 		PMD_DRV_LOG(ERR,
@@ -1941,7 +2011,9 @@ int iavf_set_q_tc_map(struct rte_eth_dev *dev,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " VIRTCHNL_OP_CONFIG_TC_MAP");
@@ -1964,7 +2036,9 @@ int iavf_set_q_bw(struct rte_eth_dev *dev,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " VIRTCHNL_OP_CONFIG_QUEUE_BW");
@@ -2009,7 +2083,9 @@ iavf_add_del_mc_addr_list(struct iavf_adapter *adapter,
 		i * sizeof(struct virtchnl_ether_addr);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
@@ -2054,12 +2130,16 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
 		/* disable interrupt to avoid the admin queue message to be read
 		 * before iavf_read_msg_from_pf.
 		 */
+		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);
 	} else {
 		rte_eal_alarm_cancel(iavf_dev_alarm_handler, dev);
+		rte_spinlock_lock(&vf->aq_lock);
 		err = iavf_execute_vf_cmd(adapter, &args, 0);
+		rte_spinlock_unlock(&vf->aq_lock);
 		rte_eal_alarm_set(IAVF_ALARM_INTERVAL,
 				  iavf_dev_alarm_handler, dev);
 	}
@@ -2098,7 +2178,9 @@ iavf_get_max_rss_queue_region(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to execute command of VIRTCHNL_OP_GET_MAX_RSS_QREGION");
 		return err;
@@ -2129,7 +2211,9 @@ iavf_ipsec_crypto_request(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 1);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 				"OP_INLINE_IPSEC_CRYPTO");
@@ -2163,7 +2247,9 @@ iavf_set_vf_quanta_size(struct iavf_adapter *adapter, u16 start_queue_id, u16 nu
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to execute command VIRTCHNL_OP_CONFIG_QUANTA");
 		return err;
@@ -2189,7 +2275,9 @@ iavf_get_ptp_cap(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_1588_PTP_GET_CAPS");
@@ -2216,8 +2304,10 @@ iavf_get_phc_time(struct iavf_rx_queue *rxq)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
+	rte_spinlock_lock(&vf->aq_lock);
 	rte_spinlock_lock(&vf->phc_time_aq_lock);
 	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	rte_spinlock_unlock(&vf->aq_lock);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of VIRTCHNL_OP_1588_PTP_GET_TIME");
-- 
2.31.1


^ permalink raw reply	[flat|nested] 8+ messages in thread
* RE: [PATCH v2] net/iavf: add lock for vf commands
  2022-12-20 15:54   ` [PATCH v2] " Mike Pattrick
@ 2022-12-26  6:01     ` Zhang, Qi Z
  2022-12-28 22:28     ` [PATCH v3] " Mike Pattrick
  2022-12-28 23:00     ` [PATCH v4] " Mike Pattrick
  2 siblings, 0 replies; 8+ messages in thread
From: Zhang, Qi Z @ 2022-12-26  6:01 UTC (permalink / raw)
  To: Mike Pattrick, dev
  Cc: thomas, david.marchand, Zhou, YidingX, ktraynor, stable



> -----Original Message-----
> From: Mike Pattrick <mkp@redhat.com>
> Sent: Tuesday, December 20, 2022 11:54 PM
> To: dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net;
> david.marchand@redhat.com; Zhou, YidingX <yidingx.zhou@intel.com>;
> ktraynor@redhat.com; Mike Pattrick <mkp@redhat.com>; stable@dpdk.org
> Subject: [PATCH v2] net/iavf: add lock for vf commands
> 
> iavf admin queue commands aren't thread-safe. Bugs surrounding this issue
> can manifest in a variety of ways but frequently pend_cmd is over written.
> Simultaneously executing commands can result in a misconfigured device or
> DPDK sleeping in a thread for 2 second.
> 
> Despite this limitation, vf commands may be executed from both
> iavf_dev_alarm_handler() in a control thread and the applications main
> thread. This is trivial to simulate in the testpmd application by creating a
> bond of vf's in active backup mode, and then turning the bond off and then
> on again repeatedly.
> 
> Previously [1] was proposed as a potential solution, but this commit did not
> resolve all potential issues concerning the admin queue and has been
> reverted from the stable branch. I propose adding locks until a more
> complete solution is available.
> 
> [1] commit cb5c1b91f76f ("net/iavf: add thread for event callbacks")
> 
> Fixes: 48de41ca11f0 ("net/avf: enable link status update")
> Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel
> message")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mike Pattrick <mkp@redhat.com>
> 
> ---
> 
> v2:
>  - Added locks around some iavf_execute_vf_cmd calls which were missed  in
> v1
>  - Changed description to indicate that [1] was only reverted out of the  stable
> branch.
> ---
>  drivers/net/iavf/iavf.h       |  1 +
>  drivers/net/iavf/iavf_vchnl.c | 90 +++++++++++++++++++++++++++++++++++
>  2 files changed, 91 insertions(+)
> 
> diff --git a/drivers/net/iavf/iavf.h b/drivers/net/iavf/iavf.h index
> 1edebab8dc..aa18650ffa 100644
> --- a/drivers/net/iavf/iavf.h
> +++ b/drivers/net/iavf/iavf.h
> @@ -262,6 +262,7 @@ struct iavf_info {
>  	struct iavf_qv_map *qv_map; /* queue vector mapping */
>  	struct iavf_flow_list flow_list;
>  	rte_spinlock_t flow_ops_lock;
> +	rte_spinlock_t aq_lock;
>  	struct iavf_parser_list rss_parser_list;
>  	struct iavf_parser_list dist_parser_list;
>  	struct iavf_parser_list ipsec_crypto_parser_list; diff --git
> a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c index
> f92daf97f2..edb74edcac 100644
> --- a/drivers/net/iavf/iavf_vchnl.c
> +++ b/drivers/net/iavf/iavf_vchnl.c
> @@ -554,7 +554,9 @@ iavf_enable_vlan_strip(struct iavf_adapter *adapter)
>  	args.in_args_size = 0;
>  	args.out_buffer = vf->aq_resp;
>  	args.out_size = IAVF_AQ_BUF_SZ;
> +	rte_spinlock_lock(&vf->aq_lock);
>  	ret = iavf_execute_vf_cmd(adapter, &args, 0);
> +	rte_spinlock_unlock(&vf->aq_lock);
>  	if (ret)
>  		PMD_DRV_LOG(ERR, "Failed to execute command of"
>  			    " OP_ENABLE_VLAN_STRIPPING");
> @@ -575,7 +577,9 @@ iavf_disable_vlan_strip(struct iavf_adapter *adapter)
>  	args.in_args_size = 0;
>  	args.out_buffer = vf->aq_resp;
>  	args.out_size = IAVF_AQ_BUF_SZ;
> +	rte_spinlock_lock(&vf->aq_lock);
>  	ret = iavf_execute_vf_cmd(adapter, &args, 0);
> +	rte_spinlock_unlock(&vf->aq_lock);
>  	if (ret)
>  		PMD_DRV_LOG(ERR, "Failed to execute command of"
>  			    " OP_DISABLE_VLAN_STRIPPING");
> @@ -604,7 +608,9 @@ iavf_check_api_version(struct iavf_adapter *adapter)
>  	args.out_buffer = vf->aq_resp;
>  	args.out_size = IAVF_AQ_BUF_SZ;
> 
> +	rte_spinlock_lock(&vf->aq_lock);
>  	err = iavf_execute_vf_cmd(adapter, &args, 0);
> +	rte_spinlock_unlock(&vf->aq_lock);

Instead of wrapping lock and unlock every place, can we add a new wrap function e.g.: iavf_execute_vf_cmd_safe? 



^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH v3] net/iavf: add lock for vf commands
  2022-12-20 15:54   ` [PATCH v2] " Mike Pattrick
  2022-12-26  6:01     ` Zhang, Qi Z
@ 2022-12-28 22:28     ` Mike Pattrick
  2022-12-28 23:00     ` [PATCH v4] " Mike Pattrick
  2 siblings, 0 replies; 8+ messages in thread
From: Mike Pattrick @ 2022-12-28 22:28 UTC (permalink / raw)
  To: dev
  Cc: qi.z.zhang, thomas, david.marchand, yidingx.zhou, ktraynor,
	Mike Pattrick, stable

iavf admin queue commands aren't thread-safe. Bugs surrounding this
issue can manifest in a variety of ways but frequently pend_cmd is
over written. Simultaneously executing commands can result in a
misconfigured device or DPDK sleeping in a thread for 2 second.

Despite this limitation, vf commands may be executed from both
iavf_dev_alarm_handler() in a control thread and the applications main
thread. This is trivial to simulate in the testpmd application by
creating a bond of vf's in active backup mode, and then turning the
bond off and then on again repeatedly.

Previously [1] was proposed as a potential solution, but this commit did
not resolve all potential issues concerning the admin queue and has been
reverted from the stable branch. I propose adding locks until a more
complete solution is available.

[1] commit cb5c1b91f76f ("net/iavf: add thread for event callbacks")

Fixes: 48de41ca11f0 ("net/avf: enable link status update")
Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel message")
Cc: stable@dpdk.org

Signed-off-by: Mike Pattrick <mkp@redhat.com>

---

v2:
 - Added locks around some iavf_execute_vf_cmd calls which were missed
 in v1
 - Changed description to indicate that [1] was only reverted out of the
 stable branch.

v3:
 - Refacted all locks into single function.
---
 drivers/net/iavf/iavf.h       |   1 +
 drivers/net/iavf/iavf_vchnl.c | 105 ++++++++++++++++++++--------------
 2 files changed, 62 insertions(+), 44 deletions(-)

diff --git a/drivers/net/iavf/iavf.h b/drivers/net/iavf/iavf.h
index 1edebab8dc..aa18650ffa 100644
--- a/drivers/net/iavf/iavf.h
+++ b/drivers/net/iavf/iavf.h
@@ -262,6 +262,7 @@ struct iavf_info {
 	struct iavf_qv_map *qv_map; /* queue vector mapping */
 	struct iavf_flow_list flow_list;
 	rte_spinlock_t flow_ops_lock;
+	rte_spinlock_t aq_lock;
 	struct iavf_parser_list rss_parser_list;
 	struct iavf_parser_list dist_parser_list;
 	struct iavf_parser_list ipsec_crypto_parser_list;
diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index f92daf97f2..7bd992c882 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -397,6 +397,19 @@ iavf_execute_vf_cmd(struct iavf_adapter *adapter, struct iavf_cmd_info *args,
 	return err;
 }
 
+static int
+iavf_execute_vf_cmd_safe(struct iavf_adapter *adapter,
+	struct iavf_cmd_info *args, int async)
+{
+	int ret;
+
+	rte_spinlock_lock(&vf->aq_lock);
+	ret = iavf_execute_vf_cmd(adapter, args, async);
+	rte_spinlock_unlock(&vf->aq_lock);
+
+	return ret;
+}
+
 static void
 iavf_handle_pf_event_msg(struct rte_eth_dev *dev, uint8_t *msg,
 			uint16_t msglen)
@@ -554,7 +567,7 @@ iavf_enable_vlan_strip(struct iavf_adapter *adapter)
 	args.in_args_size = 0;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (ret)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " OP_ENABLE_VLAN_STRIPPING");
@@ -575,7 +588,7 @@ iavf_disable_vlan_strip(struct iavf_adapter *adapter)
 	args.in_args_size = 0;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (ret)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " OP_DISABLE_VLAN_STRIPPING");
@@ -604,7 +617,7 @@ iavf_check_api_version(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_INIT_LOG(ERR, "Fail to execute command of OP_VERSION");
 		return err;
@@ -665,7 +678,7 @@ iavf_get_vf_resource(struct iavf_adapter *adapter)
 	args.in_args = (uint8_t *)&caps;
 	args.in_args_size = sizeof(caps);
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 
 	if (err) {
 		PMD_DRV_LOG(ERR,
@@ -710,7 +723,7 @@ iavf_get_supported_rxdid(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (ret) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_GET_SUPPORTED_RXDIDS");
@@ -754,7 +767,7 @@ iavf_config_vlan_strip_v2(struct iavf_adapter *adapter, bool enable)
 	args.in_args_size = sizeof(vlan_strip);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (ret)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    enable ? "VIRTCHNL_OP_ENABLE_VLAN_STRIPPING_V2" :
@@ -794,7 +807,7 @@ iavf_config_vlan_insert_v2(struct iavf_adapter *adapter, bool enable)
 	args.in_args_size = sizeof(vlan_insert);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (ret)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    enable ? "VIRTCHNL_OP_ENABLE_VLAN_INSERTION_V2" :
@@ -837,7 +850,7 @@ iavf_add_del_vlan_v2(struct iavf_adapter *adapter, uint16_t vlanid, bool add)
 	args.in_args_size = sizeof(vlan_filter);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    add ? "OP_ADD_VLAN_V2" :  "OP_DEL_VLAN_V2");
@@ -858,7 +871,7 @@ iavf_get_vlan_offload_caps_v2(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (ret) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of VIRTCHNL_OP_GET_OFFLOAD_VLAN_V2_CAPS");
@@ -889,7 +902,7 @@ iavf_enable_queues(struct iavf_adapter *adapter)
 	args.in_args_size = sizeof(queue_select);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_ENABLE_QUEUES");
@@ -917,7 +930,7 @@ iavf_disable_queues(struct iavf_adapter *adapter)
 	args.in_args_size = sizeof(queue_select);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_DISABLE_QUEUES");
@@ -953,7 +966,7 @@ iavf_switch_queue(struct iavf_adapter *adapter, uint16_t qid,
 	args.in_args_size = sizeof(queue_select);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of %s",
 			    on ? "OP_ENABLE_QUEUES" : "OP_DISABLE_QUEUES");
@@ -995,7 +1008,7 @@ iavf_enable_queues_lv(struct iavf_adapter *adapter)
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_ENABLE_QUEUES_V2");
@@ -1039,7 +1052,7 @@ iavf_disable_queues_lv(struct iavf_adapter *adapter)
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_DISABLE_QUEUES_V2");
@@ -1085,7 +1098,7 @@ iavf_switch_queue_lv(struct iavf_adapter *adapter, uint16_t qid,
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of %s",
 			    on ? "OP_ENABLE_QUEUES_V2" : "OP_DISABLE_QUEUES_V2");
@@ -1117,7 +1130,7 @@ iavf_configure_rss_lut(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_CONFIG_RSS_LUT");
@@ -1149,7 +1162,7 @@ iavf_configure_rss_key(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_CONFIG_RSS_KEY");
@@ -1247,7 +1260,7 @@ iavf_configure_queues(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " VIRTCHNL_OP_CONFIG_VSI_QUEUES");
@@ -1288,7 +1301,7 @@ iavf_config_irq_map(struct iavf_adapter *adapter)
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command OP_CONFIG_IRQ_MAP");
 
@@ -1329,7 +1342,7 @@ iavf_config_irq_map_lv(struct iavf_adapter *adapter, uint16_t num,
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command OP_MAP_QUEUE_VECTOR");
 
@@ -1389,7 +1402,7 @@ iavf_add_del_all_mac_addr(struct iavf_adapter *adapter, bool add)
 		args.in_args_size = len;
 		args.out_buffer = vf->aq_resp;
 		args.out_size = IAVF_AQ_BUF_SZ;
-		err = iavf_execute_vf_cmd(adapter, &args, 0);
+		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 		if (err)
 			PMD_DRV_LOG(ERR, "fail to execute command %s",
 				    add ? "OP_ADD_ETHER_ADDRESS" :
@@ -1419,7 +1432,7 @@ iavf_query_stats(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_GET_STATS");
 		*pstats = NULL;
@@ -1457,7 +1470,7 @@ iavf_config_promisc(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cm_safed(adapter, &args, 0);
 
 	if (err) {
 		PMD_DRV_LOG(ERR,
@@ -1500,7 +1513,7 @@ iavf_add_del_eth_addr(struct iavf_adapter *adapter, struct rte_ether_addr *addr,
 	args.in_args_size = sizeof(cmd_buffer);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    add ? "OP_ADD_ETH_ADDR" :  "OP_DEL_ETH_ADDR");
@@ -1527,7 +1540,7 @@ iavf_add_del_vlan(struct iavf_adapter *adapter, uint16_t vlanid, bool add)
 	args.in_args_size = sizeof(cmd_buffer);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    add ? "OP_ADD_VLAN" :  "OP_DEL_VLAN");
@@ -1554,7 +1567,7 @@ iavf_fdir_add(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_ADD_FDIR_FILTER");
 		return err;
@@ -1614,7 +1627,7 @@ iavf_fdir_del(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_DEL_FDIR_FILTER");
 		return err;
@@ -1661,7 +1674,7 @@ iavf_fdir_check(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to check flow director rule");
 		return err;
@@ -1704,7 +1717,7 @@ iavf_flow_sub(struct iavf_adapter *adapter, struct iavf_fsub_conf *filter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to execute command of "
 				 "OP_FLOW_SUBSCRIBE");
@@ -1755,7 +1768,7 @@ iavf_flow_unsub(struct iavf_adapter *adapter, struct iavf_fsub_conf *filter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to execute command of "
 				 "OP_FLOW_UNSUBSCRIBE");
@@ -1798,7 +1811,7 @@ iavf_flow_sub_check(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to check flow subscription rule");
 		return err;
@@ -1838,7 +1851,7 @@ iavf_add_del_rss_cfg(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of %s",
@@ -1861,7 +1874,7 @@ iavf_get_hena_caps(struct iavf_adapter *adapter, uint64_t *caps)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_GET_RSS_HENA_CAPS");
@@ -1887,7 +1900,7 @@ iavf_set_hena(struct iavf_adapter *adapter, uint64_t hena)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_SET_RSS_HENA");
@@ -1908,7 +1921,7 @@ iavf_get_qos_cap(struct iavf_adapter *adapter)
 	args.in_args_size = 0;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cm_safed(adapter, &args, 0);
 
 	if (err) {
 		PMD_DRV_LOG(ERR,
@@ -1941,7 +1954,7 @@ int iavf_set_q_tc_map(struct rte_eth_dev *dev,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " VIRTCHNL_OP_CONFIG_TC_MAP");
@@ -1964,7 +1977,7 @@ int iavf_set_q_bw(struct rte_eth_dev *dev,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " VIRTCHNL_OP_CONFIG_QUEUE_BW");
@@ -2009,7 +2022,7 @@ iavf_add_del_mc_addr_list(struct iavf_adapter *adapter,
 		i * sizeof(struct virtchnl_ether_addr);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cm_safed(adapter, &args, 0);
 
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
@@ -2053,13 +2066,17 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
 	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);
 	} else {
 		rte_eal_alarm_cancel(iavf_dev_alarm_handler, dev);
-		err = iavf_execute_vf_cmd(adapter, &args, 0);
+		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 		rte_eal_alarm_set(IAVF_ALARM_INTERVAL,
 				  iavf_dev_alarm_handler, dev);
 	}
@@ -2098,7 +2115,7 @@ iavf_get_max_rss_queue_region(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to execute command of VIRTCHNL_OP_GET_MAX_RSS_QREGION");
 		return err;
@@ -2129,7 +2146,7 @@ iavf_ipsec_crypto_request(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 1);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 1);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 				"OP_INLINE_IPSEC_CRYPTO");
@@ -2163,7 +2180,7 @@ iavf_set_vf_quanta_size(struct iavf_adapter *adapter, u16 start_queue_id, u16 nu
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to execute command VIRTCHNL_OP_CONFIG_QUANTA");
 		return err;
@@ -2189,7 +2206,7 @@ iavf_get_ptp_cap(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_1588_PTP_GET_CAPS");
@@ -2217,7 +2234,7 @@ iavf_get_phc_time(struct iavf_rx_queue *rxq)
 	args.out_size = IAVF_AQ_BUF_SZ;
 
 	rte_spinlock_lock(&vf->phc_time_aq_lock);
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of VIRTCHNL_OP_1588_PTP_GET_TIME");
-- 
2.31.1


^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH v4] net/iavf: add lock for vf commands
  2022-12-20 15:54   ` [PATCH v2] " Mike Pattrick
  2022-12-26  6:01     ` Zhang, Qi Z
  2022-12-28 22:28     ` [PATCH v3] " Mike Pattrick
@ 2022-12-28 23:00     ` Mike Pattrick
  2023-01-13 13:35       ` David Marchand
  2023-01-17  1:20       ` Zhang, Qi Z
  2 siblings, 2 replies; 8+ messages in thread
From: Mike Pattrick @ 2022-12-28 23:00 UTC (permalink / raw)
  To: dev
  Cc: qi.z.zhang, thomas, david.marchand, yidingx.zhou, ktraynor,
	Mike Pattrick, stable

iavf admin queue commands aren't thread-safe. Bugs surrounding this
issue can manifest in a variety of ways but frequently pend_cmd is
over written. Simultaneously executing commands can result in a
misconfigured device or DPDK sleeping in a thread for 2 second.

Despite this limitation, vf commands may be executed from both
iavf_dev_alarm_handler() in a control thread and the applications main
thread. This is trivial to simulate in the testpmd application by
creating a bond of vf's in active backup mode, and then turning the
bond off and then on again repeatedly.

Previously [1] was proposed as a potential solution, but this commit did
not resolve all potential issues concerning the admin queue and has been
reverted from the stable branch. I propose adding locks until a more
complete solution is available.

[1] commit cb5c1b91f76f ("net/iavf: add thread for event callbacks")

Fixes: 48de41ca11f0 ("net/avf: enable link status update")
Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel message")
Cc: stable@dpdk.org

Signed-off-by: Mike Pattrick <mkp@redhat.com>

---

v2:
 - Added locks around some iavf_execute_vf_cmd calls which were missed
 in v1
 - Changed description to indicate that [1] was only reverted out of the
 stable branch.

v3:
 - Refacted all locks into single function.

v4:
 - Previous patch got corrupted
---
 drivers/net/iavf/iavf.h       |   1 +
 drivers/net/iavf/iavf_vchnl.c | 106 ++++++++++++++++++++--------------
 2 files changed, 63 insertions(+), 44 deletions(-)

diff --git a/drivers/net/iavf/iavf.h b/drivers/net/iavf/iavf.h
index 1edebab8dc..aa18650ffa 100644
--- a/drivers/net/iavf/iavf.h
+++ b/drivers/net/iavf/iavf.h
@@ -262,6 +262,7 @@ struct iavf_info {
 	struct iavf_qv_map *qv_map; /* queue vector mapping */
 	struct iavf_flow_list flow_list;
 	rte_spinlock_t flow_ops_lock;
+	rte_spinlock_t aq_lock;
 	struct iavf_parser_list rss_parser_list;
 	struct iavf_parser_list dist_parser_list;
 	struct iavf_parser_list ipsec_crypto_parser_list;
diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index f92daf97f2..9adaadb173 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -397,6 +397,20 @@ iavf_execute_vf_cmd(struct iavf_adapter *adapter, struct iavf_cmd_info *args,
 	return err;
 }
 
+static int
+iavf_execute_vf_cmd_safe(struct iavf_adapter *adapter,
+	struct iavf_cmd_info *args, int async)
+{
+	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
+	int ret;
+
+	rte_spinlock_lock(&vf->aq_lock);
+	ret = iavf_execute_vf_cmd(adapter, args, async);
+	rte_spinlock_unlock(&vf->aq_lock);
+
+	return ret;
+}
+
 static void
 iavf_handle_pf_event_msg(struct rte_eth_dev *dev, uint8_t *msg,
 			uint16_t msglen)
@@ -554,7 +568,7 @@ iavf_enable_vlan_strip(struct iavf_adapter *adapter)
 	args.in_args_size = 0;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (ret)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " OP_ENABLE_VLAN_STRIPPING");
@@ -575,7 +589,7 @@ iavf_disable_vlan_strip(struct iavf_adapter *adapter)
 	args.in_args_size = 0;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (ret)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " OP_DISABLE_VLAN_STRIPPING");
@@ -604,7 +618,7 @@ iavf_check_api_version(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_INIT_LOG(ERR, "Fail to execute command of OP_VERSION");
 		return err;
@@ -665,7 +679,7 @@ iavf_get_vf_resource(struct iavf_adapter *adapter)
 	args.in_args = (uint8_t *)&caps;
 	args.in_args_size = sizeof(caps);
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 
 	if (err) {
 		PMD_DRV_LOG(ERR,
@@ -710,7 +724,7 @@ iavf_get_supported_rxdid(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (ret) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_GET_SUPPORTED_RXDIDS");
@@ -754,7 +768,7 @@ iavf_config_vlan_strip_v2(struct iavf_adapter *adapter, bool enable)
 	args.in_args_size = sizeof(vlan_strip);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (ret)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    enable ? "VIRTCHNL_OP_ENABLE_VLAN_STRIPPING_V2" :
@@ -794,7 +808,7 @@ iavf_config_vlan_insert_v2(struct iavf_adapter *adapter, bool enable)
 	args.in_args_size = sizeof(vlan_insert);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (ret)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    enable ? "VIRTCHNL_OP_ENABLE_VLAN_INSERTION_V2" :
@@ -837,7 +851,7 @@ iavf_add_del_vlan_v2(struct iavf_adapter *adapter, uint16_t vlanid, bool add)
 	args.in_args_size = sizeof(vlan_filter);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    add ? "OP_ADD_VLAN_V2" :  "OP_DEL_VLAN_V2");
@@ -858,7 +872,7 @@ iavf_get_vlan_offload_caps_v2(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	ret = iavf_execute_vf_cmd(adapter, &args, 0);
+	ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (ret) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of VIRTCHNL_OP_GET_OFFLOAD_VLAN_V2_CAPS");
@@ -889,7 +903,7 @@ iavf_enable_queues(struct iavf_adapter *adapter)
 	args.in_args_size = sizeof(queue_select);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_ENABLE_QUEUES");
@@ -917,7 +931,7 @@ iavf_disable_queues(struct iavf_adapter *adapter)
 	args.in_args_size = sizeof(queue_select);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_DISABLE_QUEUES");
@@ -953,7 +967,7 @@ iavf_switch_queue(struct iavf_adapter *adapter, uint16_t qid,
 	args.in_args_size = sizeof(queue_select);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of %s",
 			    on ? "OP_ENABLE_QUEUES" : "OP_DISABLE_QUEUES");
@@ -995,7 +1009,7 @@ iavf_enable_queues_lv(struct iavf_adapter *adapter)
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_ENABLE_QUEUES_V2");
@@ -1039,7 +1053,7 @@ iavf_disable_queues_lv(struct iavf_adapter *adapter)
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_DISABLE_QUEUES_V2");
@@ -1085,7 +1099,7 @@ iavf_switch_queue_lv(struct iavf_adapter *adapter, uint16_t qid,
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of %s",
 			    on ? "OP_ENABLE_QUEUES_V2" : "OP_DISABLE_QUEUES_V2");
@@ -1117,7 +1131,7 @@ iavf_configure_rss_lut(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_CONFIG_RSS_LUT");
@@ -1149,7 +1163,7 @@ iavf_configure_rss_key(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_CONFIG_RSS_KEY");
@@ -1247,7 +1261,7 @@ iavf_configure_queues(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " VIRTCHNL_OP_CONFIG_VSI_QUEUES");
@@ -1288,7 +1302,7 @@ iavf_config_irq_map(struct iavf_adapter *adapter)
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command OP_CONFIG_IRQ_MAP");
 
@@ -1329,7 +1343,7 @@ iavf_config_irq_map_lv(struct iavf_adapter *adapter, uint16_t num,
 	args.in_args_size = len;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command OP_MAP_QUEUE_VECTOR");
 
@@ -1389,7 +1403,7 @@ iavf_add_del_all_mac_addr(struct iavf_adapter *adapter, bool add)
 		args.in_args_size = len;
 		args.out_buffer = vf->aq_resp;
 		args.out_size = IAVF_AQ_BUF_SZ;
-		err = iavf_execute_vf_cmd(adapter, &args, 0);
+		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 		if (err)
 			PMD_DRV_LOG(ERR, "fail to execute command %s",
 				    add ? "OP_ADD_ETHER_ADDRESS" :
@@ -1419,7 +1433,7 @@ iavf_query_stats(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_GET_STATS");
 		*pstats = NULL;
@@ -1457,7 +1471,7 @@ iavf_config_promisc(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 
 	if (err) {
 		PMD_DRV_LOG(ERR,
@@ -1500,7 +1514,7 @@ iavf_add_del_eth_addr(struct iavf_adapter *adapter, struct rte_ether_addr *addr,
 	args.in_args_size = sizeof(cmd_buffer);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    add ? "OP_ADD_ETH_ADDR" :  "OP_DEL_ETH_ADDR");
@@ -1527,7 +1541,7 @@ iavf_add_del_vlan(struct iavf_adapter *adapter, uint16_t vlanid, bool add)
 	args.in_args_size = sizeof(cmd_buffer);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 			    add ? "OP_ADD_VLAN" :  "OP_DEL_VLAN");
@@ -1554,7 +1568,7 @@ iavf_fdir_add(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_ADD_FDIR_FILTER");
 		return err;
@@ -1614,7 +1628,7 @@ iavf_fdir_del(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_DEL_FDIR_FILTER");
 		return err;
@@ -1661,7 +1675,7 @@ iavf_fdir_check(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to check flow director rule");
 		return err;
@@ -1704,7 +1718,7 @@ iavf_flow_sub(struct iavf_adapter *adapter, struct iavf_fsub_conf *filter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to execute command of "
 				 "OP_FLOW_SUBSCRIBE");
@@ -1755,7 +1769,7 @@ iavf_flow_unsub(struct iavf_adapter *adapter, struct iavf_fsub_conf *filter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to execute command of "
 				 "OP_FLOW_UNSUBSCRIBE");
@@ -1798,7 +1812,7 @@ iavf_flow_sub_check(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to check flow subscription rule");
 		return err;
@@ -1838,7 +1852,7 @@ iavf_add_del_rss_cfg(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of %s",
@@ -1861,7 +1875,7 @@ iavf_get_hena_caps(struct iavf_adapter *adapter, uint64_t *caps)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_GET_RSS_HENA_CAPS");
@@ -1887,7 +1901,7 @@ iavf_set_hena(struct iavf_adapter *adapter, uint64_t hena)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_SET_RSS_HENA");
@@ -1908,7 +1922,7 @@ iavf_get_qos_cap(struct iavf_adapter *adapter)
 	args.in_args_size = 0;
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 
 	if (err) {
 		PMD_DRV_LOG(ERR,
@@ -1941,7 +1955,7 @@ int iavf_set_q_tc_map(struct rte_eth_dev *dev,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " VIRTCHNL_OP_CONFIG_TC_MAP");
@@ -1964,7 +1978,7 @@ int iavf_set_q_bw(struct rte_eth_dev *dev,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err)
 		PMD_DRV_LOG(ERR, "Failed to execute command of"
 			    " VIRTCHNL_OP_CONFIG_QUEUE_BW");
@@ -2009,7 +2023,7 @@ iavf_add_del_mc_addr_list(struct iavf_adapter *adapter,
 		i * sizeof(struct virtchnl_ether_addr);
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
@@ -2053,13 +2067,17 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
 	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);
 	} else {
 		rte_eal_alarm_cancel(iavf_dev_alarm_handler, dev);
-		err = iavf_execute_vf_cmd(adapter, &args, 0);
+		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 		rte_eal_alarm_set(IAVF_ALARM_INTERVAL,
 				  iavf_dev_alarm_handler, dev);
 	}
@@ -2098,7 +2116,7 @@ iavf_get_max_rss_queue_region(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to execute command of VIRTCHNL_OP_GET_MAX_RSS_QREGION");
 		return err;
@@ -2129,7 +2147,7 @@ iavf_ipsec_crypto_request(struct iavf_adapter *adapter,
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 1);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 1);
 	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command %s",
 				"OP_INLINE_IPSEC_CRYPTO");
@@ -2163,7 +2181,7 @@ iavf_set_vf_quanta_size(struct iavf_adapter *adapter, u16 start_queue_id, u16 nu
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to execute command VIRTCHNL_OP_CONFIG_QUANTA");
 		return err;
@@ -2189,7 +2207,7 @@ iavf_get_ptp_cap(struct iavf_adapter *adapter)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = IAVF_AQ_BUF_SZ;
 
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of OP_1588_PTP_GET_CAPS");
@@ -2217,7 +2235,7 @@ iavf_get_phc_time(struct iavf_rx_queue *rxq)
 	args.out_size = IAVF_AQ_BUF_SZ;
 
 	rte_spinlock_lock(&vf->phc_time_aq_lock);
-	err = iavf_execute_vf_cmd(adapter, &args, 0);
+	err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
 	if (err) {
 		PMD_DRV_LOG(ERR,
 			    "Failed to execute command of VIRTCHNL_OP_1588_PTP_GET_TIME");
-- 
2.31.1


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v4] net/iavf: add lock for vf commands
  2022-12-28 23:00     ` [PATCH v4] " Mike Pattrick
@ 2023-01-13 13:35       ` David Marchand
  2023-01-17  1:20       ` Zhang, Qi Z
  1 sibling, 0 replies; 8+ messages in thread
From: David Marchand @ 2023-01-13 13:35 UTC (permalink / raw)
  To: Jingjing Wu, Beilei Xing
  Cc: dev, qi.z.zhang, thomas, yidingx.zhou, ktraynor, stable,
	Mike Pattrick, Mcnamara, John

Hi iavf maintainers,

On Thu, Dec 29, 2022 at 12:00 AM Mike Pattrick <mkp@redhat.com> wrote:
>
> iavf admin queue commands aren't thread-safe. Bugs surrounding this
> issue can manifest in a variety of ways but frequently pend_cmd is
> over written. Simultaneously executing commands can result in a
> misconfigured device or DPDK sleeping in a thread for 2 second.
>
> Despite this limitation, vf commands may be executed from both
> iavf_dev_alarm_handler() in a control thread and the applications main
> thread. This is trivial to simulate in the testpmd application by
> creating a bond of vf's in active backup mode, and then turning the
> bond off and then on again repeatedly.
>
> Previously [1] was proposed as a potential solution, but this commit did
> not resolve all potential issues concerning the admin queue and has been
> reverted from the stable branch. I propose adding locks until a more
> complete solution is available.
>
> [1] commit cb5c1b91f76f ("net/iavf: add thread for event callbacks")
>
> Fixes: 48de41ca11f0 ("net/avf: enable link status update")
> Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel message")
> Cc: stable@dpdk.org
>
> Signed-off-by: Mike Pattrick <mkp@redhat.com>

Review please.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 8+ messages in thread
* RE: [PATCH v4] net/iavf: add lock for vf commands
  2022-12-28 23:00     ` [PATCH v4] " Mike Pattrick
  2023-01-13 13:35       ` David Marchand
@ 2023-01-17  1:20       ` Zhang, Qi Z
  1 sibling, 0 replies; 8+ messages in thread
From: Zhang, Qi Z @ 2023-01-17  1:20 UTC (permalink / raw)
  To: Mike Pattrick, dev
  Cc: thomas, david.marchand, Zhou, YidingX, ktraynor, stable



> -----Original Message-----
> From: Mike Pattrick <mkp@redhat.com>
> Sent: Thursday, December 29, 2022 7:00 AM
> To: dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net;
> david.marchand@redhat.com; Zhou, YidingX <yidingx.zhou@intel.com>;
> ktraynor@redhat.com; Mike Pattrick <mkp@redhat.com>; stable@dpdk.org
> Subject: [PATCH v4] net/iavf: add lock for vf commands
> 
> iavf admin queue commands aren't thread-safe. Bugs surrounding this issue
> can manifest in a variety of ways but frequently pend_cmd is over written.
> Simultaneously executing commands can result in a misconfigured device or
> DPDK sleeping in a thread for 2 second.
> 
> Despite this limitation, vf commands may be executed from both
> iavf_dev_alarm_handler() in a control thread and the applications main
> thread. This is trivial to simulate in the testpmd application by creating a bond
> of vf's in active backup mode, and then turning the bond off and then on
> again repeatedly.
> 
> Previously [1] was proposed as a potential solution, but this commit did not
> resolve all potential issues concerning the admin queue and has been
> reverted from the stable branch. I propose adding locks until a more
> complete solution is available.
> 
> [1] commit cb5c1b91f76f ("net/iavf: add thread for event callbacks")
> 
> Fixes: 48de41ca11f0 ("net/avf: enable link status update")
> Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel
> message")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mike Pattrick <mkp@redhat.com>

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

Applied to dpdk-next-net-intel.

Thanks
Qi

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-01-17  1:20 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-19 23:23 [PATCH] net/iavf: add lock for vf commands Mike Pattrick
2022-12-20 15:02 ` David Marchand
2022-12-20 15:54   ` [PATCH v2] " Mike Pattrick
2022-12-26  6:01     ` Zhang, Qi Z
2022-12-28 22:28     ` [PATCH v3] " Mike Pattrick
2022-12-28 23:00     ` [PATCH v4] " Mike Pattrick
2023-01-13 13:35       ` David Marchand
2023-01-17  1:20       ` Zhang, Qi Z

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).