Skip to content

Commit

Permalink
net/iavf: add lock for VF commands
Browse files Browse the repository at this point in the history 
[ upstream commit 91bf37d250aacfc9512a33ce9ec6d4783766804e ]

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 cb5c1b9 ("net/iavf: add thread for event callbacks")

Fixes: 48de41c ("net/avf: enable link status update")
Fixes: 8410842 ("net/iavf: support asynchronous virtual channel message")

Signed-off-by: Mike Pattrick <mkp@redhat.com>
Acked-by: Qi Zhang <qi.z.zhang@intel.com>
  • Loading branch information
@mkp-rh @bluca
mkp-rh authored and bluca committed Feb 23, 2023
1 parent 060c26e commit a44e427
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 27 deletions.
1 change: 1 addition & 0 deletions drivers/net/iavf/iavf.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,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;

Expand Down
72 changes: 45 additions & 27 deletions drivers/net/iavf/iavf_vchnl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,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)
{
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);
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)
Expand Down Expand Up @@ -504,7 +518,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);
ret = iavf_execute_vf_cmd_safe(adapter, &args);
if (ret)
PMD_DRV_LOG(ERR, "Failed to execute command of"
" OP_ENABLE_VLAN_STRIPPING");
Expand All @@ -525,7 +539,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);
ret = iavf_execute_vf_cmd_safe(adapter, &args);
if (ret)
PMD_DRV_LOG(ERR, "Failed to execute command of"
" OP_DISABLE_VLAN_STRIPPING");
Expand Down Expand Up @@ -554,7 +568,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err) {
PMD_INIT_LOG(ERR, "Fail to execute command of OP_VERSION");
return err;
Expand Down Expand Up @@ -609,7 +623,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);

if (err) {
PMD_DRV_LOG(ERR,
Expand Down Expand Up @@ -654,7 +668,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);
ret = iavf_execute_vf_cmd_safe(adapter, &args);
if (ret) {
PMD_DRV_LOG(ERR,
"Failed to execute command of OP_GET_SUPPORTED_RXDIDS");
Expand Down Expand Up @@ -686,7 +700,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err) {
PMD_DRV_LOG(ERR,
"Failed to execute command of OP_ENABLE_QUEUES");
Expand Down Expand Up @@ -714,7 +728,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err) {
PMD_DRV_LOG(ERR,
"Failed to execute command of OP_DISABLE_QUEUES");
Expand Down Expand Up @@ -747,7 +761,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err)
PMD_DRV_LOG(ERR, "Failed to execute command of %s",
on ? "OP_ENABLE_QUEUES" : "OP_DISABLE_QUEUES");
Expand Down Expand Up @@ -789,7 +803,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err)
PMD_DRV_LOG(ERR,
"Failed to execute command of OP_ENABLE_QUEUES_V2");
Expand Down Expand Up @@ -833,7 +847,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err)
PMD_DRV_LOG(ERR,
"Failed to execute command of OP_DISABLE_QUEUES_V2");
Expand Down Expand Up @@ -879,7 +893,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err)
PMD_DRV_LOG(ERR, "Failed to execute command of %s",
on ? "OP_ENABLE_QUEUES_V2" : "OP_DISABLE_QUEUES_V2");
Expand Down Expand Up @@ -911,7 +925,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err)
PMD_DRV_LOG(ERR,
"Failed to execute command of OP_CONFIG_RSS_LUT");
Expand Down Expand Up @@ -943,7 +957,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err)
PMD_DRV_LOG(ERR,
"Failed to execute command of OP_CONFIG_RSS_KEY");
Expand Down Expand Up @@ -1035,7 +1049,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err)
PMD_DRV_LOG(ERR, "Failed to execute command of"
" VIRTCHNL_OP_CONFIG_VSI_QUEUES");
Expand Down Expand Up @@ -1076,7 +1090,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err)
PMD_DRV_LOG(ERR, "fail to execute command OP_CONFIG_IRQ_MAP");

Expand Down Expand Up @@ -1117,7 +1131,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err)
PMD_DRV_LOG(ERR, "fail to execute command OP_MAP_QUEUE_VECTOR");

Expand Down Expand Up @@ -1179,7 +1193,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err)
PMD_DRV_LOG(ERR, "fail to execute command %s",
add ? "OP_ADD_ETHER_ADDRESS" :
Expand All @@ -1206,7 +1220,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err) {
PMD_DRV_LOG(ERR, "fail to execute command OP_GET_STATS");
*pstats = NULL;
Expand Down Expand Up @@ -1241,7 +1255,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);

if (err) {
PMD_DRV_LOG(ERR,
Expand Down Expand Up @@ -1281,7 +1295,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err)
PMD_DRV_LOG(ERR, "fail to execute command %s",
add ? "OP_ADD_ETH_ADDR" : "OP_DEL_ETH_ADDR");
Expand All @@ -1308,7 +1322,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err)
PMD_DRV_LOG(ERR, "fail to execute command %s",
add ? "OP_ADD_VLAN" : "OP_DEL_VLAN");
Expand All @@ -1335,7 +1349,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err) {
PMD_DRV_LOG(ERR, "fail to execute command OP_ADD_FDIR_FILTER");
return err;
Expand Down Expand Up @@ -1395,7 +1409,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err) {
PMD_DRV_LOG(ERR, "fail to execute command OP_DEL_FDIR_FILTER");
return err;
Expand Down Expand Up @@ -1442,7 +1456,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err) {
PMD_DRV_LOG(ERR, "fail to check flow director rule");
return err;
Expand Down Expand Up @@ -1483,7 +1497,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err)
PMD_DRV_LOG(ERR,
"Failed to execute command of %s",
Expand Down Expand Up @@ -1536,7 +1550,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);

if (err) {
PMD_DRV_LOG(ERR, "fail to execute command %s",
Expand Down Expand Up @@ -1580,10 +1594,14 @@ 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.
*
* 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);
rte_intr_enable(&pci_dev->intr_handle);
rte_spinlock_unlock(&vf->aq_lock);
if (err) {
PMD_DRV_LOG(ERR, "fail to execute command OP_REQUEST_QUEUES");
return err;
Expand Down Expand Up @@ -1618,7 +1636,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);
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err) {
PMD_DRV_LOG(ERR, "Failed to execute command of VIRTCHNL_OP_GET_MAX_RSS_QREGION");
return err;
Expand Down

0 comments on commit a44e427

Please sign in to comment.