Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider: - [High] ice: lock inversion between vfs.table_lock and PF devlink lock in ice_init_vf_devlink() - [Medium] ice: VF devlink RSS LUT changes race with PF reset and VF VSI rebuild - [Medium] ice: restored PF vsi->rss_size in ice_rebuild_rss_lut_pf() is overwritten - [Medium] ice: unlocked vsi->rss_lut_type and rss_size in ice_vsi_cfg_rss_lut_key() - [Medium] ice: u8 ring count overflow in ice_vc_map_q_vector_msg() with 256 queues - [Medium] ice: uncapped queue offer and dead code in ice_vc_request_qs_msg() - [Medium] ice: VF devlink registered before resources in ice_init_vf_devlink() - [Low] ice: duplicate ice_get_vf_vsi() lookup in ice_vc_config_rss_lut() --
commit ad0fafa098b2509023a0c2fdd740574478b1b268 Author: Przemek Kitszel <[email protected]> ice: support up to 256 VF queues This commit adds support for up to 256 queues per VF by allowing global (512-entry) and PF (2048-entry) RSS LUTs to be assigned to VFs via nested VF devlink resource instances, deferring VF resets triggered by LUT changes to the service task, and wiring up the large queue pair virtchnl operations. > diff --git a/drivers/net/ethernet/intel/ice/devlink/resource.c > b/drivers/net/ethernet/intel/ice/devlink/resource.c > index b0b2b1adae0c1..5a0ab267bf8bc 100644 > --- a/drivers/net/ethernet/intel/ice/devlink/resource.c > +++ b/drivers/net/ethernet/intel/ice/devlink/resource.c [ ... ] > @@ -414,6 +465,41 @@ static int ice_rss_lut_pf_occ_set_global(u64 size, > ICE_ANY_SLOT, extack); > } > > +static int ice_rss_lut_vf_occ_set_pf(u64 size, struct netlink_ext_ack > *extack, > + void *occ_priv) > +{ > + struct ice_vf *vf = occ_priv; > + struct ice_pf *pf = vf->pf; > + int pf_id; > + > + pf_id = ice_devl_pf_id(pf); > + scoped_guard(mutex, &vf->cfg_lock) { > + if (!ice_vf_is_ready(vf)) > + return -EBUSY; > + > + scoped_guard(ice_adapter_devl, pf->adapter) > + return ice_devl_res_change(size, ICE_RSS_LUT_PF, pf, vf, > + pf_id, extack); > + } > +} > + > +static int ice_rss_lut_vf_occ_set_global(u64 size, > + struct netlink_ext_ack *extack, > + void *occ_priv) > +{ > + struct ice_vf *vf = occ_priv; > + struct ice_pf *pf = vf->pf; > + > + scoped_guard(mutex, &vf->cfg_lock) { > + if (!ice_vf_is_ready(vf)) > + return -EBUSY; > + > + scoped_guard(ice_adapter_devl, pf->adapter) > + return ice_devl_res_change(size, ICE_RSS_LUT_GLOBAL, pf, > + vf, ICE_ANY_SLOT, extack); > + } > +} [Severity: Medium] Can ice_rss_lut_vf_occ_set_pf() and ice_rss_lut_vf_occ_set_global() race with PF reset recovery or SR-IOV teardown and leave the LUT resource in pf->adapter->resources out of sync with the VF VSI state? During a PF reset, ice_prepare_for_reset() calls ice_set_vf_state_dis(), which only clears ICE_VF_STATE_QS_ENA without clearing ICE_VF_STATE_INIT or setting ICE_VF_STATE_DIS in vf->vf_states. Later, in ice_do_reset() and ice_reset_subtask(), the PF reset-in-progress state bits are cleared after ice_rebuild() finishes and before ice_reset_all_vfs() runs: drivers/net/ethernet/intel/ice/ice_main.c:ice_do_reset() { ... ice_rebuild(pf, reset_type); clear_bit(ICE_PREPARED_FOR_RESET, pf->state); clear_bit(ICE_PFR_REQ, pf->state); wake_up(&pf->reset_wait_queue); ice_reset_all_vfs(pf); ... } In the window before ice_reset_all_vfs() triggers the VF resets (and similarly in ice_free_vfs() after ICE_VF_DIS is set on pf->state before the per-VF loop takes vf->cfg_lock), ice_is_reset_in_progress(pf->state) is false while ice_vf_is_ready(vf) is still true because ice_vf_is_ready() only tests ICE_VF_STATE_INIT and !ICE_VF_STATE_DIS (ignoring ICE_VF_DIS on pf->state, unlike ice_is_vf_disabled()). If a devlink resource change runs in that window, ice_devl_res_change() claims the ICE_RSS_LUT_PF or ICE_RSS_LUT_GLOBAL slot for the VF in pf->adapter->resources, updates vsi->rss_lut_type, and sets ICE_VF_STATE_NEEDS_RESET. When ice_reset_all_vfs() then acquires vf->cfg_lock, it clears ICE_VF_STATE_NEEDS_RESET and calls ice_vf_rebuild_vsi()->ice_vsi_rebuild(..., ICE_VSI_FLAG_INIT)-> ice_vsi_cfg_def()->ice_vsi_set_dflt_rss_params(), which unconditionally resets vsi->rss_lut_type to ICE_LUT_VSI and vsi->rss_table_size to ICE_LUT_VSI_SIZE while the VF remains recorded as the owner of the LUT resource in pf->adapter->resources. Should ICE_VF_STATE_DIS (or ICE_VF_DIS on pf->state) be set in ice_prepare_for_reset() and checked by ice_vf_is_ready() so VF LUT changes are rejected until ice_reset_all_vfs() finishes rebuilding the VFs? > @@ -527,6 +613,8 @@ static int ice_rebuild_rss_lut_pf(struct ice_pf *pf) > vsi->rss_lut_user = NULL; > vsi->rss_table_size = ice_lut_type_to_size(ICE_LUT_PF); > vsi->rss_lut_type = ICE_LUT_PF; > + vsi->rss_size = min(vsi->num_rxq, > + ice_lut_type_to_qs_num(ICE_LUT_PF)); > } > mutex_unlock(&pf->rss_lut_lock); > err = 0; [Severity: Medium] Does this vsi->rss_size update in ice_rebuild_rss_lut_pf() get overwritten immediately afterwards during the PF VSI rebuild? In ice_rebuild(), ice_rebuild_rss_luts() runs right before ice_vsi_rebuild_by_type(pf, ICE_VSI_PF): drivers/net/ethernet/intel/ice/ice_main.c:ice_rebuild() { ... err = ice_rebuild_rss_luts(pf); if (err) { dev_err(dev, "RSS LUT rebuild failed: %d\n", err); goto err_vsi_rebuild; } /* rebuild PF VSI */ err = ice_vsi_rebuild_by_type(pf, ICE_VSI_PF); ... } ice_vsi_rebuild_by_type() calls ice_vsi_rebuild(vsi, ICE_VSI_FLAG_INIT)-> ice_vsi_cfg_def(vsi). Because ICE_VSI_FLAG_INIT is set on vsi->flags, ice_vsi_cfg_def() calls ice_vsi_set_dflt_rss_params(vsi), which unconditionally resets vsi->rss_size to min_t(u16, netif_get_num_default_rss_queues(), max_rss_size). If a PF had more Rx queues configured via ethtool -L than netif_get_num_default_rss_queues(), switched to a global LUT, and then went through a PF reset, won't the rebuilt PF RSS LUT stay clamped to netif_get_num_default_rss_queues() instead of vsi->num_rxq? > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c > b/drivers/net/ethernet/intel/ice/ice_lib.c > index 215244a1c043b..b83916bf90bb6 100644 > --- a/drivers/net/ethernet/intel/ice/ice_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c [ ... ] > @@ -1663,7 +1680,12 @@ int ice_vsi_cfg_rss_lut_key(struct ice_vsi *vsi) > (test_bit(ICE_FLAG_TC_MQPRIO, pf->flags))) { > vsi->rss_size = min_t(u16, vsi->rss_size, vsi->ch_rss_size); > } else { > - vsi->rss_size = min_t(u16, vsi->rss_size, vsi->num_rxq); > + /* VF LUT type could change on reset, don't clamp the old value > */ > + if (vsi->type == ICE_VSI_VF) > + vsi->rss_size = min(vsi->num_rxq, > + > ice_lut_type_to_qs_num(vsi->rss_lut_type)); > + else > + vsi->rss_size = min_t(u16, vsi->rss_size, vsi->num_rxq); > > /* If orig_rss_size is valid and it is less than determined > * main VSI's rss_size, update main VSI's rss_size to be [Severity: Medium] Should the vsi->rss_size calculation and read of vsi->rss_lut_type in ice_vsi_cfg_rss_lut_key() be inside the pf->rss_lut_lock critical section just below? Callers such as ice_set_channels()->ice_vsi_recfg_qs()->ice_vsi_rebuild() for the PF and ice_sriov_set_msix_vec_count()->ice_vsi_rebuild() for a VF invoke ice_vsi_cfg_rss_lut_key() without holding pf->adapter->devlink->lock, vf->cfg_lock, or pf->rss_lut_lock. If ice_maybe_change_rss_lut() runs concurrently under pf->rss_lut_lock and switches the VSI to a smaller LUT, computing vsi->rss_size outside pf->rss_lut_lock can read the old vsi->rss_lut_type (or old vsi->rss_size) and overwrite the clamped vsi->rss_size before ice_fill_rss_lut() fills the new LUT. > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c > b/drivers/net/ethernet/intel/ice/ice_vf_lib.c > index 320b393ddaaf0..03c70d62baa7e 100644 > --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c [ ... ] > @@ -1450,3 +1471,49 @@ void ice_vf_update_mac_lldp_num(struct ice_vf *vf, > struct ice_vsi *vsi, > if (was_ena != is_ena) > ice_vsi_cfg_sw_lldp(vsi, false, is_ena); > } > + > +void ice_init_vf_devlink(struct ice_vf *vf) > +{ > + struct devlink *pf_devlink = priv_to_devlink(vf->pf); > + static const struct devlink_ops noop = {}; > + struct devlink *devlink; > + int err; > + > + lockdep_assert_held(&vf->pf->vfs.table_lock); > + > + /* no VF pci_dev when the VFs were not scanned, e.g. on s390 */ > + if (!vf->vfdev) > + return; > + > + devlink = devlink_alloc_drv(&noop, 0, &vf->vfdev->dev, > + ice_pf_to_dev(vf->pf)->driver); > + if (!devlink) > + return; > + > + scoped_guard(devl, pf_devlink) > + err = devl_nested_devlink_set(pf_devlink, devlink); [Severity: High] Doesn't acquiring devl_lock(pf_devlink) inside ice_init_vf_devlink() while holding vf->pf->vfs.table_lock introduce an AB-BA lock inversion with ice_tear_down_devlink_rate_tree()? When SR-IOV VFs are enabled via ice_ena_vfs()->ice_start_vfs()-> ice_init_vf_devlink(), pf->vfs.table_lock is acquired before devl_lock(priv_to_devlink(pf)). However, ice_tear_down_devlink_rate_tree() (called from ice_setup_tc_mqprio_qdisc() and ice_pf_dcb_cfg()) takes those two locks in the opposite order: drivers/net/ethernet/intel/ice/devlink/devlink.c:ice_tear_down_devlink_rate_tree() { struct devlink *devlink; devlink = priv_to_devlink(pf); devl_lock(devlink); mutex_lock(&pf->vfs.table_lock); ... } Unlike ice_eswitch_attach_vf(), which only runs in switchdev mode, ice_init_vf_devlink() runs unconditionally in legacy SR-IOV mode where MQPRIO and DCB configurations are allowed. > + if (err) { > + devlink_free(devlink); > + return; > + } > + > + vf->devlink = devlink; > + devlink_register(devlink); > + > + ice_devlink_vf_resources_register(vf); > +} > + > +void ice_deinit_vf_devlink(struct ice_vf *vf) > +{ > + struct devlink *devlink = vf->devlink; > + > + lockdep_assert_held(&vf->pf->vfs.table_lock); > + > + if (!devlink) > + return; > + > + vf->devlink = NULL; > + devlink_resources_unregister(devlink); > + devlink_unregister(devlink); > + devlink_free(devlink); > +} [Severity: Medium] In ice_init_vf_devlink() and ice_deinit_vf_devlink(), should the VF devlink resources be registered before devlink_register() (and unregistered after devlink_unregister(), or under a single devl_lock(devlink) section using devl_register() and devl_unregister())? Calling devlink_register(devlink) first marks the instance registered, sends the DEVLINK_CMD_NEW notification to userspace, and drops devlink->lock before ice_devlink_vf_resources_register() registers rss, lut_512, and lut_2048. A userspace listener reacting to DEVLINK_CMD_NEW (or querying the VF devlink instance during teardown between devlink_resources_unregister() and devlink_unregister()) can observe the registered devlink instance with no resources attached. > diff --git a/drivers/net/ethernet/intel/ice/virt/queues.c > b/drivers/net/ethernet/intel/ice/virt/queues.c > index f87ccf5d370bb..2d0aeacfdf5ba 100644 > --- a/drivers/net/ethernet/intel/ice/virt/queues.c > +++ b/drivers/net/ethernet/intel/ice/virt/queues.c > @@ -1005,7 +1005,9 @@ int ice_vc_request_qs_msg(struct ice_vf *vf, u8 *msg) > struct ice_pf *pf = vf->pf; > u16 max_allowed_vf_queues; > u16 tx_rx_queue_left; > + struct ice_vsi *vsi; > struct device *dev; > + u16 max_lut_queues; > u16 cur_queues; > > dev = ice_pf_to_dev(pf); > @@ -1014,10 +1016,26 @@ int ice_vc_request_qs_msg(struct ice_vf *vf, u8 *msg) > goto error_param; > } > > + vsi = ice_get_vf_vsi(vf); > + if (!vsi) { > + v_ret = VIRTCHNL_STATUS_ERR_PARAM; > + goto error_param; > + } > + > cur_queues = vf->num_vf_qs; > + /* without LARGE_NUM_QPAIRS, VF can only use the v1 queue opcodes */ > + if (vf->driver_caps & VIRTCHNL_VF_LARGE_NUM_QPAIRS) > + max_lut_queues = ice_lut_type_to_qs_num(vsi->rss_lut_type); > + else > + max_lut_queues = ICE_MAX_QS_PER_VF_VCV1; > tx_rx_queue_left = min_t(u16, ice_get_avail_txq_count(pf), > ice_get_avail_rxq_count(pf)); > max_allowed_vf_queues = tx_rx_queue_left + cur_queues; > + if (req_queues > max_lut_queues) { > + vfres->num_queue_pairs = max_lut_queues; > + goto error_param; > + } > + > if (!req_queues) { > dev_err(dev, "VF %d tried to request 0 queues. Ignoring.\n", > vf->vf_id); [Severity: Medium] When req_queues > max_lut_queues in ice_vc_request_qs_msg(), what happens if max_allowed_vf_queues is smaller than max_lut_queues? Setting vfres->num_queue_pairs = max_lut_queues and jumping to error_param offers max_lut_queues to the VF without clamping to max_allowed_vf_queues, so the VF's retry with the suggested count will fail again if the PF has fewer than max_lut_queues available. Also, since max_lut_queues is at most ICE_MAX_QS_PER_VF (256), the subsequent else if (req_queues > ICE_MAX_QS_PER_VF) branch in ice_vc_request_qs_msg() becomes unreachable. Should max_lut_queues be folded into the upper bound alongside ICE_MAX_QS_PER_VF and max_allowed_vf_queues? > diff --git a/drivers/net/ethernet/intel/ice/virt/rss.c > b/drivers/net/ethernet/intel/ice/virt/rss.c > index 960012ca91b53..8bc984c9cf72d 100644 > --- a/drivers/net/ethernet/intel/ice/virt/rss.c > +++ b/drivers/net/ethernet/intel/ice/virt/rss.c > @@ -1746,7 +1747,13 @@ int ice_vc_config_rss_lut(struct ice_vf *vf, u8 *msg) > goto error_param; > } > > - if (vrl->lut_entries != ICE_LUT_VSI_SIZE) { > + vsi = ice_get_vf_vsi(vf); > + if (!vsi) { > + v_ret = VIRTCHNL_STATUS_ERR_PARAM; > + goto error_param; > + } > + > + if (vrl->lut_entries != vsi->rss_table_size) { > v_ret = VIRTCHNL_STATUS_ERR_PARAM; > goto error_param; > } > @@ -1762,7 +1769,7 @@ int ice_vc_config_rss_lut(struct ice_vf *vf, u8 *msg) > goto error_param; > } > > - if (ice_set_rss_lut(vsi, vrl->lut, ICE_LUT_VSI_SIZE)) > + if (ice_set_rss_lut(vsi, vrl->lut, vrl->lut_entries)) > v_ret = VIRTCHNL_STATUS_ERR_ADMIN_QUEUE_ERROR; > error_param: > return ice_vc_send_msg_to_vf(vf, VIRTCHNL_OP_CONFIG_RSS_LUT, v_ret, [Severity: Low] This isn't a bug, but ice_vc_config_rss_lut() now calls ice_get_vf_vsi(vf) and checks !vsi twice in a row. Can the second lookup just after the ICE_FLAG_RSS_ENA check be dropped? > diff --git a/drivers/net/ethernet/intel/ice/virt/virtchnl.c > b/drivers/net/ethernet/intel/ice/virt/virtchnl.c > index ca8018e3dd428..dc4f7b2b55b2d 100644 > --- a/drivers/net/ethernet/intel/ice/virt/virtchnl.c > +++ b/drivers/net/ethernet/intel/ice/virt/virtchnl.c [ ... ] > @@ -2901,6 +2919,20 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct > ice_rq_event_info *event, > case VIRTCHNL_OP_GET_QOS_CAPS: > err = ops->get_qos_caps(vf); > break; > + case VIRTCHNL_OP_GET_MAX_RSS_QREGION: > + err = ops->get_max_rss_qregion(vf); > + break; > + case VIRTCHNL_OP_ENABLE_QUEUES_V2: > + err = ops->ena_qs_v2_msg(vf, msg, msglen); > + if (!err) > + ice_vc_notify_vf_link_state(vf); > + break; > + case VIRTCHNL_OP_DISABLE_QUEUES_V2: > + err = ops->dis_qs_v2_msg(vf, msg, msglen); > + break; > + case VIRTCHNL_OP_MAP_QUEUE_VECTOR: > + err = ops->map_q_vector_msg(vf, msg, msglen); > + break; [Severity: Medium] With up to 256 VF queues enabled and VIRTCHNL_OP_MAP_QUEUE_VECTOR wired up to ice_vc_map_q_vector_msg(), can q_vector->num_ring_rx and q_vector->num_ring_tx overflow when a VF maps all 256 queues onto a single interrupt vector? In struct ice_q_vector, both counters are u8: drivers/net/ethernet/intel/ice/ice.h:struct ice_q_vector { ... u8 num_ring_rx; /* total number of Rx rings in vector */ u8 num_ring_tx; /* total number of Tx rings in vector */ ... }; When a VF has 256 queues and maps all 256 Rx or Tx rings to one vector in ice_vc_map_q_vector_msg() (or during initial mapping in ice_vsi_map_rings_to_vectors() if the VF has only 1 queue vector), incrementing num_ring_rx or num_ring_tx 256 times wraps the u8 counter to 0, causing ice_vsi_release_msix() and ice_cfg_itr() to treat the vector as having no rings attached. Should num_ring_rx and num_ring_tx in struct ice_q_vector be u16? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=15
