| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4559a556-9b35-42ab-ae03-391495c0b9f4@intel.com>
Date: Tue, 4 Apr 2023 12:07:47 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
Simon Horman <simon.horman@...igine.com>
Subject: Re: [Intel-wired-lan] [PATCH net-next v2 2/4] ice: remove redundant
Rx field from rule info
From: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
Date: Tue, 4 Apr 2023 09:28:31 +0200
> Information about the direction is currently stored in sw_act.flag.
> There is no need to duplicate it in another field.
>
> Setting direction flag doesn't mean that there is a match criteria for
> direction in rule. It is only a information for HW from where switch id
> should be collected (VSI or port). In current implementation of advance
> rule handling, without matching for direction meta data, we can always
> set one the same flag and everything will work the same.
>
> Ability to match on direction matadata will be added in follow up
> patches.
>
> Recipe 0, 3 and 9 loaded from package has direction match
> criteria, but they are handled in other function.
>
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> Reviewed-by: Piotr Raczynski <piotr.raczynski@...el.com>
> Reviewed-by: Simon Horman <simon.horman@...igine.com>
> ---
> drivers/net/ethernet/intel/ice/ice_eswitch.c | 1 -
> drivers/net/ethernet/intel/ice/ice_switch.c | 22 ++++++++++----------
> drivers/net/ethernet/intel/ice/ice_switch.h | 2 --
> drivers/net/ethernet/intel/ice/ice_tc_lib.c | 5 -----
> 4 files changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> index f6dd3f8fd936..2c80d57331d0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> @@ -39,7 +39,6 @@ ice_eswitch_add_vf_mac_rule(struct ice_pf *pf, struct ice_vf *vf, const u8 *mac)
> 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.rx = false;
> rule_info.sw_act.fwd_id.q_id = hw->func_caps.common_cap.rxq_first_id +
> ctrl_vsi->rxq_map[vf->vf_id];
> rule_info.flags_info.act |= ICE_SINGLE_ACT_LB_ENABLE;
> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
> index 5c3f266fa80f..4d3a92e0c61f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_switch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
> @@ -6121,8 +6121,7 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
> if (rinfo->sw_act.fltr_act == ICE_FWD_TO_VSI)
> rinfo->sw_act.fwd_id.hw_vsi_id =
> ice_get_hw_vsi_num(hw, vsi_handle);
> - if (rinfo->sw_act.flag & ICE_FLTR_TX)
> - rinfo->sw_act.src = ice_get_hw_vsi_num(hw, vsi_handle);
> + rinfo->sw_act.src = ice_get_hw_vsi_num(hw, vsi_handle);
>
> status = ice_add_adv_recipe(hw, lkups, lkups_cnt, rinfo, &rid);
> if (status)
> @@ -6190,19 +6189,20 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
> goto err_ice_add_adv_rule;
> }
>
> - /* set the rule LOOKUP type based on caller specified 'Rx'
> - * instead of hardcoding it to be either LOOKUP_TX/RX
> + /* If there is no matching criteria for direction there
> + * is only one difference between Rx and Tx:
> + * - get switch id base on VSI number from source field (Tx)
> + * - get switch id base on port number (Rx)
> *
> - * for 'Rx' set the source to be the port number
> - * for 'Tx' set the source to be the source HW VSI number (determined
> - * by caller)
> + * If matching on direction metadata is chose rule direction is
> + * extracted from type value set here.
> */
> - if (rinfo->rx) {
> - s_rule->hdr.type = cpu_to_le16(ICE_AQC_SW_RULES_T_LKUP_RX);
> - s_rule->src = cpu_to_le16(hw->port_info->lport);
> - } else {
> + if (rinfo->sw_act.flag & ICE_FLTR_TX) {
> s_rule->hdr.type = cpu_to_le16(ICE_AQC_SW_RULES_T_LKUP_TX);
> s_rule->src = cpu_to_le16(rinfo->sw_act.src);
> + } else {
> + s_rule->hdr.type = cpu_to_le16(ICE_AQC_SW_RULES_T_LKUP_RX);
> + s_rule->src = cpu_to_le16(hw->port_info->lport);
> }
>
> s_rule->recipe_id = cpu_to_le16(rid);
> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.h b/drivers/net/ethernet/intel/ice/ice_switch.h
> index 68d8e8a6a189..44aa37b80111 100644
> --- a/drivers/net/ethernet/intel/ice/ice_switch.h
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.h
> @@ -10,7 +10,6 @@
> #define ICE_DFLT_VSI_INVAL 0xff
> #define ICE_FLTR_RX BIT(0)
> #define ICE_FLTR_TX BIT(1)
> -#define ICE_FLTR_TX_RX (ICE_FLTR_RX | ICE_FLTR_TX)
> #define ICE_VSI_INVAL_ID 0xffff
> #define ICE_INVAL_Q_HANDLE 0xFFFF
>
> @@ -190,7 +189,6 @@ struct ice_adv_rule_info {
> enum ice_sw_tunnel_type tun_type;
> struct ice_sw_act_ctrl sw_act;
> u32 priority;
> - u8 rx; /* true means LOOKUP_RX otherwise LOOKUP_TX */
> u16 fltr_rule_id;
> u16 vlan_type;
> struct ice_adv_rule_flags_info flags_info;
That u8 here was really off, was introducing at least 1 byte hole. Good
thing you dropped it.
Have you checked whether there are any holes left, maybe move fields
around a bit?
> diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> index 76f29a5bf8d7..b5af6cd5592b 100644
> --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> @@ -697,11 +697,9 @@ ice_eswitch_add_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
> if (fltr->direction == ICE_ESWITCH_FLTR_INGRESS) {
> rule_info.sw_act.flag |= ICE_FLTR_RX;
> rule_info.sw_act.src = hw->pf_id;
> - rule_info.rx = true;
> } else {
> rule_info.sw_act.flag |= ICE_FLTR_TX;
> rule_info.sw_act.src = vsi->idx;
> - rule_info.rx = false;
> rule_info.flags_info.act = ICE_SINGLE_ACT_LAN_ENABLE;
> rule_info.flags_info.act_valid = true;
> }
> @@ -909,7 +907,6 @@ ice_add_tc_flower_adv_fltr(struct ice_vsi *vsi,
> rule_info.sw_act.vsi_handle = dest_vsi->idx;
> rule_info.priority = ICE_SWITCH_FLTR_PRIO_VSI;
> rule_info.sw_act.src = hw->pf_id;
> - rule_info.rx = true;
> dev_dbg(dev, "add switch rule for TC:%u vsi_idx:%u, lkups_cnt:%u\n",
> tc_fltr->action.fwd.tc.tc_class,
> rule_info.sw_act.vsi_handle, lkups_cnt);
> @@ -920,7 +917,6 @@ ice_add_tc_flower_adv_fltr(struct ice_vsi *vsi,
> rule_info.sw_act.vsi_handle = dest_vsi->idx;
> rule_info.priority = ICE_SWITCH_FLTR_PRIO_QUEUE;
> rule_info.sw_act.src = hw->pf_id;
> - rule_info.rx = true;
> dev_dbg(dev, "add switch rule action to forward to queue:%u (HW queue %u), lkups_cnt:%u\n",
> tc_fltr->action.fwd.q.queue,
> tc_fltr->action.fwd.q.hw_queue, lkups_cnt);
> @@ -928,7 +924,6 @@ ice_add_tc_flower_adv_fltr(struct ice_vsi *vsi,
> case ICE_DROP_PACKET:
> rule_info.sw_act.flag |= ICE_FLTR_RX;
> rule_info.sw_act.src = hw->pf_id;
> - rule_info.rx = true;
> rule_info.priority = ICE_SWITCH_FLTR_PRIO_VSI;
> break;
> default:
Thanks,
Olek
Powered by blists - more mailing lists