From df830543d63c9a8e4594d92c0c1ffb48c240947f Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Wed, 16 Feb 2022 13:37:28 -0800 Subject: [PATCH 01/11] ice: refactor unwind cleanup in eswitch mode The code for supporting eswitch mode and port representors on VFs uses an unwind based cleanup flow when handling errors. These flows are used to cleanup and get everything back to the state prior to attempting to switch from legacy to representor mode or back. The unwind iterations make sense, but complicate a plan to refactor the VF array structure. In the future we won't have a clean method of reversing an iteration of the VFs. Instead, we can change the cleanup flow to just iterate over all VF structures and clean up appropriately. First notice that ice_repr_add_for_all_vfs and ice_repr_rem_from_all_vfs have an additional step of re-assigning the VC ops. There is no good reason to do this outside of ice_repr_add and ice_repr_rem. It can simply be done as the last step of these functions. Second, make sure ice_repr_rem is safe to call on a VF which does not have a representor. Check if vf->repr is NULL first and exit early if so. Move ice_repr_rem_from_all_vfs above ice_repr_add_for_all_vfs so that we can call it from the cleanup function. In ice_eswitch.c, replace the unwind iteration with a call to ice_eswitch_release_reprs. This will go through all of the VFs and revert the VF back to the standard model without the eswitch mode. To make this safe, ensure this function checks whether or not the represent or has been moved. Rely on the metadata destination in vf->repr->dst. This must be NULL if the representor has not been moved to eswitch mode. Ensure that we always re-assign this value back to NULL after freeing it, and move the ice_eswitch_release_reprs so that it can be called from the setup function. With these changes, eswitch cleanup no longer uses an unwind flow that is problematic for the planned VF data structure change. Signed-off-by: Jacob Keller Tested-by: Sandeep Penigalapati Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_eswitch.c | 63 ++++++++++---------- drivers/net/ethernet/intel/ice/ice_repr.c | 45 +++++++------- 2 files changed, 53 insertions(+), 55 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c index 95c81fc9ec9f1..22babf59d40bf 100644 --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c @@ -202,6 +202,34 @@ static void ice_eswitch_remap_rings_to_vectors(struct ice_pf *pf) } } +/** + * ice_eswitch_release_reprs - clear PR VSIs configuration + * @pf: poiner to PF struct + * @ctrl_vsi: pointer to switchdev control VSI + */ +static void +ice_eswitch_release_reprs(struct ice_pf *pf, struct ice_vsi *ctrl_vsi) +{ + int i; + + ice_for_each_vf(pf, i) { + struct ice_vsi *vsi = pf->vf[i].repr->src_vsi; + struct ice_vf *vf = &pf->vf[i]; + + /* Skip VFs that aren't configured */ + if (!vf->repr->dst) + continue; + + ice_vsi_update_security(vsi, ice_vsi_ctx_set_antispoof); + metadata_dst_free(vf->repr->dst); + vf->repr->dst = NULL; + ice_fltr_add_mac_and_broadcast(vsi, vf->hw_lan_addr.addr, + ICE_FWD_TO_VSI); + + netif_napi_del(&vf->repr->q_vector->napi); + } +} + /** * ice_eswitch_setup_reprs - configure port reprs to run in switchdev mode * @pf: pointer to PF struct @@ -231,6 +259,7 @@ static int ice_eswitch_setup_reprs(struct ice_pf *pf) vf->hw_lan_addr.addr, ICE_FWD_TO_VSI); metadata_dst_free(vf->repr->dst); + vf->repr->dst = NULL; goto err; } @@ -239,6 +268,7 @@ static int ice_eswitch_setup_reprs(struct ice_pf *pf) vf->hw_lan_addr.addr, ICE_FWD_TO_VSI); metadata_dst_free(vf->repr->dst); + vf->repr->dst = NULL; ice_vsi_update_security(vsi, ice_vsi_ctx_set_antispoof); goto err; } @@ -266,42 +296,11 @@ static int ice_eswitch_setup_reprs(struct ice_pf *pf) return 0; err: - for (i = i - 1; i >= 0; i--) { - struct ice_vsi *vsi = pf->vf[i].repr->src_vsi; - struct ice_vf *vf = &pf->vf[i]; - - ice_vsi_update_security(vsi, ice_vsi_ctx_set_antispoof); - metadata_dst_free(vf->repr->dst); - ice_fltr_add_mac_and_broadcast(vsi, vf->hw_lan_addr.addr, - ICE_FWD_TO_VSI); - } + ice_eswitch_release_reprs(pf, ctrl_vsi); return -ENODEV; } -/** - * ice_eswitch_release_reprs - clear PR VSIs configuration - * @pf: poiner to PF struct - * @ctrl_vsi: pointer to switchdev control VSI - */ -static void -ice_eswitch_release_reprs(struct ice_pf *pf, struct ice_vsi *ctrl_vsi) -{ - int i; - - ice_for_each_vf(pf, i) { - struct ice_vsi *vsi = pf->vf[i].repr->src_vsi; - struct ice_vf *vf = &pf->vf[i]; - - ice_vsi_update_security(vsi, ice_vsi_ctx_set_antispoof); - metadata_dst_free(vf->repr->dst); - ice_fltr_add_mac_and_broadcast(vsi, vf->hw_lan_addr.addr, - ICE_FWD_TO_VSI); - - netif_napi_del(&vf->repr->q_vector->napi); - } -} - /** * ice_eswitch_update_repr - reconfigure VF port representor * @vsi: VF VSI for which port representor is configured diff --git a/drivers/net/ethernet/intel/ice/ice_repr.c b/drivers/net/ethernet/intel/ice/ice_repr.c index dcc310e293005..787f51c8ddb27 100644 --- a/drivers/net/ethernet/intel/ice/ice_repr.c +++ b/drivers/net/ethernet/intel/ice/ice_repr.c @@ -284,6 +284,8 @@ static int ice_repr_add(struct ice_vf *vf) devlink_port_type_eth_set(&vf->devlink_port, repr->netdev); + ice_vc_change_ops_to_repr(&vf->vc_ops); + return 0; err_netdev: @@ -311,6 +313,9 @@ static int ice_repr_add(struct ice_vf *vf) */ static void ice_repr_rem(struct ice_vf *vf) { + if (!vf->repr) + return; + ice_devlink_destroy_vf_port(vf); kfree(vf->repr->q_vector); vf->repr->q_vector = NULL; @@ -323,54 +328,48 @@ static void ice_repr_rem(struct ice_vf *vf) #endif kfree(vf->repr); vf->repr = NULL; + + ice_vc_set_dflt_vf_ops(&vf->vc_ops); } /** - * ice_repr_add_for_all_vfs - add port representor for all VFs + * ice_repr_rem_from_all_vfs - remove port representor for all VFs * @pf: pointer to PF structure */ -int ice_repr_add_for_all_vfs(struct ice_pf *pf) +void ice_repr_rem_from_all_vfs(struct ice_pf *pf) { - int err; int i; ice_for_each_vf(pf, i) { struct ice_vf *vf = &pf->vf[i]; - err = ice_repr_add(vf); - if (err) - goto err; - - ice_vc_change_ops_to_repr(&vf->vc_ops); - } - - return 0; - -err: - for (i = i - 1; i >= 0; i--) { - struct ice_vf *vf = &pf->vf[i]; - ice_repr_rem(vf); - ice_vc_set_dflt_vf_ops(&vf->vc_ops); } - - return err; } /** - * ice_repr_rem_from_all_vfs - remove port representor for all VFs + * ice_repr_add_for_all_vfs - add port representor for all VFs * @pf: pointer to PF structure */ -void ice_repr_rem_from_all_vfs(struct ice_pf *pf) +int ice_repr_add_for_all_vfs(struct ice_pf *pf) { + int err; int i; ice_for_each_vf(pf, i) { struct ice_vf *vf = &pf->vf[i]; - ice_repr_rem(vf); - ice_vc_set_dflt_vf_ops(&vf->vc_ops); + err = ice_repr_add(vf); + if (err) + goto err; } + + return 0; + +err: + ice_repr_rem_from_all_vfs(pf); + + return err; } /** From b03d519d3460f3aaf8b5afef582dd98466925352 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Wed, 16 Feb 2022 13:37:29 -0800 Subject: [PATCH 02/11] ice: store VF pointer instead of VF ID The VSI structure contains a vf_id field used to associate a VSI with a VF. This is used mainly for ICE_VSI_VF as well as partially for ICE_VSI_CTRL associated with the VFs. This API was designed with the idea that VFs are stored in a simple array that was expected to be static throughout most of the driver's life. We plan on refactoring VF storage in a few key ways: 1) converting from a simple static array to a hash table 2) using krefs to track VF references obtained from the hash table 3) use RCU to delay release of VF memory until after all references are dropped This is motivated by the goal to ensure that the lifetime of VF structures is accounted for, and prevent various use-after-free bugs. With the existing vsi->vf_id, the reference tracking for VFs would become somewhat convoluted, because each VSI maintains a vf_id field which will then require performing a look up. This means all these flows will require reference tracking and proper usage of rcu_read_lock, etc. We know that the VF VSI will always be backed by a valid VF structure, because the VSI is created during VF initialization and removed before the VF is destroyed. Rely on this and store a reference to the VF in the VSI structure instead of storing a VF ID. This will simplify the usage and avoid the need to perform lookups on the hash table in the future. For ICE_VSI_VF, it is expected that vsi->vf is always non-NULL after ice_vsi_alloc succeeds. Because of this, use WARN_ON when checking if a vsi->vf pointer is valid when dealing with VF VSIs. This will aid in debugging code which violates this assumption and avoid more disastrous panics. Signed-off-by: Jacob Keller Tested-by: Konrad Jankowski Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice.h | 3 +- drivers/net/ethernet/intel/ice/ice_base.c | 4 +- drivers/net/ethernet/intel/ice/ice_eswitch.c | 7 +- drivers/net/ethernet/intel/ice/ice_lib.c | 178 +++++++++++------- drivers/net/ethernet/intel/ice/ice_lib.h | 3 +- drivers/net/ethernet/intel/ice/ice_main.c | 10 +- drivers/net/ethernet/intel/ice/ice_txrx.c | 2 +- .../ethernet/intel/ice/ice_vf_vsi_vlan_ops.c | 19 +- .../ethernet/intel/ice/ice_virtchnl_fdir.c | 5 +- .../net/ethernet/intel/ice/ice_virtchnl_pf.c | 6 +- 10 files changed, 142 insertions(+), 95 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h index 1ca1273a7cd37..56944b003c128 100644 --- a/drivers/net/ethernet/intel/ice/ice.h +++ b/drivers/net/ethernet/intel/ice/ice.h @@ -109,7 +109,6 @@ /* All VF control VSIs share the same IRQ, so assign a unique ID for them */ #define ICE_RES_VF_CTRL_VEC_ID (ICE_RES_RDMA_VEC_ID - 1) #define ICE_INVAL_Q_INDEX 0xffff -#define ICE_INVAL_VFID 256 #define ICE_MAX_RXQS_PER_TC 256 /* Used when setting VSI context per TC Rx queues */ @@ -333,7 +332,7 @@ struct ice_vsi { u16 vsi_num; /* HW (absolute) index of this VSI */ u16 idx; /* software index in pf->vsi[] */ - s16 vf_id; /* VF ID for SR-IOV VSIs */ + struct ice_vf *vf; /* VF associated with this VSI */ u16 ethtype; /* Ethernet protocol for pause frame */ u16 num_gfltr; diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c index 2360e6abdb1e2..a3094470d31de 100644 --- a/drivers/net/ethernet/intel/ice/ice_base.c +++ b/drivers/net/ethernet/intel/ice/ice_base.c @@ -323,7 +323,7 @@ ice_setup_tx_ctx(struct ice_tx_ring *ring, struct ice_tlan_ctx *tlan_ctx, u16 pf break; case ICE_VSI_VF: /* Firmware expects vmvf_num to be absolute VF ID */ - tlan_ctx->vmvf_num = hw->func_caps.vf_base_id + vsi->vf_id; + tlan_ctx->vmvf_num = hw->func_caps.vf_base_id + vsi->vf->vf_id; tlan_ctx->vmvf_type = ICE_TLAN_CTX_VMVF_TYPE_VF; break; case ICE_VSI_SWITCHDEV_CTRL: @@ -429,7 +429,7 @@ static int ice_setup_rx_ctx(struct ice_rx_ring *ring) */ if (ice_is_dvm_ena(hw)) if (vsi->type == ICE_VSI_VF && - ice_vf_is_port_vlan_ena(&vsi->back->vf[vsi->vf_id])) + ice_vf_is_port_vlan_ena(vsi->vf)) rlan_ctx.l2tsel = 1; else rlan_ctx.l2tsel = 0; diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c index 22babf59d40bf..e47331e363ea8 100644 --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c @@ -315,7 +315,7 @@ void ice_eswitch_update_repr(struct ice_vsi *vsi) if (!ice_is_switchdev_running(pf)) return; - vf = &pf->vf[vsi->vf_id]; + vf = vsi->vf; repr = vf->repr; repr->src_vsi = vsi; repr->dst->u.port_info.port_id = vsi->vsi_num; @@ -323,7 +323,8 @@ void ice_eswitch_update_repr(struct ice_vsi *vsi) ret = ice_vsi_update_security(vsi, ice_vsi_ctx_clear_antispoof); if (ret) { ice_fltr_add_mac_and_broadcast(vsi, vf->hw_lan_addr.addr, ICE_FWD_TO_VSI); - dev_err(ice_pf_to_dev(pf), "Failed to update VF %d port representor", vsi->vf_id); + dev_err(ice_pf_to_dev(pf), "Failed to update VF %d port representor", + vsi->vf->vf_id); } } @@ -407,7 +408,7 @@ static void ice_eswitch_release_env(struct ice_pf *pf) static struct ice_vsi * ice_eswitch_vsi_setup(struct ice_pf *pf, struct ice_port_info *pi) { - return ice_vsi_setup(pf, pi, ICE_VSI_SWITCHDEV_CTRL, ICE_INVAL_VFID, NULL); + return ice_vsi_setup(pf, pi, ICE_VSI_SWITCHDEV_CTRL, NULL, NULL); } /** diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c index 662493d1002b0..5af36311a4042 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_lib.c @@ -166,21 +166,19 @@ static void ice_vsi_set_num_desc(struct ice_vsi *vsi) /** * ice_vsi_set_num_qs - Set number of queues, descriptors and vectors for a VSI * @vsi: the VSI being configured - * @vf_id: ID of the VF being configured + * @vf: the VF associated with this VSI, if any * * Return 0 on success and a negative value on error */ -static void ice_vsi_set_num_qs(struct ice_vsi *vsi, u16 vf_id) +static void ice_vsi_set_num_qs(struct ice_vsi *vsi, struct ice_vf *vf) { + enum ice_vsi_type vsi_type = vsi->type; struct ice_pf *pf = vsi->back; - struct ice_vf *vf = NULL; - if (vsi->type == ICE_VSI_VF) - vsi->vf_id = vf_id; - else - vsi->vf_id = ICE_INVAL_VFID; + if (WARN_ON(vsi_type == ICE_VSI_VF && !vf)) + return; - switch (vsi->type) { + switch (vsi_type) { case ICE_VSI_PF: if (vsi->req_txq) { vsi->alloc_txq = vsi->req_txq; @@ -222,7 +220,6 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, u16 vf_id) vsi->num_q_vectors = 1; break; case ICE_VSI_VF: - vf = &pf->vf[vsi->vf_id]; if (vf->num_req_qs) vf->num_vf_qs = vf->num_req_qs; vsi->alloc_txq = vf->num_vf_qs; @@ -248,7 +245,7 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, u16 vf_id) vsi->alloc_rxq = 1; break; default: - dev_warn(ice_pf_to_dev(pf), "Unknown VSI type %d\n", vsi->type); + dev_warn(ice_pf_to_dev(pf), "Unknown VSI type %d\n", vsi_type); break; } @@ -299,7 +296,7 @@ void ice_vsi_delete(struct ice_vsi *vsi) return; if (vsi->type == ICE_VSI_VF) - ctxt->vf_num = vsi->vf_id; + ctxt->vf_num = vsi->vf->vf_id; ctxt->vsi_num = vsi->vsi_num; memcpy(&ctxt->info, &vsi->info, sizeof(ctxt->info)); @@ -384,8 +381,7 @@ int ice_vsi_clear(struct ice_vsi *vsi) pf->vsi[vsi->idx] = NULL; if (vsi->idx < pf->next_vsi && vsi->type != ICE_VSI_CTRL) pf->next_vsi = vsi->idx; - if (vsi->idx < pf->next_vsi && vsi->type == ICE_VSI_CTRL && - vsi->vf_id != ICE_INVAL_VFID) + if (vsi->idx < pf->next_vsi && vsi->type == ICE_VSI_CTRL && vsi->vf) pf->next_vsi = vsi->idx; ice_vsi_free_arrays(vsi); @@ -453,17 +449,24 @@ static irqreturn_t ice_eswitch_msix_clean_rings(int __always_unused irq, void *d * @pf: board private structure * @vsi_type: type of VSI * @ch: ptr to channel - * @vf_id: ID of the VF being configured + * @vf: VF for ICE_VSI_VF and ICE_VSI_CTRL + * + * The VF pointer is used for ICE_VSI_VF and ICE_VSI_CTRL. For ICE_VSI_CTRL, + * it may be NULL in the case there is no association with a VF. For + * ICE_VSI_VF the VF pointer *must not* be NULL. * * returns a pointer to a VSI on success, NULL on failure. */ static struct ice_vsi * ice_vsi_alloc(struct ice_pf *pf, enum ice_vsi_type vsi_type, - struct ice_channel *ch, u16 vf_id) + struct ice_channel *ch, struct ice_vf *vf) { struct device *dev = ice_pf_to_dev(pf); struct ice_vsi *vsi = NULL; + if (WARN_ON(vsi_type == ICE_VSI_VF && !vf)) + return NULL; + /* Need to protect the allocation of the VSIs at the PF level */ mutex_lock(&pf->sw_mutex); @@ -485,9 +488,9 @@ ice_vsi_alloc(struct ice_pf *pf, enum ice_vsi_type vsi_type, set_bit(ICE_VSI_DOWN, vsi->state); if (vsi_type == ICE_VSI_VF) - ice_vsi_set_num_qs(vsi, vf_id); + ice_vsi_set_num_qs(vsi, vf); else if (vsi_type != ICE_VSI_CHNL) - ice_vsi_set_num_qs(vsi, ICE_INVAL_VFID); + ice_vsi_set_num_qs(vsi, NULL); switch (vsi->type) { case ICE_VSI_SWITCHDEV_CTRL: @@ -510,10 +513,16 @@ ice_vsi_alloc(struct ice_pf *pf, enum ice_vsi_type vsi_type, /* Setup ctrl VSI MSIX irq handler */ vsi->irq_handler = ice_msix_clean_ctrl_vsi; + + /* For the PF control VSI this is NULL, for the VF control VSI + * this will be the first VF to allocate it. + */ + vsi->vf = vf; break; case ICE_VSI_VF: if (ice_vsi_alloc_arrays(vsi)) goto err_rings; + vsi->vf = vf; break; case ICE_VSI_CHNL: if (!ch) @@ -531,7 +540,7 @@ ice_vsi_alloc(struct ice_pf *pf, enum ice_vsi_type vsi_type, goto unlock_pf; } - if (vsi->type == ICE_VSI_CTRL && vf_id == ICE_INVAL_VFID) { + if (vsi->type == ICE_VSI_CTRL && !vf) { /* Use the last VSI slot as the index for PF control VSI */ vsi->idx = pf->num_alloc_vsi - 1; pf->ctrl_vsi_idx = vsi->idx; @@ -546,8 +555,8 @@ ice_vsi_alloc(struct ice_pf *pf, enum ice_vsi_type vsi_type, pf->next_vsi); } - if (vsi->type == ICE_VSI_CTRL && vf_id != ICE_INVAL_VFID) - pf->vf[vf_id].ctrl_vsi_idx = vsi->idx; + if (vsi->type == ICE_VSI_CTRL && vf) + vf->ctrl_vsi_idx = vsi->idx; goto unlock_pf; err_rings: @@ -1130,7 +1139,7 @@ static int ice_vsi_init(struct ice_vsi *vsi, bool init_vsi) case ICE_VSI_VF: ctxt->flags = ICE_AQ_VSI_TYPE_VF; /* VF number here is the absolute VF number (0-255) */ - ctxt->vf_num = vsi->vf_id + hw->func_caps.vf_base_id; + ctxt->vf_num = vsi->vf->vf_id + hw->func_caps.vf_base_id; break; default: ret = -ENODEV; @@ -1321,6 +1330,31 @@ ice_get_res(struct ice_pf *pf, struct ice_res_tracker *res, u16 needed, u16 id) return ice_search_res(res, needed, id); } +/** + * ice_get_vf_ctrl_res - Get VF control VSI resource + * @pf: pointer to the PF structure + * @vsi: the VSI to allocate a resource for + * + * Look up whether another VF has already allocated the control VSI resource. + * If so, re-use this resource so that we share it among all VFs. + * + * Otherwise, allocate the resource and return it. + */ +static int ice_get_vf_ctrl_res(struct ice_pf *pf, struct ice_vsi *vsi) +{ + int i; + + ice_for_each_vf(pf, i) { + struct ice_vf *vf = &pf->vf[i]; + + if (vf != vsi->vf && vf->ctrl_vsi_idx != ICE_NO_VSI) + return pf->vsi[vf->ctrl_vsi_idx]->base_vector; + } + + return ice_get_res(pf, pf->irq_tracker, vsi->num_q_vectors, + ICE_RES_VF_CTRL_VEC_ID); +} + /** * ice_vsi_setup_vector_base - Set up the base vector for the given VSI * @vsi: ptr to the VSI @@ -1353,20 +1387,8 @@ static int ice_vsi_setup_vector_base(struct ice_vsi *vsi) num_q_vectors = vsi->num_q_vectors; /* reserve slots from OS requested IRQs */ - if (vsi->type == ICE_VSI_CTRL && vsi->vf_id != ICE_INVAL_VFID) { - int i; - - ice_for_each_vf(pf, i) { - struct ice_vf *vf = &pf->vf[i]; - - if (i != vsi->vf_id && vf->ctrl_vsi_idx != ICE_NO_VSI) { - base = pf->vsi[vf->ctrl_vsi_idx]->base_vector; - break; - } - } - if (i == pf->num_alloc_vfs) - base = ice_get_res(pf, pf->irq_tracker, num_q_vectors, - ICE_RES_VF_CTRL_VEC_ID); + if (vsi->type == ICE_VSI_CTRL && vsi->vf) { + base = ice_get_vf_ctrl_res(pf, vsi); } else { base = ice_get_res(pf, pf->irq_tracker, num_q_vectors, vsi->idx); @@ -2218,7 +2240,7 @@ ice_vsi_set_q_vectors_reg_idx(struct ice_vsi *vsi) } if (vsi->type == ICE_VSI_VF) { - struct ice_vf *vf = &vsi->back->vf[vsi->vf_id]; + struct ice_vf *vf = vsi->vf; q_vector->reg_idx = ice_calc_vf_reg_idx(vf, q_vector); } else { @@ -2403,9 +2425,8 @@ static void ice_set_agg_vsi(struct ice_vsi *vsi) * @pf: board private structure * @pi: pointer to the port_info instance * @vsi_type: VSI type - * @vf_id: defines VF ID to which this VSI connects. This field is meant to be - * used only for ICE_VSI_VF VSI type. For other VSI types, should - * fill-in ICE_INVAL_VFID as input. + * @vf: pointer to VF to which this VSI connects. This field is used primarily + * for the ICE_VSI_VF type. Other VSI types should pass NULL. * @ch: ptr to channel * * This allocates the sw VSI structure and its queue resources. @@ -2415,7 +2436,8 @@ static void ice_set_agg_vsi(struct ice_vsi *vsi) */ struct ice_vsi * ice_vsi_setup(struct ice_pf *pf, struct ice_port_info *pi, - enum ice_vsi_type vsi_type, u16 vf_id, struct ice_channel *ch) + enum ice_vsi_type vsi_type, struct ice_vf *vf, + struct ice_channel *ch) { u16 max_txqs[ICE_MAX_TRAFFIC_CLASS] = { 0 }; struct device *dev = ice_pf_to_dev(pf); @@ -2423,11 +2445,11 @@ ice_vsi_setup(struct ice_pf *pf, struct ice_port_info *pi, int ret, i; if (vsi_type == ICE_VSI_CHNL) - vsi = ice_vsi_alloc(pf, vsi_type, ch, ICE_INVAL_VFID); + vsi = ice_vsi_alloc(pf, vsi_type, ch, NULL); else if (vsi_type == ICE_VSI_VF || vsi_type == ICE_VSI_CTRL) - vsi = ice_vsi_alloc(pf, vsi_type, NULL, vf_id); + vsi = ice_vsi_alloc(pf, vsi_type, NULL, vf); else - vsi = ice_vsi_alloc(pf, vsi_type, NULL, ICE_INVAL_VFID); + vsi = ice_vsi_alloc(pf, vsi_type, NULL, NULL); if (!vsi) { dev_err(dev, "could not allocate VSI\n"); @@ -2439,9 +2461,6 @@ ice_vsi_setup(struct ice_pf *pf, struct ice_port_info *pi, if (vsi->type == ICE_VSI_PF) vsi->ethtype = ETH_P_PAUSE; - if (vsi->type == ICE_VSI_VF || vsi->type == ICE_VSI_CTRL) - vsi->vf_id = vf_id; - ice_alloc_fd_res(vsi); if (vsi_type != ICE_VSI_CHNL) { @@ -2861,6 +2880,34 @@ void ice_napi_del(struct ice_vsi *vsi) netif_napi_del(&vsi->q_vectors[v_idx]->napi); } +/** + * ice_free_vf_ctrl_res - Free the VF control VSI resource + * @pf: pointer to PF structure + * @vsi: the VSI to free resources for + * + * Check if the VF control VSI resource is still in use. If no VF is using it + * any more, release the VSI resource. Otherwise, leave it to be cleaned up + * once no other VF uses it. + */ +static void ice_free_vf_ctrl_res(struct ice_pf *pf, struct ice_vsi *vsi) +{ + int i; + + ice_for_each_vf(pf, i) { + struct ice_vf *vf = &pf->vf[i]; + + if (vf != vsi->vf && vf->ctrl_vsi_idx != ICE_NO_VSI) + return; + } + + /* No other VFs left that have control VSI. It is now safe to reclaim + * SW interrupts back to the common pool. + */ + ice_free_res(pf->irq_tracker, vsi->base_vector, + ICE_RES_VF_CTRL_VEC_ID); + pf->num_avail_sw_msix += vsi->num_q_vectors; +} + /** * ice_vsi_release - Delete a VSI and free its resources * @vsi: the VSI being removed @@ -2904,23 +2951,8 @@ int ice_vsi_release(struct ice_vsi *vsi) * many interrupts each VF needs. SR-IOV MSIX resources are also * cleared in the same manner. */ - if (vsi->type == ICE_VSI_CTRL && vsi->vf_id != ICE_INVAL_VFID) { - int i; - - ice_for_each_vf(pf, i) { - struct ice_vf *vf = &pf->vf[i]; - - if (i != vsi->vf_id && vf->ctrl_vsi_idx != ICE_NO_VSI) - break; - } - if (i == pf->num_alloc_vfs) { - /* No other VFs left that have control VSI, reclaim SW - * interrupts back to the common pool - */ - ice_free_res(pf->irq_tracker, vsi->base_vector, - ICE_RES_VF_CTRL_VEC_ID); - pf->num_avail_sw_msix += vsi->num_q_vectors; - } + if (vsi->type == ICE_VSI_CTRL && vsi->vf) { + ice_free_vf_ctrl_res(pf, vsi); } else if (vsi->type != ICE_VSI_VF) { /* reclaim SW interrupts back to the common pool */ ice_free_res(pf->irq_tracker, vsi->base_vector, vsi->idx); @@ -3104,7 +3136,6 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, bool init_vsi) u16 max_txqs[ICE_MAX_TRAFFIC_CLASS] = { 0 }; struct ice_coalesce_stored *coalesce; int prev_num_q_vectors = 0; - struct ice_vf *vf = NULL; enum ice_vsi_type vtype; struct ice_pf *pf; int ret, i; @@ -3114,8 +3145,8 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, bool init_vsi) pf = vsi->back; vtype = vsi->type; - if (vtype == ICE_VSI_VF) - vf = &pf->vf[vsi->vf_id]; + if (WARN_ON(vtype == ICE_VSI_VF) && !vsi->vf) + return -EINVAL; ice_vsi_init_vlan_ops(vsi); @@ -3154,9 +3185,9 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, bool init_vsi) ice_vsi_clear_rings(vsi); ice_vsi_free_arrays(vsi); if (vtype == ICE_VSI_VF) - ice_vsi_set_num_qs(vsi, vf->vf_id); + ice_vsi_set_num_qs(vsi, vsi->vf); else - ice_vsi_set_num_qs(vsi, ICE_INVAL_VFID); + ice_vsi_set_num_qs(vsi, NULL); ret = ice_vsi_alloc_arrays(vsi); if (ret < 0) @@ -4013,9 +4044,14 @@ static u16 ice_vsi_num_zero_vlans(struct ice_vsi *vsi) #define ICE_DVM_NUM_ZERO_VLAN_FLTRS 2 #define ICE_SVM_NUM_ZERO_VLAN_FLTRS 1 /* no VLAN 0 filter is created when a port VLAN is active */ - if (vsi->type == ICE_VSI_VF && - ice_vf_is_port_vlan_ena(&vsi->back->vf[vsi->vf_id])) - return 0; + if (vsi->type == ICE_VSI_VF) { + if (WARN_ON(!vsi->vf)) + return 0; + + if (ice_vf_is_port_vlan_ena(vsi->vf)) + return 0; + } + if (ice_is_dvm_ena(&vsi->back->hw)) return ICE_DVM_NUM_ZERO_VLAN_FLTRS; else diff --git a/drivers/net/ethernet/intel/ice/ice_lib.h b/drivers/net/ethernet/intel/ice/ice_lib.h index 491f13f98797f..0095329949d4b 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.h +++ b/drivers/net/ethernet/intel/ice/ice_lib.h @@ -52,7 +52,8 @@ void ice_vsi_cfg_netdev_tc(struct ice_vsi *vsi, u8 ena_tc); struct ice_vsi * ice_vsi_setup(struct ice_pf *pf, struct ice_port_info *pi, - enum ice_vsi_type vsi_type, u16 vf_id, struct ice_channel *ch); + enum ice_vsi_type vsi_type, struct ice_vf *vf, + struct ice_channel *ch); void ice_napi_del(struct ice_vsi *vsi); diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index 95dfd04e8c3ab..b80e9be3167c7 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -2439,7 +2439,7 @@ static int ice_vsi_req_irq_msix(struct ice_vsi *vsi, char *basename) /* skip this unused q_vector */ continue; } - if (vsi->type == ICE_VSI_CTRL && vsi->vf_id != ICE_INVAL_VFID) + if (vsi->type == ICE_VSI_CTRL && vsi->vf) err = devm_request_irq(dev, irq_num, vsi->irq_handler, IRQF_SHARED, q_vector->name, q_vector); @@ -3386,14 +3386,14 @@ void ice_fill_rss_lut(u8 *lut, u16 rss_table_size, u16 rss_size) static struct ice_vsi * ice_pf_vsi_setup(struct ice_pf *pf, struct ice_port_info *pi) { - return ice_vsi_setup(pf, pi, ICE_VSI_PF, ICE_INVAL_VFID, NULL); + return ice_vsi_setup(pf, pi, ICE_VSI_PF, NULL, NULL); } static struct ice_vsi * ice_chnl_vsi_setup(struct ice_pf *pf, struct ice_port_info *pi, struct ice_channel *ch) { - return ice_vsi_setup(pf, pi, ICE_VSI_CHNL, ICE_INVAL_VFID, ch); + return ice_vsi_setup(pf, pi, ICE_VSI_CHNL, NULL, ch); } /** @@ -3407,7 +3407,7 @@ ice_chnl_vsi_setup(struct ice_pf *pf, struct ice_port_info *pi, static struct ice_vsi * ice_ctrl_vsi_setup(struct ice_pf *pf, struct ice_port_info *pi) { - return ice_vsi_setup(pf, pi, ICE_VSI_CTRL, ICE_INVAL_VFID, NULL); + return ice_vsi_setup(pf, pi, ICE_VSI_CTRL, NULL, NULL); } /** @@ -3421,7 +3421,7 @@ ice_ctrl_vsi_setup(struct ice_pf *pf, struct ice_port_info *pi) struct ice_vsi * ice_lb_vsi_setup(struct ice_pf *pf, struct ice_port_info *pi) { - return ice_vsi_setup(pf, pi, ICE_VSI_LB, ICE_INVAL_VFID, NULL); + return ice_vsi_setup(pf, pi, ICE_VSI_LB, NULL, NULL); } /** diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c index ff93ec71aed63..853f57a9589a8 100644 --- a/drivers/net/ethernet/intel/ice/ice_txrx.c +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c @@ -1165,7 +1165,7 @@ int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget) struct ice_vsi *ctrl_vsi = rx_ring->vsi; if (rx_desc->wb.rxdid == FDIR_DESC_RXDID && - ctrl_vsi->vf_id != ICE_INVAL_VFID) + ctrl_vsi->vf) ice_vc_fdir_irq_handler(ctrl_vsi, rx_desc); ice_put_rx_buf(rx_ring, NULL, 0); cleaned_count++; diff --git a/drivers/net/ethernet/intel/ice/ice_vf_vsi_vlan_ops.c b/drivers/net/ethernet/intel/ice/ice_vf_vsi_vlan_ops.c index 39f2d36cabba3..b16f946185f24 100644 --- a/drivers/net/ethernet/intel/ice/ice_vf_vsi_vlan_ops.c +++ b/drivers/net/ethernet/intel/ice/ice_vf_vsi_vlan_ops.c @@ -34,9 +34,10 @@ void ice_vf_vsi_init_vlan_ops(struct ice_vsi *vsi) { struct ice_vsi_vlan_ops *vlan_ops; struct ice_pf *pf = vsi->back; - struct ice_vf *vf; + struct ice_vf *vf = vsi->vf; - vf = &pf->vf[vsi->vf_id]; + if (WARN_ON(!vf)) + return; if (ice_is_dvm_ena(&pf->hw)) { vlan_ops = &vsi->outer_vlan_ops; @@ -126,9 +127,14 @@ void ice_vf_vsi_init_vlan_ops(struct ice_vsi *vsi) */ void ice_vf_vsi_cfg_dvm_legacy_vlan_mode(struct ice_vsi *vsi) { - struct ice_vf *vf = &vsi->back->vf[vsi->vf_id]; - struct device *dev = ice_pf_to_dev(vf->pf); struct ice_vsi_vlan_ops *vlan_ops; + struct ice_vf *vf = vsi->vf; + struct device *dev; + + if (WARN_ON(!vf)) + return; + + dev = ice_pf_to_dev(vf->pf); if (!ice_is_dvm_ena(&vsi->back->hw) || ice_vf_is_port_vlan_ena(vf)) return; @@ -192,7 +198,10 @@ void ice_vf_vsi_cfg_dvm_legacy_vlan_mode(struct ice_vsi *vsi) */ void ice_vf_vsi_cfg_svm_legacy_vlan_mode(struct ice_vsi *vsi) { - struct ice_vf *vf = &vsi->back->vf[vsi->vf_id]; + struct ice_vf *vf = vsi->vf; + + if (WARN_ON(!vf)) + return; if (ice_is_dvm_ena(&vsi->back->hw) || ice_vf_is_port_vlan_ena(vf)) return; diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c index d64df81d48935..25b9c1dfc1ac8 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c @@ -1288,15 +1288,16 @@ ice_vc_fdir_irq_handler(struct ice_vsi *ctrl_vsi, union ice_32b_rx_flex_desc *rx_desc) { struct ice_pf *pf = ctrl_vsi->back; + struct ice_vf *vf = ctrl_vsi->vf; struct ice_vf_fdir_ctx *ctx_done; struct ice_vf_fdir_ctx *ctx_irq; struct ice_vf_fdir *fdir; unsigned long flags; struct device *dev; - struct ice_vf *vf; int ret; - vf = &pf->vf[ctrl_vsi->vf_id]; + if (WARN_ON(!vf)) + return; fdir = &vf->fdir; ctx_done = &fdir->ctx_done; diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c index 455ce5dd00315..8955a2873d57f 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c @@ -667,7 +667,7 @@ static struct ice_vsi *ice_vf_vsi_setup(struct ice_vf *vf) struct ice_pf *pf = vf->pf; struct ice_vsi *vsi; - vsi = ice_vsi_setup(pf, pi, ICE_VSI_VF, vf->vf_id, NULL); + vsi = ice_vsi_setup(pf, pi, ICE_VSI_VF, vf, NULL); if (!vsi) { dev_err(ice_pf_to_dev(pf), "Failed to create VF VSI\n"); @@ -694,7 +694,7 @@ struct ice_vsi *ice_vf_ctrl_vsi_setup(struct ice_vf *vf) struct ice_pf *pf = vf->pf; struct ice_vsi *vsi; - vsi = ice_vsi_setup(pf, pi, ICE_VSI_CTRL, vf->vf_id, NULL); + vsi = ice_vsi_setup(pf, pi, ICE_VSI_CTRL, vf, NULL); if (!vsi) { dev_err(ice_pf_to_dev(pf), "Failed to create VF control VSI\n"); ice_vf_ctrl_invalidate_vsi(vf); @@ -2526,7 +2526,7 @@ bool ice_vc_isvalid_vsi_id(struct ice_vf *vf, u16 vsi_id) vsi = ice_find_vsi_from_id(pf, vsi_id); - return (vsi && (vsi->vf_id == vf->vf_id)); + return (vsi && (vsi->vf == vf)); } /** From cd0f4f3b2c048fe99ccd2e0df2c0900408ce8507 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Wed, 16 Feb 2022 13:37:30 -0800 Subject: [PATCH 03/11] ice: pass num_vfs to ice_set_per_vf_res() We are planning to replace the simple array structure tracking VFs with a hash table. This change will also remove the "num_alloc_vfs" variable. Instead, new access functions to use the hash table as the source of truth will be introduced. These will generally be equivalent to existing checks, except during VF initialization. Specifically, ice_set_per_vf_res() cannot use the hash table as it will be operating prior to VF structures being inserted into the hash table. Instead of using pf->num_alloc_vfs, simply pass the num_vfs value in from the caller. Note that a sub-function of ice_set_per_vf_res, ice_determine_res, also implicitly depends on pf->num_alloc_vfs. Replace ice_determine_res with a simpler inline implementation based on rounddown_pow_of_two. Note that we must explicitly check that the argument is non-zero since it does not play well with zero as a value. Instead of using the function and while loop, simply calculate the number of queues we have available by dividing by num_vfs. Check if the desired queues are available. If not, round down to the nearest power of 2 that fits within our available queues. This matches the behavior of ice_determine_res but is easier to follow as simple in-line logic. Remove ice_determine_res entirely. With this change, we no longer depend on the pf->num_alloc_vfs during the initialization phase of VFs. This will allow us to safely remove it in a future planned refactor of the VF data structures. Signed-off-by: Jacob Keller Tested-by: Konrad Jankowski Signed-off-by: Tony Nguyen --- .../net/ethernet/intel/ice/ice_virtchnl_pf.c | 87 ++++++------------- 1 file changed, 26 insertions(+), 61 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c index 8955a2873d57f..9ee2b3ebe486a 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c @@ -1069,45 +1069,6 @@ static void ice_ena_vf_mappings(struct ice_vf *vf) ice_ena_vf_q_mappings(vf, vsi->alloc_txq, vsi->alloc_rxq); } -/** - * ice_determine_res - * @pf: pointer to the PF structure - * @avail_res: available resources in the PF structure - * @max_res: maximum resources that can be given per VF - * @min_res: minimum resources that can be given per VF - * - * Returns non-zero value if resources (queues/vectors) are available or - * returns zero if PF cannot accommodate for all num_alloc_vfs. - */ -static int -ice_determine_res(struct ice_pf *pf, u16 avail_res, u16 max_res, u16 min_res) -{ - bool checked_min_res = false; - int res; - - /* start by checking if PF can assign max number of resources for - * all num_alloc_vfs. - * if yes, return number per VF - * If no, divide by 2 and roundup, check again - * repeat the loop till we reach a point where even minimum resources - * are not available, in that case return 0 - */ - res = max_res; - while ((res >= min_res) && !checked_min_res) { - int num_all_res; - - num_all_res = pf->num_alloc_vfs * res; - if (num_all_res <= avail_res) - return res; - - if (res == min_res) - checked_min_res = true; - - res = DIV_ROUND_UP(res, 2); - } - return 0; -} - /** * ice_calc_vf_reg_idx - Calculate the VF's register index in the PF space * @vf: VF to calculate the register index for @@ -1187,6 +1148,7 @@ static int ice_sriov_set_msix_res(struct ice_pf *pf, u16 num_msix_needed) /** * ice_set_per_vf_res - check if vectors and queues are available * @pf: pointer to the PF structure + * @num_vfs: the number of SR-IOV VFs being configured * * First, determine HW interrupts from common pool. If we allocate fewer VFs, we * get more vectors and can enable more queues per VF. Note that this does not @@ -1205,20 +1167,20 @@ static int ice_sriov_set_msix_res(struct ice_pf *pf, u16 num_msix_needed) * Lastly, set queue and MSI-X VF variables tracked by the PF so it can be used * by each VF during VF initialization and reset. */ -static int ice_set_per_vf_res(struct ice_pf *pf) +static int ice_set_per_vf_res(struct ice_pf *pf, u16 num_vfs) { int max_valid_res_idx = ice_get_max_valid_res_idx(pf->irq_tracker); + u16 num_msix_per_vf, num_txq, num_rxq, avail_qs; int msix_avail_per_vf, msix_avail_for_sriov; struct device *dev = ice_pf_to_dev(pf); - u16 num_msix_per_vf, num_txq, num_rxq; - if (!pf->num_alloc_vfs || max_valid_res_idx < 0) + if (!num_vfs || max_valid_res_idx < 0) return -EINVAL; /* determine MSI-X resources per VF */ msix_avail_for_sriov = pf->hw.func_caps.common_cap.num_msix_vectors - pf->irq_tracker->num_entries; - msix_avail_per_vf = msix_avail_for_sriov / pf->num_alloc_vfs; + msix_avail_per_vf = msix_avail_for_sriov / num_vfs; if (msix_avail_per_vf >= ICE_NUM_VF_MSIX_MED) { num_msix_per_vf = ICE_NUM_VF_MSIX_MED; } else if (msix_avail_per_vf >= ICE_NUM_VF_MSIX_SMALL) { @@ -1230,32 +1192,35 @@ static int ice_set_per_vf_res(struct ice_pf *pf) } else { dev_err(dev, "Only %d MSI-X interrupts available for SR-IOV. Not enough to support minimum of %d MSI-X interrupts per VF for %d VFs\n", msix_avail_for_sriov, ICE_MIN_INTR_PER_VF, - pf->num_alloc_vfs); + num_vfs); return -EIO; } - /* determine queue resources per VF */ - num_txq = ice_determine_res(pf, ice_get_avail_txq_count(pf), - min_t(u16, - num_msix_per_vf - ICE_NONQ_VECS_VF, - ICE_MAX_RSS_QS_PER_VF), - ICE_MIN_QS_PER_VF); + num_txq = min_t(u16, num_msix_per_vf - ICE_NONQ_VECS_VF, + ICE_MAX_RSS_QS_PER_VF); + avail_qs = ice_get_avail_txq_count(pf) / num_vfs; + if (!avail_qs) + num_txq = 0; + else if (num_txq > avail_qs) + num_txq = rounddown_pow_of_two(avail_qs); - num_rxq = ice_determine_res(pf, ice_get_avail_rxq_count(pf), - min_t(u16, - num_msix_per_vf - ICE_NONQ_VECS_VF, - ICE_MAX_RSS_QS_PER_VF), - ICE_MIN_QS_PER_VF); + num_rxq = min_t(u16, num_msix_per_vf - ICE_NONQ_VECS_VF, + ICE_MAX_RSS_QS_PER_VF); + avail_qs = ice_get_avail_rxq_count(pf) / num_vfs; + if (!avail_qs) + num_rxq = 0; + else if (num_rxq > avail_qs) + num_rxq = rounddown_pow_of_two(avail_qs); - if (!num_txq || !num_rxq) { + if (num_txq < ICE_MIN_QS_PER_VF || num_rxq < ICE_MIN_QS_PER_VF) { dev_err(dev, "Not enough queues to support minimum of %d queue pairs per VF for %d VFs\n", - ICE_MIN_QS_PER_VF, pf->num_alloc_vfs); + ICE_MIN_QS_PER_VF, num_vfs); return -EIO; } - if (ice_sriov_set_msix_res(pf, num_msix_per_vf * pf->num_alloc_vfs)) { + if (ice_sriov_set_msix_res(pf, num_msix_per_vf * num_vfs)) { dev_err(dev, "Unable to set MSI-X resources for %d VFs\n", - pf->num_alloc_vfs); + num_vfs); return -EINVAL; } @@ -1263,7 +1228,7 @@ static int ice_set_per_vf_res(struct ice_pf *pf) pf->num_qps_per_vf = min_t(int, num_txq, num_rxq); pf->num_msix_per_vf = num_msix_per_vf; dev_info(dev, "Enabling %d VFs with %d vectors and %d queues per VF\n", - pf->num_alloc_vfs, pf->num_msix_per_vf, pf->num_qps_per_vf); + num_vfs, pf->num_msix_per_vf, pf->num_qps_per_vf); return 0; } @@ -1977,7 +1942,7 @@ static int ice_ena_vfs(struct ice_pf *pf, u16 num_vfs) if (ret) goto err_pci_disable_sriov; - if (ice_set_per_vf_res(pf)) { + if (ice_set_per_vf_res(pf, num_vfs)) { dev_err(dev, "Not enough resources for %d VFs, try with fewer number of VFs\n", num_vfs); ret = -ENOSPC; From 294627a67e964d3000357e8ac57cc26f049da296 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Wed, 16 Feb 2022 13:37:31 -0800 Subject: [PATCH 04/11] ice: move clear_malvf call in ice_free_vfs The ice_mbx_clear_malvf function is used to clear the indication and count of how many times a VF was detected as malicious. During ice_free_vfs, we use this function to ensure that all removed VFs are reset to a clean state. The call currently is done at the end of ice_free_vfs() using a tmp value to iterate over all of the entries in the bitmap. This separate iteration using tmp is problematic for a planned refactor of the VF array data structure. To avoid this, lets move the call slightly higher into the function inside the loop where we teardown all of the VFs. This avoids one use of the tmp value used for iteration. We'll fix the other user in a future change. Signed-off-by: Jacob Keller Tested-by: Konrad Jankowski Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c index 9ee2b3ebe486a..d46f7579c6507 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c @@ -536,6 +536,12 @@ void ice_free_vfs(struct ice_pf *pf) ice_free_vf_res(vf); } + /* clear malicious info since the VF is getting released */ + if (ice_mbx_clear_malvf(&hw->mbx_snapshot, pf->malvfs, + ICE_MAX_VF_COUNT, vf->vf_id)) + dev_dbg(dev, "failed to clear malicious VF state for VF %u\n", + vf->vf_id); + mutex_unlock(&vf->cfg_lock); mutex_destroy(&vf->cfg_lock); @@ -566,13 +572,6 @@ void ice_free_vfs(struct ice_pf *pf) } } - /* clear malicious info if the VFs are getting released */ - for (i = 0; i < tmp; i++) - if (ice_mbx_clear_malvf(&hw->mbx_snapshot, pf->malvfs, - ICE_MAX_VF_COUNT, i)) - dev_dbg(dev, "failed to clear malicious VF state for VF %u\n", - i); - clear_bit(ICE_VF_DIS, pf->state); clear_bit(ICE_FLAG_SRIOV_ENA, pf->flags); } From 44efe75f736f489892a71fb5b69e6f2271327613 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Wed, 16 Feb 2022 13:37:32 -0800 Subject: [PATCH 05/11] ice: move VFLR acknowledge during ice_free_vfs After removing all VFs, the driver clears the VFLR indication for VFs. This has been in ice since the beginning of SR-IOV support in the ice driver. The implementation was copied from i40e, and the motivation for the VFLR indication clearing is described in the commit f7414531a0cf ("i40e: acknowledge VFLR when disabling SR-IOV") The commit explains that we need to clear the VFLR indication because the virtual function undergoes a VFLR event. If we don't indicate that it is complete it can cause an issue when VFs are re-enabled due to a "phantom" VFLR. The register block read was added under a pci_vfs_assigned check originally. This was done because we added the check after calling pci_disable_sriov. This was later moved to disable SRIOV earlier in the flow so that the VF drivers could be torn down before we removed functionality. Move the VFLR acknowledge into the main loop that tears down VF resources. This avoids using the tmp value for iterating over VFs multiple times. The result will make it easier to refactor the VF array in a future change. It's possible we might want to modify this flow to also stop checking pci_vfs_assigned. However, it seems reasonable to keep this change: we should only clear the VFLR if we actually disabled SR-IOV. Signed-off-by: Jacob Keller Tested-by: Konrad Jankowski Signed-off-by: Tony Nguyen --- .../net/ethernet/intel/ice/ice_virtchnl_pf.c | 27 ++++++------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c index d46f7579c6507..885de0675d2a2 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c @@ -536,6 +536,14 @@ void ice_free_vfs(struct ice_pf *pf) ice_free_vf_res(vf); } + if (!pci_vfs_assigned(pf->pdev)) { + u32 reg_idx, bit_idx; + + reg_idx = (hw->func_caps.vf_base_id + vf->vf_id) / 32; + bit_idx = (hw->func_caps.vf_base_id + vf->vf_id) % 32; + wr32(hw, GLGEN_VFLRSTAT(reg_idx), BIT(bit_idx)); + } + /* clear malicious info since the VF is getting released */ if (ice_mbx_clear_malvf(&hw->mbx_snapshot, pf->malvfs, ICE_MAX_VF_COUNT, vf->vf_id)) @@ -553,25 +561,6 @@ void ice_free_vfs(struct ice_pf *pf) devm_kfree(dev, pf->vf); pf->vf = NULL; - /* This check is for when the driver is unloaded while VFs are - * assigned. Setting the number of VFs to 0 through sysfs is caught - * before this function ever gets called. - */ - if (!pci_vfs_assigned(pf->pdev)) { - unsigned int vf_id; - - /* Acknowledge VFLR for all VFs. Without this, VFs will fail to - * work correctly when SR-IOV gets re-enabled. - */ - for (vf_id = 0; vf_id < tmp; vf_id++) { - u32 reg_idx, bit_idx; - - reg_idx = (hw->func_caps.vf_base_id + vf_id) / 32; - bit_idx = (hw->func_caps.vf_base_id + vf_id) % 32; - wr32(hw, GLGEN_VFLRSTAT(reg_idx), BIT(bit_idx)); - } - } - clear_bit(ICE_VF_DIS, pf->state); clear_bit(ICE_FLAG_SRIOV_ENA, pf->flags); } From 59e1f857e37795e2aaf73d083be6eb472fe8fcc8 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Wed, 16 Feb 2022 13:37:33 -0800 Subject: [PATCH 06/11] ice: remove checks in ice_vc_send_msg_to_vf The ice_vc_send_msg_to_vf function is used by the PF to send a response to a VF. This function has overzealous checks to ensure its not passed a NULL VF pointer and to ensure that the passed in struct ice_vf has a valid vf_id sub-member. These checks have existed since commit 1071a8358a28 ("ice: Implement virtchnl commands for AVF support") and function as simple sanity checks. We are planning to refactor the ice driver to use a hash table along with appropriate locks in a future refactor. This change will modify how the ice_validate_vf_id function works. Instead of a simple >= check to ensure the VF ID is between some range, it will check the hash table to see if the specified VF ID is actually in the table. This requires that the function properly lock the table to prevent race conditions. The checks may seem ok at first glance, but they don't really provide much benefit. In order for ice_vc_send_msg_to_vf to have these checks fail, the callers must either (1) pass NULL as the VF, (2) construct an invalid VF pointer manually, or (3) be using a VF pointer which becomes invalid after they obtain it properly using ice_get_vf_by_id. For (1), a cursory glance over callers of ice_vc_send_msg_to_vf can show that in most cases the functions already operate assuming their VF pointer is valid, such as by derferencing vf->pf or other members. They obtain the VF pointer by accessing the VF array using the VF ID, which can never produce a NULL value (since its a simple address operation on the array it will not be NULL. The sole exception for (1) is that ice_vc_process_vf_msg will forward a NULL VF pointer to this function as part of its goto error handler logic. This requires some minor cleanup to simply exit immediately when an invalid VF ID is detected (Rather than use the same error flow as the rest of the function). For (2), it is unexpected for a flow to construct a VF pointer manually instead of accessing the VF array. Defending against this is likely to just hide bad programming. For (3), it is definitely true that VF pointers could become invalid, for example if a thread is processing a VF message while the VF gets removed. However, the correct solution is not to add additional checks like this which do not guarantee to prevent the race. Instead we plan to solve the root of the problem by preventing the possibility entirely. This solution will require the change to a hash table with proper locking and reference counts of the VF structures. When this is done, ice_validate_vf_id will require locking of the hash table. This will be problematic because all of the callers of ice_vc_send_msg_to_vf will already have to take the lock to obtain the VF pointer anyways. With a mutex, this leads to a double lock that could hang the kernel thread. Avoid this by removing the checks which don't provide much value, so that we can safely add the necessary protections properly. Signed-off-by: Jacob Keller Tested-by: Konrad Jankowski Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c index 885de0675d2a2..c8f2473684cee 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c @@ -2208,13 +2208,7 @@ ice_vc_send_msg_to_vf(struct ice_vf *vf, u32 v_opcode, struct ice_pf *pf; int aq_ret; - if (!vf) - return -EINVAL; - pf = vf->pf; - if (ice_validate_vf_id(pf, vf->vf_id)) - return -EINVAL; - dev = ice_pf_to_dev(pf); /* single place to detect unsuccessful return values */ @@ -5726,8 +5720,9 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) dev = ice_pf_to_dev(pf); if (ice_validate_vf_id(pf, vf_id)) { - err = -EINVAL; - goto error_handler; + dev_err(dev, "Unable to locate VF for message from VF ID %d, opcode %d, len %d\n", + vf_id, v_opcode, msglen); + return; } vf = &pf->vf[vf_id]; From 19281e866808840d2de3f79a929c361b54b017d9 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Wed, 16 Feb 2022 13:37:34 -0800 Subject: [PATCH 07/11] ice: use ice_for_each_vf for iteration during removal When removing VFs, the driver takes a weird approach of assigning pf->num_alloc_vfs to 0 before iterating over the VFs using a temporary variable. This logic has been in the driver for a long time, and seems to have been carried forward from i40e. We want to refactor the way VFs are stored, and iterating over the data structure without the ice_for_each_vf interface impedes this work. The logic relies on implicitly using the num_alloc_vfs as a sort of "safe guard" for accessing VF data. While this sort of guard makes sense for Single Root IOV where all VFs are added at once, the data structures don't work for VFs which can be added and removed dynamically. We also have a separate state flag, ICE_VF_DEINIT_IN_PROGRESS which is a stronger protection against concurrent removal and access. Avoid the custom tmp iteration and replace it with the standard ice_for_each_vf iterator. Delay the assignment of num_alloc_vfs until after this loop finishes. Signed-off-by: Jacob Keller Tested-by: Konrad Jankowski Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c index c8f2473684cee..d72a748870c97 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c @@ -500,7 +500,7 @@ void ice_free_vfs(struct ice_pf *pf) { struct device *dev = ice_pf_to_dev(pf); struct ice_hw *hw = &pf->hw; - unsigned int tmp, i; + unsigned int i; if (!pf->vf) return; @@ -519,10 +519,7 @@ void ice_free_vfs(struct ice_pf *pf) else dev_warn(dev, "VFs are assigned - not disabling SR-IOV\n"); - tmp = pf->num_alloc_vfs; - pf->num_qps_per_vf = 0; - pf->num_alloc_vfs = 0; - for (i = 0; i < tmp; i++) { + ice_for_each_vf(pf, i) { struct ice_vf *vf = &pf->vf[i]; mutex_lock(&vf->cfg_lock); @@ -558,6 +555,8 @@ void ice_free_vfs(struct ice_pf *pf) if (ice_sriov_free_msix_res(pf)) dev_err(dev, "Failed to free MSIX resources used by SR-IOV\n"); + pf->num_qps_per_vf = 0; + pf->num_alloc_vfs = 0; devm_kfree(dev, pf->vf); pf->vf = NULL; From c4c2c7db64e19f815956c750c461d67867f1cdaf Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Wed, 16 Feb 2022 13:37:35 -0800 Subject: [PATCH 08/11] ice: convert ice_for_each_vf to include VF entry iterator The ice_for_each_vf macro is intended to be used to loop over all VFs. The current implementation relies on an iterator that is the index into the VF array in the PF structure. This forces all users to perform a look up themselves. This abstraction forces a lot of duplicate work on callers and leaks the interface implementation to the caller. Replace this with an implementation that includes the VF pointer the primary iterator. This version simplifies callers which just want to iterate over every VF, as they no longer need to perform their own lookup. The "i" iterator value is replaced with a new unsigned int "bkt" parameter, as this will match the necessary interface for replacing the VF array with a hash table. For now, the bkt is the VF ID, but in the future it will simply be the hash bucket index. Document that it should not be treated as a VF ID. This change aims to simplify switching from the array to a hash table. I considered alternative implementations such as an xarray but decided that the hash table was the simplest and most suitable implementation. I also looked at methods to hide the bkt iterator entirely, but I couldn't come up with a feasible solution that worked for hash table iterators. Signed-off-by: Jacob Keller Tested-by: Konrad Jankowski Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_eswitch.c | 63 ++++---- drivers/net/ethernet/intel/ice/ice_ethtool.c | 7 +- drivers/net/ethernet/intel/ice/ice_lib.c | 21 ++- drivers/net/ethernet/intel/ice/ice_main.c | 44 +++--- drivers/net/ethernet/intel/ice/ice_repr.c | 15 +- .../ethernet/intel/ice/ice_virtchnl_fdir.c | 6 +- .../net/ethernet/intel/ice/ice_virtchnl_pf.c | 143 +++++++++--------- .../net/ethernet/intel/ice/ice_virtchnl_pf.h | 16 +- 8 files changed, 163 insertions(+), 152 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c index e47331e363ea8..aa16ea15c5ca2 100644 --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c @@ -210,11 +210,11 @@ static void ice_eswitch_remap_rings_to_vectors(struct ice_pf *pf) static void ice_eswitch_release_reprs(struct ice_pf *pf, struct ice_vsi *ctrl_vsi) { - int i; + struct ice_vf *vf; + unsigned int bkt; - ice_for_each_vf(pf, i) { - struct ice_vsi *vsi = pf->vf[i].repr->src_vsi; - struct ice_vf *vf = &pf->vf[i]; + ice_for_each_vf(pf, bkt, vf) { + struct ice_vsi *vsi = vf->repr->src_vsi; /* Skip VFs that aren't configured */ if (!vf->repr->dst) @@ -238,11 +238,11 @@ static int ice_eswitch_setup_reprs(struct ice_pf *pf) { struct ice_vsi *ctrl_vsi = pf->switchdev.control_vsi; int max_vsi_num = 0; - int i; + struct ice_vf *vf; + unsigned int bkt; - ice_for_each_vf(pf, i) { - struct ice_vsi *vsi = pf->vf[i].repr->src_vsi; - struct ice_vf *vf = &pf->vf[i]; + ice_for_each_vf(pf, bkt, vf) { + struct ice_vsi *vsi = vf->repr->src_vsi; ice_remove_vsi_fltr(&pf->hw, vsi->idx); vf->repr->dst = metadata_dst_alloc(0, METADATA_HW_PORT_MUX, @@ -282,8 +282,8 @@ static int ice_eswitch_setup_reprs(struct ice_pf *pf) netif_keep_dst(vf->repr->netdev); } - ice_for_each_vf(pf, i) { - struct ice_repr *repr = pf->vf[i].repr; + ice_for_each_vf(pf, bkt, vf) { + struct ice_repr *repr = vf->repr; struct ice_vsi *vsi = repr->src_vsi; struct metadata_dst *dst; @@ -417,10 +417,11 @@ ice_eswitch_vsi_setup(struct ice_pf *pf, struct ice_port_info *pi) */ static void ice_eswitch_napi_del(struct ice_pf *pf) { - int i; + struct ice_vf *vf; + unsigned int bkt; - ice_for_each_vf(pf, i) - netif_napi_del(&pf->vf[i].repr->q_vector->napi); + ice_for_each_vf(pf, bkt, vf) + netif_napi_del(&vf->repr->q_vector->napi); } /** @@ -429,10 +430,11 @@ static void ice_eswitch_napi_del(struct ice_pf *pf) */ static void ice_eswitch_napi_enable(struct ice_pf *pf) { - int i; + struct ice_vf *vf; + unsigned int bkt; - ice_for_each_vf(pf, i) - napi_enable(&pf->vf[i].repr->q_vector->napi); + ice_for_each_vf(pf, bkt, vf) + napi_enable(&vf->repr->q_vector->napi); } /** @@ -441,10 +443,11 @@ static void ice_eswitch_napi_enable(struct ice_pf *pf) */ static void ice_eswitch_napi_disable(struct ice_pf *pf) { - int i; + struct ice_vf *vf; + unsigned int bkt; - ice_for_each_vf(pf, i) - napi_disable(&pf->vf[i].repr->q_vector->napi); + ice_for_each_vf(pf, bkt, vf) + napi_disable(&vf->repr->q_vector->napi); } /** @@ -613,16 +616,15 @@ int ice_eswitch_configure(struct ice_pf *pf) */ static void ice_eswitch_start_all_tx_queues(struct ice_pf *pf) { - struct ice_repr *repr; - int i; + struct ice_vf *vf; + unsigned int bkt; if (test_bit(ICE_DOWN, pf->state)) return; - ice_for_each_vf(pf, i) { - repr = pf->vf[i].repr; - if (repr) - ice_repr_start_tx_queues(repr); + ice_for_each_vf(pf, bkt, vf) { + if (vf->repr) + ice_repr_start_tx_queues(vf->repr); } } @@ -632,16 +634,15 @@ static void ice_eswitch_start_all_tx_queues(struct ice_pf *pf) */ void ice_eswitch_stop_all_tx_queues(struct ice_pf *pf) { - struct ice_repr *repr; - int i; + struct ice_vf *vf; + unsigned int bkt; if (test_bit(ICE_DOWN, pf->state)) return; - ice_for_each_vf(pf, i) { - repr = pf->vf[i].repr; - if (repr) - ice_repr_stop_tx_queues(repr); + ice_for_each_vf(pf, bkt, vf) { + if (vf->repr) + ice_repr_stop_tx_queues(vf->repr); } } diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c index a3492754d0d31..d2f50d41f62da 100644 --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c @@ -316,11 +316,10 @@ ice_get_eeprom(struct net_device *netdev, struct ethtool_eeprom *eeprom, */ static bool ice_active_vfs(struct ice_pf *pf) { - unsigned int i; - - ice_for_each_vf(pf, i) { - struct ice_vf *vf = &pf->vf[i]; + struct ice_vf *vf; + unsigned int bkt; + ice_for_each_vf(pf, bkt, vf) { if (test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) return true; } diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c index 5af36311a4042..54ee85e7004f8 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_lib.c @@ -433,13 +433,14 @@ static irqreturn_t ice_eswitch_msix_clean_rings(int __always_unused irq, void *d { struct ice_q_vector *q_vector = (struct ice_q_vector *)data; struct ice_pf *pf = q_vector->vsi->back; - int i; + struct ice_vf *vf; + unsigned int bkt; if (!q_vector->tx.tx_ring && !q_vector->rx.rx_ring) return IRQ_HANDLED; - ice_for_each_vf(pf, i) - napi_schedule(&pf->vf[i].repr->q_vector->napi); + ice_for_each_vf(pf, bkt, vf) + napi_schedule(&vf->repr->q_vector->napi); return IRQ_HANDLED; } @@ -1342,11 +1343,10 @@ ice_get_res(struct ice_pf *pf, struct ice_res_tracker *res, u16 needed, u16 id) */ static int ice_get_vf_ctrl_res(struct ice_pf *pf, struct ice_vsi *vsi) { - int i; - - ice_for_each_vf(pf, i) { - struct ice_vf *vf = &pf->vf[i]; + struct ice_vf *vf; + unsigned int bkt; + ice_for_each_vf(pf, bkt, vf) { if (vf != vsi->vf && vf->ctrl_vsi_idx != ICE_NO_VSI) return pf->vsi[vf->ctrl_vsi_idx]->base_vector; } @@ -2891,11 +2891,10 @@ void ice_napi_del(struct ice_vsi *vsi) */ static void ice_free_vf_ctrl_res(struct ice_pf *pf, struct ice_vsi *vsi) { - int i; - - ice_for_each_vf(pf, i) { - struct ice_vf *vf = &pf->vf[i]; + struct ice_vf *vf; + unsigned int bkt; + ice_for_each_vf(pf, bkt, vf) { if (vf != vsi->vf && vf->ctrl_vsi_idx != ICE_NO_VSI) return; } diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index b80e9be3167c7..bc657916ba50e 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -505,7 +505,8 @@ ice_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type) { struct ice_hw *hw = &pf->hw; struct ice_vsi *vsi; - unsigned int i; + struct ice_vf *vf; + unsigned int bkt; dev_dbg(ice_pf_to_dev(pf), "reset_type=%d\n", reset_type); @@ -520,8 +521,8 @@ ice_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type) ice_vc_notify_reset(pf); /* Disable VFs until reset is completed */ - ice_for_each_vf(pf, i) - ice_set_vf_state_qs_dis(&pf->vf[i]); + ice_for_each_vf(pf, bkt, vf) + ice_set_vf_state_qs_dis(vf); if (ice_is_eswitch_mode_switchdev(pf)) { if (reset_type != ICE_RESET_PFR) @@ -1666,7 +1667,8 @@ static void ice_handle_mdd_event(struct ice_pf *pf) { struct device *dev = ice_pf_to_dev(pf); struct ice_hw *hw = &pf->hw; - unsigned int i; + struct ice_vf *vf; + unsigned int bkt; u32 reg; if (!test_and_clear_bit(ICE_MDD_EVENT_PENDING, pf->state)) { @@ -1754,47 +1756,45 @@ static void ice_handle_mdd_event(struct ice_pf *pf) /* Check to see if one of the VFs caused an MDD event, and then * increment counters and set print pending */ - ice_for_each_vf(pf, i) { - struct ice_vf *vf = &pf->vf[i]; - - reg = rd32(hw, VP_MDET_TX_PQM(i)); + ice_for_each_vf(pf, bkt, vf) { + reg = rd32(hw, VP_MDET_TX_PQM(vf->vf_id)); if (reg & VP_MDET_TX_PQM_VALID_M) { - wr32(hw, VP_MDET_TX_PQM(i), 0xFFFF); + wr32(hw, VP_MDET_TX_PQM(vf->vf_id), 0xFFFF); vf->mdd_tx_events.count++; set_bit(ICE_MDD_VF_PRINT_PENDING, pf->state); if (netif_msg_tx_err(pf)) dev_info(dev, "Malicious Driver Detection event TX_PQM detected on VF %d\n", - i); + vf->vf_id); } - reg = rd32(hw, VP_MDET_TX_TCLAN(i)); + reg = rd32(hw, VP_MDET_TX_TCLAN(vf->vf_id)); if (reg & VP_MDET_TX_TCLAN_VALID_M) { - wr32(hw, VP_MDET_TX_TCLAN(i), 0xFFFF); + wr32(hw, VP_MDET_TX_TCLAN(vf->vf_id), 0xFFFF); vf->mdd_tx_events.count++; set_bit(ICE_MDD_VF_PRINT_PENDING, pf->state); if (netif_msg_tx_err(pf)) dev_info(dev, "Malicious Driver Detection event TX_TCLAN detected on VF %d\n", - i); + vf->vf_id); } - reg = rd32(hw, VP_MDET_TX_TDPU(i)); + reg = rd32(hw, VP_MDET_TX_TDPU(vf->vf_id)); if (reg & VP_MDET_TX_TDPU_VALID_M) { - wr32(hw, VP_MDET_TX_TDPU(i), 0xFFFF); + wr32(hw, VP_MDET_TX_TDPU(vf->vf_id), 0xFFFF); vf->mdd_tx_events.count++; set_bit(ICE_MDD_VF_PRINT_PENDING, pf->state); if (netif_msg_tx_err(pf)) dev_info(dev, "Malicious Driver Detection event TX_TDPU detected on VF %d\n", - i); + vf->vf_id); } - reg = rd32(hw, VP_MDET_RX(i)); + reg = rd32(hw, VP_MDET_RX(vf->vf_id)); if (reg & VP_MDET_RX_VALID_M) { - wr32(hw, VP_MDET_RX(i), 0xFFFF); + wr32(hw, VP_MDET_RX(vf->vf_id), 0xFFFF); vf->mdd_rx_events.count++; set_bit(ICE_MDD_VF_PRINT_PENDING, pf->state); if (netif_msg_rx_err(pf)) dev_info(dev, "Malicious Driver Detection event RX detected on VF %d\n", - i); + vf->vf_id); /* Since the queue is disabled on VF Rx MDD events, the * PF can be configured to reset the VF through ethtool @@ -1805,9 +1805,9 @@ static void ice_handle_mdd_event(struct ice_pf *pf) * reset, so print the event prior to reset. */ ice_print_vf_rx_mdd_event(vf); - mutex_lock(&pf->vf[i].cfg_lock); - ice_reset_vf(&pf->vf[i], false); - mutex_unlock(&pf->vf[i].cfg_lock); + mutex_lock(&vf->cfg_lock); + ice_reset_vf(vf, false); + mutex_unlock(&vf->cfg_lock); } } } diff --git a/drivers/net/ethernet/intel/ice/ice_repr.c b/drivers/net/ethernet/intel/ice/ice_repr.c index 787f51c8ddb27..0517eb807b7f0 100644 --- a/drivers/net/ethernet/intel/ice/ice_repr.c +++ b/drivers/net/ethernet/intel/ice/ice_repr.c @@ -338,13 +338,11 @@ static void ice_repr_rem(struct ice_vf *vf) */ void ice_repr_rem_from_all_vfs(struct ice_pf *pf) { - int i; - - ice_for_each_vf(pf, i) { - struct ice_vf *vf = &pf->vf[i]; + struct ice_vf *vf; + unsigned int bkt; + ice_for_each_vf(pf, bkt, vf) ice_repr_rem(vf); - } } /** @@ -353,12 +351,11 @@ void ice_repr_rem_from_all_vfs(struct ice_pf *pf) */ int ice_repr_add_for_all_vfs(struct ice_pf *pf) { + struct ice_vf *vf; + unsigned int bkt; int err; - int i; - - ice_for_each_vf(pf, i) { - struct ice_vf *vf = &pf->vf[i]; + ice_for_each_vf(pf, bkt, vf) { err = ice_repr_add(vf); if (err) goto err; diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c index 25b9c1dfc1ac8..df7a3f251cd33 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c @@ -1572,15 +1572,15 @@ ice_vc_del_fdir_fltr_post(struct ice_vf *vf, struct ice_vf_fdir_ctx *ctx, */ void ice_flush_fdir_ctx(struct ice_pf *pf) { - int i; + struct ice_vf *vf; + unsigned int bkt; if (!test_and_clear_bit(ICE_FD_VF_FLUSH_CTX, pf->state)) return; - ice_for_each_vf(pf, i) { + ice_for_each_vf(pf, bkt, vf) { struct device *dev = ice_pf_to_dev(pf); enum virtchnl_fdir_prgm_status status; - struct ice_vf *vf = &pf->vf[i]; struct ice_vf_fdir_ctx *ctx; unsigned long flags; int ret; diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c index d72a748870c97..7e00507aa89f4 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c @@ -217,11 +217,10 @@ ice_vc_vf_broadcast(struct ice_pf *pf, enum virtchnl_ops v_opcode, enum virtchnl_status_code v_retval, u8 *msg, u16 msglen) { struct ice_hw *hw = &pf->hw; - unsigned int i; - - ice_for_each_vf(pf, i) { - struct ice_vf *vf = &pf->vf[i]; + struct ice_vf *vf; + unsigned int bkt; + ice_for_each_vf(pf, bkt, vf) { /* Not all vfs are enabled so skip the ones that are not */ if (!test_bit(ICE_VF_STATE_INIT, vf->vf_states) && !test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) @@ -500,7 +499,8 @@ void ice_free_vfs(struct ice_pf *pf) { struct device *dev = ice_pf_to_dev(pf); struct ice_hw *hw = &pf->hw; - unsigned int i; + struct ice_vf *vf; + unsigned int bkt; if (!pf->vf) return; @@ -519,9 +519,7 @@ void ice_free_vfs(struct ice_pf *pf) else dev_warn(dev, "VFs are assigned - not disabling SR-IOV\n"); - ice_for_each_vf(pf, i) { - struct ice_vf *vf = &pf->vf[i]; - + ice_for_each_vf(pf, bkt, vf) { mutex_lock(&vf->cfg_lock); ice_dis_vf_qs(vf); @@ -1462,24 +1460,26 @@ bool ice_reset_all_vfs(struct ice_pf *pf, bool is_vflr) struct device *dev = ice_pf_to_dev(pf); struct ice_hw *hw = &pf->hw; struct ice_vf *vf; - int v, i; + unsigned int bkt; /* If we don't have any VFs, then there is nothing to reset */ if (!pf->num_alloc_vfs) return false; /* clear all malicious info if the VFs are getting reset */ - ice_for_each_vf(pf, i) - if (ice_mbx_clear_malvf(&hw->mbx_snapshot, pf->malvfs, ICE_MAX_VF_COUNT, i)) - dev_dbg(dev, "failed to clear malicious VF state for VF %u\n", i); + ice_for_each_vf(pf, bkt, vf) + if (ice_mbx_clear_malvf(&hw->mbx_snapshot, pf->malvfs, + ICE_MAX_VF_COUNT, vf->vf_id)) + dev_dbg(dev, "failed to clear malicious VF state for VF %u\n", + vf->vf_id); /* If VFs have been disabled, there is no need to reset */ if (test_and_set_bit(ICE_VF_DIS, pf->state)) return false; /* Begin reset on all VFs at once */ - ice_for_each_vf(pf, v) - ice_trigger_vf_reset(&pf->vf[v], is_vflr, true); + ice_for_each_vf(pf, bkt, vf) + ice_trigger_vf_reset(vf, is_vflr, true); /* HW requires some time to make sure it can flush the FIFO for a VF * when it resets it. Poll the VPGEN_VFRSTAT register for each VF in @@ -1487,36 +1487,34 @@ bool ice_reset_all_vfs(struct ice_pf *pf, bool is_vflr) * the VFs using a simple iterator that increments once that VF has * finished resetting. */ - for (i = 0, v = 0; i < 10 && v < pf->num_alloc_vfs; i++) { - /* Check each VF in sequence */ - while (v < pf->num_alloc_vfs) { - u32 reg; - - vf = &pf->vf[v]; - reg = rd32(hw, VPGEN_VFRSTAT(vf->vf_id)); - if (!(reg & VPGEN_VFRSTAT_VFRD_M)) { - /* only delay if the check failed */ - usleep_range(10, 20); + ice_for_each_vf(pf, bkt, vf) { + bool done = false; + unsigned int i; + u32 reg; + + for (i = 0; i < 10; i++) { + reg = rd32(&pf->hw, VPGEN_VFRSTAT(vf->vf_id)); + if (reg & VPGEN_VFRSTAT_VFRD_M) { + done = true; break; } - /* If the current VF has finished resetting, move on - * to the next VF in sequence. + /* only delay if check failed */ + usleep_range(10, 20); + } + + if (!done) { + /* Display a warning if at least one VF didn't manage + * to reset in time, but continue on with the + * operation. */ - v++; + dev_warn(dev, "VF %u reset check timeout\n", vf->vf_id); + break; } } - /* Display a warning if at least one VF didn't manage to reset in - * time, but continue on with the operation. - */ - if (v < pf->num_alloc_vfs) - dev_warn(dev, "VF reset check timeout\n"); - /* free VF resources to begin resetting the VSI state */ - ice_for_each_vf(pf, v) { - vf = &pf->vf[v]; - + ice_for_each_vf(pf, bkt, vf) { mutex_lock(&vf->cfg_lock); vf->driver_caps = 0; @@ -1692,10 +1690,11 @@ bool ice_reset_vf(struct ice_vf *vf, bool is_vflr) */ void ice_vc_notify_link_state(struct ice_pf *pf) { - int i; + struct ice_vf *vf; + unsigned int bkt; - ice_for_each_vf(pf, i) - ice_vc_notify_vf_link_state(&pf->vf[i]); + ice_for_each_vf(pf, bkt, vf) + ice_vc_notify_vf_link_state(vf); } /** @@ -1817,11 +1816,12 @@ static int ice_init_vf_vsi_res(struct ice_vf *vf) static int ice_start_vfs(struct ice_pf *pf) { struct ice_hw *hw = &pf->hw; - int retval, i; - - ice_for_each_vf(pf, i) { - struct ice_vf *vf = &pf->vf[i]; + unsigned int bkt, it_cnt; + struct ice_vf *vf; + int retval; + it_cnt = 0; + ice_for_each_vf(pf, bkt, vf) { ice_clear_vf_reset_trigger(vf); retval = ice_init_vf_vsi_res(vf); @@ -1834,17 +1834,20 @@ static int ice_start_vfs(struct ice_pf *pf) set_bit(ICE_VF_STATE_INIT, vf->vf_states); ice_ena_vf_mappings(vf); wr32(hw, VFGEN_RSTAT(vf->vf_id), VIRTCHNL_VFR_VFACTIVE); + it_cnt++; } ice_flush(hw); return 0; teardown: - for (i = i - 1; i >= 0; i--) { - struct ice_vf *vf = &pf->vf[i]; + ice_for_each_vf(pf, bkt, vf) { + if (it_cnt == 0) + break; ice_dis_vf_mappings(vf); ice_vf_vsi_release(vf); + it_cnt--; } return retval; @@ -1856,13 +1859,13 @@ static int ice_start_vfs(struct ice_pf *pf) */ static void ice_set_dflt_settings_vfs(struct ice_pf *pf) { - int i; - - ice_for_each_vf(pf, i) { - struct ice_vf *vf = &pf->vf[i]; + struct ice_vf *vf; + unsigned int bkt; + u16 vf_id = 0; + ice_for_each_vf(pf, bkt, vf) { vf->pf = pf; - vf->vf_id = i; + vf->vf_id = vf_id++; vf->vf_sw_id = pf->first_sw; /* assign default capabilities */ set_bit(ICE_VIRTCHNL_VF_CAP_L2, &vf->vf_caps); @@ -2087,19 +2090,19 @@ int ice_sriov_configure(struct pci_dev *pdev, int num_vfs) void ice_process_vflr_event(struct ice_pf *pf) { struct ice_hw *hw = &pf->hw; - unsigned int vf_id; + struct ice_vf *vf; + unsigned int bkt; u32 reg; if (!test_and_clear_bit(ICE_VFLR_EVENT_PENDING, pf->state) || !pf->num_alloc_vfs) return; - ice_for_each_vf(pf, vf_id) { - struct ice_vf *vf = &pf->vf[vf_id]; + ice_for_each_vf(pf, bkt, vf) { u32 reg_idx, bit_idx; - reg_idx = (hw->func_caps.vf_base_id + vf_id) / 32; - bit_idx = (hw->func_caps.vf_base_id + vf_id) % 32; + reg_idx = (hw->func_caps.vf_base_id + vf->vf_id) / 32; + bit_idx = (hw->func_caps.vf_base_id + vf->vf_id) % 32; /* read GLGEN_VFLRSTAT register to find out the flr VFs */ reg = rd32(hw, GLGEN_VFLRSTAT(reg_idx)); if (reg & BIT(bit_idx)) { @@ -2131,10 +2134,10 @@ static void ice_vc_reset_vf(struct ice_vf *vf) */ static struct ice_vf *ice_get_vf_from_pfq(struct ice_pf *pf, u16 pfq) { - unsigned int vf_id; + struct ice_vf *vf; + unsigned int bkt; - ice_for_each_vf(pf, vf_id) { - struct ice_vf *vf = &pf->vf[vf_id]; + ice_for_each_vf(pf, bkt, vf) { struct ice_vsi *vsi; u16 rxq_idx; @@ -3011,11 +3014,10 @@ int ice_set_vf_spoofchk(struct net_device *netdev, int vf_id, bool ena) */ bool ice_is_any_vf_in_promisc(struct ice_pf *pf) { - int vf_idx; - - ice_for_each_vf(pf, vf_idx) { - struct ice_vf *vf = &pf->vf[vf_idx]; + struct ice_vf *vf; + unsigned int bkt; + ice_for_each_vf(pf, bkt, vf) { /* found a VF that has promiscuous mode configured */ if (test_bit(ICE_VF_STATE_UC_PROMISC, vf->vf_states) || test_bit(ICE_VF_STATE_MC_PROMISC, vf->vf_states)) @@ -6112,10 +6114,12 @@ int ice_set_vf_link_state(struct net_device *netdev, int vf_id, int link_state) */ static int ice_calc_all_vfs_min_tx_rate(struct ice_pf *pf) { - int rate = 0, i; + struct ice_vf *vf; + unsigned int bkt; + int rate = 0; - ice_for_each_vf(pf, i) - rate += pf->vf[i].min_tx_rate; + ice_for_each_vf(pf, bkt, vf) + rate += vf->min_tx_rate; return rate; } @@ -6296,7 +6300,8 @@ void ice_print_vfs_mdd_events(struct ice_pf *pf) { struct device *dev = ice_pf_to_dev(pf); struct ice_hw *hw = &pf->hw; - int i; + struct ice_vf *vf; + unsigned int bkt; /* check that there are pending MDD events to print */ if (!test_and_clear_bit(ICE_MDD_VF_PRINT_PENDING, pf->state)) @@ -6308,9 +6313,7 @@ void ice_print_vfs_mdd_events(struct ice_pf *pf) pf->last_printed_mdd_jiffies = jiffies; - ice_for_each_vf(pf, i) { - struct ice_vf *vf = &pf->vf[i]; - + ice_for_each_vf(pf, bkt, vf) { /* only print Rx MDD event message if there are new events */ if (vf->mdd_rx_events.count != vf->mdd_rx_events.last_printed) { vf->mdd_rx_events.last_printed = @@ -6324,7 +6327,7 @@ void ice_print_vfs_mdd_events(struct ice_pf *pf) vf->mdd_tx_events.count; dev_info(dev, "%d Tx Malicious Driver Detection events detected on PF %d VF %d MAC %pM.\n", - vf->mdd_tx_events.count, hw->pf_id, i, + vf->mdd_tx_events.count, hw->pf_id, vf->vf_id, vf->dev_lan_addr.addr); } } diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h index 4f4961043638e..9cccb5afb92d7 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h @@ -39,8 +39,20 @@ #define ICE_MAX_VF_RESET_TRIES 40 #define ICE_MAX_VF_RESET_SLEEP_MS 20 -#define ice_for_each_vf(pf, i) \ - for ((i) = 0; (i) < (pf)->num_alloc_vfs; (i)++) +/** + * ice_for_each_vf - Iterate over each VF entry + * @pf: pointer to the PF private structure + * @bkt: bucket index used for iteration + * @entry: pointer to the VF entry currently being processed in the loop. + * + * The bkt variable is an unsigned integer iterator used to traverse the VF + * entries. It is *not* guaranteed to be the VF's vf_id. Do not assume it is. + * Use vf->vf_id to get the id number if needed. + */ +#define ice_for_each_vf(pf, bkt, entry) \ + for ((bkt) = 0, (entry) = &(pf)->vf[0]; \ + (bkt) < (pf)->num_alloc_vfs; \ + (bkt)++, (entry)++) /* Specific VF states */ enum ice_vf_states { From 000773c00f52f2a6084ec04c2efdc2a28ee29d9c Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Wed, 16 Feb 2022 13:37:36 -0800 Subject: [PATCH 09/11] ice: factor VF variables to separate structure We maintain a number of values for VFs within the ice_pf structure. This includes the VF table, the number of allocated VFs, the maximum number of supported SR-IOV VFs, the number of queue pairs per VF, the number of MSI-X vectors per VF, and a bitmap of the VFs with detected MDD events. We're about to add a few more variables to this list. Clean this up first by extracting these members out into a new ice_vfs structure defined in ice_virtchnl_pf.h Signed-off-by: Jacob Keller Tested-by: Konrad Jankowski Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice.h | 10 +- drivers/net/ethernet/intel/ice/ice_eswitch.c | 20 +++- drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +- drivers/net/ethernet/intel/ice/ice_lib.c | 8 +- drivers/net/ethernet/intel/ice/ice_main.c | 2 +- .../net/ethernet/intel/ice/ice_virtchnl_pf.c | 94 ++++++++++--------- .../net/ethernet/intel/ice/ice_virtchnl_pf.h | 15 ++- 7 files changed, 83 insertions(+), 68 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h index 56944b003c128..dc42ff92dbadd 100644 --- a/drivers/net/ethernet/intel/ice/ice.h +++ b/drivers/net/ethernet/intel/ice/ice.h @@ -528,15 +528,7 @@ struct ice_pf { struct ice_vsi **vsi; /* VSIs created by the driver */ struct ice_sw *first_sw; /* first switch created by firmware */ u16 eswitch_mode; /* current mode of eswitch */ - /* Virtchnl/SR-IOV config info */ - struct ice_vf *vf; - u16 num_alloc_vfs; /* actual number of VFs allocated */ - u16 num_vfs_supported; /* num VFs supported for this PF */ - u16 num_qps_per_vf; - u16 num_msix_per_vf; - /* used to ratelimit the MDD event logging */ - unsigned long last_printed_mdd_jiffies; - DECLARE_BITMAP(malvfs, ICE_MAX_VF_COUNT); + struct ice_vfs vfs; DECLARE_BITMAP(features, ICE_F_MAX); DECLARE_BITMAP(state, ICE_STATE_NBITS); DECLARE_BITMAP(flags, ICE_PF_FLAGS_NBITS); diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c index aa16ea15c5ca2..7bcba782f74c7 100644 --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c @@ -176,10 +176,20 @@ static void ice_eswitch_remap_rings_to_vectors(struct ice_pf *pf) int q_id; ice_for_each_txq(vsi, q_id) { - struct ice_repr *repr = pf->vf[q_id].repr; - struct ice_q_vector *q_vector = repr->q_vector; - struct ice_tx_ring *tx_ring = vsi->tx_rings[q_id]; - struct ice_rx_ring *rx_ring = vsi->rx_rings[q_id]; + struct ice_q_vector *q_vector; + struct ice_tx_ring *tx_ring; + struct ice_rx_ring *rx_ring; + struct ice_repr *repr; + struct ice_vf *vf; + + if (WARN_ON(q_id >= pf->vfs.num_alloc)) + continue; + + vf = &pf->vfs.table[q_id]; + repr = vf->repr; + q_vector = repr->q_vector; + tx_ring = vsi->tx_rings[q_id]; + rx_ring = vsi->rx_rings[q_id]; q_vector->vsi = vsi; q_vector->reg_idx = vsi->q_vectors[0]->reg_idx; @@ -525,7 +535,7 @@ ice_eswitch_mode_set(struct devlink *devlink, u16 mode, if (pf->eswitch_mode == mode) return 0; - if (pf->num_alloc_vfs) { + if (pf->vfs.num_alloc) { dev_info(ice_pf_to_dev(pf), "Changing eswitch mode is allowed only if there is no VFs created"); NL_SET_ERR_MSG_MOD(extack, "Changing eswitch mode is allowed only if there is no VFs created"); return -EOPNOTSUPP; diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c index d2f50d41f62da..1181f41ff5fec 100644 --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c @@ -1297,7 +1297,7 @@ static int ice_set_priv_flags(struct net_device *netdev, u32 flags) } if (test_bit(ICE_FLAG_VF_VLAN_PRUNING, change_flags) && - pf->num_alloc_vfs) { + pf->vfs.num_alloc) { dev_err(dev, "vf-vlan-pruning: VLAN pruning cannot be changed while VFs are active.\n"); /* toggle bit back to previous state */ change_bit(ICE_FLAG_VF_VLAN_PRUNING, pf->flags); diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c index 54ee85e7004f8..ebb7d74fa7b01 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_lib.c @@ -215,8 +215,8 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, struct ice_vf *vf) /* The number of queues for ctrl VSI is equal to number of VFs. * Each ring is associated to the corresponding VF_PR netdev. */ - vsi->alloc_txq = pf->num_alloc_vfs; - vsi->alloc_rxq = pf->num_alloc_vfs; + vsi->alloc_txq = pf->vfs.num_alloc; + vsi->alloc_rxq = pf->vfs.num_alloc; vsi->num_q_vectors = 1; break; case ICE_VSI_VF: @@ -224,12 +224,12 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, struct ice_vf *vf) vf->num_vf_qs = vf->num_req_qs; vsi->alloc_txq = vf->num_vf_qs; vsi->alloc_rxq = vf->num_vf_qs; - /* pf->num_msix_per_vf includes (VF miscellaneous vector + + /* pf->vfs.num_msix_per includes (VF miscellaneous vector + * data queue interrupts). Since vsi->num_q_vectors is number * of queues vectors, subtract 1 (ICE_NONQ_VECS_VF) from the * original vector count */ - vsi->num_q_vectors = pf->num_msix_per_vf - ICE_NONQ_VECS_VF; + vsi->num_q_vectors = pf->vfs.num_msix_per - ICE_NONQ_VECS_VF; break; case ICE_VSI_CTRL: vsi->alloc_txq = 1; diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index bc657916ba50e..9028a925743a5 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -3712,7 +3712,7 @@ static void ice_set_pf_caps(struct ice_pf *pf) clear_bit(ICE_FLAG_SRIOV_CAPABLE, pf->flags); if (func_caps->common_cap.sr_iov_1_1) { set_bit(ICE_FLAG_SRIOV_CAPABLE, pf->flags); - pf->num_vfs_supported = min_t(int, func_caps->num_allocd_vfs, + pf->vfs.num_supported = min_t(int, func_caps->num_allocd_vfs, ICE_MAX_VF_COUNT); } clear_bit(ICE_FLAG_RSS_ENA, pf->flags); diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c index 7e00507aa89f4..fd83fa39d0cf9 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c @@ -182,7 +182,7 @@ struct ice_vsi *ice_get_vf_vsi(struct ice_vf *vf) static int ice_validate_vf_id(struct ice_pf *pf, u16 vf_id) { /* vf_id range is only valid for 0-255, and should always be unsigned */ - if (vf_id >= pf->num_alloc_vfs) { + if (vf_id >= pf->vfs.num_alloc) { dev_err(ice_pf_to_dev(pf), "Invalid VF ID: %u\n", vf_id); return -EINVAL; } @@ -380,7 +380,7 @@ static void ice_free_vf_res(struct ice_vf *vf) vf->num_mac = 0; } - last_vector_idx = vf->first_vector_idx + pf->num_msix_per_vf - 1; + last_vector_idx = vf->first_vector_idx + pf->vfs.num_msix_per - 1; /* clear VF MDD event information */ memset(&vf->mdd_tx_events, 0, sizeof(vf->mdd_tx_events)); @@ -416,7 +416,7 @@ static void ice_dis_vf_mappings(struct ice_vf *vf) wr32(hw, VPINT_ALLOC_PCI(vf->vf_id), 0); first = vf->first_vector_idx; - last = first + pf->num_msix_per_vf - 1; + last = first + pf->vfs.num_msix_per - 1; for (v = first; v <= last; v++) { u32 reg; @@ -498,11 +498,12 @@ static void ice_dis_vf_qs(struct ice_vf *vf) void ice_free_vfs(struct ice_pf *pf) { struct device *dev = ice_pf_to_dev(pf); + struct ice_vfs *vfs = &pf->vfs; struct ice_hw *hw = &pf->hw; struct ice_vf *vf; unsigned int bkt; - if (!pf->vf) + if (!vfs->table) return; ice_eswitch_release(pf); @@ -540,7 +541,7 @@ void ice_free_vfs(struct ice_pf *pf) } /* clear malicious info since the VF is getting released */ - if (ice_mbx_clear_malvf(&hw->mbx_snapshot, pf->malvfs, + if (ice_mbx_clear_malvf(&hw->mbx_snapshot, pf->vfs.malvfs, ICE_MAX_VF_COUNT, vf->vf_id)) dev_dbg(dev, "failed to clear malicious VF state for VF %u\n", vf->vf_id); @@ -553,10 +554,10 @@ void ice_free_vfs(struct ice_pf *pf) if (ice_sriov_free_msix_res(pf)) dev_err(dev, "Failed to free MSIX resources used by SR-IOV\n"); - pf->num_qps_per_vf = 0; - pf->num_alloc_vfs = 0; - devm_kfree(dev, pf->vf); - pf->vf = NULL; + vfs->num_qps_per = 0; + vfs->num_alloc = 0; + devm_kfree(dev, vfs->table); + vfs->table = NULL; clear_bit(ICE_VF_DIS, pf->state); clear_bit(ICE_FLAG_SRIOV_ENA, pf->flags); @@ -702,7 +703,7 @@ struct ice_vsi *ice_vf_ctrl_vsi_setup(struct ice_vf *vf) */ static int ice_calc_vf_first_vector_idx(struct ice_pf *pf, struct ice_vf *vf) { - return pf->sriov_base_vector + vf->vf_id * pf->num_msix_per_vf; + return pf->sriov_base_vector + vf->vf_id * pf->vfs.num_msix_per; } /** @@ -959,12 +960,12 @@ static void ice_ena_vf_msix_mappings(struct ice_vf *vf) hw = &pf->hw; pf_based_first_msix = vf->first_vector_idx; - pf_based_last_msix = (pf_based_first_msix + pf->num_msix_per_vf) - 1; + pf_based_last_msix = (pf_based_first_msix + pf->vfs.num_msix_per) - 1; device_based_first_msix = pf_based_first_msix + pf->hw.func_caps.common_cap.msix_vector_first_id; device_based_last_msix = - (device_based_first_msix + pf->num_msix_per_vf) - 1; + (device_based_first_msix + pf->vfs.num_msix_per) - 1; device_based_vf_id = vf->vf_id + hw->func_caps.vf_base_id; reg = (((device_based_first_msix << VPINT_ALLOC_FIRST_S) & @@ -1069,7 +1070,7 @@ int ice_calc_vf_reg_idx(struct ice_vf *vf, struct ice_q_vector *q_vector) pf = vf->pf; /* always add one to account for the OICR being the first MSIX */ - return pf->sriov_base_vector + pf->num_msix_per_vf * vf->vf_id + + return pf->sriov_base_vector + pf->vfs.num_msix_per * vf->vf_id + q_vector->v_idx + 1; } @@ -1210,10 +1211,10 @@ static int ice_set_per_vf_res(struct ice_pf *pf, u16 num_vfs) } /* only allow equal Tx/Rx queue count (i.e. queue pairs) */ - pf->num_qps_per_vf = min_t(int, num_txq, num_rxq); - pf->num_msix_per_vf = num_msix_per_vf; + pf->vfs.num_qps_per = min_t(int, num_txq, num_rxq); + pf->vfs.num_msix_per = num_msix_per_vf; dev_info(dev, "Enabling %d VFs with %d vectors and %d queues per VF\n", - num_vfs, pf->num_msix_per_vf, pf->num_qps_per_vf); + num_vfs, pf->vfs.num_msix_per, pf->vfs.num_qps_per); return 0; } @@ -1463,12 +1464,12 @@ bool ice_reset_all_vfs(struct ice_pf *pf, bool is_vflr) unsigned int bkt; /* If we don't have any VFs, then there is nothing to reset */ - if (!pf->num_alloc_vfs) + if (!pf->vfs.num_alloc) return false; /* clear all malicious info if the VFs are getting reset */ ice_for_each_vf(pf, bkt, vf) - if (ice_mbx_clear_malvf(&hw->mbx_snapshot, pf->malvfs, + if (ice_mbx_clear_malvf(&hw->mbx_snapshot, pf->vfs.malvfs, ICE_MAX_VF_COUNT, vf->vf_id)) dev_dbg(dev, "failed to clear malicious VF state for VF %u\n", vf->vf_id); @@ -1678,7 +1679,8 @@ bool ice_reset_vf(struct ice_vf *vf, bool is_vflr) ice_eswitch_replay_vf_mac_rule(vf); /* if the VF has been reset allow it to come up again */ - if (ice_mbx_clear_malvf(&hw->mbx_snapshot, pf->malvfs, ICE_MAX_VF_COUNT, vf->vf_id)) + if (ice_mbx_clear_malvf(&hw->mbx_snapshot, pf->vfs.malvfs, + ICE_MAX_VF_COUNT, vf->vf_id)) dev_dbg(dev, "failed to clear malicious VF state for VF %u\n", i); return true; @@ -1707,7 +1709,7 @@ void ice_vc_notify_reset(struct ice_pf *pf) { struct virtchnl_pf_event pfe; - if (!pf->num_alloc_vfs) + if (!pf->vfs.num_alloc) return; pfe.event = VIRTCHNL_EVENT_RESET_IMPENDING; @@ -1870,7 +1872,7 @@ static void ice_set_dflt_settings_vfs(struct ice_pf *pf) /* assign default capabilities */ set_bit(ICE_VIRTCHNL_VF_CAP_L2, &vf->vf_caps); vf->spoofchk = true; - vf->num_vf_qs = pf->num_qps_per_vf; + vf->num_vf_qs = pf->vfs.num_qps_per; ice_vc_set_default_allowlist(vf); /* ctrl_vsi_idx will be set to a valid value only when VF @@ -1899,8 +1901,8 @@ static int ice_alloc_vfs(struct ice_pf *pf, int num_vfs) if (!vfs) return -ENOMEM; - pf->vf = vfs; - pf->num_alloc_vfs = num_vfs; + pf->vfs.table = NULL; + pf->vfs.num_alloc = num_vfs; return 0; } @@ -1924,7 +1926,7 @@ static int ice_ena_vfs(struct ice_pf *pf, u16 num_vfs) ret = pci_enable_sriov(pf->pdev, num_vfs); if (ret) { - pf->num_alloc_vfs = 0; + pf->vfs.num_alloc = 0; goto err_unroll_intr; } @@ -1960,9 +1962,9 @@ static int ice_ena_vfs(struct ice_pf *pf, u16 num_vfs) return 0; err_unroll_sriov: - devm_kfree(dev, pf->vf); - pf->vf = NULL; - pf->num_alloc_vfs = 0; + devm_kfree(dev, pf->vfs.table); + pf->vfs.table = NULL; + pf->vfs.num_alloc = 0; err_pci_disable_sriov: pci_disable_sriov(pf->pdev); err_unroll_intr: @@ -1990,9 +1992,9 @@ static int ice_pci_sriov_ena(struct ice_pf *pf, int num_vfs) else if (pre_existing_vfs && pre_existing_vfs == num_vfs) return 0; - if (num_vfs > pf->num_vfs_supported) { + if (num_vfs > pf->vfs.num_supported) { dev_err(dev, "Can't enable %d VFs, max VFs supported is %d\n", - num_vfs, pf->num_vfs_supported); + num_vfs, pf->vfs.num_supported); return -EOPNOTSUPP; } @@ -2095,7 +2097,7 @@ void ice_process_vflr_event(struct ice_pf *pf) u32 reg; if (!test_and_clear_bit(ICE_VFLR_EVENT_PENDING, pf->state) || - !pf->num_alloc_vfs) + !pf->vfs.num_alloc) return; ice_for_each_vf(pf, bkt, vf) { @@ -2401,7 +2403,7 @@ static int ice_vc_get_vf_res_msg(struct ice_vf *vf, u8 *msg) vfres->num_vsis = 1; /* Tx and Rx queue are equal for VF */ vfres->num_queue_pairs = vsi->num_txq; - vfres->max_vectors = pf->num_msix_per_vf; + vfres->max_vectors = pf->vfs.num_msix_per; vfres->rss_key_size = ICE_VSIQF_HKEY_ARRAY_SIZE; vfres->rss_lut_size = ICE_VSIQF_HLUT_ARRAY_SIZE; vfres->max_mtu = ice_vc_get_max_frame_size(vf); @@ -2969,7 +2971,7 @@ int ice_set_vf_spoofchk(struct net_device *netdev, int vf_id, bool ena) if (ice_validate_vf_id(pf, vf_id)) return -EINVAL; - vf = &pf->vf[vf_id]; + vf = &pf->vfs.table[vf_id]; ret = ice_check_vf_ready_for_cfg(vf); if (ret) return ret; @@ -3544,7 +3546,7 @@ static int ice_vc_cfg_irq_map_msg(struct ice_vf *vf, u8 *msg) * there is actually at least a single VF queue vector mapped */ if (!test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states) || - pf->num_msix_per_vf < num_q_vectors_mapped || + pf->vfs.num_msix_per < num_q_vectors_mapped || !num_q_vectors_mapped) { v_ret = VIRTCHNL_STATUS_ERR_PARAM; goto error_param; @@ -3566,7 +3568,7 @@ static int ice_vc_cfg_irq_map_msg(struct ice_vf *vf, u8 *msg) /* vector_id is always 0-based for each VF, and can never be * larger than or equal to the max allowed interrupts per VF */ - if (!(vector_id < pf->num_msix_per_vf) || + if (!(vector_id < pf->vfs.num_msix_per) || !ice_vc_isvalid_vsi_id(vf, vsi_id) || (!vector_id && (map->rxq_map || map->txq_map))) { v_ret = VIRTCHNL_STATUS_ERR_PARAM; @@ -4172,7 +4174,7 @@ ice_set_vf_port_vlan(struct net_device *netdev, int vf_id, u16 vlan_id, u8 qos, return -EPROTONOSUPPORT; } - vf = &pf->vf[vf_id]; + vf = &pf->vfs.table[vf_id]; ret = ice_check_vf_ready_for_cfg(vf); if (ret) return ret; @@ -5726,7 +5728,7 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) return; } - vf = &pf->vf[vf_id]; + vf = &pf->vfs.table[vf_id]; /* Check if VF is disabled. */ if (test_bit(ICE_VF_STATE_DIS, vf->vf_states)) { @@ -5900,7 +5902,7 @@ ice_get_vf_cfg(struct net_device *netdev, int vf_id, struct ifla_vf_info *ivi) if (ice_validate_vf_id(pf, vf_id)) return -EINVAL; - vf = &pf->vf[vf_id]; + vf = &pf->vfs.table[vf_id]; if (ice_check_vf_init(pf, vf)) return -EBUSY; @@ -5982,7 +5984,7 @@ int ice_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac) return -EINVAL; } - vf = &pf->vf[vf_id]; + vf = &pf->vfs.table[vf_id]; /* nothing left to do, unicast MAC already set */ if (ether_addr_equal(vf->dev_lan_addr.addr, mac) && ether_addr_equal(vf->hw_lan_addr.addr, mac)) @@ -6044,7 +6046,7 @@ int ice_set_vf_trust(struct net_device *netdev, int vf_id, bool trusted) if (ice_validate_vf_id(pf, vf_id)) return -EINVAL; - vf = &pf->vf[vf_id]; + vf = &pf->vfs.table[vf_id]; ret = ice_check_vf_ready_for_cfg(vf); if (ret) return ret; @@ -6082,7 +6084,7 @@ int ice_set_vf_link_state(struct net_device *netdev, int vf_id, int link_state) if (ice_validate_vf_id(pf, vf_id)) return -EINVAL; - vf = &pf->vf[vf_id]; + vf = &pf->vfs.table[vf_id]; ret = ice_check_vf_ready_for_cfg(vf); if (ret) return ret; @@ -6177,7 +6179,7 @@ ice_set_vf_bw(struct net_device *netdev, int vf_id, int min_tx_rate, if (ice_validate_vf_id(pf, vf_id)) return -EINVAL; - vf = &pf->vf[vf_id]; + vf = &pf->vfs.table[vf_id]; ret = ice_check_vf_ready_for_cfg(vf); if (ret) return ret; @@ -6244,7 +6246,7 @@ int ice_get_vf_stats(struct net_device *netdev, int vf_id, if (ice_validate_vf_id(pf, vf_id)) return -EINVAL; - vf = &pf->vf[vf_id]; + vf = &pf->vfs.table[vf_id]; ret = ice_check_vf_ready_for_cfg(vf); if (ret) return ret; @@ -6308,10 +6310,10 @@ void ice_print_vfs_mdd_events(struct ice_pf *pf) return; /* VF MDD event logs are rate limited to one second intervals */ - if (time_is_after_jiffies(pf->last_printed_mdd_jiffies + HZ * 1)) + if (time_is_after_jiffies(pf->vfs.last_printed_mdd_jiffies + HZ * 1)) return; - pf->last_printed_mdd_jiffies = jiffies; + pf->vfs.last_printed_mdd_jiffies = jiffies; ice_for_each_vf(pf, bkt, vf) { /* only print Rx MDD event message if there are new events */ @@ -6385,7 +6387,7 @@ ice_is_malicious_vf(struct ice_pf *pf, struct ice_rq_event_info *event, if (ice_validate_vf_id(pf, vf_id)) return false; - vf = &pf->vf[vf_id]; + vf = &pf->vfs.table[vf_id]; /* Check if VF is disabled. */ if (test_bit(ICE_VF_STATE_DIS, vf->vf_states)) return false; @@ -6407,7 +6409,7 @@ ice_is_malicious_vf(struct ice_pf *pf, struct ice_rq_event_info *event, /* if the VF is malicious and we haven't let the user * know about it, then let them know now */ - status = ice_mbx_report_malvf(&pf->hw, pf->malvfs, + status = ice_mbx_report_malvf(&pf->hw, pf->vfs.malvfs, ICE_MAX_VF_COUNT, vf_id, &report_vf); if (status) diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h index 9cccb5afb92d7..1448e31772157 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h @@ -50,8 +50,8 @@ * Use vf->vf_id to get the id number if needed. */ #define ice_for_each_vf(pf, bkt, entry) \ - for ((bkt) = 0, (entry) = &(pf)->vf[0]; \ - (bkt) < (pf)->num_alloc_vfs; \ + for ((bkt) = 0, (entry) = &(pf)->vfs.table[0]; \ + (bkt) < (pf)->vfs.num_alloc; \ (bkt)++, (entry)++) /* Specific VF states */ @@ -116,6 +116,17 @@ struct ice_vc_vf_ops { int (*dis_vlan_insertion_v2_msg)(struct ice_vf *vf, u8 *msg); }; +/* Virtchnl/SR-IOV config info */ +struct ice_vfs { + struct ice_vf *table; /* table of VF entries */ + u16 num_alloc; /* number of allocated VFs */ + u16 num_supported; /* max supported VFs on this PF */ + u16 num_qps_per; /* number of queue pairs per VF */ + u16 num_msix_per; /* number of MSI-X vectors per VF */ + unsigned long last_printed_mdd_jiffies; /* MDD message rate limit */ + DECLARE_BITMAP(malvfs, ICE_MAX_VF_COUNT); /* malicious VF indicator */ +}; + /* VF information structure */ struct ice_vf { struct ice_pf *pf; From fb916db1f04f8c5f005fafcbe6ae01f40abd6aff Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Wed, 16 Feb 2022 13:37:37 -0800 Subject: [PATCH 10/11] ice: introduce VF accessor functions Before we switch the VF data structure storage mechanism to a hash, introduce new accessor functions to define the new interface. * ice_get_vf_by_id is a function used to obtain a reference to a VF from the table based on its VF ID * ice_has_vfs is used to quickly check if any VFs are configured * ice_get_num_vfs is used to get an exact count of how many VFs are configured We can drop the old ice_validate_vf_id function, since every caller was just going to immediately access the VF table to get a reference anyways. This way we simply use the single ice_get_vf_by_id to both validate the VF ID is within range and that there exists a VF with that ID. This change enables us to more easily convert the codebase to the hash table since most callers now properly use the interface. Signed-off-by: Jacob Keller Tested-by: Konrad Jankowski Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_eswitch.c | 6 +- drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +- drivers/net/ethernet/intel/ice/ice_lib.c | 4 +- .../net/ethernet/intel/ice/ice_virtchnl_pf.c | 131 +++++++++++------- .../net/ethernet/intel/ice/ice_virtchnl_pf.h | 25 ++++ 5 files changed, 115 insertions(+), 53 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c index 7bcba782f74c7..c27013afcadb8 100644 --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c @@ -182,10 +182,10 @@ static void ice_eswitch_remap_rings_to_vectors(struct ice_pf *pf) struct ice_repr *repr; struct ice_vf *vf; - if (WARN_ON(q_id >= pf->vfs.num_alloc)) + vf = ice_get_vf_by_id(pf, q_id); + if (WARN_ON(!vf)) continue; - vf = &pf->vfs.table[q_id]; repr = vf->repr; q_vector = repr->q_vector; tx_ring = vsi->tx_rings[q_id]; @@ -535,7 +535,7 @@ ice_eswitch_mode_set(struct devlink *devlink, u16 mode, if (pf->eswitch_mode == mode) return 0; - if (pf->vfs.num_alloc) { + if (ice_has_vfs(pf)) { dev_info(ice_pf_to_dev(pf), "Changing eswitch mode is allowed only if there is no VFs created"); NL_SET_ERR_MSG_MOD(extack, "Changing eswitch mode is allowed only if there is no VFs created"); return -EOPNOTSUPP; diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c index 1181f41ff5fec..83680eaa35658 100644 --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c @@ -1297,7 +1297,7 @@ static int ice_set_priv_flags(struct net_device *netdev, u32 flags) } if (test_bit(ICE_FLAG_VF_VLAN_PRUNING, change_flags) && - pf->vfs.num_alloc) { + ice_has_vfs(pf)) { dev_err(dev, "vf-vlan-pruning: VLAN pruning cannot be changed while VFs are active.\n"); /* toggle bit back to previous state */ change_bit(ICE_FLAG_VF_VLAN_PRUNING, pf->flags); diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c index ebb7d74fa7b01..6ea1d97418086 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_lib.c @@ -215,8 +215,8 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi, struct ice_vf *vf) /* The number of queues for ctrl VSI is equal to number of VFs. * Each ring is associated to the corresponding VF_PR netdev. */ - vsi->alloc_txq = pf->vfs.num_alloc; - vsi->alloc_rxq = pf->vfs.num_alloc; + vsi->alloc_txq = ice_get_num_vfs(pf); + vsi->alloc_rxq = vsi->alloc_txq; vsi->num_q_vectors = 1; break; case ICE_VSI_VF: diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c index fd83fa39d0cf9..535d5f1a763f3 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c @@ -175,18 +175,58 @@ struct ice_vsi *ice_get_vf_vsi(struct ice_vf *vf) } /** - * ice_validate_vf_id - helper to check if VF ID is valid - * @pf: pointer to the PF structure - * @vf_id: the ID of the VF to check + * ice_get_vf_by_id - Get pointer to VF by ID + * @pf: the PF private structure + * @vf_id: the VF ID to locate + * + * Locate and return a pointer to the VF structure associated with a given ID. + * Returns NULL if the ID does not have a valid VF structure associated with + * it. */ -static int ice_validate_vf_id(struct ice_pf *pf, u16 vf_id) +struct ice_vf *ice_get_vf_by_id(struct ice_pf *pf, u16 vf_id) { - /* vf_id range is only valid for 0-255, and should always be unsigned */ + if (!pf->vfs.table) { + dev_err(ice_pf_to_dev(pf), "VF table not allocated\n"); + return NULL; + } + if (vf_id >= pf->vfs.num_alloc) { - dev_err(ice_pf_to_dev(pf), "Invalid VF ID: %u\n", vf_id); - return -EINVAL; + dev_err(ice_pf_to_dev(pf), "Out of range VF ID: %u\n", + vf_id); + return NULL; } - return 0; + + return &pf->vfs.table[vf_id]; +} + +/** + * ice_has_vfs - Return true if the PF has any associated VFs + * @pf: the PF private structure + * + * Return whether or not the PF has any allocated VFs. + * + * Note that this function only guarantees that there are no VFs at the point + * of calling it. It does not guarantee that no more VFs will be added. + */ +bool ice_has_vfs(struct ice_pf *pf) +{ + return pf->vfs.table && pf->vfs.num_alloc > 0; +} + +/** + * ice_get_num_vfs - Get number of allocated VFs + * @pf: the PF private structure + * + * Return the total number of allocated VFs. NOTE: VF IDs are not guaranteed + * to be contiguous. Do not assume that a VF ID is guaranteed to be less than + * the output of this function. + */ +u16 ice_get_num_vfs(struct ice_pf *pf) +{ + if (!pf->vfs.table) + return 0; + + return pf->vfs.num_alloc; } /** @@ -503,7 +543,7 @@ void ice_free_vfs(struct ice_pf *pf) struct ice_vf *vf; unsigned int bkt; - if (!vfs->table) + if (!ice_has_vfs(pf)) return; ice_eswitch_release(pf); @@ -1464,7 +1504,7 @@ bool ice_reset_all_vfs(struct ice_pf *pf, bool is_vflr) unsigned int bkt; /* If we don't have any VFs, then there is nothing to reset */ - if (!pf->vfs.num_alloc) + if (!ice_has_vfs(pf)) return false; /* clear all malicious info if the VFs are getting reset */ @@ -1709,7 +1749,7 @@ void ice_vc_notify_reset(struct ice_pf *pf) { struct virtchnl_pf_event pfe; - if (!pf->vfs.num_alloc) + if (!ice_has_vfs(pf)) return; pfe.event = VIRTCHNL_EVENT_RESET_IMPENDING; @@ -1725,14 +1765,7 @@ void ice_vc_notify_reset(struct ice_pf *pf) static void ice_vc_notify_vf_reset(struct ice_vf *vf) { struct virtchnl_pf_event pfe; - struct ice_pf *pf; - - if (!vf) - return; - - pf = vf->pf; - if (ice_validate_vf_id(pf, vf->vf_id)) - return; + struct ice_pf *pf = vf->pf; /* Bail out if VF is in disabled state, neither initialized, nor active * state - otherwise proceed with notifications @@ -2097,7 +2130,7 @@ void ice_process_vflr_event(struct ice_pf *pf) u32 reg; if (!test_and_clear_bit(ICE_VFLR_EVENT_PENDING, pf->state) || - !pf->vfs.num_alloc) + !ice_has_vfs(pf)) return; ice_for_each_vf(pf, bkt, vf) { @@ -2968,10 +3001,11 @@ int ice_set_vf_spoofchk(struct net_device *netdev, int vf_id, bool ena) int ret; dev = ice_pf_to_dev(pf); - if (ice_validate_vf_id(pf, vf_id)) + + vf = ice_get_vf_by_id(pf, vf_id); + if (!vf) return -EINVAL; - vf = &pf->vfs.table[vf_id]; ret = ice_check_vf_ready_for_cfg(vf); if (ret) return ret; @@ -4159,8 +4193,6 @@ ice_set_vf_port_vlan(struct net_device *netdev, int vf_id, u16 vlan_id, u8 qos, int ret; dev = ice_pf_to_dev(pf); - if (ice_validate_vf_id(pf, vf_id)) - return -EINVAL; if (vlan_id >= VLAN_N_VID || qos > 7) { dev_err(dev, "Invalid Port VLAN parameters for VF %d, ID %d, QoS %d\n", @@ -4174,7 +4206,10 @@ ice_set_vf_port_vlan(struct net_device *netdev, int vf_id, u16 vlan_id, u8 qos, return -EPROTONOSUPPORT; } - vf = &pf->vfs.table[vf_id]; + vf = ice_get_vf_by_id(pf, vf_id); + if (!vf) + return -EINVAL; + ret = ice_check_vf_ready_for_cfg(vf); if (ret) return ret; @@ -5722,14 +5757,14 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) int err = 0; dev = ice_pf_to_dev(pf); - if (ice_validate_vf_id(pf, vf_id)) { + + vf = ice_get_vf_by_id(pf, vf_id); + if (!vf) { dev_err(dev, "Unable to locate VF for message from VF ID %d, opcode %d, len %d\n", vf_id, v_opcode, msglen); return; } - vf = &pf->vfs.table[vf_id]; - /* Check if VF is disabled. */ if (test_bit(ICE_VF_STATE_DIS, vf->vf_states)) { err = -EPERM; @@ -5898,14 +5933,15 @@ ice_get_vf_cfg(struct net_device *netdev, int vf_id, struct ifla_vf_info *ivi) { struct ice_pf *pf = ice_netdev_to_pf(netdev); struct ice_vf *vf; + int ret; - if (ice_validate_vf_id(pf, vf_id)) + vf = ice_get_vf_by_id(pf, vf_id); + if (!vf) return -EINVAL; - vf = &pf->vfs.table[vf_id]; - - if (ice_check_vf_init(pf, vf)) - return -EBUSY; + ret = ice_check_vf_ready_for_cfg(vf); + if (ret) + return ret; ivi->vf = vf_id; ether_addr_copy(ivi->mac, vf->hw_lan_addr.addr); @@ -5976,15 +6012,15 @@ int ice_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac) struct ice_vf *vf; int ret; - if (ice_validate_vf_id(pf, vf_id)) - return -EINVAL; - if (is_multicast_ether_addr(mac)) { netdev_err(netdev, "%pM not a valid unicast address\n", mac); return -EINVAL; } - vf = &pf->vfs.table[vf_id]; + vf = ice_get_vf_by_id(pf, vf_id); + if (!vf) + return -EINVAL; + /* nothing left to do, unicast MAC already set */ if (ether_addr_equal(vf->dev_lan_addr.addr, mac) && ether_addr_equal(vf->hw_lan_addr.addr, mac)) @@ -6043,10 +6079,10 @@ int ice_set_vf_trust(struct net_device *netdev, int vf_id, bool trusted) return -EOPNOTSUPP; } - if (ice_validate_vf_id(pf, vf_id)) + vf = ice_get_vf_by_id(pf, vf_id); + if (!vf) return -EINVAL; - vf = &pf->vfs.table[vf_id]; ret = ice_check_vf_ready_for_cfg(vf); if (ret) return ret; @@ -6081,10 +6117,10 @@ int ice_set_vf_link_state(struct net_device *netdev, int vf_id, int link_state) struct ice_vf *vf; int ret; - if (ice_validate_vf_id(pf, vf_id)) + vf = ice_get_vf_by_id(pf, vf_id); + if (!vf) return -EINVAL; - vf = &pf->vfs.table[vf_id]; ret = ice_check_vf_ready_for_cfg(vf); if (ret) return ret; @@ -6176,10 +6212,11 @@ ice_set_vf_bw(struct net_device *netdev, int vf_id, int min_tx_rate, int ret; dev = ice_pf_to_dev(pf); - if (ice_validate_vf_id(pf, vf_id)) + + vf = ice_get_vf_by_id(pf, vf_id); + if (!vf) return -EINVAL; - vf = &pf->vfs.table[vf_id]; ret = ice_check_vf_ready_for_cfg(vf); if (ret) return ret; @@ -6243,10 +6280,10 @@ int ice_get_vf_stats(struct net_device *netdev, int vf_id, struct ice_vf *vf; int ret; - if (ice_validate_vf_id(pf, vf_id)) + vf = ice_get_vf_by_id(pf, vf_id); + if (!vf) return -EINVAL; - vf = &pf->vfs.table[vf_id]; ret = ice_check_vf_ready_for_cfg(vf); if (ret) return ret; @@ -6384,10 +6421,10 @@ ice_is_malicious_vf(struct ice_pf *pf, struct ice_rq_event_info *event, struct ice_vf *vf; int status; - if (ice_validate_vf_id(pf, vf_id)) + vf = ice_get_vf_by_id(pf, vf_id); + if (!vf) return false; - vf = &pf->vfs.table[vf_id]; /* Check if VF is disabled. */ if (test_bit(ICE_VF_STATE_DIS, vf->vf_states)) return false; diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h index 1448e31772157..bc86358fc8e26 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h @@ -39,6 +39,13 @@ #define ICE_MAX_VF_RESET_TRIES 40 #define ICE_MAX_VF_RESET_SLEEP_MS 20 +/* VF Hash Table access functions + * + * These functions provide abstraction for interacting with the VF hash table. + * In general, direct access to the hash table should be avoided outside of + * these functions where possible. + */ + /** * ice_for_each_vf - Iterate over each VF entry * @pf: pointer to the PF private structure @@ -185,6 +192,9 @@ struct ice_vf { }; #ifdef CONFIG_PCI_IOV +struct ice_vf *ice_get_vf_by_id(struct ice_pf *pf, u16 vf_id); +bool ice_has_vfs(struct ice_pf *pf); +u16 ice_get_num_vfs(struct ice_pf *pf); struct ice_vsi *ice_get_vf_vsi(struct ice_vf *vf); void ice_process_vflr_event(struct ice_pf *pf); int ice_sriov_configure(struct pci_dev *pdev, int num_vfs); @@ -244,6 +254,21 @@ ice_vc_send_msg_to_vf(struct ice_vf *vf, u32 v_opcode, bool ice_vc_isvalid_vsi_id(struct ice_vf *vf, u16 vsi_id); bool ice_vf_is_port_vlan_ena(struct ice_vf *vf); #else /* CONFIG_PCI_IOV */ +static inline struct ice_vf *ice_get_vf_by_id(struct ice_pf *pf, u16 vf_id) +{ + return NULL; +} + +static inline bool ice_has_vfs(struct ice_pf *pf) +{ + return false; +} + +static inline u16 ice_get_num_vfs(struct ice_pf *pf) +{ + return 0; +} + static inline void ice_process_vflr_event(struct ice_pf *pf) { } static inline void ice_free_vfs(struct ice_pf *pf) { } static inline From 3d5985a185e6abfc0b38ed187819016a79eca864 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Wed, 16 Feb 2022 13:37:38 -0800 Subject: [PATCH 11/11] ice: convert VF storage to hash table with krefs and RCU The ice driver stores VF structures in a simple array which is allocated once at the time of VF creation. The VF structures are then accessed from the array by their VF ID. The ID must be between 0 and the number of allocated VFs. Multiple threads can access this table: * .ndo operations such as .ndo_get_vf_cfg or .ndo_set_vf_trust * interrupts, such as due to messages from the VF using the virtchnl communication * processing such as device reset * commands to add or remove VFs The current implementation does not keep track of when all threads are done operating on a VF and can potentially result in use-after-free issues caused by one thread accessing a VF structure after it has been released when removing VFs. Some of these are prevented with various state flags and checks. In addition, this structure is quite static and does not support a planned future where virtualization can be more dynamic. As we begin to look at supporting Scalable IOV with the ice driver (as opposed to just supporting Single Root IOV), this structure is not sufficient. In the future, VFs will be able to be added and removed individually and dynamically. To allow for this, and to better protect against a whole class of use-after-free bugs, replace the VF storage with a combination of a hash table and krefs to reference track all of the accesses to VFs through the hash table. A hash table still allows efficient look up of the VF given its ID, but also allows adding and removing VFs. It does not require contiguous VF IDs. The use of krefs allows the cleanup of the VF memory to be delayed until after all threads have released their reference (by calling ice_put_vf). To prevent corruption of the hash table, a combination of RCU and the mutex table_lock are used. Addition and removal from the hash table use the RCU-aware hash macros. This allows simple read-only look ups that iterate to locate a single VF can be fast using RCU. Accesses which modify the hash table, or which can't take RCU because they sleep, will hold the mutex lock. By using this design, we have a stronger guarantee that the VF structure can't be released until after all threads are finished operating on it. We also pave the way for the more dynamic Scalable IOV implementation in the future. Signed-off-by: Jacob Keller Tested-by: Konrad Jankowski Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_eswitch.c | 16 + drivers/net/ethernet/intel/ice/ice_ethtool.c | 13 +- drivers/net/ethernet/intel/ice/ice_lib.c | 24 +- drivers/net/ethernet/intel/ice/ice_main.c | 8 + drivers/net/ethernet/intel/ice/ice_repr.c | 4 + .../ethernet/intel/ice/ice_virtchnl_fdir.c | 2 + .../net/ethernet/intel/ice/ice_virtchnl_pf.c | 376 +++++++++++++----- .../net/ethernet/intel/ice/ice_virtchnl_pf.h | 45 ++- 8 files changed, 363 insertions(+), 125 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c index c27013afcadb8..9a84d746a6c4e 100644 --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c @@ -209,6 +209,8 @@ static void ice_eswitch_remap_rings_to_vectors(struct ice_pf *pf) rx_ring->q_vector = q_vector; rx_ring->next = NULL; rx_ring->netdev = repr->netdev; + + ice_put_vf(vf); } } @@ -223,6 +225,8 @@ ice_eswitch_release_reprs(struct ice_pf *pf, struct ice_vsi *ctrl_vsi) struct ice_vf *vf; unsigned int bkt; + lockdep_assert_held(&pf->vfs.table_lock); + ice_for_each_vf(pf, bkt, vf) { struct ice_vsi *vsi = vf->repr->src_vsi; @@ -251,6 +255,8 @@ static int ice_eswitch_setup_reprs(struct ice_pf *pf) struct ice_vf *vf; unsigned int bkt; + lockdep_assert_held(&pf->vfs.table_lock); + ice_for_each_vf(pf, bkt, vf) { struct ice_vsi *vsi = vf->repr->src_vsi; @@ -430,6 +436,8 @@ static void ice_eswitch_napi_del(struct ice_pf *pf) struct ice_vf *vf; unsigned int bkt; + lockdep_assert_held(&pf->vfs.table_lock); + ice_for_each_vf(pf, bkt, vf) netif_napi_del(&vf->repr->q_vector->napi); } @@ -443,6 +451,8 @@ static void ice_eswitch_napi_enable(struct ice_pf *pf) struct ice_vf *vf; unsigned int bkt; + lockdep_assert_held(&pf->vfs.table_lock); + ice_for_each_vf(pf, bkt, vf) napi_enable(&vf->repr->q_vector->napi); } @@ -456,6 +466,8 @@ static void ice_eswitch_napi_disable(struct ice_pf *pf) struct ice_vf *vf; unsigned int bkt; + lockdep_assert_held(&pf->vfs.table_lock); + ice_for_each_vf(pf, bkt, vf) napi_disable(&vf->repr->q_vector->napi); } @@ -629,6 +641,8 @@ static void ice_eswitch_start_all_tx_queues(struct ice_pf *pf) struct ice_vf *vf; unsigned int bkt; + lockdep_assert_held(&pf->vfs.table_lock); + if (test_bit(ICE_DOWN, pf->state)) return; @@ -647,6 +661,8 @@ void ice_eswitch_stop_all_tx_queues(struct ice_pf *pf) struct ice_vf *vf; unsigned int bkt; + lockdep_assert_held(&pf->vfs.table_lock); + if (test_bit(ICE_DOWN, pf->state)) return; diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c index 83680eaa35658..399625892f9e1 100644 --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c @@ -316,15 +316,20 @@ ice_get_eeprom(struct net_device *netdev, struct ethtool_eeprom *eeprom, */ static bool ice_active_vfs(struct ice_pf *pf) { + bool active = false; struct ice_vf *vf; unsigned int bkt; - ice_for_each_vf(pf, bkt, vf) { - if (test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) - return true; + rcu_read_lock(); + ice_for_each_vf_rcu(pf, bkt, vf) { + if (test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) { + active = true; + break; + } } + rcu_read_unlock(); - return false; + return active; } /** diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c index 6ea1d97418086..113a2c56c14ce 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_lib.c @@ -439,8 +439,10 @@ static irqreturn_t ice_eswitch_msix_clean_rings(int __always_unused irq, void *d if (!q_vector->tx.tx_ring && !q_vector->rx.rx_ring) return IRQ_HANDLED; - ice_for_each_vf(pf, bkt, vf) + rcu_read_lock(); + ice_for_each_vf_rcu(pf, bkt, vf) napi_schedule(&vf->repr->q_vector->napi); + rcu_read_unlock(); return IRQ_HANDLED; } @@ -1345,11 +1347,17 @@ static int ice_get_vf_ctrl_res(struct ice_pf *pf, struct ice_vsi *vsi) { struct ice_vf *vf; unsigned int bkt; + int base; - ice_for_each_vf(pf, bkt, vf) { - if (vf != vsi->vf && vf->ctrl_vsi_idx != ICE_NO_VSI) - return pf->vsi[vf->ctrl_vsi_idx]->base_vector; + rcu_read_lock(); + ice_for_each_vf_rcu(pf, bkt, vf) { + if (vf != vsi->vf && vf->ctrl_vsi_idx != ICE_NO_VSI) { + base = pf->vsi[vf->ctrl_vsi_idx]->base_vector; + rcu_read_unlock(); + return base; + } } + rcu_read_unlock(); return ice_get_res(pf, pf->irq_tracker, vsi->num_q_vectors, ICE_RES_VF_CTRL_VEC_ID); @@ -2894,10 +2902,14 @@ static void ice_free_vf_ctrl_res(struct ice_pf *pf, struct ice_vsi *vsi) struct ice_vf *vf; unsigned int bkt; - ice_for_each_vf(pf, bkt, vf) { - if (vf != vsi->vf && vf->ctrl_vsi_idx != ICE_NO_VSI) + rcu_read_lock(); + ice_for_each_vf_rcu(pf, bkt, vf) { + if (vf != vsi->vf && vf->ctrl_vsi_idx != ICE_NO_VSI) { + rcu_read_unlock(); return; + } } + rcu_read_unlock(); /* No other VFs left that have control VSI. It is now safe to reclaim * SW interrupts back to the common pool. diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index 9028a925743a5..289e5c99e3130 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -521,8 +521,10 @@ ice_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type) ice_vc_notify_reset(pf); /* Disable VFs until reset is completed */ + mutex_lock(&pf->vfs.table_lock); ice_for_each_vf(pf, bkt, vf) ice_set_vf_state_qs_dis(vf); + mutex_unlock(&pf->vfs.table_lock); if (ice_is_eswitch_mode_switchdev(pf)) { if (reset_type != ICE_RESET_PFR) @@ -1756,6 +1758,7 @@ static void ice_handle_mdd_event(struct ice_pf *pf) /* Check to see if one of the VFs caused an MDD event, and then * increment counters and set print pending */ + mutex_lock(&pf->vfs.table_lock); ice_for_each_vf(pf, bkt, vf) { reg = rd32(hw, VP_MDET_TX_PQM(vf->vf_id)); if (reg & VP_MDET_TX_PQM_VALID_M) { @@ -1811,6 +1814,7 @@ static void ice_handle_mdd_event(struct ice_pf *pf) } } } + mutex_unlock(&pf->vfs.table_lock); ice_print_vfs_mdd_events(pf); } @@ -3680,6 +3684,7 @@ static void ice_deinit_pf(struct ice_pf *pf) mutex_destroy(&pf->sw_mutex); mutex_destroy(&pf->tc_mutex); mutex_destroy(&pf->avail_q_mutex); + mutex_destroy(&pf->vfs.table_lock); if (pf->avail_txqs) { bitmap_free(pf->avail_txqs); @@ -3779,6 +3784,9 @@ static int ice_init_pf(struct ice_pf *pf) return -ENOMEM; } + mutex_init(&pf->vfs.table_lock); + hash_init(pf->vfs.table); + return 0; } diff --git a/drivers/net/ethernet/intel/ice/ice_repr.c b/drivers/net/ethernet/intel/ice/ice_repr.c index 0517eb807b7f0..2adfaf21e0561 100644 --- a/drivers/net/ethernet/intel/ice/ice_repr.c +++ b/drivers/net/ethernet/intel/ice/ice_repr.c @@ -341,6 +341,8 @@ void ice_repr_rem_from_all_vfs(struct ice_pf *pf) struct ice_vf *vf; unsigned int bkt; + lockdep_assert_held(&pf->vfs.table_lock); + ice_for_each_vf(pf, bkt, vf) ice_repr_rem(vf); } @@ -355,6 +357,8 @@ int ice_repr_add_for_all_vfs(struct ice_pf *pf) unsigned int bkt; int err; + lockdep_assert_held(&pf->vfs.table_lock); + ice_for_each_vf(pf, bkt, vf) { err = ice_repr_add(vf); if (err) diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c index df7a3f251cd33..07989f1d08efa 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c @@ -1578,6 +1578,7 @@ void ice_flush_fdir_ctx(struct ice_pf *pf) if (!test_and_clear_bit(ICE_FD_VF_FLUSH_CTX, pf->state)) return; + mutex_lock(&pf->vfs.table_lock); ice_for_each_vf(pf, bkt, vf) { struct device *dev = ice_pf_to_dev(pf); enum virtchnl_fdir_prgm_status status; @@ -1634,6 +1635,7 @@ void ice_flush_fdir_ctx(struct ice_pf *pf) ctx->flags &= ~ICE_VF_FDIR_CTX_VALID; spin_unlock_irqrestore(&vf->fdir.ctx_lock, flags); } + mutex_unlock(&pf->vfs.table_lock); } /** diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c index 535d5f1a763f3..4840570c494da 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c @@ -182,21 +182,61 @@ struct ice_vsi *ice_get_vf_vsi(struct ice_vf *vf) * Locate and return a pointer to the VF structure associated with a given ID. * Returns NULL if the ID does not have a valid VF structure associated with * it. + * + * This function takes a reference to the VF, which must be released by + * calling ice_put_vf() once the caller is finished accessing the VF structure + * returned. */ struct ice_vf *ice_get_vf_by_id(struct ice_pf *pf, u16 vf_id) { - if (!pf->vfs.table) { - dev_err(ice_pf_to_dev(pf), "VF table not allocated\n"); - return NULL; - } + struct ice_vf *vf; - if (vf_id >= pf->vfs.num_alloc) { - dev_err(ice_pf_to_dev(pf), "Out of range VF ID: %u\n", - vf_id); - return NULL; + rcu_read_lock(); + hash_for_each_possible_rcu(pf->vfs.table, vf, entry, vf_id) { + if (vf->vf_id == vf_id) { + struct ice_vf *found; + + if (kref_get_unless_zero(&vf->refcnt)) + found = vf; + else + found = NULL; + + rcu_read_unlock(); + return found; + } } + rcu_read_unlock(); + + return NULL; +} + +/** + * ice_release_vf - Release VF associated with a refcount + * @ref: the kref decremented to zero + * + * Callback function for kref_put to release a VF once its reference count has + * hit zero. + */ +static void ice_release_vf(struct kref *ref) +{ + struct ice_vf *vf = container_of(ref, struct ice_vf, refcnt); + + mutex_destroy(&vf->cfg_lock); + + kfree_rcu(vf, rcu); +} - return &pf->vfs.table[vf_id]; +/** + * ice_put_vf - Release a reference to a VF + * @vf: the VF structure to decrease reference count on + * + * This must be called after ice_get_vf_by_id() once the reference to the VF + * structure is no longer used. Otherwise, the VF structure will never be + * freed. + */ +void ice_put_vf(struct ice_vf *vf) +{ + kref_put(&vf->refcnt, ice_release_vf); } /** @@ -210,7 +250,10 @@ struct ice_vf *ice_get_vf_by_id(struct ice_pf *pf, u16 vf_id) */ bool ice_has_vfs(struct ice_pf *pf) { - return pf->vfs.table && pf->vfs.num_alloc > 0; + /* A simple check that the hash table is not empty does not require + * the mutex or rcu_read_lock. + */ + return !hash_empty(pf->vfs.table); } /** @@ -223,10 +266,16 @@ bool ice_has_vfs(struct ice_pf *pf) */ u16 ice_get_num_vfs(struct ice_pf *pf) { - if (!pf->vfs.table) - return 0; + struct ice_vf *vf; + unsigned int bkt; + u16 num_vfs = 0; + + rcu_read_lock(); + ice_for_each_vf_rcu(pf, bkt, vf) + num_vfs++; + rcu_read_unlock(); - return pf->vfs.num_alloc; + return num_vfs; } /** @@ -244,6 +293,32 @@ static int ice_check_vf_init(struct ice_pf *pf, struct ice_vf *vf) return 0; } +/** + * ice_free_vf_entries - Free all VF entries from the hash table + * @pf: pointer to the PF structure + * + * Iterate over the VF hash table, removing and releasing all VF entries. + * Called during VF teardown or as cleanup during failed VF initialization. + */ +static void ice_free_vf_entries(struct ice_pf *pf) +{ + struct ice_vfs *vfs = &pf->vfs; + struct hlist_node *tmp; + struct ice_vf *vf; + unsigned int bkt; + + /* Remove all VFs from the hash table and release their main + * reference. Once all references to the VF are dropped, ice_put_vf() + * will call ice_release_vf which will remove the VF memory. + */ + lockdep_assert_held(&vfs->table_lock); + + hash_for_each_safe(vfs->table, bkt, tmp, vf, entry) { + hash_del_rcu(&vf->entry); + ice_put_vf(vf); + } +} + /** * ice_vc_vf_broadcast - Broadcast a message to all VFs on PF * @pf: pointer to the PF structure @@ -260,6 +335,7 @@ ice_vc_vf_broadcast(struct ice_pf *pf, enum virtchnl_ops v_opcode, struct ice_vf *vf; unsigned int bkt; + mutex_lock(&pf->vfs.table_lock); ice_for_each_vf(pf, bkt, vf) { /* Not all vfs are enabled so skip the ones that are not */ if (!test_bit(ICE_VF_STATE_INIT, vf->vf_states) && @@ -272,6 +348,7 @@ ice_vc_vf_broadcast(struct ice_pf *pf, enum virtchnl_ops v_opcode, ice_aq_send_msg_to_vf(hw, vf->vf_id, v_opcode, v_retval, msg, msglen, NULL); } + mutex_unlock(&pf->vfs.table_lock); } /** @@ -546,8 +623,6 @@ void ice_free_vfs(struct ice_pf *pf) if (!ice_has_vfs(pf)) return; - ice_eswitch_release(pf); - while (test_and_set_bit(ICE_VF_DIS, pf->state)) usleep_range(1000, 2000); @@ -560,6 +635,10 @@ void ice_free_vfs(struct ice_pf *pf) else dev_warn(dev, "VFs are assigned - not disabling SR-IOV\n"); + mutex_lock(&vfs->table_lock); + + ice_eswitch_release(pf); + ice_for_each_vf(pf, bkt, vf) { mutex_lock(&vf->cfg_lock); @@ -587,17 +666,15 @@ void ice_free_vfs(struct ice_pf *pf) vf->vf_id); mutex_unlock(&vf->cfg_lock); - - mutex_destroy(&vf->cfg_lock); } if (ice_sriov_free_msix_res(pf)) dev_err(dev, "Failed to free MSIX resources used by SR-IOV\n"); vfs->num_qps_per = 0; - vfs->num_alloc = 0; - devm_kfree(dev, vfs->table); - vfs->table = NULL; + ice_free_vf_entries(pf); + + mutex_unlock(&vfs->table_lock); clear_bit(ICE_VF_DIS, pf->state); clear_bit(ICE_FLAG_SRIOV_ENA, pf->flags); @@ -1200,6 +1277,8 @@ static int ice_set_per_vf_res(struct ice_pf *pf, u16 num_vfs) int msix_avail_per_vf, msix_avail_for_sriov; struct device *dev = ice_pf_to_dev(pf); + lockdep_assert_held(&pf->vfs.table_lock); + if (!num_vfs || max_valid_res_idx < 0) return -EINVAL; @@ -1507,6 +1586,8 @@ bool ice_reset_all_vfs(struct ice_pf *pf, bool is_vflr) if (!ice_has_vfs(pf)) return false; + mutex_lock(&pf->vfs.table_lock); + /* clear all malicious info if the VFs are getting reset */ ice_for_each_vf(pf, bkt, vf) if (ice_mbx_clear_malvf(&hw->mbx_snapshot, pf->vfs.malvfs, @@ -1515,8 +1596,10 @@ bool ice_reset_all_vfs(struct ice_pf *pf, bool is_vflr) vf->vf_id); /* If VFs have been disabled, there is no need to reset */ - if (test_and_set_bit(ICE_VF_DIS, pf->state)) + if (test_and_set_bit(ICE_VF_DIS, pf->state)) { + mutex_unlock(&pf->vfs.table_lock); return false; + } /* Begin reset on all VFs at once */ ice_for_each_vf(pf, bkt, vf) @@ -1583,6 +1666,8 @@ bool ice_reset_all_vfs(struct ice_pf *pf, bool is_vflr) ice_flush(hw); clear_bit(ICE_VF_DIS, pf->state); + mutex_unlock(&pf->vfs.table_lock); + return true; } @@ -1735,8 +1820,10 @@ void ice_vc_notify_link_state(struct ice_pf *pf) struct ice_vf *vf; unsigned int bkt; + mutex_lock(&pf->vfs.table_lock); ice_for_each_vf(pf, bkt, vf) ice_vc_notify_vf_link_state(vf); + mutex_unlock(&pf->vfs.table_lock); } /** @@ -1855,6 +1942,8 @@ static int ice_start_vfs(struct ice_pf *pf) struct ice_vf *vf; int retval; + lockdep_assert_held(&pf->vfs.table_lock); + it_cnt = 0; ice_for_each_vf(pf, bkt, vf) { ice_clear_vf_reset_trigger(vf); @@ -1889,18 +1978,38 @@ static int ice_start_vfs(struct ice_pf *pf) } /** - * ice_set_dflt_settings_vfs - set VF defaults during initialization/creation - * @pf: PF holding reference to all VFs for default configuration + * ice_create_vf_entries - Allocate and insert VF entries + * @pf: pointer to the PF structure + * @num_vfs: the number of VFs to allocate + * + * Allocate new VF entries and insert them into the hash table. Set some + * basic default fields for initializing the new VFs. + * + * After this function exits, the hash table will have num_vfs entries + * inserted. + * + * Returns 0 on success or an integer error code on failure. */ -static void ice_set_dflt_settings_vfs(struct ice_pf *pf) +static int ice_create_vf_entries(struct ice_pf *pf, u16 num_vfs) { + struct ice_vfs *vfs = &pf->vfs; struct ice_vf *vf; - unsigned int bkt; - u16 vf_id = 0; + u16 vf_id; + int err; + + lockdep_assert_held(&vfs->table_lock); + + for (vf_id = 0; vf_id < num_vfs; vf_id++) { + vf = kzalloc(sizeof(*vf), GFP_KERNEL); + if (!vf) { + err = -ENOMEM; + goto err_free_entries; + } + kref_init(&vf->refcnt); - ice_for_each_vf(pf, bkt, vf) { vf->pf = pf; - vf->vf_id = vf_id++; + vf->vf_id = vf_id; + vf->vf_sw_id = pf->first_sw; /* assign default capabilities */ set_bit(ICE_VIRTCHNL_VF_CAP_L2, &vf->vf_caps); @@ -1917,27 +2026,15 @@ static void ice_set_dflt_settings_vfs(struct ice_pf *pf) ice_vc_set_dflt_vf_ops(&vf->vc_ops); mutex_init(&vf->cfg_lock); - } -} -/** - * ice_alloc_vfs - allocate num_vfs in the PF structure - * @pf: PF to store the allocated VFs in - * @num_vfs: number of VFs to allocate - */ -static int ice_alloc_vfs(struct ice_pf *pf, int num_vfs) -{ - struct ice_vf *vfs; - - vfs = devm_kcalloc(ice_pf_to_dev(pf), num_vfs, sizeof(*vfs), - GFP_KERNEL); - if (!vfs) - return -ENOMEM; - - pf->vfs.table = NULL; - pf->vfs.num_alloc = num_vfs; + hash_add_rcu(vfs->table, &vf->entry, vf_id); + } return 0; + +err_free_entries: + ice_free_vf_entries(pf); + return err; } /** @@ -1958,14 +2055,10 @@ static int ice_ena_vfs(struct ice_pf *pf, u16 num_vfs) ice_flush(hw); ret = pci_enable_sriov(pf->pdev, num_vfs); - if (ret) { - pf->vfs.num_alloc = 0; + if (ret) goto err_unroll_intr; - } - ret = ice_alloc_vfs(pf, num_vfs); - if (ret) - goto err_pci_disable_sriov; + mutex_lock(&pf->vfs.table_lock); if (ice_set_per_vf_res(pf, num_vfs)) { dev_err(dev, "Not enough resources for %d VFs, try with fewer number of VFs\n", @@ -1974,12 +2067,17 @@ static int ice_ena_vfs(struct ice_pf *pf, u16 num_vfs) goto err_unroll_sriov; } - ice_set_dflt_settings_vfs(pf); + ret = ice_create_vf_entries(pf, num_vfs); + if (ret) { + dev_err(dev, "Failed to allocate VF entries for %d VFs\n", + num_vfs); + goto err_unroll_sriov; + } if (ice_start_vfs(pf)) { dev_err(dev, "Failed to start VF(s)\n"); ret = -EAGAIN; - goto err_unroll_sriov; + goto err_unroll_vf_entries; } clear_bit(ICE_VF_DIS, pf->state); @@ -1992,13 +2090,14 @@ static int ice_ena_vfs(struct ice_pf *pf, u16 num_vfs) if (test_and_clear_bit(ICE_OICR_INTR_DIS, pf->state)) ice_irq_dynamic_ena(hw, NULL, NULL); + mutex_unlock(&pf->vfs.table_lock); + return 0; +err_unroll_vf_entries: + ice_free_vf_entries(pf); err_unroll_sriov: - devm_kfree(dev, pf->vfs.table); - pf->vfs.table = NULL; - pf->vfs.num_alloc = 0; -err_pci_disable_sriov: + mutex_unlock(&pf->vfs.table_lock); pci_disable_sriov(pf->pdev); err_unroll_intr: /* rearm interrupts here */ @@ -2133,6 +2232,7 @@ void ice_process_vflr_event(struct ice_pf *pf) !ice_has_vfs(pf)) return; + mutex_lock(&pf->vfs.table_lock); ice_for_each_vf(pf, bkt, vf) { u32 reg_idx, bit_idx; @@ -2147,6 +2247,7 @@ void ice_process_vflr_event(struct ice_pf *pf) mutex_unlock(&vf->cfg_lock); } } + mutex_unlock(&pf->vfs.table_lock); } /** @@ -2166,22 +2267,36 @@ static void ice_vc_reset_vf(struct ice_vf *vf) * * If no VF is found who owns the pfq then return NULL, otherwise return a * pointer to the VF who owns the pfq + * + * If this function returns non-NULL, it acquires a reference count of the VF + * structure. The caller is responsible for calling ice_put_vf() to drop this + * reference. */ static struct ice_vf *ice_get_vf_from_pfq(struct ice_pf *pf, u16 pfq) { struct ice_vf *vf; unsigned int bkt; - ice_for_each_vf(pf, bkt, vf) { + rcu_read_lock(); + ice_for_each_vf_rcu(pf, bkt, vf) { struct ice_vsi *vsi; u16 rxq_idx; vsi = ice_get_vf_vsi(vf); ice_for_each_rxq(vsi, rxq_idx) - if (vsi->rxq_map[rxq_idx] == pfq) - return vf; + if (vsi->rxq_map[rxq_idx] == pfq) { + struct ice_vf *found; + + if (kref_get_unless_zero(&vf->refcnt)) + found = vf; + else + found = NULL; + rcu_read_unlock(); + return found; + } } + rcu_read_unlock(); return NULL; } @@ -2225,6 +2340,8 @@ ice_vf_lan_overflow_event(struct ice_pf *pf, struct ice_rq_event_info *event) mutex_lock(&vf->cfg_lock); ice_vc_reset_vf(vf); mutex_unlock(&vf->cfg_lock); + + ice_put_vf(vf); } /** @@ -3008,24 +3125,27 @@ int ice_set_vf_spoofchk(struct net_device *netdev, int vf_id, bool ena) ret = ice_check_vf_ready_for_cfg(vf); if (ret) - return ret; + goto out_put_vf; vf_vsi = ice_get_vf_vsi(vf); if (!vf_vsi) { netdev_err(netdev, "VSI %d for VF %d is null\n", vf->lan_vsi_idx, vf->vf_id); - return -EINVAL; + ret = -EINVAL; + goto out_put_vf; } if (vf_vsi->type != ICE_VSI_VF) { netdev_err(netdev, "Type %d of VSI %d for VF %d is no ICE_VSI_VF\n", vf_vsi->type, vf_vsi->vsi_num, vf->vf_id); - return -ENODEV; + ret = -ENODEV; + goto out_put_vf; } if (ena == vf->spoofchk) { dev_dbg(dev, "VF spoofchk already %s\n", ena ? "ON" : "OFF"); - return 0; + ret = 0; + goto out_put_vf; } if (ena) @@ -3038,6 +3158,8 @@ int ice_set_vf_spoofchk(struct net_device *netdev, int vf_id, bool ena) else vf->spoofchk = ena; +out_put_vf: + ice_put_vf(vf); return ret; } @@ -3050,17 +3172,22 @@ int ice_set_vf_spoofchk(struct net_device *netdev, int vf_id, bool ena) */ bool ice_is_any_vf_in_promisc(struct ice_pf *pf) { + bool is_vf_promisc = false; struct ice_vf *vf; unsigned int bkt; - ice_for_each_vf(pf, bkt, vf) { + rcu_read_lock(); + ice_for_each_vf_rcu(pf, bkt, vf) { /* found a VF that has promiscuous mode configured */ if (test_bit(ICE_VF_STATE_UC_PROMISC, vf->vf_states) || - test_bit(ICE_VF_STATE_MC_PROMISC, vf->vf_states)) - return true; + test_bit(ICE_VF_STATE_MC_PROMISC, vf->vf_states)) { + is_vf_promisc = true; + break; + } } + rcu_read_unlock(); - return false; + return is_vf_promisc; } /** @@ -4212,7 +4339,7 @@ ice_set_vf_port_vlan(struct net_device *netdev, int vf_id, u16 vlan_id, u8 qos, ret = ice_check_vf_ready_for_cfg(vf); if (ret) - return ret; + goto out_put_vf; if (ice_vf_get_port_vlan_prio(vf) == qos && ice_vf_get_port_vlan_tpid(vf) == local_vlan_proto && @@ -4220,7 +4347,8 @@ ice_set_vf_port_vlan(struct net_device *netdev, int vf_id, u16 vlan_id, u8 qos, /* duplicate request, so just return success */ dev_dbg(dev, "Duplicate port VLAN %u, QoS %u, TPID 0x%04x request\n", vlan_id, qos, local_vlan_proto); - return 0; + ret = 0; + goto out_put_vf; } mutex_lock(&vf->cfg_lock); @@ -4235,7 +4363,9 @@ ice_set_vf_port_vlan(struct net_device *netdev, int vf_id, u16 vlan_id, u8 qos, ice_vc_reset_vf(vf); mutex_unlock(&vf->cfg_lock); - return 0; +out_put_vf: + ice_put_vf(vf); + return ret; } /** @@ -5786,6 +5916,7 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) ice_vc_send_msg_to_vf(vf, v_opcode, VIRTCHNL_STATUS_ERR_NOT_SUPPORTED, NULL, 0); + ice_put_vf(vf); return; } @@ -5795,6 +5926,7 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) NULL, 0); dev_err(dev, "Invalid message from VF %d, opcode %d, len %d, error %d\n", vf_id, v_opcode, msglen, err); + ice_put_vf(vf); return; } @@ -5804,6 +5936,7 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) if (!mutex_trylock(&vf->cfg_lock)) { dev_info(dev, "VF %u is being configured in another context that will trigger a VFR, so there is no need to handle this message\n", vf->vf_id); + ice_put_vf(vf); return; } @@ -5918,6 +6051,7 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) } mutex_unlock(&vf->cfg_lock); + ice_put_vf(vf); } /** @@ -5941,7 +6075,7 @@ ice_get_vf_cfg(struct net_device *netdev, int vf_id, struct ifla_vf_info *ivi) ret = ice_check_vf_ready_for_cfg(vf); if (ret) - return ret; + goto out_put_vf; ivi->vf = vf_id; ether_addr_copy(ivi->mac, vf->hw_lan_addr.addr); @@ -5962,7 +6096,10 @@ ice_get_vf_cfg(struct net_device *netdev, int vf_id, struct ifla_vf_info *ivi) ivi->linkstate = IFLA_VF_LINK_STATE_DISABLE; ivi->max_tx_rate = vf->max_tx_rate; ivi->min_tx_rate = vf->min_tx_rate; - return 0; + +out_put_vf: + ice_put_vf(vf); + return ret; } /** @@ -6023,17 +6160,20 @@ int ice_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac) /* nothing left to do, unicast MAC already set */ if (ether_addr_equal(vf->dev_lan_addr.addr, mac) && - ether_addr_equal(vf->hw_lan_addr.addr, mac)) - return 0; + ether_addr_equal(vf->hw_lan_addr.addr, mac)) { + ret = 0; + goto out_put_vf; + } ret = ice_check_vf_ready_for_cfg(vf); if (ret) - return ret; + goto out_put_vf; if (ice_unicast_mac_exists(pf, mac)) { netdev_err(netdev, "Unicast MAC %pM already exists on this PF. Preventing setting VF %u unicast MAC address to %pM\n", mac, vf_id, mac); - return -EINVAL; + ret = -EINVAL; + goto out_put_vf; } mutex_lock(&vf->cfg_lock); @@ -6057,7 +6197,10 @@ int ice_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac) ice_vc_reset_vf(vf); mutex_unlock(&vf->cfg_lock); - return 0; + +out_put_vf: + ice_put_vf(vf); + return ret; } /** @@ -6085,11 +6228,13 @@ int ice_set_vf_trust(struct net_device *netdev, int vf_id, bool trusted) ret = ice_check_vf_ready_for_cfg(vf); if (ret) - return ret; + goto out_put_vf; /* Check if already trusted */ - if (trusted == vf->trusted) - return 0; + if (trusted == vf->trusted) { + ret = 0; + goto out_put_vf; + } mutex_lock(&vf->cfg_lock); @@ -6100,7 +6245,9 @@ int ice_set_vf_trust(struct net_device *netdev, int vf_id, bool trusted) mutex_unlock(&vf->cfg_lock); - return 0; +out_put_vf: + ice_put_vf(vf); + return ret; } /** @@ -6123,7 +6270,7 @@ int ice_set_vf_link_state(struct net_device *netdev, int vf_id, int link_state) ret = ice_check_vf_ready_for_cfg(vf); if (ret) - return ret; + goto out_put_vf; switch (link_state) { case IFLA_VF_LINK_STATE_AUTO: @@ -6138,12 +6285,15 @@ int ice_set_vf_link_state(struct net_device *netdev, int vf_id, int link_state) vf->link_up = false; break; default: - return -EINVAL; + ret = -EINVAL; + goto out_put_vf; } ice_vc_notify_vf_link_state(vf); - return 0; +out_put_vf: + ice_put_vf(vf); + return ret; } /** @@ -6156,8 +6306,10 @@ static int ice_calc_all_vfs_min_tx_rate(struct ice_pf *pf) unsigned int bkt; int rate = 0; - ice_for_each_vf(pf, bkt, vf) + rcu_read_lock(); + ice_for_each_vf_rcu(pf, bkt, vf) rate += vf->min_tx_rate; + rcu_read_unlock(); return rate; } @@ -6219,7 +6371,7 @@ ice_set_vf_bw(struct net_device *netdev, int vf_id, int min_tx_rate, ret = ice_check_vf_ready_for_cfg(vf); if (ret) - return ret; + goto out_put_vf; vsi = ice_get_vf_vsi(vf); @@ -6229,23 +6381,27 @@ ice_set_vf_bw(struct net_device *netdev, int vf_id, int min_tx_rate, if (max_tx_rate && min_tx_rate > max_tx_rate) { dev_err(dev, "Cannot set min Tx rate %d Mbps greater than max Tx rate %d Mbps\n", min_tx_rate, max_tx_rate); - return -EINVAL; + ret = -EINVAL; + goto out_put_vf; } if (min_tx_rate && ice_is_dcb_active(pf)) { dev_err(dev, "DCB on PF is currently enabled. VF min Tx rate limiting not allowed on this PF.\n"); - return -EOPNOTSUPP; + ret = -EOPNOTSUPP; + goto out_put_vf; } - if (ice_min_tx_rate_oversubscribed(vf, min_tx_rate)) - return -EINVAL; + if (ice_min_tx_rate_oversubscribed(vf, min_tx_rate)) { + ret = -EINVAL; + goto out_put_vf; + } if (vf->min_tx_rate != (unsigned int)min_tx_rate) { ret = ice_set_min_bw_limit(vsi, (u64)min_tx_rate * 1000); if (ret) { dev_err(dev, "Unable to set min-tx-rate for VF %d\n", vf->vf_id); - return ret; + goto out_put_vf; } vf->min_tx_rate = min_tx_rate; @@ -6256,13 +6412,15 @@ ice_set_vf_bw(struct net_device *netdev, int vf_id, int min_tx_rate, if (ret) { dev_err(dev, "Unable to set max-tx-rate for VF %d\n", vf->vf_id); - return ret; + goto out_put_vf; } vf->max_tx_rate = max_tx_rate; } - return 0; +out_put_vf: + ice_put_vf(vf); + return ret; } /** @@ -6286,11 +6444,13 @@ int ice_get_vf_stats(struct net_device *netdev, int vf_id, ret = ice_check_vf_ready_for_cfg(vf); if (ret) - return ret; + goto out_put_vf; vsi = ice_get_vf_vsi(vf); - if (!vsi) - return -EINVAL; + if (!vsi) { + ret = -EINVAL; + goto out_put_vf; + } ice_update_eth_stats(vsi); stats = &vsi->eth_stats; @@ -6308,7 +6468,9 @@ int ice_get_vf_stats(struct net_device *netdev, int vf_id, vf_stats->rx_dropped = stats->rx_discards; vf_stats->tx_dropped = stats->tx_discards; - return 0; +out_put_vf: + ice_put_vf(vf); + return ret; } /** @@ -6352,6 +6514,7 @@ void ice_print_vfs_mdd_events(struct ice_pf *pf) pf->vfs.last_printed_mdd_jiffies = jiffies; + mutex_lock(&pf->vfs.table_lock); ice_for_each_vf(pf, bkt, vf) { /* only print Rx MDD event message if there are new events */ if (vf->mdd_rx_events.count != vf->mdd_rx_events.last_printed) { @@ -6370,6 +6533,7 @@ void ice_print_vfs_mdd_events(struct ice_pf *pf) vf->dev_lan_addr.addr); } } + mutex_unlock(&pf->vfs.table_lock); } /** @@ -6425,9 +6589,8 @@ ice_is_malicious_vf(struct ice_pf *pf, struct ice_rq_event_info *event, if (!vf) return false; - /* Check if VF is disabled. */ if (test_bit(ICE_VF_STATE_DIS, vf->vf_states)) - return false; + goto out_put_vf; mbxdata.num_msg_proc = num_msg_proc; mbxdata.num_pending_arq = num_msg_pending; @@ -6438,7 +6601,7 @@ ice_is_malicious_vf(struct ice_pf *pf, struct ice_rq_event_info *event, /* check to see if we have a malicious VF */ status = ice_mbx_vf_state_handler(&pf->hw, &mbxdata, vf_id, &malvf); if (status) - return false; + goto out_put_vf; if (malvf) { bool report_vf = false; @@ -6460,12 +6623,9 @@ ice_is_malicious_vf(struct ice_pf *pf, struct ice_rq_event_info *event, &vf->dev_lan_addr.addr[0], pf_vsi->netdev->dev_addr); } - - return true; } - /* if there was an error in detection or the VF is not malicious then - * return false - */ - return false; +out_put_vf: + ice_put_vf(vf); + return malvf; } diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h index bc86358fc8e26..02e3d306f6dd8 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h @@ -44,22 +44,45 @@ * These functions provide abstraction for interacting with the VF hash table. * In general, direct access to the hash table should be avoided outside of * these functions where possible. + * + * The VF entries in the hash table are protected by reference counting to + * track lifetime of accesses from the table. The ice_get_vf_by_id() function + * obtains a reference to the VF structure which must be dropped by using + * ice_put_vf(). */ /** * ice_for_each_vf - Iterate over each VF entry * @pf: pointer to the PF private structure * @bkt: bucket index used for iteration - * @entry: pointer to the VF entry currently being processed in the loop. + * @vf: pointer to the VF entry currently being processed in the loop. * * The bkt variable is an unsigned integer iterator used to traverse the VF * entries. It is *not* guaranteed to be the VF's vf_id. Do not assume it is. * Use vf->vf_id to get the id number if needed. + * + * The caller is expected to be under the table_lock mutex for the entire + * loop. Use this iterator if your loop is long or if it might sleep. */ -#define ice_for_each_vf(pf, bkt, entry) \ - for ((bkt) = 0, (entry) = &(pf)->vfs.table[0]; \ - (bkt) < (pf)->vfs.num_alloc; \ - (bkt)++, (entry)++) +#define ice_for_each_vf(pf, bkt, vf) \ + hash_for_each((pf)->vfs.table, (bkt), (vf), entry) + +/** + * ice_for_each_vf_rcu - Iterate over each VF entry protected by RCU + * @pf: pointer to the PF private structure + * @bkt: bucket index used for iteration + * @vf: pointer to the VF entry currently being processed in the loop. + * + * The bkt variable is an unsigned integer iterator used to traverse the VF + * entries. It is *not* guaranteed to be the VF's vf_id. Do not assume it is. + * Use vf->vf_id to get the id number if needed. + * + * The caller is expected to be under rcu_read_lock() for the entire loop. + * Only use this iterator if your loop is short and you can guarantee it does + * not sleep. + */ +#define ice_for_each_vf_rcu(pf, bkt, vf) \ + hash_for_each_rcu((pf)->vfs.table, (bkt), (vf), entry) /* Specific VF states */ enum ice_vf_states { @@ -125,8 +148,8 @@ struct ice_vc_vf_ops { /* Virtchnl/SR-IOV config info */ struct ice_vfs { - struct ice_vf *table; /* table of VF entries */ - u16 num_alloc; /* number of allocated VFs */ + DECLARE_HASHTABLE(table, 8); /* table of VF entries */ + struct mutex table_lock; /* Lock for protecting the hash table */ u16 num_supported; /* max supported VFs on this PF */ u16 num_qps_per; /* number of queue pairs per VF */ u16 num_msix_per; /* number of MSI-X vectors per VF */ @@ -136,6 +159,9 @@ struct ice_vfs { /* VF information structure */ struct ice_vf { + struct hlist_node entry; + struct rcu_head rcu; + struct kref refcnt; struct ice_pf *pf; /* Used during virtchnl message handling and NDO ops against the VF @@ -193,6 +219,7 @@ struct ice_vf { #ifdef CONFIG_PCI_IOV struct ice_vf *ice_get_vf_by_id(struct ice_pf *pf, u16 vf_id); +void ice_put_vf(struct ice_vf *vf); bool ice_has_vfs(struct ice_pf *pf); u16 ice_get_num_vfs(struct ice_pf *pf); struct ice_vsi *ice_get_vf_vsi(struct ice_vf *vf); @@ -259,6 +286,10 @@ static inline struct ice_vf *ice_get_vf_by_id(struct ice_pf *pf, u16 vf_id) return NULL; } +static inline void ice_put_vf(struct ice_vf *vf) +{ +} + static inline bool ice_has_vfs(struct ice_pf *pf) { return false;