diff mbox

[dpdk-dev] net/ixgbe: fix tunnel id format error for FDIR

Message ID 1528189935-34943-4-git-send-email-wei.zhao1@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Zhao1, Wei June 5, 2018, 9:12 a.m. UTC 
  In cloud mode for FDIR, tunnel id should be set as protocol
request, the lower 8 bits should be set as reserved.

Fixes: 82fb702077f6 ("ixgbe: support new flow director modes for X550")
Fixes: 11777435c727 ("net/ixgbe: parse flow director filter")

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
---
 drivers/net/ixgbe/ixgbe_fdir.c | 2 +-
 drivers/net/ixgbe/ixgbe_flow.c | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

Comments

Wenzhuo Lu June 12, 2018, 5:10 a.m. UTC | #1 
Hi Wei,


> -----Original Message-----
> From: Zhao1, Wei
> Sent: Tuesday, June 5, 2018 5:12 PM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org; Zhao1, Wei
> <wei.zhao1@intel.com>
> Subject: [PATCH] net/ixgbe: fix tunnel id format error for FDIR
> 
> In cloud mode for FDIR, tunnel id should be set as protocol request, the
> lower 8 bits should be set as reserved.
To my opinion, the original implementation and this patch have different understanding of the 'tunnel_id' in ' struct rte_eth_tunnel_flow'. Originally it only means the tunnel id but not including the reserved 8 bits.
This patch means it should include the reserved bits. Maybe it makes things easier because the whole 4 bytes are big endian.
So, may I suggest to add some comments in ' struct rte_eth_tunnel_flow' to let the users know what the 'tunnel_id' really means?

> 
> Fixes: 82fb702077f6 ("ixgbe: support new flow director modes for X550")
> Fixes: 11777435c727 ("net/ixgbe: parse flow director filter")
> 
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_fdir.c | 2 +-
>  drivers/net/ixgbe/ixgbe_flow.c | 5 ++---
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_fdir.c b/drivers/net/ixgbe/ixgbe_fdir.c
> index d5e5179..67ab627 100644
> --- a/drivers/net/ixgbe/ixgbe_fdir.c
> +++ b/drivers/net/ixgbe/ixgbe_fdir.c
> @@ -774,7 +774,7 @@ ixgbe_fdir_filter_to_atr_input(const struct
> rte_eth_fdir_filter *fdir_filter,
>  		input->formatted.tunnel_type =
>  			fdir_filter->input.flow.tunnel_flow.tunnel_type;
>  		input->formatted.tni_vni =
> -			fdir_filter->input.flow.tunnel_flow.tunnel_id;
> +			fdir_filter->input.flow.tunnel_flow.tunnel_id >> 8;
>  	}
> 
>  	return 0;
> diff --git a/drivers/net/ixgbe/ixgbe_flow.c b/drivers/net/ixgbe/ixgbe_flow.c
> index eb0644c..64af777 100644
> --- a/drivers/net/ixgbe/ixgbe_flow.c
> +++ b/drivers/net/ixgbe/ixgbe_flow.c
> @@ -2489,8 +2489,7 @@ ixgbe_parse_fdir_filter_tunnel(const struct
> rte_flow_attr *attr,
>  			rte_memcpy(((uint8_t *)
>  				&rule->ixgbe_fdir.formatted.tni_vni + 1),
>  				vxlan_spec->vni, RTE_DIM(vxlan_spec->vni));
> -			rule->ixgbe_fdir.formatted.tni_vni =
> rte_be_to_cpu_32(
> -				rule->ixgbe_fdir.formatted.tni_vni);
> +			rule->ixgbe_fdir.formatted.tni_vni >>= 8;
>  		}
>  	}
> 
> @@ -2587,7 +2586,7 @@ ixgbe_parse_fdir_filter_tunnel(const struct
> rte_flow_attr *attr,
>  			/* tni is a 24-bits bit field */
>  			rte_memcpy(&rule->ixgbe_fdir.formatted.tni_vni,
>  			nvgre_spec->tni, RTE_DIM(nvgre_spec->tni));
> -			rule->ixgbe_fdir.formatted.tni_vni <<= 8;
> +			rule->ixgbe_fdir.formatted.tni_vni >>= 8;
>  		}
>  	}
> 
> --
> 2.7.5
Zhao1, Wei June 12, 2018, 7:49 a.m. UTC | #2 
Hi, Wenzhuo

> -----Original Message-----
> From: Lu, Wenzhuo
> Sent: Tuesday, June 12, 2018 1:10 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH] net/ixgbe: fix tunnel id format error for FDIR
> 
> Hi Wei,
> 
> 
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Tuesday, June 5, 2018 5:12 PM
> > To: dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org; Zhao1, Wei
> > <wei.zhao1@intel.com>
> > Subject: [PATCH] net/ixgbe: fix tunnel id format error for FDIR
> >
> > In cloud mode for FDIR, tunnel id should be set as protocol request,
> > the lower 8 bits should be set as reserved.
> To my opinion, the original implementation and this patch have different
> understanding of the 'tunnel_id' in ' struct rte_eth_tunnel_flow'. Originally it
> only means the tunnel id but not including the reserved 8 bits.
> This patch means it should include the reserved bits. Maybe it makes things
> easier because the whole 4 bytes are big endian.
> So, may I suggest to add some comments in ' struct rte_eth_tunnel_flow' to
> let the users know what the 'tunnel_id' really means?

The format from input for 'tunnel_id' should be network network byte order
And it also should not contain reserved 1 byte, but hardware HASH need the format include  reserved 1 byte
And in network byte order. So, this patch convert to that format.
I will add some comment in  function ixgbe_fdir_filter_to_atr_input and ixgbe_parse_fdir_filter_tunnel,
That will not Influence other NIC format for this struct member.
Is that ok?


> 
> >
> > Fixes: 82fb702077f6 ("ixgbe: support new flow director modes for
> > X550")
> > Fixes: 11777435c727 ("net/ixgbe: parse flow director filter")
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_fdir.c | 2 +-  drivers/net/ixgbe/ixgbe_flow.c
> > | 5 ++---
> >  2 files changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_fdir.c
> > b/drivers/net/ixgbe/ixgbe_fdir.c index d5e5179..67ab627 100644
> > --- a/drivers/net/ixgbe/ixgbe_fdir.c
> > +++ b/drivers/net/ixgbe/ixgbe_fdir.c
> > @@ -774,7 +774,7 @@ ixgbe_fdir_filter_to_atr_input(const struct
> > rte_eth_fdir_filter *fdir_filter,
> >  		input->formatted.tunnel_type =
> >  			fdir_filter->input.flow.tunnel_flow.tunnel_type;
> >  		input->formatted.tni_vni =
> > -			fdir_filter->input.flow.tunnel_flow.tunnel_id;
> > +			fdir_filter->input.flow.tunnel_flow.tunnel_id >> 8;
> >  	}
> >
> >  	return 0;
> > diff --git a/drivers/net/ixgbe/ixgbe_flow.c
> > b/drivers/net/ixgbe/ixgbe_flow.c index eb0644c..64af777 100644
> > --- a/drivers/net/ixgbe/ixgbe_flow.c
> > +++ b/drivers/net/ixgbe/ixgbe_flow.c
> > @@ -2489,8 +2489,7 @@ ixgbe_parse_fdir_filter_tunnel(const struct
> > rte_flow_attr *attr,
> >  			rte_memcpy(((uint8_t *)
> >  				&rule->ixgbe_fdir.formatted.tni_vni + 1),
> >  				vxlan_spec->vni, RTE_DIM(vxlan_spec->vni));
> > -			rule->ixgbe_fdir.formatted.tni_vni =
> > rte_be_to_cpu_32(
> > -				rule->ixgbe_fdir.formatted.tni_vni);
> > +			rule->ixgbe_fdir.formatted.tni_vni >>= 8;
> >  		}
> >  	}
> >
> > @@ -2587,7 +2586,7 @@ ixgbe_parse_fdir_filter_tunnel(const struct
> > rte_flow_attr *attr,
> >  			/* tni is a 24-bits bit field */
> >  			rte_memcpy(&rule->ixgbe_fdir.formatted.tni_vni,
> >  			nvgre_spec->tni, RTE_DIM(nvgre_spec->tni));
> > -			rule->ixgbe_fdir.formatted.tni_vni <<= 8;
> > +			rule->ixgbe_fdir.formatted.tni_vni >>= 8;
> >  		}
> >  	}
> >
> > --
> > 2.7.5
Wenzhuo Lu June 12, 2018, 8:39 a.m. UTC | #3 
Hi Wei,

> -----Original Message-----
> From: Zhao1, Wei
> Sent: Tuesday, June 12, 2018 3:49 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH] net/ixgbe: fix tunnel id format error for FDIR
> 
> 
> Hi, Wenzhuo
> 
> > -----Original Message-----
> > From: Lu, Wenzhuo
> > Sent: Tuesday, June 12, 2018 1:10 PM
> > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org
> > Subject: RE: [PATCH] net/ixgbe: fix tunnel id format error for FDIR
> >
> > Hi Wei,
> >
> >
> > > -----Original Message-----
> > > From: Zhao1, Wei
> > > Sent: Tuesday, June 5, 2018 5:12 PM
> > > To: dev@dpdk.org
> > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org; Zhao1, Wei
> > > <wei.zhao1@intel.com>
> > > Subject: [PATCH] net/ixgbe: fix tunnel id format error for FDIR
> > >
> > > In cloud mode for FDIR, tunnel id should be set as protocol request,
> > > the lower 8 bits should be set as reserved.
> > To my opinion, the original implementation and this patch have
> > different understanding of the 'tunnel_id' in ' struct
> > rte_eth_tunnel_flow'. Originally it only means the tunnel id but not
> including the reserved 8 bits.
> > This patch means it should include the reserved bits. Maybe it makes
> > things easier because the whole 4 bytes are big endian.
> > So, may I suggest to add some comments in ' struct
> > rte_eth_tunnel_flow' to let the users know what the 'tunnel_id' really
> means?
> 
> The format from input for 'tunnel_id' should be network network byte order
> And it also should not contain reserved 1 byte, but hardware HASH need the
> format include  reserved 1 byte And in network byte order. So, this patch
> convert to that format.
> I will add some comment in  function ixgbe_fdir_filter_to_atr_input and
> ixgbe_parse_fdir_filter_tunnel, That will not Influence other NIC format for
> this struct member.
> Is that ok?
It's not appropriate that different NIC has different understanding of the same info provided by APP.
And I found this tunnel_id is already used by other NICs. We cannot change the meaning. So I think this patch is not good.

> 
> 
> >
> > >
> > > Fixes: 82fb702077f6 ("ixgbe: support new flow director modes for
> > > X550")
> > > Fixes: 11777435c727 ("net/ixgbe: parse flow director filter")
> > >
> > > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > > ---
> > >  drivers/net/ixgbe/ixgbe_fdir.c | 2 +-
> > > drivers/net/ixgbe/ixgbe_flow.c
> > > | 5 ++---
> > >  2 files changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/ixgbe/ixgbe_fdir.c
> > > b/drivers/net/ixgbe/ixgbe_fdir.c index d5e5179..67ab627 100644
> > > --- a/drivers/net/ixgbe/ixgbe_fdir.c
> > > +++ b/drivers/net/ixgbe/ixgbe_fdir.c
> > > @@ -774,7 +774,7 @@ ixgbe_fdir_filter_to_atr_input(const struct
> > > rte_eth_fdir_filter *fdir_filter,
> > >  		input->formatted.tunnel_type =
> > >  			fdir_filter->input.flow.tunnel_flow.tunnel_type;
> > >  		input->formatted.tni_vni =
> > > -			fdir_filter->input.flow.tunnel_flow.tunnel_id;
> > > +			fdir_filter->input.flow.tunnel_flow.tunnel_id >> 8;
> > >  	}
> > >
> > >  	return 0;
> > > diff --git a/drivers/net/ixgbe/ixgbe_flow.c
> > > b/drivers/net/ixgbe/ixgbe_flow.c index eb0644c..64af777 100644
> > > --- a/drivers/net/ixgbe/ixgbe_flow.c
> > > +++ b/drivers/net/ixgbe/ixgbe_flow.c
> > > @@ -2489,8 +2489,7 @@ ixgbe_parse_fdir_filter_tunnel(const struct
> > > rte_flow_attr *attr,
> > >  			rte_memcpy(((uint8_t *)
> > >  				&rule->ixgbe_fdir.formatted.tni_vni + 1),
> > >  				vxlan_spec->vni, RTE_DIM(vxlan_spec->vni));
> > > -			rule->ixgbe_fdir.formatted.tni_vni =
> > > rte_be_to_cpu_32(
> > > -				rule->ixgbe_fdir.formatted.tni_vni);
> > > +			rule->ixgbe_fdir.formatted.tni_vni >>= 8;
> > >  		}
> > >  	}
> > >
> > > @@ -2587,7 +2586,7 @@ ixgbe_parse_fdir_filter_tunnel(const struct
> > > rte_flow_attr *attr,
> > >  			/* tni is a 24-bits bit field */
> > >  			rte_memcpy(&rule->ixgbe_fdir.formatted.tni_vni,
> > >  			nvgre_spec->tni, RTE_DIM(nvgre_spec->tni));
> > > -			rule->ixgbe_fdir.formatted.tni_vni <<= 8;
> > > +			rule->ixgbe_fdir.formatted.tni_vni >>= 8;
> > >  		}
> > >  	}
> > >
> > > --
> > > 2.7.5
Zhao1, Wei June 12, 2018, 8:43 a.m. UTC | #4 
Hi,wenzhuo

> -----Original Message-----
> From: Lu, Wenzhuo
> Sent: Tuesday, June 12, 2018 4:39 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH] net/ixgbe: fix tunnel id format error for FDIR
> 
> Hi Wei,
> 
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Tuesday, June 12, 2018 3:49 PM
> > To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org
> > Subject: RE: [PATCH] net/ixgbe: fix tunnel id format error for FDIR
> >
> >
> > Hi, Wenzhuo
> >
> > > -----Original Message-----
> > > From: Lu, Wenzhuo
> > > Sent: Tuesday, June 12, 2018 1:10 PM
> > > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > > Cc: stable@dpdk.org
> > > Subject: RE: [PATCH] net/ixgbe: fix tunnel id format error for FDIR
> > >
> > > Hi Wei,
> > >
> > >
> > > > -----Original Message-----
> > > > From: Zhao1, Wei
> > > > Sent: Tuesday, June 5, 2018 5:12 PM
> > > > To: dev@dpdk.org
> > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org; Zhao1,
> > > > Wei <wei.zhao1@intel.com>
> > > > Subject: [PATCH] net/ixgbe: fix tunnel id format error for FDIR
> > > >
> > > > In cloud mode for FDIR, tunnel id should be set as protocol
> > > > request, the lower 8 bits should be set as reserved.
> > > To my opinion, the original implementation and this patch have
> > > different understanding of the 'tunnel_id' in ' struct
> > > rte_eth_tunnel_flow'. Originally it only means the tunnel id but not
> > including the reserved 8 bits.
> > > This patch means it should include the reserved bits. Maybe it makes
> > > things easier because the whole 4 bytes are big endian.
> > > So, may I suggest to add some comments in ' struct
> > > rte_eth_tunnel_flow' to let the users know what the 'tunnel_id'
> > > really
> > means?
> >
> > The format from input for 'tunnel_id' should be network network byte
> > order And it also should not contain reserved 1 byte, but hardware
> > HASH need the format include  reserved 1 byte And in network byte
> > order. So, this patch convert to that format.
> > I will add some comment in  function ixgbe_fdir_filter_to_atr_input
> > and ixgbe_parse_fdir_filter_tunnel, That will not Influence other NIC
> > format for this struct member.
> > Is that ok?
> It's not appropriate that different NIC has different understanding of the
> same info provided by APP.
> And I found this tunnel_id is already used by other NICs. We cannot change
> the meaning. So I think this patch is not good.

We do not change the input format in testpmd level, but we MUST change in our IXGBE PMD, because this request by hardware HASH.
This is the most important root cause for IXGBE vxlan packet fail.  
> 
> >
> >
> > >
> > > >
> > > > Fixes: 82fb702077f6 ("ixgbe: support new flow director modes for
> > > > X550")
> > > > Fixes: 11777435c727 ("net/ixgbe: parse flow director filter")
> > > >
> > > > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > > > ---
> > > >  drivers/net/ixgbe/ixgbe_fdir.c | 2 +-
> > > > drivers/net/ixgbe/ixgbe_flow.c
> > > > | 5 ++---
> > > >  2 files changed, 3 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ixgbe/ixgbe_fdir.c
> > > > b/drivers/net/ixgbe/ixgbe_fdir.c index d5e5179..67ab627 100644
> > > > --- a/drivers/net/ixgbe/ixgbe_fdir.c
> > > > +++ b/drivers/net/ixgbe/ixgbe_fdir.c
> > > > @@ -774,7 +774,7 @@ ixgbe_fdir_filter_to_atr_input(const struct
> > > > rte_eth_fdir_filter *fdir_filter,
> > > >  		input->formatted.tunnel_type =
> > > >  			fdir_filter->input.flow.tunnel_flow.tunnel_type;
> > > >  		input->formatted.tni_vni =
> > > > -			fdir_filter->input.flow.tunnel_flow.tunnel_id;
> > > > +			fdir_filter->input.flow.tunnel_flow.tunnel_id >> 8;
> > > >  	}
> > > >
> > > >  	return 0;
> > > > diff --git a/drivers/net/ixgbe/ixgbe_flow.c
> > > > b/drivers/net/ixgbe/ixgbe_flow.c index eb0644c..64af777 100644
> > > > --- a/drivers/net/ixgbe/ixgbe_flow.c
> > > > +++ b/drivers/net/ixgbe/ixgbe_flow.c
> > > > @@ -2489,8 +2489,7 @@ ixgbe_parse_fdir_filter_tunnel(const struct
> > > > rte_flow_attr *attr,
> > > >  			rte_memcpy(((uint8_t *)
> > > >  				&rule->ixgbe_fdir.formatted.tni_vni + 1),
> > > >  				vxlan_spec->vni, RTE_DIM(vxlan_spec->vni));
> > > > -			rule->ixgbe_fdir.formatted.tni_vni =
> > > > rte_be_to_cpu_32(
> > > > -				rule->ixgbe_fdir.formatted.tni_vni);
> > > > +			rule->ixgbe_fdir.formatted.tni_vni >>= 8;
> > > >  		}
> > > >  	}
> > > >
> > > > @@ -2587,7 +2586,7 @@ ixgbe_parse_fdir_filter_tunnel(const struct
> > > > rte_flow_attr *attr,
> > > >  			/* tni is a 24-bits bit field */
> > > >  			rte_memcpy(&rule->ixgbe_fdir.formatted.tni_vni,
> > > >  			nvgre_spec->tni, RTE_DIM(nvgre_spec->tni));
> > > > -			rule->ixgbe_fdir.formatted.tni_vni <<= 8;
> > > > +			rule->ixgbe_fdir.formatted.tni_vni >>= 8;
> > > >  		}
> > > >  	}
> > > >
> > > > --
> > > > 2.7.5
diff mbox

Patch

diff --git a/drivers/net/ixgbe/ixgbe_fdir.c b/drivers/net/ixgbe/ixgbe_fdir.c
index d5e5179..67ab627 100644
--- a/drivers/net/ixgbe/ixgbe_fdir.c
+++ b/drivers/net/ixgbe/ixgbe_fdir.c
@@ -774,7 +774,7 @@  ixgbe_fdir_filter_to_atr_input(const struct rte_eth_fdir_filter *fdir_filter,
 		input->formatted.tunnel_type =
 			fdir_filter->input.flow.tunnel_flow.tunnel_type;
 		input->formatted.tni_vni =
-			fdir_filter->input.flow.tunnel_flow.tunnel_id;
+			fdir_filter->input.flow.tunnel_flow.tunnel_id >> 8;
 	}
 
 	return 0;
diff --git a/drivers/net/ixgbe/ixgbe_flow.c b/drivers/net/ixgbe/ixgbe_flow.c
index eb0644c..64af777 100644
--- a/drivers/net/ixgbe/ixgbe_flow.c
+++ b/drivers/net/ixgbe/ixgbe_flow.c
@@ -2489,8 +2489,7 @@  ixgbe_parse_fdir_filter_tunnel(const struct rte_flow_attr *attr,
 			rte_memcpy(((uint8_t *)
 				&rule->ixgbe_fdir.formatted.tni_vni + 1),
 				vxlan_spec->vni, RTE_DIM(vxlan_spec->vni));
-			rule->ixgbe_fdir.formatted.tni_vni = rte_be_to_cpu_32(
-				rule->ixgbe_fdir.formatted.tni_vni);
+			rule->ixgbe_fdir.formatted.tni_vni >>= 8;
 		}
 	}
 
@@ -2587,7 +2586,7 @@  ixgbe_parse_fdir_filter_tunnel(const struct rte_flow_attr *attr,
 			/* tni is a 24-bits bit field */
 			rte_memcpy(&rule->ixgbe_fdir.formatted.tni_vni,
 			nvgre_spec->tni, RTE_DIM(nvgre_spec->tni));
-			rule->ixgbe_fdir.formatted.tni_vni <<= 8;
+			rule->ixgbe_fdir.formatted.tni_vni >>= 8;
 		}
 	}