netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [iwl-next v1 0/2] ice: extend tc flower offload
@ 2024-02-20 10:59 Michal Swiatkowski
  2024-02-20 10:59 ` [iwl-next v1 1/2] ice: tc: check src_vsi in case of traffic from VF Michal Swiatkowski
  2024-02-20 10:59 ` [iwl-next v1 2/2] ice: tc: allow ip_proto matching Michal Swiatkowski
  0 siblings, 2 replies; 7+ messages in thread
From: Michal Swiatkowski @ 2024-02-20 10:59 UTC (permalink / raw)
  To: intel-wired-lan
  Cc: netdev, marcin.szycik, sridhar.samudrala, wojciech.drewek,
	Michal Swiatkowski

Extend hardware support for tc flower to match ip_proto field (patch 2)
and source VSI metadata when the packets are sent from VF (patch 1).

Michal Swiatkowski (2):
  ice: tc: check src_vsi in case of traffic from VF
  ice: tc: allow ip_proto matching

 drivers/net/ethernet/intel/ice/ice_tc_lib.c | 25 +++++++++++++++++++--
 drivers/net/ethernet/intel/ice/ice_tc_lib.h |  1 +
 2 files changed, 24 insertions(+), 2 deletions(-)

-- 
2.42.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [iwl-next v1 1/2] ice: tc: check src_vsi in case of traffic from VF
  2024-02-20 10:59 [iwl-next v1 0/2] ice: extend tc flower offload Michal Swiatkowski
@ 2024-02-20 10:59 ` Michal Swiatkowski
  2024-02-20 11:23   ` [Intel-wired-lan] " Paul Menzel
  2024-02-20 10:59 ` [iwl-next v1 2/2] ice: tc: allow ip_proto matching Michal Swiatkowski
  1 sibling, 1 reply; 7+ messages in thread
From: Michal Swiatkowski @ 2024-02-20 10:59 UTC (permalink / raw)
  To: intel-wired-lan
  Cc: netdev, marcin.szycik, sridhar.samudrala, wojciech.drewek,
	Michal Swiatkowski, Jedrzej Jagielski

In case of traffic going from the VF (so ingress for port representor)
there should be a check for source VSI. It is needed for hardware to not
match packets from different port with filters added on other port.

It is only for "from VF" traffic, because other traffic direction
doesn't have source VSI.

Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 drivers/net/ethernet/intel/ice/ice_tc_lib.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
index b890410a2bc0..49ed5fd7db10 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
@@ -28,6 +28,8 @@ ice_tc_count_lkups(u32 flags, struct ice_tc_flower_lyr_2_4_hdrs *headers,
 	 * - ICE_TC_FLWR_FIELD_VLAN_TPID (present if specified)
 	 * - Tunnel flag (present if tunnel)
 	 */
+	if (fltr->direction == ICE_ESWITCH_FLTR_EGRESS)
+		lkups_cnt++;
 
 	if (flags & ICE_TC_FLWR_FIELD_TENANT_ID)
 		lkups_cnt++;
@@ -363,6 +365,11 @@ ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
 	/* Always add direction metadata */
 	ice_rule_add_direction_metadata(&list[ICE_TC_METADATA_LKUP_IDX]);
 
+	if (tc_fltr->direction == ICE_ESWITCH_FLTR_EGRESS) {
+		ice_rule_add_src_vsi_metadata(&list[i]);
+		i++;
+	}
+
 	rule_info->tun_type = ice_sw_type_from_tunnel(tc_fltr->tunnel_type);
 	if (tc_fltr->tunnel_type != TNL_LAST) {
 		i = ice_tc_fill_tunnel_outer(flags, tc_fltr, list, i);
@@ -820,6 +827,7 @@ ice_eswitch_add_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
 
 	/* specify the cookie as filter_rule_id */
 	rule_info.fltr_rule_id = fltr->cookie;
+	rule_info.src_vsi = vsi->idx;
 
 	ret = ice_add_adv_rule(hw, list, lkups_cnt, &rule_info, &rule_added);
 	if (ret == -EEXIST) {
-- 
2.42.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [iwl-next v1 2/2] ice: tc: allow ip_proto matching
  2024-02-20 10:59 [iwl-next v1 0/2] ice: extend tc flower offload Michal Swiatkowski
  2024-02-20 10:59 ` [iwl-next v1 1/2] ice: tc: check src_vsi in case of traffic from VF Michal Swiatkowski
@ 2024-02-20 10:59 ` Michal Swiatkowski
  2024-02-20 12:26   ` [Intel-wired-lan] " Paul Menzel
  1 sibling, 1 reply; 7+ messages in thread
From: Michal Swiatkowski @ 2024-02-20 10:59 UTC (permalink / raw)
  To: intel-wired-lan
  Cc: netdev, marcin.szycik, sridhar.samudrala, wojciech.drewek,
	Michal Swiatkowski, Jedrzej Jagielski

Add new matching type. There is no encap version of ip_proto field.

Use it in the same lookup type as for TTL. In hardware it have the same
protocol ID, but different offset.

Example command to add filter with ip_proto:
$tc filter add dev eth10 ingress protocol ip flower ip_proto icmp \
 skip_sw action mirred egress redirect dev eth0

Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 drivers/net/ethernet/intel/ice/ice_tc_lib.c | 17 +++++++++++++++--
 drivers/net/ethernet/intel/ice/ice_tc_lib.h |  1 +
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
index 49ed5fd7db10..f7c0f62fb730 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
@@ -78,7 +78,8 @@ ice_tc_count_lkups(u32 flags, struct ice_tc_flower_lyr_2_4_hdrs *headers,
 		     ICE_TC_FLWR_FIELD_DEST_IPV6 | ICE_TC_FLWR_FIELD_SRC_IPV6))
 		lkups_cnt++;
 
-	if (flags & (ICE_TC_FLWR_FIELD_IP_TOS | ICE_TC_FLWR_FIELD_IP_TTL))
+	if (flags & (ICE_TC_FLWR_FIELD_IP_TOS | ICE_TC_FLWR_FIELD_IP_TTL |
+		     ICE_TC_FLWR_FIELD_IP_PROTO))
 		lkups_cnt++;
 
 	/* are L2TPv3 options specified? */
@@ -530,7 +531,8 @@ ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
 	}
 
 	if (headers->l2_key.n_proto == htons(ETH_P_IP) &&
-	    (flags & (ICE_TC_FLWR_FIELD_IP_TOS | ICE_TC_FLWR_FIELD_IP_TTL))) {
+	    (flags & (ICE_TC_FLWR_FIELD_IP_TOS | ICE_TC_FLWR_FIELD_IP_TTL |
+		      ICE_TC_FLWR_FIELD_IP_PROTO))) {
 		list[i].type = ice_proto_type_from_ipv4(inner);
 
 		if (flags & ICE_TC_FLWR_FIELD_IP_TOS) {
@@ -545,6 +547,13 @@ ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
 				headers->l3_mask.ttl;
 		}
 
+		if (flags & ICE_TC_FLWR_FIELD_IP_PROTO) {
+			list[i].h_u.ipv4_hdr.protocol =
+				headers->l3_key.ip_proto;
+			list[i].m_u.ipv4_hdr.protocol =
+				headers->l3_mask.ip_proto;
+		}
+
 		i++;
 	}
 
@@ -1515,7 +1524,11 @@ ice_parse_cls_flower(struct net_device *filter_dev, struct ice_vsi *vsi,
 
 		headers->l2_key.n_proto = cpu_to_be16(n_proto_key);
 		headers->l2_mask.n_proto = cpu_to_be16(n_proto_mask);
+
+		if (match.key->ip_proto)
+			fltr->flags |= ICE_TC_FLWR_FIELD_IP_PROTO;
 		headers->l3_key.ip_proto = match.key->ip_proto;
+		headers->l3_mask.ip_proto = match.mask->ip_proto;
 	}
 
 	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.h b/drivers/net/ethernet/intel/ice/ice_tc_lib.h
index 65d387163a46..856f371d0687 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.h
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.h
@@ -34,6 +34,7 @@
 #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_FLWR_FIELD_IP_PROTO		BIT(30)
 
 #define ICE_TC_FLOWER_MASK_32   0xFFFFFFFF
 
-- 
2.42.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [Intel-wired-lan] [iwl-next v1 1/2] ice: tc: check src_vsi in case of traffic from VF
  2024-02-20 10:59 ` [iwl-next v1 1/2] ice: tc: check src_vsi in case of traffic from VF Michal Swiatkowski
@ 2024-02-20 11:23   ` Paul Menzel
  2024-02-20 12:24     ` Michal Swiatkowski
  0 siblings, 1 reply; 7+ messages in thread
From: Paul Menzel @ 2024-02-20 11:23 UTC (permalink / raw)
  To: Michal Swiatkowski
  Cc: wojciech.drewek, marcin.szycik, intel-wired-lan, netdev,
	Jedrzej Jagielski, sridhar.samudrala

Dear Michal,


Thank you for the patch.

Am 20.02.24 um 11:59 schrieb Michal Swiatkowski:
> In case of traffic going from the VF (so ingress for port representor)
> there should be a check for source VSI. It is needed for hardware to not
> match packets from different port with filters added on other port.

… from different port*s* …?

> It is only for "from VF" traffic, because other traffic direction
> doesn't have source VSI.

Do you have a test case to reproduce this?

> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_tc_lib.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> index b890410a2bc0..49ed5fd7db10 100644
> --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> @@ -28,6 +28,8 @@ ice_tc_count_lkups(u32 flags, struct ice_tc_flower_lyr_2_4_hdrs *headers,
>   	 * - ICE_TC_FLWR_FIELD_VLAN_TPID (present if specified)
>   	 * - Tunnel flag (present if tunnel)
>   	 */
> +	if (fltr->direction == ICE_ESWITCH_FLTR_EGRESS)
> +		lkups_cnt++;

Why does the count variable need to be incremented?

>   	if (flags & ICE_TC_FLWR_FIELD_TENANT_ID)
>   		lkups_cnt++;
> @@ -363,6 +365,11 @@ ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
>   	/* Always add direction metadata */
>   	ice_rule_add_direction_metadata(&list[ICE_TC_METADATA_LKUP_IDX]);
>   
> +	if (tc_fltr->direction == ICE_ESWITCH_FLTR_EGRESS) {
> +		ice_rule_add_src_vsi_metadata(&list[i]);
> +		i++;
> +	}
> +
>   	rule_info->tun_type = ice_sw_type_from_tunnel(tc_fltr->tunnel_type);
>   	if (tc_fltr->tunnel_type != TNL_LAST) {
>   		i = ice_tc_fill_tunnel_outer(flags, tc_fltr, list, i);
> @@ -820,6 +827,7 @@ ice_eswitch_add_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
>   
>   	/* specify the cookie as filter_rule_id */
>   	rule_info.fltr_rule_id = fltr->cookie;
> +	rule_info.src_vsi = vsi->idx;

Besides the comment above being redundant (as the code does exactly 
that), the new line looks like to belong to the comment. Please excuse 
my ignorance, but the commit message only talks about adding checks and 
not overwriting the `src_vsi`. It’d be great, if you could elaborate.

>   	ret = ice_add_adv_rule(hw, list, lkups_cnt, &rule_info, &rule_added);
>   	if (ret == -EEXIST) {


Kind regards,

Paul

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Intel-wired-lan] [iwl-next v1 1/2] ice: tc: check src_vsi in case of traffic from VF
  2024-02-20 11:23   ` [Intel-wired-lan] " Paul Menzel
@ 2024-02-20 12:24     ` Michal Swiatkowski
  0 siblings, 0 replies; 7+ messages in thread
From: Michal Swiatkowski @ 2024-02-20 12:24 UTC (permalink / raw)
  To: Paul Menzel
  Cc: wojciech.drewek, marcin.szycik, intel-wired-lan, netdev,
	Jedrzej Jagielski, sridhar.samudrala

On Tue, Feb 20, 2024 at 12:23:11PM +0100, Paul Menzel wrote:
> Dear Michal,
> 
> 
> Thank you for the patch.
>

Thanks for the review.

> Am 20.02.24 um 11:59 schrieb Michal Swiatkowski:
> > In case of traffic going from the VF (so ingress for port representor)
> > there should be a check for source VSI. It is needed for hardware to not
> > match packets from different port with filters added on other port.
> 
> … from different port*s* …?
> 

Will fix it.

> > It is only for "from VF" traffic, because other traffic direction
> > doesn't have source VSI.
> 
> Do you have a test case to reproduce this?
>

I can add tc fileter call in v2. In short, any redirect from VF0 to
uplink should allow going packets only from VF0, but currently it is
also matching traffic from other VFs (like VF1, VF2, etc.)

> > Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> > Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> > ---
> >   drivers/net/ethernet/intel/ice/ice_tc_lib.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> > index b890410a2bc0..49ed5fd7db10 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> > @@ -28,6 +28,8 @@ ice_tc_count_lkups(u32 flags, struct ice_tc_flower_lyr_2_4_hdrs *headers,
> >   	 * - ICE_TC_FLWR_FIELD_VLAN_TPID (present if specified)
> >   	 * - Tunnel flag (present if tunnel)
> >   	 */
> > +	if (fltr->direction == ICE_ESWITCH_FLTR_EGRESS)
> > +		lkups_cnt++;
> 
> Why does the count variable need to be incremented?
>
AS you wrote belowe it is needed to add another lookup.

> >   	if (flags & ICE_TC_FLWR_FIELD_TENANT_ID)
> >   		lkups_cnt++;
> > @@ -363,6 +365,11 @@ ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
> >   	/* Always add direction metadata */
> >   	ice_rule_add_direction_metadata(&list[ICE_TC_METADATA_LKUP_IDX]);
> > +	if (tc_fltr->direction == ICE_ESWITCH_FLTR_EGRESS) {
> > +		ice_rule_add_src_vsi_metadata(&list[i]);
> > +		i++;
> > +	}
> > +
> >   	rule_info->tun_type = ice_sw_type_from_tunnel(tc_fltr->tunnel_type);
> >   	if (tc_fltr->tunnel_type != TNL_LAST) {
> >   		i = ice_tc_fill_tunnel_outer(flags, tc_fltr, list, i);
> > @@ -820,6 +827,7 @@ ice_eswitch_add_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
> >   	/* specify the cookie as filter_rule_id */
> >   	rule_info.fltr_rule_id = fltr->cookie;
> > +	rule_info.src_vsi = vsi->idx;
> 
> Besides the comment above being redundant (as the code does exactly that),
> the new line looks like to belong to the comment. Please excuse my
> ignorance, but the commit message only talks about adding checks and not
> overwriting the `src_vsi`. It’d be great, if you could elaborate.
>

I will rephrase commit message to mark that it is not checking in code,
but matching in hardware, thanks.

> >   	ret = ice_add_adv_rule(hw, list, lkups_cnt, &rule_info, &rule_added);
> >   	if (ret == -EEXIST) {
> 
> 
> Kind regards,
> 
> Paul

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Intel-wired-lan] [iwl-next v1 2/2] ice: tc: allow ip_proto matching
  2024-02-20 10:59 ` [iwl-next v1 2/2] ice: tc: allow ip_proto matching Michal Swiatkowski
@ 2024-02-20 12:26   ` Paul Menzel
  2024-02-20 13:14     ` Michal Swiatkowski
  0 siblings, 1 reply; 7+ messages in thread
From: Paul Menzel @ 2024-02-20 12:26 UTC (permalink / raw)
  To: Michal Swiatkowski
  Cc: intel-wired-lan, wojciech.drewek, marcin.szycik, netdev,
	Jedrzej Jagielski, sridhar.samudrala

Dear Michal,


Thank you for the patch. Some minor nits from my side.

Am 20.02.24 um 11:59 schrieb Michal Swiatkowski:
> Add new matching type. There is no encap version of ip_proto field.

Excuse my ignorance, I do not understand the second sentence. Is an 
encap version going to be added?

> Use it in the same lookup type as for TTL. In hardware it have the same

s/have/has/

> protocol ID, but different offset.
> 
> Example command to add filter with ip_proto:
> $tc filter add dev eth10 ingress protocol ip flower ip_proto icmp \
>   skip_sw action mirred egress redirect dev eth0
> 
> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_tc_lib.c | 17 +++++++++++++++--
>   drivers/net/ethernet/intel/ice/ice_tc_lib.h |  1 +
>   2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> index 49ed5fd7db10..f7c0f62fb730 100644
> --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> @@ -78,7 +78,8 @@ ice_tc_count_lkups(u32 flags, struct ice_tc_flower_lyr_2_4_hdrs *headers,
>   		     ICE_TC_FLWR_FIELD_DEST_IPV6 | ICE_TC_FLWR_FIELD_SRC_IPV6))
>   		lkups_cnt++;
>   
> -	if (flags & (ICE_TC_FLWR_FIELD_IP_TOS | ICE_TC_FLWR_FIELD_IP_TTL))
> +	if (flags & (ICE_TC_FLWR_FIELD_IP_TOS | ICE_TC_FLWR_FIELD_IP_TTL |
> +		     ICE_TC_FLWR_FIELD_IP_PROTO))

Should this be sorted? (Also below).

>   		lkups_cnt++;
>   
>   	/* are L2TPv3 options specified? */
> @@ -530,7 +531,8 @@ ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
>   	}
>   
>   	if (headers->l2_key.n_proto == htons(ETH_P_IP) &&
> -	    (flags & (ICE_TC_FLWR_FIELD_IP_TOS | ICE_TC_FLWR_FIELD_IP_TTL))) {
> +	    (flags & (ICE_TC_FLWR_FIELD_IP_TOS | ICE_TC_FLWR_FIELD_IP_TTL |
> +		      ICE_TC_FLWR_FIELD_IP_PROTO))) {
>   		list[i].type = ice_proto_type_from_ipv4(inner);
>   
>   		if (flags & ICE_TC_FLWR_FIELD_IP_TOS) {
> @@ -545,6 +547,13 @@ ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
>   				headers->l3_mask.ttl;
>   		}
>   
> +		if (flags & ICE_TC_FLWR_FIELD_IP_PROTO) {
> +			list[i].h_u.ipv4_hdr.protocol =
> +				headers->l3_key.ip_proto;
> +			list[i].m_u.ipv4_hdr.protocol =
> +				headers->l3_mask.ip_proto;

(Strange to break the line each time, but seems to be the surrounding 
coding style.)

> +		}
> +
>   		i++;
>   	}
>   
> @@ -1515,7 +1524,11 @@ ice_parse_cls_flower(struct net_device *filter_dev, struct ice_vsi *vsi,
>   
>   		headers->l2_key.n_proto = cpu_to_be16(n_proto_key);
>   		headers->l2_mask.n_proto = cpu_to_be16(n_proto_mask);
> +
> +		if (match.key->ip_proto)
> +			fltr->flags |= ICE_TC_FLWR_FIELD_IP_PROTO;
>   		headers->l3_key.ip_proto = match.key->ip_proto;
> +		headers->l3_mask.ip_proto = match.mask->ip_proto;
>   	}
>   
>   	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
> diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.h b/drivers/net/ethernet/intel/ice/ice_tc_lib.h
> index 65d387163a46..856f371d0687 100644
> --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.h
> +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.h
> @@ -34,6 +34,7 @@
>   #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_FLWR_FIELD_IP_PROTO		BIT(30)
>   
>   #define ICE_TC_FLOWER_MASK_32   0xFFFFFFFF
>   


Kind regards,

Paul

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Intel-wired-lan] [iwl-next v1 2/2] ice: tc: allow ip_proto matching
  2024-02-20 12:26   ` [Intel-wired-lan] " Paul Menzel
@ 2024-02-20 13:14     ` Michal Swiatkowski
  0 siblings, 0 replies; 7+ messages in thread
From: Michal Swiatkowski @ 2024-02-20 13:14 UTC (permalink / raw)
  To: Paul Menzel
  Cc: intel-wired-lan, wojciech.drewek, marcin.szycik, netdev,
	Jedrzej Jagielski, sridhar.samudrala

On Tue, Feb 20, 2024 at 01:26:34PM +0100, Paul Menzel wrote:
> Dear Michal,
> 
> 
> Thank you for the patch. Some minor nits from my side.
> 
> Am 20.02.24 um 11:59 schrieb Michal Swiatkowski:
> > Add new matching type. There is no encap version of ip_proto field.
> 
> Excuse my ignorance, I do not understand the second sentence. Is an encap
> version going to be added?
> 

No, I will rephrase it, thanks.

> > Use it in the same lookup type as for TTL. In hardware it have the same
> 
> s/have/has/
>

Will fix.

> > protocol ID, but different offset.
> > 
> > Example command to add filter with ip_proto:
> > $tc filter add dev eth10 ingress protocol ip flower ip_proto icmp \
> >   skip_sw action mirred egress redirect dev eth0
> > 
> > Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> > ---
> >   drivers/net/ethernet/intel/ice/ice_tc_lib.c | 17 +++++++++++++++--
> >   drivers/net/ethernet/intel/ice/ice_tc_lib.h |  1 +
> >   2 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> > index 49ed5fd7db10..f7c0f62fb730 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> > @@ -78,7 +78,8 @@ ice_tc_count_lkups(u32 flags, struct ice_tc_flower_lyr_2_4_hdrs *headers,
> >   		     ICE_TC_FLWR_FIELD_DEST_IPV6 | ICE_TC_FLWR_FIELD_SRC_IPV6))
> >   		lkups_cnt++;
> > -	if (flags & (ICE_TC_FLWR_FIELD_IP_TOS | ICE_TC_FLWR_FIELD_IP_TTL))
> > +	if (flags & (ICE_TC_FLWR_FIELD_IP_TOS | ICE_TC_FLWR_FIELD_IP_TTL |
> > +		     ICE_TC_FLWR_FIELD_IP_PROTO))
> 
> Should this be sorted? (Also below).
>

Do you mean PROTO before TOS and TTL? I like the current order, because
for ipv6 we don't have PROTO, but we have TOS and TTL, it looks better
when PROTO is as additional one here.

> >   		lkups_cnt++;
> >   	/* are L2TPv3 options specified? */
> > @@ -530,7 +531,8 @@ ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
> >   	}
> >   	if (headers->l2_key.n_proto == htons(ETH_P_IP) &&
> > -	    (flags & (ICE_TC_FLWR_FIELD_IP_TOS | ICE_TC_FLWR_FIELD_IP_TTL))) {
> > +	    (flags & (ICE_TC_FLWR_FIELD_IP_TOS | ICE_TC_FLWR_FIELD_IP_TTL |
> > +		      ICE_TC_FLWR_FIELD_IP_PROTO))) {
> >   		list[i].type = ice_proto_type_from_ipv4(inner);
> >   		if (flags & ICE_TC_FLWR_FIELD_IP_TOS) {
> > @@ -545,6 +547,13 @@ ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
> >   				headers->l3_mask.ttl;
> >   		}
> > +		if (flags & ICE_TC_FLWR_FIELD_IP_PROTO) {
> > +			list[i].h_u.ipv4_hdr.protocol =
> > +				headers->l3_key.ip_proto;
> > +			list[i].m_u.ipv4_hdr.protocol =
> > +				headers->l3_mask.ip_proto;
> 
> (Strange to break the line each time, but seems to be the surrounding coding
> style.)
>

Yeah, without breaking it is longer than 80.

> > +		}
> > +
> >   		i++;
> >   	}
> > @@ -1515,7 +1524,11 @@ ice_parse_cls_flower(struct net_device *filter_dev, struct ice_vsi *vsi,
> >   		headers->l2_key.n_proto = cpu_to_be16(n_proto_key);
> >   		headers->l2_mask.n_proto = cpu_to_be16(n_proto_mask);
> > +
> > +		if (match.key->ip_proto)
> > +			fltr->flags |= ICE_TC_FLWR_FIELD_IP_PROTO;
> >   		headers->l3_key.ip_proto = match.key->ip_proto;
> > +		headers->l3_mask.ip_proto = match.mask->ip_proto;
> >   	}
> >   	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
> > diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.h b/drivers/net/ethernet/intel/ice/ice_tc_lib.h
> > index 65d387163a46..856f371d0687 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.h
> > @@ -34,6 +34,7 @@
> >   #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_FLWR_FIELD_IP_PROTO		BIT(30)
> >   #define ICE_TC_FLOWER_MASK_32   0xFFFFFFFF
> 
> 
> Kind regards,
> 
> Paul

Thanks,
Michal

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2024-02-20 13:14 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-20 10:59 [iwl-next v1 0/2] ice: extend tc flower offload Michal Swiatkowski
2024-02-20 10:59 ` [iwl-next v1 1/2] ice: tc: check src_vsi in case of traffic from VF Michal Swiatkowski
2024-02-20 11:23   ` [Intel-wired-lan] " Paul Menzel
2024-02-20 12:24     ` Michal Swiatkowski
2024-02-20 10:59 ` [iwl-next v1 2/2] ice: tc: allow ip_proto matching Michal Swiatkowski
2024-02-20 12:26   ` [Intel-wired-lan] " Paul Menzel
2024-02-20 13:14     ` Michal Swiatkowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).