All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3 RESEND] mlx5_vdpa: Support MAC/VLAN haw offloading
@ 2022-04-11 12:29 Eli Cohen
  2022-04-11 12:29 ` [PATCH 1/3] vdpa/mlx5: Remove flow counter from steering Eli Cohen
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Eli Cohen @ 2022-04-11 12:29 UTC (permalink / raw)
  To: mst, jasowang; +Cc: virtualization, linux-kernel, si-wei.liu, Eli Cohen

The following patches introduce MAC/VLAN filter suport for mlx5 vdpa.
First patch remove code that uses hardware counters which are not
necessary anymore.
Second patch add missing struct to carry VLAN IDs.
The third patch is the actual vlan MAC/VLAN filtering implementation.

Sorry about the resend. I had a mistake in Jason's email.

Eli Cohen (3):
  vdpa/mlx5: Remove flow counter from steering
  virtio_net: Add control VQ struct to carry vlan id
  vdpa/mlx5: Add RX MAC VLAN filter support

 drivers/vdpa/mlx5/net/mlx5_vnet.c | 292 ++++++++++++++++++++++--------
 include/uapi/linux/virtio_net.h   |   3 +
 2 files changed, 222 insertions(+), 73 deletions(-)

-- 
2.35.1


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

* [PATCH 1/3] vdpa/mlx5: Remove flow counter from steering
  2022-04-11 12:29 [PATCH 0/3 RESEND] mlx5_vdpa: Support MAC/VLAN haw offloading Eli Cohen
@ 2022-04-11 12:29 ` Eli Cohen
  2022-04-11 12:29 ` [PATCH 2/3] virtio_net: Add control VQ struct to carry vlan id Eli Cohen
  2022-04-11 12:29 ` [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support Eli Cohen
  2 siblings, 0 replies; 13+ messages in thread
From: Eli Cohen @ 2022-04-11 12:29 UTC (permalink / raw)
  To: mst, jasowang; +Cc: virtualization, linux-kernel, si-wei.liu, Eli Cohen

The flow counter has been introduced in early versions of the driver to
aid in debugging. It is no longer needed and can harm performance.

Remove it.

Signed-off-by: Eli Cohen <elic@nvidia.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index e0de44000d92..5aa6220c7129 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -156,7 +156,6 @@ struct mlx5_vdpa_net {
 	 */
 	struct mutex reslock;
 	struct mlx5_flow_table *rxft;
-	struct mlx5_fc *rx_counter;
 	struct mlx5_flow_handle *rx_rule_ucast;
 	struct mlx5_flow_handle *rx_rule_mcast;
 	bool setup;
@@ -1349,7 +1348,7 @@ static void destroy_tir(struct mlx5_vdpa_net *ndev)
 
 static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
 {
-	struct mlx5_flow_destination dest[2] = {};
+	struct mlx5_flow_destination dest = {};
 	struct mlx5_flow_table_attr ft_attr = {};
 	struct mlx5_flow_act flow_act = {};
 	struct mlx5_flow_namespace *ns;
@@ -1381,12 +1380,6 @@ static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
 		goto err_ns;
 	}
 
-	ndev->rx_counter = mlx5_fc_create(ndev->mvdev.mdev, false);
-	if (IS_ERR(ndev->rx_counter)) {
-		err = PTR_ERR(ndev->rx_counter);
-		goto err_fc;
-	}
-
 	headers_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, outer_headers);
 	dmac_c = MLX5_ADDR_OF(fte_match_param, headers_c, outer_headers.dmac_47_16);
 	memset(dmac_c, 0xff, ETH_ALEN);
@@ -1394,12 +1387,10 @@ static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
 	dmac_v = MLX5_ADDR_OF(fte_match_param, headers_v, outer_headers.dmac_47_16);
 	ether_addr_copy(dmac_v, ndev->config.mac);
 
-	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST | MLX5_FLOW_CONTEXT_ACTION_COUNT;
-	dest[0].type = MLX5_FLOW_DESTINATION_TYPE_TIR;
-	dest[0].tir_num = ndev->res.tirn;
-	dest[1].type = MLX5_FLOW_DESTINATION_TYPE_COUNTER;
-	dest[1].counter_id = mlx5_fc_id(ndev->rx_counter);
-	ndev->rx_rule_ucast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, dest, 2);
+	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
+	dest.type = MLX5_FLOW_DESTINATION_TYPE_TIR;
+	dest.tir_num = ndev->res.tirn;
+	ndev->rx_rule_ucast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
 
 	if (IS_ERR(ndev->rx_rule_ucast)) {
 		err = PTR_ERR(ndev->rx_rule_ucast);
@@ -1412,7 +1403,7 @@ static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
 	dmac_c[0] = 1;
 	dmac_v[0] = 1;
 	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
-	ndev->rx_rule_mcast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, dest, 1);
+	ndev->rx_rule_mcast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
 	if (IS_ERR(ndev->rx_rule_mcast)) {
 		err = PTR_ERR(ndev->rx_rule_mcast);
 		ndev->rx_rule_mcast = NULL;
@@ -1426,8 +1417,6 @@ static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
 	mlx5_del_flow_rules(ndev->rx_rule_ucast);
 	ndev->rx_rule_ucast = NULL;
 err_rule_ucast:
-	mlx5_fc_destroy(ndev->mvdev.mdev, ndev->rx_counter);
-err_fc:
 	mlx5_destroy_flow_table(ndev->rxft);
 err_ns:
 	kvfree(spec);
@@ -1443,7 +1432,6 @@ static void remove_fwd_to_tir(struct mlx5_vdpa_net *ndev)
 	ndev->rx_rule_mcast = NULL;
 	mlx5_del_flow_rules(ndev->rx_rule_ucast);
 	ndev->rx_rule_ucast = NULL;
-	mlx5_fc_destroy(ndev->mvdev.mdev, ndev->rx_counter);
 	mlx5_destroy_flow_table(ndev->rxft);
 }
 
-- 
2.35.1


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

* [PATCH 2/3] virtio_net: Add control VQ struct to carry vlan id
  2022-04-11 12:29 [PATCH 0/3 RESEND] mlx5_vdpa: Support MAC/VLAN haw offloading Eli Cohen
  2022-04-11 12:29 ` [PATCH 1/3] vdpa/mlx5: Remove flow counter from steering Eli Cohen
@ 2022-04-11 12:29 ` Eli Cohen
  2022-04-15  3:00     ` Jason Wang
  2022-04-11 12:29 ` [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support Eli Cohen
  2 siblings, 1 reply; 13+ messages in thread
From: Eli Cohen @ 2022-04-11 12:29 UTC (permalink / raw)
  To: mst, jasowang; +Cc: virtualization, linux-kernel, si-wei.liu, Eli Cohen

Add structure to define the payload of control VQ messages carrying the
configured vlan ID. It will be used in subsequent patches of this
series.

Signed-off-by: Eli Cohen <elic@nvidia.com>
---
 include/uapi/linux/virtio_net.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index 3f55a4215f11..b94a405fa8d2 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -270,6 +270,9 @@ struct virtio_net_ctrl_mac {
 #define VIRTIO_NET_CTRL_VLAN       2
  #define VIRTIO_NET_CTRL_VLAN_ADD             0
  #define VIRTIO_NET_CTRL_VLAN_DEL             1
+struct virtio_net_ctrl_vlan {
+	__virtio16 id;
+};
 
 /*
  * Control link announce acknowledgement
-- 
2.35.1


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

* [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support
  2022-04-11 12:29 [PATCH 0/3 RESEND] mlx5_vdpa: Support MAC/VLAN haw offloading Eli Cohen
  2022-04-11 12:29 ` [PATCH 1/3] vdpa/mlx5: Remove flow counter from steering Eli Cohen
  2022-04-11 12:29 ` [PATCH 2/3] virtio_net: Add control VQ struct to carry vlan id Eli Cohen
@ 2022-04-11 12:29 ` Eli Cohen
  2022-04-15  3:58     ` Jason Wang
  2 siblings, 1 reply; 13+ messages in thread
From: Eli Cohen @ 2022-04-11 12:29 UTC (permalink / raw)
  To: mst, jasowang; +Cc: virtualization, linux-kernel, si-wei.liu, Eli Cohen

Support HW offloaded filtering of MAC/VLAN packets.
To allow that, we add a handler to handle VLAN configurations coming
through the control VQ. Two operations are supported.

1. Adding VLAN - in this case, an entry will be added to the RX flow
   table that will allow the combination of the MAC/VLAN to be
   forwarded to the TIR.
2. Removing VLAN - will remove the entry from the flow table,
   effectively blocking such packets from going through.

Currently the control VQ does not propagate changes to the MAC of the
VLAN device so we always use the MAC of the parent device.

Examples:
1. Create vlan device:
$ ip link add link ens1 name ens1.8 type vlan id 8

Signed-off-by: Eli Cohen <elic@nvidia.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 274 +++++++++++++++++++++++-------
 1 file changed, 216 insertions(+), 58 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 5aa6220c7129..f81f9a213ed2 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -48,6 +48,8 @@ MODULE_LICENSE("Dual BSD/GPL");
 
 #define MLX5_FEATURE(_mvdev, _feature) (!!((_mvdev)->actual_features & BIT_ULL(_feature)))
 
+#define MLX5V_UNTAGGED 0x1000
+
 struct mlx5_vdpa_net_resources {
 	u32 tisn;
 	u32 tdn;
@@ -143,6 +145,8 @@ static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
 	return idx <= mvdev->max_idx;
 }
 
+#define MLX5V_MACVLAN_SIZE 256
+
 struct mlx5_vdpa_net {
 	struct mlx5_vdpa_dev mvdev;
 	struct mlx5_vdpa_net_resources res;
@@ -156,14 +160,20 @@ struct mlx5_vdpa_net {
 	 */
 	struct mutex reslock;
 	struct mlx5_flow_table *rxft;
-	struct mlx5_flow_handle *rx_rule_ucast;
-	struct mlx5_flow_handle *rx_rule_mcast;
 	bool setup;
 	u32 cur_num_vqs;
 	u32 rqt_size;
 	struct notifier_block nb;
 	struct vdpa_callback config_cb;
 	struct mlx5_vdpa_wq_ent cvq_ent;
+	struct hlist_head macvlan_hash[MLX5V_MACVLAN_SIZE];
+};
+
+struct macvlan_node {
+	struct hlist_node hlist;
+	struct mlx5_flow_handle *ucast_rule;
+	struct mlx5_flow_handle *mcast_rule;
+	u64 macvlan;
 };
 
 static void free_resources(struct mlx5_vdpa_net *ndev);
@@ -1346,12 +1356,17 @@ static void destroy_tir(struct mlx5_vdpa_net *ndev)
 	mlx5_vdpa_destroy_tir(&ndev->mvdev, ndev->res.tirn);
 }
 
-static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
+#define MAX_STEERING_ENT 0x8000
+#define MAX_STEERING_GROUPS 2
+
+static int mlx5_vdpa_add_mac_vlan_rules(struct mlx5_vdpa_net *ndev, u8 *mac,
+					u16 vid, bool tagged,
+					struct mlx5_flow_handle **ucast,
+					struct mlx5_flow_handle **mcast)
 {
 	struct mlx5_flow_destination dest = {};
-	struct mlx5_flow_table_attr ft_attr = {};
 	struct mlx5_flow_act flow_act = {};
-	struct mlx5_flow_namespace *ns;
+	struct mlx5_flow_handle *rule;
 	struct mlx5_flow_spec *spec;
 	void *headers_c;
 	void *headers_v;
@@ -1364,74 +1379,178 @@ static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
 		return -ENOMEM;
 
 	spec->match_criteria_enable = MLX5_MATCH_OUTER_HEADERS;
-	ft_attr.max_fte = 2;
-	ft_attr.autogroup.max_num_groups = 2;
-
-	ns = mlx5_get_flow_namespace(ndev->mvdev.mdev, MLX5_FLOW_NAMESPACE_BYPASS);
-	if (!ns) {
-		mlx5_vdpa_warn(&ndev->mvdev, "failed to get flow namespace\n");
-		err = -EOPNOTSUPP;
-		goto err_ns;
-	}
-
-	ndev->rxft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
-	if (IS_ERR(ndev->rxft)) {
-		err = PTR_ERR(ndev->rxft);
-		goto err_ns;
-	}
-
 	headers_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, outer_headers);
-	dmac_c = MLX5_ADDR_OF(fte_match_param, headers_c, outer_headers.dmac_47_16);
-	memset(dmac_c, 0xff, ETH_ALEN);
 	headers_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, outer_headers);
+	dmac_c = MLX5_ADDR_OF(fte_match_param, headers_c, outer_headers.dmac_47_16);
 	dmac_v = MLX5_ADDR_OF(fte_match_param, headers_v, outer_headers.dmac_47_16);
-	ether_addr_copy(dmac_v, ndev->config.mac);
-
+	memset(dmac_c, 0xff, ETH_ALEN);
+	ether_addr_copy(dmac_v, mac);
+	MLX5_SET(fte_match_set_lyr_2_4, headers_c, cvlan_tag, 1);
+	if (tagged) {
+		MLX5_SET(fte_match_set_lyr_2_4, headers_v, cvlan_tag, 1);
+		MLX5_SET_TO_ONES(fte_match_set_lyr_2_4, headers_c, first_vid);
+		MLX5_SET(fte_match_set_lyr_2_4, headers_c, first_vid, vid);
+	}
 	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
 	dest.type = MLX5_FLOW_DESTINATION_TYPE_TIR;
 	dest.tir_num = ndev->res.tirn;
-	ndev->rx_rule_ucast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
+	rule = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
+	if (IS_ERR(rule))
+		return PTR_ERR(rule);
 
-	if (IS_ERR(ndev->rx_rule_ucast)) {
-		err = PTR_ERR(ndev->rx_rule_ucast);
-		ndev->rx_rule_ucast = NULL;
-		goto err_rule_ucast;
-	}
+	*ucast = rule;
 
 	memset(dmac_c, 0, ETH_ALEN);
 	memset(dmac_v, 0, ETH_ALEN);
 	dmac_c[0] = 1;
 	dmac_v[0] = 1;
-	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
-	ndev->rx_rule_mcast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
-	if (IS_ERR(ndev->rx_rule_mcast)) {
-		err = PTR_ERR(ndev->rx_rule_mcast);
-		ndev->rx_rule_mcast = NULL;
-		goto err_rule_mcast;
+	rule = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
+	kvfree(spec);
+	if (IS_ERR(rule)) {
+		err = PTR_ERR(rule);
+		goto err_mcast;
 	}
 
-	kvfree(spec);
+	*mcast = rule;
 	return 0;
 
-err_rule_mcast:
-	mlx5_del_flow_rules(ndev->rx_rule_ucast);
-	ndev->rx_rule_ucast = NULL;
-err_rule_ucast:
-	mlx5_destroy_flow_table(ndev->rxft);
-err_ns:
-	kvfree(spec);
+err_mcast:
+	mlx5_del_flow_rules(*ucast);
+	return err;
+}
+
+static void mlx5_vdpa_del_mac_vlan_rules(struct mlx5_vdpa_net *ndev,
+					 struct mlx5_flow_handle *ucast,
+					 struct mlx5_flow_handle *mcast)
+{
+	mlx5_del_flow_rules(ucast);
+	mlx5_del_flow_rules(mcast);
+}
+
+static u64 search_val(u8 *mac, u16 vlan, bool tagged)
+{
+	u64 val;
+
+	if (!tagged)
+		vlan = MLX5V_UNTAGGED;
+
+	val = (u64)vlan << 48 |
+	      (u64)mac[0] << 40 |
+	      (u64)mac[1] << 32 |
+	      (u64)mac[2] << 24 |
+	      (u64)mac[3] << 16 |
+	      (u64)mac[4] << 8 |
+	      (u64)mac[5];
+
+	return val;
+}
+
+static struct macvlan_node *mac_vlan_lookup(struct mlx5_vdpa_net *ndev, u64 value)
+{
+	struct macvlan_node *pos;
+	u32 idx;
+
+	idx = hash_64(value, 8); // tbd 8
+	hlist_for_each_entry(pos, &ndev->macvlan_hash[idx], hlist) {
+		if (pos->macvlan == value)
+			return pos;
+	}
+	return NULL;
+}
+
+static int mac_vlan_add(struct mlx5_vdpa_net *ndev, u8 *mac, u16 vlan, bool tagged) // vlan -> vid
+{
+	struct macvlan_node *ptr;
+	u64 val;
+	u32 idx;
+	int err;
+
+	val = search_val(mac, vlan, tagged);
+	if (mac_vlan_lookup(ndev, val))
+		return -EEXIST;
+
+	ptr = kzalloc(sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	err = mlx5_vdpa_add_mac_vlan_rules(ndev, ndev->config.mac, vlan, tagged,
+					   &ptr->ucast_rule, &ptr->mcast_rule);
+	if (err)
+		goto err_add;
+
+	ptr->macvlan = val;
+	idx = hash_64(val, 8);
+	hlist_add_head(&ptr->hlist, &ndev->macvlan_hash[idx]);
+	return 0;
+
+err_add:
+	kfree(ptr);
 	return err;
 }
 
-static void remove_fwd_to_tir(struct mlx5_vdpa_net *ndev)
+static void mac_vlan_del(struct mlx5_vdpa_net *ndev, u8 *mac, u16 vlan, bool tagged)
 {
-	if (!ndev->rx_rule_ucast)
+	struct macvlan_node *ptr;
+
+	ptr = mac_vlan_lookup(ndev, search_val(mac, vlan, tagged));
+	if (!ptr)
 		return;
 
-	mlx5_del_flow_rules(ndev->rx_rule_mcast);
-	ndev->rx_rule_mcast = NULL;
-	mlx5_del_flow_rules(ndev->rx_rule_ucast);
-	ndev->rx_rule_ucast = NULL;
+	hlist_del(&ptr->hlist);
+	mlx5_vdpa_del_mac_vlan_rules(ndev, ptr->ucast_rule, ptr->mcast_rule);
+	kfree(ptr);
+}
+
+static void clear_mac_vlan_table(struct mlx5_vdpa_net *ndev)
+{
+	struct macvlan_node *pos;
+	struct hlist_node *n;
+	int i;
+
+	for (i = 0; i < MLX5V_MACVLAN_SIZE; i++) {
+		hlist_for_each_entry_safe(pos, n, &ndev->macvlan_hash[i], hlist) {
+			hlist_del(&pos->hlist);
+			mlx5_vdpa_del_mac_vlan_rules(ndev, pos->ucast_rule, pos->mcast_rule);
+			kfree(pos);
+		}
+	}
+}
+
+static int setup_steering(struct mlx5_vdpa_net *ndev)
+{
+	struct mlx5_flow_table_attr ft_attr = {};
+	struct mlx5_flow_namespace *ns;
+	int err;
+
+	ft_attr.max_fte = MAX_STEERING_ENT;
+	ft_attr.autogroup.max_num_groups = MAX_STEERING_GROUPS;
+
+	ns = mlx5_get_flow_namespace(ndev->mvdev.mdev, MLX5_FLOW_NAMESPACE_BYPASS);
+	if (!ns) {
+		mlx5_vdpa_warn(&ndev->mvdev, "failed to get flow namespace\n");
+		return -EOPNOTSUPP;
+	}
+
+	ndev->rxft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
+	if (IS_ERR(ndev->rxft)) {
+		mlx5_vdpa_warn(&ndev->mvdev, "failed to create flow table\n");
+		return PTR_ERR(ndev->rxft);
+	}
+
+	err = mac_vlan_add(ndev, ndev->config.mac, 0, false);
+	if (err)
+		goto err_add;
+
+	return 0;
+
+err_add:
+	mlx5_destroy_flow_table(ndev->rxft);
+	return err;
+}
+
+static void teardown_steering(struct mlx5_vdpa_net *ndev)
+{
+	clear_mac_vlan_table(ndev);
 	mlx5_destroy_flow_table(ndev->rxft);
 }
 
@@ -1482,9 +1601,9 @@ static virtio_net_ctrl_ack handle_ctrl_mac(struct mlx5_vdpa_dev *mvdev, u8 cmd)
 
 		/* Need recreate the flow table entry, so that the packet could forward back
 		 */
-		remove_fwd_to_tir(ndev);
+		mac_vlan_del(ndev, ndev->config.mac, 0, false);
 
-		if (add_fwd_to_tir(ndev)) {
+		if (mac_vlan_add(ndev, ndev->config.mac, 0, false)) {
 			mlx5_vdpa_warn(mvdev, "failed to insert forward rules, try to restore\n");
 
 			/* Although it hardly run here, we still need double check */
@@ -1508,7 +1627,7 @@ static virtio_net_ctrl_ack handle_ctrl_mac(struct mlx5_vdpa_dev *mvdev, u8 cmd)
 
 			memcpy(ndev->config.mac, mac_back, ETH_ALEN);
 
-			if (add_fwd_to_tir(ndev))
+			if (mac_vlan_add(ndev, ndev->config.mac, 0, false))
 				mlx5_vdpa_warn(mvdev, "restore forward rules failed: insert forward rules failed\n");
 
 			break;
@@ -1610,6 +1729,42 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
 	return status;
 }
 
+static virtio_net_ctrl_ack handle_ctrl_vlan(struct mlx5_vdpa_dev *mvdev, u8 cmd)
+{
+	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
+	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
+	struct mlx5_control_vq *cvq = &mvdev->cvq;
+	struct virtio_net_ctrl_vlan vlan;
+	size_t read;
+	u16 id;
+
+	switch (cmd) {
+	case VIRTIO_NET_CTRL_VLAN_ADD:
+		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&vlan, sizeof(vlan));
+		if (read != sizeof(vlan))
+			break;
+
+		id = mlx5vdpa16_to_cpu(mvdev, vlan.id);
+		if (mac_vlan_add(ndev, ndev->config.mac, id, true))
+			break;
+
+		status = VIRTIO_NET_OK;
+		break;
+	case VIRTIO_NET_CTRL_VLAN_DEL:
+		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&vlan, sizeof(vlan));
+		if (read != sizeof(vlan))
+			break;
+
+		id = mlx5vdpa16_to_cpu(mvdev, vlan.id);
+		mac_vlan_del(ndev, ndev->config.mac, id, true);
+		break;
+	default:
+	break;
+}
+
+return status;
+}
+
 static void mlx5_cvq_kick_handler(struct work_struct *work)
 {
 	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
@@ -1654,7 +1809,9 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 		case VIRTIO_NET_CTRL_MQ:
 			status = handle_ctrl_mq(mvdev, ctrl.cmd);
 			break;
-
+		case VIRTIO_NET_CTRL_VLAN:
+			status = handle_ctrl_vlan(mvdev, ctrl.cmd);
+			break;
 		default:
 			break;
 		}
@@ -1913,6 +2070,7 @@ static u64 get_supported_features(struct mlx5_core_dev *mdev)
 	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_MQ);
 	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
 	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_MTU);
+	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_CTRL_VLAN);
 
 	return mlx_vdpa_features;
 }
@@ -2198,9 +2356,9 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
 		goto err_tir;
 	}
 
-	err = add_fwd_to_tir(ndev);
+	err = setup_steering(ndev);
 	if (err) {
-		mlx5_vdpa_warn(mvdev, "add_fwd_to_tir\n");
+		mlx5_vdpa_warn(mvdev, "setup_steering\n");
 		goto err_fwd;
 	}
 	ndev->setup = true;
@@ -2226,7 +2384,7 @@ static void teardown_driver(struct mlx5_vdpa_net *ndev)
 	if (!ndev->setup)
 		return;
 
-	remove_fwd_to_tir(ndev);
+	teardown_steering(ndev);
 	destroy_tir(ndev);
 	destroy_rqt(ndev);
 	teardown_virtqueues(ndev);
-- 
2.35.1


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

* Re: [PATCH 2/3] virtio_net: Add control VQ struct to carry vlan id
  2022-04-11 12:29 ` [PATCH 2/3] virtio_net: Add control VQ struct to carry vlan id Eli Cohen
@ 2022-04-15  3:00     ` Jason Wang
  0 siblings, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-04-15  3:00 UTC (permalink / raw)
  To: Eli Cohen, mst; +Cc: virtualization, linux-kernel, si-wei.liu


在 2022/4/11 20:29, Eli Cohen 写道:
> Add structure to define the payload of control VQ messages carrying the
> configured vlan ID. It will be used in subsequent patches of this
> series.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   include/uapi/linux/virtio_net.h | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index 3f55a4215f11..b94a405fa8d2 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -270,6 +270,9 @@ struct virtio_net_ctrl_mac {
>   #define VIRTIO_NET_CTRL_VLAN       2
>    #define VIRTIO_NET_CTRL_VLAN_ADD             0
>    #define VIRTIO_NET_CTRL_VLAN_DEL             1
> +struct virtio_net_ctrl_vlan {
> +	__virtio16 id;
> +};


It looks to me there's no need to bother uAPI and we can simply use 
__virtio16 in patch 3?

Thanks


>   
>   /*
>    * Control link announce acknowledgement


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

* Re: [PATCH 2/3] virtio_net: Add control VQ struct to carry vlan id
@ 2022-04-15  3:00     ` Jason Wang
  0 siblings, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-04-15  3:00 UTC (permalink / raw)
  To: Eli Cohen, mst; +Cc: si-wei.liu, linux-kernel, virtualization


在 2022/4/11 20:29, Eli Cohen 写道:
> Add structure to define the payload of control VQ messages carrying the
> configured vlan ID. It will be used in subsequent patches of this
> series.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   include/uapi/linux/virtio_net.h | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index 3f55a4215f11..b94a405fa8d2 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -270,6 +270,9 @@ struct virtio_net_ctrl_mac {
>   #define VIRTIO_NET_CTRL_VLAN       2
>    #define VIRTIO_NET_CTRL_VLAN_ADD             0
>    #define VIRTIO_NET_CTRL_VLAN_DEL             1
> +struct virtio_net_ctrl_vlan {
> +	__virtio16 id;
> +};


It looks to me there's no need to bother uAPI and we can simply use 
__virtio16 in patch 3?

Thanks


>   
>   /*
>    * Control link announce acknowledgement

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

* Re: [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support
  2022-04-11 12:29 ` [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support Eli Cohen
@ 2022-04-15  3:58     ` Jason Wang
  0 siblings, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-04-15  3:58 UTC (permalink / raw)
  To: Eli Cohen, mst; +Cc: virtualization, linux-kernel, si-wei.liu


在 2022/4/11 20:29, Eli Cohen 写道:
> Support HW offloaded filtering of MAC/VLAN packets.
> To allow that, we add a handler to handle VLAN configurations coming
> through the control VQ. Two operations are supported.
>
> 1. Adding VLAN - in this case, an entry will be added to the RX flow
>     table that will allow the combination of the MAC/VLAN to be
>     forwarded to the TIR.
> 2. Removing VLAN - will remove the entry from the flow table,
>     effectively blocking such packets from going through.
>
> Currently the control VQ does not propagate changes to the MAC of the
> VLAN device so we always use the MAC of the parent device.
>
> Examples:
> 1. Create vlan device:
> $ ip link add link ens1 name ens1.8 type vlan id 8
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 274 +++++++++++++++++++++++-------
>   1 file changed, 216 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 5aa6220c7129..f81f9a213ed2 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -48,6 +48,8 @@ MODULE_LICENSE("Dual BSD/GPL");
>   
>   #define MLX5_FEATURE(_mvdev, _feature) (!!((_mvdev)->actual_features & BIT_ULL(_feature)))
>   
> +#define MLX5V_UNTAGGED 0x1000
> +
>   struct mlx5_vdpa_net_resources {
>   	u32 tisn;
>   	u32 tdn;
> @@ -143,6 +145,8 @@ static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
>   	return idx <= mvdev->max_idx;
>   }
>   
> +#define MLX5V_MACVLAN_SIZE 256
> +
>   struct mlx5_vdpa_net {
>   	struct mlx5_vdpa_dev mvdev;
>   	struct mlx5_vdpa_net_resources res;
> @@ -156,14 +160,20 @@ struct mlx5_vdpa_net {
>   	 */
>   	struct mutex reslock;
>   	struct mlx5_flow_table *rxft;
> -	struct mlx5_flow_handle *rx_rule_ucast;
> -	struct mlx5_flow_handle *rx_rule_mcast;
>   	bool setup;
>   	u32 cur_num_vqs;
>   	u32 rqt_size;
>   	struct notifier_block nb;
>   	struct vdpa_callback config_cb;
>   	struct mlx5_vdpa_wq_ent cvq_ent;
> +	struct hlist_head macvlan_hash[MLX5V_MACVLAN_SIZE];
> +};
> +
> +struct macvlan_node {
> +	struct hlist_node hlist;
> +	struct mlx5_flow_handle *ucast_rule;
> +	struct mlx5_flow_handle *mcast_rule;
> +	u64 macvlan;
>   };
>   
>   static void free_resources(struct mlx5_vdpa_net *ndev);
> @@ -1346,12 +1356,17 @@ static void destroy_tir(struct mlx5_vdpa_net *ndev)
>   	mlx5_vdpa_destroy_tir(&ndev->mvdev, ndev->res.tirn);
>   }
>   
> -static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
> +#define MAX_STEERING_ENT 0x8000
> +#define MAX_STEERING_GROUPS 2
> +
> +static int mlx5_vdpa_add_mac_vlan_rules(struct mlx5_vdpa_net *ndev, u8 *mac,
> +					u16 vid, bool tagged,
> +					struct mlx5_flow_handle **ucast,
> +					struct mlx5_flow_handle **mcast)
>   {
>   	struct mlx5_flow_destination dest = {};
> -	struct mlx5_flow_table_attr ft_attr = {};
>   	struct mlx5_flow_act flow_act = {};
> -	struct mlx5_flow_namespace *ns;
> +	struct mlx5_flow_handle *rule;
>   	struct mlx5_flow_spec *spec;
>   	void *headers_c;
>   	void *headers_v;
> @@ -1364,74 +1379,178 @@ static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
>   		return -ENOMEM;
>   
>   	spec->match_criteria_enable = MLX5_MATCH_OUTER_HEADERS;
> -	ft_attr.max_fte = 2;
> -	ft_attr.autogroup.max_num_groups = 2;
> -
> -	ns = mlx5_get_flow_namespace(ndev->mvdev.mdev, MLX5_FLOW_NAMESPACE_BYPASS);
> -	if (!ns) {
> -		mlx5_vdpa_warn(&ndev->mvdev, "failed to get flow namespace\n");
> -		err = -EOPNOTSUPP;
> -		goto err_ns;
> -	}
> -
> -	ndev->rxft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
> -	if (IS_ERR(ndev->rxft)) {
> -		err = PTR_ERR(ndev->rxft);
> -		goto err_ns;
> -	}
> -
>   	headers_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, outer_headers);
> -	dmac_c = MLX5_ADDR_OF(fte_match_param, headers_c, outer_headers.dmac_47_16);
> -	memset(dmac_c, 0xff, ETH_ALEN);
>   	headers_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, outer_headers);
> +	dmac_c = MLX5_ADDR_OF(fte_match_param, headers_c, outer_headers.dmac_47_16);
>   	dmac_v = MLX5_ADDR_OF(fte_match_param, headers_v, outer_headers.dmac_47_16);
> -	ether_addr_copy(dmac_v, ndev->config.mac);
> -
> +	memset(dmac_c, 0xff, ETH_ALEN);
> +	ether_addr_copy(dmac_v, mac);
> +	MLX5_SET(fte_match_set_lyr_2_4, headers_c, cvlan_tag, 1);
> +	if (tagged) {
> +		MLX5_SET(fte_match_set_lyr_2_4, headers_v, cvlan_tag, 1);
> +		MLX5_SET_TO_ONES(fte_match_set_lyr_2_4, headers_c, first_vid);
> +		MLX5_SET(fte_match_set_lyr_2_4, headers_c, first_vid, vid);
> +	}
>   	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
>   	dest.type = MLX5_FLOW_DESTINATION_TYPE_TIR;
>   	dest.tir_num = ndev->res.tirn;
> -	ndev->rx_rule_ucast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
> +	rule = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
> +	if (IS_ERR(rule))
> +		return PTR_ERR(rule);
>   
> -	if (IS_ERR(ndev->rx_rule_ucast)) {
> -		err = PTR_ERR(ndev->rx_rule_ucast);
> -		ndev->rx_rule_ucast = NULL;
> -		goto err_rule_ucast;
> -	}
> +	*ucast = rule;
>   
>   	memset(dmac_c, 0, ETH_ALEN);
>   	memset(dmac_v, 0, ETH_ALEN);
>   	dmac_c[0] = 1;
>   	dmac_v[0] = 1;
> -	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
> -	ndev->rx_rule_mcast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
> -	if (IS_ERR(ndev->rx_rule_mcast)) {
> -		err = PTR_ERR(ndev->rx_rule_mcast);
> -		ndev->rx_rule_mcast = NULL;
> -		goto err_rule_mcast;
> +	rule = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
> +	kvfree(spec);
> +	if (IS_ERR(rule)) {
> +		err = PTR_ERR(rule);
> +		goto err_mcast;
>   	}
>   
> -	kvfree(spec);
> +	*mcast = rule;
>   	return 0;
>   
> -err_rule_mcast:
> -	mlx5_del_flow_rules(ndev->rx_rule_ucast);
> -	ndev->rx_rule_ucast = NULL;
> -err_rule_ucast:
> -	mlx5_destroy_flow_table(ndev->rxft);
> -err_ns:
> -	kvfree(spec);
> +err_mcast:
> +	mlx5_del_flow_rules(*ucast);
> +	return err;
> +}
> +
> +static void mlx5_vdpa_del_mac_vlan_rules(struct mlx5_vdpa_net *ndev,
> +					 struct mlx5_flow_handle *ucast,
> +					 struct mlx5_flow_handle *mcast)
> +{
> +	mlx5_del_flow_rules(ucast);
> +	mlx5_del_flow_rules(mcast);
> +}
> +
> +static u64 search_val(u8 *mac, u16 vlan, bool tagged)
> +{
> +	u64 val;
> +
> +	if (!tagged)
> +		vlan = MLX5V_UNTAGGED;
> +
> +	val = (u64)vlan << 48 |
> +	      (u64)mac[0] << 40 |
> +	      (u64)mac[1] << 32 |
> +	      (u64)mac[2] << 24 |
> +	      (u64)mac[3] << 16 |
> +	      (u64)mac[4] << 8 |
> +	      (u64)mac[5];
> +
> +	return val;
> +}
> +
> +static struct macvlan_node *mac_vlan_lookup(struct mlx5_vdpa_net *ndev, u64 value)
> +{
> +	struct macvlan_node *pos;
> +	u32 idx;
> +
> +	idx = hash_64(value, 8); // tbd 8
> +	hlist_for_each_entry(pos, &ndev->macvlan_hash[idx], hlist) {
> +		if (pos->macvlan == value)
> +			return pos;
> +	}
> +	return NULL;
> +}
> +
> +static int mac_vlan_add(struct mlx5_vdpa_net *ndev, u8 *mac, u16 vlan, bool tagged) // vlan -> vid
> +{


I guess checkpatch may not be happy with such kind of comment.


> +	struct macvlan_node *ptr;
> +	u64 val;
> +	u32 idx;
> +	int err;
> +
> +	val = search_val(mac, vlan, tagged);
> +	if (mac_vlan_lookup(ndev, val))
> +		return -EEXIST;
> +
> +	ptr = kzalloc(sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return -ENOMEM;
> +
> +	err = mlx5_vdpa_add_mac_vlan_rules(ndev, ndev->config.mac, vlan, tagged,
> +					   &ptr->ucast_rule, &ptr->mcast_rule);
> +	if (err)
> +		goto err_add;
> +
> +	ptr->macvlan = val;
> +	idx = hash_64(val, 8);
> +	hlist_add_head(&ptr->hlist, &ndev->macvlan_hash[idx]);
> +	return 0;
> +
> +err_add:
> +	kfree(ptr);
>   	return err;
>   }
>   
> -static void remove_fwd_to_tir(struct mlx5_vdpa_net *ndev)
> +static void mac_vlan_del(struct mlx5_vdpa_net *ndev, u8 *mac, u16 vlan, bool tagged)
>   {
> -	if (!ndev->rx_rule_ucast)
> +	struct macvlan_node *ptr;
> +
> +	ptr = mac_vlan_lookup(ndev, search_val(mac, vlan, tagged));
> +	if (!ptr)
>   		return;
>   
> -	mlx5_del_flow_rules(ndev->rx_rule_mcast);
> -	ndev->rx_rule_mcast = NULL;
> -	mlx5_del_flow_rules(ndev->rx_rule_ucast);
> -	ndev->rx_rule_ucast = NULL;
> +	hlist_del(&ptr->hlist);
> +	mlx5_vdpa_del_mac_vlan_rules(ndev, ptr->ucast_rule, ptr->mcast_rule);
> +	kfree(ptr);
> +}
> +
> +static void clear_mac_vlan_table(struct mlx5_vdpa_net *ndev)
> +{
> +	struct macvlan_node *pos;
> +	struct hlist_node *n;
> +	int i;
> +
> +	for (i = 0; i < MLX5V_MACVLAN_SIZE; i++) {
> +		hlist_for_each_entry_safe(pos, n, &ndev->macvlan_hash[i], hlist) {
> +			hlist_del(&pos->hlist);
> +			mlx5_vdpa_del_mac_vlan_rules(ndev, pos->ucast_rule, pos->mcast_rule);
> +			kfree(pos);
> +		}
> +	}
> +}
> +
> +static int setup_steering(struct mlx5_vdpa_net *ndev)
> +{
> +	struct mlx5_flow_table_attr ft_attr = {};
> +	struct mlx5_flow_namespace *ns;
> +	int err;
> +
> +	ft_attr.max_fte = MAX_STEERING_ENT;
> +	ft_attr.autogroup.max_num_groups = MAX_STEERING_GROUPS;
> +
> +	ns = mlx5_get_flow_namespace(ndev->mvdev.mdev, MLX5_FLOW_NAMESPACE_BYPASS);
> +	if (!ns) {
> +		mlx5_vdpa_warn(&ndev->mvdev, "failed to get flow namespace\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ndev->rxft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
> +	if (IS_ERR(ndev->rxft)) {
> +		mlx5_vdpa_warn(&ndev->mvdev, "failed to create flow table\n");
> +		return PTR_ERR(ndev->rxft);
> +	}
> +
> +	err = mac_vlan_add(ndev, ndev->config.mac, 0, false);
> +	if (err)
> +		goto err_add;
> +
> +	return 0;
> +
> +err_add:
> +	mlx5_destroy_flow_table(ndev->rxft);
> +	return err;
> +}
> +
> +static void teardown_steering(struct mlx5_vdpa_net *ndev)
> +{
> +	clear_mac_vlan_table(ndev);
>   	mlx5_destroy_flow_table(ndev->rxft);
>   }
>   
> @@ -1482,9 +1601,9 @@ static virtio_net_ctrl_ack handle_ctrl_mac(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>   
>   		/* Need recreate the flow table entry, so that the packet could forward back
>   		 */
> -		remove_fwd_to_tir(ndev);
> +		mac_vlan_del(ndev, ndev->config.mac, 0, false);
>   
> -		if (add_fwd_to_tir(ndev)) {
> +		if (mac_vlan_add(ndev, ndev->config.mac, 0, false)) {
>   			mlx5_vdpa_warn(mvdev, "failed to insert forward rules, try to restore\n");
>   
>   			/* Although it hardly run here, we still need double check */
> @@ -1508,7 +1627,7 @@ static virtio_net_ctrl_ack handle_ctrl_mac(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>   
>   			memcpy(ndev->config.mac, mac_back, ETH_ALEN);
>   
> -			if (add_fwd_to_tir(ndev))
> +			if (mac_vlan_add(ndev, ndev->config.mac, 0, false))
>   				mlx5_vdpa_warn(mvdev, "restore forward rules failed: insert forward rules failed\n");
>   
>   			break;
> @@ -1610,6 +1729,42 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>   	return status;
>   }
>   
> +static virtio_net_ctrl_ack handle_ctrl_vlan(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> +{
> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> +	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> +	struct mlx5_control_vq *cvq = &mvdev->cvq;
> +	struct virtio_net_ctrl_vlan vlan;
> +	size_t read;
> +	u16 id;
> +
> +	switch (cmd) {
> +	case VIRTIO_NET_CTRL_VLAN_ADD:
> +		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&vlan, sizeof(vlan));
> +		if (read != sizeof(vlan))
> +			break;
> +
> +		id = mlx5vdpa16_to_cpu(mvdev, vlan.id);
> +		if (mac_vlan_add(ndev, ndev->config.mac, id, true))
> +			break;


This may work now but I wonder if we had the plan to support 
VIRTIO_NET_F_CTRL_RX?

if $mac is not in $mac_table
     drop;
if $vlan is not in $vlan_table
     drop;

With that features we probably requires the dedicated vlan filters 
without a mac? Or do we want to a $mac * $vlans rules?

If yes, maybe it's better to decouple vlan filters from mac now.

Thanks


> +
> +		status = VIRTIO_NET_OK;
> +		break;
> +	case VIRTIO_NET_CTRL_VLAN_DEL:
> +		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&vlan, sizeof(vlan));
> +		if (read != sizeof(vlan))
> +			break;
> +
> +		id = mlx5vdpa16_to_cpu(mvdev, vlan.id);
> +		mac_vlan_del(ndev, ndev->config.mac, id, true);
> +		break;
> +	default:
> +	break;
> +}
> +
> +return status;
> +}
> +
>   static void mlx5_cvq_kick_handler(struct work_struct *work)
>   {
>   	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> @@ -1654,7 +1809,9 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>   		case VIRTIO_NET_CTRL_MQ:
>   			status = handle_ctrl_mq(mvdev, ctrl.cmd);
>   			break;
> -
> +		case VIRTIO_NET_CTRL_VLAN:
> +			status = handle_ctrl_vlan(mvdev, ctrl.cmd);
> +			break;
>   		default:
>   			break;
>   		}
> @@ -1913,6 +2070,7 @@ static u64 get_supported_features(struct mlx5_core_dev *mdev)
>   	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_MQ);
>   	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
>   	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_MTU);
> +	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_CTRL_VLAN);
>   
>   	return mlx_vdpa_features;
>   }
> @@ -2198,9 +2356,9 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
>   		goto err_tir;
>   	}
>   
> -	err = add_fwd_to_tir(ndev);
> +	err = setup_steering(ndev);
>   	if (err) {
> -		mlx5_vdpa_warn(mvdev, "add_fwd_to_tir\n");
> +		mlx5_vdpa_warn(mvdev, "setup_steering\n");
>   		goto err_fwd;
>   	}
>   	ndev->setup = true;
> @@ -2226,7 +2384,7 @@ static void teardown_driver(struct mlx5_vdpa_net *ndev)
>   	if (!ndev->setup)
>   		return;
>   
> -	remove_fwd_to_tir(ndev);
> +	teardown_steering(ndev);
>   	destroy_tir(ndev);
>   	destroy_rqt(ndev);
>   	teardown_virtqueues(ndev);


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

* Re: [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support
@ 2022-04-15  3:58     ` Jason Wang
  0 siblings, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-04-15  3:58 UTC (permalink / raw)
  To: Eli Cohen, mst; +Cc: si-wei.liu, linux-kernel, virtualization


在 2022/4/11 20:29, Eli Cohen 写道:
> Support HW offloaded filtering of MAC/VLAN packets.
> To allow that, we add a handler to handle VLAN configurations coming
> through the control VQ. Two operations are supported.
>
> 1. Adding VLAN - in this case, an entry will be added to the RX flow
>     table that will allow the combination of the MAC/VLAN to be
>     forwarded to the TIR.
> 2. Removing VLAN - will remove the entry from the flow table,
>     effectively blocking such packets from going through.
>
> Currently the control VQ does not propagate changes to the MAC of the
> VLAN device so we always use the MAC of the parent device.
>
> Examples:
> 1. Create vlan device:
> $ ip link add link ens1 name ens1.8 type vlan id 8
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 274 +++++++++++++++++++++++-------
>   1 file changed, 216 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 5aa6220c7129..f81f9a213ed2 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -48,6 +48,8 @@ MODULE_LICENSE("Dual BSD/GPL");
>   
>   #define MLX5_FEATURE(_mvdev, _feature) (!!((_mvdev)->actual_features & BIT_ULL(_feature)))
>   
> +#define MLX5V_UNTAGGED 0x1000
> +
>   struct mlx5_vdpa_net_resources {
>   	u32 tisn;
>   	u32 tdn;
> @@ -143,6 +145,8 @@ static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
>   	return idx <= mvdev->max_idx;
>   }
>   
> +#define MLX5V_MACVLAN_SIZE 256
> +
>   struct mlx5_vdpa_net {
>   	struct mlx5_vdpa_dev mvdev;
>   	struct mlx5_vdpa_net_resources res;
> @@ -156,14 +160,20 @@ struct mlx5_vdpa_net {
>   	 */
>   	struct mutex reslock;
>   	struct mlx5_flow_table *rxft;
> -	struct mlx5_flow_handle *rx_rule_ucast;
> -	struct mlx5_flow_handle *rx_rule_mcast;
>   	bool setup;
>   	u32 cur_num_vqs;
>   	u32 rqt_size;
>   	struct notifier_block nb;
>   	struct vdpa_callback config_cb;
>   	struct mlx5_vdpa_wq_ent cvq_ent;
> +	struct hlist_head macvlan_hash[MLX5V_MACVLAN_SIZE];
> +};
> +
> +struct macvlan_node {
> +	struct hlist_node hlist;
> +	struct mlx5_flow_handle *ucast_rule;
> +	struct mlx5_flow_handle *mcast_rule;
> +	u64 macvlan;
>   };
>   
>   static void free_resources(struct mlx5_vdpa_net *ndev);
> @@ -1346,12 +1356,17 @@ static void destroy_tir(struct mlx5_vdpa_net *ndev)
>   	mlx5_vdpa_destroy_tir(&ndev->mvdev, ndev->res.tirn);
>   }
>   
> -static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
> +#define MAX_STEERING_ENT 0x8000
> +#define MAX_STEERING_GROUPS 2
> +
> +static int mlx5_vdpa_add_mac_vlan_rules(struct mlx5_vdpa_net *ndev, u8 *mac,
> +					u16 vid, bool tagged,
> +					struct mlx5_flow_handle **ucast,
> +					struct mlx5_flow_handle **mcast)
>   {
>   	struct mlx5_flow_destination dest = {};
> -	struct mlx5_flow_table_attr ft_attr = {};
>   	struct mlx5_flow_act flow_act = {};
> -	struct mlx5_flow_namespace *ns;
> +	struct mlx5_flow_handle *rule;
>   	struct mlx5_flow_spec *spec;
>   	void *headers_c;
>   	void *headers_v;
> @@ -1364,74 +1379,178 @@ static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
>   		return -ENOMEM;
>   
>   	spec->match_criteria_enable = MLX5_MATCH_OUTER_HEADERS;
> -	ft_attr.max_fte = 2;
> -	ft_attr.autogroup.max_num_groups = 2;
> -
> -	ns = mlx5_get_flow_namespace(ndev->mvdev.mdev, MLX5_FLOW_NAMESPACE_BYPASS);
> -	if (!ns) {
> -		mlx5_vdpa_warn(&ndev->mvdev, "failed to get flow namespace\n");
> -		err = -EOPNOTSUPP;
> -		goto err_ns;
> -	}
> -
> -	ndev->rxft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
> -	if (IS_ERR(ndev->rxft)) {
> -		err = PTR_ERR(ndev->rxft);
> -		goto err_ns;
> -	}
> -
>   	headers_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, outer_headers);
> -	dmac_c = MLX5_ADDR_OF(fte_match_param, headers_c, outer_headers.dmac_47_16);
> -	memset(dmac_c, 0xff, ETH_ALEN);
>   	headers_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, outer_headers);
> +	dmac_c = MLX5_ADDR_OF(fte_match_param, headers_c, outer_headers.dmac_47_16);
>   	dmac_v = MLX5_ADDR_OF(fte_match_param, headers_v, outer_headers.dmac_47_16);
> -	ether_addr_copy(dmac_v, ndev->config.mac);
> -
> +	memset(dmac_c, 0xff, ETH_ALEN);
> +	ether_addr_copy(dmac_v, mac);
> +	MLX5_SET(fte_match_set_lyr_2_4, headers_c, cvlan_tag, 1);
> +	if (tagged) {
> +		MLX5_SET(fte_match_set_lyr_2_4, headers_v, cvlan_tag, 1);
> +		MLX5_SET_TO_ONES(fte_match_set_lyr_2_4, headers_c, first_vid);
> +		MLX5_SET(fte_match_set_lyr_2_4, headers_c, first_vid, vid);
> +	}
>   	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
>   	dest.type = MLX5_FLOW_DESTINATION_TYPE_TIR;
>   	dest.tir_num = ndev->res.tirn;
> -	ndev->rx_rule_ucast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
> +	rule = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
> +	if (IS_ERR(rule))
> +		return PTR_ERR(rule);
>   
> -	if (IS_ERR(ndev->rx_rule_ucast)) {
> -		err = PTR_ERR(ndev->rx_rule_ucast);
> -		ndev->rx_rule_ucast = NULL;
> -		goto err_rule_ucast;
> -	}
> +	*ucast = rule;
>   
>   	memset(dmac_c, 0, ETH_ALEN);
>   	memset(dmac_v, 0, ETH_ALEN);
>   	dmac_c[0] = 1;
>   	dmac_v[0] = 1;
> -	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
> -	ndev->rx_rule_mcast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
> -	if (IS_ERR(ndev->rx_rule_mcast)) {
> -		err = PTR_ERR(ndev->rx_rule_mcast);
> -		ndev->rx_rule_mcast = NULL;
> -		goto err_rule_mcast;
> +	rule = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
> +	kvfree(spec);
> +	if (IS_ERR(rule)) {
> +		err = PTR_ERR(rule);
> +		goto err_mcast;
>   	}
>   
> -	kvfree(spec);
> +	*mcast = rule;
>   	return 0;
>   
> -err_rule_mcast:
> -	mlx5_del_flow_rules(ndev->rx_rule_ucast);
> -	ndev->rx_rule_ucast = NULL;
> -err_rule_ucast:
> -	mlx5_destroy_flow_table(ndev->rxft);
> -err_ns:
> -	kvfree(spec);
> +err_mcast:
> +	mlx5_del_flow_rules(*ucast);
> +	return err;
> +}
> +
> +static void mlx5_vdpa_del_mac_vlan_rules(struct mlx5_vdpa_net *ndev,
> +					 struct mlx5_flow_handle *ucast,
> +					 struct mlx5_flow_handle *mcast)
> +{
> +	mlx5_del_flow_rules(ucast);
> +	mlx5_del_flow_rules(mcast);
> +}
> +
> +static u64 search_val(u8 *mac, u16 vlan, bool tagged)
> +{
> +	u64 val;
> +
> +	if (!tagged)
> +		vlan = MLX5V_UNTAGGED;
> +
> +	val = (u64)vlan << 48 |
> +	      (u64)mac[0] << 40 |
> +	      (u64)mac[1] << 32 |
> +	      (u64)mac[2] << 24 |
> +	      (u64)mac[3] << 16 |
> +	      (u64)mac[4] << 8 |
> +	      (u64)mac[5];
> +
> +	return val;
> +}
> +
> +static struct macvlan_node *mac_vlan_lookup(struct mlx5_vdpa_net *ndev, u64 value)
> +{
> +	struct macvlan_node *pos;
> +	u32 idx;
> +
> +	idx = hash_64(value, 8); // tbd 8
> +	hlist_for_each_entry(pos, &ndev->macvlan_hash[idx], hlist) {
> +		if (pos->macvlan == value)
> +			return pos;
> +	}
> +	return NULL;
> +}
> +
> +static int mac_vlan_add(struct mlx5_vdpa_net *ndev, u8 *mac, u16 vlan, bool tagged) // vlan -> vid
> +{


I guess checkpatch may not be happy with such kind of comment.


> +	struct macvlan_node *ptr;
> +	u64 val;
> +	u32 idx;
> +	int err;
> +
> +	val = search_val(mac, vlan, tagged);
> +	if (mac_vlan_lookup(ndev, val))
> +		return -EEXIST;
> +
> +	ptr = kzalloc(sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return -ENOMEM;
> +
> +	err = mlx5_vdpa_add_mac_vlan_rules(ndev, ndev->config.mac, vlan, tagged,
> +					   &ptr->ucast_rule, &ptr->mcast_rule);
> +	if (err)
> +		goto err_add;
> +
> +	ptr->macvlan = val;
> +	idx = hash_64(val, 8);
> +	hlist_add_head(&ptr->hlist, &ndev->macvlan_hash[idx]);
> +	return 0;
> +
> +err_add:
> +	kfree(ptr);
>   	return err;
>   }
>   
> -static void remove_fwd_to_tir(struct mlx5_vdpa_net *ndev)
> +static void mac_vlan_del(struct mlx5_vdpa_net *ndev, u8 *mac, u16 vlan, bool tagged)
>   {
> -	if (!ndev->rx_rule_ucast)
> +	struct macvlan_node *ptr;
> +
> +	ptr = mac_vlan_lookup(ndev, search_val(mac, vlan, tagged));
> +	if (!ptr)
>   		return;
>   
> -	mlx5_del_flow_rules(ndev->rx_rule_mcast);
> -	ndev->rx_rule_mcast = NULL;
> -	mlx5_del_flow_rules(ndev->rx_rule_ucast);
> -	ndev->rx_rule_ucast = NULL;
> +	hlist_del(&ptr->hlist);
> +	mlx5_vdpa_del_mac_vlan_rules(ndev, ptr->ucast_rule, ptr->mcast_rule);
> +	kfree(ptr);
> +}
> +
> +static void clear_mac_vlan_table(struct mlx5_vdpa_net *ndev)
> +{
> +	struct macvlan_node *pos;
> +	struct hlist_node *n;
> +	int i;
> +
> +	for (i = 0; i < MLX5V_MACVLAN_SIZE; i++) {
> +		hlist_for_each_entry_safe(pos, n, &ndev->macvlan_hash[i], hlist) {
> +			hlist_del(&pos->hlist);
> +			mlx5_vdpa_del_mac_vlan_rules(ndev, pos->ucast_rule, pos->mcast_rule);
> +			kfree(pos);
> +		}
> +	}
> +}
> +
> +static int setup_steering(struct mlx5_vdpa_net *ndev)
> +{
> +	struct mlx5_flow_table_attr ft_attr = {};
> +	struct mlx5_flow_namespace *ns;
> +	int err;
> +
> +	ft_attr.max_fte = MAX_STEERING_ENT;
> +	ft_attr.autogroup.max_num_groups = MAX_STEERING_GROUPS;
> +
> +	ns = mlx5_get_flow_namespace(ndev->mvdev.mdev, MLX5_FLOW_NAMESPACE_BYPASS);
> +	if (!ns) {
> +		mlx5_vdpa_warn(&ndev->mvdev, "failed to get flow namespace\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ndev->rxft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
> +	if (IS_ERR(ndev->rxft)) {
> +		mlx5_vdpa_warn(&ndev->mvdev, "failed to create flow table\n");
> +		return PTR_ERR(ndev->rxft);
> +	}
> +
> +	err = mac_vlan_add(ndev, ndev->config.mac, 0, false);
> +	if (err)
> +		goto err_add;
> +
> +	return 0;
> +
> +err_add:
> +	mlx5_destroy_flow_table(ndev->rxft);
> +	return err;
> +}
> +
> +static void teardown_steering(struct mlx5_vdpa_net *ndev)
> +{
> +	clear_mac_vlan_table(ndev);
>   	mlx5_destroy_flow_table(ndev->rxft);
>   }
>   
> @@ -1482,9 +1601,9 @@ static virtio_net_ctrl_ack handle_ctrl_mac(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>   
>   		/* Need recreate the flow table entry, so that the packet could forward back
>   		 */
> -		remove_fwd_to_tir(ndev);
> +		mac_vlan_del(ndev, ndev->config.mac, 0, false);
>   
> -		if (add_fwd_to_tir(ndev)) {
> +		if (mac_vlan_add(ndev, ndev->config.mac, 0, false)) {
>   			mlx5_vdpa_warn(mvdev, "failed to insert forward rules, try to restore\n");
>   
>   			/* Although it hardly run here, we still need double check */
> @@ -1508,7 +1627,7 @@ static virtio_net_ctrl_ack handle_ctrl_mac(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>   
>   			memcpy(ndev->config.mac, mac_back, ETH_ALEN);
>   
> -			if (add_fwd_to_tir(ndev))
> +			if (mac_vlan_add(ndev, ndev->config.mac, 0, false))
>   				mlx5_vdpa_warn(mvdev, "restore forward rules failed: insert forward rules failed\n");
>   
>   			break;
> @@ -1610,6 +1729,42 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>   	return status;
>   }
>   
> +static virtio_net_ctrl_ack handle_ctrl_vlan(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> +{
> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> +	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> +	struct mlx5_control_vq *cvq = &mvdev->cvq;
> +	struct virtio_net_ctrl_vlan vlan;
> +	size_t read;
> +	u16 id;
> +
> +	switch (cmd) {
> +	case VIRTIO_NET_CTRL_VLAN_ADD:
> +		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&vlan, sizeof(vlan));
> +		if (read != sizeof(vlan))
> +			break;
> +
> +		id = mlx5vdpa16_to_cpu(mvdev, vlan.id);
> +		if (mac_vlan_add(ndev, ndev->config.mac, id, true))
> +			break;


This may work now but I wonder if we had the plan to support 
VIRTIO_NET_F_CTRL_RX?

if $mac is not in $mac_table
     drop;
if $vlan is not in $vlan_table
     drop;

With that features we probably requires the dedicated vlan filters 
without a mac? Or do we want to a $mac * $vlans rules?

If yes, maybe it's better to decouple vlan filters from mac now.

Thanks


> +
> +		status = VIRTIO_NET_OK;
> +		break;
> +	case VIRTIO_NET_CTRL_VLAN_DEL:
> +		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&vlan, sizeof(vlan));
> +		if (read != sizeof(vlan))
> +			break;
> +
> +		id = mlx5vdpa16_to_cpu(mvdev, vlan.id);
> +		mac_vlan_del(ndev, ndev->config.mac, id, true);
> +		break;
> +	default:
> +	break;
> +}
> +
> +return status;
> +}
> +
>   static void mlx5_cvq_kick_handler(struct work_struct *work)
>   {
>   	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> @@ -1654,7 +1809,9 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>   		case VIRTIO_NET_CTRL_MQ:
>   			status = handle_ctrl_mq(mvdev, ctrl.cmd);
>   			break;
> -
> +		case VIRTIO_NET_CTRL_VLAN:
> +			status = handle_ctrl_vlan(mvdev, ctrl.cmd);
> +			break;
>   		default:
>   			break;
>   		}
> @@ -1913,6 +2070,7 @@ static u64 get_supported_features(struct mlx5_core_dev *mdev)
>   	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_MQ);
>   	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
>   	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_MTU);
> +	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_CTRL_VLAN);
>   
>   	return mlx_vdpa_features;
>   }
> @@ -2198,9 +2356,9 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
>   		goto err_tir;
>   	}
>   
> -	err = add_fwd_to_tir(ndev);
> +	err = setup_steering(ndev);
>   	if (err) {
> -		mlx5_vdpa_warn(mvdev, "add_fwd_to_tir\n");
> +		mlx5_vdpa_warn(mvdev, "setup_steering\n");
>   		goto err_fwd;
>   	}
>   	ndev->setup = true;
> @@ -2226,7 +2384,7 @@ static void teardown_driver(struct mlx5_vdpa_net *ndev)
>   	if (!ndev->setup)
>   		return;
>   
> -	remove_fwd_to_tir(ndev);
> +	teardown_steering(ndev);
>   	destroy_tir(ndev);
>   	destroy_rqt(ndev);
>   	teardown_virtqueues(ndev);

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

* RE: [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support
  2022-04-15  3:58     ` Jason Wang
  (?)
@ 2022-05-02  5:38     ` Eli Cohen
  2022-05-05  8:14         ` Jason Wang
  -1 siblings, 1 reply; 13+ messages in thread
From: Eli Cohen @ 2022-05-02  5:38 UTC (permalink / raw)
  To: Jason Wang, mst; +Cc: virtualization, linux-kernel, si-wei.liu

> From: Jason Wang <jasowang@redhat.com>
> Sent: Friday, April 15, 2022 6:59 AM
> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com
> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org; si-wei.liu@oracle.com
> Subject: Re: [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support
> 
> 
> 在 2022/4/11 20:29, Eli Cohen 写道:
> > Support HW offloaded filtering of MAC/VLAN packets.
> > To allow that, we add a handler to handle VLAN configurations coming
> > through the control VQ. Two operations are supported.
> >
> > 1. Adding VLAN - in this case, an entry will be added to the RX flow
> >     table that will allow the combination of the MAC/VLAN to be
> >     forwarded to the TIR.
> > 2. Removing VLAN - will remove the entry from the flow table,
> >     effectively blocking such packets from going through.
> >
> > Currently the control VQ does not propagate changes to the MAC of the
> > VLAN device so we always use the MAC of the parent device.
> >
> > Examples:
> > 1. Create vlan device:
> > $ ip link add link ens1 name ens1.8 type vlan id 8
> >
> > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > ---
> >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 274 +++++++++++++++++++++++-------
> >   1 file changed, 216 insertions(+), 58 deletions(-)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index 5aa6220c7129..f81f9a213ed2 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -48,6 +48,8 @@ MODULE_LICENSE("Dual BSD/GPL");
> >
> >   #define MLX5_FEATURE(_mvdev, _feature) (!!((_mvdev)->actual_features & BIT_ULL(_feature)))
> >
> > +#define MLX5V_UNTAGGED 0x1000
> > +
> >   struct mlx5_vdpa_net_resources {
> >   	u32 tisn;
> >   	u32 tdn;
> > @@ -143,6 +145,8 @@ static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
> >   	return idx <= mvdev->max_idx;
> >   }
> >
> > +#define MLX5V_MACVLAN_SIZE 256
> > +
> >   struct mlx5_vdpa_net {
> >   	struct mlx5_vdpa_dev mvdev;
> >   	struct mlx5_vdpa_net_resources res;
> > @@ -156,14 +160,20 @@ struct mlx5_vdpa_net {
> >   	 */
> >   	struct mutex reslock;
> >   	struct mlx5_flow_table *rxft;
> > -	struct mlx5_flow_handle *rx_rule_ucast;
> > -	struct mlx5_flow_handle *rx_rule_mcast;
> >   	bool setup;
> >   	u32 cur_num_vqs;
> >   	u32 rqt_size;
> >   	struct notifier_block nb;
> >   	struct vdpa_callback config_cb;
> >   	struct mlx5_vdpa_wq_ent cvq_ent;
> > +	struct hlist_head macvlan_hash[MLX5V_MACVLAN_SIZE];
> > +};
> > +
> > +struct macvlan_node {
> > +	struct hlist_node hlist;
> > +	struct mlx5_flow_handle *ucast_rule;
> > +	struct mlx5_flow_handle *mcast_rule;
> > +	u64 macvlan;
> >   };
> >
> >   static void free_resources(struct mlx5_vdpa_net *ndev);
> > @@ -1346,12 +1356,17 @@ static void destroy_tir(struct mlx5_vdpa_net *ndev)
> >   	mlx5_vdpa_destroy_tir(&ndev->mvdev, ndev->res.tirn);
> >   }
> >
> > -static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
> > +#define MAX_STEERING_ENT 0x8000
> > +#define MAX_STEERING_GROUPS 2
> > +
> > +static int mlx5_vdpa_add_mac_vlan_rules(struct mlx5_vdpa_net *ndev, u8 *mac,
> > +					u16 vid, bool tagged,
> > +					struct mlx5_flow_handle **ucast,
> > +					struct mlx5_flow_handle **mcast)
> >   {
> >   	struct mlx5_flow_destination dest = {};
> > -	struct mlx5_flow_table_attr ft_attr = {};
> >   	struct mlx5_flow_act flow_act = {};
> > -	struct mlx5_flow_namespace *ns;
> > +	struct mlx5_flow_handle *rule;
> >   	struct mlx5_flow_spec *spec;
> >   	void *headers_c;
> >   	void *headers_v;
> > @@ -1364,74 +1379,178 @@ static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
> >   		return -ENOMEM;
> >
> >   	spec->match_criteria_enable = MLX5_MATCH_OUTER_HEADERS;
> > -	ft_attr.max_fte = 2;
> > -	ft_attr.autogroup.max_num_groups = 2;
> > -
> > -	ns = mlx5_get_flow_namespace(ndev->mvdev.mdev, MLX5_FLOW_NAMESPACE_BYPASS);
> > -	if (!ns) {
> > -		mlx5_vdpa_warn(&ndev->mvdev, "failed to get flow namespace\n");
> > -		err = -EOPNOTSUPP;
> > -		goto err_ns;
> > -	}
> > -
> > -	ndev->rxft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
> > -	if (IS_ERR(ndev->rxft)) {
> > -		err = PTR_ERR(ndev->rxft);
> > -		goto err_ns;
> > -	}
> > -
> >   	headers_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, outer_headers);
> > -	dmac_c = MLX5_ADDR_OF(fte_match_param, headers_c, outer_headers.dmac_47_16);
> > -	memset(dmac_c, 0xff, ETH_ALEN);
> >   	headers_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, outer_headers);
> > +	dmac_c = MLX5_ADDR_OF(fte_match_param, headers_c, outer_headers.dmac_47_16);
> >   	dmac_v = MLX5_ADDR_OF(fte_match_param, headers_v, outer_headers.dmac_47_16);
> > -	ether_addr_copy(dmac_v, ndev->config.mac);
> > -
> > +	memset(dmac_c, 0xff, ETH_ALEN);
> > +	ether_addr_copy(dmac_v, mac);
> > +	MLX5_SET(fte_match_set_lyr_2_4, headers_c, cvlan_tag, 1);
> > +	if (tagged) {
> > +		MLX5_SET(fte_match_set_lyr_2_4, headers_v, cvlan_tag, 1);
> > +		MLX5_SET_TO_ONES(fte_match_set_lyr_2_4, headers_c, first_vid);
> > +		MLX5_SET(fte_match_set_lyr_2_4, headers_c, first_vid, vid);
> > +	}
> >   	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
> >   	dest.type = MLX5_FLOW_DESTINATION_TYPE_TIR;
> >   	dest.tir_num = ndev->res.tirn;
> > -	ndev->rx_rule_ucast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
> > +	rule = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
> > +	if (IS_ERR(rule))
> > +		return PTR_ERR(rule);
> >
> > -	if (IS_ERR(ndev->rx_rule_ucast)) {
> > -		err = PTR_ERR(ndev->rx_rule_ucast);
> > -		ndev->rx_rule_ucast = NULL;
> > -		goto err_rule_ucast;
> > -	}
> > +	*ucast = rule;
> >
> >   	memset(dmac_c, 0, ETH_ALEN);
> >   	memset(dmac_v, 0, ETH_ALEN);
> >   	dmac_c[0] = 1;
> >   	dmac_v[0] = 1;
> > -	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
> > -	ndev->rx_rule_mcast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
> > -	if (IS_ERR(ndev->rx_rule_mcast)) {
> > -		err = PTR_ERR(ndev->rx_rule_mcast);
> > -		ndev->rx_rule_mcast = NULL;
> > -		goto err_rule_mcast;
> > +	rule = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
> > +	kvfree(spec);
> > +	if (IS_ERR(rule)) {
> > +		err = PTR_ERR(rule);
> > +		goto err_mcast;
> >   	}
> >
> > -	kvfree(spec);
> > +	*mcast = rule;
> >   	return 0;
> >
> > -err_rule_mcast:
> > -	mlx5_del_flow_rules(ndev->rx_rule_ucast);
> > -	ndev->rx_rule_ucast = NULL;
> > -err_rule_ucast:
> > -	mlx5_destroy_flow_table(ndev->rxft);
> > -err_ns:
> > -	kvfree(spec);
> > +err_mcast:
> > +	mlx5_del_flow_rules(*ucast);
> > +	return err;
> > +}
> > +
> > +static void mlx5_vdpa_del_mac_vlan_rules(struct mlx5_vdpa_net *ndev,
> > +					 struct mlx5_flow_handle *ucast,
> > +					 struct mlx5_flow_handle *mcast)
> > +{
> > +	mlx5_del_flow_rules(ucast);
> > +	mlx5_del_flow_rules(mcast);
> > +}
> > +
> > +static u64 search_val(u8 *mac, u16 vlan, bool tagged)
> > +{
> > +	u64 val;
> > +
> > +	if (!tagged)
> > +		vlan = MLX5V_UNTAGGED;
> > +
> > +	val = (u64)vlan << 48 |
> > +	      (u64)mac[0] << 40 |
> > +	      (u64)mac[1] << 32 |
> > +	      (u64)mac[2] << 24 |
> > +	      (u64)mac[3] << 16 |
> > +	      (u64)mac[4] << 8 |
> > +	      (u64)mac[5];
> > +
> > +	return val;
> > +}
> > +
> > +static struct macvlan_node *mac_vlan_lookup(struct mlx5_vdpa_net *ndev, u64 value)
> > +{
> > +	struct macvlan_node *pos;
> > +	u32 idx;
> > +
> > +	idx = hash_64(value, 8); // tbd 8
> > +	hlist_for_each_entry(pos, &ndev->macvlan_hash[idx], hlist) {
> > +		if (pos->macvlan == value)
> > +			return pos;
> > +	}
> > +	return NULL;
> > +}
> > +
> > +static int mac_vlan_add(struct mlx5_vdpa_net *ndev, u8 *mac, u16 vlan, bool tagged) // vlan -> vid
> > +{
> 
> 
> I guess checkpatch may not be happy with such kind of comment.
Checkpatch did not catch this but I will fix.
> 
> 
> > +	struct macvlan_node *ptr;
> > +	u64 val;
> > +	u32 idx;
> > +	int err;
> > +
> > +	val = search_val(mac, vlan, tagged);
> > +	if (mac_vlan_lookup(ndev, val))
> > +		return -EEXIST;
> > +
> > +	ptr = kzalloc(sizeof(*ptr), GFP_KERNEL);
> > +	if (!ptr)
> > +		return -ENOMEM;
> > +
> > +	err = mlx5_vdpa_add_mac_vlan_rules(ndev, ndev->config.mac, vlan, tagged,
> > +					   &ptr->ucast_rule, &ptr->mcast_rule);
> > +	if (err)
> > +		goto err_add;
> > +
> > +	ptr->macvlan = val;
> > +	idx = hash_64(val, 8);
> > +	hlist_add_head(&ptr->hlist, &ndev->macvlan_hash[idx]);
> > +	return 0;
> > +
> > +err_add:
> > +	kfree(ptr);
> >   	return err;
> >   }
> >
> > -static void remove_fwd_to_tir(struct mlx5_vdpa_net *ndev)
> > +static void mac_vlan_del(struct mlx5_vdpa_net *ndev, u8 *mac, u16 vlan, bool tagged)
> >   {
> > -	if (!ndev->rx_rule_ucast)
> > +	struct macvlan_node *ptr;
> > +
> > +	ptr = mac_vlan_lookup(ndev, search_val(mac, vlan, tagged));
> > +	if (!ptr)
> >   		return;
> >
> > -	mlx5_del_flow_rules(ndev->rx_rule_mcast);
> > -	ndev->rx_rule_mcast = NULL;
> > -	mlx5_del_flow_rules(ndev->rx_rule_ucast);
> > -	ndev->rx_rule_ucast = NULL;
> > +	hlist_del(&ptr->hlist);
> > +	mlx5_vdpa_del_mac_vlan_rules(ndev, ptr->ucast_rule, ptr->mcast_rule);
> > +	kfree(ptr);
> > +}
> > +
> > +static void clear_mac_vlan_table(struct mlx5_vdpa_net *ndev)
> > +{
> > +	struct macvlan_node *pos;
> > +	struct hlist_node *n;
> > +	int i;
> > +
> > +	for (i = 0; i < MLX5V_MACVLAN_SIZE; i++) {
> > +		hlist_for_each_entry_safe(pos, n, &ndev->macvlan_hash[i], hlist) {
> > +			hlist_del(&pos->hlist);
> > +			mlx5_vdpa_del_mac_vlan_rules(ndev, pos->ucast_rule, pos->mcast_rule);
> > +			kfree(pos);
> > +		}
> > +	}
> > +}
> > +
> > +static int setup_steering(struct mlx5_vdpa_net *ndev)
> > +{
> > +	struct mlx5_flow_table_attr ft_attr = {};
> > +	struct mlx5_flow_namespace *ns;
> > +	int err;
> > +
> > +	ft_attr.max_fte = MAX_STEERING_ENT;
> > +	ft_attr.autogroup.max_num_groups = MAX_STEERING_GROUPS;
> > +
> > +	ns = mlx5_get_flow_namespace(ndev->mvdev.mdev, MLX5_FLOW_NAMESPACE_BYPASS);
> > +	if (!ns) {
> > +		mlx5_vdpa_warn(&ndev->mvdev, "failed to get flow namespace\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	ndev->rxft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
> > +	if (IS_ERR(ndev->rxft)) {
> > +		mlx5_vdpa_warn(&ndev->mvdev, "failed to create flow table\n");
> > +		return PTR_ERR(ndev->rxft);
> > +	}
> > +
> > +	err = mac_vlan_add(ndev, ndev->config.mac, 0, false);
> > +	if (err)
> > +		goto err_add;
> > +
> > +	return 0;
> > +
> > +err_add:
> > +	mlx5_destroy_flow_table(ndev->rxft);
> > +	return err;
> > +}
> > +
> > +static void teardown_steering(struct mlx5_vdpa_net *ndev)
> > +{
> > +	clear_mac_vlan_table(ndev);
> >   	mlx5_destroy_flow_table(ndev->rxft);
> >   }
> >
> > @@ -1482,9 +1601,9 @@ static virtio_net_ctrl_ack handle_ctrl_mac(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> >
> >   		/* Need recreate the flow table entry, so that the packet could forward back
> >   		 */
> > -		remove_fwd_to_tir(ndev);
> > +		mac_vlan_del(ndev, ndev->config.mac, 0, false);
> >
> > -		if (add_fwd_to_tir(ndev)) {
> > +		if (mac_vlan_add(ndev, ndev->config.mac, 0, false)) {
> >   			mlx5_vdpa_warn(mvdev, "failed to insert forward rules, try to restore\n");
> >
> >   			/* Although it hardly run here, we still need double check */
> > @@ -1508,7 +1627,7 @@ static virtio_net_ctrl_ack handle_ctrl_mac(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> >
> >   			memcpy(ndev->config.mac, mac_back, ETH_ALEN);
> >
> > -			if (add_fwd_to_tir(ndev))
> > +			if (mac_vlan_add(ndev, ndev->config.mac, 0, false))
> >   				mlx5_vdpa_warn(mvdev, "restore forward rules failed: insert forward rules failed\n");
> >
> >   			break;
> > @@ -1610,6 +1729,42 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> >   	return status;
> >   }
> >
> > +static virtio_net_ctrl_ack handle_ctrl_vlan(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> > +{
> > +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > +	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> > +	struct mlx5_control_vq *cvq = &mvdev->cvq;
> > +	struct virtio_net_ctrl_vlan vlan;
> > +	size_t read;
> > +	u16 id;
> > +
> > +	switch (cmd) {
> > +	case VIRTIO_NET_CTRL_VLAN_ADD:
> > +		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&vlan, sizeof(vlan));
> > +		if (read != sizeof(vlan))
> > +			break;
> > +
> > +		id = mlx5vdpa16_to_cpu(mvdev, vlan.id);
> > +		if (mac_vlan_add(ndev, ndev->config.mac, id, true))
> > +			break;
> 
> 
> This may work now but I wonder if we had the plan to support
> VIRTIO_NET_F_CTRL_RX?
> 
> if $mac is not in $mac_table
>      drop;
> if $vlan is not in $vlan_table
>      drop;
> 
> With that features we probably requires the dedicated vlan filters
> without a mac? Or do we want to a $mac * $vlans rules?
> 
> If yes, maybe it's better to decouple vlan filters from mac now.
> 

If we use dedicated filter tables for VLAN and MAC working in series,
we may not have full control over incoming traffic filtering.
For example, suppose we have VLAN table allowing v1 and v2 to go through,
and a MAC table allowing m1 and m2 to go through.

If we want only (v1, m1) and (v2, m2) to go through but not (v1, m2) or (v2, m1)
then it would not be possible to block the latter.

As I can see, the spec does not require that finesse but I wonder if this not
what real life requires.
If you think we should follow the spec let me know and will prepare a new version.

> Thanks
> 
> 
> > +
> > +		status = VIRTIO_NET_OK;
> > +		break;
> > +	case VIRTIO_NET_CTRL_VLAN_DEL:
> > +		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&vlan, sizeof(vlan));
> > +		if (read != sizeof(vlan))
> > +			break;
> > +
> > +		id = mlx5vdpa16_to_cpu(mvdev, vlan.id);
> > +		mac_vlan_del(ndev, ndev->config.mac, id, true);
> > +		break;
> > +	default:
> > +	break;
> > +}
> > +
> > +return status;
> > +}
> > +
> >   static void mlx5_cvq_kick_handler(struct work_struct *work)
> >   {
> >   	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> > @@ -1654,7 +1809,9 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
> >   		case VIRTIO_NET_CTRL_MQ:
> >   			status = handle_ctrl_mq(mvdev, ctrl.cmd);
> >   			break;
> > -
> > +		case VIRTIO_NET_CTRL_VLAN:
> > +			status = handle_ctrl_vlan(mvdev, ctrl.cmd);
> > +			break;
> >   		default:
> >   			break;
> >   		}
> > @@ -1913,6 +2070,7 @@ static u64 get_supported_features(struct mlx5_core_dev *mdev)
> >   	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_MQ);
> >   	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
> >   	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_MTU);
> > +	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_CTRL_VLAN);
> >
> >   	return mlx_vdpa_features;
> >   }
> > @@ -2198,9 +2356,9 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
> >   		goto err_tir;
> >   	}
> >
> > -	err = add_fwd_to_tir(ndev);
> > +	err = setup_steering(ndev);
> >   	if (err) {
> > -		mlx5_vdpa_warn(mvdev, "add_fwd_to_tir\n");
> > +		mlx5_vdpa_warn(mvdev, "setup_steering\n");
> >   		goto err_fwd;
> >   	}
> >   	ndev->setup = true;
> > @@ -2226,7 +2384,7 @@ static void teardown_driver(struct mlx5_vdpa_net *ndev)
> >   	if (!ndev->setup)
> >   		return;
> >
> > -	remove_fwd_to_tir(ndev);
> > +	teardown_steering(ndev);
> >   	destroy_tir(ndev);
> >   	destroy_rqt(ndev);
> >   	teardown_virtqueues(ndev);


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

* RE: [PATCH 2/3] virtio_net: Add control VQ struct to carry vlan id
  2022-04-15  3:00     ` Jason Wang
  (?)
@ 2022-05-02  5:39     ` Eli Cohen
  -1 siblings, 0 replies; 13+ messages in thread
From: Eli Cohen @ 2022-05-02  5:39 UTC (permalink / raw)
  To: Jason Wang, mst; +Cc: virtualization, linux-kernel, si-wei.liu

> From: Jason Wang <jasowang@redhat.com>
> Sent: Friday, April 15, 2022 6:01 AM
> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com
> Cc: virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org; si-wei.liu@oracle.com
> Subject: Re: [PATCH 2/3] virtio_net: Add control VQ struct to carry vlan id
> 
> 
> 在 2022/4/11 20:29, Eli Cohen 写道:
> > Add structure to define the payload of control VQ messages carrying the
> > configured vlan ID. It will be used in subsequent patches of this
> > series.
> >
> > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > ---
> >   include/uapi/linux/virtio_net.h | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> > index 3f55a4215f11..b94a405fa8d2 100644
> > --- a/include/uapi/linux/virtio_net.h
> > +++ b/include/uapi/linux/virtio_net.h
> > @@ -270,6 +270,9 @@ struct virtio_net_ctrl_mac {
> >   #define VIRTIO_NET_CTRL_VLAN       2
> >    #define VIRTIO_NET_CTRL_VLAN_ADD             0
> >    #define VIRTIO_NET_CTRL_VLAN_DEL             1
> > +struct virtio_net_ctrl_vlan {
> > +	__virtio16 id;
> > +};
> 
> 
> It looks to me there's no need to bother uAPI and we can simply use
> __virtio16 in patch 3?
> 

Sure

> Thanks
> 
> 
> >
> >   /*
> >    * Control link announce acknowledgement


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

* Re: [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support
  2022-05-02  5:38     ` Eli Cohen
@ 2022-05-05  8:14         ` Jason Wang
  0 siblings, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-05-05  8:14 UTC (permalink / raw)
  To: Eli Cohen, mst; +Cc: si-wei.liu, linux-kernel, virtualization


在 2022/5/2 13:38, Eli Cohen 写道:
>>> +static virtio_net_ctrl_ack handle_ctrl_vlan(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>>> +{
>>> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> +	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
>>> +	struct mlx5_control_vq *cvq = &mvdev->cvq;
>>> +	struct virtio_net_ctrl_vlan vlan;
>>> +	size_t read;
>>> +	u16 id;
>>> +
>>> +	switch (cmd) {
>>> +	case VIRTIO_NET_CTRL_VLAN_ADD:
>>> +		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&vlan, sizeof(vlan));
>>> +		if (read != sizeof(vlan))
>>> +			break;
>>> +
>>> +		id = mlx5vdpa16_to_cpu(mvdev, vlan.id);
>>> +		if (mac_vlan_add(ndev, ndev->config.mac, id, true))
>>> +			break;
>> This may work now but I wonder if we had the plan to support
>> VIRTIO_NET_F_CTRL_RX?
>>
>> if $mac is not in $mac_table
>>       drop;
>> if $vlan is not in $vlan_table
>>       drop;
>>
>> With that features we probably requires the dedicated vlan filters
>> without a mac? Or do we want to a $mac * $vlans rules?
>>
>> If yes, maybe it's better to decouple vlan filters from mac now.
>>
> If we use dedicated filter tables for VLAN and MAC working in series,
> we may not have full control over incoming traffic filtering.
> For example, suppose we have VLAN table allowing v1 and v2 to go through,
> and a MAC table allowing m1 and m2 to go through.
>
> If we want only (v1, m1) and (v2, m2) to go through but not (v1, m2) or (v2, m1)
> then it would not be possible to block the latter.


Yes, but this is currently how virtio-net work in the spec.


>
> As I can see, the spec does not require that finesse


Yes.


>   but I wonder if this not
> what real life requires.


Then we need to extend the spec.


> If you think we should follow the spec let me know and will prepare a new version.


It should be fine now. (But if we want too support CTRL_RX we need some 
refactoring on the codes).

So:

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks


>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

* Re: [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support
@ 2022-05-05  8:14         ` Jason Wang
  0 siblings, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-05-05  8:14 UTC (permalink / raw)
  To: Eli Cohen, mst; +Cc: virtualization, linux-kernel, si-wei.liu


在 2022/5/2 13:38, Eli Cohen 写道:
>>> +static virtio_net_ctrl_ack handle_ctrl_vlan(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>>> +{
>>> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> +	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
>>> +	struct mlx5_control_vq *cvq = &mvdev->cvq;
>>> +	struct virtio_net_ctrl_vlan vlan;
>>> +	size_t read;
>>> +	u16 id;
>>> +
>>> +	switch (cmd) {
>>> +	case VIRTIO_NET_CTRL_VLAN_ADD:
>>> +		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&vlan, sizeof(vlan));
>>> +		if (read != sizeof(vlan))
>>> +			break;
>>> +
>>> +		id = mlx5vdpa16_to_cpu(mvdev, vlan.id);
>>> +		if (mac_vlan_add(ndev, ndev->config.mac, id, true))
>>> +			break;
>> This may work now but I wonder if we had the plan to support
>> VIRTIO_NET_F_CTRL_RX?
>>
>> if $mac is not in $mac_table
>>       drop;
>> if $vlan is not in $vlan_table
>>       drop;
>>
>> With that features we probably requires the dedicated vlan filters
>> without a mac? Or do we want to a $mac * $vlans rules?
>>
>> If yes, maybe it's better to decouple vlan filters from mac now.
>>
> If we use dedicated filter tables for VLAN and MAC working in series,
> we may not have full control over incoming traffic filtering.
> For example, suppose we have VLAN table allowing v1 and v2 to go through,
> and a MAC table allowing m1 and m2 to go through.
>
> If we want only (v1, m1) and (v2, m2) to go through but not (v1, m2) or (v2, m1)
> then it would not be possible to block the latter.


Yes, but this is currently how virtio-net work in the spec.


>
> As I can see, the spec does not require that finesse


Yes.


>   but I wonder if this not
> what real life requires.


Then we need to extend the spec.


> If you think we should follow the spec let me know and will prepare a new version.


It should be fine now. (But if we want too support CTRL_RX we need some 
refactoring on the codes).

So:

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks


>


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

* [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support
  2022-04-11 12:27 [PATCH 0/3] mlx5_vdpa: Support MAC/VLAN haw offloading Eli Cohen
@ 2022-04-11 12:27 ` Eli Cohen
  0 siblings, 0 replies; 13+ messages in thread
From: Eli Cohen @ 2022-04-11 12:27 UTC (permalink / raw)
  To: mst, asowang; +Cc: virtualization, linux-kernel, si-wei.liu, Eli Cohen

Support HW offloaded filtering of MAC/VLAN packets.
To allow that, we add a handler to handle VLAN configurations coming
through the control VQ. Two operations are supported.

1. Adding VLAN - in this case, an entry will be added to the RX flow
   table that will allow the combination of the MAC/VLAN to be
   forwarded to the TIR.
2. Removing VLAN - will remove the entry from the flow table,
   effectively blocking such packets from going through.

Currently the control VQ does not propagate changes to the MAC of the
VLAN device so we always use the MAC of the parent device.

Examples:
1. Create vlan device:
$ ip link add link ens1 name ens1.8 type vlan id 8

Signed-off-by: Eli Cohen <elic@nvidia.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 274 +++++++++++++++++++++++-------
 1 file changed, 216 insertions(+), 58 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 5aa6220c7129..f81f9a213ed2 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -48,6 +48,8 @@ MODULE_LICENSE("Dual BSD/GPL");
 
 #define MLX5_FEATURE(_mvdev, _feature) (!!((_mvdev)->actual_features & BIT_ULL(_feature)))
 
+#define MLX5V_UNTAGGED 0x1000
+
 struct mlx5_vdpa_net_resources {
 	u32 tisn;
 	u32 tdn;
@@ -143,6 +145,8 @@ static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
 	return idx <= mvdev->max_idx;
 }
 
+#define MLX5V_MACVLAN_SIZE 256
+
 struct mlx5_vdpa_net {
 	struct mlx5_vdpa_dev mvdev;
 	struct mlx5_vdpa_net_resources res;
@@ -156,14 +160,20 @@ struct mlx5_vdpa_net {
 	 */
 	struct mutex reslock;
 	struct mlx5_flow_table *rxft;
-	struct mlx5_flow_handle *rx_rule_ucast;
-	struct mlx5_flow_handle *rx_rule_mcast;
 	bool setup;
 	u32 cur_num_vqs;
 	u32 rqt_size;
 	struct notifier_block nb;
 	struct vdpa_callback config_cb;
 	struct mlx5_vdpa_wq_ent cvq_ent;
+	struct hlist_head macvlan_hash[MLX5V_MACVLAN_SIZE];
+};
+
+struct macvlan_node {
+	struct hlist_node hlist;
+	struct mlx5_flow_handle *ucast_rule;
+	struct mlx5_flow_handle *mcast_rule;
+	u64 macvlan;
 };
 
 static void free_resources(struct mlx5_vdpa_net *ndev);
@@ -1346,12 +1356,17 @@ static void destroy_tir(struct mlx5_vdpa_net *ndev)
 	mlx5_vdpa_destroy_tir(&ndev->mvdev, ndev->res.tirn);
 }
 
-static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
+#define MAX_STEERING_ENT 0x8000
+#define MAX_STEERING_GROUPS 2
+
+static int mlx5_vdpa_add_mac_vlan_rules(struct mlx5_vdpa_net *ndev, u8 *mac,
+					u16 vid, bool tagged,
+					struct mlx5_flow_handle **ucast,
+					struct mlx5_flow_handle **mcast)
 {
 	struct mlx5_flow_destination dest = {};
-	struct mlx5_flow_table_attr ft_attr = {};
 	struct mlx5_flow_act flow_act = {};
-	struct mlx5_flow_namespace *ns;
+	struct mlx5_flow_handle *rule;
 	struct mlx5_flow_spec *spec;
 	void *headers_c;
 	void *headers_v;
@@ -1364,74 +1379,178 @@ static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
 		return -ENOMEM;
 
 	spec->match_criteria_enable = MLX5_MATCH_OUTER_HEADERS;
-	ft_attr.max_fte = 2;
-	ft_attr.autogroup.max_num_groups = 2;
-
-	ns = mlx5_get_flow_namespace(ndev->mvdev.mdev, MLX5_FLOW_NAMESPACE_BYPASS);
-	if (!ns) {
-		mlx5_vdpa_warn(&ndev->mvdev, "failed to get flow namespace\n");
-		err = -EOPNOTSUPP;
-		goto err_ns;
-	}
-
-	ndev->rxft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
-	if (IS_ERR(ndev->rxft)) {
-		err = PTR_ERR(ndev->rxft);
-		goto err_ns;
-	}
-
 	headers_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, outer_headers);
-	dmac_c = MLX5_ADDR_OF(fte_match_param, headers_c, outer_headers.dmac_47_16);
-	memset(dmac_c, 0xff, ETH_ALEN);
 	headers_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, outer_headers);
+	dmac_c = MLX5_ADDR_OF(fte_match_param, headers_c, outer_headers.dmac_47_16);
 	dmac_v = MLX5_ADDR_OF(fte_match_param, headers_v, outer_headers.dmac_47_16);
-	ether_addr_copy(dmac_v, ndev->config.mac);
-
+	memset(dmac_c, 0xff, ETH_ALEN);
+	ether_addr_copy(dmac_v, mac);
+	MLX5_SET(fte_match_set_lyr_2_4, headers_c, cvlan_tag, 1);
+	if (tagged) {
+		MLX5_SET(fte_match_set_lyr_2_4, headers_v, cvlan_tag, 1);
+		MLX5_SET_TO_ONES(fte_match_set_lyr_2_4, headers_c, first_vid);
+		MLX5_SET(fte_match_set_lyr_2_4, headers_c, first_vid, vid);
+	}
 	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
 	dest.type = MLX5_FLOW_DESTINATION_TYPE_TIR;
 	dest.tir_num = ndev->res.tirn;
-	ndev->rx_rule_ucast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
+	rule = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
+	if (IS_ERR(rule))
+		return PTR_ERR(rule);
 
-	if (IS_ERR(ndev->rx_rule_ucast)) {
-		err = PTR_ERR(ndev->rx_rule_ucast);
-		ndev->rx_rule_ucast = NULL;
-		goto err_rule_ucast;
-	}
+	*ucast = rule;
 
 	memset(dmac_c, 0, ETH_ALEN);
 	memset(dmac_v, 0, ETH_ALEN);
 	dmac_c[0] = 1;
 	dmac_v[0] = 1;
-	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
-	ndev->rx_rule_mcast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
-	if (IS_ERR(ndev->rx_rule_mcast)) {
-		err = PTR_ERR(ndev->rx_rule_mcast);
-		ndev->rx_rule_mcast = NULL;
-		goto err_rule_mcast;
+	rule = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act, &dest, 1);
+	kvfree(spec);
+	if (IS_ERR(rule)) {
+		err = PTR_ERR(rule);
+		goto err_mcast;
 	}
 
-	kvfree(spec);
+	*mcast = rule;
 	return 0;
 
-err_rule_mcast:
-	mlx5_del_flow_rules(ndev->rx_rule_ucast);
-	ndev->rx_rule_ucast = NULL;
-err_rule_ucast:
-	mlx5_destroy_flow_table(ndev->rxft);
-err_ns:
-	kvfree(spec);
+err_mcast:
+	mlx5_del_flow_rules(*ucast);
+	return err;
+}
+
+static void mlx5_vdpa_del_mac_vlan_rules(struct mlx5_vdpa_net *ndev,
+					 struct mlx5_flow_handle *ucast,
+					 struct mlx5_flow_handle *mcast)
+{
+	mlx5_del_flow_rules(ucast);
+	mlx5_del_flow_rules(mcast);
+}
+
+static u64 search_val(u8 *mac, u16 vlan, bool tagged)
+{
+	u64 val;
+
+	if (!tagged)
+		vlan = MLX5V_UNTAGGED;
+
+	val = (u64)vlan << 48 |
+	      (u64)mac[0] << 40 |
+	      (u64)mac[1] << 32 |
+	      (u64)mac[2] << 24 |
+	      (u64)mac[3] << 16 |
+	      (u64)mac[4] << 8 |
+	      (u64)mac[5];
+
+	return val;
+}
+
+static struct macvlan_node *mac_vlan_lookup(struct mlx5_vdpa_net *ndev, u64 value)
+{
+	struct macvlan_node *pos;
+	u32 idx;
+
+	idx = hash_64(value, 8); // tbd 8
+	hlist_for_each_entry(pos, &ndev->macvlan_hash[idx], hlist) {
+		if (pos->macvlan == value)
+			return pos;
+	}
+	return NULL;
+}
+
+static int mac_vlan_add(struct mlx5_vdpa_net *ndev, u8 *mac, u16 vlan, bool tagged) // vlan -> vid
+{
+	struct macvlan_node *ptr;
+	u64 val;
+	u32 idx;
+	int err;
+
+	val = search_val(mac, vlan, tagged);
+	if (mac_vlan_lookup(ndev, val))
+		return -EEXIST;
+
+	ptr = kzalloc(sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	err = mlx5_vdpa_add_mac_vlan_rules(ndev, ndev->config.mac, vlan, tagged,
+					   &ptr->ucast_rule, &ptr->mcast_rule);
+	if (err)
+		goto err_add;
+
+	ptr->macvlan = val;
+	idx = hash_64(val, 8);
+	hlist_add_head(&ptr->hlist, &ndev->macvlan_hash[idx]);
+	return 0;
+
+err_add:
+	kfree(ptr);
 	return err;
 }
 
-static void remove_fwd_to_tir(struct mlx5_vdpa_net *ndev)
+static void mac_vlan_del(struct mlx5_vdpa_net *ndev, u8 *mac, u16 vlan, bool tagged)
 {
-	if (!ndev->rx_rule_ucast)
+	struct macvlan_node *ptr;
+
+	ptr = mac_vlan_lookup(ndev, search_val(mac, vlan, tagged));
+	if (!ptr)
 		return;
 
-	mlx5_del_flow_rules(ndev->rx_rule_mcast);
-	ndev->rx_rule_mcast = NULL;
-	mlx5_del_flow_rules(ndev->rx_rule_ucast);
-	ndev->rx_rule_ucast = NULL;
+	hlist_del(&ptr->hlist);
+	mlx5_vdpa_del_mac_vlan_rules(ndev, ptr->ucast_rule, ptr->mcast_rule);
+	kfree(ptr);
+}
+
+static void clear_mac_vlan_table(struct mlx5_vdpa_net *ndev)
+{
+	struct macvlan_node *pos;
+	struct hlist_node *n;
+	int i;
+
+	for (i = 0; i < MLX5V_MACVLAN_SIZE; i++) {
+		hlist_for_each_entry_safe(pos, n, &ndev->macvlan_hash[i], hlist) {
+			hlist_del(&pos->hlist);
+			mlx5_vdpa_del_mac_vlan_rules(ndev, pos->ucast_rule, pos->mcast_rule);
+			kfree(pos);
+		}
+	}
+}
+
+static int setup_steering(struct mlx5_vdpa_net *ndev)
+{
+	struct mlx5_flow_table_attr ft_attr = {};
+	struct mlx5_flow_namespace *ns;
+	int err;
+
+	ft_attr.max_fte = MAX_STEERING_ENT;
+	ft_attr.autogroup.max_num_groups = MAX_STEERING_GROUPS;
+
+	ns = mlx5_get_flow_namespace(ndev->mvdev.mdev, MLX5_FLOW_NAMESPACE_BYPASS);
+	if (!ns) {
+		mlx5_vdpa_warn(&ndev->mvdev, "failed to get flow namespace\n");
+		return -EOPNOTSUPP;
+	}
+
+	ndev->rxft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
+	if (IS_ERR(ndev->rxft)) {
+		mlx5_vdpa_warn(&ndev->mvdev, "failed to create flow table\n");
+		return PTR_ERR(ndev->rxft);
+	}
+
+	err = mac_vlan_add(ndev, ndev->config.mac, 0, false);
+	if (err)
+		goto err_add;
+
+	return 0;
+
+err_add:
+	mlx5_destroy_flow_table(ndev->rxft);
+	return err;
+}
+
+static void teardown_steering(struct mlx5_vdpa_net *ndev)
+{
+	clear_mac_vlan_table(ndev);
 	mlx5_destroy_flow_table(ndev->rxft);
 }
 
@@ -1482,9 +1601,9 @@ static virtio_net_ctrl_ack handle_ctrl_mac(struct mlx5_vdpa_dev *mvdev, u8 cmd)
 
 		/* Need recreate the flow table entry, so that the packet could forward back
 		 */
-		remove_fwd_to_tir(ndev);
+		mac_vlan_del(ndev, ndev->config.mac, 0, false);
 
-		if (add_fwd_to_tir(ndev)) {
+		if (mac_vlan_add(ndev, ndev->config.mac, 0, false)) {
 			mlx5_vdpa_warn(mvdev, "failed to insert forward rules, try to restore\n");
 
 			/* Although it hardly run here, we still need double check */
@@ -1508,7 +1627,7 @@ static virtio_net_ctrl_ack handle_ctrl_mac(struct mlx5_vdpa_dev *mvdev, u8 cmd)
 
 			memcpy(ndev->config.mac, mac_back, ETH_ALEN);
 
-			if (add_fwd_to_tir(ndev))
+			if (mac_vlan_add(ndev, ndev->config.mac, 0, false))
 				mlx5_vdpa_warn(mvdev, "restore forward rules failed: insert forward rules failed\n");
 
 			break;
@@ -1610,6 +1729,42 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
 	return status;
 }
 
+static virtio_net_ctrl_ack handle_ctrl_vlan(struct mlx5_vdpa_dev *mvdev, u8 cmd)
+{
+	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
+	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
+	struct mlx5_control_vq *cvq = &mvdev->cvq;
+	struct virtio_net_ctrl_vlan vlan;
+	size_t read;
+	u16 id;
+
+	switch (cmd) {
+	case VIRTIO_NET_CTRL_VLAN_ADD:
+		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&vlan, sizeof(vlan));
+		if (read != sizeof(vlan))
+			break;
+
+		id = mlx5vdpa16_to_cpu(mvdev, vlan.id);
+		if (mac_vlan_add(ndev, ndev->config.mac, id, true))
+			break;
+
+		status = VIRTIO_NET_OK;
+		break;
+	case VIRTIO_NET_CTRL_VLAN_DEL:
+		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&vlan, sizeof(vlan));
+		if (read != sizeof(vlan))
+			break;
+
+		id = mlx5vdpa16_to_cpu(mvdev, vlan.id);
+		mac_vlan_del(ndev, ndev->config.mac, id, true);
+		break;
+	default:
+	break;
+}
+
+return status;
+}
+
 static void mlx5_cvq_kick_handler(struct work_struct *work)
 {
 	virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
@@ -1654,7 +1809,9 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 		case VIRTIO_NET_CTRL_MQ:
 			status = handle_ctrl_mq(mvdev, ctrl.cmd);
 			break;
-
+		case VIRTIO_NET_CTRL_VLAN:
+			status = handle_ctrl_vlan(mvdev, ctrl.cmd);
+			break;
 		default:
 			break;
 		}
@@ -1913,6 +2070,7 @@ static u64 get_supported_features(struct mlx5_core_dev *mdev)
 	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_MQ);
 	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
 	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_MTU);
+	mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_CTRL_VLAN);
 
 	return mlx_vdpa_features;
 }
@@ -2198,9 +2356,9 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
 		goto err_tir;
 	}
 
-	err = add_fwd_to_tir(ndev);
+	err = setup_steering(ndev);
 	if (err) {
-		mlx5_vdpa_warn(mvdev, "add_fwd_to_tir\n");
+		mlx5_vdpa_warn(mvdev, "setup_steering\n");
 		goto err_fwd;
 	}
 	ndev->setup = true;
@@ -2226,7 +2384,7 @@ static void teardown_driver(struct mlx5_vdpa_net *ndev)
 	if (!ndev->setup)
 		return;
 
-	remove_fwd_to_tir(ndev);
+	teardown_steering(ndev);
 	destroy_tir(ndev);
 	destroy_rqt(ndev);
 	teardown_virtqueues(ndev);
-- 
2.35.1


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

end of thread, other threads:[~2022-05-05  8:16 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-11 12:29 [PATCH 0/3 RESEND] mlx5_vdpa: Support MAC/VLAN haw offloading Eli Cohen
2022-04-11 12:29 ` [PATCH 1/3] vdpa/mlx5: Remove flow counter from steering Eli Cohen
2022-04-11 12:29 ` [PATCH 2/3] virtio_net: Add control VQ struct to carry vlan id Eli Cohen
2022-04-15  3:00   ` Jason Wang
2022-04-15  3:00     ` Jason Wang
2022-05-02  5:39     ` Eli Cohen
2022-04-11 12:29 ` [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support Eli Cohen
2022-04-15  3:58   ` Jason Wang
2022-04-15  3:58     ` Jason Wang
2022-05-02  5:38     ` Eli Cohen
2022-05-05  8:14       ` Jason Wang
2022-05-05  8:14         ` Jason Wang
  -- strict thread matches above, loose matches on Subject: below --
2022-04-11 12:27 [PATCH 0/3] mlx5_vdpa: Support MAC/VLAN haw offloading Eli Cohen
2022-04-11 12:27 ` [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support Eli Cohen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.