diff mbox series

[1/4] vdpa/mlx5: Fix rule forwarding VLAN to TIR

Message ID 20221018111232.4021-2-elic@nvidia.com
State New
Headers
Series vdpa:/mlx5: Add debugfs subtree |

Commit Message

Eli Cohen Oct. 18, 2022, 11:12 a.m. UTC 
  Set the VLAN id to the header values field instead of overwriting the
headers criteria field.

Before this fix, VLAN filtering would not really work and tagged packets
would be forwarded unfiltered to the TIR.

Fixes: baf2ad3f6a98 ("vdpa/mlx5: Add RX MAC VLAN filter support")

Signed-off-by: Eli Cohen <elic@nvidia.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Si-Wei Liu Oct. 18, 2022, 7:20 p.m. UTC | #1 
Hi Eli,

It's not for this patch but something related, so just a friendly 
heads-up. I haven't validated the VLAN tagging behavior yet for mlx5 
vdpa, but from my quick read of the code it doesn't seem it 
differentiates the case with and without VIRTIO_NET_F_CTRL_VLAN, to be 
compatible/compliant with what's been implemented in QEMU software (a 
spec addendum was filed as requested by Michael):

https://github.com/oasis-tcs/virtio-spec/issues/147

- when VIRTIO_NET_F_CTRL_VLAN is negotiated, the device starts with
all VLANs filtered (meaning only untagged traffic can be received,
and traffic with VLAN tag will be dropped).

- when VIRTIO_NET_F_CTRL_VLAN is not negotiated, all traffic including
untagged and tagged can be received.

Can you please help check if we need further fix in terms of VLAN tagging?

Thanks,
-Siwei


On 10/18/2022 4:12 AM, Eli Cohen wrote:
> Set the VLAN id to the header values field instead of overwriting the
> headers criteria field.
>
> Before this fix, VLAN filtering would not really work and tagged packets
> would be forwarded unfiltered to the TIR.
>
> Fixes: baf2ad3f6a98 ("vdpa/mlx5: Add RX MAC VLAN filter support")
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>

> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 90913365def4..dd29fdfc24ed 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1472,7 +1472,7 @@ static int mlx5_vdpa_add_mac_vlan_rules(struct mlx5_vdpa_net *ndev, u8 *mac,
>   	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);
> +		MLX5_SET(fte_match_set_lyr_2_4, headers_v, first_vid, vid);
>   	}
>   	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
>   	dest.type = MLX5_FLOW_DESTINATION_TYPE_TIR;
Eli Cohen Oct. 19, 2022, 5:31 a.m. UTC | #2 
> From: Si-Wei Liu <si-wei.liu@oracle.com>
> Sent: Tuesday, 18 October 2022 22:21
> To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com;
> linux-kernel@vger.kernel.org; virtualization@lists.linux-foundation.org
> Cc: eperezma@redhat.com; lulu@redhat.com
> Subject: Re: [PATCH 1/4] vdpa/mlx5: Fix rule forwarding VLAN to TIR
> 
> Hi Eli,
> 
> It's not for this patch but something related, so just a friendly
> heads-up. I haven't validated the VLAN tagging behavior yet for mlx5
> vdpa, but from my quick read of the code it doesn't seem it
> differentiates the case with and without VIRTIO_NET_F_CTRL_VLAN, to be
> compatible/compliant with what's been implemented in QEMU software (a
> spec addendum was filed as requested by Michael):
> 
> https://github.com/oasis-tcs/virtio-spec/issues/147
> 
> - when VIRTIO_NET_F_CTRL_VLAN is negotiated, the device starts with
> all VLANs filtered (meaning only untagged traffic can be received,
> and traffic with VLAN tag will be dropped).
> 
> - when VIRTIO_NET_F_CTRL_VLAN is not negotiated, all traffic including
> untagged and tagged can be received.
> 
> Can you please help check if we need further fix in terms of VLAN tagging?
> 

Sure. It's broken today. I will fix this to conform to the above requirements and send V1.

> Thanks,
> -Siwei
> 
> 
> On 10/18/2022 4:12 AM, Eli Cohen wrote:
> > Set the VLAN id to the header values field instead of overwriting the
> > headers criteria field.
> >
> > Before this fix, VLAN filtering would not really work and tagged packets
> > would be forwarded unfiltered to the TIR.
> >
> > Fixes: baf2ad3f6a98 ("vdpa/mlx5: Add RX MAC VLAN filter support")
> >
> > Signed-off-by: Eli Cohen <elic@nvidia.com>
> Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> 
> > ---
> >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index 90913365def4..dd29fdfc24ed 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -1472,7 +1472,7 @@ static int mlx5_vdpa_add_mac_vlan_rules(struct
> mlx5_vdpa_net *ndev, u8 *mac,
> >   	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);
> > +		MLX5_SET(fte_match_set_lyr_2_4, headers_v, first_vid, vid);
> >   	}
> >   	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
> >   	dest.type = MLX5_FLOW_DESTINATION_TYPE_TIR;
Michael S. Tsirkin Oct. 19, 2022, 5:34 a.m. UTC | #3 
On Wed, Oct 19, 2022 at 05:31:48AM +0000, Eli Cohen wrote:
> > From: Si-Wei Liu <si-wei.liu@oracle.com>
> > Sent: Tuesday, 18 October 2022 22:21
> > To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com;
> > linux-kernel@vger.kernel.org; virtualization@lists.linux-foundation.org
> > Cc: eperezma@redhat.com; lulu@redhat.com
> > Subject: Re: [PATCH 1/4] vdpa/mlx5: Fix rule forwarding VLAN to TIR
> > 
> > Hi Eli,
> > 
> > It's not for this patch but something related, so just a friendly
> > heads-up. I haven't validated the VLAN tagging behavior yet for mlx5
> > vdpa, but from my quick read of the code it doesn't seem it
> > differentiates the case with and without VIRTIO_NET_F_CTRL_VLAN, to be
> > compatible/compliant with what's been implemented in QEMU software (a
> > spec addendum was filed as requested by Michael):
> > 
> > https://github.com/oasis-tcs/virtio-spec/issues/147
> > 
> > - when VIRTIO_NET_F_CTRL_VLAN is negotiated, the device starts with
> > all VLANs filtered (meaning only untagged traffic can be received,
> > and traffic with VLAN tag will be dropped).
> > 
> > - when VIRTIO_NET_F_CTRL_VLAN is not negotiated, all traffic including
> > untagged and tagged can be received.
> > 
> > Can you please help check if we need further fix in terms of VLAN tagging?
> > 
> 
> Sure. It's broken today. I will fix this to conform to the above requirements and send V1.

Did you mean v2?

> > Thanks,
> > -Siwei
> > 
> > 
> > On 10/18/2022 4:12 AM, Eli Cohen wrote:
> > > Set the VLAN id to the header values field instead of overwriting the
> > > headers criteria field.
> > >
> > > Before this fix, VLAN filtering would not really work and tagged packets
> > > would be forwarded unfiltered to the TIR.
> > >
> > > Fixes: baf2ad3f6a98 ("vdpa/mlx5: Add RX MAC VLAN filter support")
> > >
> > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> > 
> > > ---
> > >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index 90913365def4..dd29fdfc24ed 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -1472,7 +1472,7 @@ static int mlx5_vdpa_add_mac_vlan_rules(struct
> > mlx5_vdpa_net *ndev, u8 *mac,
> > >   	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);
> > > +		MLX5_SET(fte_match_set_lyr_2_4, headers_v, first_vid, vid);
> > >   	}
> > >   	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
> > >   	dest.type = MLX5_FLOW_DESTINATION_TYPE_TIR;
>
Eli Cohen Oct. 19, 2022, 5:36 a.m. UTC | #4 
> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, 19 October 2022 8:34
> To: Eli Cohen <elic@nvidia.com>
> Cc: Si-Wei Liu <si-wei.liu@oracle.com>; jasowang@redhat.com; linux-
> kernel@vger.kernel.org; virtualization@lists.linux-foundation.org;
> eperezma@redhat.com; lulu@redhat.com
> Subject: Re: [PATCH 1/4] vdpa/mlx5: Fix rule forwarding VLAN to TIR
> 
> On Wed, Oct 19, 2022 at 05:31:48AM +0000, Eli Cohen wrote:
> > > From: Si-Wei Liu <si-wei.liu@oracle.com>
> > > Sent: Tuesday, 18 October 2022 22:21
> > > To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com;
> > > linux-kernel@vger.kernel.org; virtualization@lists.linux-foundation.org
> > > Cc: eperezma@redhat.com; lulu@redhat.com
> > > Subject: Re: [PATCH 1/4] vdpa/mlx5: Fix rule forwarding VLAN to TIR
> > >
> > > Hi Eli,
> > >
> > > It's not for this patch but something related, so just a friendly
> > > heads-up. I haven't validated the VLAN tagging behavior yet for mlx5
> > > vdpa, but from my quick read of the code it doesn't seem it
> > > differentiates the case with and without VIRTIO_NET_F_CTRL_VLAN, to be
> > > compatible/compliant with what's been implemented in QEMU software (a
> > > spec addendum was filed as requested by Michael):
> > >
> > > https://github.com/oasis-tcs/virtio-spec/issues/147
> > >
> > > - when VIRTIO_NET_F_CTRL_VLAN is negotiated, the device starts with
> > > all VLANs filtered (meaning only untagged traffic can be received,
> > > and traffic with VLAN tag will be dropped).
> > >
> > > - when VIRTIO_NET_F_CTRL_VLAN is not negotiated, all traffic including
> > > untagged and tagged can be received.
> > >
> > > Can you please help check if we need further fix in terms of VLAN tagging?
> > >
> >
> > Sure. It's broken today. I will fix this to conform to the above requirements
> and send V1.
> 
> Did you mean v2?
> 

I count from 0 and have been following this scheme but I can make it v2 if that's the norm.

> > > Thanks,
> > > -Siwei
> > >
> > >
> > > On 10/18/2022 4:12 AM, Eli Cohen wrote:
> > > > Set the VLAN id to the header values field instead of overwriting the
> > > > headers criteria field.
> > > >
> > > > Before this fix, VLAN filtering would not really work and tagged packets
> > > > would be forwarded unfiltered to the TIR.
> > > >
> > > > Fixes: baf2ad3f6a98 ("vdpa/mlx5: Add RX MAC VLAN filter support")
> > > >
> > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> > >
> > > > ---
> > > >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > index 90913365def4..dd29fdfc24ed 100644
> > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > @@ -1472,7 +1472,7 @@ static int
> mlx5_vdpa_add_mac_vlan_rules(struct
> > > mlx5_vdpa_net *ndev, u8 *mac,
> > > >   	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);
> > > > +		MLX5_SET(fte_match_set_lyr_2_4, headers_v, first_vid, vid);
> > > >   	}
> > > >   	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
> > > >   	dest.type = MLX5_FLOW_DESTINATION_TYPE_TIR;
> >
Michael S. Tsirkin Oct. 19, 2022, 5:39 a.m. UTC | #5 
On Wed, Oct 19, 2022 at 05:36:17AM +0000, Eli Cohen wrote:
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, 19 October 2022 8:34
> > To: Eli Cohen <elic@nvidia.com>
> > Cc: Si-Wei Liu <si-wei.liu@oracle.com>; jasowang@redhat.com; linux-
> > kernel@vger.kernel.org; virtualization@lists.linux-foundation.org;
> > eperezma@redhat.com; lulu@redhat.com
> > Subject: Re: [PATCH 1/4] vdpa/mlx5: Fix rule forwarding VLAN to TIR
> > 
> > On Wed, Oct 19, 2022 at 05:31:48AM +0000, Eli Cohen wrote:
> > > > From: Si-Wei Liu <si-wei.liu@oracle.com>
> > > > Sent: Tuesday, 18 October 2022 22:21
> > > > To: Eli Cohen <elic@nvidia.com>; mst@redhat.com; jasowang@redhat.com;
> > > > linux-kernel@vger.kernel.org; virtualization@lists.linux-foundation.org
> > > > Cc: eperezma@redhat.com; lulu@redhat.com
> > > > Subject: Re: [PATCH 1/4] vdpa/mlx5: Fix rule forwarding VLAN to TIR
> > > >
> > > > Hi Eli,
> > > >
> > > > It's not for this patch but something related, so just a friendly
> > > > heads-up. I haven't validated the VLAN tagging behavior yet for mlx5
> > > > vdpa, but from my quick read of the code it doesn't seem it
> > > > differentiates the case with and without VIRTIO_NET_F_CTRL_VLAN, to be
> > > > compatible/compliant with what's been implemented in QEMU software (a
> > > > spec addendum was filed as requested by Michael):
> > > >
> > > > https://github.com/oasis-tcs/virtio-spec/issues/147
> > > >
> > > > - when VIRTIO_NET_F_CTRL_VLAN is negotiated, the device starts with
> > > > all VLANs filtered (meaning only untagged traffic can be received,
> > > > and traffic with VLAN tag will be dropped).
> > > >
> > > > - when VIRTIO_NET_F_CTRL_VLAN is not negotiated, all traffic including
> > > > untagged and tagged can be received.
> > > >
> > > > Can you please help check if we need further fix in terms of VLAN tagging?
> > > >
> > >
> > > Sure. It's broken today. I will fix this to conform to the above requirements
> > and send V1.
> > 
> > Did you mean v2?
> > 
> 
> I count from 0 and have been following this scheme but I can make it v2 if that's the norm.

Yes, most people seem to count patches from 1 so [PATCH] is followed by
[PATCH v2].  I don't know why. But it doesn't matter much - I just
wanted to understand whether you will be sending a new version of this
patchset. I know now.


> > > > Thanks,
> > > > -Siwei
> > > >
> > > >
> > > > On 10/18/2022 4:12 AM, Eli Cohen wrote:
> > > > > Set the VLAN id to the header values field instead of overwriting the
> > > > > headers criteria field.
> > > > >
> > > > > Before this fix, VLAN filtering would not really work and tagged packets
> > > > > would be forwarded unfiltered to the TIR.
> > > > >
> > > > > Fixes: baf2ad3f6a98 ("vdpa/mlx5: Add RX MAC VLAN filter support")
> > > > >
> > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> > > >
> > > > > ---
> > > > >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > index 90913365def4..dd29fdfc24ed 100644
> > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > @@ -1472,7 +1472,7 @@ static int
> > mlx5_vdpa_add_mac_vlan_rules(struct
> > > > mlx5_vdpa_net *ndev, u8 *mac,
> > > > >   	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);
> > > > > +		MLX5_SET(fte_match_set_lyr_2_4, headers_v, first_vid, vid);
> > > > >   	}
> > > > >   	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
> > > > >   	dest.type = MLX5_FLOW_DESTINATION_TYPE_TIR;
> > >
>
diff mbox series

Patch

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 90913365def4..dd29fdfc24ed 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1472,7 +1472,7 @@  static int mlx5_vdpa_add_mac_vlan_rules(struct mlx5_vdpa_net *ndev, u8 *mac,
 	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);
+		MLX5_SET(fte_match_set_lyr_2_4, headers_v, first_vid, vid);
 	}
 	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
 	dest.type = MLX5_FLOW_DESTINATION_TYPE_TIR;