Re: [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support

[Jason Wang <jasowang@redhat.com>]

在 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


>