| Message ID | 20230404072833.3676891-5-michal.swiatkowski@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ice: allow matching on meta data | expand |
From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> Date: Tue, 4 Apr 2023 09:28:33 +0200 > The use of a source MAC to direct packets from the VF to the > corresponding port representor is only ok if there is only one > MAC on a VF. To support this functionality when the number > of MACs on a VF is greater, it is necessary to match a source > VSI instead of a source MAC. [...] > @@ -32,11 +31,9 @@ > if·(!list) > return·-ENOMEM; > > - list[0].type·=·ICE_MAC_OFOS; > - ether_addr_copy(list[0].h_u.eth_hdr.src_addr,·mac); > - eth_broadcast_addr(list[0].m_u.eth_hdr.src_addr); > + ice_rule_add_src_vsi_metadata(&list[0]); &list[0] == list. > - rule_info.sw_act.flag·|=·ICE_FLTR_TX; > + rule_info.sw_act.flag·=·ICE_FLTR_TX; > rule_info.sw_act.vsi_handle·=·ctrl_vsi->idx; [...] > @@ -269,10 +235,18 @@ static int ice_eswitch_setup_reprs(struct ice_pf *pf) > goto err; > } > > + if (ice_eswitch_add_vf_sp_rule(pf, vf)) { > + ice_fltr_add_mac_and_broadcast(vsi, > + vf->hw_lan_addr, Fits into the previous line :p > + ICE_FWD_TO_VSI); > + goto err; > + } > + [...] > diff --git a/drivers/net/ethernet/intel/ice/ice_protocol_type.h b/drivers/net/ethernet/intel/ice/ice_protocol_type.h > index ed0ab8177c61..664e2f45e249 100644 > --- a/drivers/net/ethernet/intel/ice/ice_protocol_type.h > +++ b/drivers/net/ethernet/intel/ice/ice_protocol_type.h > @@ -256,7 +256,9 @@ struct ice_nvgre_hdr { > * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > * > * Source VSI = Source VSI of packet loopbacked in switch (for egress) (10b). > - * > + */ > +#define ICE_MDID_SOURCE_VSI_MASK 0x3ff GENMASK()? > +/* A newline before this line maybe to improve readability a bit? > * MDID 20 > * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > * |A|B|C|D|E|F|R|R|G|H|I|J|K|L|M|N| [...] > --- a/drivers/net/ethernet/intel/ice/ice_repr.h > +++ b/drivers/net/ethernet/intel/ice/ice_repr.h > @@ -13,9 +13,8 @@ struct ice_repr { > struct net_device *netdev; > struct metadata_dst *dst; > #ifdef CONFIG_ICE_SWITCHDEV > - /* info about slow path MAC rule */ > - struct ice_rule_query_data *mac_rule; > - u8 rule_added; > + /* info about slow path rule */ Two spaces after 'rule' here :s > + struct ice_rule_query_data sp_rule; > #endif > }; [...] Thanks, Olek
Dear Michal, Thank you for your patch. Am 04.04.23 um 09:28 schrieb Michal Swiatkowski: > The use of a source MAC to direct packets from the VF to the One space before MAC. > corresponding port representor is only ok if there is only one > MAC on a VF. To support this functionality when the number > of MACs on a VF is greater, it is necessary to match a source > VSI instead of a source MAC. Please reflow for 72/75 characters per line. This paragraph fits in four lines. > Let's use the new switch API that allows matching on metadata. > > If MAC isn't used in match criteria there is no need to handle adding > rule after virtchnl command. Instead add new rule while port representor > is being configured. > > Remove rule_added field, checking for sp_rule can be used instead. > Remove also checking for switchdev running in deleting rule as it can be > call from unroll context when running flag isn't set. Checking for call*ed* > sp_rule cover both context (with and without running flag). cover*s* > Rules are added in eswitch configuration flow, so there is no need to > have replay function. > > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > Reviewed-by: Piotr Raczynski <piotr.raczynski@intel.com> > Reviewed-by: Simon Horman <simon.horman@corigine.com> > --- > drivers/net/ethernet/intel/ice/ice_eswitch.c | 75 +++++++------------ > drivers/net/ethernet/intel/ice/ice_eswitch.h | 14 ---- > .../ethernet/intel/ice/ice_protocol_type.h | 4 +- > drivers/net/ethernet/intel/ice/ice_repr.c | 17 ----- > drivers/net/ethernet/intel/ice/ice_repr.h | 5 +- > drivers/net/ethernet/intel/ice/ice_switch.c | 6 ++ > drivers/net/ethernet/intel/ice/ice_switch.h | 1 + > drivers/net/ethernet/intel/ice/ice_vf_lib.c | 3 - > drivers/net/ethernet/intel/ice/ice_virtchnl.c | 8 -- > 9 files changed, 37 insertions(+), 96 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c > index 2c80d57331d0..69fc25a213ef 100644 > --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c > +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c > @@ -10,16 +10,15 @@ > #include "ice_tc_lib.h" > > /** > - * ice_eswitch_add_vf_mac_rule - add adv rule with VF's MAC > + * ice_eswitch_add_vf_sp_rule - add adv rule with VF's VSI index > * @pf: pointer to PF struct > * @vf: pointer to VF struct > - * @mac: VF's MAC address > * > * This function adds advanced rule that forwards packets with > - * VF's MAC address (src MAC) to the corresponding switchdev ctrl VSI queue. > + * VF's VSI index to the corresponding switchdev ctrl VSI queue. > */ > -int > -ice_eswitch_add_vf_mac_rule(struct ice_pf *pf, struct ice_vf *vf, const u8 *mac) > +static int > +ice_eswitch_add_vf_sp_rule(struct ice_pf *pf, struct ice_vf *vf) > { > struct ice_vsi *ctrl_vsi = pf->switchdev.control_vsi; > struct ice_adv_rule_info rule_info = { 0 }; > @@ -32,11 +31,9 @@ ice_eswitch_add_vf_mac_rule(struct ice_pf *pf, struct ice_vf *vf, const u8 *mac) > if (!list) > return -ENOMEM; > > - list[0].type = ICE_MAC_OFOS; > - ether_addr_copy(list[0].h_u.eth_hdr.src_addr, mac); > - eth_broadcast_addr(list[0].m_u.eth_hdr.src_addr); > + ice_rule_add_src_vsi_metadata(&list[0]); > > - rule_info.sw_act.flag |= ICE_FLTR_TX; > + rule_info.sw_act.flag = ICE_FLTR_TX; > rule_info.sw_act.vsi_handle = ctrl_vsi->idx; > rule_info.sw_act.fltr_act = ICE_FWD_TO_Q; > rule_info.sw_act.fwd_id.q_id = hw->func_caps.common_cap.rxq_first_id + > @@ -44,63 +41,31 @@ ice_eswitch_add_vf_mac_rule(struct ice_pf *pf, struct ice_vf *vf, const u8 *mac) > rule_info.flags_info.act |= ICE_SINGLE_ACT_LB_ENABLE; > rule_info.flags_info.act_valid = true; > rule_info.tun_type = ICE_SW_TUN_AND_NON_TUN; > + rule_info.src_vsi = vf->lan_vsi_idx; > > err = ice_add_adv_rule(hw, list, lkups_cnt, &rule_info, > - vf->repr->mac_rule); > + &vf->repr->sp_rule); > if (err) > - dev_err(ice_pf_to_dev(pf), "Unable to add VF mac rule in switchdev mode for VF %d", > + dev_err(ice_pf_to_dev(pf), "Unable to add VF slow-path rule in switchdev mode for VF %d", > vf->vf_id); > - else > - vf->repr->rule_added = true; > > kfree(list); > return err; > } > > /** > - * ice_eswitch_replay_vf_mac_rule - replay adv rule with VF's MAC > - * @vf: pointer to vF struct > - * > - * This function replays VF's MAC rule after reset. > - */ > -void ice_eswitch_replay_vf_mac_rule(struct ice_vf *vf) > -{ > - int err; > - > - if (!ice_is_switchdev_running(vf->pf)) > - return; > - > - if (is_valid_ether_addr(vf->hw_lan_addr)) { > - err = ice_eswitch_add_vf_mac_rule(vf->pf, vf, > - vf->hw_lan_addr); > - if (err) { > - dev_err(ice_pf_to_dev(vf->pf), "Failed to add MAC %pM for VF %d\n, error %d\n", > - vf->hw_lan_addr, vf->vf_id, err); > - return; > - } > - vf->num_mac++; > - > - ether_addr_copy(vf->dev_lan_addr, vf->hw_lan_addr); > - } > -} > - > -/** > - * ice_eswitch_del_vf_mac_rule - delete adv rule with VF's MAC > + * ice_eswitch_del_vf_sp_rule - delete adv rule with VF's VSI index > * @vf: pointer to the VF struct > * > - * Delete the advanced rule that was used to forward packets with the VF's MAC > - * address (src MAC) to the corresponding switchdev ctrl VSI queue. > + * Delete the advanced rule that was used to forward packets with the VF's VSI > + * index to the corresponding switchdev ctrl VSI queue. > */ > -void ice_eswitch_del_vf_mac_rule(struct ice_vf *vf) > +static void ice_eswitch_del_vf_sp_rule(struct ice_vf *vf) > { > - if (!ice_is_switchdev_running(vf->pf)) > + if (!vf->repr) > return; > > - if (!vf->repr->rule_added) > - return; > - > - ice_rem_adv_rule_by_id(&vf->pf->hw, vf->repr->mac_rule); > - vf->repr->rule_added = false; > + ice_rem_adv_rule_by_id(&vf->pf->hw, &vf->repr->sp_rule); > } > > /** > @@ -236,6 +201,7 @@ ice_eswitch_release_reprs(struct ice_pf *pf, struct ice_vsi *ctrl_vsi) > ice_vsi_update_security(vsi, ice_vsi_ctx_set_antispoof); > metadata_dst_free(vf->repr->dst); > vf->repr->dst = NULL; > + ice_eswitch_del_vf_sp_rule(vf); > ice_fltr_add_mac_and_broadcast(vsi, vf->hw_lan_addr, > ICE_FWD_TO_VSI); > > @@ -269,10 +235,18 @@ static int ice_eswitch_setup_reprs(struct ice_pf *pf) > goto err; > } > > + if (ice_eswitch_add_vf_sp_rule(pf, vf)) { > + ice_fltr_add_mac_and_broadcast(vsi, > + vf->hw_lan_addr, > + ICE_FWD_TO_VSI); > + goto err; > + } > + > if (ice_vsi_update_security(vsi, ice_vsi_ctx_clear_antispoof)) { > ice_fltr_add_mac_and_broadcast(vsi, > vf->hw_lan_addr, > ICE_FWD_TO_VSI); > + ice_eswitch_del_vf_sp_rule(vf); > metadata_dst_free(vf->repr->dst); > vf->repr->dst = NULL; > goto err; > @@ -282,6 +256,7 @@ static int ice_eswitch_setup_reprs(struct ice_pf *pf) > ice_fltr_add_mac_and_broadcast(vsi, > vf->hw_lan_addr, > ICE_FWD_TO_VSI); > + ice_eswitch_del_vf_sp_rule(vf); > metadata_dst_free(vf->repr->dst); > vf->repr->dst = NULL; > ice_vsi_update_security(vsi, ice_vsi_ctx_set_antispoof); > diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.h b/drivers/net/ethernet/intel/ice/ice_eswitch.h > index 6a413331572b..b18bf83a2f5b 100644 > --- a/drivers/net/ethernet/intel/ice/ice_eswitch.h > +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.h > @@ -20,11 +20,6 @@ bool ice_is_eswitch_mode_switchdev(struct ice_pf *pf); > void ice_eswitch_update_repr(struct ice_vsi *vsi); > > void ice_eswitch_stop_all_tx_queues(struct ice_pf *pf); > -int > -ice_eswitch_add_vf_mac_rule(struct ice_pf *pf, struct ice_vf *vf, > - const u8 *mac); > -void ice_eswitch_replay_vf_mac_rule(struct ice_vf *vf); > -void ice_eswitch_del_vf_mac_rule(struct ice_vf *vf); > > void ice_eswitch_set_target_vsi(struct sk_buff *skb, > struct ice_tx_offload_params *off); > @@ -34,15 +29,6 @@ ice_eswitch_port_start_xmit(struct sk_buff *skb, struct net_device *netdev); > static inline void ice_eswitch_release(struct ice_pf *pf) { } > > static inline void ice_eswitch_stop_all_tx_queues(struct ice_pf *pf) { } > -static inline void ice_eswitch_replay_vf_mac_rule(struct ice_vf *vf) { } > -static inline void ice_eswitch_del_vf_mac_rule(struct ice_vf *vf) { } > - > -static inline int > -ice_eswitch_add_vf_mac_rule(struct ice_pf *pf, struct ice_vf *vf, > - const u8 *mac) > -{ > - return -EOPNOTSUPP; > -} > > static inline void > ice_eswitch_set_target_vsi(struct sk_buff *skb, > diff --git a/drivers/net/ethernet/intel/ice/ice_protocol_type.h b/drivers/net/ethernet/intel/ice/ice_protocol_type.h > index ed0ab8177c61..664e2f45e249 100644 > --- a/drivers/net/ethernet/intel/ice/ice_protocol_type.h > +++ b/drivers/net/ethernet/intel/ice/ice_protocol_type.h > @@ -256,7 +256,9 @@ struct ice_nvgre_hdr { > * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > * > * Source VSI = Source VSI of packet loopbacked in switch (for egress) (10b). > - * > + */ > +#define ICE_MDID_SOURCE_VSI_MASK 0x3ff > +/* > * MDID 20 > * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > * |A|B|C|D|E|F|R|R|G|H|I|J|K|L|M|N| > diff --git a/drivers/net/ethernet/intel/ice/ice_repr.c b/drivers/net/ethernet/intel/ice/ice_repr.c > index fd1f8b0ad0ab..e30e12321abd 100644 > --- a/drivers/net/ethernet/intel/ice/ice_repr.c > +++ b/drivers/net/ethernet/intel/ice/ice_repr.c > @@ -298,14 +298,6 @@ static int ice_repr_add(struct ice_vf *vf) > if (!repr) > return -ENOMEM; > > -#ifdef CONFIG_ICE_SWITCHDEV > - repr->mac_rule = kzalloc(sizeof(*repr->mac_rule), GFP_KERNEL); > - if (!repr->mac_rule) { > - err = -ENOMEM; > - goto err_alloc_rule; > - } > -#endif > - > repr->netdev = alloc_etherdev(sizeof(struct ice_netdev_priv)); > if (!repr->netdev) { > err = -ENOMEM; > @@ -351,11 +343,6 @@ static int ice_repr_add(struct ice_vf *vf) > free_netdev(repr->netdev); > repr->netdev = NULL; > err_alloc: > -#ifdef CONFIG_ICE_SWITCHDEV > - kfree(repr->mac_rule); > - repr->mac_rule = NULL; > -err_alloc_rule: > -#endif > kfree(repr); > vf->repr = NULL; > return err; > @@ -376,10 +363,6 @@ static void ice_repr_rem(struct ice_vf *vf) > ice_devlink_destroy_vf_port(vf); > free_netdev(vf->repr->netdev); > vf->repr->netdev = NULL; > -#ifdef CONFIG_ICE_SWITCHDEV > - kfree(vf->repr->mac_rule); > - vf->repr->mac_rule = NULL; > -#endif > kfree(vf->repr); > vf->repr = NULL; > > diff --git a/drivers/net/ethernet/intel/ice/ice_repr.h b/drivers/net/ethernet/intel/ice/ice_repr.h > index 378a45bfa256..5a28bb42f72a 100644 > --- a/drivers/net/ethernet/intel/ice/ice_repr.h > +++ b/drivers/net/ethernet/intel/ice/ice_repr.h > @@ -13,9 +13,8 @@ struct ice_repr { > struct net_device *netdev; > struct metadata_dst *dst; > #ifdef CONFIG_ICE_SWITCHDEV > - /* info about slow path MAC rule */ > - struct ice_rule_query_data *mac_rule; > - u8 rule_added; > + /* info about slow path rule */ > + struct ice_rule_query_data sp_rule; I’d not abbreviate slowpath in the names. No idea if it would be too long. Kind regards, Paul > #endif > }; > > diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c > index 8c2bbfd2613f..76f5a817929a 100644 > --- a/drivers/net/ethernet/intel/ice/ice_switch.c > +++ b/drivers/net/ethernet/intel/ice/ice_switch.c > @@ -6007,6 +6007,12 @@ void ice_rule_add_vlan_metadata(struct ice_adv_lkup_elem *lkup) > cpu_to_be16(ICE_PKT_VLAN_MASK); > } > > +void ice_rule_add_src_vsi_metadata(struct ice_adv_lkup_elem *lkup) > +{ > + lkup->type = ICE_HW_METADATA; > + lkup->m_u.metadata.source_vsi = cpu_to_be16(ICE_MDID_SOURCE_VSI_MASK); > +} > + > /** > * ice_add_adv_rule - helper function to create an advanced switch rule > * @hw: pointer to the hardware structure > diff --git a/drivers/net/ethernet/intel/ice/ice_switch.h b/drivers/net/ethernet/intel/ice/ice_switch.h > index 245d4ad4e9bc..fbd0936750af 100644 > --- a/drivers/net/ethernet/intel/ice/ice_switch.h > +++ b/drivers/net/ethernet/intel/ice/ice_switch.h > @@ -344,6 +344,7 @@ ice_free_res_cntr(struct ice_hw *hw, u8 type, u8 alloc_shared, u16 num_items, > /* Switch/bridge related commands */ > void ice_rule_add_tunnel_metadata(struct ice_adv_lkup_elem *lkup); > void ice_rule_add_vlan_metadata(struct ice_adv_lkup_elem *lkup); > +void ice_rule_add_src_vsi_metadata(struct ice_adv_lkup_elem *lkup); > int > ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups, > u16 lkups_cnt, struct ice_adv_rule_info *rinfo, > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c > index 68142facc85d..294e91c3453c 100644 > --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c > @@ -670,8 +670,6 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags) > */ > ice_vf_clear_all_promisc_modes(vf, vsi); > > - ice_eswitch_del_vf_mac_rule(vf); > - > ice_vf_fdir_exit(vf); > ice_vf_fdir_init(vf); > /* clean VF control VSI when resetting VF since it should be setup > @@ -697,7 +695,6 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags) > } > > ice_eswitch_update_repr(vsi); > - ice_eswitch_replay_vf_mac_rule(vf); > > /* if the VF has been reset allow it to come up again */ > ice_mbx_clear_malvf(&vf->mbx_info); > diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > index 97243c616d5d..dcf628b1fccd 100644 > --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c > +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > @@ -3730,7 +3730,6 @@ static int ice_vc_repr_add_mac(struct ice_vf *vf, u8 *msg) > > for (i = 0; i < al->num_elements; i++) { > u8 *mac_addr = al->list[i].addr; > - int result; > > if (!is_unicast_ether_addr(mac_addr) || > ether_addr_equal(mac_addr, vf->hw_lan_addr)) > @@ -3742,13 +3741,6 @@ static int ice_vc_repr_add_mac(struct ice_vf *vf, u8 *msg) > goto handle_mac_exit; > } > > - result = ice_eswitch_add_vf_mac_rule(pf, vf, mac_addr); > - if (result) { > - dev_err(ice_pf_to_dev(pf), "Failed to add MAC %pM for VF %d\n, error %d\n", > - mac_addr, vf->vf_id, result); > - goto handle_mac_exit; > - } > - > ice_vfhw_mac_add(vf, &al->list[i]); > vf->num_mac++; > break;
On Tue, Apr 04, 2023 at 12:30:42PM +0200, Alexander Lobakin wrote: > From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > Date: Tue, 4 Apr 2023 09:28:33 +0200 > > > The use of a source MAC to direct packets from the VF to the > > corresponding port representor is only ok if there is only one > > MAC on a VF. To support this functionality when the number > > of MACs on a VF is greater, it is necessary to match a source > > VSI instead of a source MAC. > > [...] > > > @@ -32,11 +31,9 @@ > > if·(!list) > > return·-ENOMEM; > > > > - list[0].type·=·ICE_MAC_OFOS; > > - ether_addr_copy(list[0].h_u.eth_hdr.src_addr,·mac); > > - eth_broadcast_addr(list[0].m_u.eth_hdr.src_addr); > > + ice_rule_add_src_vsi_metadata(&list[0]); > > &list[0] == list. > Will do > > - rule_info.sw_act.flag·|=·ICE_FLTR_TX; > > + rule_info.sw_act.flag·=·ICE_FLTR_TX; > > rule_info.sw_act.vsi_handle·=·ctrl_vsi->idx; > > [...] > > > @@ -269,10 +235,18 @@ static int ice_eswitch_setup_reprs(struct ice_pf *pf) > > goto err; > > } > > > > + if (ice_eswitch_add_vf_sp_rule(pf, vf)) { > > + ice_fltr_add_mac_and_broadcast(vsi, > > + vf->hw_lan_addr, > > Fits into the previous line :p > Yeah, will move it. > > + ICE_FWD_TO_VSI); > > + goto err; > > + } > > + > > [...] > > > diff --git a/drivers/net/ethernet/intel/ice/ice_protocol_type.h b/drivers/net/ethernet/intel/ice/ice_protocol_type.h > > index ed0ab8177c61..664e2f45e249 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_protocol_type.h > > +++ b/drivers/net/ethernet/intel/ice/ice_protocol_type.h > > @@ -256,7 +256,9 @@ struct ice_nvgre_hdr { > > * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > > * > > * Source VSI = Source VSI of packet loopbacked in switch (for egress) (10b). > > - * > > + */ > > +#define ICE_MDID_SOURCE_VSI_MASK 0x3ff > > GENMASK()? > Sorry, it should be there (Simon pointed it), but I forgot about amending :( . Thanks for catching it. > > +/* > > A newline before this line maybe to improve readability a bit? > Will add > > * MDID 20 > > * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > > * |A|B|C|D|E|F|R|R|G|H|I|J|K|L|M|N| > > [...] > > > --- a/drivers/net/ethernet/intel/ice/ice_repr.h > > +++ b/drivers/net/ethernet/intel/ice/ice_repr.h > > @@ -13,9 +13,8 @@ struct ice_repr { > > struct net_device *netdev; > > struct metadata_dst *dst; > > #ifdef CONFIG_ICE_SWITCHDEV > > - /* info about slow path MAC rule */ > > - struct ice_rule_query_data *mac_rule; > > - u8 rule_added; > > + /* info about slow path rule */ > > Two spaces after 'rule' here :s > Will fix > > + struct ice_rule_query_data sp_rule; > > #endif > > }; > [...] > > Thanks, > Olek Thanks for the review Michal
On Tue, Apr 04, 2023 at 01:38:26PM +0200, Paul Menzel wrote: > Dear Michal, > > > Thank you for your patch. > > Am 04.04.23 um 09:28 schrieb Michal Swiatkowski: > > The use of a source MAC to direct packets from the VF to the > > One space before MAC. > > > corresponding port representor is only ok if there is only one > > MAC on a VF. To support this functionality when the number > > of MACs on a VF is greater, it is necessary to match a source > > VSI instead of a source MAC. > > Please reflow for 72/75 characters per line. This paragraph fits in four > lines. > > > Let's use the new switch API that allows matching on metadata. > > > > If MAC isn't used in match criteria there is no need to handle adding > > rule after virtchnl command. Instead add new rule while port representor > > is being configured. > > > > Remove rule_added field, checking for sp_rule can be used instead. > > Remove also checking for switchdev running in deleting rule as it can be > > call from unroll context when running flag isn't set. Checking for > > call*ed* > > > sp_rule cover both context (with and without running flag). > > cover*s* > Thanks, fixed > > Rules are added in eswitch configuration flow, so there is no need to > > have replay function. > > > > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > > Reviewed-by: Piotr Raczynski <piotr.raczynski@intel.com> > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > > @@ -13,9 +13,8 @@ struct ice_repr { [...] > > struct net_device *netdev; > > struct metadata_dst *dst; > > #ifdef CONFIG_ICE_SWITCHDEV > > - /* info about slow path MAC rule */ > > - struct ice_rule_query_data *mac_rule; > > - u8 rule_added; > > + /* info about slow path rule */ > > + struct ice_rule_query_data sp_rule; > > I’d not abbreviate slowpath in the names. No idea if it would be too long. > I think it will be too long. It also can be only rule, as there is only one specific rule stored in representor struct. > > Kind regards, > > Paul > Thanks, Michal > > > #endif > > }; > > diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c > > index 8c2bbfd2613f..76f5a817929a 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_switch.c > > +++ b/drivers/net/ethernet/intel/ice/ice_switch.c > > @@ -6007,6 +6007,12 @@ void ice_rule_add_vlan_metadata(struct ice_adv_lkup_elem *lkup) > > cpu_to_be16(ICE_PKT_VLAN_MASK); > > } > > +void ice_rule_add_src_vsi_metadata(struct ice_adv_lkup_elem *lkup) > > +{ > > + lkup->type = ICE_HW_METADATA; > > + lkup->m_u.metadata.source_vsi = cpu_to_be16(ICE_MDID_SOURCE_VSI_MASK); > > +} > > + > > /** > > * ice_add_adv_rule - helper function to create an advanced switch rule > > * @hw: pointer to the hardware structure > > diff --git a/drivers/net/ethernet/intel/ice/ice_switch.h b/drivers/net/ethernet/intel/ice/ice_switch.h > > index 245d4ad4e9bc..fbd0936750af 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_switch.h > > +++ b/drivers/net/ethernet/intel/ice/ice_switch.h > > @@ -344,6 +344,7 @@ ice_free_res_cntr(struct ice_hw *hw, u8 type, u8 alloc_shared, u16 num_items, > > /* Switch/bridge related commands */ > > void ice_rule_add_tunnel_metadata(struct ice_adv_lkup_elem *lkup); > > void ice_rule_add_vlan_metadata(struct ice_adv_lkup_elem *lkup); > > +void ice_rule_add_src_vsi_metadata(struct ice_adv_lkup_elem *lkup); > > int > > ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups, > > u16 lkups_cnt, struct ice_adv_rule_info *rinfo, > > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c > > index 68142facc85d..294e91c3453c 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c > > +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c > > @@ -670,8 +670,6 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags) > > */ > > ice_vf_clear_all_promisc_modes(vf, vsi); > > - ice_eswitch_del_vf_mac_rule(vf); > > - > > ice_vf_fdir_exit(vf); > > ice_vf_fdir_init(vf); > > /* clean VF control VSI when resetting VF since it should be setup > > @@ -697,7 +695,6 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags) > > } > > ice_eswitch_update_repr(vsi); > > - ice_eswitch_replay_vf_mac_rule(vf); > > /* if the VF has been reset allow it to come up again */ > > ice_mbx_clear_malvf(&vf->mbx_info); > > diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > > index 97243c616d5d..dcf628b1fccd 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c > > +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > > @@ -3730,7 +3730,6 @@ static int ice_vc_repr_add_mac(struct ice_vf *vf, u8 *msg) > > for (i = 0; i < al->num_elements; i++) { > > u8 *mac_addr = al->list[i].addr; > > - int result; > > if (!is_unicast_ether_addr(mac_addr) || > > ether_addr_equal(mac_addr, vf->hw_lan_addr)) > > @@ -3742,13 +3741,6 @@ static int ice_vc_repr_add_mac(struct ice_vf *vf, u8 *msg) > > goto handle_mac_exit; > > } > > - result = ice_eswitch_add_vf_mac_rule(pf, vf, mac_addr); > > - if (result) { > > - dev_err(ice_pf_to_dev(pf), "Failed to add MAC %pM for VF %d\n, error %d\n", > > - mac_addr, vf->vf_id, result); > > - goto handle_mac_exit; > > - } > > - > > ice_vfhw_mac_add(vf, &al->list[i]); > > vf->num_mac++; > > break;
diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c index 2c80d57331d0..69fc25a213ef 100644 --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c @@ -10,16 +10,15 @@ #include "ice_tc_lib.h" /** - * ice_eswitch_add_vf_mac_rule - add adv rule with VF's MAC + * ice_eswitch_add_vf_sp_rule - add adv rule with VF's VSI index * @pf: pointer to PF struct * @vf: pointer to VF struct - * @mac: VF's MAC address * * This function adds advanced rule that forwards packets with - * VF's MAC address (src MAC) to the corresponding switchdev ctrl VSI queue. + * VF's VSI index to the corresponding switchdev ctrl VSI queue. */ -int -ice_eswitch_add_vf_mac_rule(struct ice_pf *pf, struct ice_vf *vf, const u8 *mac) +static int +ice_eswitch_add_vf_sp_rule(struct ice_pf *pf, struct ice_vf *vf) { struct ice_vsi *ctrl_vsi = pf->switchdev.control_vsi; struct ice_adv_rule_info rule_info = { 0 }; @@ -32,11 +31,9 @@ ice_eswitch_add_vf_mac_rule(struct ice_pf *pf, struct ice_vf *vf, const u8 *mac) if (!list) return -ENOMEM; - list[0].type = ICE_MAC_OFOS; - ether_addr_copy(list[0].h_u.eth_hdr.src_addr, mac); - eth_broadcast_addr(list[0].m_u.eth_hdr.src_addr); + ice_rule_add_src_vsi_metadata(&list[0]); - rule_info.sw_act.flag |= ICE_FLTR_TX; + rule_info.sw_act.flag = ICE_FLTR_TX; rule_info.sw_act.vsi_handle = ctrl_vsi->idx; rule_info.sw_act.fltr_act = ICE_FWD_TO_Q; rule_info.sw_act.fwd_id.q_id = hw->func_caps.common_cap.rxq_first_id + @@ -44,63 +41,31 @@ ice_eswitch_add_vf_mac_rule(struct ice_pf *pf, struct ice_vf *vf, const u8 *mac) rule_info.flags_info.act |= ICE_SINGLE_ACT_LB_ENABLE; rule_info.flags_info.act_valid = true; rule_info.tun_type = ICE_SW_TUN_AND_NON_TUN; + rule_info.src_vsi = vf->lan_vsi_idx; err = ice_add_adv_rule(hw, list, lkups_cnt, &rule_info, - vf->repr->mac_rule); + &vf->repr->sp_rule); if (err) - dev_err(ice_pf_to_dev(pf), "Unable to add VF mac rule in switchdev mode for VF %d", + dev_err(ice_pf_to_dev(pf), "Unable to add VF slow-path rule in switchdev mode for VF %d", vf->vf_id); - else - vf->repr->rule_added = true; kfree(list); return err; } /** - * ice_eswitch_replay_vf_mac_rule - replay adv rule with VF's MAC - * @vf: pointer to vF struct - * - * This function replays VF's MAC rule after reset. - */ -void ice_eswitch_replay_vf_mac_rule(struct ice_vf *vf) -{ - int err; - - if (!ice_is_switchdev_running(vf->pf)) - return; - - if (is_valid_ether_addr(vf->hw_lan_addr)) { - err = ice_eswitch_add_vf_mac_rule(vf->pf, vf, - vf->hw_lan_addr); - if (err) { - dev_err(ice_pf_to_dev(vf->pf), "Failed to add MAC %pM for VF %d\n, error %d\n", - vf->hw_lan_addr, vf->vf_id, err); - return; - } - vf->num_mac++; - - ether_addr_copy(vf->dev_lan_addr, vf->hw_lan_addr); - } -} - -/** - * ice_eswitch_del_vf_mac_rule - delete adv rule with VF's MAC + * ice_eswitch_del_vf_sp_rule - delete adv rule with VF's VSI index * @vf: pointer to the VF struct * - * Delete the advanced rule that was used to forward packets with the VF's MAC - * address (src MAC) to the corresponding switchdev ctrl VSI queue. + * Delete the advanced rule that was used to forward packets with the VF's VSI + * index to the corresponding switchdev ctrl VSI queue. */ -void ice_eswitch_del_vf_mac_rule(struct ice_vf *vf) +static void ice_eswitch_del_vf_sp_rule(struct ice_vf *vf) { - if (!ice_is_switchdev_running(vf->pf)) + if (!vf->repr) return; - if (!vf->repr->rule_added) - return; - - ice_rem_adv_rule_by_id(&vf->pf->hw, vf->repr->mac_rule); - vf->repr->rule_added = false; + ice_rem_adv_rule_by_id(&vf->pf->hw, &vf->repr->sp_rule); } /** @@ -236,6 +201,7 @@ ice_eswitch_release_reprs(struct ice_pf *pf, struct ice_vsi *ctrl_vsi) ice_vsi_update_security(vsi, ice_vsi_ctx_set_antispoof); metadata_dst_free(vf->repr->dst); vf->repr->dst = NULL; + ice_eswitch_del_vf_sp_rule(vf); ice_fltr_add_mac_and_broadcast(vsi, vf->hw_lan_addr, ICE_FWD_TO_VSI); @@ -269,10 +235,18 @@ static int ice_eswitch_setup_reprs(struct ice_pf *pf) goto err; } + if (ice_eswitch_add_vf_sp_rule(pf, vf)) { + ice_fltr_add_mac_and_broadcast(vsi, + vf->hw_lan_addr, + ICE_FWD_TO_VSI); + goto err; + } + if (ice_vsi_update_security(vsi, ice_vsi_ctx_clear_antispoof)) { ice_fltr_add_mac_and_broadcast(vsi, vf->hw_lan_addr, ICE_FWD_TO_VSI); + ice_eswitch_del_vf_sp_rule(vf); metadata_dst_free(vf->repr->dst); vf->repr->dst = NULL; goto err; @@ -282,6 +256,7 @@ static int ice_eswitch_setup_reprs(struct ice_pf *pf) ice_fltr_add_mac_and_broadcast(vsi, vf->hw_lan_addr, ICE_FWD_TO_VSI); + ice_eswitch_del_vf_sp_rule(vf); metadata_dst_free(vf->repr->dst); vf->repr->dst = NULL; ice_vsi_update_security(vsi, ice_vsi_ctx_set_antispoof); diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.h b/drivers/net/ethernet/intel/ice/ice_eswitch.h index 6a413331572b..b18bf83a2f5b 100644 --- a/drivers/net/ethernet/intel/ice/ice_eswitch.h +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.h @@ -20,11 +20,6 @@ bool ice_is_eswitch_mode_switchdev(struct ice_pf *pf); void ice_eswitch_update_repr(struct ice_vsi *vsi); void ice_eswitch_stop_all_tx_queues(struct ice_pf *pf); -int -ice_eswitch_add_vf_mac_rule(struct ice_pf *pf, struct ice_vf *vf, - const u8 *mac); -void ice_eswitch_replay_vf_mac_rule(struct ice_vf *vf); -void ice_eswitch_del_vf_mac_rule(struct ice_vf *vf); void ice_eswitch_set_target_vsi(struct sk_buff *skb, struct ice_tx_offload_params *off); @@ -34,15 +29,6 @@ ice_eswitch_port_start_xmit(struct sk_buff *skb, struct net_device *netdev); static inline void ice_eswitch_release(struct ice_pf *pf) { } static inline void ice_eswitch_stop_all_tx_queues(struct ice_pf *pf) { } -static inline void ice_eswitch_replay_vf_mac_rule(struct ice_vf *vf) { } -static inline void ice_eswitch_del_vf_mac_rule(struct ice_vf *vf) { } - -static inline int -ice_eswitch_add_vf_mac_rule(struct ice_pf *pf, struct ice_vf *vf, - const u8 *mac) -{ - return -EOPNOTSUPP; -} static inline void ice_eswitch_set_target_vsi(struct sk_buff *skb, diff --git a/drivers/net/ethernet/intel/ice/ice_protocol_type.h b/drivers/net/ethernet/intel/ice/ice_protocol_type.h index ed0ab8177c61..664e2f45e249 100644 --- a/drivers/net/ethernet/intel/ice/ice_protocol_type.h +++ b/drivers/net/ethernet/intel/ice/ice_protocol_type.h @@ -256,7 +256,9 @@ struct ice_nvgre_hdr { * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ * * Source VSI = Source VSI of packet loopbacked in switch (for egress) (10b). - * + */ +#define ICE_MDID_SOURCE_VSI_MASK 0x3ff +/* * MDID 20 * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ * |A|B|C|D|E|F|R|R|G|H|I|J|K|L|M|N| diff --git a/drivers/net/ethernet/intel/ice/ice_repr.c b/drivers/net/ethernet/intel/ice/ice_repr.c index fd1f8b0ad0ab..e30e12321abd 100644 --- a/drivers/net/ethernet/intel/ice/ice_repr.c +++ b/drivers/net/ethernet/intel/ice/ice_repr.c @@ -298,14 +298,6 @@ static int ice_repr_add(struct ice_vf *vf) if (!repr) return -ENOMEM; -#ifdef CONFIG_ICE_SWITCHDEV - repr->mac_rule = kzalloc(sizeof(*repr->mac_rule), GFP_KERNEL); - if (!repr->mac_rule) { - err = -ENOMEM; - goto err_alloc_rule; - } -#endif - repr->netdev = alloc_etherdev(sizeof(struct ice_netdev_priv)); if (!repr->netdev) { err = -ENOMEM; @@ -351,11 +343,6 @@ static int ice_repr_add(struct ice_vf *vf) free_netdev(repr->netdev); repr->netdev = NULL; err_alloc: -#ifdef CONFIG_ICE_SWITCHDEV - kfree(repr->mac_rule); - repr->mac_rule = NULL; -err_alloc_rule: -#endif kfree(repr); vf->repr = NULL; return err; @@ -376,10 +363,6 @@ static void ice_repr_rem(struct ice_vf *vf) ice_devlink_destroy_vf_port(vf); free_netdev(vf->repr->netdev); vf->repr->netdev = NULL; -#ifdef CONFIG_ICE_SWITCHDEV - kfree(vf->repr->mac_rule); - vf->repr->mac_rule = NULL; -#endif kfree(vf->repr); vf->repr = NULL; diff --git a/drivers/net/ethernet/intel/ice/ice_repr.h b/drivers/net/ethernet/intel/ice/ice_repr.h index 378a45bfa256..5a28bb42f72a 100644 --- a/drivers/net/ethernet/intel/ice/ice_repr.h +++ b/drivers/net/ethernet/intel/ice/ice_repr.h @@ -13,9 +13,8 @@ struct ice_repr { struct net_device *netdev; struct metadata_dst *dst; #ifdef CONFIG_ICE_SWITCHDEV - /* info about slow path MAC rule */ - struct ice_rule_query_data *mac_rule; - u8 rule_added; + /* info about slow path rule */ + struct ice_rule_query_data sp_rule; #endif }; diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c index 8c2bbfd2613f..76f5a817929a 100644 --- a/drivers/net/ethernet/intel/ice/ice_switch.c +++ b/drivers/net/ethernet/intel/ice/ice_switch.c @@ -6007,6 +6007,12 @@ void ice_rule_add_vlan_metadata(struct ice_adv_lkup_elem *lkup) cpu_to_be16(ICE_PKT_VLAN_MASK); } +void ice_rule_add_src_vsi_metadata(struct ice_adv_lkup_elem *lkup) +{ + lkup->type = ICE_HW_METADATA; + lkup->m_u.metadata.source_vsi = cpu_to_be16(ICE_MDID_SOURCE_VSI_MASK); +} + /** * ice_add_adv_rule - helper function to create an advanced switch rule * @hw: pointer to the hardware structure diff --git a/drivers/net/ethernet/intel/ice/ice_switch.h b/drivers/net/ethernet/intel/ice/ice_switch.h index 245d4ad4e9bc..fbd0936750af 100644 --- a/drivers/net/ethernet/intel/ice/ice_switch.h +++ b/drivers/net/ethernet/intel/ice/ice_switch.h @@ -344,6 +344,7 @@ ice_free_res_cntr(struct ice_hw *hw, u8 type, u8 alloc_shared, u16 num_items, /* Switch/bridge related commands */ void ice_rule_add_tunnel_metadata(struct ice_adv_lkup_elem *lkup); void ice_rule_add_vlan_metadata(struct ice_adv_lkup_elem *lkup); +void ice_rule_add_src_vsi_metadata(struct ice_adv_lkup_elem *lkup); int ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups, u16 lkups_cnt, struct ice_adv_rule_info *rinfo, diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c index 68142facc85d..294e91c3453c 100644 --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c @@ -670,8 +670,6 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags) */ ice_vf_clear_all_promisc_modes(vf, vsi); - ice_eswitch_del_vf_mac_rule(vf); - ice_vf_fdir_exit(vf); ice_vf_fdir_init(vf); /* clean VF control VSI when resetting VF since it should be setup @@ -697,7 +695,6 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags) } ice_eswitch_update_repr(vsi); - ice_eswitch_replay_vf_mac_rule(vf); /* if the VF has been reset allow it to come up again */ ice_mbx_clear_malvf(&vf->mbx_info); diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c index 97243c616d5d..dcf628b1fccd 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c @@ -3730,7 +3730,6 @@ static int ice_vc_repr_add_mac(struct ice_vf *vf, u8 *msg) for (i = 0; i < al->num_elements; i++) { u8 *mac_addr = al->list[i].addr; - int result; if (!is_unicast_ether_addr(mac_addr) || ether_addr_equal(mac_addr, vf->hw_lan_addr)) @@ -3742,13 +3741,6 @@ static int ice_vc_repr_add_mac(struct ice_vf *vf, u8 *msg) goto handle_mac_exit; } - result = ice_eswitch_add_vf_mac_rule(pf, vf, mac_addr); - if (result) { - dev_err(ice_pf_to_dev(pf), "Failed to add MAC %pM for VF %d\n, error %d\n", - mac_addr, vf->vf_id, result); - goto handle_mac_exit; - } - ice_vfhw_mac_add(vf, &al->list[i]); vf->num_mac++; break;