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

[PATCH net-next v3 0/5] 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: Remove unused argument from functions.
Patch-2: Expose helper function vxlan_build_gbp_hdr.
Patch-3: Add helper function for encap_info_equal for tunnels with options.
Patch-4: constify input argument of ip_tunnel_info_opts.
Patch-5: Add HW offloading support for TC flows with VxLAN GBP encap/decap
        in mlx ethernet driver.

Gavin Li (5):
  vxlan: Remove unused argument from vxlan_build_gbp_hdr( ) and
    vxlan_build_gpe_hdr( )
---
changelog:
v2->v3
- Addressed comments from Paolo Abeni
- Add new patch
---
  vxlan: Expose helper vxlan_build_gbp_hdr
---
changelog:
v1->v2
- Addressed comments from Alexander Lobakin
- Use const to annotate read-only the pointer parameter
---
  net/mlx5e: Add helper for encap_info_equal for tunnels with options
changelog:
v1->v2
- Addressed comments from Alexander Lobakin
- Replace confusing pointer arithmetic with function call
- Use boolean operator NOT to check if the function return value is not zero
---
  ip_tunnel: constify input argument of ip_tunnel_info_opts( )
---
changelog:
v2->v3
- Addressed comments from Alexander Lobakin
- Add new patch
---
  net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload
---
changelog:
v2->v3
- Addressed comments from Alexander Lobakin
- Remove the WA by casting away
v1->v2
- Addressed comments from Alexander Lobakin
- Add a separate pair of braces around bitops
- Remove the WA by casting away
- Fit all log messages into one line
- Use NL_SET_ERR_MSG_FMT_MOD to print the invalid value on error
---

Gavin Li (5):
  vxlan: Remove unused argument from vxlan_build_gbp_hdr( ) and
    vxlan_build_gpe_hdr( )
  vxlan: Expose helper vxlan_build_gbp_hdr
  net/mlx5e: Add helper for encap_info_equal for tunnels with options
  ip_tunnel: constify input argument of ip_tunnel_info_opts( )
  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      | 32 ++++++++
 .../mellanox/mlx5/core/en/tc_tun_geneve.c     | 24 +-----
 .../mellanox/mlx5/core/en/tc_tun_vxlan.c      | 76 ++++++++++++++++++-
 drivers/net/vxlan/vxlan_core.c                | 27 +------
 include/linux/mlx5/device.h                   |  6 ++
 include/linux/mlx5/mlx5_ifc.h                 | 13 +++-
 include/net/ip_tunnels.h                      |  4 +-
 include/net/vxlan.h                           | 19 +++++
 9 files changed, 151 insertions(+), 53 deletions(-)

-- 
2.31.1

[PATCH net-next v3 1/5] vxlan: Remove unused argument from vxlan_build_gbp_hdr( ) and vxlan_build_gpe_hdr( )

[Gavin Li]

Remove unused argument (i.e. u32 vxflags) in vxlan_build_gbp_hdr( ) and
vxlan_build_gpe_hdr( ) function arguments.

Signed-off-by: Gavin Li <gavinl@nvidia.com>
---
 drivers/net/vxlan/vxlan_core.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index b1b179effe2a..86967277ab97 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -2140,8 +2140,7 @@ 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)
+static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, struct vxlan_metadata *md)
 {
 	struct vxlanhdr_gbp *gbp;
 
@@ -2160,8 +2159,7 @@ static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, u32 vxflags,
 	gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
 }
 
-static int vxlan_build_gpe_hdr(struct vxlanhdr *vxh, u32 vxflags,
-			       __be16 protocol)
+static int vxlan_build_gpe_hdr(struct vxlanhdr *vxh, __be16 protocol)
 {
 	struct vxlanhdr_gpe *gpe = (struct vxlanhdr_gpe *)vxh;
 
@@ -2224,9 +2222,9 @@ static int vxlan_build_skb(struct sk_buff *skb, struct dst_entry *dst,
 	}
 
 	if (vxflags & VXLAN_F_GBP)
-		vxlan_build_gbp_hdr(vxh, vxflags, md);
+		vxlan_build_gbp_hdr(vxh, md);
 	if (vxflags & VXLAN_F_GPE) {
-		err = vxlan_build_gpe_hdr(vxh, vxflags, skb->protocol);
+		err = vxlan_build_gpe_hdr(vxh, skb->protocol);
 		if (err < 0)
 			return err;
 		inner_protocol = skb->protocol;
-- 
2.31.1

[PATCH net-next v3 2/5] 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.

Signed-off-by: Gavin Li <gavinl@nvidia.com>
Reviewed-by: Gavi Teitz <gavi@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 | 19 -------------------
 include/net/vxlan.h            | 19 +++++++++++++++++++
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 86967277ab97..13faab36b3e1 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -2140,25 +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, 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, __be16 protocol)
 {
 	struct vxlanhdr_gpe *gpe = (struct vxlanhdr_gpe *)vxh;
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index bca5b01af247..b6d419fa7ab1 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -566,4 +566,23 @@ static inline bool vxlan_fdb_nh_path_select(struct nexthop *nh,
 	return true;
 }
 
+static inline void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const 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

[PATCH net-next v3 3/5] 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.

Signed-off-by: Gavin Li <gavinl@nvidia.com>
Reviewed-by: Gavi Teitz <gavi@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      | 32 +++++++++++++++++++
 .../mellanox/mlx5/core/en/tc_tun_geneve.c     | 24 +-------------
 3 files changed, 36 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..f1d132f16fcc 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
@@ -3,6 +3,7 @@
 
 #include <net/fib_notifier.h>
 #include <net/nexthop.h>
+#include <net/ip_tunnels.h>
 #include "tc_tun_encap.h"
 #include "en_tc.h"
 #include "tc_tun.h"
@@ -571,6 +572,37 @@ 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(ip_tunnel_info_opts(a_info),
+			ip_tunnel_info_opts(b_info),
+			a_info->options_len);
+}
+
 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

[PATCH net-next v3 4/5] ip_tunnel: constify input argument of ip_tunnel_info_opts( )

[Gavin Li]

Constify input argument(i.e. struct ip_tunnel_info *info) of
ip_tunnel_info_opts( ) so that it wouldn't be needed to W/A it each time
in each driver.

Signed-off-by: Gavin Li <gavinl@nvidia.com>
---
 include/net/ip_tunnels.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index fca357679816..32c77f149c6e 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -485,9 +485,9 @@ static inline void iptunnel_xmit_stats(struct net_device *dev, int pkt_len)
 	}
 }
 
-static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
+static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
 {
-	return info + 1;
+	return (void *)(info + 1);
 }
 
 static inline void ip_tunnel_info_opts_get(void *to,
-- 
2.31.1

[PATCH net-next v3 5/5] 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

Signed-off-by: Gavin Li <gavinl@nvidia.com>
Reviewed-by: Gavi Teitz <gavi@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      | 76 ++++++++++++++++++-
 include/linux/mlx5/device.h                   |  6 ++
 include/linux/mlx5/mlx5_ifc.h                 | 13 +++-
 3 files changed, 91 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..381b6090b759 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);
+	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,61 @@ 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(e->tun_info);
+		vxlan_build_gbp_hdr(vxh, 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_FMT_MOD(extack, "Wrong VxLAN GBP mask(0x%08X)\n", *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 +180,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 +209,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 +232,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 v3 4/5] ip_tunnel: constify input argument of ip_tunnel_info_opts( )

[Simon Horman]

On Fri, Feb 17, 2023 at 05:39:24AM +0200, Gavin Li wrote:
> Constify input argument(i.e. struct ip_tunnel_info *info) of
> ip_tunnel_info_opts( ) so that it wouldn't be needed to W/A it each time
> in each driver.
> 
> Signed-off-by: Gavin Li <gavinl@nvidia.com>
> ---
>  include/net/ip_tunnels.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index fca357679816..32c77f149c6e 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -485,9 +485,9 @@ static inline void iptunnel_xmit_stats(struct net_device *dev, int pkt_len)
>  	}
>  }
>  
> -static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
> +static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
>  {
> -	return info + 1;
> +	return (void *)(info + 1);

I'm unclear on what problem this is trying to solve,
but info being const, and then returning (info +1)
as non-const feels like it is masking rather than fixing a problem.

>  }
>  
>  static inline void ip_tunnel_info_opts_get(void *to,
> -- 
> 2.31.1
> 

Re: [PATCH net-next v3 1/5] vxlan: Remove unused argument from vxlan_build_gbp_hdr( ) and vxlan_build_gpe_hdr( )

[Simon Horman]

On Fri, Feb 17, 2023 at 05:39:21AM +0200, Gavin Li wrote:
> Remove unused argument (i.e. u32 vxflags) in vxlan_build_gbp_hdr( ) and
> vxlan_build_gpe_hdr( ) function arguments.
> 
> Signed-off-by: Gavin Li <gavinl@nvidia.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

[Simon Horman]

On Fri, Feb 17, 2023 at 05:39:22AM +0200, Gavin Li wrote:
> vxlan_build_gbp_hdr will be used by other modules to build gbp option in
> vxlan header according to gbp flags.
> 
> Signed-off-by: Gavin Li <gavinl@nvidia.com>
> Reviewed-by: Gavi Teitz <gavi@nvidia.com>
> Reviewed-by: Roi Dayan <roid@nvidia.com>
> Reviewed-by: Maor Dickman <maord@nvidia.com>
> Acked-by: Saeed Mahameed <saeedm@nvidia.com>

I do wonder if this needs to be a static inline function.
But nonetheless,

Reviewed-by: Simon Horman <simon.horman@corigine.com>

> ---
>  drivers/net/vxlan/vxlan_core.c | 19 -------------------
>  include/net/vxlan.h            | 19 +++++++++++++++++++
>  2 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 86967277ab97..13faab36b3e1 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -2140,25 +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, 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, __be16 protocol)
>  {
>  	struct vxlanhdr_gpe *gpe = (struct vxlanhdr_gpe *)vxh;
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index bca5b01af247..b6d419fa7ab1 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -566,4 +566,23 @@ static inline bool vxlan_fdb_nh_path_select(struct nexthop *nh,
>  	return true;
>  }
>  
> +static inline void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const 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 v3 3/5] net/mlx5e: Add helper for encap_info_equal for tunnels with options

[Simon Horman]

On Fri, Feb 17, 2023 at 05:39:23AM +0200, Gavin Li wrote:
> 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.
> 
> Signed-off-by: Gavin Li <gavinl@nvidia.com>
> Reviewed-by: Gavi Teitz <gavi@nvidia.com>
> Reviewed-by: Roi Dayan <roid@nvidia.com>
> Reviewed-by: Maor Dickman <maord@nvidia.com>
> Acked-by: Saeed Mahameed <saeedm@nvidia.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next v3 4/5] ip_tunnel: constify input argument of ip_tunnel_info_opts( )

[Simon Horman]

On Sun, Feb 19, 2023 at 09:29:21PM +0100, Simon Horman wrote:
> On Fri, Feb 17, 2023 at 05:39:24AM +0200, Gavin Li wrote:
> > Constify input argument(i.e. struct ip_tunnel_info *info) of
> > ip_tunnel_info_opts( ) so that it wouldn't be needed to W/A it each time
> > in each driver.
> > 
> > Signed-off-by: Gavin Li <gavinl@nvidia.com>
> > ---
> >  include/net/ip_tunnels.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> > index fca357679816..32c77f149c6e 100644
> > --- a/include/net/ip_tunnels.h
> > +++ b/include/net/ip_tunnels.h
> > @@ -485,9 +485,9 @@ static inline void iptunnel_xmit_stats(struct net_device *dev, int pkt_len)
> >  	}
> >  }
> >  
> > -static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
> > +static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
> >  {
> > -	return info + 1;
> > +	return (void *)(info + 1);
> 
> I'm unclear on what problem this is trying to solve,
> but info being const, and then returning (info +1)
> as non-const feels like it is masking rather than fixing a problem.

I now see that an example of the problem is added by path 5/5.

...
  CC [M]  drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.o
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c: In function 'mlx5e_gen_ip_tunnel_header_vxlan':
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:103:43: error: passing argument 1 of 'ip_tunnel_info_opts' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
  103 |                 md = ip_tunnel_info_opts(e->tun_info);
      |                                          ~^~~~~~~~~~
In file included from drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:4:
./include/net/ip_tunnels.h:488:64: note: expected 'struct ip_tunnel_info *' but argument is of type 'const struct ip_tunnel_info *'
  488 | static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
      |                                         ~~~~~~~~~~~~~~~~~~~~~~~^~~~
...

But I really do wonder if this patch masks rather than fixes the problem.

> 
> >  }
> >  
> >  static inline void ip_tunnel_info_opts_get(void *to,
> > -- 
> > 2.31.1
> > 

Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

[Gavin Li]

On 2/20/2023 4:32 AM, Simon Horman wrote:
> External email: Use caution opening links or attachments
>
>
> On Fri, Feb 17, 2023 at 05:39:22AM +0200, Gavin Li wrote:
>> vxlan_build_gbp_hdr will be used by other modules to build gbp option in
>> vxlan header according to gbp flags.
>>
>> Signed-off-by: Gavin Li <gavinl@nvidia.com>
>> Reviewed-by: Gavi Teitz <gavi@nvidia.com>
>> Reviewed-by: Roi Dayan <roid@nvidia.com>
>> Reviewed-by: Maor Dickman <maord@nvidia.com>
>> Acked-by: Saeed Mahameed <saeedm@nvidia.com>
> I do wonder if this needs to be a static inline function.
> But nonetheless,

Will get "unused-function" from gcc without "inline"

./include/net/vxlan.h:569:13: warning: ‘vxlan_build_gbp_hdr’ defined but 
not used [-Wunused-function]
  static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct 
vxlan_metadata *md)

>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
>
>> ---
>>   drivers/net/vxlan/vxlan_core.c | 19 -------------------
>>   include/net/vxlan.h            | 19 +++++++++++++++++++
>>   2 files changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
>> index 86967277ab97..13faab36b3e1 100644
>> --- a/drivers/net/vxlan/vxlan_core.c
>> +++ b/drivers/net/vxlan/vxlan_core.c
>> @@ -2140,25 +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, 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, __be16 protocol)
>>   {
>>        struct vxlanhdr_gpe *gpe = (struct vxlanhdr_gpe *)vxh;
>> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
>> index bca5b01af247..b6d419fa7ab1 100644
>> --- a/include/net/vxlan.h
>> +++ b/include/net/vxlan.h
>> @@ -566,4 +566,23 @@ static inline bool vxlan_fdb_nh_path_select(struct nexthop *nh,
>>        return true;
>>   }
>>
>> +static inline void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const 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 v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

[Simon Horman]

On Mon, Feb 20, 2023 at 10:05:00AM +0800, Gavin Li wrote:
> 
> On 2/20/2023 4:32 AM, Simon Horman wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Fri, Feb 17, 2023 at 05:39:22AM +0200, Gavin Li wrote:
> > > vxlan_build_gbp_hdr will be used by other modules to build gbp option in
> > > vxlan header according to gbp flags.
> > > 
> > > Signed-off-by: Gavin Li <gavinl@nvidia.com>
> > > Reviewed-by: Gavi Teitz <gavi@nvidia.com>
> > > Reviewed-by: Roi Dayan <roid@nvidia.com>
> > > Reviewed-by: Maor Dickman <maord@nvidia.com>
> > > Acked-by: Saeed Mahameed <saeedm@nvidia.com>
> > I do wonder if this needs to be a static inline function.
> > But nonetheless,
> 
> Will get "unused-function" from gcc without "inline"
> 
> ./include/net/vxlan.h:569:13: warning: ‘vxlan_build_gbp_hdr’ defined but not
> used [-Wunused-function]
>  static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct
> vxlan_metadata *md)

Right. But what I was really wondering is if the definition
of the function could stay in drivers/net/vxlan/vxlan_core.c,
without being static. And have a declaration in include/net/vxlan.h

> > Reviewed-by: Simon Horman <simon.horman@corigine.com>
> > 
> > > ---
> > >   drivers/net/vxlan/vxlan_core.c | 19 -------------------
> > >   include/net/vxlan.h            | 19 +++++++++++++++++++
> > >   2 files changed, 19 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> > > index 86967277ab97..13faab36b3e1 100644
> > > --- a/drivers/net/vxlan/vxlan_core.c
> > > +++ b/drivers/net/vxlan/vxlan_core.c
> > > @@ -2140,25 +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, 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, __be16 protocol)
> > >   {
> > >        struct vxlanhdr_gpe *gpe = (struct vxlanhdr_gpe *)vxh;
> > > diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> > > index bca5b01af247..b6d419fa7ab1 100644
> > > --- a/include/net/vxlan.h
> > > +++ b/include/net/vxlan.h
> > > @@ -566,4 +566,23 @@ static inline bool vxlan_fdb_nh_path_select(struct nexthop *nh,
> > >        return true;
> > >   }
> > > 
> > > +static inline void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const 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 v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

[Gavin Li]

On 2/20/2023 2:40 PM, Simon Horman wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, Feb 20, 2023 at 10:05:00AM +0800, Gavin Li wrote:
>> On 2/20/2023 4:32 AM, Simon Horman wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Fri, Feb 17, 2023 at 05:39:22AM +0200, Gavin Li wrote:
>>>> vxlan_build_gbp_hdr will be used by other modules to build gbp option in
>>>> vxlan header according to gbp flags.
>>>>
>>>> Signed-off-by: Gavin Li <gavinl@nvidia.com>
>>>> Reviewed-by: Gavi Teitz <gavi@nvidia.com>
>>>> Reviewed-by: Roi Dayan <roid@nvidia.com>
>>>> Reviewed-by: Maor Dickman <maord@nvidia.com>
>>>> Acked-by: Saeed Mahameed <saeedm@nvidia.com>
>>> I do wonder if this needs to be a static inline function.
>>> But nonetheless,
>> Will get "unused-function" from gcc without "inline"
>>
>> ./include/net/vxlan.h:569:13: warning: ‘vxlan_build_gbp_hdr’ defined but not
>> used [-Wunused-function]
>>   static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct
>> vxlan_metadata *md)
> Right. But what I was really wondering is if the definition
> of the function could stay in drivers/net/vxlan/vxlan_core.c,
> without being static. And have a declaration in include/net/vxlan.h

Tried that the first time the function was called by driver code. It 
would introduce dependency in linking between the driver and the kernel 
module.

Do you think it's OK to have such dependency?

>
>>> Reviewed-by: Simon Horman <simon.horman@corigine.com>
>>>
>>>> ---
>>>>    drivers/net/vxlan/vxlan_core.c | 19 -------------------
>>>>    include/net/vxlan.h            | 19 +++++++++++++++++++
>>>>    2 files changed, 19 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
>>>> index 86967277ab97..13faab36b3e1 100644
>>>> --- a/drivers/net/vxlan/vxlan_core.c
>>>> +++ b/drivers/net/vxlan/vxlan_core.c
>>>> @@ -2140,25 +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, 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, __be16 protocol)
>>>>    {
>>>>         struct vxlanhdr_gpe *gpe = (struct vxlanhdr_gpe *)vxh;
>>>> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
>>>> index bca5b01af247..b6d419fa7ab1 100644
>>>> --- a/include/net/vxlan.h
>>>> +++ b/include/net/vxlan.h
>>>> @@ -566,4 +566,23 @@ static inline bool vxlan_fdb_nh_path_select(struct nexthop *nh,
>>>>         return true;
>>>>    }
>>>>
>>>> +static inline void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const 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 v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

[Simon Horman]

On Mon, Feb 20, 2023 at 03:15:20PM +0800, Gavin Li wrote:
> 
> On 2/20/2023 2:40 PM, Simon Horman wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Mon, Feb 20, 2023 at 10:05:00AM +0800, Gavin Li wrote:
> > > On 2/20/2023 4:32 AM, Simon Horman wrote:
> > > > External email: Use caution opening links or attachments
> > > > 
> > > > 
> > > > On Fri, Feb 17, 2023 at 05:39:22AM +0200, Gavin Li wrote:
> > > > > vxlan_build_gbp_hdr will be used by other modules to build gbp option in
> > > > > vxlan header according to gbp flags.
> > > > > 
> > > > > Signed-off-by: Gavin Li <gavinl@nvidia.com>
> > > > > Reviewed-by: Gavi Teitz <gavi@nvidia.com>
> > > > > Reviewed-by: Roi Dayan <roid@nvidia.com>
> > > > > Reviewed-by: Maor Dickman <maord@nvidia.com>
> > > > > Acked-by: Saeed Mahameed <saeedm@nvidia.com>
> > > > I do wonder if this needs to be a static inline function.
> > > > But nonetheless,
> > > Will get "unused-function" from gcc without "inline"
> > > 
> > > ./include/net/vxlan.h:569:13: warning: ‘vxlan_build_gbp_hdr’ defined but not
> > > used [-Wunused-function]
> > >   static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct
> > > vxlan_metadata *md)
> > Right. But what I was really wondering is if the definition
> > of the function could stay in drivers/net/vxlan/vxlan_core.c,
> > without being static. And have a declaration in include/net/vxlan.h
> 
> Tried that the first time the function was called by driver code. It would
> introduce dependency in linking between the driver and the kernel module.
> 
> Do you think it's OK to have such dependency?

IMHO, yes. But others may feel differently.

I do wonder if any performance overhead of a non-inline function
also needs to be considered.

Re: [PATCH net-next v3 4/5] ip_tunnel: constify input argument of ip_tunnel_info_opts( )

[Gavin Li]

On 2/20/2023 4:46 AM, Simon Horman wrote:
> External email: Use caution opening links or attachments
>
>
> On Sun, Feb 19, 2023 at 09:29:21PM +0100, Simon Horman wrote:
>> On Fri, Feb 17, 2023 at 05:39:24AM +0200, Gavin Li wrote:
>>> Constify input argument(i.e. struct ip_tunnel_info *info) of
>>> ip_tunnel_info_opts( ) so that it wouldn't be needed to W/A it each time
>>> in each driver.
>>>
>>> Signed-off-by: Gavin Li <gavinl@nvidia.com>
>>> ---
>>>   include/net/ip_tunnels.h | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
>>> index fca357679816..32c77f149c6e 100644
>>> --- a/include/net/ip_tunnels.h
>>> +++ b/include/net/ip_tunnels.h
>>> @@ -485,9 +485,9 @@ static inline void iptunnel_xmit_stats(struct net_device *dev, int pkt_len)
>>>      }
>>>   }
>>>
>>> -static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
>>> +static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
>>>   {
>>> -   return info + 1;
>>> +   return (void *)(info + 1);
>> I'm unclear on what problem this is trying to solve,
>> but info being const, and then returning (info +1)
>> as non-const feels like it is masking rather than fixing a problem.
> I now see that an example of the problem is added by path 5/5.
>
> ...
>    CC [M]  drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.o
> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c: In function 'mlx5e_gen_ip_tunnel_header_vxlan':
> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:103:43: error: passing argument 1 of 'ip_tunnel_info_opts' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
>    103 |                 md = ip_tunnel_info_opts(e->tun_info);
>        |                                          ~^~~~~~~~~~
> In file included from drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:4:
> ./include/net/ip_tunnels.h:488:64: note: expected 'struct ip_tunnel_info *' but argument is of type 'const struct ip_tunnel_info *'
>    488 | static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
>        |                                         ~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ...
>
> But I really do wonder if this patch masks rather than fixes the problem.
Hi Olek, any comment?
>
>>>   }
>>>
>>>   static inline void ip_tunnel_info_opts_get(void *to,
>>> --
>>> 2.31.1
>>>

Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

[Jakub Kicinski]

On Mon, 20 Feb 2023 11:31:59 +0100 Simon Horman wrote:
> On Mon, Feb 20, 2023 at 03:15:20PM +0800, Gavin Li wrote:
> > > Right. But what I was really wondering is if the definition
> > > of the function could stay in drivers/net/vxlan/vxlan_core.c,
> > > without being static. And have a declaration in include/net/vxlan.h  
> > 
> > Tried that the first time the function was called by driver code. It would
> > introduce dependency in linking between the driver and the kernel module.
> > 
> > Do you think it's OK to have such dependency?  
> 
> IMHO, yes. But others may feel differently.
> 
> I do wonder if any performance overhead of a non-inline function
> also needs to be considered.

Do you recall any details of why Hannes broke the dependency in the
first place? 
Commit b7aade15485a ("vxlan: break dependency with netdev drivers")
Maybe we should stick to the static inline, it doesn't look too
large/terrible?

Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

[Paolo Abeni]

On Mon, 2023-02-20 at 12:30 -0800, Jakub Kicinski wrote:
> On Mon, 20 Feb 2023 11:31:59 +0100 Simon Horman wrote:
> > On Mon, Feb 20, 2023 at 03:15:20PM +0800, Gavin Li wrote:
> > > > Right. But what I was really wondering is if the definition
> > > > of the function could stay in drivers/net/vxlan/vxlan_core.c,
> > > > without being static. And have a declaration in include/net/vxlan.h  
> > > 
> > > Tried that the first time the function was called by driver code. It would
> > > introduce dependency in linking between the driver and the kernel module.
> > > 
> > > Do you think it's OK to have such dependency?  
> > 
> > IMHO, yes. But others may feel differently.
> > 
> > I do wonder if any performance overhead of a non-inline function
> > also needs to be considered.
> 
> Do you recall any details of why Hannes broke the dependency in the
> first place?

IIRC it was that was a cleanup thing, so that setup not using vxlan
does not load the module (and the related deps chain) for no reasons.

Cheers,

Paolo

> Commit b7aade15485a ("vxlan: break dependency with netdev drivers")
> Maybe we should stick to the static inline, it doesn't look too
> large/terrible?

IMHO static inline is good enough here.

Thanks,

Paolo

Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

[Simon Horman]

On Tue, Feb 21, 2023 at 08:38:17AM +0100, Paolo Abeni wrote:
> On Mon, 2023-02-20 at 12:30 -0800, Jakub Kicinski wrote:
> > On Mon, 20 Feb 2023 11:31:59 +0100 Simon Horman wrote:
> > > On Mon, Feb 20, 2023 at 03:15:20PM +0800, Gavin Li wrote:
> > > > > Right. But what I was really wondering is if the definition
> > > > > of the function could stay in drivers/net/vxlan/vxlan_core.c,
> > > > > without being static. And have a declaration in include/net/vxlan.h  
> > > > 
> > > > Tried that the first time the function was called by driver code. It would
> > > > introduce dependency in linking between the driver and the kernel module.
> > > > 
> > > > Do you think it's OK to have such dependency?  
> > > 
> > > IMHO, yes. But others may feel differently.
> > > 
> > > I do wonder if any performance overhead of a non-inline function
> > > also needs to be considered.
> > 
> > Do you recall any details of why Hannes broke the dependency in the
> > first place?
> 
> IIRC it was that was a cleanup thing, so that setup not using vxlan
> does not load the module (and the related deps chain) for no reasons.
> 
> Cheers,
> 
> Paolo
> 
> > Commit b7aade15485a ("vxlan: break dependency with netdev drivers")
> > Maybe we should stick to the static inline, it doesn't look too
> > large/terrible?
> 
> IMHO static inline is good enough here.

Thanks Paolo and Jakub,

I do not recall the background to the change.
But your reasoning sounds good to me.

Let's stick with static inline.

Re: [PATCH net-next v3 4/5] ip_tunnel: constify input argument of ip_tunnel_info_opts( )

[Gavin Li]

On 2/20/2023 4:46 AM, Simon Horman wrote:
> External email: Use caution opening links or attachments
>
>
> On Sun, Feb 19, 2023 at 09:29:21PM +0100, Simon Horman wrote:
>> On Fri, Feb 17, 2023 at 05:39:24AM +0200, Gavin Li wrote:
>>> Constify input argument(i.e. struct ip_tunnel_info *info) of
>>> ip_tunnel_info_opts( ) so that it wouldn't be needed to W/A it each time
>>> in each driver.
>>>
>>> Signed-off-by: Gavin Li <gavinl@nvidia.com>
>>> ---
>>>   include/net/ip_tunnels.h | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
>>> index fca357679816..32c77f149c6e 100644
>>> --- a/include/net/ip_tunnels.h
>>> +++ b/include/net/ip_tunnels.h
>>> @@ -485,9 +485,9 @@ static inline void iptunnel_xmit_stats(struct net_device *dev, int pkt_len)
>>>      }
>>>   }
>>>
>>> -static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
>>> +static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
>>>   {
>>> -   return info + 1;
>>> +   return (void *)(info + 1);
>> I'm unclear on what problem this is trying to solve,
>> but info being const, and then returning (info +1)
>> as non-const feels like it is masking rather than fixing a problem.
> I now see that an example of the problem is added by path 5/5.
>
> ...
>    CC [M]  drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.o
> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c: In function 'mlx5e_gen_ip_tunnel_header_vxlan':
> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:103:43: error: passing argument 1 of 'ip_tunnel_info_opts' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
>    103 |                 md = ip_tunnel_info_opts(e->tun_info);
>        |                                          ~^~~~~~~~~~
> In file included from drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:4:
> ./include/net/ip_tunnels.h:488:64: note: expected 'struct ip_tunnel_info *' but argument is of type 'const struct ip_tunnel_info *'
>    488 | static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
>        |                                         ~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ...
>
> But I really do wonder if this patch masks rather than fixes the problem.
ACK
>
>>>   }
>>>
>>>   static inline void ip_tunnel_info_opts_get(void *to,
>>> --
>>> 2.31.1
>>>

Re: [PATCH net-next v3 4/5] ip_tunnel: constify input argument of ip_tunnel_info_opts( )

[Alexander Lobakin]

From: Gavin Li <gavinl@nvidia.com>
Date: Mon, 20 Feb 2023 18:42:14 +0800

> 
> On 2/20/2023 4:46 AM, Simon Horman wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On Sun, Feb 19, 2023 at 09:29:21PM +0100, Simon Horman wrote:
>>> On Fri, Feb 17, 2023 at 05:39:24AM +0200, Gavin Li wrote:
>>>> Constify input argument(i.e. struct ip_tunnel_info *info) of
>>>> ip_tunnel_info_opts( ) so that it wouldn't be needed to W/A it each
>>>> time
>>>> in each driver.
>>>>
>>>> Signed-off-by: Gavin Li <gavinl@nvidia.com>
>>>> ---
>>>>   include/net/ip_tunnels.h | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
>>>> index fca357679816..32c77f149c6e 100644
>>>> --- a/include/net/ip_tunnels.h
>>>> +++ b/include/net/ip_tunnels.h
>>>> @@ -485,9 +485,9 @@ static inline void iptunnel_xmit_stats(struct
>>>> net_device *dev, int pkt_len)
>>>>      }
>>>>   }
>>>>
>>>> -static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
>>>> +static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info
>>>> *info)
>>>>   {
>>>> -   return info + 1;
>>>> +   return (void *)(info + 1);
>>> I'm unclear on what problem this is trying to solve,
>>> but info being const, and then returning (info +1)
>>> as non-const feels like it is masking rather than fixing a problem.
>> I now see that an example of the problem is added by path 5/5.
>>
>> ...
>>    CC [M]  drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.o
>> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c: In function
>> 'mlx5e_gen_ip_tunnel_header_vxlan':
>> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:103:43:
>> error: passing argument 1 of 'ip_tunnel_info_opts' discards 'const'
>> qualifier from pointer target type [-Werror=discarded-qualifiers]
>>    103 |                 md = ip_tunnel_info_opts(e->tun_info);
>>        |                                          ~^~~~~~~~~~
>> In file included from
>> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:4:
>> ./include/net/ip_tunnels.h:488:64: note: expected 'struct
>> ip_tunnel_info *' but argument is of type 'const struct ip_tunnel_info *'
>>    488 | static inline void *ip_tunnel_info_opts(struct ip_tunnel_info
>> *info)
>>        |                                        
>> ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> ...
>>
>> But I really do wonder if this patch masks rather than fixes the problem.
> Hi Olek, any comment?

Hi,

Sorry for the late reply, was busy at work ._.

I initially proposed a solution via _Generic or __builtin_choose_expr to
return const or non-const opts basing on the input pointer type. I don't
like simple cast-aways.

See container_of_const() how it dynamically chooses whether to add
`const` or not when returning the result.

>>
>>>>   }
>>>>
>>>>   static inline void ip_tunnel_info_opts_get(void *to,
>>>> -- 
>>>> 2.31.1
>>>>

Thanks,
Olek