Message-ID: <DM8PR12MB540032F367E9DB4C277AB946ABC19@DM8PR12MB5400.namprd12.prod.outlook.com>
Date:   Mon, 2 May 2022 05:38:40 +0000
From:   Eli Cohen <elic@...dia.com>
To:     Jason Wang <jasowang@...hat.com>, "mst@...hat.com" <mst@...hat.com>
CC:     "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "si-wei.liu@...cle.com" <si-wei.liu@...cle.com>
Subject: RE: [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support

> From: Jason Wang <jasowang@...hat.com>
> Sent: Friday, April 15, 2022 6:59 AM
> To: Eli Cohen <elic@...dia.com>; mst@...hat.com
> Cc: virtualization@...ts.linux-foundation.org; linux-kernel@...r.kernel.org; si-wei.liu@...cle.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@...dia.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);

Powered by blists - more mailing lists