[PATCH net-next v1 0/3] net/mlx5e: Add GBP VxLAN HW offload support

[PATCH net-next v1 0/3] net/mlx5e: Add GBP VxLAN HW offload support

[Gavin Li]

This patch series adds HW offloading support for TC flows with VxLAN GBP encap/decap.

Patch-1: Expose helper function vxlan_build_gbp_hdr.
Patch-2: Add helper function for encap_info_equal for tunnels with options.
Patch-3: Add HW offloading support for TC flows with VxLAN GBP encap/decap
        in mlx ethernet driver.

Gavin Li (3):
  vxlan: Expose helper vxlan_build_gbp_hdr
  net/mlx5e: Add helper for encap_info_equal for tunnels with options
  net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload

 .../ethernet/mellanox/mlx5/core/en/tc_tun.h   |  3 +
 .../mellanox/mlx5/core/en/tc_tun_encap.c      | 29 +++++++
 .../mellanox/mlx5/core/en/tc_tun_geneve.c     | 24 +-----
 .../mellanox/mlx5/core/en/tc_tun_vxlan.c      | 85 ++++++++++++++++++-
 drivers/net/vxlan/vxlan_core.c                | 20 -----
 include/linux/mlx5/device.h                   |  6 ++
 include/linux/mlx5/mlx5_ifc.h                 | 13 ++-
 include/net/vxlan.h                           | 20 +++++
 8 files changed, 153 insertions(+), 47 deletions(-)

-- 
2.31.1

[PATCH net-next v1 1/3] vxlan: Expose helper vxlan_build_gbp_hdr

[Gavin Li]

vxlan_build_gbp_hdr will be used by other modules to build gbp option in
vxlan header according to gbp flags.

Change-Id: I10d8dd31d6048e1fcd08cd76ee3bcd3029053552
Signed-off-by: Gavin Li <gavinl@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
Reviewed-by: Maor Dickman <maord@nvidia.com>
Acked-by: Saeed Mahameed <saeedm@nvidia.com>
---
 drivers/net/vxlan/vxlan_core.c | 20 --------------------
 include/net/vxlan.h            | 20 ++++++++++++++++++++
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index b1b179effe2a..bd44467a5a39 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -2140,26 +2140,6 @@ static bool route_shortcircuit(struct net_device *dev, struct sk_buff *skb)
 	return false;
 }
 
-static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, u32 vxflags,
-				struct vxlan_metadata *md)
-{
-	struct vxlanhdr_gbp *gbp;
-
-	if (!md->gbp)
-		return;
-
-	gbp = (struct vxlanhdr_gbp *)vxh;
-	vxh->vx_flags |= VXLAN_HF_GBP;
-
-	if (md->gbp & VXLAN_GBP_DONT_LEARN)
-		gbp->dont_learn = 1;
-
-	if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
-		gbp->policy_applied = 1;
-
-	gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
-}
-
 static int vxlan_build_gpe_hdr(struct vxlanhdr *vxh, u32 vxflags,
 			       __be16 protocol)
 {
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index bca5b01af247..08bc762a7e94 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -566,4 +566,24 @@ static inline bool vxlan_fdb_nh_path_select(struct nexthop *nh,
 	return true;
 }
 
+static inline void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, u32 vxflags,
+				       struct vxlan_metadata *md)
+{
+	struct vxlanhdr_gbp *gbp;
+
+	if (!md->gbp)
+		return;
+
+	gbp = (struct vxlanhdr_gbp *)vxh;
+	vxh->vx_flags |= VXLAN_HF_GBP;
+
+	if (md->gbp & VXLAN_GBP_DONT_LEARN)
+		gbp->dont_learn = 1;
+
+	if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
+		gbp->policy_applied = 1;
+
+	gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
+}
+
 #endif
-- 
2.31.1

Re: [PATCH net-next v1 1/3] vxlan: Expose helper vxlan_build_gbp_hdr

[Alexander Lobakin]

From: Gavin Li <gavinl@nvidia.com>
Date: Tue, 14 Feb 2023 15:41:35 +0200

> vxlan_build_gbp_hdr will be used by other modules to build gbp option in
> vxlan header according to gbp flags.

(not sure if it's okay to write abbreviations with non-capital)

> 
> Change-Id: I10d8dd31d6048e1fcd08cd76ee3bcd3029053552
> Signed-off-by: Gavin Li <gavinl@nvidia.com>
> Reviewed-by: Roi Dayan <roid@nvidia.com>
> Reviewed-by: Maor Dickman <maord@nvidia.com>
> Acked-by: Saeed Mahameed <saeedm@nvidia.com>

Besides the nit above:

Reviewed-by: Alexander Lobakin <alexandr.lobakin@intel.com>

> ---
>  drivers/net/vxlan/vxlan_core.c | 20 --------------------
>  include/net/vxlan.h            | 20 ++++++++++++++++++++
>  2 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index b1b179effe2a..bd44467a5a39 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -2140,26 +2140,6 @@ static bool route_shortcircuit(struct net_device *dev, struct sk_buff *skb)
>  	return false;
>  }
>  
> -static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, u32 vxflags,
> -				struct vxlan_metadata *md)
> -{
> -	struct vxlanhdr_gbp *gbp;
> -
> -	if (!md->gbp)
> -		return;
> -
> -	gbp = (struct vxlanhdr_gbp *)vxh;
> -	vxh->vx_flags |= VXLAN_HF_GBP;
> -
> -	if (md->gbp & VXLAN_GBP_DONT_LEARN)
> -		gbp->dont_learn = 1;
> -
> -	if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
> -		gbp->policy_applied = 1;
> -
> -	gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
> -}
> -
>  static int vxlan_build_gpe_hdr(struct vxlanhdr *vxh, u32 vxflags,
>  			       __be16 protocol)
>  {
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index bca5b01af247..08bc762a7e94 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -566,4 +566,24 @@ static inline bool vxlan_fdb_nh_path_select(struct nexthop *nh,
>  	return true;
>  }
>  
> +static inline void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, u32 vxflags,
> +				       struct vxlan_metadata *md)
> +{
> +	struct vxlanhdr_gbp *gbp;
> +
> +	if (!md->gbp)
> +		return;
> +
> +	gbp = (struct vxlanhdr_gbp *)vxh;
> +	vxh->vx_flags |= VXLAN_HF_GBP;
> +
> +	if (md->gbp & VXLAN_GBP_DONT_LEARN)
> +		gbp->dont_learn = 1;
> +
> +	if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
> +		gbp->policy_applied = 1;
> +
> +	gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
> +}
> +
>  #endif

Thanks,
Olek

Re: [PATCH net-next v1 1/3] vxlan: Expose helper vxlan_build_gbp_hdr

[Gavin Li]

On 2/14/2023 10:56 PM, Alexander Lobakin wrote:
> External email: Use caution opening links or attachments
>
>
> From: Gavin Li <gavinl@nvidia.com>
> Date: Tue, 14 Feb 2023 15:41:35 +0200
>
>> vxlan_build_gbp_hdr will be used by other modules to build gbp option in
>> vxlan header according to gbp flags.
> (not sure if it's okay to write abbreviations with non-capital)
Don't sure either but most of the functions in vxlan (if not all) are 
declared in this way.
>
>> Change-Id: I10d8dd31d6048e1fcd08cd76ee3bcd3029053552
>> Signed-off-by: Gavin Li <gavinl@nvidia.com>
>> Reviewed-by: Roi Dayan <roid@nvidia.com>
>> Reviewed-by: Maor Dickman <maord@nvidia.com>
>> Acked-by: Saeed Mahameed <saeedm@nvidia.com>
> Besides the nit above:
>
> Reviewed-by: Alexander Lobakin <alexandr.lobakin@intel.com>
>
>> ---
>>   drivers/net/vxlan/vxlan_core.c | 20 --------------------
>>   include/net/vxlan.h            | 20 ++++++++++++++++++++
>>   2 files changed, 20 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
>> index b1b179effe2a..bd44467a5a39 100644
>> --- a/drivers/net/vxlan/vxlan_core.c
>> +++ b/drivers/net/vxlan/vxlan_core.c
>> @@ -2140,26 +2140,6 @@ static bool route_shortcircuit(struct net_device *dev, struct sk_buff *skb)
>>        return false;
>>   }
>>
>> -static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, u32 vxflags,
>> -                             struct vxlan_metadata *md)
>> -{
>> -     struct vxlanhdr_gbp *gbp;
>> -
>> -     if (!md->gbp)
>> -             return;
>> -
>> -     gbp = (struct vxlanhdr_gbp *)vxh;
>> -     vxh->vx_flags |= VXLAN_HF_GBP;
>> -
>> -     if (md->gbp & VXLAN_GBP_DONT_LEARN)
>> -             gbp->dont_learn = 1;
>> -
>> -     if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
>> -             gbp->policy_applied = 1;
>> -
>> -     gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
>> -}
>> -
>>   static int vxlan_build_gpe_hdr(struct vxlanhdr *vxh, u32 vxflags,
>>                               __be16 protocol)
>>   {
>> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
>> index bca5b01af247..08bc762a7e94 100644
>> --- a/include/net/vxlan.h
>> +++ b/include/net/vxlan.h
>> @@ -566,4 +566,24 @@ static inline bool vxlan_fdb_nh_path_select(struct nexthop *nh,
>>        return true;
>>   }
>>
>> +static inline void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, u32 vxflags,
>> +                                    struct vxlan_metadata *md)
>> +{
>> +     struct vxlanhdr_gbp *gbp;
>> +
>> +     if (!md->gbp)
>> +             return;
>> +
>> +     gbp = (struct vxlanhdr_gbp *)vxh;
>> +     vxh->vx_flags |= VXLAN_HF_GBP;
>> +
>> +     if (md->gbp & VXLAN_GBP_DONT_LEARN)
>> +             gbp->dont_learn = 1;
>> +
>> +     if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
>> +             gbp->policy_applied = 1;
>> +
>> +     gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
>> +}
>> +
>>   #endif
> Thanks,
> Olek

[PATCH net-next v1 2/3] net/mlx5e: Add helper for encap_info_equal for tunnels with options

[Gavin Li]

For tunnels with options, eg, geneve and vxlan with gbp, they share the
same way to compare the headers and options. Extract the code as a common
function for them

Change-Id: I3ea697293c8d5d66c0c20080dbde88f60bcbd62f
Signed-off-by: Gavin Li <gavinl@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
Reviewed-by: Maor Dickman <maord@nvidia.com>
Acked-by: Saeed Mahameed <saeedm@nvidia.com>
---
 .../ethernet/mellanox/mlx5/core/en/tc_tun.h   |  3 ++
 .../mellanox/mlx5/core/en/tc_tun_encap.c      | 29 +++++++++++++++++++
 .../mellanox/mlx5/core/en/tc_tun_geneve.c     | 24 +--------------
 3 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
index b38f693bbb52..92065568bb19 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
@@ -115,6 +115,9 @@ int mlx5e_tc_tun_parse_udp_ports(struct mlx5e_priv *priv,
 bool mlx5e_tc_tun_encap_info_equal_generic(struct mlx5e_encap_key *a,
 					   struct mlx5e_encap_key *b);
 
+bool mlx5e_tc_tun_encap_info_equal_options(struct mlx5e_encap_key *a,
+					   struct mlx5e_encap_key *b,
+					   __be16 tun_flags);
 #endif /* CONFIG_MLX5_ESWITCH */
 
 #endif //__MLX5_EN_TC_TUNNEL_H__
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
index 780224fd67a1..4df9d27a63ad 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
@@ -571,6 +571,35 @@ bool mlx5e_tc_tun_encap_info_equal_generic(struct mlx5e_encap_key *a,
 		a->tc_tunnel->tunnel_type == b->tc_tunnel->tunnel_type;
 }
 
+bool mlx5e_tc_tun_encap_info_equal_options(struct mlx5e_encap_key *a,
+					   struct mlx5e_encap_key *b,
+					   __be16 tun_flags)
+{
+	struct ip_tunnel_info *a_info;
+	struct ip_tunnel_info *b_info;
+	bool a_has_opts, b_has_opts;
+
+	if (!mlx5e_tc_tun_encap_info_equal_generic(a, b))
+		return false;
+
+	a_has_opts = !!(a->ip_tun_key->tun_flags & tun_flags);
+	b_has_opts = !!(b->ip_tun_key->tun_flags & tun_flags);
+
+	/* keys are equal when both don't have any options attached */
+	if (!a_has_opts && !b_has_opts)
+		return true;
+
+	if (a_has_opts != b_has_opts)
+		return false;
+
+	/* options stored in memory next to ip_tunnel_info struct */
+	a_info = container_of(a->ip_tun_key, struct ip_tunnel_info, key);
+	b_info = container_of(b->ip_tun_key, struct ip_tunnel_info, key);
+
+	return a_info->options_len == b_info->options_len &&
+		memcmp(a_info + 1, b_info + 1, a_info->options_len) == 0;
+}
+
 static int cmp_decap_info(struct mlx5e_decap_key *a,
 			  struct mlx5e_decap_key *b)
 {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c
index 054d80c4e65c..2bcd10b6d653 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c
@@ -337,29 +337,7 @@ static int mlx5e_tc_tun_parse_geneve(struct mlx5e_priv *priv,
 static bool mlx5e_tc_tun_encap_info_equal_geneve(struct mlx5e_encap_key *a,
 						 struct mlx5e_encap_key *b)
 {
-	struct ip_tunnel_info *a_info;
-	struct ip_tunnel_info *b_info;
-	bool a_has_opts, b_has_opts;
-
-	if (!mlx5e_tc_tun_encap_info_equal_generic(a, b))
-		return false;
-
-	a_has_opts = !!(a->ip_tun_key->tun_flags & TUNNEL_GENEVE_OPT);
-	b_has_opts = !!(b->ip_tun_key->tun_flags & TUNNEL_GENEVE_OPT);
-
-	/* keys are equal when both don't have any options attached */
-	if (!a_has_opts && !b_has_opts)
-		return true;
-
-	if (a_has_opts != b_has_opts)
-		return false;
-
-	/* geneve options stored in memory next to ip_tunnel_info struct */
-	a_info = container_of(a->ip_tun_key, struct ip_tunnel_info, key);
-	b_info = container_of(b->ip_tun_key, struct ip_tunnel_info, key);
-
-	return a_info->options_len == b_info->options_len &&
-		memcmp(a_info + 1, b_info + 1, a_info->options_len) == 0;
+	return mlx5e_tc_tun_encap_info_equal_options(a, b, TUNNEL_GENEVE_OPT);
 }
 
 struct mlx5e_tc_tunnel geneve_tunnel = {
-- 
2.31.1

Re: [PATCH net-next v1 2/3] net/mlx5e: Add helper for encap_info_equal for tunnels with options

[Alexander Lobakin]

From: Gavin Li <gavinl@nvidia.com>
Date: Tue, 14 Feb 2023 15:41:36 +0200

(dropping non-existent nikolay@nvidia.com)

> For tunnels with options, eg, geneve and vxlan with gbp, they share the
> same way to compare the headers and options. Extract the code as a common
> function for them
> 
> Change-Id: I3ea697293c8d5d66c0c20080dbde88f60bcbd62f
> Signed-off-by: Gavin Li <gavinl@nvidia.com>
> Reviewed-by: Roi Dayan <roid@nvidia.com>
> Reviewed-by: Maor Dickman <maord@nvidia.com>
> Acked-by: Saeed Mahameed <saeedm@nvidia.com>

[...]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
> index 780224fd67a1..4df9d27a63ad 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
> @@ -571,6 +571,35 @@ bool mlx5e_tc_tun_encap_info_equal_generic(struct mlx5e_encap_key *a,
>  		a->tc_tunnel->tunnel_type == b->tc_tunnel->tunnel_type;
>  }
>  
> +bool mlx5e_tc_tun_encap_info_equal_options(struct mlx5e_encap_key *a,
> +					   struct mlx5e_encap_key *b,
> +					   __be16 tun_flags)
> +{
> +	struct ip_tunnel_info *a_info;
> +	struct ip_tunnel_info *b_info;
> +	bool a_has_opts, b_has_opts;
> +
> +	if (!mlx5e_tc_tun_encap_info_equal_generic(a, b))
> +		return false;
> +
> +	a_has_opts = !!(a->ip_tun_key->tun_flags & tun_flags);
> +	b_has_opts = !!(b->ip_tun_key->tun_flags & tun_flags);
> +
> +	/* keys are equal when both don't have any options attached */
> +	if (!a_has_opts && !b_has_opts)
> +		return true;
> +
> +	if (a_has_opts != b_has_opts)
> +		return false;
> +
> +	/* options stored in memory next to ip_tunnel_info struct */
> +	a_info = container_of(a->ip_tun_key, struct ip_tunnel_info, key);
> +	b_info = container_of(b->ip_tun_key, struct ip_tunnel_info, key);
> +
> +	return a_info->options_len == b_info->options_len &&
> +		memcmp(a_info + 1, b_info + 1, a_info->options_len) == 0;

1. memcmp() is not aligned to the first expr (off-by-one to the right).
2. `!expr` is preferred over `expr == 0`.

> +}
> +
>  static int cmp_decap_info(struct mlx5e_decap_key *a,
>  			  struct mlx5e_decap_key *b)
>  {
[...]

Thanks,
Olek 

Re: [PATCH net-next v1 2/3] net/mlx5e: Add helper for encap_info_equal for tunnels with options

[Gavin Li]

On 2/14/2023 11:01 PM, Alexander Lobakin wrote:
> External email: Use caution opening links or attachments
>
>
> From: Gavin Li <gavinl@nvidia.com>
> Date: Tue, 14 Feb 2023 15:41:36 +0200
>
> (dropping non-existent nikolay@nvidia.com)
>
>> For tunnels with options, eg, geneve and vxlan with gbp, they share the
>> same way to compare the headers and options. Extract the code as a common
>> function for them
>>
>> Change-Id: I3ea697293c8d5d66c0c20080dbde88f60bcbd62f
>> Signed-off-by: Gavin Li <gavinl@nvidia.com>
>> Reviewed-by: Roi Dayan <roid@nvidia.com>
>> Reviewed-by: Maor Dickman <maord@nvidia.com>
>> Acked-by: Saeed Mahameed <saeedm@nvidia.com>
> [...]
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
>> index 780224fd67a1..4df9d27a63ad 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
>> @@ -571,6 +571,35 @@ bool mlx5e_tc_tun_encap_info_equal_generic(struct mlx5e_encap_key *a,
>>                a->tc_tunnel->tunnel_type == b->tc_tunnel->tunnel_type;
>>   }
>>
>> +bool mlx5e_tc_tun_encap_info_equal_options(struct mlx5e_encap_key *a,
>> +                                        struct mlx5e_encap_key *b,
>> +                                        __be16 tun_flags)
>> +{
>> +     struct ip_tunnel_info *a_info;
>> +     struct ip_tunnel_info *b_info;
>> +     bool a_has_opts, b_has_opts;
>> +
>> +     if (!mlx5e_tc_tun_encap_info_equal_generic(a, b))
>> +             return false;
>> +
>> +     a_has_opts = !!(a->ip_tun_key->tun_flags & tun_flags);
>> +     b_has_opts = !!(b->ip_tun_key->tun_flags & tun_flags);
>> +
>> +     /* keys are equal when both don't have any options attached */
>> +     if (!a_has_opts && !b_has_opts)
>> +             return true;
>> +
>> +     if (a_has_opts != b_has_opts)
>> +             return false;
>> +
>> +     /* options stored in memory next to ip_tunnel_info struct */
>> +     a_info = container_of(a->ip_tun_key, struct ip_tunnel_info, key);
>> +     b_info = container_of(b->ip_tun_key, struct ip_tunnel_info, key);
>> +
>> +     return a_info->options_len == b_info->options_len &&
>> +             memcmp(a_info + 1, b_info + 1, a_info->options_len) == 0;
> 1. memcmp() is not aligned to the first expr (off-by-one to the right).
Options start from "info + 1", see ip_tunnel_info_opts and will use it 
here to replace the "info+1".
> 2. `!expr` is preferred over `expr == 0`.
ACK
>
>> +}
>> +
>>   static int cmp_decap_info(struct mlx5e_decap_key *a,
>>                          struct mlx5e_decap_key *b)
>>   {
> [...]
>
> Thanks,
> Olek

Re: [PATCH net-next v1 2/3] net/mlx5e: Add helper for encap_info_equal for tunnels with options

[Alexander Lobakin]

From: Gavin Li <gavinl@nvidia.com>
Date: Wed, 15 Feb 2023 10:54:12 +0800

> 
> On 2/14/2023 11:01 PM, Alexander Lobakin wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> From: Gavin Li <gavinl@nvidia.com>
>> Date: Tue, 14 Feb 2023 15:41:36 +0200

[...]

>>> +     if (a_has_opts != b_has_opts)
>>> +             return false;
>>> +
>>> +     /* options stored in memory next to ip_tunnel_info struct */
>>> +     a_info = container_of(a->ip_tun_key, struct ip_tunnel_info, key);
>>> +     b_info = container_of(b->ip_tun_key, struct ip_tunnel_info, key);
>>> +
>>> +     return a_info->options_len == b_info->options_len &&
>>> +             memcmp(a_info + 1, b_info + 1, a_info->options_len) == 0;
>> 1. memcmp() is not aligned to the first expr (off-by-one to the right).
> Options start from "info + 1", see ip_tunnel_info_opts and will use it
> here to replace the "info+1".

Nah, I mean the following. Your code:

	return a_info->options_len == b_info->options_len &&
		memcmp(a_info + 1, b_info + 1, ...

should be:

	return a_info->options_len == b_info->options_len &&
	       memcmp(a_info + 1, b_info + 1, ...

7 spaces instead of a tab to have it aligned to the prev line.

>> 2. `!expr` is preferred over `expr == 0`.
> ACK
>>
>>> +}
>>> +
>>>   static int cmp_decap_info(struct mlx5e_decap_key *a,
>>>                          struct mlx5e_decap_key *b)
>>>   {
>> [...]
>>
>> Thanks,
>> Olek
Thanks,
Olek

Re: [PATCH net-next v1 2/3] net/mlx5e: Add helper for encap_info_equal for tunnels with options

[Gavin Li]

On 2/16/2023 12:56 AM, Alexander Lobakin wrote:
> External email: Use caution opening links or attachments
>
>
> From: Gavin Li <gavinl@nvidia.com>
> Date: Wed, 15 Feb 2023 10:54:12 +0800
>
>> On 2/14/2023 11:01 PM, Alexander Lobakin wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> From: Gavin Li <gavinl@nvidia.com>
>>> Date: Tue, 14 Feb 2023 15:41:36 +0200
> [...]
>
>>>> +     if (a_has_opts != b_has_opts)
>>>> +             return false;
>>>> +
>>>> +     /* options stored in memory next to ip_tunnel_info struct */
>>>> +     a_info = container_of(a->ip_tun_key, struct ip_tunnel_info, key);
>>>> +     b_info = container_of(b->ip_tun_key, struct ip_tunnel_info, key);
>>>> +
>>>> +     return a_info->options_len == b_info->options_len &&
>>>> +             memcmp(a_info + 1, b_info + 1, a_info->options_len) == 0;
>>> 1. memcmp() is not aligned to the first expr (off-by-one to the right).
>> Options start from "info + 1", see ip_tunnel_info_opts and will use it
>> here to replace the "info+1".
> Nah, I mean the following. Your code:
>
>          return a_info->options_len == b_info->options_len &&
>                  memcmp(a_info + 1, b_info + 1, ...
>
> should be:
>
>          return a_info->options_len == b_info->options_len &&
>                 memcmp(a_info + 1, b_info + 1, ...
>
> 7 spaces instead of a tab to have it aligned to the prev line.
ACK
>
>>> 2. `!expr` is preferred over `expr == 0`.
>> ACK
>>>> +}
>>>> +
>>>>    static int cmp_decap_info(struct mlx5e_decap_key *a,
>>>>                           struct mlx5e_decap_key *b)
>>>>    {
>>> [...]
>>>
>>> Thanks,
>>> Olek
> Thanks,
> Olek

[PATCH net-next v1 3/3] net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload

[Gavin Li]

Add HW offloading support for TC flows with VxLAN GBP encap/decap.

Example of encap rule:
tc filter add dev eth0 protocol ip ingress flower \
    action tunnel_key set id 42 vxlan_opts 512 \
    action mirred egress redirect dev vxlan1

Example of decap rule:
tc filter add dev vxlan1 protocol ip ingress flower \
    enc_key_id 42 enc_dst_port 4789 vxlan_opts 1024 \
    action tunnel_key unset action mirred egress redirect dev eth0

Change-Id: I48f61d02201bf3f79dcbe5d0f022f7bb27ed630f
Signed-off-by: Gavin Li <gavinl@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
Reviewed-by: Maor Dickman <maord@nvidia.com>
Acked-by: Saeed Mahameed <saeedm@nvidia.com>
---
 .../mellanox/mlx5/core/en/tc_tun_vxlan.c      | 85 ++++++++++++++++++-
 include/linux/mlx5/device.h                   |  6 ++
 include/linux/mlx5/mlx5_ifc.h                 | 13 ++-
 3 files changed, 100 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
index 1f62c702b625..444512ca9e0d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
 /* Copyright (c) 2018 Mellanox Technologies. */
 
+#include <net/ip_tunnels.h>
 #include <net/vxlan.h>
 #include "lib/vxlan.h"
 #include "en/tc_tun.h"
@@ -86,9 +87,11 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
 	const struct ip_tunnel_key *tun_key = &e->tun_info->key;
 	__be32 tun_id = tunnel_id_to_key32(tun_key->tun_id);
 	struct udphdr *udp = (struct udphdr *)(buf);
+	const struct vxlan_metadata *md;
 	struct vxlanhdr *vxh;
 
-	if (tun_key->tun_flags & TUNNEL_VXLAN_OPT)
+	if (tun_key->tun_flags & TUNNEL_VXLAN_OPT &&
+	    e->tun_info->options_len != sizeof(*md))
 		return -EOPNOTSUPP;
 	vxh = (struct vxlanhdr *)((char *)udp + sizeof(struct udphdr));
 	*ip_proto = IPPROTO_UDP;
@@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
 	udp->dest = tun_key->tp_dst;
 	vxh->vx_flags = VXLAN_HF_VNI;
 	vxh->vx_vni = vxlan_vni_field(tun_id);
+	if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
+		md = ip_tunnel_info_opts((struct ip_tunnel_info *)e->tun_info);
+		vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
+				    (struct vxlan_metadata *)md);
+	}
+
+	return 0;
+}
+
+static int mlx5e_tc_tun_parse_vxlan_gbp_option(struct mlx5e_priv *priv,
+					       struct mlx5_flow_spec *spec,
+					       struct flow_cls_offload *f)
+{
+	struct flow_rule *rule = flow_cls_offload_flow_rule(f);
+	struct netlink_ext_ack *extack = f->common.extack;
+	struct flow_match_enc_opts enc_opts;
+	void *misc5_c, *misc5_v;
+	u32 *gbp, *gbp_mask;
+
+	flow_rule_match_enc_opts(rule, &enc_opts);
+
+	if (memchr_inv(&enc_opts.mask->data, 0, sizeof(enc_opts.mask->data)) &&
+	    !MLX5_CAP_ESW_FT_FIELD_SUPPORT_2(priv->mdev, tunnel_header_0_1)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Matching on VxLAN GBP is not supported");
+		netdev_warn(priv->netdev,
+			    "Matching on VxLAN GBP is not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (enc_opts.key->dst_opt_type != TUNNEL_VXLAN_OPT) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Wrong VxLAN option type: not GBP");
+		netdev_warn(priv->netdev,
+			    "Wrong VxLAN option type: not GBP\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (enc_opts.key->len != sizeof(*gbp) ||
+	    enc_opts.mask->len != sizeof(*gbp_mask)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "VxLAN GBP option/mask len is not 32 bits");
+		netdev_warn(priv->netdev,
+			    "VxLAN GBP option/mask len is not 32 bits\n");
+		return -EINVAL;
+	}
+
+	gbp = (u32 *)&enc_opts.key->data[0];
+	gbp_mask = (u32 *)&enc_opts.mask->data[0];
+
+	if (*gbp_mask & ~VXLAN_GBP_MASK) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Wrong VxLAN GBP mask");
+		netdev_warn(priv->netdev,
+			    "Wrong VxLAN GBP mask(0x%08X)\n", *gbp_mask);
+		return -EINVAL;
+	}
+
+	misc5_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters_5);
+	misc5_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters_5);
+	MLX5_SET(fte_match_set_misc5, misc5_c, tunnel_header_0, *gbp_mask);
+	MLX5_SET(fte_match_set_misc5, misc5_v, tunnel_header_0, *gbp);
+
+	spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS_5;
 
 	return 0;
 }
@@ -122,6 +189,14 @@ static int mlx5e_tc_tun_parse_vxlan(struct mlx5e_priv *priv,
 	if (!enc_keyid.mask->keyid)
 		return 0;
 
+	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_OPTS)) {
+		int err;
+
+		err = mlx5e_tc_tun_parse_vxlan_gbp_option(priv, spec, f);
+		if (err)
+			return err;
+	}
+
 	/* match on VNI is required */
 
 	if (!MLX5_CAP_ESW_FLOWTABLE_FDB(priv->mdev,
@@ -143,6 +218,12 @@ static int mlx5e_tc_tun_parse_vxlan(struct mlx5e_priv *priv,
 	return 0;
 }
 
+static bool mlx5e_tc_tun_encap_info_equal_vxlan(struct mlx5e_encap_key *a,
+						struct mlx5e_encap_key *b)
+{
+	return mlx5e_tc_tun_encap_info_equal_options(a, b, TUNNEL_VXLAN_OPT);
+}
+
 static int mlx5e_tc_tun_get_remote_ifindex(struct net_device *mirred_dev)
 {
 	const struct vxlan_dev *vxlan = netdev_priv(mirred_dev);
@@ -160,6 +241,6 @@ struct mlx5e_tc_tunnel vxlan_tunnel = {
 	.generate_ip_tun_hdr  = mlx5e_gen_ip_tunnel_header_vxlan,
 	.parse_udp_ports      = mlx5e_tc_tun_parse_udp_ports_vxlan,
 	.parse_tunnel         = mlx5e_tc_tun_parse_vxlan,
-	.encap_info_equal     = mlx5e_tc_tun_encap_info_equal_generic,
+	.encap_info_equal     = mlx5e_tc_tun_encap_info_equal_vxlan,
 	.get_remote_ifindex   = mlx5e_tc_tun_get_remote_ifindex,
 };
diff --git a/include/linux/mlx5/device.h b/include/linux/mlx5/device.h
index 71b06ebad402..af4dd536a52c 100644
--- a/include/linux/mlx5/device.h
+++ b/include/linux/mlx5/device.h
@@ -1357,6 +1357,12 @@ enum mlx5_qcam_feature_groups {
 #define MLX5_CAP_ESW_INGRESS_ACL_MAX(mdev, cap) \
 	MLX5_CAP_ESW_FLOWTABLE_MAX(mdev, flow_table_properties_esw_acl_ingress.cap)
 
+#define MLX5_CAP_ESW_FT_FIELD_SUPPORT_2(mdev, cap) \
+	MLX5_CAP_ESW_FLOWTABLE(mdev, ft_field_support_2_esw_fdb.cap)
+
+#define MLX5_CAP_ESW_FT_FIELD_SUPPORT_2_MAX(mdev, cap) \
+	MLX5_CAP_ESW_FLOWTABLE_MAX(mdev, ft_field_support_2_esw_fdb.cap)
+
 #define MLX5_CAP_ESW(mdev, cap) \
 	MLX5_GET(e_switch_cap, \
 		 mdev->caps.hca[MLX5_CAP_ESWITCH]->cur, cap)
diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index 1e530a8a2cf5..caef6aa20454 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -399,10 +399,13 @@ struct mlx5_ifc_flow_table_fields_supported_bits {
 	u8         metadata_reg_c_0[0x1];
 };
 
+/* Table 2170 - Flow Table Fields Supported 2 Format */
 struct mlx5_ifc_flow_table_fields_supported_2_bits {
 	u8         reserved_at_0[0xe];
 	u8         bth_opcode[0x1];
-	u8         reserved_at_f[0x11];
+	u8         reserved_at_f[0x1];
+	u8         tunnel_header_0_1[0x1];
+	u8         reserved_at_11[0xf];
 
 	u8         reserved_at_20[0x60];
 };
@@ -890,7 +893,13 @@ struct mlx5_ifc_flow_table_eswitch_cap_bits {
 
 	struct mlx5_ifc_flow_table_prop_layout_bits flow_table_properties_esw_acl_egress;
 
-	u8      reserved_at_800[0x1000];
+	u8      reserved_at_800[0xC00];
+
+	struct mlx5_ifc_flow_table_fields_supported_2_bits ft_field_support_2_esw_fdb;
+
+	struct mlx5_ifc_flow_table_fields_supported_2_bits ft_field_bitmask_support_2_esw_fdb;
+
+	u8      reserved_at_1500[0x300];
 
 	u8      sw_steering_fdb_action_drop_icm_address_rx[0x40];
 
-- 
2.31.1

Re: [PATCH net-next v1 3/3] net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload

[Alexander Lobakin]

From: Gavin Li <gavinl@nvidia.com>
Date: Tue, 14 Feb 2023 15:41:37 +0200

> Add HW offloading support for TC flows with VxLAN GBP encap/decap.
> 
> Example of encap rule:
> tc filter add dev eth0 protocol ip ingress flower \
>     action tunnel_key set id 42 vxlan_opts 512 \
>     action mirred egress redirect dev vxlan1
> 
> Example of decap rule:
> tc filter add dev vxlan1 protocol ip ingress flower \
>     enc_key_id 42 enc_dst_port 4789 vxlan_opts 1024 \
>     action tunnel_key unset action mirred egress redirect dev eth0
> 
> Change-Id: I48f61d02201bf3f79dcbe5d0f022f7bb27ed630f
> Signed-off-by: Gavin Li <gavinl@nvidia.com>
> Reviewed-by: Roi Dayan <roid@nvidia.com>
> Reviewed-by: Maor Dickman <maord@nvidia.com>
> Acked-by: Saeed Mahameed <saeedm@nvidia.com>
> ---
>  .../mellanox/mlx5/core/en/tc_tun_vxlan.c      | 85 ++++++++++++++++++-
>  include/linux/mlx5/device.h                   |  6 ++
>  include/linux/mlx5/mlx5_ifc.h                 | 13 ++-
>  3 files changed, 100 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
> index 1f62c702b625..444512ca9e0d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>  /* Copyright (c) 2018 Mellanox Technologies. */
>  
> +#include <net/ip_tunnels.h>
>  #include <net/vxlan.h>
>  #include "lib/vxlan.h"
>  #include "en/tc_tun.h"
> @@ -86,9 +87,11 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
>  	const struct ip_tunnel_key *tun_key = &e->tun_info->key;
>  	__be32 tun_id = tunnel_id_to_key32(tun_key->tun_id);
>  	struct udphdr *udp = (struct udphdr *)(buf);
> +	const struct vxlan_metadata *md;
>  	struct vxlanhdr *vxh;
>  
> -	if (tun_key->tun_flags & TUNNEL_VXLAN_OPT)
> +	if (tun_key->tun_flags & TUNNEL_VXLAN_OPT &&

A separate pair of braces is preferred around bitops.

> +	    e->tun_info->options_len != sizeof(*md))
>  		return -EOPNOTSUPP;
>  	vxh = (struct vxlanhdr *)((char *)udp + sizeof(struct udphdr));
>  	*ip_proto = IPPROTO_UDP;
> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
>  	udp->dest = tun_key->tp_dst;
>  	vxh->vx_flags = VXLAN_HF_VNI;
>  	vxh->vx_vni = vxlan_vni_field(tun_id);
> +	if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
> +		md = ip_tunnel_info_opts((struct ip_tunnel_info *)e->tun_info);
> +		vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
> +				    (struct vxlan_metadata *)md);

Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
arguments instead of working around by casting away?

> +	}
> +
> +	return 0;
> +}
> +
> +static int mlx5e_tc_tun_parse_vxlan_gbp_option(struct mlx5e_priv *priv,
> +					       struct mlx5_flow_spec *spec,
> +					       struct flow_cls_offload *f)
> +{
> +	struct flow_rule *rule = flow_cls_offload_flow_rule(f);
> +	struct netlink_ext_ack *extack = f->common.extack;
> +	struct flow_match_enc_opts enc_opts;
> +	void *misc5_c, *misc5_v;
> +	u32 *gbp, *gbp_mask;
> +
> +	flow_rule_match_enc_opts(rule, &enc_opts);
> +
> +	if (memchr_inv(&enc_opts.mask->data, 0, sizeof(enc_opts.mask->data)) &&
> +	    !MLX5_CAP_ESW_FT_FIELD_SUPPORT_2(priv->mdev, tunnel_header_0_1)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Matching on VxLAN GBP is not supported");
> +		netdev_warn(priv->netdev,
> +			    "Matching on VxLAN GBP is not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (enc_opts.key->dst_opt_type != TUNNEL_VXLAN_OPT) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Wrong VxLAN option type: not GBP");

Fits into one line I believe.

> +		netdev_warn(priv->netdev,
> +			    "Wrong VxLAN option type: not GBP\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (enc_opts.key->len != sizeof(*gbp) ||
> +	    enc_opts.mask->len != sizeof(*gbp_mask)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "VxLAN GBP option/mask len is not 32 bits");
> +		netdev_warn(priv->netdev,
> +			    "VxLAN GBP option/mask len is not 32 bits\n");
> +		return -EINVAL;
> +	}
> +
> +	gbp = (u32 *)&enc_opts.key->data[0];
> +	gbp_mask = (u32 *)&enc_opts.mask->data[0];
> +
> +	if (*gbp_mask & ~VXLAN_GBP_MASK) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Wrong VxLAN GBP mask");

You can use new NL_SET_ERR_MSG_FMT_MOD() here to print @gbp_mask to the
user, as you do it next line.

> +		netdev_warn(priv->netdev,
> +			    "Wrong VxLAN GBP mask(0x%08X)\n", *gbp_mask);
> +		return -EINVAL;
> +	}
> +
> +	misc5_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters_5);
> +	misc5_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters_5);
> +	MLX5_SET(fte_match_set_misc5, misc5_c, tunnel_header_0, *gbp_mask);
> +	MLX5_SET(fte_match_set_misc5, misc5_v, tunnel_header_0, *gbp);
> +
> +	spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS_5;
>  
>  	return 0;
>  }

Thanks,
Olek

Re: [PATCH net-next v1 3/3] net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload

[Gavin Li]

On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
> External email: Use caution opening links or attachments
>
>
> From: Gavin Li <gavinl@nvidia.com>
> Date: Tue, 14 Feb 2023 15:41:37 +0200
>
>> Add HW offloading support for TC flows with VxLAN GBP encap/decap.
>>
>> Example of encap rule:
>> tc filter add dev eth0 protocol ip ingress flower \
>>      action tunnel_key set id 42 vxlan_opts 512 \
>>      action mirred egress redirect dev vxlan1
>>
>> Example of decap rule:
>> tc filter add dev vxlan1 protocol ip ingress flower \
>>      enc_key_id 42 enc_dst_port 4789 vxlan_opts 1024 \
>>      action tunnel_key unset action mirred egress redirect dev eth0
>>
>> Change-Id: I48f61d02201bf3f79dcbe5d0f022f7bb27ed630f
>> Signed-off-by: Gavin Li <gavinl@nvidia.com>
>> Reviewed-by: Roi Dayan <roid@nvidia.com>
>> Reviewed-by: Maor Dickman <maord@nvidia.com>
>> Acked-by: Saeed Mahameed <saeedm@nvidia.com>
>> ---
>>   .../mellanox/mlx5/core/en/tc_tun_vxlan.c      | 85 ++++++++++++++++++-
>>   include/linux/mlx5/device.h                   |  6 ++
>>   include/linux/mlx5/mlx5_ifc.h                 | 13 ++-
>>   3 files changed, 100 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>> index 1f62c702b625..444512ca9e0d 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>> @@ -1,6 +1,7 @@
>>   // SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>>   /* Copyright (c) 2018 Mellanox Technologies. */
>>
>> +#include <net/ip_tunnels.h>
>>   #include <net/vxlan.h>
>>   #include "lib/vxlan.h"
>>   #include "en/tc_tun.h"
>> @@ -86,9 +87,11 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
>>        const struct ip_tunnel_key *tun_key = &e->tun_info->key;
>>        __be32 tun_id = tunnel_id_to_key32(tun_key->tun_id);
>>        struct udphdr *udp = (struct udphdr *)(buf);
>> +     const struct vxlan_metadata *md;
>>        struct vxlanhdr *vxh;
>>
>> -     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT)
>> +     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT &&
> A separate pair of braces is preferred around bitops.
ACK
>
>> +         e->tun_info->options_len != sizeof(*md))
>>                return -EOPNOTSUPP;
>>        vxh = (struct vxlanhdr *)((char *)udp + sizeof(struct udphdr));
>>        *ip_proto = IPPROTO_UDP;
>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
>>        udp->dest = tun_key->tp_dst;
>>        vxh->vx_flags = VXLAN_HF_VNI;
>>        vxh->vx_vni = vxlan_vni_field(tun_id);
>> +     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>> +             md = ip_tunnel_info_opts((struct ip_tunnel_info *)e->tun_info);
>> +             vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>> +                                 (struct vxlan_metadata *)md);
> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
> arguments instead of working around by casting away?
The reason to cast it is a WA to use ip_tunnel_info_opts which takes 
non-const arg while

e->tun_info here is a const one.

>
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int mlx5e_tc_tun_parse_vxlan_gbp_option(struct mlx5e_priv *priv,
>> +                                            struct mlx5_flow_spec *spec,
>> +                                            struct flow_cls_offload *f)
>> +{
>> +     struct flow_rule *rule = flow_cls_offload_flow_rule(f);
>> +     struct netlink_ext_ack *extack = f->common.extack;
>> +     struct flow_match_enc_opts enc_opts;
>> +     void *misc5_c, *misc5_v;
>> +     u32 *gbp, *gbp_mask;
>> +
>> +     flow_rule_match_enc_opts(rule, &enc_opts);
>> +
>> +     if (memchr_inv(&enc_opts.mask->data, 0, sizeof(enc_opts.mask->data)) &&
>> +         !MLX5_CAP_ESW_FT_FIELD_SUPPORT_2(priv->mdev, tunnel_header_0_1)) {
>> +             NL_SET_ERR_MSG_MOD(extack,
>> +                                "Matching on VxLAN GBP is not supported");
>> +             netdev_warn(priv->netdev,
>> +                         "Matching on VxLAN GBP is not supported\n");
>> +             return -EOPNOTSUPP;
>> +     }
>> +
>> +     if (enc_opts.key->dst_opt_type != TUNNEL_VXLAN_OPT) {
>> +             NL_SET_ERR_MSG_MOD(extack,
>> +                                "Wrong VxLAN option type: not GBP");
> Fits into one line I believe.
ACK
>
>> +             netdev_warn(priv->netdev,
>> +                         "Wrong VxLAN option type: not GBP\n");
>> +             return -EOPNOTSUPP;
>> +     }
>> +
>> +     if (enc_opts.key->len != sizeof(*gbp) ||
>> +         enc_opts.mask->len != sizeof(*gbp_mask)) {
>> +             NL_SET_ERR_MSG_MOD(extack,
>> +                                "VxLAN GBP option/mask len is not 32 bits");
>> +             netdev_warn(priv->netdev,
>> +                         "VxLAN GBP option/mask len is not 32 bits\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     gbp = (u32 *)&enc_opts.key->data[0];
>> +     gbp_mask = (u32 *)&enc_opts.mask->data[0];
>> +
>> +     if (*gbp_mask & ~VXLAN_GBP_MASK) {
>> +             NL_SET_ERR_MSG_MOD(extack,
>> +                                "Wrong VxLAN GBP mask");
> You can use new NL_SET_ERR_MSG_FMT_MOD() here to print @gbp_mask to the
> user, as you do it next line.
ACK
>
>> +             netdev_warn(priv->netdev,
>> +                         "Wrong VxLAN GBP mask(0x%08X)\n", *gbp_mask);
>> +             return -EINVAL;
>> +     }
>> +
>> +     misc5_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters_5);
>> +     misc5_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters_5);
>> +     MLX5_SET(fte_match_set_misc5, misc5_c, tunnel_header_0, *gbp_mask);
>> +     MLX5_SET(fte_match_set_misc5, misc5_v, tunnel_header_0, *gbp);
>> +
>> +     spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS_5;
>>
>>        return 0;
>>   }
> Thanks,
> Olek

Re: [PATCH net-next v1 3/3] net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload

[Alexander Lobakin]

From: Gavin Li <gavinl@nvidia.com>
Date: Wed, 15 Feb 2023 10:50:04 +0800

> 
> On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> From: Gavin Li <gavinl@nvidia.com>
>> Date: Tue, 14 Feb 2023 15:41:37 +0200
>>
>>> Add HW offloading support for TC flows with VxLAN GBP encap/decap.

[...]

>>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char
>>> buf[],
>>>        udp->dest = tun_key->tp_dst;
>>>        vxh->vx_flags = VXLAN_HF_VNI;
>>>        vxh->vx_vni = vxlan_vni_field(tun_id);
>>> +     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>>> +             md = ip_tunnel_info_opts((struct ip_tunnel_info
>>> *)e->tun_info);
>>> +             vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>>> +                                 (struct vxlan_metadata *)md);
>> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
>> arguments instead of working around by casting away?
> The reason to cast it is a WA to use ip_tunnel_info_opts which takes
> non-const arg while>
> e->tun_info here is a const one.

That's what I'm asking - why not make ip_tunnel_info_opts() and
vxlan_build_gbp_hdr() take const tun_info?

> 
>>
>>> +     }
>>> +
>>> +     return 0;
>>> +}
[...]

Thanks,
Olek

Re: [PATCH net-next v1 3/3] net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload

[Gavin Li]

On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
> External email: Use caution opening links or attachments
>
>
> From: Gavin Li <gavinl@nvidia.com>
> Date: Tue, 14 Feb 2023 15:41:37 +0200
>
>> Add HW offloading support for TC flows with VxLAN GBP encap/decap.
>>
>> Example of encap rule:
>> tc filter add dev eth0 protocol ip ingress flower \
>>      action tunnel_key set id 42 vxlan_opts 512 \
>>      action mirred egress redirect dev vxlan1
>>
>> Example of decap rule:
>> tc filter add dev vxlan1 protocol ip ingress flower \
>>      enc_key_id 42 enc_dst_port 4789 vxlan_opts 1024 \
>>      action tunnel_key unset action mirred egress redirect dev eth0
>>
>> Change-Id: I48f61d02201bf3f79dcbe5d0f022f7bb27ed630f
>> Signed-off-by: Gavin Li <gavinl@nvidia.com>
>> Reviewed-by: Roi Dayan <roid@nvidia.com>
>> Reviewed-by: Maor Dickman <maord@nvidia.com>
>> Acked-by: Saeed Mahameed <saeedm@nvidia.com>
>> ---
>>   .../mellanox/mlx5/core/en/tc_tun_vxlan.c      | 85 ++++++++++++++++++-
>>   include/linux/mlx5/device.h                   |  6 ++
>>   include/linux/mlx5/mlx5_ifc.h                 | 13 ++-
>>   3 files changed, 100 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>> index 1f62c702b625..444512ca9e0d 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>> @@ -1,6 +1,7 @@
>>   // SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>>   /* Copyright (c) 2018 Mellanox Technologies. */
>>
>> +#include <net/ip_tunnels.h>
>>   #include <net/vxlan.h>
>>   #include "lib/vxlan.h"
>>   #include "en/tc_tun.h"
>> @@ -86,9 +87,11 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
>>        const struct ip_tunnel_key *tun_key = &e->tun_info->key;
>>        __be32 tun_id = tunnel_id_to_key32(tun_key->tun_id);
>>        struct udphdr *udp = (struct udphdr *)(buf);
>> +     const struct vxlan_metadata *md;
>>        struct vxlanhdr *vxh;
>>
>> -     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT)
>> +     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT &&
> A separate pair of braces is preferred around bitops.
>
>> +         e->tun_info->options_len != sizeof(*md))
>>                return -EOPNOTSUPP;
>>        vxh = (struct vxlanhdr *)((char *)udp + sizeof(struct udphdr));
>>        *ip_proto = IPPROTO_UDP;
>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
>>        udp->dest = tun_key->tp_dst;
>>        vxh->vx_flags = VXLAN_HF_VNI;
>>        vxh->vx_vni = vxlan_vni_field(tun_id);
>> +     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>> +             md = ip_tunnel_info_opts((struct ip_tunnel_info *)e->tun_info);
>> +             vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>> +                                 (struct vxlan_metadata *)md);
> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
> arguments instead of working around by casting away?
ACK. Sorry for the confusion---I misunderstood the comment.
>
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int mlx5e_tc_tun_parse_vxlan_gbp_option(struct mlx5e_priv *priv,
>> +                                            struct mlx5_flow_spec *spec,
>> +                                            struct flow_cls_offload *f)
>> +{
>> +     struct flow_rule *rule = flow_cls_offload_flow_rule(f);
>> +     struct netlink_ext_ack *extack = f->common.extack;
>> +     struct flow_match_enc_opts enc_opts;
>> +     void *misc5_c, *misc5_v;
>> +     u32 *gbp, *gbp_mask;
>> +
>> +     flow_rule_match_enc_opts(rule, &enc_opts);
>> +
>> +     if (memchr_inv(&enc_opts.mask->data, 0, sizeof(enc_opts.mask->data)) &&
>> +         !MLX5_CAP_ESW_FT_FIELD_SUPPORT_2(priv->mdev, tunnel_header_0_1)) {
>> +             NL_SET_ERR_MSG_MOD(extack,
>> +                                "Matching on VxLAN GBP is not supported");
>> +             netdev_warn(priv->netdev,
>> +                         "Matching on VxLAN GBP is not supported\n");
>> +             return -EOPNOTSUPP;
>> +     }
>> +
>> +     if (enc_opts.key->dst_opt_type != TUNNEL_VXLAN_OPT) {
>> +             NL_SET_ERR_MSG_MOD(extack,
>> +                                "Wrong VxLAN option type: not GBP");
> Fits into one line I believe.
>
>> +             netdev_warn(priv->netdev,
>> +                         "Wrong VxLAN option type: not GBP\n");
>> +             return -EOPNOTSUPP;
>> +     }
>> +
>> +     if (enc_opts.key->len != sizeof(*gbp) ||
>> +         enc_opts.mask->len != sizeof(*gbp_mask)) {
>> +             NL_SET_ERR_MSG_MOD(extack,
>> +                                "VxLAN GBP option/mask len is not 32 bits");
>> +             netdev_warn(priv->netdev,
>> +                         "VxLAN GBP option/mask len is not 32 bits\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     gbp = (u32 *)&enc_opts.key->data[0];
>> +     gbp_mask = (u32 *)&enc_opts.mask->data[0];
>> +
>> +     if (*gbp_mask & ~VXLAN_GBP_MASK) {
>> +             NL_SET_ERR_MSG_MOD(extack,
>> +                                "Wrong VxLAN GBP mask");
> You can use new NL_SET_ERR_MSG_FMT_MOD() here to print @gbp_mask to the
> user, as you do it next line.
>
>> +             netdev_warn(priv->netdev,
>> +                         "Wrong VxLAN GBP mask(0x%08X)\n", *gbp_mask);
>> +             return -EINVAL;
>> +     }
>> +
>> +     misc5_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters_5);
>> +     misc5_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters_5);
>> +     MLX5_SET(fte_match_set_misc5, misc5_c, tunnel_header_0, *gbp_mask);
>> +     MLX5_SET(fte_match_set_misc5, misc5_v, tunnel_header_0, *gbp);
>> +
>> +     spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS_5;
>>
>>        return 0;
>>   }
> Thanks,
> Olek

Re: [PATCH net-next v1 3/3] net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload

[Gavin Li]

On 2/15/2023 11:36 AM, Gavin Li wrote:
> External email: Use caution opening links or attachments
>
>
> On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> From: Gavin Li <gavinl@nvidia.com>
>> Date: Tue, 14 Feb 2023 15:41:37 +0200
>>
>>> Add HW offloading support for TC flows with VxLAN GBP encap/decap.
>>>
>>> Example of encap rule:
>>> tc filter add dev eth0 protocol ip ingress flower \
>>>      action tunnel_key set id 42 vxlan_opts 512 \
>>>      action mirred egress redirect dev vxlan1
>>>
>>> Example of decap rule:
>>> tc filter add dev vxlan1 protocol ip ingress flower \
>>>      enc_key_id 42 enc_dst_port 4789 vxlan_opts 1024 \
>>>      action tunnel_key unset action mirred egress redirect dev eth0
>>>
>>> Change-Id: I48f61d02201bf3f79dcbe5d0f022f7bb27ed630f
>>> Signed-off-by: Gavin Li <gavinl@nvidia.com>
>>> Reviewed-by: Roi Dayan <roid@nvidia.com>
>>> Reviewed-by: Maor Dickman <maord@nvidia.com>
>>> Acked-by: Saeed Mahameed <saeedm@nvidia.com>
>>> ---
>>>   .../mellanox/mlx5/core/en/tc_tun_vxlan.c      | 85 
>>> ++++++++++++++++++-
>>>   include/linux/mlx5/device.h                   |  6 ++
>>>   include/linux/mlx5/mlx5_ifc.h                 | 13 ++-
>>>   3 files changed, 100 insertions(+), 4 deletions(-)
>>>
>>> diff --git 
>>> a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c 
>>> b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>>> index 1f62c702b625..444512ca9e0d 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>>> @@ -1,6 +1,7 @@
>>>   // SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>>>   /* Copyright (c) 2018 Mellanox Technologies. */
>>>
>>> +#include <net/ip_tunnels.h>
>>>   #include <net/vxlan.h>
>>>   #include "lib/vxlan.h"
>>>   #include "en/tc_tun.h"
>>> @@ -86,9 +87,11 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char 
>>> buf[],
>>>        const struct ip_tunnel_key *tun_key = &e->tun_info->key;
>>>        __be32 tun_id = tunnel_id_to_key32(tun_key->tun_id);
>>>        struct udphdr *udp = (struct udphdr *)(buf);
>>> +     const struct vxlan_metadata *md;
>>>        struct vxlanhdr *vxh;
>>>
>>> -     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT)
>>> +     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT &&
>> A separate pair of braces is preferred around bitops.
ACK
>>
>>> +         e->tun_info->options_len != sizeof(*md))
>>>                return -EOPNOTSUPP;
>>>        vxh = (struct vxlanhdr *)((char *)udp + sizeof(struct udphdr));
>>>        *ip_proto = IPPROTO_UDP;
>>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char 
>>> buf[],
>>>        udp->dest = tun_key->tp_dst;
>>>        vxh->vx_flags = VXLAN_HF_VNI;
>>>        vxh->vx_vni = vxlan_vni_field(tun_id);
>>> +     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>>> +             md = ip_tunnel_info_opts((struct ip_tunnel_info 
>>> *)e->tun_info);
>>> +             vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>>> +                                 (struct vxlan_metadata *)md);
>> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
>> arguments instead of working around by casting away?
> ACK. Sorry for the confusion---I misunderstood the comment.
This ip_tunnel_info_opts is tricky to use const to annotate the arg 
because it will have to cast from const to non-const again upon returning.
>>
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int mlx5e_tc_tun_parse_vxlan_gbp_option(struct mlx5e_priv 
>>> *priv,
>>> +                                            struct mlx5_flow_spec 
>>> *spec,
>>> +                                            struct flow_cls_offload 
>>> *f)
>>> +{
>>> +     struct flow_rule *rule = flow_cls_offload_flow_rule(f);
>>> +     struct netlink_ext_ack *extack = f->common.extack;
>>> +     struct flow_match_enc_opts enc_opts;
>>> +     void *misc5_c, *misc5_v;
>>> +     u32 *gbp, *gbp_mask;
>>> +
>>> +     flow_rule_match_enc_opts(rule, &enc_opts);
>>> +
>>> +     if (memchr_inv(&enc_opts.mask->data, 0, 
>>> sizeof(enc_opts.mask->data)) &&
>>> +         !MLX5_CAP_ESW_FT_FIELD_SUPPORT_2(priv->mdev, 
>>> tunnel_header_0_1)) {
>>> +             NL_SET_ERR_MSG_MOD(extack,
>>> +                                "Matching on VxLAN GBP is not 
>>> supported");
>>> +             netdev_warn(priv->netdev,
>>> +                         "Matching on VxLAN GBP is not supported\n");
>>> +             return -EOPNOTSUPP;
>>> +     }
>>> +
>>> +     if (enc_opts.key->dst_opt_type != TUNNEL_VXLAN_OPT) {
>>> +             NL_SET_ERR_MSG_MOD(extack,
>>> +                                "Wrong VxLAN option type: not GBP");
>> Fits into one line I believe.
ACK
>>
>>> + netdev_warn(priv->netdev,
>>> +                         "Wrong VxLAN option type: not GBP\n");
>>> +             return -EOPNOTSUPP;
>>> +     }
>>> +
>>> +     if (enc_opts.key->len != sizeof(*gbp) ||
>>> +         enc_opts.mask->len != sizeof(*gbp_mask)) {
>>> +             NL_SET_ERR_MSG_MOD(extack,
>>> +                                "VxLAN GBP option/mask len is not 
>>> 32 bits");
>>> +             netdev_warn(priv->netdev,
>>> +                         "VxLAN GBP option/mask len is not 32 
>>> bits\n");
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     gbp = (u32 *)&enc_opts.key->data[0];
>>> +     gbp_mask = (u32 *)&enc_opts.mask->data[0];
>>> +
>>> +     if (*gbp_mask & ~VXLAN_GBP_MASK) {
>>> +             NL_SET_ERR_MSG_MOD(extack,
>>> +                                "Wrong VxLAN GBP mask");
>> You can use new NL_SET_ERR_MSG_FMT_MOD() here to print @gbp_mask to the
>> user, as you do it next line.
ACK
>>
>>> + netdev_warn(priv->netdev,
>>> +                         "Wrong VxLAN GBP mask(0x%08X)\n", *gbp_mask);
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     misc5_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, 
>>> misc_parameters_5);
>>> +     misc5_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, 
>>> misc_parameters_5);
>>> +     MLX5_SET(fte_match_set_misc5, misc5_c, tunnel_header_0, 
>>> *gbp_mask);
>>> +     MLX5_SET(fte_match_set_misc5, misc5_v, tunnel_header_0, *gbp);
>>> +
>>> +     spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS_5;
>>>
>>>        return 0;
>>>   }
>> Thanks,
>> Olek

Re: [PATCH net-next v1 3/3] net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload

[Alexander Lobakin]

From: Gavin Li <gavinl@nvidia.com>
Date: Wed, 15 Feb 2023 16:30:04 +0800

> 
> On 2/15/2023 11:36 AM, Gavin Li wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> From: Gavin Li <gavinl@nvidia.com>
>>> Date: Tue, 14 Feb 2023 15:41:37 +0200

[...]

>>>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char
>>>> buf[],
>>>>        udp->dest = tun_key->tp_dst;
>>>>        vxh->vx_flags = VXLAN_HF_VNI;
>>>>        vxh->vx_vni = vxlan_vni_field(tun_id);
>>>> +     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>>>> +             md = ip_tunnel_info_opts((struct ip_tunnel_info
>>>> *)e->tun_info);
>>>> +             vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>>>> +                                 (struct vxlan_metadata *)md);
>>> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
>>> arguments instead of working around by casting away?
>> ACK. Sorry for the confusion---I misunderstood the comment.
> This ip_tunnel_info_opts is tricky to use const to annotate the arg
> because it will have to cast from const to non-const again upon returning.

It's okay to cast away for the `void *` returned.
Alternatively, use can convert it to a macro and use
__builtin_choose_expr() or _Generic to return const or non-const
depending on whether the argument is constant. That's what was recently
done for container_of() IIRC.

>>>
>>>> +     }
>>>> +
>>>> +     return 0;
>>>> +}
[...]

Thanks,
Olek

Re: [PATCH net-next v1 3/3] net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload

[Gavin Li]

On 2/16/2023 1:01 AM, Alexander Lobakin wrote:
> External email: Use caution opening links or attachments
>
>
> From: Gavin Li <gavinl@nvidia.com>
> Date: Wed, 15 Feb 2023 16:30:04 +0800
>
>> On 2/15/2023 11:36 AM, Gavin Li wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> From: Gavin Li <gavinl@nvidia.com>
>>>> Date: Tue, 14 Feb 2023 15:41:37 +0200
> [...]
>
>>>>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char
>>>>> buf[],
>>>>>         udp->dest = tun_key->tp_dst;
>>>>>         vxh->vx_flags = VXLAN_HF_VNI;
>>>>>         vxh->vx_vni = vxlan_vni_field(tun_id);
>>>>> +     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>>>>> +             md = ip_tunnel_info_opts((struct ip_tunnel_info
>>>>> *)e->tun_info);
>>>>> +             vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>>>>> +                                 (struct vxlan_metadata *)md);
>>>> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
>>>> arguments instead of working around by casting away?
>>> ACK. Sorry for the confusion---I misunderstood the comment.
>> This ip_tunnel_info_opts is tricky to use const to annotate the arg
>> because it will have to cast from const to non-const again upon returning.
> It's okay to cast away for the `void *` returned.
> Alternatively, use can convert it to a macro and use
> __builtin_choose_expr() or _Generic to return const or non-const
> depending on whether the argument is constant. That's what was recently
> done for container_of() IIRC.

I've fixed vxlan_build_gbp_hdr in V2. For ip_tunnel_info_opts, it's 
confusing to me.

It would be as below after constifying the parameter.

static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
{
     return (void *)(info + 1);
}
Is there any value gained by this change?

>
>>>>> +     }
>>>>> +
>>>>> +     return 0;
>>>>> +}
> [...]
>
> Thanks,
> Olek

Re: [PATCH net-next v1 3/3] net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload

[Alexander Lobakin]

From: Gavin Li <gavinl@nvidia.com>
Date: Thu, 16 Feb 2023 16:40:33 +0800

> 
> On 2/16/2023 1:01 AM, Alexander Lobakin wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> From: Gavin Li <gavinl@nvidia.com>
>> Date: Wed, 15 Feb 2023 16:30:04 +0800
>>
>>> On 2/15/2023 11:36 AM, Gavin Li wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> From: Gavin Li <gavinl@nvidia.com>
>>>>> Date: Tue, 14 Feb 2023 15:41:37 +0200
>> [...]
>>
>>>>>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char
>>>>>> buf[],
>>>>>>         udp->dest = tun_key->tp_dst;
>>>>>>         vxh->vx_flags = VXLAN_HF_VNI;
>>>>>>         vxh->vx_vni = vxlan_vni_field(tun_id);
>>>>>> +     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>>>>>> +             md = ip_tunnel_info_opts((struct ip_tunnel_info
>>>>>> *)e->tun_info);
>>>>>> +             vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>>>>>> +                                 (struct vxlan_metadata *)md);
>>>>> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
>>>>> arguments instead of working around by casting away?
>>>> ACK. Sorry for the confusion---I misunderstood the comment.
>>> This ip_tunnel_info_opts is tricky to use const to annotate the arg
>>> because it will have to cast from const to non-const again upon
>>> returning.
>> It's okay to cast away for the `void *` returned.
>> Alternatively, use can convert it to a macro and use
>> __builtin_choose_expr() or _Generic to return const or non-const
>> depending on whether the argument is constant. That's what was recently
>> done for container_of() IIRC.
> 
> I've fixed vxlan_build_gbp_hdr in V2. For ip_tunnel_info_opts, it's
> confusing to me.
> 
> It would be as below after constifying the parameter.
> 
> static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
> {
>     return (void *)(info + 1);
> }
> Is there any value gained by this change?

You wouldn't need to W/A it each time in each driver, just do it once in
the inline itself.
I did it once in __skb_header_pointer()[0] to be able to pass data
pointer as const to optimize code a bit and point out explicitly that
the function doesn't modify the packet anyhow, don't see any reason to
not do the same here.
Or, as I said, you can use macros + __builtin_choose_expr() or _Generic.
container_of_const() uses the latter[1]. A __builtin_choose_expr()
variant could rely on the __same_type() macro to check whether the
pointer passed from the driver const or not.

> 
>>
>>>>>> +     }
>>>>>> +
>>>>>> +     return 0;
>>>>>> +}
>> [...]
>>
>> Thanks,
>> Olek

[0]
https://elixir.bootlin.com/linux/v6.2-rc8/source/include/linux/skbuff.h#L3992
[1]
https://elixir.bootlin.com/linux/v6.2-rc8/source/include/linux/container_of.h#L33

Thanks,
Olek

Re: [PATCH net-next v1 3/3] net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload

[Gavin Li]

On 2/16/2023 11:19 PM, Alexander Lobakin wrote:
> External email: Use caution opening links or attachments
>
>
> From: Gavin Li <gavinl@nvidia.com>
> Date: Thu, 16 Feb 2023 16:40:33 +0800
>
>> On 2/16/2023 1:01 AM, Alexander Lobakin wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> From: Gavin Li <gavinl@nvidia.com>
>>> Date: Wed, 15 Feb 2023 16:30:04 +0800
>>>
>>>> On 2/15/2023 11:36 AM, Gavin Li wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> From: Gavin Li <gavinl@nvidia.com>
>>>>>> Date: Tue, 14 Feb 2023 15:41:37 +0200
>>> [...]
>>>
>>>>>>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char
>>>>>>> buf[],
>>>>>>>          udp->dest = tun_key->tp_dst;
>>>>>>>          vxh->vx_flags = VXLAN_HF_VNI;
>>>>>>>          vxh->vx_vni = vxlan_vni_field(tun_id);
>>>>>>> +     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>>>>>>> +             md = ip_tunnel_info_opts((struct ip_tunnel_info
>>>>>>> *)e->tun_info);
>>>>>>> +             vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>>>>>>> +                                 (struct vxlan_metadata *)md);
>>>>>> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
>>>>>> arguments instead of working around by casting away?
>>>>> ACK. Sorry for the confusion---I misunderstood the comment.
>>>> This ip_tunnel_info_opts is tricky to use const to annotate the arg
>>>> because it will have to cast from const to non-const again upon
>>>> returning.
>>> It's okay to cast away for the `void *` returned.
>>> Alternatively, use can convert it to a macro and use
>>> __builtin_choose_expr() or _Generic to return const or non-const
>>> depending on whether the argument is constant. That's what was recently
>>> done for container_of() IIRC.
>> I've fixed vxlan_build_gbp_hdr in V2. For ip_tunnel_info_opts, it's
>> confusing to me.
>>
>> It would be as below after constifying the parameter.
>>
>> static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
>> {
>>      return (void *)(info + 1);
>> }
>> Is there any value gained by this change?
> You wouldn't need to W/A it each time in each driver, just do it once in
> the inline itself.
> I did it once in __skb_header_pointer()[0] to be able to pass data
> pointer as const to optimize code a bit and point out explicitly that
> the function doesn't modify the packet anyhow, don't see any reason to
> not do the same here.
> Or, as I said, you can use macros + __builtin_choose_expr() or _Generic.
> container_of_const() uses the latter[1]. A __builtin_choose_expr()
> variant could rely on the __same_type() macro to check whether the
> pointer passed from the driver const or not.
ACK
>
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     return 0;
>>>>>>> +}
>>> [...]
>>>
>>> Thanks,
>>> Olek
> [0]
> https://elixir.bootlin.com/linux/v6.2-rc8/source/include/linux/skbuff.h#L3992
> [1]
> https://elixir.bootlin.com/linux/v6.2-rc8/source/include/linux/container_of.h#L33
>
> Thanks,
> Olek

Re: [PATCH net-next v1 3/3] net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload

[Alexander Lobakin]

From: Gavin Li <gavinl@nvidia.com>
Date: Wed, 15 Feb 2023 11:36:24 +0800

> 
> On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> From: Gavin Li <gavinl@nvidia.com>
>> Date: Tue, 14 Feb 2023 15:41:37 +0200

[...]

>>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char
>>> buf[],
>>>        udp->dest = tun_key->tp_dst;
>>>        vxh->vx_flags = VXLAN_HF_VNI;
>>>        vxh->vx_vni = vxlan_vni_field(tun_id);
>>> +     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>>> +             md = ip_tunnel_info_opts((struct ip_tunnel_info
>>> *)e->tun_info);
>>> +             vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>>> +                                 (struct vxlan_metadata *)md);
>> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
>> arguments instead of working around by casting away?
> ACK. Sorry for the confusion---I misunderstood the comment.

Ah, no worries :D Sorry that I sent the prev mail and only then opened
this one.

>>
>>> +     }
>>> +
>>> +     return 0;
>>> +}
Thanks,
Olek