[PATCH net-next v2 0/4] ice: allow matching on meta data

[PATCH net-next v2 0/4] ice: allow matching on meta data

[Michal Swiatkowski]

Hi,

This patchset is intended to improve the usability of the switchdev
slow path. Without matching on a meta data values slow path works
based on VF's MAC addresses. It causes a problem when the VF wants
to use more than one MAC address (e.g. when it is in trusted mode).

Parse all meta data in the same place where protocol type fields are
parsed. Add description for the currently implemented meta data. It is
important to note that depending on DDP not all described meta data can
be available. Using not available meta data leads to error returned by
function which is looking for correct words in profiles read from DDP.

There is also one small improvement, remove of rx field in rule info
structure (patch 2). It is redundant.

v1 --> v2:
 * fix spell issues
 * use GENMASK to define source VSI mask

Michal Swiatkowski (4):
  ice: define meta data to match in switch
  ice: remove redundant Rx field from rule info
  ice: allow matching on meta data
  ice: use src VSI instead of src MAC in slow-path

 drivers/net/ethernet/intel/ice/ice_eswitch.c  |  76 +++----
 drivers/net/ethernet/intel/ice/ice_eswitch.h  |  14 --
 .../ethernet/intel/ice/ice_protocol_type.h    | 196 +++++++++++++++++-
 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   | 182 +++++++---------
 drivers/net/ethernet/intel/ice/ice_switch.h   |   9 +-
 drivers/net/ethernet/intel/ice/ice_tc_lib.c   |  34 ++-
 drivers/net/ethernet/intel/ice/ice_tc_lib.h   |   1 +
 drivers/net/ethernet/intel/ice/ice_vf_lib.c   |   3 -
 drivers/net/ethernet/intel/ice/ice_virtchnl.c |   8 -
 .../net/ethernet/intel/ice/ice_vlan_mode.c    |   2 +-
 12 files changed, 319 insertions(+), 228 deletions(-)

-- 
2.39.2

[PATCH net-next v2 1/4] ice: define meta data to match in switch

[Michal Swiatkowski]

Add description for each meta data. Redefine tunnel mask to match only
tunneled MAC and tunneled VLAN. It shouldn't try to match other flags
(previously it was 0xff, it is redundant).

VLAN mask was 0xd000, change it to 0xf000. 4 last bits are flags
depending on the same field in packets (VLAN tag). Because of that,
It isn't harmful to match also on ITAG.

Group all MDID and MDID offsets into enums to keep things organized.

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>
---
 .../ethernet/intel/ice/ice_protocol_type.h    | 186 +++++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_switch.c   |  11 +-
 .../net/ethernet/intel/ice/ice_vlan_mode.c    |   2 +-
 3 files changed, 183 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_protocol_type.h b/drivers/net/ethernet/intel/ice/ice_protocol_type.h
index 02a4e1cf624e..8a84f106bd4d 100644
--- a/drivers/net/ethernet/intel/ice/ice_protocol_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_protocol_type.h
@@ -115,17 +115,7 @@ enum ice_prot_id {
 #define ICE_L2TPV3_HW		104
 
 #define ICE_UDP_OF_HW	52 /* UDP Tunnels */
-#define ICE_META_DATA_ID_HW 255 /* this is used for tunnel and VLAN type */
 
-#define ICE_MDID_SIZE 2
-
-#define ICE_TUN_FLAG_MDID 21
-#define ICE_TUN_FLAG_MDID_OFF (ICE_MDID_SIZE * ICE_TUN_FLAG_MDID)
-#define ICE_TUN_FLAG_MASK 0xFF
-
-#define ICE_VLAN_FLAG_MDID 20
-#define ICE_VLAN_FLAG_MDID_OFF (ICE_MDID_SIZE * ICE_VLAN_FLAG_MDID)
-#define ICE_PKT_FLAGS_0_TO_15_VLAN_FLAGS_MASK 0xD000
 
 #define ICE_TUN_FLAG_FV_IND 2
 
@@ -230,6 +220,181 @@ struct ice_nvgre_hdr {
 	__be32 tni_flow;
 };
 
+/* Metadata information
+ *
+ * Not all MDIDs can be used by switch block. It depends on package version.
+ *
+ * MDID 16 (Rx offset)
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |  A  |   B     |   Reserved    |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ * A = Source port where the transaction came from (3b).
+ *
+ * B = Destination TC of the packet. The TC is relative to a port (5b).
+ *
+ * MDID 17
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |      PTYPE        | Reserved  |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ * PTYPE = Encodes the packet type (10b).
+ *
+ * MDID 18
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * | Packet length             | R |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ * Packet length = Length of the packet in bytes
+ *		   (packet always carriers CRC) (14b).
+ * R = Reserved (2b).
+ *
+ * MDID 19
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |   Source VSI      | Reserved  |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ * Source VSI = Source VSI of packet loopbacked in switch (for egress) (10b).
+ *
+ * MDID 20
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |A|B|C|D|E|F|R|R|G|H|I|J|K|L|M|N|
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ * A = DSI - set for DSI RX pkts.
+ * B = ipsec_decrypted - invalid on NIC.
+ * C = marker - this is a marker packet.
+ * D = from_network - for TX sets to 0
+ *		      for RX:
+ *		        * 1 - packet is from external link
+ *		        * 0 - packet source is from internal
+ * E = source_interface_is_rx - reflect the physical interface from where the
+ *				packet was received:
+ *				* 1 - Rx
+ *				* 0 - Tx
+ * F = from_mng - The bit signals that the packet's origin is the management.
+ * G = ucast - Outer L2 MAC address is unicast.
+ * H = mcast - Outer L2 MAC address is multicast.
+ * I = bcast - Outer L2 MAC address is broadcast.
+ * J = second_outer_mac_present - 2 outer MAC headers are present in the packet.
+ * K = STAG or BVLAN - Outer L2 header has STAG (ethernet type 0x88a8) or
+ *		       BVLAN (ethernet type 0x88a8).
+ * L = ITAG - Outer L2 header has ITAG *ethernet type 0x88e7)
+ * M = EVLAN (0x8100) - Outer L2 header has EVLAN (ethernet type 0x8100)
+ * N = EVLAN (0x9100) - Outer L2 header has EVLAN (ethernet type 0x9100)
+ */
+#define ICE_PKT_VLAN_STAG	BIT(12)
+#define ICE_PKT_VLAN_ITAG	BIT(13)
+#define ICE_PKT_VLAN_EVLAN	(BIT(14) | BIT(15))
+#define ICE_PKT_VLAN_MASK	(ICE_PKT_VLAN_STAG | ICE_PKT_VLAN_ITAG | \
+				ICE_PKT_VLAN_EVLAN)
+/* MDID 21
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |A|B|C|D|E|F|G|H|I|J|R|R|K|L|M|N|
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ * A = VLAN (0x8100) - Outer L2 header has VLAN (ethernet type 0x8100)
+ * B = NSHoE - Outer L2 header has NSH (ethernet type 0x894f)
+ * C = MPLS (0x8847) - There is at least 1 MPLS tag in the outer header
+ *		       (ethernet type 0x8847)
+ * D = MPLS (0x8848) - There is at least 1 MPLS tag in the outer header
+ *		       (ethernet type 0x8848)
+ * E = multi MPLS - There is more than a single MPLS tag in the outer header
+ * F = inner MPLS - There is inner MPLS tag in the packet
+ * G = tunneled MAC - Set if the packet includes a tunneled MAC
+ * H = tunneled VLAN - Same as VLAN, but for a tunneled header
+ * I = pkt_is_frag - Packet is fragmented (ipv4 or ipv6)
+ * J = ipv6_ext - The packet has routing or destination ipv6 extension in inner
+ *		  or outer ipv6 headers
+ * K = RoCE - UDP packet detected as RoCEv2
+ * L = UDP_XSUM_0 - Set to 1 if L4 checksum is 0 in a UDP packet
+ * M = ESP - This is a ESP packet
+ * N = NAT_ESP - This is a ESP packet encapsulated in UDP NAT
+ */
+#define ICE_PKT_TUNNEL_MAC	BIT(6)
+#define ICE_PKT_TUNNEL_VLAN	BIT(7)
+#define ICE_PKT_TUNNEL_MASK	(ICE_PKT_TUNNEL_MAC | ICE_PKT_TUNNEL_VLAN)
+
+/* MDID 22
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |A|B|C|D|E|F|  G  |H|I|J| K |L|M|
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ * A = fin - fin flag in tcp header
+ * B = sync - sync flag in tcp header
+ * C = rst - rst flag in tcp header
+ * D = psh - psh flag in tcp header
+ * E = ack - ack flag in tcp header
+ * F = urg - urg flag in tcp header
+ * G = tunnel type (3b) - Flags used to decode tunnel type:
+ *			  * b000 - not a VXLAN/Geneve/GRE tunnel
+ *			  * b001 - VXLAN-GPE
+ *			  * b010 - VXLAN (non-GPE)
+ *			  * b011 - Geneve
+ *			  * b100 - GRE (no key, no xsum)
+ *			  * b101 - GREK (key, no xsum)
+ *			  * b110 - GREC (no key, xsum)
+ *			  * b111 - GREKC (key, xsum)
+ * H = UDP_GRE - Packet is UDP (VXLAN or VLAN_GPE or Geneve or MPLSoUDP or GRE)
+ *		 tunnel
+ * I = OAM - VXLAN/Geneve/tunneled NSH packet with the OAM bit set
+ * J = tunneled NSH - Packet has NSHoGRE or NSHoUDP
+ * K = switch (2b) - Direction on switch
+ *		     * b00 - normal
+ *		     * b01 - TX force only LAN
+ *		     * b10 - TX disable LAN
+ *		     * b11 - direct to VSI
+ * L = swpe - Represents SWPE bit in TX command
+ * M = sw_cmd - Switch command
+ *
+ * MDID 23
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |A|B|C|D|        R        |E|F|R|
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ * A = MAC error - Produced by MAC according to L2 error conditions
+ * B = PPRS no offload - FIFO overflow in PPRS or any problematic condition in
+ *			 PPRS ANA
+ * C = abort - Set when malicious packet is detected
+ * D = partial analysis - ANA's analysing got cut in the middle
+ *			 (header > 504B etc.)
+ * E = FLM - Flow director hit indication
+ * F = FDLONG - Flow direector long bucket indication
+ *
+ */
+#define ICE_MDID_SIZE 2
+#define ICE_META_DATA_ID_HW 255
+
+enum ice_hw_metadata_id {
+	ICE_SOURCE_PORT_MDID = 16,
+	ICE_PTYPE_MDID = 17,
+	ICE_PACKET_LENGTH_MDID = 18,
+	ICE_SOURCE_VSI_MDID = 19,
+	ICE_PKT_VLAN_MDID = 20,
+	ICE_PKT_TUNNEL_MDID = 21,
+	ICE_PKT_TCP_MDID = 22,
+	ICE_PKT_ERROR_MDID = 23,
+};
+
+enum ice_hw_metadata_offset {
+	ICE_SOURCE_PORT_MDID_OFFSET = ICE_MDID_SIZE * ICE_SOURCE_PORT_MDID,
+	ICE_PTYPE_MDID_OFFSET = ICE_MDID_SIZE * ICE_PTYPE_MDID,
+	ICE_PACKET_LENGTH_MDID_OFFSET = ICE_MDID_SIZE * ICE_PACKET_LENGTH_MDID,
+	ICE_SOURCE_VSI_MDID_OFFSET = ICE_MDID_SIZE * ICE_SOURCE_VSI_MDID,
+	ICE_PKT_VLAN_MDID_OFFSET = ICE_MDID_SIZE * ICE_PKT_VLAN_MDID,
+	ICE_PKT_TUNNEL_MDID_OFFSET = ICE_MDID_SIZE * ICE_PKT_TUNNEL_MDID,
+	ICE_PKT_TCP_MDID_OFFSET = ICE_MDID_SIZE * ICE_PKT_TCP_MDID,
+	ICE_PKT_ERROR_MDID_OFFSET = ICE_MDID_SIZE * ICE_PKT_ERROR_MDID,
+};
+
+struct ice_hw_metadata {
+	__be16 source_port;
+	__be16 ptype;
+	__be16 packet_length;
+	__be16 source_vsi;
+	__be16 flags[4];
+};
+
 union ice_prot_hdr {
 	struct ice_ether_hdr eth_hdr;
 	struct ice_ethtype_hdr ethertype;
@@ -243,6 +408,7 @@ union ice_prot_hdr {
 	struct ice_udp_gtp_hdr gtp_hdr;
 	struct ice_pppoe_hdr pppoe_hdr;
 	struct ice_l2tpv3_sess_hdr l2tpv3_sess_hdr;
+	struct ice_hw_metadata metadata;
 };
 
 /* This is mapping table entry that maps every word within a given protocol
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index 46b36851af46..5c3f266fa80f 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -5268,7 +5268,7 @@ static bool ice_tun_type_match_word(enum ice_sw_tunnel_type tun_type, u16 *mask)
 	case ICE_SW_TUN_NVGRE:
 	case ICE_SW_TUN_GTPU:
 	case ICE_SW_TUN_GTPC:
-		*mask = ICE_TUN_FLAG_MASK;
+		*mask = ICE_PKT_TUNNEL_MASK;
 		return true;
 
 	default:
@@ -5297,7 +5297,8 @@ ice_add_special_words(struct ice_adv_rule_info *rinfo,
 			u8 word = lkup_exts->n_val_words++;
 
 			lkup_exts->fv_words[word].prot_id = ICE_META_DATA_ID_HW;
-			lkup_exts->fv_words[word].off = ICE_TUN_FLAG_MDID_OFF;
+			lkup_exts->fv_words[word].off =
+				ICE_PKT_TUNNEL_MDID_OFFSET;
 			lkup_exts->field_mask[word] = mask;
 		} else {
 			return -ENOSPC;
@@ -5309,9 +5310,9 @@ ice_add_special_words(struct ice_adv_rule_info *rinfo,
 			u8 word = lkup_exts->n_val_words++;
 
 			lkup_exts->fv_words[word].prot_id = ICE_META_DATA_ID_HW;
-			lkup_exts->fv_words[word].off = ICE_VLAN_FLAG_MDID_OFF;
-			lkup_exts->field_mask[word] =
-					ICE_PKT_FLAGS_0_TO_15_VLAN_FLAGS_MASK;
+			lkup_exts->fv_words[word].off =
+				ICE_PKT_VLAN_MDID_OFFSET;
+			lkup_exts->field_mask[word] = ICE_PKT_VLAN_MASK;
 		} else {
 			return -ENOSPC;
 		}
diff --git a/drivers/net/ethernet/intel/ice/ice_vlan_mode.c b/drivers/net/ethernet/intel/ice/ice_vlan_mode.c
index bcda2e004807..1279c1ffe31c 100644
--- a/drivers/net/ethernet/intel/ice/ice_vlan_mode.c
+++ b/drivers/net/ethernet/intel/ice/ice_vlan_mode.c
@@ -219,7 +219,7 @@ static struct ice_update_recipe_lkup_idx_params ice_dvm_dflt_recipes[] = {
 		.rid = ICE_SW_LKUP_VLAN,
 		.fv_idx = ICE_PKT_FLAGS_0_TO_15_FV_IDX,
 		.ignore_valid = false,
-		.mask = ICE_PKT_FLAGS_0_TO_15_VLAN_FLAGS_MASK,
+		.mask = ICE_PKT_VLAN_MASK,
 		.mask_valid = true,
 		.lkup_idx = ICE_SW_LKUP_VLAN_PKT_FLAGS_LKUP_IDX,
 	},
-- 
2.39.2

[PATCH net-next v2 2/4] ice: remove redundant Rx field from rule info

[Michal Swiatkowski]

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@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 |  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;
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:
-- 
2.39.2

Re: [Intel-wired-lan] [PATCH net-next v2 2/4] ice: remove redundant Rx field from rule info

[Alexander Lobakin]

From: Michal Swiatkowski <michal.swiatkowski@linux.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@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 |  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

Re: [Intel-wired-lan] [PATCH net-next v2 2/4] ice: remove redundant Rx field from rule info

[Michal Swiatkowski]

On Tue, Apr 04, 2023 at 12:07:47PM +0200, Alexander Lobakin wrote:
> From: Michal Swiatkowski <michal.swiatkowski@linux.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@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 |  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?
> 

As You pointed in patch 3 there are, I will move it to avoid holes.

> > 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

[PATCH net-next v2 3/4] ice: allow matching on meta data

[Michal Swiatkowski]

Add meta data matching criteria in the same place as protocol matching
criteria. There is no need to add meta data as special words after
parsing all lookups. Trade meta data in the same why as other lookups.

The one difference between meta data lookups and protocol lookups is
that meta data doesn't impact how the packets looks like. Because of that
ignore it when filling testing packet.

Match on tunnel type mata data always if tunnel type is different than
TNL_LAST.

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>
---
 .../ethernet/intel/ice/ice_protocol_type.h    |   8 +
 drivers/net/ethernet/intel/ice/ice_switch.c   | 159 +++++++-----------
 drivers/net/ethernet/intel/ice/ice_switch.h   |   6 +-
 drivers/net/ethernet/intel/ice/ice_tc_lib.c   |  29 +++-
 drivers/net/ethernet/intel/ice/ice_tc_lib.h   |   1 +
 5 files changed, 97 insertions(+), 106 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_protocol_type.h b/drivers/net/ethernet/intel/ice/ice_protocol_type.h
index 8a84f106bd4d..ed0ab8177c61 100644
--- a/drivers/net/ethernet/intel/ice/ice_protocol_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_protocol_type.h
@@ -47,6 +47,7 @@ enum ice_protocol_type {
 	ICE_L2TPV3,
 	ICE_VLAN_EX,
 	ICE_VLAN_IN,
+	ICE_HW_METADATA,
 	ICE_VXLAN_GPE,
 	ICE_SCTP_IL,
 	ICE_PROTOCOL_LAST
@@ -387,6 +388,13 @@ enum ice_hw_metadata_offset {
 	ICE_PKT_ERROR_MDID_OFFSET = ICE_MDID_SIZE * ICE_PKT_ERROR_MDID,
 };
 
+enum ice_pkt_flags {
+	ICE_PKT_FLAGS_VLAN = 0,
+	ICE_PKT_FLAGS_TUNNEL = 1,
+	ICE_PKT_FLAGS_TCP = 2,
+	ICE_PKT_FLAGS_ERROR = 3,
+};
+
 struct ice_hw_metadata {
 	__be16 source_port;
 	__be16 ptype;
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index 4d3a92e0c61f..8c2bbfd2613f 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -4573,6 +4573,15 @@ static const struct ice_prot_ext_tbl_entry ice_prot_ext[ICE_PROTOCOL_LAST] = {
 	{ ICE_L2TPV3,		{ 0, 2, 4, 6, 8, 10 } },
 	{ ICE_VLAN_EX,          { 2, 0 } },
 	{ ICE_VLAN_IN,          { 2, 0 } },
+	{ ICE_HW_METADATA,	{ ICE_SOURCE_PORT_MDID_OFFSET,
+				  ICE_PTYPE_MDID_OFFSET,
+				  ICE_PACKET_LENGTH_MDID_OFFSET,
+				  ICE_SOURCE_VSI_MDID_OFFSET,
+				  ICE_PKT_VLAN_MDID_OFFSET,
+				  ICE_PKT_TUNNEL_MDID_OFFSET,
+				  ICE_PKT_TCP_MDID_OFFSET,
+				  ICE_PKT_ERROR_MDID_OFFSET,
+				}},
 };
 
 static struct ice_protocol_entry ice_prot_id_tbl[ICE_PROTOCOL_LAST] = {
@@ -4597,6 +4606,7 @@ static struct ice_protocol_entry ice_prot_id_tbl[ICE_PROTOCOL_LAST] = {
 	{ ICE_L2TPV3,		ICE_L2TPV3_HW },
 	{ ICE_VLAN_EX,          ICE_VLAN_OF_HW },
 	{ ICE_VLAN_IN,          ICE_VLAN_OL_HW },
+	{ ICE_HW_METADATA,      ICE_META_DATA_ID_HW},
 };
 
 /**
@@ -5255,72 +5265,6 @@ ice_create_recipe_group(struct ice_hw *hw, struct ice_sw_recipe *rm,
 	return status;
 }
 
-/**
- * ice_tun_type_match_word - determine if tun type needs a match mask
- * @tun_type: tunnel type
- * @mask: mask to be used for the tunnel
- */
-static bool ice_tun_type_match_word(enum ice_sw_tunnel_type tun_type, u16 *mask)
-{
-	switch (tun_type) {
-	case ICE_SW_TUN_GENEVE:
-	case ICE_SW_TUN_VXLAN:
-	case ICE_SW_TUN_NVGRE:
-	case ICE_SW_TUN_GTPU:
-	case ICE_SW_TUN_GTPC:
-		*mask = ICE_PKT_TUNNEL_MASK;
-		return true;
-
-	default:
-		*mask = 0;
-		return false;
-	}
-}
-
-/**
- * ice_add_special_words - Add words that are not protocols, such as metadata
- * @rinfo: other information regarding the rule e.g. priority and action info
- * @lkup_exts: lookup word structure
- * @dvm_ena: is double VLAN mode enabled
- */
-static int
-ice_add_special_words(struct ice_adv_rule_info *rinfo,
-		      struct ice_prot_lkup_ext *lkup_exts, bool dvm_ena)
-{
-	u16 mask;
-
-	/* If this is a tunneled packet, then add recipe index to match the
-	 * tunnel bit in the packet metadata flags.
-	 */
-	if (ice_tun_type_match_word(rinfo->tun_type, &mask)) {
-		if (lkup_exts->n_val_words < ICE_MAX_CHAIN_WORDS) {
-			u8 word = lkup_exts->n_val_words++;
-
-			lkup_exts->fv_words[word].prot_id = ICE_META_DATA_ID_HW;
-			lkup_exts->fv_words[word].off =
-				ICE_PKT_TUNNEL_MDID_OFFSET;
-			lkup_exts->field_mask[word] = mask;
-		} else {
-			return -ENOSPC;
-		}
-	}
-
-	if (rinfo->vlan_type != 0 && dvm_ena) {
-		if (lkup_exts->n_val_words < ICE_MAX_CHAIN_WORDS) {
-			u8 word = lkup_exts->n_val_words++;
-
-			lkup_exts->fv_words[word].prot_id = ICE_META_DATA_ID_HW;
-			lkup_exts->fv_words[word].off =
-				ICE_PKT_VLAN_MDID_OFFSET;
-			lkup_exts->field_mask[word] = ICE_PKT_VLAN_MASK;
-		} else {
-			return -ENOSPC;
-		}
-	}
-
-	return 0;
-}
-
 /* ice_get_compat_fv_bitmap - Get compatible field vector bitmap for rule
  * @hw: pointer to hardware structure
  * @rinfo: other information regarding the rule e.g. priority and action info
@@ -5434,13 +5378,6 @@ ice_add_adv_recipe(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
 	if (status)
 		goto err_unroll;
 
-	/* Create any special protocol/offset pairs, such as looking at tunnel
-	 * bits by extracting metadata
-	 */
-	status = ice_add_special_words(rinfo, lkup_exts, ice_is_dvm_ena(hw));
-	if (status)
-		goto err_unroll;
-
 	/* Group match words into recipes using preferred recipe grouping
 	 * criteria.
 	 */
@@ -5726,6 +5663,10 @@ ice_fill_adv_dummy_packet(struct ice_adv_lkup_elem *lkups, u16 lkups_cnt,
 		 * was already checked when search for the dummy packet
 		 */
 		type = lkups[i].type;
+		/* metadata isn't lockated in packet */
+		if (type == ICE_HW_METADATA)
+			continue;
+
 		for (j = 0; offsets[j].type != ICE_PROTOCOL_LAST; j++) {
 			if (type == offsets[j].type) {
 				offset = offsets[j].offset;
@@ -5861,16 +5802,21 @@ ice_fill_adv_packet_tun(struct ice_hw *hw, enum ice_sw_tunnel_type tun_type,
 
 /**
  * ice_fill_adv_packet_vlan - fill dummy packet with VLAN tag type
+ * @hw: pointer to hw structure
  * @vlan_type: VLAN tag type
  * @pkt: dummy packet to fill in
  * @offsets: offset info for the dummy packet
  */
 static int
-ice_fill_adv_packet_vlan(u16 vlan_type, u8 *pkt,
+ice_fill_adv_packet_vlan(struct ice_hw *hw, u16 vlan_type, u8 *pkt,
 			 const struct ice_dummy_pkt_offsets *offsets)
 {
 	u16 i;
 
+	/* Check if there is something to do */
+	if (vlan_type == 0 || !ice_is_dvm_ena(hw))
+		return 0;
+
 	/* Find VLAN header and insert VLAN TPID */
 	for (i = 0; offsets[i].type != ICE_PROTOCOL_LAST; i++) {
 		if (offsets[i].type == ICE_VLAN_OFOS ||
@@ -5889,6 +5835,15 @@ ice_fill_adv_packet_vlan(u16 vlan_type, u8 *pkt,
 	return -EIO;
 }
 
+static bool ice_is_rule_info_the_same(struct ice_adv_rule_info *first,
+				      struct ice_adv_rule_info *second)
+{
+	return first->sw_act.flag == second->sw_act.flag &&
+	       first->tun_type == second->tun_type &&
+	       first->vlan_type == second->vlan_type &&
+	       first->src_vsi == second->src_vsi;
+}
+
 /**
  * ice_find_adv_rule_entry - Search a rule entry
  * @hw: pointer to the hardware structure
@@ -5922,9 +5877,7 @@ ice_find_adv_rule_entry(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
 				lkups_matched = false;
 				break;
 			}
-		if (rinfo->sw_act.flag == list_itr->rule_info.sw_act.flag &&
-		    rinfo->tun_type == list_itr->rule_info.tun_type &&
-		    rinfo->vlan_type == list_itr->rule_info.vlan_type &&
+		if (ice_is_rule_info_the_same(rinfo, &list_itr->rule_info) &&
 		    lkups_matched)
 			return list_itr;
 	}
@@ -6040,6 +5993,20 @@ ice_adv_add_update_vsi_list(struct ice_hw *hw,
 	return status;
 }
 
+void ice_rule_add_tunnel_metadata(struct ice_adv_lkup_elem *lkup)
+{
+	lkup->type = ICE_HW_METADATA;
+	lkup->m_u.metadata.flags[ICE_PKT_FLAGS_TUNNEL] =
+		cpu_to_be16(ICE_PKT_TUNNEL_MASK);
+}
+
+void ice_rule_add_vlan_metadata(struct ice_adv_lkup_elem *lkup)
+{
+	lkup->type = ICE_HW_METADATA;
+	lkup->m_u.metadata.flags[ICE_PKT_FLAGS_VLAN] =
+		cpu_to_be16(ICE_PKT_VLAN_MASK);
+}
+
 /**
  * ice_add_adv_rule - helper function to create an advanced switch rule
  * @hw: pointer to the hardware structure
@@ -6121,7 +6088,12 @@ 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);
-	rinfo->sw_act.src = ice_get_hw_vsi_num(hw, vsi_handle);
+
+	if (rinfo->src_vsi)
+		rinfo->sw_act.src =
+			ice_get_hw_vsi_num(hw, rinfo->src_vsi);
+	else
+		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)
@@ -6212,22 +6184,16 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
 	if (status)
 		goto err_ice_add_adv_rule;
 
-	if (rinfo->tun_type != ICE_NON_TUN &&
-	    rinfo->tun_type != ICE_SW_TUN_AND_NON_TUN) {
-		status = ice_fill_adv_packet_tun(hw, rinfo->tun_type,
-						 s_rule->hdr_data,
-						 profile->offsets);
-		if (status)
-			goto err_ice_add_adv_rule;
-	}
+	status = ice_fill_adv_packet_tun(hw, rinfo->tun_type, s_rule->hdr_data,
+					 profile->offsets);
+	if (status)
+		goto err_ice_add_adv_rule;
 
-	if (rinfo->vlan_type != 0 && ice_is_dvm_ena(hw)) {
-		status = ice_fill_adv_packet_vlan(rinfo->vlan_type,
-						  s_rule->hdr_data,
-						  profile->offsets);
-		if (status)
-			goto err_ice_add_adv_rule;
-	}
+	status = ice_fill_adv_packet_vlan(hw, rinfo->vlan_type,
+					  s_rule->hdr_data,
+					  profile->offsets);
+	if (status)
+		goto err_ice_add_adv_rule;
 
 	status = ice_aq_sw_rules(hw, (struct ice_aqc_sw_rules *)s_rule,
 				 rule_buf_sz, 1, ice_aqc_opc_add_sw_rules,
@@ -6470,13 +6436,6 @@ ice_rem_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
 			return -EIO;
 	}
 
-	/* Create any special protocol/offset pairs, such as looking at tunnel
-	 * bits by extracting metadata
-	 */
-	status = ice_add_special_words(rinfo, &lkup_exts, ice_is_dvm_ena(hw));
-	if (status)
-		return status;
-
 	rid = ice_find_recp(hw, &lkup_exts, rinfo->tun_type);
 	/* If did not find a recipe that match the existing criteria */
 	if (rid == ICE_MAX_NUM_RECIPES)
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.h b/drivers/net/ethernet/intel/ice/ice_switch.h
index 44aa37b80111..245d4ad4e9bc 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.h
+++ b/drivers/net/ethernet/intel/ice/ice_switch.h
@@ -186,11 +186,13 @@ struct ice_adv_rule_flags_info {
 };
 
 struct ice_adv_rule_info {
+	/* Store metadata values in rule info */
 	enum ice_sw_tunnel_type tun_type;
+	u16 vlan_type;
+	u16 src_vsi;
 	struct ice_sw_act_ctrl sw_act;
 	u32 priority;
 	u16 fltr_rule_id;
-	u16 vlan_type;
 	struct ice_adv_rule_flags_info flags_info;
 };
 
@@ -340,6 +342,8 @@ ice_free_res_cntr(struct ice_hw *hw, u8 type, u8 alloc_shared, u16 num_items,
 		  u16 counter_id);
 
 /* 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);
 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_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
index b5af6cd5592b..85241da1a41e 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
@@ -54,6 +54,10 @@ ice_tc_count_lkups(u32 flags, struct ice_tc_flower_lyr_2_4_hdrs *headers,
 	if (flags & (ICE_TC_FLWR_FIELD_VLAN | ICE_TC_FLWR_FIELD_VLAN_PRIO))
 		lkups_cnt++;
 
+	/* is VLAN TPID specified */
+	if (flags & ICE_TC_FLWR_FIELD_VLAN_TPID)
+		lkups_cnt++;
+
 	/* is CVLAN specified? */
 	if (flags & (ICE_TC_FLWR_FIELD_CVLAN | ICE_TC_FLWR_FIELD_CVLAN_PRIO))
 		lkups_cnt++;
@@ -80,6 +84,10 @@ ice_tc_count_lkups(u32 flags, struct ice_tc_flower_lyr_2_4_hdrs *headers,
 		     ICE_TC_FLWR_FIELD_SRC_L4_PORT))
 		lkups_cnt++;
 
+	/* matching for tunneled packets in metadata */
+	if (fltr->tunnel_type != TNL_LAST)
+		lkups_cnt++;
+
 	return lkups_cnt;
 }
 
@@ -320,6 +328,10 @@ ice_tc_fill_tunnel_outer(u32 flags, struct ice_tc_flower_fltr *fltr,
 		i++;
 	}
 
+	/* always fill matching on tunneled packets in metadata */
+	ice_rule_add_tunnel_metadata(&list[i]);
+	i++;
+
 	return i;
 }
 
@@ -390,10 +402,6 @@ ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
 
 	/* copy VLAN info */
 	if (flags & (ICE_TC_FLWR_FIELD_VLAN | ICE_TC_FLWR_FIELD_VLAN_PRIO)) {
-		vlan_tpid = be16_to_cpu(headers->vlan_hdr.vlan_tpid);
-		rule_info->vlan_type =
-				ice_check_supported_vlan_tpid(vlan_tpid);
-
 		if (flags & ICE_TC_FLWR_FIELD_CVLAN)
 			list[i].type = ICE_VLAN_EX;
 		else
@@ -418,6 +426,15 @@ ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
 		i++;
 	}
 
+	if (flags & ICE_TC_FLWR_FIELD_VLAN_TPID) {
+		vlan_tpid = be16_to_cpu(headers->vlan_hdr.vlan_tpid);
+		rule_info->vlan_type =
+				ice_check_supported_vlan_tpid(vlan_tpid);
+
+		ice_rule_add_vlan_metadata(&list[i]);
+		i++;
+	}
+
 	if (flags & (ICE_TC_FLWR_FIELD_CVLAN | ICE_TC_FLWR_FIELD_CVLAN_PRIO)) {
 		list[i].type = ICE_VLAN_IN;
 
@@ -1454,8 +1471,10 @@ ice_parse_cls_flower(struct net_device *filter_dev, struct ice_vsi *vsi,
 						 VLAN_PRIO_MASK);
 		}
 
-		if (match.mask->vlan_tpid)
+		if (match.mask->vlan_tpid) {
 			headers->vlan_hdr.vlan_tpid = match.key->vlan_tpid;
+			fltr->flags |= ICE_TC_FLWR_FIELD_VLAN_TPID;
+		}
 	}
 
 	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_CVLAN)) {
diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.h b/drivers/net/ethernet/intel/ice/ice_tc_lib.h
index 8d5e22ac7023..8bbc1a62bdb1 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.h
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.h
@@ -33,6 +33,7 @@
 #define ICE_TC_FLWR_FIELD_L2TPV3_SESSID		BIT(26)
 #define ICE_TC_FLWR_FIELD_VLAN_PRIO		BIT(27)
 #define ICE_TC_FLWR_FIELD_CVLAN_PRIO		BIT(28)
+#define ICE_TC_FLWR_FIELD_VLAN_TPID		BIT(29)
 
 #define ICE_TC_FLOWER_MASK_32   0xFFFFFFFF
 
-- 
2.39.2

Re: [Intel-wired-lan] [PATCH net-next v2 3/4] ice: allow matching on meta data

[Alexander Lobakin]

From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Date: Tue,  4 Apr 2023 09:28:32 +0200

> Add meta data matching criteria in the same place as protocol matching
> criteria. There is no need to add meta data as special words after
> parsing all lookups. Trade meta data in the same why as other lookups.

[...]

> --- a/drivers/net/ethernet/intel/ice/ice_switch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
> @@ -4573,6 +4573,15 @@ static const struct ice_prot_ext_tbl_entry ice_prot_ext[ICE_PROTOCOL_LAST] = {
>  	{ ICE_L2TPV3,		{ 0, 2, 4, 6, 8, 10 } },
>  	{ ICE_VLAN_EX,          { 2, 0 } },
>  	{ ICE_VLAN_IN,          { 2, 0 } },
> +	{ ICE_HW_METADATA,	{ ICE_SOURCE_PORT_MDID_OFFSET,
> +				  ICE_PTYPE_MDID_OFFSET,
> +				  ICE_PACKET_LENGTH_MDID_OFFSET,
> +				  ICE_SOURCE_VSI_MDID_OFFSET,
> +				  ICE_PKT_VLAN_MDID_OFFSET,
> +				  ICE_PKT_TUNNEL_MDID_OFFSET,
> +				  ICE_PKT_TCP_MDID_OFFSET,
> +				  ICE_PKT_ERROR_MDID_OFFSET,
> +				}},

I don't think this is proper indenting. I believe it should like this:

	/* This line is unchanged except the opening brace at the end */
	{ ICE_VLAN_IN,          { 2, 0 } }, {
		ICE_HW_METADATA, {
			ICE_SOURCE_PORT_MDID_OFFSET,
			ICE_PTYPE_MDID_OFFSET,
			[...]
		/* Don't forget commas after last elements */
		},
	},

or

	{
		ICE_HW_METADATA,
		{
			ICE_SOURCE_PORT_MDID_OFFSET,
			ICE_PTYPE_MDID_OFFSET,
			[...]
		},
	},

(but I'd prefer the first one)

Also, I think anonymous initializers are now discouraged in favour of
designated, at least randstruct sometimes complains about that. Could
we start always specifying field names? You could define a macro for
this particular struct to not bloat the code.

>  };
>  
>  static struct ice_protocol_entry ice_prot_id_tbl[ICE_PROTOCOL_LAST] = {
> @@ -4597,6 +4606,7 @@ static struct ice_protocol_entry ice_prot_id_tbl[ICE_PROTOCOL_LAST] = {
>  	{ ICE_L2TPV3,		ICE_L2TPV3_HW },
>  	{ ICE_VLAN_EX,          ICE_VLAN_OF_HW },
>  	{ ICE_VLAN_IN,          ICE_VLAN_OL_HW },
> +	{ ICE_HW_METADATA,      ICE_META_DATA_ID_HW},

Please replace spaces with tabs (as it's done for ICE_L2TPV3_HW).
Also missing space before the last brace.

>  };
>  
>  /**

[...]

> @@ -5726,6 +5663,10 @@ ice_fill_adv_dummy_packet(struct ice_adv_lkup_elem *lkups, u16 lkups_cnt,
>  		 * was already checked when search for the dummy packet
>  		 */
>  		type = lkups[i].type;
> +		/* metadata isn't lockated in packet */

("located", but I'd say "metadata isn't present in the packet")

> +		if (type == ICE_HW_METADATA)
> +			continue;
> +
>  		for (j = 0; offsets[j].type != ICE_PROTOCOL_LAST; j++) {
>  			if (type == offsets[j].type) {
>  				offset = offsets[j].offset;
> @@ -5861,16 +5802,21 @@ ice_fill_adv_packet_tun(struct ice_hw *hw, enum ice_sw_tunnel_type tun_type,
>  
>  /**
>   * ice_fill_adv_packet_vlan - fill dummy packet with VLAN tag type
> + * @hw: pointer to hw structure
>   * @vlan_type: VLAN tag type
>   * @pkt: dummy packet to fill in
>   * @offsets: offset info for the dummy packet
>   */
>  static int
> -ice_fill_adv_packet_vlan(u16 vlan_type, u8 *pkt,
> +ice_fill_adv_packet_vlan(struct ice_hw *hw, u16 vlan_type, u8 *pkt,
>  			 const struct ice_dummy_pkt_offsets *offsets)
>  {
>  	u16 i;
>  
> +	/* Check if there is something to do */
> +	if (vlan_type == 0 || !ice_is_dvm_ena(hw))

`!vlan_type` is preferred over `== 0`.

> +		return 0;
> +
>  	/* Find VLAN header and insert VLAN TPID */
>  	for (i = 0; offsets[i].type != ICE_PROTOCOL_LAST; i++) {
>  		if (offsets[i].type == ICE_VLAN_OFOS ||
> @@ -5889,6 +5835,15 @@ ice_fill_adv_packet_vlan(u16 vlan_type, u8 *pkt,
>  	return -EIO;
>  }
>  
> +static bool ice_is_rule_info_the_same(struct ice_adv_rule_info *first,

Doesn't sound natural. "ice_rules_equal"?

> +				      struct ice_adv_rule_info *second)

The function is read-only, `const` for both arguments.

> +{
> +	return first->sw_act.flag == second->sw_act.flag &&
> +	       first->tun_type == second->tun_type &&
> +	       first->vlan_type == second->vlan_type &&
> +	       first->src_vsi == second->src_vsi;
> +}

[...]

> @@ -6121,7 +6088,12 @@ 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);
> -	rinfo->sw_act.src = ice_get_hw_vsi_num(hw, vsi_handle);
> +
> +	if (rinfo->src_vsi)
> +		rinfo->sw_act.src =
> +			ice_get_hw_vsi_num(hw, rinfo->src_vsi);

This fits into one line in my editor :D

> +	else
> +		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)

[...]

> --- a/drivers/net/ethernet/intel/ice/ice_switch.h
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.h
> @@ -186,11 +186,13 @@ struct ice_adv_rule_flags_info {
>  };
>  
>  struct ice_adv_rule_info {
> +	/* Store metadata values in rule info */
>  	enum ice_sw_tunnel_type tun_type;
> +	u16 vlan_type;
> +	u16 src_vsi;
>  	struct ice_sw_act_ctrl sw_act;
>  	u32 priority;
>  	u16 fltr_rule_id;
> -	u16 vlan_type;
>  	struct ice_adv_rule_flags_info flags_info;

Please check holes within the structure. I see at least one in between
`fltr_rule_id` and `flags_info`. Some fields can definitely be moved around.

>  };
>  
Thanks,
Olek

Re: [Intel-wired-lan] [PATCH net-next v2 3/4] ice: allow matching on meta data

[Michal Swiatkowski]

On Tue, Apr 04, 2023 at 12:22:38PM +0200, Alexander Lobakin wrote:
> From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Date: Tue,  4 Apr 2023 09:28:32 +0200
> 
> > Add meta data matching criteria in the same place as protocol matching
> > criteria. There is no need to add meta data as special words after
> > parsing all lookups. Trade meta data in the same why as other lookups.
> 
> [...]
> 
> > --- a/drivers/net/ethernet/intel/ice/ice_switch.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
> > @@ -4573,6 +4573,15 @@ static const struct ice_prot_ext_tbl_entry ice_prot_ext[ICE_PROTOCOL_LAST] = {
> >  	{ ICE_L2TPV3,		{ 0, 2, 4, 6, 8, 10 } },
> >  	{ ICE_VLAN_EX,          { 2, 0 } },
> >  	{ ICE_VLAN_IN,          { 2, 0 } },
> > +	{ ICE_HW_METADATA,	{ ICE_SOURCE_PORT_MDID_OFFSET,
> > +				  ICE_PTYPE_MDID_OFFSET,
> > +				  ICE_PACKET_LENGTH_MDID_OFFSET,
> > +				  ICE_SOURCE_VSI_MDID_OFFSET,
> > +				  ICE_PKT_VLAN_MDID_OFFSET,
> > +				  ICE_PKT_TUNNEL_MDID_OFFSET,
> > +				  ICE_PKT_TCP_MDID_OFFSET,
> > +				  ICE_PKT_ERROR_MDID_OFFSET,
> > +				}},
> 
> I don't think this is proper indenting. I believe it should like this:
> 
> 	/* This line is unchanged except the opening brace at the end */
> 	{ ICE_VLAN_IN,          { 2, 0 } }, {
> 		ICE_HW_METADATA, {
> 			ICE_SOURCE_PORT_MDID_OFFSET,
> 			ICE_PTYPE_MDID_OFFSET,
> 			[...]
> 		/* Don't forget commas after last elements */
> 		},
> 	},
> 
> or
> 
> 	{
> 		ICE_HW_METADATA,
> 		{
> 			ICE_SOURCE_PORT_MDID_OFFSET,
> 			ICE_PTYPE_MDID_OFFSET,
> 			[...]
> 		},
> 	},
> 
> (but I'd prefer the first one)
> 
> Also, I think anonymous initializers are now discouraged in favour of
> designated, at least randstruct sometimes complains about that. Could
> we start always specifying field names? You could define a macro for
> this particular struct to not bloat the code.
> 
> >  };

Thanks, will fix it in new version.

> >  
> >  static struct ice_protocol_entry ice_prot_id_tbl[ICE_PROTOCOL_LAST] = {
> > @@ -4597,6 +4606,7 @@ static struct ice_protocol_entry ice_prot_id_tbl[ICE_PROTOCOL_LAST] = {
> >  	{ ICE_L2TPV3,		ICE_L2TPV3_HW },
> >  	{ ICE_VLAN_EX,          ICE_VLAN_OF_HW },
> >  	{ ICE_VLAN_IN,          ICE_VLAN_OL_HW },
> > +	{ ICE_HW_METADATA,      ICE_META_DATA_ID_HW},
> 
> Please replace spaces with tabs (as it's done for ICE_L2TPV3_HW).
> Also missing space before the last brace.
> 
> >  };

Sure

> >  
> >  /**
> 
> [...]
> 
> > @@ -5726,6 +5663,10 @@ ice_fill_adv_dummy_packet(struct ice_adv_lkup_elem *lkups, u16 lkups_cnt,
> >  		 * was already checked when search for the dummy packet
> >  		 */
> >  		type = lkups[i].type;
> > +		/* metadata isn't lockated in packet */
> 
> ("located", but I'd say "metadata isn't present in the packet")
>

Right :)

> > +		if (type == ICE_HW_METADATA)
> > +			continue;
> > +
> >  		for (j = 0; offsets[j].type != ICE_PROTOCOL_LAST; j++) {
> >  			if (type == offsets[j].type) {
> >  				offset = offsets[j].offset;
> > @@ -5861,16 +5802,21 @@ ice_fill_adv_packet_tun(struct ice_hw *hw, enum ice_sw_tunnel_type tun_type,
> >  
> >  /**
> >   * ice_fill_adv_packet_vlan - fill dummy packet with VLAN tag type
> > + * @hw: pointer to hw structure
> >   * @vlan_type: VLAN tag type
> >   * @pkt: dummy packet to fill in
> >   * @offsets: offset info for the dummy packet
> >   */
> >  static int
> > -ice_fill_adv_packet_vlan(u16 vlan_type, u8 *pkt,
> > +ice_fill_adv_packet_vlan(struct ice_hw *hw, u16 vlan_type, u8 *pkt,
> >  			 const struct ice_dummy_pkt_offsets *offsets)
> >  {
> >  	u16 i;
> >  
> > +	/* Check if there is something to do */
> > +	if (vlan_type == 0 || !ice_is_dvm_ena(hw))
> 
> `!vlan_type` is preferred over `== 0`.
>

Will do

> > +		return 0;
> > +
> >  	/* Find VLAN header and insert VLAN TPID */
> >  	for (i = 0; offsets[i].type != ICE_PROTOCOL_LAST; i++) {
> >  		if (offsets[i].type == ICE_VLAN_OFOS ||
> > @@ -5889,6 +5835,15 @@ ice_fill_adv_packet_vlan(u16 vlan_type, u8 *pkt,
> >  	return -EIO;
> >  }
> >  
> > +static bool ice_is_rule_info_the_same(struct ice_adv_rule_info *first,
> 
> Doesn't sound natural. "ice_rules_equal"?
>

Sound better, thanks

> > +				      struct ice_adv_rule_info *second)
> 
> The function is read-only, `const` for both arguments.
>

Good point, will do

> > +{
> > +	return first->sw_act.flag == second->sw_act.flag &&
> > +	       first->tun_type == second->tun_type &&
> > +	       first->vlan_type == second->vlan_type &&
> > +	       first->src_vsi == second->src_vsi;
> > +}
> 
> [...]
> 
> > @@ -6121,7 +6088,12 @@ 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);
> > -	rinfo->sw_act.src = ice_get_hw_vsi_num(hw, vsi_handle);
> > +
> > +	if (rinfo->src_vsi)
> > +		rinfo->sw_act.src =
> > +			ice_get_hw_vsi_num(hw, rinfo->src_vsi);
> 
> This fits into one line in my editor :D
>

In my too :D

> > +	else
> > +		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)
> 
> [...]
> 
> > --- a/drivers/net/ethernet/intel/ice/ice_switch.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_switch.h
> > @@ -186,11 +186,13 @@ struct ice_adv_rule_flags_info {
> >  };
> >  
> >  struct ice_adv_rule_info {
> > +	/* Store metadata values in rule info */
> >  	enum ice_sw_tunnel_type tun_type;
> > +	u16 vlan_type;
> > +	u16 src_vsi;
> >  	struct ice_sw_act_ctrl sw_act;
> >  	u32 priority;
> >  	u16 fltr_rule_id;
> > -	u16 vlan_type;
> >  	struct ice_adv_rule_flags_info flags_info;
> 
> Please check holes within the structure. I see at least one in between
> `fltr_rule_id` and `flags_info`. Some fields can definitely be moved around.
> 
> >  };

You are right, will fix in new version.

> >  
> Thanks,
> Olek

[PATCH net-next v2 4/4] ice: use src VSI instead of src MAC in slow-path

[Michal Swiatkowski]

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.

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
sp_rule cover both context (with and without running flag).

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;
 #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;
-- 
2.39.2

Re: [Intel-wired-lan] [PATCH net-next v2 4/4] ice: use src VSI instead of src MAC in slow-path

[Alexander Lobakin]

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

Re: [Intel-wired-lan] [PATCH net-next v2 4/4] ice: use src VSI instead of src MAC in slow-path

[Michal Swiatkowski]

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

Re: [Intel-wired-lan] [PATCH net-next v2 4/4] ice: use src VSI instead of src MAC in slow-path

[Paul Menzel]

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;

Re: [Intel-wired-lan] [PATCH net-next v2 4/4] ice: use src VSI instead of src MAC in slow-path

[Michal Swiatkowski]

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;