[dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation

[dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation

[Dekel Peled]

Set MPLS label value in appropriate location at mplsoudp_encap_conf,
so it is correctly copied to rte_flow_item_mpls.

Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
Cc: orika@mellanox.com

Signed-off-by: Dekel Peled <dekelp@mellanox.com>
---
 app/test-pmd/cmdline.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 1275074..40e64cc 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -15804,10 +15804,10 @@ static void cmd_set_mplsoudp_encap_parsed(void *parsed_result,
 	struct cmd_set_mplsoudp_encap_result *res = parsed_result;
 	union {
 		uint32_t mplsoudp_label;
-		uint8_t label[3];
+		uint8_t label[4];
 	} id = {
 		.mplsoudp_label =
-			rte_cpu_to_be_32(res->label) & RTE_BE32(0x00ffffff),
+			rte_cpu_to_be_32(res->label<<4) & RTE_BE32(0x00ffffff),
 	};
 
 	if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0)
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation

[Ori Kam]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Dekel Peled
> Sent: Monday, November 19, 2018 6:55 PM
> To: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> bernard.iremonger@intel.com
> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; Dekel Peled <dekelp@mellanox.com>
> Subject: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
> 
> Set MPLS label value in appropriate location at mplsoudp_encap_conf,
> so it is correctly copied to rte_flow_item_mpls.
> 
> Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
> Cc: orika@mellanox.com
> 
> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> ---
>  app/test-pmd/cmdline.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 1275074..40e64cc 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -15804,10 +15804,10 @@ static void
> cmd_set_mplsoudp_encap_parsed(void *parsed_result,
>  	struct cmd_set_mplsoudp_encap_result *res = parsed_result;
>  	union {
>  		uint32_t mplsoudp_label;
> -		uint8_t label[3];
> +		uint8_t label[4];
>  	} id = {
>  		.mplsoudp_label =
> -			rte_cpu_to_be_32(res->label) & RTE_BE32(0x00ffffff),
> +			rte_cpu_to_be_32(res->label<<4) &
> RTE_BE32(0x00ffffff),
>  	};
> 
>  	if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0)
> --
> 1.8.3.1

Acked-by: Ori Kam <orika@mellanox.com>

Thanks,
Ori

Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation

[Ferruh Yigit]

On 11/20/2018 8:23 AM, Ori Kam wrote:
> 
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Dekel Peled
>> Sent: Monday, November 19, 2018 6:55 PM
>> To: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
>> bernard.iremonger@intel.com
>> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; Shahaf Shuler
>> <shahafs@mellanox.com>; Dekel Peled <dekelp@mellanox.com>
>> Subject: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
>>
>> Set MPLS label value in appropriate location at mplsoudp_encap_conf,
>> so it is correctly copied to rte_flow_item_mpls.
>>
>> Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
>> Cc: orika@mellanox.com
>>
>> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
>> ---
>>  app/test-pmd/cmdline.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 1275074..40e64cc 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -15804,10 +15804,10 @@ static void
>> cmd_set_mplsoudp_encap_parsed(void *parsed_result,
>>  	struct cmd_set_mplsoudp_encap_result *res = parsed_result;
>>  	union {
>>  		uint32_t mplsoudp_label;
>> -		uint8_t label[3];
>> +		uint8_t label[4];
>>  	} id = {
>>  		.mplsoudp_label =
>> -			rte_cpu_to_be_32(res->label) & RTE_BE32(0x00ffffff),
>> +			rte_cpu_to_be_32(res->label<<4) &
>> RTE_BE32(0x00ffffff),
>>  	};
>>
>>  	if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0)
>> --
>> 1.8.3.1
> 
> Acked-by: Ori Kam <orika@mellanox.com>

Hi Ori, Dekel,

What is the scope of this patch? Briefly how critical it is and what will be
broken and what is exposure of it?

Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation

[Ori Kam]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, November 21, 2018 5:19 PM
> To: Ori Kam <orika@mellanox.com>; Dekel Peled <dekelp@mellanox.com>;
> wenzhuo.lu@intel.com; jingjing.wu@intel.com; bernard.iremonger@intel.com
> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
> 
> On 11/20/2018 8:23 AM, Ori Kam wrote:
> >
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Dekel Peled
> >> Sent: Monday, November 19, 2018 6:55 PM
> >> To: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> >> bernard.iremonger@intel.com
> >> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; Shahaf Shuler
> >> <shahafs@mellanox.com>; Dekel Peled <dekelp@mellanox.com>
> >> Subject: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
> >>
> >> Set MPLS label value in appropriate location at mplsoudp_encap_conf,
> >> so it is correctly copied to rte_flow_item_mpls.
> >>
> >> Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
> >> Cc: orika@mellanox.com
> >>
> >> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> >> ---
> >>  app/test-pmd/cmdline.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> >> index 1275074..40e64cc 100644
> >> --- a/app/test-pmd/cmdline.c
> >> +++ b/app/test-pmd/cmdline.c
> >> @@ -15804,10 +15804,10 @@ static void
> >> cmd_set_mplsoudp_encap_parsed(void *parsed_result,
> >>  	struct cmd_set_mplsoudp_encap_result *res = parsed_result;
> >>  	union {
> >>  		uint32_t mplsoudp_label;
> >> -		uint8_t label[3];
> >> +		uint8_t label[4];
> >>  	} id = {
> >>  		.mplsoudp_label =
> >> -			rte_cpu_to_be_32(res->label) & RTE_BE32(0x00ffffff),
> >> +			rte_cpu_to_be_32(res->label<<4) &
> >> RTE_BE32(0x00ffffff),
> >>  	};
> >>
> >>  	if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0)
> >> --
> >> 1.8.3.1
> >
> > Acked-by: Ori Kam <orika@mellanox.com>
> 
> Hi Ori, Dekel,
> 
> What is the scope of this patch? Briefly how critical it is and what will be
> broken and what is exposure of it?

The only issue is that we are setting incorrect MPLS label.
As defined by the MPLS spec the label is 20 bits, so this patch simply
pushes the label to the correct place.
I don't think that there any exposure from this patch.

Best,
Ori

Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation

[Adrien Mazarguil]

On Mon, Nov 19, 2018 at 06:54:50PM +0200, Dekel Peled wrote:
> Set MPLS label value in appropriate location at mplsoudp_encap_conf,
> so it is correctly copied to rte_flow_item_mpls.
> 
> Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
> Cc: orika@mellanox.com
> 
> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> ---
>  app/test-pmd/cmdline.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 1275074..40e64cc 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -15804,10 +15804,10 @@ static void cmd_set_mplsoudp_encap_parsed(void *parsed_result,
>  	struct cmd_set_mplsoudp_encap_result *res = parsed_result;
>  	union {
>  		uint32_t mplsoudp_label;
> -		uint8_t label[3];
> +		uint8_t label[4];
>  	} id = {
>  		.mplsoudp_label =
> -			rte_cpu_to_be_32(res->label) & RTE_BE32(0x00ffffff),
> +			rte_cpu_to_be_32(res->label<<4) & RTE_BE32(0x00ffffff),

Just to be sure, since label is a 20-bit value, isn't the shift supposed to
be 12 bits? In which case that mask is harmless but misleading. How about:

 .mplsoudp_label = rte_cpu_to_be32((res->label & 0xfffff) << 12);

>  	};
>  
>  	if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0)
> -- 
> 1.8.3.1
> 

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation

[Dekel Peled]

Thanks, PSB.

> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Thursday, November 22, 2018 11:05 AM
> To: Dekel Peled <dekelp@mellanox.com>
> Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> bernard.iremonger@intel.com; dev@dpdk.org; Ori Kam
> <orika@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
> 
> On Mon, Nov 19, 2018 at 06:54:50PM +0200, Dekel Peled wrote:
> > Set MPLS label value in appropriate location at mplsoudp_encap_conf,
> > so it is correctly copied to rte_flow_item_mpls.
> >
> > Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
> > Cc: orika@mellanox.com
> >
> > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > ---
> >  app/test-pmd/cmdline.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > 1275074..40e64cc 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -15804,10 +15804,10 @@ static void
> cmd_set_mplsoudp_encap_parsed(void *parsed_result,
> >  	struct cmd_set_mplsoudp_encap_result *res = parsed_result;
> >  	union {
> >  		uint32_t mplsoudp_label;
> > -		uint8_t label[3];
> > +		uint8_t label[4];
> >  	} id = {
> >  		.mplsoudp_label =
> > -			rte_cpu_to_be_32(res->label) &
> RTE_BE32(0x00ffffff),
> > +			rte_cpu_to_be_32(res->label<<4) &
> RTE_BE32(0x00ffffff),
> 
> Just to be sure, since label is a 20-bit value, isn't the shift supposed to be 12
> bits? In which case that mask is harmless but misleading. How about:
> 
>  .mplsoudp_label = rte_cpu_to_be32((res->label & 0xfffff) << 12);
> 

Label is 20-bits value in a 24-bits field, see struct rte_flow_item_mpls.

> >  	};
> >
> >  	if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0)
> > --
> > 1.8.3.1
> >
> 
> --
> Adrien Mazarguil
> 6WIND

Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation

[Adrien Mazarguil]

On Thu, Nov 22, 2018 at 09:56:09AM +0000, Dekel Peled wrote:
> Thanks, PSB.
> 
> > -----Original Message-----
> > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Sent: Thursday, November 22, 2018 11:05 AM
> > To: Dekel Peled <dekelp@mellanox.com>
> > Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> > bernard.iremonger@intel.com; dev@dpdk.org; Ori Kam
> > <orika@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
> > Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
> > 
> > On Mon, Nov 19, 2018 at 06:54:50PM +0200, Dekel Peled wrote:
> > > Set MPLS label value in appropriate location at mplsoudp_encap_conf,
> > > so it is correctly copied to rte_flow_item_mpls.
> > >
> > > Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
> > > Cc: orika@mellanox.com
> > >
> > > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > > ---
> > >  app/test-pmd/cmdline.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > 1275074..40e64cc 100644
> > > --- a/app/test-pmd/cmdline.c
> > > +++ b/app/test-pmd/cmdline.c
> > > @@ -15804,10 +15804,10 @@ static void
> > cmd_set_mplsoudp_encap_parsed(void *parsed_result,
> > >  	struct cmd_set_mplsoudp_encap_result *res = parsed_result;
> > >  	union {
> > >  		uint32_t mplsoudp_label;
> > > -		uint8_t label[3];
> > > +		uint8_t label[4];
> > >  	} id = {
> > >  		.mplsoudp_label =
> > > -			rte_cpu_to_be_32(res->label) &
> > RTE_BE32(0x00ffffff),
> > > +			rte_cpu_to_be_32(res->label<<4) &
> > RTE_BE32(0x00ffffff),
> > 
> > Just to be sure, since label is a 20-bit value, isn't the shift supposed to be 12
> > bits? In which case that mask is harmless but misleading. How about:
> > 
> >  .mplsoudp_label = rte_cpu_to_be32((res->label & 0xfffff) << 12);
> > 
> 
> Label is 20-bits value in a 24-bits field, see struct rte_flow_item_mpls.

OK, I know, what I missed was the following line:

 rte_memcpy(mplsoudp_encap_conf.label, &id.label[1], 3);

Just a suggestion then: using the same memcpy() offsets in both places for
clarity:

  rte_be32_t label = rte_cpu_to_be32(res->label << 12);

  memcpy(mplsodudp_encap_conf.label, &label, 3);

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation

[Dekel Peled]

Hi,

The current implementation is already validated, and since this is the last minute I prefer my patch to be applied as-is.
Please ack.

Regards,
Dekel

> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Thursday, November 22, 2018 12:14 PM
> To: Dekel Peled <dekelp@mellanox.com>
> Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> bernard.iremonger@intel.com; dev@dpdk.org; Ori Kam
> <orika@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
> 
> On Thu, Nov 22, 2018 at 09:56:09AM +0000, Dekel Peled wrote:
> > Thanks, PSB.
> >
> > > -----Original Message-----
> > > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > Sent: Thursday, November 22, 2018 11:05 AM
> > > To: Dekel Peled <dekelp@mellanox.com>
> > > Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> > > bernard.iremonger@intel.com; dev@dpdk.org; Ori Kam
> > > <orika@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
> > > Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP
> > > encapsulation
> > >
> > > On Mon, Nov 19, 2018 at 06:54:50PM +0200, Dekel Peled wrote:
> > > > Set MPLS label value in appropriate location at
> > > > mplsoudp_encap_conf, so it is correctly copied to rte_flow_item_mpls.
> > > >
> > > > Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
> > > > Cc: orika@mellanox.com
> > > >
> > > > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > > > ---
> > > >  app/test-pmd/cmdline.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > > 1275074..40e64cc 100644
> > > > --- a/app/test-pmd/cmdline.c
> > > > +++ b/app/test-pmd/cmdline.c
> > > > @@ -15804,10 +15804,10 @@ static void
> > > cmd_set_mplsoudp_encap_parsed(void *parsed_result,
> > > >  	struct cmd_set_mplsoudp_encap_result *res = parsed_result;
> > > >  	union {
> > > >  		uint32_t mplsoudp_label;
> > > > -		uint8_t label[3];
> > > > +		uint8_t label[4];
> > > >  	} id = {
> > > >  		.mplsoudp_label =
> > > > -			rte_cpu_to_be_32(res->label) &
> > > RTE_BE32(0x00ffffff),
> > > > +			rte_cpu_to_be_32(res->label<<4) &
> > > RTE_BE32(0x00ffffff),
> > >
> > > Just to be sure, since label is a 20-bit value, isn't the shift
> > > supposed to be 12 bits? In which case that mask is harmless but
> misleading. How about:
> > >
> > >  .mplsoudp_label = rte_cpu_to_be32((res->label & 0xfffff) << 12);
> > >
> >
> > Label is 20-bits value in a 24-bits field, see struct rte_flow_item_mpls.
> 
> OK, I know, what I missed was the following line:
> 
>  rte_memcpy(mplsoudp_encap_conf.label, &id.label[1], 3);
> 
> Just a suggestion then: using the same memcpy() offsets in both places for
> clarity:
> 
>   rte_be32_t label = rte_cpu_to_be32(res->label << 12);
> 
>   memcpy(mplsodudp_encap_conf.label, &label, 3);
> 
> --
> Adrien Mazarguil
> 6WIND

Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation

[Ferruh Yigit]

On 11/22/2018 4:18 PM, Dekel Peled wrote:
> Hi,
> 
> The current implementation is already validated, and since this is the last minute I prefer my patch to be applied as-is.
> Please ack.

Hi Dekel,

I think logic is other-way around, a patch has been acked clearly, without
question can justify to go in last minute. Going last minute doesn't justify an ack.

> 
> Regards,
> Dekel
> 
>> -----Original Message-----
>> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>> Sent: Thursday, November 22, 2018 12:14 PM
>> To: Dekel Peled <dekelp@mellanox.com>
>> Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
>> bernard.iremonger@intel.com; dev@dpdk.org; Ori Kam
>> <orika@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
>> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
>>
>> On Thu, Nov 22, 2018 at 09:56:09AM +0000, Dekel Peled wrote:
>>> Thanks, PSB.
>>>
>>>> -----Original Message-----
>>>> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>>>> Sent: Thursday, November 22, 2018 11:05 AM
>>>> To: Dekel Peled <dekelp@mellanox.com>
>>>> Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
>>>> bernard.iremonger@intel.com; dev@dpdk.org; Ori Kam
>>>> <orika@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
>>>> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP
>>>> encapsulation
>>>>
>>>> On Mon, Nov 19, 2018 at 06:54:50PM +0200, Dekel Peled wrote:
>>>>> Set MPLS label value in appropriate location at
>>>>> mplsoudp_encap_conf, so it is correctly copied to rte_flow_item_mpls.
>>>>>
>>>>> Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
>>>>> Cc: orika@mellanox.com
>>>>>
>>>>> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
>>>>> ---
>>>>>  app/test-pmd/cmdline.c | 4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>>>>> 1275074..40e64cc 100644
>>>>> --- a/app/test-pmd/cmdline.c
>>>>> +++ b/app/test-pmd/cmdline.c
>>>>> @@ -15804,10 +15804,10 @@ static void
>>>> cmd_set_mplsoudp_encap_parsed(void *parsed_result,
>>>>>  	struct cmd_set_mplsoudp_encap_result *res = parsed_result;
>>>>>  	union {
>>>>>  		uint32_t mplsoudp_label;
>>>>> -		uint8_t label[3];
>>>>> +		uint8_t label[4];
>>>>>  	} id = {
>>>>>  		.mplsoudp_label =
>>>>> -			rte_cpu_to_be_32(res->label) &
>>>> RTE_BE32(0x00ffffff),
>>>>> +			rte_cpu_to_be_32(res->label<<4) &
>>>> RTE_BE32(0x00ffffff),
>>>>
>>>> Just to be sure, since label is a 20-bit value, isn't the shift
>>>> supposed to be 12 bits? In which case that mask is harmless but
>> misleading. How about:
>>>>
>>>>  .mplsoudp_label = rte_cpu_to_be32((res->label & 0xfffff) << 12);
>>>>
>>>
>>> Label is 20-bits value in a 24-bits field, see struct rte_flow_item_mpls.
>>
>> OK, I know, what I missed was the following line:
>>
>>  rte_memcpy(mplsoudp_encap_conf.label, &id.label[1], 3);
>>
>> Just a suggestion then: using the same memcpy() offsets in both places for
>> clarity:
>>
>>   rte_be32_t label = rte_cpu_to_be32(res->label << 12);
>>
>>   memcpy(mplsodudp_encap_conf.label, &label, 3);
>>
>> --
>> Adrien Mazarguil
>> 6WIND

[dpdk-dev] [PATCH v2] app/testpmd: fix MPLSoUDP encapsulation

[Dekel Peled]

In function cmd_set_mplsoudp_encap_parsed(), MPLS label value was
set in mplsoudp_encap_conf struct without the required offset.
As a result the value was copied incorrectly into
rte_flow_item_mpls struct.

This patch sets MPLS label value in appropriate location at
mplsoudp_encap_conf struct, so it is correctly copied to
rte_flow_item_mpls struct.

Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
Cc: orika@mellanox.com

Signed-off-by: Dekel Peled <dekelp@mellanox.com>

---
v2: Update code fix and elaborate patch log for clarity.
---
---
 app/test-pmd/cmdline.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 1275074..8630be6 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -15804,10 +15804,9 @@ static void cmd_set_mplsoudp_encap_parsed(void *parsed_result,
 	struct cmd_set_mplsoudp_encap_result *res = parsed_result;
 	union {
 		uint32_t mplsoudp_label;
-		uint8_t label[3];
+		uint8_t label[4];
 	} id = {
-		.mplsoudp_label =
-			rte_cpu_to_be_32(res->label) & RTE_BE32(0x00ffffff),
+		.mplsoudp_label = rte_cpu_to_be_32(res->label<<12),
 	};
 
 	if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0)
@@ -15820,7 +15819,7 @@ static void cmd_set_mplsoudp_encap_parsed(void *parsed_result,
 		mplsoudp_encap_conf.select_ipv4 = 0;
 	else
 		return;
-	rte_memcpy(mplsoudp_encap_conf.label, &id.label[1], 3);
+	rte_memcpy(mplsoudp_encap_conf.label, &id.label, 3);
 	mplsoudp_encap_conf.udp_src = rte_cpu_to_be_16(res->udp_src);
 	mplsoudp_encap_conf.udp_dst = rte_cpu_to_be_16(res->udp_dst);
 	if (mplsoudp_encap_conf.select_ipv4) {
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] app/testpmd: fix MPLSoUDP encapsulation

[Ori Kam]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Dekel Peled
> Sent: Tuesday, December 4, 2018 3:52 PM
> To: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> bernard.iremonger@intel.com
> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; Dekel Peled <dekelp@mellanox.com>
> Subject: [dpdk-dev] [PATCH v2] app/testpmd: fix MPLSoUDP encapsulation
> 
> In function cmd_set_mplsoudp_encap_parsed(), MPLS label value was
> set in mplsoudp_encap_conf struct without the required offset.
> As a result the value was copied incorrectly into
> rte_flow_item_mpls struct.
> 
> This patch sets MPLS label value in appropriate location at
> mplsoudp_encap_conf struct, so it is correctly copied to
> rte_flow_item_mpls struct.
> 
> Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
> Cc: orika@mellanox.com
> 
> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> 
> ---
> v2: Update code fix and elaborate patch log for clarity.
> ---
> ---
>  app/test-pmd/cmdline.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 1275074..8630be6 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -15804,10 +15804,9 @@ static void
> cmd_set_mplsoudp_encap_parsed(void *parsed_result,
>  	struct cmd_set_mplsoudp_encap_result *res = parsed_result;
>  	union {
>  		uint32_t mplsoudp_label;
> -		uint8_t label[3];
> +		uint8_t label[4];
>  	} id = {
> -		.mplsoudp_label =
> -			rte_cpu_to_be_32(res->label) & RTE_BE32(0x00ffffff),
> +		.mplsoudp_label = rte_cpu_to_be_32(res->label<<12),

Why did you remove the mask?

>  	};
> 
>  	if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0)
> @@ -15820,7 +15819,7 @@ static void cmd_set_mplsoudp_encap_parsed(void
> *parsed_result,
>  		mplsoudp_encap_conf.select_ipv4 = 0;
>  	else
>  		return;
> -	rte_memcpy(mplsoudp_encap_conf.label, &id.label[1], 3);
> +	rte_memcpy(mplsoudp_encap_conf.label, &id.label, 3);
>  	mplsoudp_encap_conf.udp_src = rte_cpu_to_be_16(res->udp_src);
>  	mplsoudp_encap_conf.udp_dst = rte_cpu_to_be_16(res->udp_dst);
>  	if (mplsoudp_encap_conf.select_ipv4) {
> --
> 1.8.3.1

Best,
Ori

Re: [dpdk-dev] [PATCH v2] app/testpmd: fix MPLSoUDP encapsulation

[Dekel Peled]

Thanks, PSB.

> -----Original Message-----
> From: Ori Kam
> Sent: Tuesday, December 4, 2018 11:23 PM
> To: Dekel Peled <dekelp@mellanox.com>; wenzhuo.lu@intel.com;
> jingjing.wu@intel.com; bernard.iremonger@intel.com
> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>; Dekel Peled
> <dekelp@mellanox.com>
> Subject: RE: [dpdk-dev] [PATCH v2] app/testpmd: fix MPLSoUDP
> encapsulation
> 
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Dekel Peled
> > Sent: Tuesday, December 4, 2018 3:52 PM
> > To: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> > bernard.iremonger@intel.com
> > Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; Shahaf Shuler
> > <shahafs@mellanox.com>; Dekel Peled <dekelp@mellanox.com>
> > Subject: [dpdk-dev] [PATCH v2] app/testpmd: fix MPLSoUDP encapsulation
> >
> > In function cmd_set_mplsoudp_encap_parsed(), MPLS label value was set
> > in mplsoudp_encap_conf struct without the required offset.
> > As a result the value was copied incorrectly into rte_flow_item_mpls
> > struct.
> >
> > This patch sets MPLS label value in appropriate location at
> > mplsoudp_encap_conf struct, so it is correctly copied to
> > rte_flow_item_mpls struct.
> >
> > Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
> > Cc: orika@mellanox.com
> >
> > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> >
> > ---
> > v2: Update code fix and elaborate patch log for clarity.
> > ---
> > ---
> >  app/test-pmd/cmdline.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > 1275074..8630be6 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -15804,10 +15804,9 @@ static void
> > cmd_set_mplsoudp_encap_parsed(void *parsed_result,
> >  	struct cmd_set_mplsoudp_encap_result *res = parsed_result;
> >  	union {
> >  		uint32_t mplsoudp_label;
> > -		uint8_t label[3];
> > +		uint8_t label[4];
> >  	} id = {
> > -		.mplsoudp_label =
> > -			rte_cpu_to_be_32(res->label) &
> RTE_BE32(0x00ffffff),
> > +		.mplsoudp_label = rte_cpu_to_be_32(res->label<<12),
> 
> Why did you remove the mask?

The mask of all valid bits set to 1 is redundant.
After <<12 the 20 valid bits are not changed, and the other 12 bits are set to 0.

> 
> >  	};
> >
> >  	if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0) @@ -15820,7
> > +15819,7 @@ static void cmd_set_mplsoudp_encap_parsed(void
> > *parsed_result,
> >  		mplsoudp_encap_conf.select_ipv4 = 0;
> >  	else
> >  		return;
> > -	rte_memcpy(mplsoudp_encap_conf.label, &id.label[1], 3);
> > +	rte_memcpy(mplsoudp_encap_conf.label, &id.label, 3);
> >  	mplsoudp_encap_conf.udp_src = rte_cpu_to_be_16(res->udp_src);
> >  	mplsoudp_encap_conf.udp_dst = rte_cpu_to_be_16(res->udp_dst);
> >  	if (mplsoudp_encap_conf.select_ipv4) {
> > --
> > 1.8.3.1
> 
> Best,
> Ori

Re: [dpdk-dev] [PATCH v2] app/testpmd: fix MPLSoUDP encapsulation

[Ori Kam]

> -----Original Message-----
> From: Dekel Peled
> Sent: Thursday, December 6, 2018 10:18 AM
> To: Ori Kam <orika@mellanox.com>; wenzhuo.lu@intel.com;
> jingjing.wu@intel.com; bernard.iremonger@intel.com
> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>
> Subject: RE: [dpdk-dev] [PATCH v2] app/testpmd: fix MPLSoUDP encapsulation
> 
> Thanks, PSB.
> 
> > -----Original Message-----
> > From: Ori Kam
> > Sent: Tuesday, December 4, 2018 11:23 PM
> > To: Dekel Peled <dekelp@mellanox.com>; wenzhuo.lu@intel.com;
> > jingjing.wu@intel.com; bernard.iremonger@intel.com
> > Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>; Dekel Peled
> > <dekelp@mellanox.com>
> > Subject: RE: [dpdk-dev] [PATCH v2] app/testpmd: fix MPLSoUDP
> > encapsulation
> >
> >
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Dekel Peled
> > > Sent: Tuesday, December 4, 2018 3:52 PM
> > > To: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> > > bernard.iremonger@intel.com
> > > Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; Shahaf Shuler
> > > <shahafs@mellanox.com>; Dekel Peled <dekelp@mellanox.com>
> > > Subject: [dpdk-dev] [PATCH v2] app/testpmd: fix MPLSoUDP encapsulation
> > >
> > > In function cmd_set_mplsoudp_encap_parsed(), MPLS label value was set
> > > in mplsoudp_encap_conf struct without the required offset.
> > > As a result the value was copied incorrectly into rte_flow_item_mpls
> > > struct.
> > >
> > > This patch sets MPLS label value in appropriate location at
> > > mplsoudp_encap_conf struct, so it is correctly copied to
> > > rte_flow_item_mpls struct.
> > >
> > > Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
> > > Cc: orika@mellanox.com
> > >
> > > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > >
> > > ---
> > > v2: Update code fix and elaborate patch log for clarity.
> > > ---
> > > ---
> > >  app/test-pmd/cmdline.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > 1275074..8630be6 100644
> > > --- a/app/test-pmd/cmdline.c
> > > +++ b/app/test-pmd/cmdline.c
> > > @@ -15804,10 +15804,9 @@ static void
> > > cmd_set_mplsoudp_encap_parsed(void *parsed_result,
> > >  	struct cmd_set_mplsoudp_encap_result *res = parsed_result;
> > >  	union {
> > >  		uint32_t mplsoudp_label;
> > > -		uint8_t label[3];
> > > +		uint8_t label[4];
> > >  	} id = {
> > > -		.mplsoudp_label =
> > > -			rte_cpu_to_be_32(res->label) &
> > RTE_BE32(0x00ffffff),
> > > +		.mplsoudp_label = rte_cpu_to_be_32(res->label<<12),
> >
> > Why did you remove the mask?
> 
> The mask of all valid bits set to 1 is redundant.
> After <<12 the 20 valid bits are not changed, and the other 12 bits are set to 0.
> 
> >
> > >  	};
> > >
> > >  	if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0) @@ -15820,7
> > > +15819,7 @@ static void cmd_set_mplsoudp_encap_parsed(void
> > > *parsed_result,
> > >  		mplsoudp_encap_conf.select_ipv4 = 0;
> > >  	else
> > >  		return;
> > > -	rte_memcpy(mplsoudp_encap_conf.label, &id.label[1], 3);
> > > +	rte_memcpy(mplsoudp_encap_conf.label, &id.label, 3);
> > >  	mplsoudp_encap_conf.udp_src = rte_cpu_to_be_16(res->udp_src);
> > >  	mplsoudp_encap_conf.udp_dst = rte_cpu_to_be_16(res->udp_dst);
> > >  	if (mplsoudp_encap_conf.select_ipv4) {
> > > --
> > > 1.8.3.1
> >
> > Best,
> > Ori

Acked-by: Ori Kam <orika@mellanox.com>
Thanks,
Ori

Re: [dpdk-dev] [PATCH v2] app/testpmd: fix MPLSoUDP encapsulation

[Ferruh Yigit]

On 12/6/2018 9:38 AM, Ori Kam wrote:
<...>

>>>> Subject: [dpdk-dev] [PATCH v2] app/testpmd: fix MPLSoUDP encapsulation
>>>>
>>>> In function cmd_set_mplsoudp_encap_parsed(), MPLS label value was set
>>>> in mplsoudp_encap_conf struct without the required offset.
>>>> As a result the value was copied incorrectly into rte_flow_item_mpls
>>>> struct.
>>>>
>>>> This patch sets MPLS label value in appropriate location at
>>>> mplsoudp_encap_conf struct, so it is correctly copied to
>>>> rte_flow_item_mpls struct.
>>>>
>>>> Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
>>>> Cc: orika@mellanox.com
>>>>
>>>> Signed-off-by: Dekel Peled <dekelp@mellanox.com>>
> Acked-by: Ori Kam <orika@mellanox.com>

Applied to dpdk-next-net/master, thanks.