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

Discussion:

Dekel Peled

2018-11-19 16:54:50 UTC

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: ***@mellanox.com

Signed-off-by: Dekel Peled <***@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

Ori Kam

2018-11-20 08:23:14 UTC

Permalink

-----Original Message-----
Sent: Monday, November 19, 2018 6:55 PM
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")
---
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 <***@mellanox.com>

Thanks,
Ori

Ferruh Yigit

2018-11-21 15:19:04 UTC

Permalink

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?

Ori Kam

2018-11-21 15:39:54 UTC

Permalink

-----Original Message-----
Sent: Wednesday, November 21, 2018 5:19 PM
Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation

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

Adrien Mazarguil

2018-11-22 09:04:33 UTC

Permalink

Post by 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")
---
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);

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

--
Adrien Mazarguil
6WIND

Dekel Peled

2018-11-22 09:56:09 UTC

Permalink

Thanks, PSB.

-----Original Message-----
Sent: Thursday, November 22, 2018 11:05 AM
Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation

cmd_set_mplsoudp_encap_parsed(void *parsed_result,

Post by Dekel Peled
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),

Post by Dekel Peled
+ 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
.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.

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

--
Adrien Mazarguil
6WIND

Adrien Mazarguil

2018-11-22 10:14:21 UTC

Permalink

Post by Dekel Peled
Thanks, PSB.

-----Original Message-----
Sent: Thursday, November 22, 2018 11:05 AM
Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation

cmd_set_mplsoudp_encap_parsed(void *parsed_result,

RTE_BE32(0x00ffffff),

Post by Dekel Peled
+ 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
.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

Dekel Peled

2018-11-22 16:18:04 UTC

Permalink

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-----
Sent: Thursday, November 22, 2018 12:14 PM
Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation

Post by Dekel Peled
Thanks, PSB.

-----Original Message-----
Sent: Thursday, November 22, 2018 11:05 AM
Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP
encapsulation

cmd_set_mplsoudp_encap_parsed(void *parsed_result,

RTE_BE32(0x00ffffff),

Post by Dekel Peled
+ 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
.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.

rte_memcpy(mplsoudp_encap_conf.label, &id.label[1], 3);
Just a suggestion then: using the same memcpy() offsets in both places for
rte_be32_t label = rte_cpu_to_be32(res->label << 12);
memcpy(mplsodudp_encap_conf.label, &label, 3);
--
Adrien Mazarguil
6WIND

Ferruh Yigit

2018-11-22 16:39:12 UTC

Permalink

Post by 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.

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.

Post by Dekel Peled
Regards,
Dekel

-----Original Message-----
Sent: Thursday, November 22, 2018 12:14 PM
Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation

Post by Dekel Peled
Thanks, PSB.

-----Original Message-----
Sent: Thursday, November 22, 2018 11:05 AM
Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP
encapsulation

cmd_set_mplsoudp_encap_parsed(void *parsed_result,

RTE_BE32(0x00ffffff),

Post by Dekel Peled
+ rte_cpu_to_be_32(res->label<<4) &

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

Dekel Peled

2018-12-04 13:51:33 UTC

Permalink

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: ***@mellanox.com

Signed-off-by: Dekel Peled <***@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

Ori Kam

2018-12-04 21:23:04 UTC

Permalink

-----Original Message-----
Sent: Tuesday, December 4, 2018 3:52 PM
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")
---
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

Dekel Peled

2018-12-06 08:17:37 UTC

Permalink

Thanks, PSB.

-----Original Message-----
From: Ori Kam
Sent: Tuesday, December 4, 2018 11:23 PM
Subject: RE: [dpdk-dev] [PATCH v2] app/testpmd: fix MPLSoUDP
encapsulation

-----Original Message-----
Sent: Tuesday, December 4, 2018 3:52 PM
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")
---
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.

};
*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

Ori Kam

2018-12-06 09:38:03 UTC

Permalink

-----Original Message-----
From: Dekel Peled
Sent: Thursday, December 6, 2018 10:18 AM
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
Subject: RE: [dpdk-dev] [PATCH v2] app/testpmd: fix MPLSoUDP
encapsulation

-----Original Message-----
Sent: Tuesday, December 4, 2018 3:52 PM
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")
---
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.

Best,
Ori

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