[dpdk-dev] [RFC 1/3] ethdev: update modify field flow action

[dpdk-dev] [RFC 1/3] ethdev: update modify field flow action

[Viacheslav Ovsiienko]

The generic modify field flow action introduced in [1] has
some issues related to the immediate source operand:

  - immediate source can be presented either as an unsigned
    64-bit integer or pointer to data pattern in memory.
    There was no explicit pointer field defined in the union

  - the byte ordering for 64-bit integer was not specified.
    Many fields have lesser lengths and byte ordering
    is crucial.

  - how the bit offset is applied to the immediate source
    field was not defined and documented

  - 64-bit integer size is not enough to provide MAC and
    IPv6 addresses

In order to cover the issues and exclude any ambiguities
the following is done:

  - introduce the explicit pointer field
    in rte_flow_action_modify_data structure

  - replace the 64-bit unsigned integer with 16-byte array

  - update the modify field flow action documentation

[1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 doc/guides/prog_guide/rte_flow.rst     |  8 ++++++++
 doc/guides/rel_notes/release_21_11.rst |  7 +++++++
 lib/ethdev/rte_flow.h                  | 15 ++++++++++++---
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 2b42d5ec8c..a54760a7b4 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -2835,6 +2835,14 @@ a packet to any other part of it.
 ``value`` sets an immediate value to be used as a source or points to a
 location of the value in memory. It is used instead of ``level`` and ``offset``
 for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER`` respectively.
+The data in memory should be presented exactly in the same byte order and
+length as in the relevant flow item, i.e. data for field with type
+RTE_FLOW_FIELD_MAC_DST should follow the conventions of dst field
+in rte_flow_item_eth structure, with type RTE_FLOW_FIELD_IPV6_SRC -
+rte_flow_item_ipv6 conventions, and so on. The bitfield exatracted from the
+memory being applied as second operation parameter is defined by width and
+the destination field offset. If the field size is large than 16 bytes the
+pattern can be provided as pointer only.
 
 .. _table_rte_flow_action_modify_field:
 
diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index d707a554ef..fdeba27e14 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -84,6 +84,10 @@ API Changes
    Also, make sure to start the actual text at the margin.
    =======================================================
 
+* ethdev: ``rte_flow_action_modify_data`` structure udpdated, immediate data
+  array is extended, data pointer field is explicitly added to union, the
+  action behavior is defined in more strict fashion and documentation uddated.
+
 
 ABI Changes
 -----------
@@ -101,6 +105,9 @@ ABI Changes
    =======================================================
 
 
+* ethdev: ``rte_flow_action_modify_data`` structure udpdated.
+
+
 Known Issues
 ------------
 
diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
index 70f455d47d..50e6f34579 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -3204,6 +3204,9 @@ enum rte_flow_field_id {
 };
 
 /**
+ * @warning
+ * @b EXPERIMENTAL: this structure may change without prior notice
+ *
  * Field description for MODIFY_FIELD action.
  */
 struct rte_flow_action_modify_data {
@@ -3217,10 +3220,16 @@ struct rte_flow_action_modify_data {
 			uint32_t offset;
 		};
 		/**
-		 * Immediate value for RTE_FLOW_FIELD_VALUE or
-		 * memory address for RTE_FLOW_FIELD_POINTER.
+		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented in the
+		 * same byte order and length as in relevant rte_flow_item_xxx.
 		 */
-		uint64_t value;
+		uint8_t value[16];
+		/*
+		 * Memory address for RTE_FLOW_FIELD_POINTER, memory layout
+		 * should be the same as for relevant field in the
+		 * rte_flow_item_xxx structure.
+		 */
+		void *pvalue;
 	};
 };
 
-- 
2.18.1

[dpdk-dev] [RFC 2/3] app/testpmd: update modify field flow action support

[Viacheslav Ovsiienko]

The testpmd flow create command updates provided:

  - modify field action supports the updated actions
  - pointer type added for action source field
  - pointer and value source field takes hex string
    instead of unsigned int in host endianness

There are some examples of flow with update modified
field action:

1. IPv6 destination address bytes 4-7 assignment:
   0000::1111 - > 0000:xxxx:4455:6677::1111

   flow create 0 egress group 1
     pattern eth / ipv6 dst is 0000::1111 / udp / end
     actions modify_field op set
             dst_type ipv6_dst
	     dst_offset 32
             src_type value
             src_value 0011223344556677
	     width 32 / end

2. Copy second byte of IPv4 destination address to the
   third byte of source address:
     10.0.118.4 -> 192.168.100.1
     10.0.168.4 -> 192.168.100.1

   flow create 0 egress group 1
     pattern eth / ipv4 / udp / end
     actions modify_field op set
             dst_type ipv4_src
             dst_offset 16
	     src_type ipv4_dst
	     src_offset 8
	     width 8 / end

3. Assign METADATA value with 11223344 value from the
   hex string in the linear buffer. Please note, the value
   definition should follow host-endian, example is given
   for x86 (little-endian):

   flow create 0 egress group 1
     pattern eth / ipv4 / end
     actions modify_field op set
             dst_type meta
	     src_type pointer
	     src_ptr 44332211
	     width 32 / end

4. Assign destination MAC with EA:11:0B:AD:0B:ED value:

   flow create 0 egress group 1
     pattern eth / end
     actions modify_field op set
             dst_type mac_dst
	     src_type value
	     src_value EA110BAD0BED
	     width 48 / end

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 55 +++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 6cd99bf37f..529ead6b2a 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -448,6 +448,7 @@ enum index {
 	ACTION_MODIFY_FIELD_SRC_LEVEL,
 	ACTION_MODIFY_FIELD_SRC_OFFSET,
 	ACTION_MODIFY_FIELD_SRC_VALUE,
+	ACTION_MODIFY_FIELD_SRC_POINTER,
 	ACTION_MODIFY_FIELD_WIDTH,
 	ACTION_CONNTRACK,
 	ACTION_CONNTRACK_UPDATE,
@@ -468,6 +469,14 @@ enum index {
 #define ITEM_RAW_SIZE \
 	(sizeof(struct rte_flow_item_raw) + ITEM_RAW_PATTERN_SIZE)
 
+/** Maximum size for external pattern in struct rte_flow_action_modify_data. */
+#define ACTION_MODIFY_PATTERN_SIZE 32
+
+/** Storage size for struct rte_flow_action_modify_field including pattern. */
+#define ACTION_MODIFY_SIZE \
+	(sizeof(struct rte_flow_action_modify_field) + \
+	ACTION_MODIFY_PATTERN_SIZE)
+
 /** Maximum number of queue indices in struct rte_flow_action_rss. */
 #define ACTION_RSS_QUEUE_NUM 128
 
@@ -1704,6 +1713,7 @@ static const enum index action_modify_field_src[] = {
 	ACTION_MODIFY_FIELD_SRC_LEVEL,
 	ACTION_MODIFY_FIELD_SRC_OFFSET,
 	ACTION_MODIFY_FIELD_SRC_VALUE,
+	ACTION_MODIFY_FIELD_SRC_POINTER,
 	ACTION_MODIFY_FIELD_WIDTH,
 	ZERO,
 };
@@ -4455,8 +4465,7 @@ static const struct token token_list[] = {
 	[ACTION_MODIFY_FIELD] = {
 		.name = "modify_field",
 		.help = "modify destination field with data from source field",
-		.priv = PRIV_ACTION(MODIFY_FIELD,
-			sizeof(struct rte_flow_action_modify_field)),
+		.priv = PRIV_ACTION(MODIFY_FIELD, ACTION_MODIFY_SIZE),
 		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_OP)),
 		.call = parse_vc,
 	},
@@ -4539,11 +4548,26 @@ static const struct token token_list[] = {
 		.name = "src_value",
 		.help = "source immediate value",
 		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_WIDTH),
-			NEXT_ENTRY(COMMON_UNSIGNED)),
-		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_modify_field,
+			     NEXT_ENTRY(COMMON_HEX)),
+		.args = ARGS(ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY(struct rte_flow_action_modify_field,
 					src.value)),
 		.call = parse_vc_conf,
 	},
+	[ACTION_MODIFY_FIELD_SRC_POINTER] = {
+		.name = "src_ptr",
+		.help = "pointer to source immediate value",
+		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_WIDTH),
+			     NEXT_ENTRY(COMMON_HEX)),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_modify_field,
+					src.pvalue),
+			     ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY_ARB
+				(sizeof(struct rte_flow_action_modify_field),
+				 ACTION_MODIFY_PATTERN_SIZE)),
+		.call = parse_vc_conf,
+	},
 	[ACTION_MODIFY_FIELD_WIDTH] = {
 		.name = "width",
 		.help = "number of bits to copy",
@@ -7830,15 +7854,11 @@ static int
 comp_set_modify_field_op(struct context *ctx, const struct token *token,
 		   unsigned int ent, char *buf, unsigned int size)
 {
-	uint16_t idx = 0;
-
 	RTE_SET_USED(ctx);
 	RTE_SET_USED(token);
-	for (idx = 0; modify_field_ops[idx]; ++idx)
-		;
 	if (!buf)
-		return idx + 1;
-	if (ent < idx)
+		return RTE_DIM(modify_field_ops);
+	if (ent < RTE_DIM(modify_field_ops) - 1)
 		return strlcpy(buf, modify_field_ops[ent], size);
 	return -1;
 }
@@ -7848,16 +7868,17 @@ static int
 comp_set_modify_field_id(struct context *ctx, const struct token *token,
 		   unsigned int ent, char *buf, unsigned int size)
 {
-	uint16_t idx = 0;
+	const char *name;
 
-	RTE_SET_USED(ctx);
 	RTE_SET_USED(token);
-	for (idx = 0; modify_field_ids[idx]; ++idx)
-		;
 	if (!buf)
-		return idx + 1;
-	if (ent < idx)
-		return strlcpy(buf, modify_field_ids[ent], size);
+		return RTE_DIM(modify_field_ids);
+	if (ent >= RTE_DIM(modify_field_ids) - 1)
+		return -1;
+	name = modify_field_ids[ent];
+	if (ctx->curr == ACTION_MODIFY_FIELD_SRC_TYPE ||
+	    (strcmp(name, "pointer") && strcmp(name, "value")))
+		return strlcpy(buf, name, size);
 	return -1;
 }
 
-- 
2.18.1

[dpdk-dev] [RFC 3/3] app/testpmd: fix hex string parser in flow commands

[Viacheslav Ovsiienko]

The hexadecimal string parser does not check the target
field buffer size, buffer overflow happens and might
cause the application failure (segmentation fault
is observed usually).

Fixes: 169a9fed1f4c ("app/testpmd: fix hex string parser support for flow API")
Cc: stable@dpdk.org

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 529ead6b2a..9e7fe1ae9f 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -7291,10 +7291,13 @@ parse_hex(struct context *ctx, const struct token *token,
 		hexlen -= 2;
 	}
 	if (hexlen > length)
-		return -1;
+		goto error;
 	ret = parse_hex_string(str, hex_tmp, &hexlen);
 	if (ret < 0)
 		goto error;
+	/* Check the converted binary fits into data buffer. */
+	if (hexlen > size)
+		goto error;
 	/* Let parse_int() fill length information first. */
 	ret = snprintf(tmp, sizeof(tmp), "%u", hexlen);
 	if (ret < 0)
-- 
2.18.1

[dpdk-dev] [PATCH 0/3] ethdev: update modify field flow action

[Viacheslav Ovsiienko]

ethdev: update modify field flow action

The generic modify field flow action introduced in [1] has
some issues related to the immediate source operand:

  - immediate source can be presented either as an unsigned
    64-bit integer or pointer to data pattern in memory.
    There was no explicit pointer field defined in the union

  - the byte ordering for 64-bit integer was not specified.
    Many fields have lesser lengths and byte ordering
    is crucial.

  - how the bit offset is applied to the immediate source
    field was not defined and documented

  - 64-bit integer size is not enough to provide MAC and
    IPv6 addresses

In order to cover the issues and exclude any ambiguities
the following is done:

  - introduce the explicit pointer field
    in rte_flow_action_modify_data structure

  - replace the 64-bit unsigned integer with 16-byte array

  - update the modify field flow action documentation

[1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
[2] RFC: http://patches.dpdk.org/project/dpdk/patch/20210910141609.8410-1-viacheslavo@nvidia.com/
[3] Deprecation notice: http://patches.dpdk.org/project/dpdk/patch/20210803085754.643180-1-orika@nvidia.com/


Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>

Viacheslav Ovsiienko (3):
  ethdev: update modify field flow action
  app/testpmd: update modify field flow action support
  app/testpmd: fix hex string parser in flow commands

 app/test-pmd/cmdline_flow.c            | 60 ++++++++++++++++++--------
 doc/guides/prog_guide/rte_flow.rst     |  8 ++++
 doc/guides/rel_notes/release_21_11.rst |  7 +++
 lib/ethdev/rte_flow.h                  | 15 +++++--
 4 files changed, 69 insertions(+), 21 deletions(-)

-- 
2.18.1

[dpdk-dev] [PATCH 1/3] ethdev: update modify field flow action

[Viacheslav Ovsiienko]

The generic modify field flow action introduced in [1] has
some issues related to the immediate source operand:

  - immediate source can be presented either as an unsigned
    64-bit integer or pointer to data pattern in memory.
    There was no explicit pointer field defined in the union

  - the byte ordering for 64-bit integer was not specified.
    Many fields have lesser lengths and byte ordering
    is crucial.

  - how the bit offset is applied to the immediate source
    field was not defined and documented

  - 64-bit integer size is not enough to provide MAC and
    IPv6 addresses

In order to cover the issues and exclude any ambiguities
the following is done:

  - introduce the explicit pointer field
    in rte_flow_action_modify_data structure

  - replace the 64-bit unsigned integer with 16-byte array

  - update the modify field flow action documentation

[1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 doc/guides/prog_guide/rte_flow.rst     |  8 ++++++++
 doc/guides/rel_notes/release_21_11.rst |  7 +++++++
 lib/ethdev/rte_flow.h                  | 15 ++++++++++++---
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 2b42d5ec8c..a54760a7b4 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -2835,6 +2835,14 @@ a packet to any other part of it.
 ``value`` sets an immediate value to be used as a source or points to a
 location of the value in memory. It is used instead of ``level`` and ``offset``
 for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER`` respectively.
+The data in memory should be presented exactly in the same byte order and
+length as in the relevant flow item, i.e. data for field with type
+RTE_FLOW_FIELD_MAC_DST should follow the conventions of dst field
+in rte_flow_item_eth structure, with type RTE_FLOW_FIELD_IPV6_SRC -
+rte_flow_item_ipv6 conventions, and so on. The bitfield exatracted from the
+memory being applied as second operation parameter is defined by width and
+the destination field offset. If the field size is large than 16 bytes the
+pattern can be provided as pointer only.
 
 .. _table_rte_flow_action_modify_field:
 
diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index 73e377a007..7db6cccab0 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -170,6 +170,10 @@ API Changes
   the crypto/security operation. This field will be used to communicate
   events such as soft expiry with IPsec in lookaside mode.
 
+* ethdev: ``rte_flow_action_modify_data`` structure udpdated, immediate data
+  array is extended, data pointer field is explicitly added to union, the
+  action behavior is defined in more strict fashion and documentation uddated.
+
 
 ABI Changes
 -----------
@@ -206,6 +210,9 @@ ABI Changes
   and hard expiry limits. Limits can be either in number of packets or bytes.
 
 
+* ethdev: ``rte_flow_action_modify_data`` structure udpdated.
+
+
 Known Issues
 ------------
 
diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
index 7b1ed7f110..af4c693ead 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -3204,6 +3204,9 @@ enum rte_flow_field_id {
 };
 
 /**
+ * @warning
+ * @b EXPERIMENTAL: this structure may change without prior notice
+ *
  * Field description for MODIFY_FIELD action.
  */
 struct rte_flow_action_modify_data {
@@ -3217,10 +3220,16 @@ struct rte_flow_action_modify_data {
 			uint32_t offset;
 		};
 		/**
-		 * Immediate value for RTE_FLOW_FIELD_VALUE or
-		 * memory address for RTE_FLOW_FIELD_POINTER.
+		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented in the
+		 * same byte order and length as in relevant rte_flow_item_xxx.
 		 */
-		uint64_t value;
+		uint8_t value[16];
+		/*
+		 * Memory address for RTE_FLOW_FIELD_POINTER, memory layout
+		 * should be the same as for relevant field in the
+		 * rte_flow_item_xxx structure.
+		 */
+		void *pvalue;
 	};
 };
 
-- 
2.18.1

[dpdk-dev] [PATCH 2/3] app/testpmd: update modify field flow action support

[Viacheslav Ovsiienko]

The testpmd flow create command updates provided:

  - modify field action supports the updated actions
  - pointer type added for action source field
  - pointer and value source field takes hex string
    instead of unsigned int in host endianness

There are some examples of flow with update modified
field action:

1. IPv6 destination address bytes 4-7 assignment:
   0000::1111 - > 0000:xxxx:4455:6677::1111

   flow create 0 egress group 1
     pattern eth / ipv6 dst is 0000::1111 / udp / end
     actions modify_field op set
             dst_type ipv6_dst
	     dst_offset 32
             src_type value
             src_value 0011223344556677
	     width 32 / end

2. Copy second byte of IPv4 destination address to the
   third byte of source address:
     10.0.118.4 -> 192.168.100.1
     10.0.168.4 -> 192.168.100.1

   flow create 0 egress group 1
     pattern eth / ipv4 / udp / end
     actions modify_field op set
             dst_type ipv4_src
             dst_offset 16
	     src_type ipv4_dst
	     src_offset 8
	     width 8 / end

3. Assign METADATA value with 11223344 value from the
   hex string in the linear buffer. Please note, the value
   definition should follow host-endian, example is given
   for x86 (little-endian):

   flow create 0 egress group 1
     pattern eth / ipv4 / end
     actions modify_field op set
             dst_type meta
	     src_type pointer
	     src_ptr 44332211
	     width 32 / end

4. Assign destination MAC with EA:11:0B:AD:0B:ED value:

   flow create 0 egress group 1
     pattern eth / end
     actions modify_field op set
             dst_type mac_dst
	     src_type value
	     src_value EA110BAD0BED
	     width 48 / end

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 55 +++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index bb22294dd3..736029c4fd 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -448,6 +448,7 @@ enum index {
 	ACTION_MODIFY_FIELD_SRC_LEVEL,
 	ACTION_MODIFY_FIELD_SRC_OFFSET,
 	ACTION_MODIFY_FIELD_SRC_VALUE,
+	ACTION_MODIFY_FIELD_SRC_POINTER,
 	ACTION_MODIFY_FIELD_WIDTH,
 	ACTION_CONNTRACK,
 	ACTION_CONNTRACK_UPDATE,
@@ -468,6 +469,14 @@ enum index {
 #define ITEM_RAW_SIZE \
 	(sizeof(struct rte_flow_item_raw) + ITEM_RAW_PATTERN_SIZE)
 
+/** Maximum size for external pattern in struct rte_flow_action_modify_data. */
+#define ACTION_MODIFY_PATTERN_SIZE 32
+
+/** Storage size for struct rte_flow_action_modify_field including pattern. */
+#define ACTION_MODIFY_SIZE \
+	(sizeof(struct rte_flow_action_modify_field) + \
+	ACTION_MODIFY_PATTERN_SIZE)
+
 /** Maximum number of queue indices in struct rte_flow_action_rss. */
 #define ACTION_RSS_QUEUE_NUM 128
 
@@ -1704,6 +1713,7 @@ static const enum index action_modify_field_src[] = {
 	ACTION_MODIFY_FIELD_SRC_LEVEL,
 	ACTION_MODIFY_FIELD_SRC_OFFSET,
 	ACTION_MODIFY_FIELD_SRC_VALUE,
+	ACTION_MODIFY_FIELD_SRC_POINTER,
 	ACTION_MODIFY_FIELD_WIDTH,
 	ZERO,
 };
@@ -4455,8 +4465,7 @@ static const struct token token_list[] = {
 	[ACTION_MODIFY_FIELD] = {
 		.name = "modify_field",
 		.help = "modify destination field with data from source field",
-		.priv = PRIV_ACTION(MODIFY_FIELD,
-			sizeof(struct rte_flow_action_modify_field)),
+		.priv = PRIV_ACTION(MODIFY_FIELD, ACTION_MODIFY_SIZE),
 		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_OP)),
 		.call = parse_vc,
 	},
@@ -4539,11 +4548,26 @@ static const struct token token_list[] = {
 		.name = "src_value",
 		.help = "source immediate value",
 		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_WIDTH),
-			NEXT_ENTRY(COMMON_UNSIGNED)),
-		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_modify_field,
+			     NEXT_ENTRY(COMMON_HEX)),
+		.args = ARGS(ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY(struct rte_flow_action_modify_field,
 					src.value)),
 		.call = parse_vc_conf,
 	},
+	[ACTION_MODIFY_FIELD_SRC_POINTER] = {
+		.name = "src_ptr",
+		.help = "pointer to source immediate value",
+		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_WIDTH),
+			     NEXT_ENTRY(COMMON_HEX)),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_modify_field,
+					src.pvalue),
+			     ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY_ARB
+				(sizeof(struct rte_flow_action_modify_field),
+				 ACTION_MODIFY_PATTERN_SIZE)),
+		.call = parse_vc_conf,
+	},
 	[ACTION_MODIFY_FIELD_WIDTH] = {
 		.name = "width",
 		.help = "number of bits to copy",
@@ -7830,15 +7854,11 @@ static int
 comp_set_modify_field_op(struct context *ctx, const struct token *token,
 		   unsigned int ent, char *buf, unsigned int size)
 {
-	uint16_t idx = 0;
-
 	RTE_SET_USED(ctx);
 	RTE_SET_USED(token);
-	for (idx = 0; modify_field_ops[idx]; ++idx)
-		;
 	if (!buf)
-		return idx + 1;
-	if (ent < idx)
+		return RTE_DIM(modify_field_ops);
+	if (ent < RTE_DIM(modify_field_ops) - 1)
 		return strlcpy(buf, modify_field_ops[ent], size);
 	return -1;
 }
@@ -7848,16 +7868,17 @@ static int
 comp_set_modify_field_id(struct context *ctx, const struct token *token,
 		   unsigned int ent, char *buf, unsigned int size)
 {
-	uint16_t idx = 0;
+	const char *name;
 
-	RTE_SET_USED(ctx);
 	RTE_SET_USED(token);
-	for (idx = 0; modify_field_ids[idx]; ++idx)
-		;
 	if (!buf)
-		return idx + 1;
-	if (ent < idx)
-		return strlcpy(buf, modify_field_ids[ent], size);
+		return RTE_DIM(modify_field_ids);
+	if (ent >= RTE_DIM(modify_field_ids) - 1)
+		return -1;
+	name = modify_field_ids[ent];
+	if (ctx->curr == ACTION_MODIFY_FIELD_SRC_TYPE ||
+	    (strcmp(name, "pointer") && strcmp(name, "value")))
+		return strlcpy(buf, name, size);
 	return -1;
 }
 
-- 
2.18.1

[dpdk-dev] [PATCH 3/3] app/testpmd: fix hex string parser in flow commands

[Viacheslav Ovsiienko]

The hexadecimal string parser does not check the target
field buffer size, buffer overflow happens and might
cause the application failure (segmentation fault
is observed usually).

Fixes: 169a9fed1f4c ("app/testpmd: fix hex string parser support for flow API")
Cc: stable@dpdk.org

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 736029c4fd..6827d9228f 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -7291,10 +7291,13 @@ parse_hex(struct context *ctx, const struct token *token,
 		hexlen -= 2;
 	}
 	if (hexlen > length)
-		return -1;
+		goto error;
 	ret = parse_hex_string(str, hex_tmp, &hexlen);
 	if (ret < 0)
 		goto error;
+	/* Check the converted binary fits into data buffer. */
+	if (hexlen > size)
+		goto error;
 	/* Let parse_int() fill length information first. */
 	ret = snprintf(tmp, sizeof(tmp), "%u", hexlen);
 	if (ret < 0)
-- 
2.18.1

Re: [dpdk-dev] [PATCH 1/3] ethdev: update modify field flow action

[Ori Kam]

Hi Slava,

> -----Original Message-----
> From: Slava Ovsiienko <viacheslavo@nvidia.com>
> Sent: Friday, October 1, 2021 10:52 PM
> Subject: [PATCH 1/3] ethdev: update modify field flow action
> 
> The generic modify field flow action introduced in [1] has some issues related
> to the immediate source operand:
> 
>   - immediate source can be presented either as an unsigned
>     64-bit integer or pointer to data pattern in memory.
>     There was no explicit pointer field defined in the union
> 
>   - the byte ordering for 64-bit integer was not specified.
>     Many fields have lesser lengths and byte ordering
>     is crucial.
> 
>   - how the bit offset is applied to the immediate source
>     field was not defined and documented
> 
>   - 64-bit integer size is not enough to provide MAC and

I think for mac it is enough.

>     IPv6 addresses
> 
> In order to cover the issues and exclude any ambiguities the following is
> done:
> 
>   - introduce the explicit pointer field
>     in rte_flow_action_modify_data structure
> 
>   - replace the 64-bit unsigned integer with 16-byte array
> 
>   - update the modify field flow action documentation
> 
> [1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
>  doc/guides/prog_guide/rte_flow.rst     |  8 ++++++++
>  doc/guides/rel_notes/release_21_11.rst |  7 +++++++
>  lib/ethdev/rte_flow.h                  | 15 ++++++++++++---
>  3 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> index 2b42d5ec8c..a54760a7b4 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -2835,6 +2835,14 @@ a packet to any other part of it.
>  ``value`` sets an immediate value to be used as a source or points to a
> location of the value in memory. It is used instead of ``level`` and ``offset``
> for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER``
> respectively.
> +The data in memory should be presented exactly in the same byte order
> +and length as in the relevant flow item, i.e. data for field with type
> +RTE_FLOW_FIELD_MAC_DST should follow the conventions of dst field in
> +rte_flow_item_eth structure, with type RTE_FLOW_FIELD_IPV6_SRC -
> +rte_flow_item_ipv6 conventions, and so on. The bitfield exatracted from
> +the memory being applied as second operation parameter is defined by
> +width and the destination field offset. If the field size is large than
> +16 bytes the pattern can be provided as pointer only.
> 
You should specify where is the offset of the src is taken from.
Per your example if the application wants to change the 2 byte of source mac
it should giveas an imidate value 6 bytes, with the second byte as the new value to set
so from where do it takes the offset? Since offset is not valid in case of immediate value.
I assume it is based on the offset of the destination.

>  .. _table_rte_flow_action_modify_field:
> 
> diff --git a/doc/guides/rel_notes/release_21_11.rst
> b/doc/guides/rel_notes/release_21_11.rst
> index 73e377a007..7db6cccab0 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -170,6 +170,10 @@ API Changes
>    the crypto/security operation. This field will be used to communicate
>    events such as soft expiry with IPsec in lookaside mode.
> 
> +* ethdev: ``rte_flow_action_modify_data`` structure udpdated, immediate
> +data
> +  array is extended, data pointer field is explicitly added to union,
> +the
> +  action behavior is defined in more strict fashion and documentation
> uddated.
> +
Uddated ->updated?
I think it is important to document here that the behavior has changed,
from seting only the relevant value to update to setting all the field and
the mask is done internally.

> 
>  ABI Changes
>  -----------
> @@ -206,6 +210,9 @@ ABI Changes
>    and hard expiry limits. Limits can be either in number of packets or bytes.
> 
> 
> +* ethdev: ``rte_flow_action_modify_data`` structure udpdated.
> +
> +
>  Known Issues
>  ------------
> 
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
> 7b1ed7f110..af4c693ead 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -3204,6 +3204,9 @@ enum rte_flow_field_id {  };
> 
>  /**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
>   * Field description for MODIFY_FIELD action.
>   */
>  struct rte_flow_action_modify_data {
> @@ -3217,10 +3220,16 @@ struct rte_flow_action_modify_data {
>  			uint32_t offset;
>  		};
>  		/**
> -		 * Immediate value for RTE_FLOW_FIELD_VALUE or
> -		 * memory address for RTE_FLOW_FIELD_POINTER.
> +		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented
> in the
> +		 * same byte order and length as in relevant
> rte_flow_item_xxx.

Please see my comment about how to get the offset.

>  		 */
> -		uint64_t value;
> +		uint8_t value[16];
> +		/*
> +		 * Memory address for RTE_FLOW_FIELD_POINTER, memory
> layout
> +		 * should be the same as for relevant field in the
> +		 * rte_flow_item_xxx structure.

I assume also in this case the offset comes from the dest right?

> +		 */
> +		void *pvalue;
>  	};
>  };
> 
> --
> 2.18.1

Thanks,
Ori

[dpdk-dev] [PATCH v2 0/5] ethdev: update modify field flow action

[Viacheslav Ovsiienko]

The generic modify field flow action introduced in [1] has
some issues related to the immediate source operand:

  - immediate source can be presented either as an unsigned
    64-bit integer or pointer to data pattern in memory.
    There was no explicit pointer field defined in the union

  - the byte ordering for 64-bit integer was not specified.
    Many fields have lesser lengths and byte ordering
    is crucial.

  - how the bit offset is applied to the immediate source
    field was not defined and documented

  - 64-bit integer size is not enough to provide MAC and
    IPv6 addresses

In order to cover the issues and exclude any ambiguities
the following is done:

  - introduce the explicit pointer field
    in rte_flow_action_modify_data structure

  - replace the 64-bit unsigned integer with 16-byte array

  - update the modify field flow action documentation

[1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
[2] RFC: http://patches.dpdk.org/project/dpdk/patch/20210910141609.8410-1-viacheslavo@nvidia.com/
[3] Deprecation notice: http://patches.dpdk.org/project/dpdk/patch/20210803085754.643180-1-orika@nvidia.com/
[4] v1 - http://patches.dpdk.org/project/dpdk/cover/20211001195223.31909-1-viacheslavo@nvidia.com/
    
v2: - comments addressed
    - documentation updated
    - typos fixed
    - mlx5 PMD updated

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>

Viacheslav Ovsiienko (5):
  ethdev: update modify field flow action
  app/testpmd: update modify field flow action support
  app/testpmd: fix hex string parser in flow commands
  net/mlx5: update modify field action
  doc: remove modify field action data deprecation notice

 app/test-pmd/cmdline_flow.c            | 60 ++++++++++++++++++--------
 doc/guides/prog_guide/rte_flow.rst     | 16 +++++++
 doc/guides/rel_notes/deprecation.rst   |  4 --
 doc/guides/rel_notes/release_21_11.rst |  9 ++++
 drivers/net/mlx5/mlx5_flow_dv.c        | 50 ++++++++++++---------
 lib/ethdev/rte_flow.h                  | 17 ++++++--
 6 files changed, 110 insertions(+), 46 deletions(-)

-- 
2.18.1

[dpdk-dev] [PATCH v2 1/5] ethdev: update modify field flow action

[Viacheslav Ovsiienko]

The generic modify field flow action introduced in [1] has
some issues related to the immediate source operand:

  - immediate source can be presented either as an unsigned
    64-bit integer or pointer to data pattern in memory.
    There was no explicit pointer field defined in the union.

  - the byte ordering for 64-bit integer was not specified.
    Many fields have shorter lengths and byte ordering
    is crucial.

  - how the bit offset is applied to the immediate source
    field was not defined and documented.

  - 64-bit integer size is not enough to provide IPv6
    addresses.

In order to cover the issues and exclude any ambiguities
the following is done:

  - introduce the explicit pointer field
    in rte_flow_action_modify_data structure

  - replace the 64-bit unsigned integer with 16-byte array

  - update the modify field flow action documentation

[1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 doc/guides/prog_guide/rte_flow.rst     | 16 ++++++++++++++++
 doc/guides/rel_notes/release_21_11.rst |  9 +++++++++
 lib/ethdev/rte_flow.h                  | 17 ++++++++++++++---
 3 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 2b42d5ec8c..1ceecb399f 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -2835,6 +2835,22 @@ a packet to any other part of it.
 ``value`` sets an immediate value to be used as a source or points to a
 location of the value in memory. It is used instead of ``level`` and ``offset``
 for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER`` respectively.
+The data in memory should be presented exactly in the same byte order and
+length as in the relevant flow item, i.e. data for field with type
+RTE_FLOW_FIELD_MAC_DST should follow the conventions of dst field
+in rte_flow_item_eth structure, with type RTE_FLOW_FIELD_IPV6_SRC -
+rte_flow_item_ipv6 conventions, and so on. If the field size is large than
+16 bytes the pattern can be provided as pointer only.
+
+The bitfield extracted from the memory being applied as second operation
+parameter is defined by action width and by the destination field offset.
+Application should provide the data in immediate value memory (either as
+buffer or by pointer) exactly as item field without any applied explicit offset,
+and destination packet field (with specified width and bit offset) will be
+replaced by immediate source bits from the same bit offset. For example,
+to replace the third byte of MAC address with value 0x85, application should
+specify destination width as 8, destination width as 16, and provide immediate
+value as sequence of bytes {xxx, xxx, 0x85, xxx, xxx, xxx}.
 
 .. _table_rte_flow_action_modify_field:
 
diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index dfc2cbdeed..41a087d7c1 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -187,6 +187,13 @@ API Changes
   the crypto/security operation. This field will be used to communicate
   events such as soft expiry with IPsec in lookaside mode.
 
+* ethdev: ``rte_flow_action_modify_data`` structure udpdated, immediate data
+  array is extended, data pointer field is explicitly added to union, the
+  action behavior is defined in more strict fashion and documentation updated.
+  The immediate value behavior has been changed, the entire immediate field
+  should be provided, and offset for immediate source bitfield is assigned
+  from destination one.
+
 
 ABI Changes
 -----------
@@ -222,6 +229,8 @@ ABI Changes
   ``rte_security_ipsec_xform`` to allow applications to configure SA soft
   and hard expiry limits. Limits can be either in number of packets or bytes.
 
+* ethdev: ``rte_flow_action_modify_data`` structure udpdated.
+
 
 Known Issues
 ------------
diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
index 7b1ed7f110..953924d42b 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -3204,6 +3204,9 @@ enum rte_flow_field_id {
 };
 
 /**
+ * @warning
+ * @b EXPERIMENTAL: this structure may change without prior notice
+ *
  * Field description for MODIFY_FIELD action.
  */
 struct rte_flow_action_modify_data {
@@ -3217,10 +3220,18 @@ struct rte_flow_action_modify_data {
 			uint32_t offset;
 		};
 		/**
-		 * Immediate value for RTE_FLOW_FIELD_VALUE or
-		 * memory address for RTE_FLOW_FIELD_POINTER.
+		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented in the
+		 * same byte order and length as in relevant rte_flow_item_xxx.
+		 * The immediate source bitfield offset is inherited from
+		 * the destination's one.
 		 */
-		uint64_t value;
+		uint8_t value[16];
+		/*
+		 * Memory address for RTE_FLOW_FIELD_POINTER, memory layout
+		 * should be the same as for relevant field in the
+		 * rte_flow_item_xxx structure.
+		 */
+		void *pvalue;
 	};
 };
 
-- 
2.18.1

[dpdk-dev] [PATCH v2 2/5] app/testpmd: update modify field flow action support

[Viacheslav Ovsiienko]

The testpmd flow create command updates provided:

  - modify field action supports the updated actions
  - pointer type added for action source field
  - pointer and value source field takes hex string
    instead of unsigned int in host endianness

There are some examples of flow with update modified
field action:

1. IPv6 destination address bytes 4-7 assignment:
   0000::1111 - > 0000:xxxx:4455:6677::1111

   flow create 0 egress group 1
     pattern eth / ipv6 dst is 0000::1111 / udp / end
     actions modify_field op set
             dst_type ipv6_dst
	     dst_offset 32
             src_type value
             src_value 0011223344556677
	     width 32 / end

2. Copy second byte of IPv4 destination address to the
   third byte of source address:
     10.0.118.4 -> 192.168.100.1
     10.0.168.4 -> 192.168.100.1

   flow create 0 egress group 1
     pattern eth / ipv4 / udp / end
     actions modify_field op set
             dst_type ipv4_src
             dst_offset 16
	     src_type ipv4_dst
	     src_offset 8
	     width 8 / end

3. Assign METADATA value with 11223344 value from the
   hex string in the linear buffer. Please note, the value
   definition should follow host-endian, example is given
   for x86 (little-endian):

   flow create 0 egress group 1
     pattern eth / ipv4 / end
     actions modify_field op set
             dst_type meta
	     src_type pointer
	     src_ptr 44332211
	     width 32 / end

4. Assign destination MAC with EA:11:0B:AD:0B:ED value:

   flow create 0 egress group 1
     pattern eth / end
     actions modify_field op set
             dst_type mac_dst
	     src_type value
	     src_value EA110BAD0BED
	     width 48 / end

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 55 +++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index bb22294dd3..736029c4fd 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -448,6 +448,7 @@ enum index {
 	ACTION_MODIFY_FIELD_SRC_LEVEL,
 	ACTION_MODIFY_FIELD_SRC_OFFSET,
 	ACTION_MODIFY_FIELD_SRC_VALUE,
+	ACTION_MODIFY_FIELD_SRC_POINTER,
 	ACTION_MODIFY_FIELD_WIDTH,
 	ACTION_CONNTRACK,
 	ACTION_CONNTRACK_UPDATE,
@@ -468,6 +469,14 @@ enum index {
 #define ITEM_RAW_SIZE \
 	(sizeof(struct rte_flow_item_raw) + ITEM_RAW_PATTERN_SIZE)
 
+/** Maximum size for external pattern in struct rte_flow_action_modify_data. */
+#define ACTION_MODIFY_PATTERN_SIZE 32
+
+/** Storage size for struct rte_flow_action_modify_field including pattern. */
+#define ACTION_MODIFY_SIZE \
+	(sizeof(struct rte_flow_action_modify_field) + \
+	ACTION_MODIFY_PATTERN_SIZE)
+
 /** Maximum number of queue indices in struct rte_flow_action_rss. */
 #define ACTION_RSS_QUEUE_NUM 128
 
@@ -1704,6 +1713,7 @@ static const enum index action_modify_field_src[] = {
 	ACTION_MODIFY_FIELD_SRC_LEVEL,
 	ACTION_MODIFY_FIELD_SRC_OFFSET,
 	ACTION_MODIFY_FIELD_SRC_VALUE,
+	ACTION_MODIFY_FIELD_SRC_POINTER,
 	ACTION_MODIFY_FIELD_WIDTH,
 	ZERO,
 };
@@ -4455,8 +4465,7 @@ static const struct token token_list[] = {
 	[ACTION_MODIFY_FIELD] = {
 		.name = "modify_field",
 		.help = "modify destination field with data from source field",
-		.priv = PRIV_ACTION(MODIFY_FIELD,
-			sizeof(struct rte_flow_action_modify_field)),
+		.priv = PRIV_ACTION(MODIFY_FIELD, ACTION_MODIFY_SIZE),
 		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_OP)),
 		.call = parse_vc,
 	},
@@ -4539,11 +4548,26 @@ static const struct token token_list[] = {
 		.name = "src_value",
 		.help = "source immediate value",
 		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_WIDTH),
-			NEXT_ENTRY(COMMON_UNSIGNED)),
-		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_modify_field,
+			     NEXT_ENTRY(COMMON_HEX)),
+		.args = ARGS(ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY(struct rte_flow_action_modify_field,
 					src.value)),
 		.call = parse_vc_conf,
 	},
+	[ACTION_MODIFY_FIELD_SRC_POINTER] = {
+		.name = "src_ptr",
+		.help = "pointer to source immediate value",
+		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_WIDTH),
+			     NEXT_ENTRY(COMMON_HEX)),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_modify_field,
+					src.pvalue),
+			     ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY_ARB
+				(sizeof(struct rte_flow_action_modify_field),
+				 ACTION_MODIFY_PATTERN_SIZE)),
+		.call = parse_vc_conf,
+	},
 	[ACTION_MODIFY_FIELD_WIDTH] = {
 		.name = "width",
 		.help = "number of bits to copy",
@@ -7830,15 +7854,11 @@ static int
 comp_set_modify_field_op(struct context *ctx, const struct token *token,
 		   unsigned int ent, char *buf, unsigned int size)
 {
-	uint16_t idx = 0;
-
 	RTE_SET_USED(ctx);
 	RTE_SET_USED(token);
-	for (idx = 0; modify_field_ops[idx]; ++idx)
-		;
 	if (!buf)
-		return idx + 1;
-	if (ent < idx)
+		return RTE_DIM(modify_field_ops);
+	if (ent < RTE_DIM(modify_field_ops) - 1)
 		return strlcpy(buf, modify_field_ops[ent], size);
 	return -1;
 }
@@ -7848,16 +7868,17 @@ static int
 comp_set_modify_field_id(struct context *ctx, const struct token *token,
 		   unsigned int ent, char *buf, unsigned int size)
 {
-	uint16_t idx = 0;
+	const char *name;
 
-	RTE_SET_USED(ctx);
 	RTE_SET_USED(token);
-	for (idx = 0; modify_field_ids[idx]; ++idx)
-		;
 	if (!buf)
-		return idx + 1;
-	if (ent < idx)
-		return strlcpy(buf, modify_field_ids[ent], size);
+		return RTE_DIM(modify_field_ids);
+	if (ent >= RTE_DIM(modify_field_ids) - 1)
+		return -1;
+	name = modify_field_ids[ent];
+	if (ctx->curr == ACTION_MODIFY_FIELD_SRC_TYPE ||
+	    (strcmp(name, "pointer") && strcmp(name, "value")))
+		return strlcpy(buf, name, size);
 	return -1;
 }
 
-- 
2.18.1

[dpdk-dev] [PATCH v2 3/5] app/testpmd: fix hex string parser in flow commands

[Viacheslav Ovsiienko]

The hexadecimal string parser does not check the target
field buffer size, buffer overflow happens and might
cause the application failure (segmentation fault
is observed usually).

Fixes: 169a9fed1f4c ("app/testpmd: fix hex string parser support for flow API")
Cc: stable@dpdk.org

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 736029c4fd..6827d9228f 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -7291,10 +7291,13 @@ parse_hex(struct context *ctx, const struct token *token,
 		hexlen -= 2;
 	}
 	if (hexlen > length)
-		return -1;
+		goto error;
 	ret = parse_hex_string(str, hex_tmp, &hexlen);
 	if (ret < 0)
 		goto error;
+	/* Check the converted binary fits into data buffer. */
+	if (hexlen > size)
+		goto error;
 	/* Let parse_int() fill length information first. */
 	ret = snprintf(tmp, sizeof(tmp), "%u", hexlen);
 	if (ret < 0)
-- 
2.18.1

[dpdk-dev] [PATCH v2 4/5] net/mlx5: update modify field action

[Viacheslav Ovsiienko]

Update immediate value/pointer source operand support
for modify field RTE Flow action:

  - source operand data can be presented by byte buffer
    (instead of former uint64_t) or by pointer
  - no host byte ordering is assumed anymore for immediate
    data buffer (not uint64_t anymore)
  - no immediate value offset is expected

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow_dv.c | 50 +++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index c6370cd1d6..867f587960 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -1391,7 +1391,7 @@ flow_dv_convert_action_modify_ipv6_dscp
 
 static int
 mlx5_flow_item_field_width(struct mlx5_priv *priv,
-			   enum rte_flow_field_id field)
+			   enum rte_flow_field_id field, int inherit)
 {
 	switch (field) {
 	case RTE_FLOW_FIELD_START:
@@ -1442,7 +1442,8 @@ mlx5_flow_item_field_width(struct mlx5_priv *priv,
 		return __builtin_popcount(priv->sh->dv_meta_mask);
 	case RTE_FLOW_FIELD_POINTER:
 	case RTE_FLOW_FIELD_VALUE:
-		return 64;
+		MLX5_ASSERT(inherit >= 0);
+		return inherit < 0 ? 0 : inherit;
 	default:
 		MLX5_ASSERT(false);
 	}
@@ -1462,7 +1463,8 @@ mlx5_flow_field_id_to_modify_info
 	struct mlx5_priv *priv = dev->data->dev_private;
 	uint32_t idx = 0;
 	uint32_t off = 0;
-	uint64_t val = 0;
+	uint8_t *pval;
+
 	switch (data->field) {
 	case RTE_FLOW_FIELD_START:
 		/* not supported yet */
@@ -1838,32 +1840,37 @@ mlx5_flow_field_id_to_modify_info
 		break;
 	case RTE_FLOW_FIELD_POINTER:
 	case RTE_FLOW_FIELD_VALUE:
-		if (data->field == RTE_FLOW_FIELD_POINTER)
-			memcpy(&val, (void *)(uintptr_t)data->value,
-			       sizeof(uint64_t));
-		else
-			val = data->value;
+		pval = data->field == RTE_FLOW_FIELD_POINTER ?
+		       (uint8_t *)(uintptr_t)data->pvalue :
+		       (uint8_t *)(uintptr_t)&data->value;
 		for (idx = 0; idx < MLX5_ACT_MAX_MOD_FIELDS; idx++) {
+			if (!dst_width)
+				break;
 			if (mask[idx]) {
 				if (dst_width == 48) {
 					/*special case for MAC addresses */
-					value[idx] = rte_cpu_to_be_16(val);
-					val >>= 16;
+					value[idx] = rte_cpu_to_be_16
+						(*(unaligned_uint16_t *)pval);
+					pval += sizeof(uint16_t);
 					dst_width -= 16;
 				} else if (dst_width > 16) {
-					value[idx] = rte_cpu_to_be_32(val);
-					val >>= 32;
+					value[idx] = rte_cpu_to_be_32
+						(*(unaligned_uint32_t *)pval);
+					pval += sizeof(uint32_t);
+					dst_width -= RTE_MIN(32u, dst_width);
 				} else if (dst_width > 8) {
-					value[idx] = rte_cpu_to_be_16(val);
-					val >>= 16;
+					value[idx] = rte_cpu_to_be_16
+						(*(unaligned_uint16_t *)pval);
+					pval += sizeof(uint16_t);
+					dst_width -= RTE_MIN(16u, dst_width);
 				} else {
-					value[idx] = (uint8_t)val;
-					val >>= 8;
+					value[idx] = *pval++;
+					dst_width -= RTE_MIN(8u, dst_width);
 				}
 				if (*shift)
 					value[idx] <<= *shift;
-				if (!val)
-					break;
+			} else {
+				pval += sizeof(uint32_t);
 			}
 		}
 		break;
@@ -1910,8 +1917,9 @@ flow_dv_convert_action_modify_field
 	uint32_t value[MLX5_ACT_MAX_MOD_FIELDS] = {0, 0, 0, 0, 0};
 	uint32_t type;
 	uint32_t shift = 0;
-	uint32_t dst_width = mlx5_flow_item_field_width(priv, conf->dst.field);
+	uint32_t dst_width;
 
+	dst_width = mlx5_flow_item_field_width(priv, conf->dst.field, -1);
 	if (conf->src.field == RTE_FLOW_FIELD_POINTER ||
 		conf->src.field == RTE_FLOW_FIELD_VALUE) {
 		type = MLX5_MODIFICATION_TYPE_SET;
@@ -4874,9 +4882,9 @@ flow_dv_validate_action_modify_field(struct rte_eth_dev *dev,
 	const struct rte_flow_action_modify_field *action_modify_field =
 		action->conf;
 	uint32_t dst_width = mlx5_flow_item_field_width(priv,
-				action_modify_field->dst.field);
+				action_modify_field->dst.field, -1);
 	uint32_t src_width = mlx5_flow_item_field_width(priv,
-				action_modify_field->src.field);
+				action_modify_field->src.field, dst_width);
 
 	ret = flow_dv_validate_action_modify_hdr(action_flags, action, error);
 	if (ret)
-- 
2.18.1

[dpdk-dev] [PATCH v2 5/5] doc: remove modify field action data deprecation notice

[Viacheslav Ovsiienko]

The generic modify field flow action introduced
is updated, deprecation notice should be removed.

Fixes: 2ba49b5f3721 ("doc: announce change to ethdev modify action data")

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 doc/guides/rel_notes/deprecation.rst | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index a2fe766d4b..dee14077a5 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -120,10 +120,6 @@ Deprecation Notices
 * ethdev: Announce moving from dedicated modify function for each field,
   to using the general ``rte_flow_modify_field`` action.
 
-* ethdev: The struct ``rte_flow_action_modify_data`` will be modified
-  to support modifying fields larger than 64 bits.
-  In addition, documentation will be updated to clarify byte order.
-
 * ethdev: Attribute ``shared`` of the ``struct rte_flow_action_count``
   is deprecated and will be removed in DPDK 21.11. Shared counters should
   be managed using shared actions API (``rte_flow_shared_action_create`` etc).
-- 
2.18.1

Re: [dpdk-dev] [PATCH v2 1/5] ethdev: update modify field flow action

[Ori Kam]

Hi Slava,

> -----Original Message-----
> From: Slava Ovsiienko <viacheslavo@nvidia.com>
> Sent: Monday, October 11, 2021 2:46 AM
> Subject: [PATCH v2 1/5] ethdev: update modify field flow action
> 
> The generic modify field flow action introduced in [1] has some issues related to the immediate source
> operand:
> 
>   - immediate source can be presented either as an unsigned
>     64-bit integer or pointer to data pattern in memory.
>     There was no explicit pointer field defined in the union.
> 
>   - the byte ordering for 64-bit integer was not specified.
>     Many fields have shorter lengths and byte ordering
>     is crucial.
> 
>   - how the bit offset is applied to the immediate source
>     field was not defined and documented.
> 
>   - 64-bit integer size is not enough to provide IPv6
>     addresses.
> 
> In order to cover the issues and exclude any ambiguities the following is done:
> 
>   - introduce the explicit pointer field
>     in rte_flow_action_modify_data structure
> 
>   - replace the 64-bit unsigned integer with 16-byte array
> 
>   - update the modify field flow action documentation
> 
> [1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
>  doc/guides/prog_guide/rte_flow.rst     | 16 ++++++++++++++++
>  doc/guides/rel_notes/release_21_11.rst |  9 +++++++++
>  lib/ethdev/rte_flow.h                  | 17 ++++++++++++++---
>  3 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 2b42d5ec8c..1ceecb399f 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -2835,6 +2835,22 @@ a packet to any other part of it.
>  ``value`` sets an immediate value to be used as a source or points to a  location of the value in memory. It
> is used instead of ``level`` and ``offset``  for ``RTE_FLOW_FIELD_VALUE`` and
> ``RTE_FLOW_FIELD_POINTER`` respectively.
> +The data in memory should be presented exactly in the same byte order
> +and length as in the relevant flow item, i.e. data for field with type
> +RTE_FLOW_FIELD_MAC_DST should follow the conventions of dst field in
> +rte_flow_item_eth structure, with type RTE_FLOW_FIELD_IPV6_SRC -
> +rte_flow_item_ipv6 conventions, and so on. If the field size is large
> +than
> +16 bytes the pattern can be provided as pointer only.
> +
> +The bitfield extracted from the memory being applied as second
> +operation parameter is defined by action width and by the destination field offset.
> +Application should provide the data in immediate value memory (either
> +as buffer or by pointer) exactly as item field without any applied
> +explicit offset, and destination packet field (with specified width and
> +bit offset) will be replaced by immediate source bits from the same bit
> +offset. For example, to replace the third byte of MAC address with
> +value 0x85, application should specify destination width as 8,
> +destination width as 16, and provide immediate value as sequence of bytes {xxx, xxx, 0x85, xxx, xxx, xxx}.
> 

I think you have a typo destination width as 16 should be destination offset as 16.
If fixed you can add my ack to the next version.

Best,
Ori

Re: [dpdk-dev] [PATCH v2 2/5] app/testpmd: update modify field flow action support

[Ori Kam]

Hi Slava,


> -----Original Message-----
> From: Slava Ovsiienko <viacheslavo@nvidia.com>
> Sent: Monday, October 11, 2021 2:46 AM
> Subject: [PATCH v2 2/5] app/testpmd: update modify field flow action support
> 
> The testpmd flow create command updates provided:
> 
>   - modify field action supports the updated actions
>   - pointer type added for action source field
>   - pointer and value source field takes hex string
>     instead of unsigned int in host endianness
> 
> There are some examples of flow with update modified field action:
> 
> 1. IPv6 destination address bytes 4-7 assignment:
>    0000::1111 - > 0000:xxxx:4455:6677::1111
> 
>    flow create 0 egress group 1
>      pattern eth / ipv6 dst is 0000::1111 / udp / end
>      actions modify_field op set
>              dst_type ipv6_dst
> 	     dst_offset 32
>              src_type value
>              src_value 0011223344556677
> 	     width 32 / end
> 
> 2. Copy second byte of IPv4 destination address to the
>    third byte of source address:
>      10.0.118.4 -> 192.168.100.1
>      10.0.168.4 -> 192.168.100.1
> 
>    flow create 0 egress group 1
>      pattern eth / ipv4 / udp / end
>      actions modify_field op set
>              dst_type ipv4_src
>              dst_offset 16
> 	     src_type ipv4_dst
> 	     src_offset 8
> 	     width 8 / end
> 
> 3. Assign METADATA value with 11223344 value from the
>    hex string in the linear buffer. Please note, the value
>    definition should follow host-endian, example is given
>    for x86 (little-endian):
> 
>    flow create 0 egress group 1
>      pattern eth / ipv4 / end
>      actions modify_field op set
>              dst_type meta
> 	     src_type pointer
> 	     src_ptr 44332211
> 	     width 32 / end
> 
> 4. Assign destination MAC with EA:11:0B:AD:0B:ED value:
> 
>    flow create 0 egress group 1
>      pattern eth / end
>      actions modify_field op set
>              dst_type mac_dst
> 	     src_type value
> 	     src_value EA110BAD0BED
> 	     width 48 / end
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
>  app/test-pmd/cmdline_flow.c | 55 +++++++++++++++++++++++++------------
>  1 file changed, 38 insertions(+), 17 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c index
> bb22294dd3..736029c4fd 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -448,6 +448,7 @@ enum index {
>  	ACTION_MODIFY_FIELD_SRC_LEVEL,
>  	ACTION_MODIFY_FIELD_SRC_OFFSET,
>  	ACTION_MODIFY_FIELD_SRC_VALUE,
> +	ACTION_MODIFY_FIELD_SRC_POINTER,
>  	ACTION_MODIFY_FIELD_WIDTH,
>  	ACTION_CONNTRACK,
>  	ACTION_CONNTRACK_UPDATE,
> @@ -468,6 +469,14 @@ enum index {
>  #define ITEM_RAW_SIZE \
>  	(sizeof(struct rte_flow_item_raw) + ITEM_RAW_PATTERN_SIZE)
> 
> +/** Maximum size for external pattern in struct
> +rte_flow_action_modify_data. */ #define ACTION_MODIFY_PATTERN_SIZE 32
> +
> +/** Storage size for struct rte_flow_action_modify_field including
> +pattern. */ #define ACTION_MODIFY_SIZE \
> +	(sizeof(struct rte_flow_action_modify_field) + \
> +	ACTION_MODIFY_PATTERN_SIZE)
> +
>  /** Maximum number of queue indices in struct rte_flow_action_rss. */  #define
> ACTION_RSS_QUEUE_NUM 128
> 
> @@ -1704,6 +1713,7 @@ static const enum index action_modify_field_src[] = {
>  	ACTION_MODIFY_FIELD_SRC_LEVEL,
>  	ACTION_MODIFY_FIELD_SRC_OFFSET,
>  	ACTION_MODIFY_FIELD_SRC_VALUE,
> +	ACTION_MODIFY_FIELD_SRC_POINTER,
>  	ACTION_MODIFY_FIELD_WIDTH,
>  	ZERO,
>  };
> @@ -4455,8 +4465,7 @@ static const struct token token_list[] = {
>  	[ACTION_MODIFY_FIELD] = {
>  		.name = "modify_field",
>  		.help = "modify destination field with data from source field",
> -		.priv = PRIV_ACTION(MODIFY_FIELD,
> -			sizeof(struct rte_flow_action_modify_field)),
> +		.priv = PRIV_ACTION(MODIFY_FIELD, ACTION_MODIFY_SIZE),
>  		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_OP)),
>  		.call = parse_vc,
>  	},
> @@ -4539,11 +4548,26 @@ static const struct token token_list[] = {
>  		.name = "src_value",
>  		.help = "source immediate value",
>  		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_WIDTH),
> -			NEXT_ENTRY(COMMON_UNSIGNED)),
> -		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_modify_field,
> +			     NEXT_ENTRY(COMMON_HEX)),
> +		.args = ARGS(ARGS_ENTRY_ARB(0, 0),
> +			     ARGS_ENTRY_ARB(0, 0),
> +			     ARGS_ENTRY(struct rte_flow_action_modify_field,
>  					src.value)),
>  		.call = parse_vc_conf,
>  	},
> +	[ACTION_MODIFY_FIELD_SRC_POINTER] = {
> +		.name = "src_ptr",
> +		.help = "pointer to source immediate value",
> +		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_WIDTH),
> +			     NEXT_ENTRY(COMMON_HEX)),
> +		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_modify_field,
> +					src.pvalue),
> +			     ARGS_ENTRY_ARB(0, 0),
> +			     ARGS_ENTRY_ARB
> +				(sizeof(struct rte_flow_action_modify_field),
> +				 ACTION_MODIFY_PATTERN_SIZE)),
> +		.call = parse_vc_conf,
> +	},
>  	[ACTION_MODIFY_FIELD_WIDTH] = {
>  		.name = "width",
>  		.help = "number of bits to copy",
> @@ -7830,15 +7854,11 @@ static int
>  comp_set_modify_field_op(struct context *ctx, const struct token *token,
>  		   unsigned int ent, char *buf, unsigned int size)  {
> -	uint16_t idx = 0;
> -
>  	RTE_SET_USED(ctx);
>  	RTE_SET_USED(token);
> -	for (idx = 0; modify_field_ops[idx]; ++idx)
> -		;
>  	if (!buf)
> -		return idx + 1;
> -	if (ent < idx)
> +		return RTE_DIM(modify_field_ops);
> +	if (ent < RTE_DIM(modify_field_ops) - 1)
>  		return strlcpy(buf, modify_field_ops[ent], size);
>  	return -1;
>  }
> @@ -7848,16 +7868,17 @@ static int
>  comp_set_modify_field_id(struct context *ctx, const struct token *token,
>  		   unsigned int ent, char *buf, unsigned int size)  {
> -	uint16_t idx = 0;
> +	const char *name;
> 
> -	RTE_SET_USED(ctx);
>  	RTE_SET_USED(token);
> -	for (idx = 0; modify_field_ids[idx]; ++idx)
> -		;
>  	if (!buf)
> -		return idx + 1;
> -	if (ent < idx)
> -		return strlcpy(buf, modify_field_ids[ent], size);
> +		return RTE_DIM(modify_field_ids);
> +	if (ent >= RTE_DIM(modify_field_ids) - 1)
> +		return -1;
> +	name = modify_field_ids[ent];
> +	if (ctx->curr == ACTION_MODIFY_FIELD_SRC_TYPE ||
> +	    (strcmp(name, "pointer") && strcmp(name, "value")))
> +		return strlcpy(buf, name, size);
>  	return -1;
>  }
> 
> --
> 2.18.1

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

Re: [dpdk-dev] [PATCH v2 5/5] doc: remove modify field action data deprecation notice

[Ori Kam]

Hi Slava,

> -----Original Message-----
> From: Slava Ovsiienko <viacheslavo@nvidia.com>
> Sent: Monday, October 11, 2021 2:46 AM
> Subject: [PATCH v2 5/5] doc: remove modify field action data deprecation notice
> 
> The generic modify field flow action introduced is updated, deprecation notice should be removed.
> 
> Fixes: 2ba49b5f3721 ("doc: announce change to ethdev modify action data")
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index a2fe766d4b..dee14077a5 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -120,10 +120,6 @@ Deprecation Notices
>  * ethdev: Announce moving from dedicated modify function for each field,
>    to using the general ``rte_flow_modify_field`` action.
> 
> -* ethdev: The struct ``rte_flow_action_modify_data`` will be modified
> -  to support modifying fields larger than 64 bits.
> -  In addition, documentation will be updated to clarify byte order.
> -
>  * ethdev: Attribute ``shared`` of the ``struct rte_flow_action_count``
>    is deprecated and will be removed in DPDK 21.11. Shared counters should
>    be managed using shared actions API (``rte_flow_shared_action_create`` etc).
> --
> 2.18.1

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

Re: [dpdk-dev] [PATCH v2 3/5] app/testpmd: fix hex string parser in flow commands

[Ori Kam]

Hi Slava,

> -----Original Message-----
> From: Slava Ovsiienko <viacheslavo@nvidia.com>
> Sent: Monday, October 11, 2021 2:46 AM
> Subject: [PATCH v2 3/5] app/testpmd: fix hex string parser in flow commands
> 
> The hexadecimal string parser does not check the target field buffer size, buffer overflow happens and
> might cause the application failure (segmentation fault is observed usually).
> 
> Fixes: 169a9fed1f4c ("app/testpmd: fix hex string parser support for flow API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
>  app/test-pmd/cmdline_flow.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c index
> 736029c4fd..6827d9228f 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -7291,10 +7291,13 @@ parse_hex(struct context *ctx, const struct token *token,
>  		hexlen -= 2;
>  	}
>  	if (hexlen > length)
> -		return -1;
> +		goto error;
>  	ret = parse_hex_string(str, hex_tmp, &hexlen);
>  	if (ret < 0)
>  		goto error;
> +	/* Check the converted binary fits into data buffer. */
> +	if (hexlen > size)
> +		goto error;
>  	/* Let parse_int() fill length information first. */
>  	ret = snprintf(tmp, sizeof(tmp), "%u", hexlen);
>  	if (ret < 0)
> --
> 2.18.1

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

Re: [dpdk-dev] [PATCH v2 5/5] doc: remove modify field action data deprecation notice

[Andrew Rybchenko]

On 10/11/21 2:45 AM, Viacheslav Ovsiienko wrote:
> The generic modify field flow action introduced
> is updated, deprecation notice should be removed.
> 
> Fixes: 2ba49b5f3721 ("doc: announce change to ethdev modify action data")
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>

It should be done when deprecation notice is addressed.
I.e. in the first patch of the series.

Re: [dpdk-dev] [PATCH v2 1/5] ethdev: update modify field flow action

[Andrew Rybchenko]

On 10/11/21 2:45 AM, Viacheslav Ovsiienko wrote:
> The generic modify field flow action introduced in [1] has
> some issues related to the immediate source operand:
> 
>   - immediate source can be presented either as an unsigned
>     64-bit integer or pointer to data pattern in memory.
>     There was no explicit pointer field defined in the union.
> 
>   - the byte ordering for 64-bit integer was not specified.
>     Many fields have shorter lengths and byte ordering
>     is crucial.
> 
>   - how the bit offset is applied to the immediate source
>     field was not defined and documented.
> 
>   - 64-bit integer size is not enough to provide IPv6
>     addresses.
> 
> In order to cover the issues and exclude any ambiguities
> the following is done:
> 
>   - introduce the explicit pointer field
>     in rte_flow_action_modify_data structure
> 
>   - replace the 64-bit unsigned integer with 16-byte array
> 
>   - update the modify field flow action documentation
> 
> [1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
>  doc/guides/prog_guide/rte_flow.rst     | 16 ++++++++++++++++
>  doc/guides/rel_notes/release_21_11.rst |  9 +++++++++
>  lib/ethdev/rte_flow.h                  | 17 ++++++++++++++---
>  3 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 2b42d5ec8c..1ceecb399f 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -2835,6 +2835,22 @@ a packet to any other part of it.
>  ``value`` sets an immediate value to be used as a source or points to a
>  location of the value in memory. It is used instead of ``level`` and ``offset``
>  for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER`` respectively.
> +The data in memory should be presented exactly in the same byte order and
> +length as in the relevant flow item, i.e. data for field with type
> +RTE_FLOW_FIELD_MAC_DST should follow the conventions of dst field
> +in rte_flow_item_eth structure, with type RTE_FLOW_FIELD_IPV6_SRC -
> +rte_flow_item_ipv6 conventions, and so on. If the field size is large than

large -> larger

> +16 bytes the pattern can be provided as pointer only.

RTE_FLOW_FIELD_MAC_DST, dst, rte_flow_item_eth, RTE_FLOW_FIELD_IPV6_SRC,
rte_flow_item_ipv6 should be ``x``.

> +
> +The bitfield extracted from the memory being applied as second operation
> +parameter is defined by action width and by the destination field offset.
> +Application should provide the data in immediate value memory (either as
> +buffer or by pointer) exactly as item field without any applied explicit offset,
> +and destination packet field (with specified width and bit offset) will be
> +replaced by immediate source bits from the same bit offset. For example,
> +to replace the third byte of MAC address with value 0x85, application should
> +specify destination width as 8, destination width as 16, and provide immediate

destination width twice above

> +value as sequence of bytes {xxx, xxx, 0x85, xxx, xxx, xxx}.
>  
>  .. _table_rte_flow_action_modify_field:

pvalue should be added in the "destination/source field
definition".

dst and src members documentation should be improved to
highlight the difference. Destination cannot be "immediate" and
"pointer". In fact, "pointer" is a kind of "immediate". May be
it is better to use "constant value" instead of "immediate".

>  
> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
> index dfc2cbdeed..41a087d7c1 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -187,6 +187,13 @@ API Changes
>    the crypto/security operation. This field will be used to communicate
>    events such as soft expiry with IPsec in lookaside mode.
>  
> +* ethdev: ``rte_flow_action_modify_data`` structure udpdated, immediate data

udpdated -> updated

> +  array is extended, data pointer field is explicitly added to union, the
> +  action behavior is defined in more strict fashion and documentation updated.
> +  The immediate value behavior has been changed, the entire immediate field
> +  should be provided, and offset for immediate source bitfield is assigned
> +  from destination one.
> +
>  
>  ABI Changes
>  -----------
> @@ -222,6 +229,8 @@ ABI Changes
>    ``rte_security_ipsec_xform`` to allow applications to configure SA soft
>    and hard expiry limits. Limits can be either in number of packets or bytes.
>  
> +* ethdev: ``rte_flow_action_modify_data`` structure udpdated.

udpdated -> updated
I'm not sure that it makes sense to duplicate ABI changes if
API is changed.

> +
>  
>  Known Issues
>  ------------
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> index 7b1ed7f110..953924d42b 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -3204,6 +3204,9 @@ enum rte_flow_field_id {
>  };
>  
>  /**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *

Isn't a separate fix to add missing experimental header?

>   * Field description for MODIFY_FIELD action.
>   */

"Another packet field" in the next paragraph I read as
a field of another packet which sounds confusing.
I guess it is "Another field of the packet" in fact.
I think it would be nice to clarify as well.

>  struct rte_flow_action_modify_data {
> @@ -3217,10 +3220,18 @@ struct rte_flow_action_modify_data {
>  			uint32_t offset;
>  		};
>  		/**
> -		 * Immediate value for RTE_FLOW_FIELD_VALUE or
> -		 * memory address for RTE_FLOW_FIELD_POINTER.
> +		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented in the
> +		 * same byte order and length as in relevant rte_flow_item_xxx.
> +		 * The immediate source bitfield offset is inherited from
> +		 * the destination's one.
>  		 */
> -		uint64_t value;
> +		uint8_t value[16];
> +		/*

It should be a Doxygen style comment.

> +		 * Memory address for RTE_FLOW_FIELD_POINTER, memory layout
> +		 * should be the same as for relevant field in the
> +		 * rte_flow_item_xxx structure.
> +		 */
> +		void *pvalue;
>  	};
>  };
>  
> 

[dpdk-dev] [PATCH v3 0/5] ethdev: update modify field flow action

[Viacheslav Ovsiienko]

The generic modify field flow action introduced in [1] has
some issues related to the immediate source operand:

  - immediate source can be presented either as an unsigned
    64-bit integer or pointer to data pattern in memory.
    There was no explicit pointer field defined in the union

  - the byte ordering for 64-bit integer was not specified.
    Many fields have lesser lengths and byte ordering
    is crucial.

  - how the bit offset is applied to the immediate source
    field was not defined and documented

  - 64-bit integer size is not enough to provide MAC and
    IPv6 addresses

In order to cover the issues and exclude any ambiguities
the following is done:

  - introduce the explicit pointer field
    in rte_flow_action_modify_data structure

  - replace the 64-bit unsigned integer with 16-byte array

  - update the modify field flow action documentation

Appropriate commit message has been removed.

[1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
[2] RFC: http://patches.dpdk.org/project/dpdk/patch/20210910141609.8410-1-viacheslavo@nvidia.com/
[3] Deprecation notice: http://patches.dpdk.org/project/dpdk/patch/20210803085754.643180-1-orika@nvidia.com/
[4] v1 - http://patches.dpdk.org/project/dpdk/cover/20211001195223.31909-1-viacheslavo@nvidia.com/
[5] v2 - http://patches.dpdk.org/project/dpdk/patch/20211010234547.1495-2-viacheslavo@nvidia.com/

v2: - comments addressed
    - documentation updated
    - typos fixed
    - mlx5 PMD updated

v3: - comments addressed
    - documentation updated
    - typos fixed

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>

Viacheslav Ovsiienko (5):
  ethdev: update modify field flow action
  ethdev: fix missed experimental tag for modify field action
  app/testpmd: update modify field flow action support
  app/testpmd: fix hex string parser in flow commands
  net/mlx5: update modify field action

 app/test-pmd/cmdline_flow.c            | 60 ++++++++++++++++++--------
 doc/guides/prog_guide/rte_flow.rst     | 24 ++++++++++-
 doc/guides/rel_notes/deprecation.rst   |  4 --
 doc/guides/rel_notes/release_21_11.rst |  7 +++
 drivers/net/mlx5/mlx5_flow_dv.c        | 50 ++++++++++++---------
 lib/ethdev/rte_flow.h                  | 19 ++++++--
 6 files changed, 116 insertions(+), 48 deletions(-)

-- 
2.18.1

[dpdk-dev] [PATCH v3 1/5] ethdev: update modify field flow action

[Viacheslav Ovsiienko]

The generic modify field flow action introduced in [1] has
some issues related to the immediate source operand:

  - immediate source can be presented either as an unsigned
    64-bit integer or pointer to data pattern in memory.
    There was no explicit pointer field defined in the union.

  - the byte ordering for 64-bit integer was not specified.
    Many fields have shorter lengths and byte ordering
    is crucial.

  - how the bit offset is applied to the immediate source
    field was not defined and documented.

  - 64-bit integer size is not enough to provide IPv6
    addresses.

In order to cover the issues and exclude any ambiguities
the following is done:

  - introduce the explicit pointer field
    in rte_flow_action_modify_data structure

  - replace the 64-bit unsigned integer with 16-byte array

  - update the modify field flow action documentation

Appropriate deprecation notice has been removed.

[1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")

Fixes: 2ba49b5f3721 ("doc: announce change to ethdev modify action data")

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 doc/guides/prog_guide/rte_flow.rst     | 24 +++++++++++++++++++++++-
 doc/guides/rel_notes/deprecation.rst   |  4 ----
 doc/guides/rel_notes/release_21_11.rst |  7 +++++++
 lib/ethdev/rte_flow.h                  | 16 ++++++++++++----
 4 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 2b42d5ec8c..b08087511f 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -2835,6 +2835,22 @@ a packet to any other part of it.
 ``value`` sets an immediate value to be used as a source or points to a
 location of the value in memory. It is used instead of ``level`` and ``offset``
 for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER`` respectively.
+The data in memory should be presented exactly in the same byte order and
+length as in the relevant flow item, i.e. data for field with type
+``RTE_FLOW_FIELD_MAC_DST`` should follow the conventions of ``dst`` field
+in ``rte_flow_item_eth`` structure, with type ``RTE_FLOW_FIELD_IPV6_SRC`` -
+``rte_flow_item_ipv6`` conventions, and so on. If the field size is larger than
+16 bytes the pattern can be provided as pointer only.
+
+The bitfield extracted from the memory being applied as second operation
+parameter is defined by action width and by the destination field offset.
+Application should provide the data in immediate value memory (either as
+buffer or by pointer) exactly as item field without any applied explicit offset,
+and destination packet field (with specified width and bit offset) will be
+replaced by immediate source bits from the same bit offset. For example,
+to replace the third byte of MAC address with value 0x85, application should
+specify destination width as 8, destination offset as 16, and provide immediate
+value as sequence of bytes {xxx, xxx, 0x85, xxx, xxx, xxx}.
 
 .. _table_rte_flow_action_modify_field:
 
@@ -2865,7 +2881,13 @@ for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER`` respectively.
    +---------------+----------------------------------------------------------+
    | ``offset``    | number of bits to skip at the beginning                  |
    +---------------+----------------------------------------------------------+
-   | ``value``     | immediate value or a pointer to this value               |
+   | ``value``     | immediate value buffer (source field only, not           |
+   |               | applicable to destination) for RTE_FLOW_FIELD_VALUE      |
+   |               | field type                                               |
+   +---------------+----------------------------------------------------------+
+   | ``pvalue``    | pointer to immediate value data (source field only, not  |
+   |               | applicable to destination) for RTE_FLOW_FIELD_POINTER    |
+   |               | field type                                               |
    +---------------+----------------------------------------------------------+
 
 Action: ``CONNTRACK``
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index a2fe766d4b..dee14077a5 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -120,10 +120,6 @@ Deprecation Notices
 * ethdev: Announce moving from dedicated modify function for each field,
   to using the general ``rte_flow_modify_field`` action.
 
-* ethdev: The struct ``rte_flow_action_modify_data`` will be modified
-  to support modifying fields larger than 64 bits.
-  In addition, documentation will be updated to clarify byte order.
-
 * ethdev: Attribute ``shared`` of the ``struct rte_flow_action_count``
   is deprecated and will be removed in DPDK 21.11. Shared counters should
   be managed using shared actions API (``rte_flow_shared_action_create`` etc).
diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index dfc2cbdeed..578c1206e7 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -187,6 +187,13 @@ API Changes
   the crypto/security operation. This field will be used to communicate
   events such as soft expiry with IPsec in lookaside mode.
 
+* ethdev: ``rte_flow_action_modify_data`` structure updated, immediate data
+  array is extended, data pointer field is explicitly added to union, the
+  action behavior is defined in more strict fashion and documentation updated.
+  The immediate value behavior has been changed, the entire immediate field
+  should be provided, and offset for immediate source bitfield is assigned
+  from destination one.
+
 
 ABI Changes
 -----------
diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
index 7b1ed7f110..f14f77772b 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -3217,10 +3217,18 @@ struct rte_flow_action_modify_data {
 			uint32_t offset;
 		};
 		/**
-		 * Immediate value for RTE_FLOW_FIELD_VALUE or
-		 * memory address for RTE_FLOW_FIELD_POINTER.
+		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented in the
+		 * same byte order and length as in relevant rte_flow_item_xxx.
+		 * The immediate source bitfield offset is inherited from
+		 * the destination's one.
 		 */
-		uint64_t value;
+		uint8_t value[16];
+		/**
+		 * Memory address for RTE_FLOW_FIELD_POINTER, memory layout
+		 * should be the same as for relevant field in the
+		 * rte_flow_item_xxx structure.
+		 */
+		void *pvalue;
 	};
 };
 
@@ -3240,7 +3248,7 @@ enum rte_flow_modify_op {
  * RTE_FLOW_ACTION_TYPE_MODIFY_FIELD
  *
  * Modify a destination header field according to the specified
- * operation. Another packet field can be used as a source as well
+ * operation. Another field of the packet can be used as a source as well
  * as tag, mark, metadata, immediate value or a pointer to it.
  */
 struct rte_flow_action_modify_field {
-- 
2.18.1

[dpdk-dev] [PATCH v3 2/5] ethdev: fix missed experimental tag for modify field action

[Viacheslav Ovsiienko]

EXPERIMENTAL tag was missed in rte_flow_action_modify_data
structure description.

Fixes: 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
Cc: stable@dpdk.org

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 lib/ethdev/rte_flow.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
index f14f77772b..8a1eddd0b7 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -3204,6 +3204,9 @@ enum rte_flow_field_id {
 };
 
 /**
+ * @warning
+ * @b EXPERIMENTAL: this structure may change without prior notice
+ *
  * Field description for MODIFY_FIELD action.
  */
 struct rte_flow_action_modify_data {
-- 
2.18.1

[dpdk-dev] [PATCH v3 3/5] app/testpmd: update modify field flow action support

[Viacheslav Ovsiienko]

The testpmd flow create command updates provided:

  - modify field action supports the updated actions
  - pointer type added for action source field
  - pointer and value source field takes hex string
    instead of unsigned int in host endianness

There are some examples of flow with update modified
field action:

1. IPv6 destination address bytes 4-7 assignment:
   0000::1111 - > 0000:xxxx:4455:6677::1111

   flow create 0 egress group 1
     pattern eth / ipv6 dst is 0000::1111 / udp / end
     actions modify_field op set
             dst_type ipv6_dst
	     dst_offset 32
             src_type value
             src_value 0011223344556677
	     width 32 / end

2. Copy second byte of IPv4 destination address to the
   third byte of source address:
     10.0.118.4 -> 192.168.100.1
     10.0.168.4 -> 192.168.100.1

   flow create 0 egress group 1
     pattern eth / ipv4 / udp / end
     actions modify_field op set
             dst_type ipv4_src
             dst_offset 16
	     src_type ipv4_dst
	     src_offset 8
	     width 8 / end

3. Assign METADATA value with 11223344 value from the
   hex string in the linear buffer. Please note, the value
   definition should follow host-endian, example is given
   for x86 (little-endian):

   flow create 0 egress group 1
     pattern eth / ipv4 / end
     actions modify_field op set
             dst_type meta
	     src_type pointer
	     src_ptr 44332211
	     width 32 / end

4. Assign destination MAC with EA:11:0B:AD:0B:ED value:

   flow create 0 egress group 1
     pattern eth / end
     actions modify_field op set
             dst_type mac_dst
	     src_type value
	     src_value EA110BAD0BED
	     width 48 / end

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 55 +++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index bb22294dd3..736029c4fd 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -448,6 +448,7 @@ enum index {
 	ACTION_MODIFY_FIELD_SRC_LEVEL,
 	ACTION_MODIFY_FIELD_SRC_OFFSET,
 	ACTION_MODIFY_FIELD_SRC_VALUE,
+	ACTION_MODIFY_FIELD_SRC_POINTER,
 	ACTION_MODIFY_FIELD_WIDTH,
 	ACTION_CONNTRACK,
 	ACTION_CONNTRACK_UPDATE,
@@ -468,6 +469,14 @@ enum index {
 #define ITEM_RAW_SIZE \
 	(sizeof(struct rte_flow_item_raw) + ITEM_RAW_PATTERN_SIZE)
 
+/** Maximum size for external pattern in struct rte_flow_action_modify_data. */
+#define ACTION_MODIFY_PATTERN_SIZE 32
+
+/** Storage size for struct rte_flow_action_modify_field including pattern. */
+#define ACTION_MODIFY_SIZE \
+	(sizeof(struct rte_flow_action_modify_field) + \
+	ACTION_MODIFY_PATTERN_SIZE)
+
 /** Maximum number of queue indices in struct rte_flow_action_rss. */
 #define ACTION_RSS_QUEUE_NUM 128
 
@@ -1704,6 +1713,7 @@ static const enum index action_modify_field_src[] = {
 	ACTION_MODIFY_FIELD_SRC_LEVEL,
 	ACTION_MODIFY_FIELD_SRC_OFFSET,
 	ACTION_MODIFY_FIELD_SRC_VALUE,
+	ACTION_MODIFY_FIELD_SRC_POINTER,
 	ACTION_MODIFY_FIELD_WIDTH,
 	ZERO,
 };
@@ -4455,8 +4465,7 @@ static const struct token token_list[] = {
 	[ACTION_MODIFY_FIELD] = {
 		.name = "modify_field",
 		.help = "modify destination field with data from source field",
-		.priv = PRIV_ACTION(MODIFY_FIELD,
-			sizeof(struct rte_flow_action_modify_field)),
+		.priv = PRIV_ACTION(MODIFY_FIELD, ACTION_MODIFY_SIZE),
 		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_OP)),
 		.call = parse_vc,
 	},
@@ -4539,11 +4548,26 @@ static const struct token token_list[] = {
 		.name = "src_value",
 		.help = "source immediate value",
 		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_WIDTH),
-			NEXT_ENTRY(COMMON_UNSIGNED)),
-		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_modify_field,
+			     NEXT_ENTRY(COMMON_HEX)),
+		.args = ARGS(ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY(struct rte_flow_action_modify_field,
 					src.value)),
 		.call = parse_vc_conf,
 	},
+	[ACTION_MODIFY_FIELD_SRC_POINTER] = {
+		.name = "src_ptr",
+		.help = "pointer to source immediate value",
+		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_WIDTH),
+			     NEXT_ENTRY(COMMON_HEX)),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_modify_field,
+					src.pvalue),
+			     ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY_ARB
+				(sizeof(struct rte_flow_action_modify_field),
+				 ACTION_MODIFY_PATTERN_SIZE)),
+		.call = parse_vc_conf,
+	},
 	[ACTION_MODIFY_FIELD_WIDTH] = {
 		.name = "width",
 		.help = "number of bits to copy",
@@ -7830,15 +7854,11 @@ static int
 comp_set_modify_field_op(struct context *ctx, const struct token *token,
 		   unsigned int ent, char *buf, unsigned int size)
 {
-	uint16_t idx = 0;
-
 	RTE_SET_USED(ctx);
 	RTE_SET_USED(token);
-	for (idx = 0; modify_field_ops[idx]; ++idx)
-		;
 	if (!buf)
-		return idx + 1;
-	if (ent < idx)
+		return RTE_DIM(modify_field_ops);
+	if (ent < RTE_DIM(modify_field_ops) - 1)
 		return strlcpy(buf, modify_field_ops[ent], size);
 	return -1;
 }
@@ -7848,16 +7868,17 @@ static int
 comp_set_modify_field_id(struct context *ctx, const struct token *token,
 		   unsigned int ent, char *buf, unsigned int size)
 {
-	uint16_t idx = 0;
+	const char *name;
 
-	RTE_SET_USED(ctx);
 	RTE_SET_USED(token);
-	for (idx = 0; modify_field_ids[idx]; ++idx)
-		;
 	if (!buf)
-		return idx + 1;
-	if (ent < idx)
-		return strlcpy(buf, modify_field_ids[ent], size);
+		return RTE_DIM(modify_field_ids);
+	if (ent >= RTE_DIM(modify_field_ids) - 1)
+		return -1;
+	name = modify_field_ids[ent];
+	if (ctx->curr == ACTION_MODIFY_FIELD_SRC_TYPE ||
+	    (strcmp(name, "pointer") && strcmp(name, "value")))
+		return strlcpy(buf, name, size);
 	return -1;
 }
 
-- 
2.18.1

[dpdk-dev] [PATCH v3 4/5] app/testpmd: fix hex string parser in flow commands

[Viacheslav Ovsiienko]

The hexadecimal string parser does not check the target
field buffer size, buffer overflow happens and might
cause the application failure (segmentation fault
is observed usually).

Fixes: 169a9fed1f4c ("app/testpmd: fix hex string parser support for flow API")
Cc: stable@dpdk.org

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 736029c4fd..6827d9228f 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -7291,10 +7291,13 @@ parse_hex(struct context *ctx, const struct token *token,
 		hexlen -= 2;
 	}
 	if (hexlen > length)
-		return -1;
+		goto error;
 	ret = parse_hex_string(str, hex_tmp, &hexlen);
 	if (ret < 0)
 		goto error;
+	/* Check the converted binary fits into data buffer. */
+	if (hexlen > size)
+		goto error;
 	/* Let parse_int() fill length information first. */
 	ret = snprintf(tmp, sizeof(tmp), "%u", hexlen);
 	if (ret < 0)
-- 
2.18.1

[dpdk-dev] [PATCH v3 5/5] net/mlx5: update modify field action

[Viacheslav Ovsiienko]

Update immediate value/pointer source operand support
for modify field RTE Flow action:

  - source operand data can be presented by byte buffer
    (instead of former uint64_t) or by pointer
  - no host byte ordering is assumed anymore for immediate
    data buffer (not uint64_t anymore)
  - no immediate value offset is expected

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow_dv.c | 50 +++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index c6370cd1d6..867f587960 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -1391,7 +1391,7 @@ flow_dv_convert_action_modify_ipv6_dscp
 
 static int
 mlx5_flow_item_field_width(struct mlx5_priv *priv,
-			   enum rte_flow_field_id field)
+			   enum rte_flow_field_id field, int inherit)
 {
 	switch (field) {
 	case RTE_FLOW_FIELD_START:
@@ -1442,7 +1442,8 @@ mlx5_flow_item_field_width(struct mlx5_priv *priv,
 		return __builtin_popcount(priv->sh->dv_meta_mask);
 	case RTE_FLOW_FIELD_POINTER:
 	case RTE_FLOW_FIELD_VALUE:
-		return 64;
+		MLX5_ASSERT(inherit >= 0);
+		return inherit < 0 ? 0 : inherit;
 	default:
 		MLX5_ASSERT(false);
 	}
@@ -1462,7 +1463,8 @@ mlx5_flow_field_id_to_modify_info
 	struct mlx5_priv *priv = dev->data->dev_private;
 	uint32_t idx = 0;
 	uint32_t off = 0;
-	uint64_t val = 0;
+	uint8_t *pval;
+
 	switch (data->field) {
 	case RTE_FLOW_FIELD_START:
 		/* not supported yet */
@@ -1838,32 +1840,37 @@ mlx5_flow_field_id_to_modify_info
 		break;
 	case RTE_FLOW_FIELD_POINTER:
 	case RTE_FLOW_FIELD_VALUE:
-		if (data->field == RTE_FLOW_FIELD_POINTER)
-			memcpy(&val, (void *)(uintptr_t)data->value,
-			       sizeof(uint64_t));
-		else
-			val = data->value;
+		pval = data->field == RTE_FLOW_FIELD_POINTER ?
+		       (uint8_t *)(uintptr_t)data->pvalue :
+		       (uint8_t *)(uintptr_t)&data->value;
 		for (idx = 0; idx < MLX5_ACT_MAX_MOD_FIELDS; idx++) {
+			if (!dst_width)
+				break;
 			if (mask[idx]) {
 				if (dst_width == 48) {
 					/*special case for MAC addresses */
-					value[idx] = rte_cpu_to_be_16(val);
-					val >>= 16;
+					value[idx] = rte_cpu_to_be_16
+						(*(unaligned_uint16_t *)pval);
+					pval += sizeof(uint16_t);
 					dst_width -= 16;
 				} else if (dst_width > 16) {
-					value[idx] = rte_cpu_to_be_32(val);
-					val >>= 32;
+					value[idx] = rte_cpu_to_be_32
+						(*(unaligned_uint32_t *)pval);
+					pval += sizeof(uint32_t);
+					dst_width -= RTE_MIN(32u, dst_width);
 				} else if (dst_width > 8) {
-					value[idx] = rte_cpu_to_be_16(val);
-					val >>= 16;
+					value[idx] = rte_cpu_to_be_16
+						(*(unaligned_uint16_t *)pval);
+					pval += sizeof(uint16_t);
+					dst_width -= RTE_MIN(16u, dst_width);
 				} else {
-					value[idx] = (uint8_t)val;
-					val >>= 8;
+					value[idx] = *pval++;
+					dst_width -= RTE_MIN(8u, dst_width);
 				}
 				if (*shift)
 					value[idx] <<= *shift;
-				if (!val)
-					break;
+			} else {
+				pval += sizeof(uint32_t);
 			}
 		}
 		break;
@@ -1910,8 +1917,9 @@ flow_dv_convert_action_modify_field
 	uint32_t value[MLX5_ACT_MAX_MOD_FIELDS] = {0, 0, 0, 0, 0};
 	uint32_t type;
 	uint32_t shift = 0;
-	uint32_t dst_width = mlx5_flow_item_field_width(priv, conf->dst.field);
+	uint32_t dst_width;
 
+	dst_width = mlx5_flow_item_field_width(priv, conf->dst.field, -1);
 	if (conf->src.field == RTE_FLOW_FIELD_POINTER ||
 		conf->src.field == RTE_FLOW_FIELD_VALUE) {
 		type = MLX5_MODIFICATION_TYPE_SET;
@@ -4874,9 +4882,9 @@ flow_dv_validate_action_modify_field(struct rte_eth_dev *dev,
 	const struct rte_flow_action_modify_field *action_modify_field =
 		action->conf;
 	uint32_t dst_width = mlx5_flow_item_field_width(priv,
-				action_modify_field->dst.field);
+				action_modify_field->dst.field, -1);
 	uint32_t src_width = mlx5_flow_item_field_width(priv,
-				action_modify_field->src.field);
+				action_modify_field->src.field, dst_width);
 
 	ret = flow_dv_validate_action_modify_hdr(action_flags, action, error);
 	if (ret)
-- 
2.18.1

Re: [dpdk-dev] [PATCH v3 2/5] ethdev: fix missed experimental tag for modify field action

[Ori Kam]

Hi Slava,

> -----Original Message-----
> From: Slava Ovsiienko <viacheslavo@nvidia.com>
> Sent: Tuesday, October 12, 2021 11:06 AM
> To: dev@dpdk.org
> Subject: [PATCH v3 2/5] ethdev: fix missed experimental tag for modify field action
> 
> EXPERIMENTAL tag was missed in rte_flow_action_modify_data structure description.
> 
> Fixes: 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
>  lib/ethdev/rte_flow.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index f14f77772b..8a1eddd0b7 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -3204,6 +3204,9 @@ enum rte_flow_field_id {  };
> 
>  /**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
>   * Field description for MODIFY_FIELD action.
>   */
>  struct rte_flow_action_modify_data {
> --
> 2.18.1

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

Re: [dpdk-dev] [PATCH v3 2/5] ethdev: fix missed experimental tag for modify field action

[Andrew Rybchenko]

On 10/12/21 11:06 AM, Viacheslav Ovsiienko wrote:
> EXPERIMENTAL tag was missed in rte_flow_action_modify_data
> structure description.
> 
> Fixes: 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [dpdk-dev] [PATCH v3 1/5] ethdev: update modify field flow action

[Andrew Rybchenko]

On 10/12/21 11:06 AM, Viacheslav Ovsiienko wrote:
> The generic modify field flow action introduced in [1] has
> some issues related to the immediate source operand:
> 
>   - immediate source can be presented either as an unsigned
>     64-bit integer or pointer to data pattern in memory.
>     There was no explicit pointer field defined in the union.
> 
>   - the byte ordering for 64-bit integer was not specified.
>     Many fields have shorter lengths and byte ordering
>     is crucial.
> 
>   - how the bit offset is applied to the immediate source
>     field was not defined and documented.
> 
>   - 64-bit integer size is not enough to provide IPv6
>     addresses.
> 
> In order to cover the issues and exclude any ambiguities
> the following is done:
> 
>   - introduce the explicit pointer field
>     in rte_flow_action_modify_data structure
> 
>   - replace the 64-bit unsigned integer with 16-byte array
> 
>   - update the modify field flow action documentation
> 
> Appropriate deprecation notice has been removed.
> 
> [1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
> 
> Fixes: 2ba49b5f3721 ("doc: announce change to ethdev modify action data")
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> Acked-by: Ori Kam <orika@nvidia.com>

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

[dpdk-dev] [PATCH v5 0/5] ethdev: update modify field flow action

[Viacheslav Ovsiienko]

The generic modify field flow action introduced in [1] has
some issues related to the immediate source operand:

  - immediate source can be presented either as an unsigned
    64-bit integer or pointer to data pattern in memory.
    There was no explicit pointer field defined in the union

  - the byte ordering for 64-bit integer was not specified.
    Many fields have lesser lengths and byte ordering
    is crucial.

  - how the bit offset is applied to the immediate source
    field was not defined and documented

  - 64-bit integer size is not enough to provide MAC and
    IPv6 addresses

In order to cover the issues and exclude any ambiguities
the following is done:

  - introduce the explicit pointer field
    in rte_flow_action_modify_data structure

  - replace the 64-bit unsigned integer with 16-byte array

  - update the modify field flow action documentation

Appropriate commit message has been removed.

[1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
[2] RFC: http://patches.dpdk.org/project/dpdk/patch/20210910141609.8410-1-viacheslavo@nvidia.com/
[3] Deprecation notice: http://patches.dpdk.org/project/dpdk/patch/20210803085754.643180-1-orika@nvidia.com/
[4] v1 - http://patches.dpdk.org/project/dpdk/cover/20211001195223.31909-1-viacheslavo@nvidia.com/
[5] v2 - http://patches.dpdk.org/project/dpdk/patch/20211010234547.1495-2-viacheslavo@nvidia.com/
[6] v3 - http://patches.dpdk.org/project/dpdk/cover/20211012080631.28504-1-viacheslavo@nvidia.com/
[7] v4 - http://patches.dpdk.org/project/dpdk/cover/20211012104919.13145-1-viacheslavo@nvidia.com/

v2: - comments addressed
    - documentation updated
    - typos fixed
    - mlx5 PMD updated

v3: - comments addressed
    - documentation updated
    - typos fixed

v4: - removed errorneously added Ack by Ori K. for mlx5 patch
    - mlx5 patch updated - bug fixes and cleanup

v5: - fix compilation issue with unused variable in mlx5

Viacheslav Ovsiienko (5):
  ethdev: update modify field flow action
  ethdev: fix missed experimental tag for modify field action
  app/testpmd: update modify field flow action support
  app/testpmd: fix hex string parser in flow commands
  net/mlx5: update modify field action

 app/test-pmd/cmdline_flow.c            |  60 ++++++++----
 doc/guides/prog_guide/rte_flow.rst     |  24 ++++-
 doc/guides/rel_notes/deprecation.rst   |   4 -
 doc/guides/rel_notes/release_21_11.rst |   7 ++
 drivers/net/mlx5/mlx5_flow_dv.c        | 123 ++++++++-----------------
 lib/ethdev/rte_flow.h                  |  19 +++-
 6 files changed, 127 insertions(+), 110 deletions(-)

-- 
2.18.1

[dpdk-dev] [PATCH v5 1/5] ethdev: update modify field flow action

[Viacheslav Ovsiienko]

The generic modify field flow action introduced in [1] has
some issues related to the immediate source operand:

  - immediate source can be presented either as an unsigned
    64-bit integer or pointer to data pattern in memory.
    There was no explicit pointer field defined in the union.

  - the byte ordering for 64-bit integer was not specified.
    Many fields have shorter lengths and byte ordering
    is crucial.

  - how the bit offset is applied to the immediate source
    field was not defined and documented.

  - 64-bit integer size is not enough to provide IPv6
    addresses.

In order to cover the issues and exclude any ambiguities
the following is done:

  - introduce the explicit pointer field
    in rte_flow_action_modify_data structure

  - replace the 64-bit unsigned integer with 16-byte array

  - update the modify field flow action documentation

Appropriate deprecation notice has been removed.

[1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")

Fixes: 2ba49b5f3721 ("doc: announce change to ethdev modify action data")

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 doc/guides/prog_guide/rte_flow.rst     | 24 +++++++++++++++++++++++-
 doc/guides/rel_notes/deprecation.rst   |  4 ----
 doc/guides/rel_notes/release_21_11.rst |  7 +++++++
 lib/ethdev/rte_flow.h                  | 16 ++++++++++++----
 4 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 2b42d5ec8c..b08087511f 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -2835,6 +2835,22 @@ a packet to any other part of it.
 ``value`` sets an immediate value to be used as a source or points to a
 location of the value in memory. It is used instead of ``level`` and ``offset``
 for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER`` respectively.
+The data in memory should be presented exactly in the same byte order and
+length as in the relevant flow item, i.e. data for field with type
+``RTE_FLOW_FIELD_MAC_DST`` should follow the conventions of ``dst`` field
+in ``rte_flow_item_eth`` structure, with type ``RTE_FLOW_FIELD_IPV6_SRC`` -
+``rte_flow_item_ipv6`` conventions, and so on. If the field size is larger than
+16 bytes the pattern can be provided as pointer only.
+
+The bitfield extracted from the memory being applied as second operation
+parameter is defined by action width and by the destination field offset.
+Application should provide the data in immediate value memory (either as
+buffer or by pointer) exactly as item field without any applied explicit offset,
+and destination packet field (with specified width and bit offset) will be
+replaced by immediate source bits from the same bit offset. For example,
+to replace the third byte of MAC address with value 0x85, application should
+specify destination width as 8, destination offset as 16, and provide immediate
+value as sequence of bytes {xxx, xxx, 0x85, xxx, xxx, xxx}.
 
 .. _table_rte_flow_action_modify_field:
 
@@ -2865,7 +2881,13 @@ for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER`` respectively.
    +---------------+----------------------------------------------------------+
    | ``offset``    | number of bits to skip at the beginning                  |
    +---------------+----------------------------------------------------------+
-   | ``value``     | immediate value or a pointer to this value               |
+   | ``value``     | immediate value buffer (source field only, not           |
+   |               | applicable to destination) for RTE_FLOW_FIELD_VALUE      |
+   |               | field type                                               |
+   +---------------+----------------------------------------------------------+
+   | ``pvalue``    | pointer to immediate value data (source field only, not  |
+   |               | applicable to destination) for RTE_FLOW_FIELD_POINTER    |
+   |               | field type                                               |
    +---------------+----------------------------------------------------------+
 
 Action: ``CONNTRACK``
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index a2fe766d4b..dee14077a5 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -120,10 +120,6 @@ Deprecation Notices
 * ethdev: Announce moving from dedicated modify function for each field,
   to using the general ``rte_flow_modify_field`` action.
 
-* ethdev: The struct ``rte_flow_action_modify_data`` will be modified
-  to support modifying fields larger than 64 bits.
-  In addition, documentation will be updated to clarify byte order.
-
 * ethdev: Attribute ``shared`` of the ``struct rte_flow_action_count``
   is deprecated and will be removed in DPDK 21.11. Shared counters should
   be managed using shared actions API (``rte_flow_shared_action_create`` etc).
diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index dfc2cbdeed..578c1206e7 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -187,6 +187,13 @@ API Changes
   the crypto/security operation. This field will be used to communicate
   events such as soft expiry with IPsec in lookaside mode.
 
+* ethdev: ``rte_flow_action_modify_data`` structure updated, immediate data
+  array is extended, data pointer field is explicitly added to union, the
+  action behavior is defined in more strict fashion and documentation updated.
+  The immediate value behavior has been changed, the entire immediate field
+  should be provided, and offset for immediate source bitfield is assigned
+  from destination one.
+
 
 ABI Changes
 -----------
diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
index 7b1ed7f110..f14f77772b 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -3217,10 +3217,18 @@ struct rte_flow_action_modify_data {
 			uint32_t offset;
 		};
 		/**
-		 * Immediate value for RTE_FLOW_FIELD_VALUE or
-		 * memory address for RTE_FLOW_FIELD_POINTER.
+		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented in the
+		 * same byte order and length as in relevant rte_flow_item_xxx.
+		 * The immediate source bitfield offset is inherited from
+		 * the destination's one.
 		 */
-		uint64_t value;
+		uint8_t value[16];
+		/**
+		 * Memory address for RTE_FLOW_FIELD_POINTER, memory layout
+		 * should be the same as for relevant field in the
+		 * rte_flow_item_xxx structure.
+		 */
+		void *pvalue;
 	};
 };
 
@@ -3240,7 +3248,7 @@ enum rte_flow_modify_op {
  * RTE_FLOW_ACTION_TYPE_MODIFY_FIELD
  *
  * Modify a destination header field according to the specified
- * operation. Another packet field can be used as a source as well
+ * operation. Another field of the packet can be used as a source as well
  * as tag, mark, metadata, immediate value or a pointer to it.
  */
 struct rte_flow_action_modify_field {
-- 
2.18.1

[dpdk-dev] [PATCH v5 2/5] ethdev: fix missed experimental tag for modify field action

[Viacheslav Ovsiienko]

EXPERIMENTAL tag was missed in rte_flow_action_modify_data
structure description.

Fixes: 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
Cc: stable@dpdk.org

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 lib/ethdev/rte_flow.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
index f14f77772b..8a1eddd0b7 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -3204,6 +3204,9 @@ enum rte_flow_field_id {
 };
 
 /**
+ * @warning
+ * @b EXPERIMENTAL: this structure may change without prior notice
+ *
  * Field description for MODIFY_FIELD action.
  */
 struct rte_flow_action_modify_data {
-- 
2.18.1

[dpdk-dev] [PATCH v5 3/5] app/testpmd: update modify field flow action support

[Viacheslav Ovsiienko]

The testpmd flow create command updates provided:

  - modify field action supports the updated actions
  - pointer type added for action source field
  - pointer and value source field takes hex string
    instead of unsigned int in host endianness

There are some examples of flow with update modified
field action:

1. IPv6 destination address bytes 4-7 assignment:
   0000::1111 - > 0000:xxxx:4455:6677::1111

   flow create 0 egress group 1
     pattern eth / ipv6 dst is 0000::1111 / udp / end
     actions modify_field op set
             dst_type ipv6_dst
	     dst_offset 32
             src_type value
             src_value 0011223344556677
	     width 32 / end

2. Copy second byte of IPv4 destination address to the
   third byte of source address:
     10.0.118.4 -> 192.168.100.1
     10.0.168.4 -> 192.168.100.1

   flow create 0 egress group 1
     pattern eth / ipv4 / udp / end
     actions modify_field op set
             dst_type ipv4_src
             dst_offset 16
	     src_type ipv4_dst
	     src_offset 8
	     width 8 / end

3. Assign METADATA value with 11223344 value from the
   hex string in the linear buffer. Please note, the value
   definition should follow host-endian, example is given
   for x86 (little-endian):

   flow create 0 egress group 1
     pattern eth / ipv4 / end
     actions modify_field op set
             dst_type meta
	     src_type pointer
	     src_ptr 44332211
	     width 32 / end

4. Assign destination MAC with EA:11:0B:AD:0B:ED value:

   flow create 0 egress group 1
     pattern eth / end
     actions modify_field op set
             dst_type mac_dst
	     src_type value
	     src_value EA110BAD0BED
	     width 48 / end

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 55 +++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index bb22294dd3..736029c4fd 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -448,6 +448,7 @@ enum index {
 	ACTION_MODIFY_FIELD_SRC_LEVEL,
 	ACTION_MODIFY_FIELD_SRC_OFFSET,
 	ACTION_MODIFY_FIELD_SRC_VALUE,
+	ACTION_MODIFY_FIELD_SRC_POINTER,
 	ACTION_MODIFY_FIELD_WIDTH,
 	ACTION_CONNTRACK,
 	ACTION_CONNTRACK_UPDATE,
@@ -468,6 +469,14 @@ enum index {
 #define ITEM_RAW_SIZE \
 	(sizeof(struct rte_flow_item_raw) + ITEM_RAW_PATTERN_SIZE)
 
+/** Maximum size for external pattern in struct rte_flow_action_modify_data. */
+#define ACTION_MODIFY_PATTERN_SIZE 32
+
+/** Storage size for struct rte_flow_action_modify_field including pattern. */
+#define ACTION_MODIFY_SIZE \
+	(sizeof(struct rte_flow_action_modify_field) + \
+	ACTION_MODIFY_PATTERN_SIZE)
+
 /** Maximum number of queue indices in struct rte_flow_action_rss. */
 #define ACTION_RSS_QUEUE_NUM 128
 
@@ -1704,6 +1713,7 @@ static const enum index action_modify_field_src[] = {
 	ACTION_MODIFY_FIELD_SRC_LEVEL,
 	ACTION_MODIFY_FIELD_SRC_OFFSET,
 	ACTION_MODIFY_FIELD_SRC_VALUE,
+	ACTION_MODIFY_FIELD_SRC_POINTER,
 	ACTION_MODIFY_FIELD_WIDTH,
 	ZERO,
 };
@@ -4455,8 +4465,7 @@ static const struct token token_list[] = {
 	[ACTION_MODIFY_FIELD] = {
 		.name = "modify_field",
 		.help = "modify destination field with data from source field",
-		.priv = PRIV_ACTION(MODIFY_FIELD,
-			sizeof(struct rte_flow_action_modify_field)),
+		.priv = PRIV_ACTION(MODIFY_FIELD, ACTION_MODIFY_SIZE),
 		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_OP)),
 		.call = parse_vc,
 	},
@@ -4539,11 +4548,26 @@ static const struct token token_list[] = {
 		.name = "src_value",
 		.help = "source immediate value",
 		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_WIDTH),
-			NEXT_ENTRY(COMMON_UNSIGNED)),
-		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_modify_field,
+			     NEXT_ENTRY(COMMON_HEX)),
+		.args = ARGS(ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY(struct rte_flow_action_modify_field,
 					src.value)),
 		.call = parse_vc_conf,
 	},
+	[ACTION_MODIFY_FIELD_SRC_POINTER] = {
+		.name = "src_ptr",
+		.help = "pointer to source immediate value",
+		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_WIDTH),
+			     NEXT_ENTRY(COMMON_HEX)),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_modify_field,
+					src.pvalue),
+			     ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY_ARB
+				(sizeof(struct rte_flow_action_modify_field),
+				 ACTION_MODIFY_PATTERN_SIZE)),
+		.call = parse_vc_conf,
+	},
 	[ACTION_MODIFY_FIELD_WIDTH] = {
 		.name = "width",
 		.help = "number of bits to copy",
@@ -7830,15 +7854,11 @@ static int
 comp_set_modify_field_op(struct context *ctx, const struct token *token,
 		   unsigned int ent, char *buf, unsigned int size)
 {
-	uint16_t idx = 0;
-
 	RTE_SET_USED(ctx);
 	RTE_SET_USED(token);
-	for (idx = 0; modify_field_ops[idx]; ++idx)
-		;
 	if (!buf)
-		return idx + 1;
-	if (ent < idx)
+		return RTE_DIM(modify_field_ops);
+	if (ent < RTE_DIM(modify_field_ops) - 1)
 		return strlcpy(buf, modify_field_ops[ent], size);
 	return -1;
 }
@@ -7848,16 +7868,17 @@ static int
 comp_set_modify_field_id(struct context *ctx, const struct token *token,
 		   unsigned int ent, char *buf, unsigned int size)
 {
-	uint16_t idx = 0;
+	const char *name;
 
-	RTE_SET_USED(ctx);
 	RTE_SET_USED(token);
-	for (idx = 0; modify_field_ids[idx]; ++idx)
-		;
 	if (!buf)
-		return idx + 1;
-	if (ent < idx)
-		return strlcpy(buf, modify_field_ids[ent], size);
+		return RTE_DIM(modify_field_ids);
+	if (ent >= RTE_DIM(modify_field_ids) - 1)
+		return -1;
+	name = modify_field_ids[ent];
+	if (ctx->curr == ACTION_MODIFY_FIELD_SRC_TYPE ||
+	    (strcmp(name, "pointer") && strcmp(name, "value")))
+		return strlcpy(buf, name, size);
 	return -1;
 }
 
-- 
2.18.1

[dpdk-dev] [PATCH v5 4/5] app/testpmd: fix hex string parser in flow commands

[Viacheslav Ovsiienko]

The hexadecimal string parser does not check the target
field buffer size, buffer overflow happens and might
cause the application failure (segmentation fault
is observed usually).

Fixes: 169a9fed1f4c ("app/testpmd: fix hex string parser support for flow API")
Cc: stable@dpdk.org

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 736029c4fd..6827d9228f 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -7291,10 +7291,13 @@ parse_hex(struct context *ctx, const struct token *token,
 		hexlen -= 2;
 	}
 	if (hexlen > length)
-		return -1;
+		goto error;
 	ret = parse_hex_string(str, hex_tmp, &hexlen);
 	if (ret < 0)
 		goto error;
+	/* Check the converted binary fits into data buffer. */
+	if (hexlen > size)
+		goto error;
 	/* Let parse_int() fill length information first. */
 	ret = snprintf(tmp, sizeof(tmp), "%u", hexlen);
 	if (ret < 0)
-- 
2.18.1

[dpdk-dev] [PATCH v5 5/5] net/mlx5: update modify field action

[Viacheslav Ovsiienko]

Update immediate value/pointer source operand support
for modify field RTE Flow action:

  - source operand data can be presented by byte buffer
    (instead of former uint64_t) or by pointer
  - no host byte ordering is assumed anymore for immediate
    data buffer (not uint64_t anymore)
  - no immediate value offset is expected (the source
    subfield is located at the same offset as in destination)

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow_dv.c | 123 +++++++++++---------------------
 1 file changed, 40 insertions(+), 83 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index c6370cd1d6..673dfd53db 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -1391,7 +1391,7 @@ flow_dv_convert_action_modify_ipv6_dscp
 
 static int
 mlx5_flow_item_field_width(struct mlx5_priv *priv,
-			   enum rte_flow_field_id field)
+			   enum rte_flow_field_id field, int inherit)
 {
 	switch (field) {
 	case RTE_FLOW_FIELD_START:
@@ -1442,7 +1442,7 @@ mlx5_flow_item_field_width(struct mlx5_priv *priv,
 		return __builtin_popcount(priv->sh->dv_meta_mask);
 	case RTE_FLOW_FIELD_POINTER:
 	case RTE_FLOW_FIELD_VALUE:
-		return 64;
+		return inherit < 0 ? 0 : inherit;
 	default:
 		MLX5_ASSERT(false);
 	}
@@ -1452,17 +1452,14 @@ mlx5_flow_item_field_width(struct mlx5_priv *priv,
 static void
 mlx5_flow_field_id_to_modify_info
 		(const struct rte_flow_action_modify_data *data,
-		 struct field_modify_info *info,
-		 uint32_t *mask, uint32_t *value,
-		 uint32_t width, uint32_t dst_width,
-		 uint32_t *shift, struct rte_eth_dev *dev,
-		 const struct rte_flow_attr *attr,
-		 struct rte_flow_error *error)
+		 struct field_modify_info *info, uint32_t *mask,
+		 uint32_t width, uint32_t *shift, struct rte_eth_dev *dev,
+		 const struct rte_flow_attr *attr, struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
 	uint32_t idx = 0;
 	uint32_t off = 0;
-	uint64_t val = 0;
+
 	switch (data->field) {
 	case RTE_FLOW_FIELD_START:
 		/* not supported yet */
@@ -1472,7 +1469,7 @@ mlx5_flow_field_id_to_modify_info
 		off = data->offset > 16 ? data->offset - 16 : 0;
 		if (mask) {
 			if (data->offset < 16) {
-				info[idx] = (struct field_modify_info){2, 0,
+				info[idx] = (struct field_modify_info){2, 4,
 						MLX5_MODI_OUT_DMAC_15_0};
 				if (width < 16) {
 					mask[idx] = rte_cpu_to_be_16(0xffff >>
@@ -1486,15 +1483,15 @@ mlx5_flow_field_id_to_modify_info
 					break;
 				++idx;
 			}
-			info[idx] = (struct field_modify_info){4, 4 * idx,
+			info[idx] = (struct field_modify_info){4, 0,
 						MLX5_MODI_OUT_DMAC_47_16};
 			mask[idx] = rte_cpu_to_be_32((0xffffffff >>
 						      (32 - width)) << off);
 		} else {
 			if (data->offset < 16)
-				info[idx++] = (struct field_modify_info){2, 0,
+				info[idx++] = (struct field_modify_info){2, 4,
 						MLX5_MODI_OUT_DMAC_15_0};
-			info[idx] = (struct field_modify_info){4, off,
+			info[idx] = (struct field_modify_info){4, 0,
 						MLX5_MODI_OUT_DMAC_47_16};
 		}
 		break;
@@ -1502,7 +1499,7 @@ mlx5_flow_field_id_to_modify_info
 		off = data->offset > 16 ? data->offset - 16 : 0;
 		if (mask) {
 			if (data->offset < 16) {
-				info[idx] = (struct field_modify_info){2, 0,
+				info[idx] = (struct field_modify_info){2, 4,
 						MLX5_MODI_OUT_SMAC_15_0};
 				if (width < 16) {
 					mask[idx] = rte_cpu_to_be_16(0xffff >>
@@ -1516,15 +1513,15 @@ mlx5_flow_field_id_to_modify_info
 					break;
 				++idx;
 			}
-			info[idx] = (struct field_modify_info){4, 4 * idx,
+			info[idx] = (struct field_modify_info){4, 0,
 						MLX5_MODI_OUT_SMAC_47_16};
 			mask[idx] = rte_cpu_to_be_32((0xffffffff >>
 						      (32 - width)) << off);
 		} else {
 			if (data->offset < 16)
-				info[idx++] = (struct field_modify_info){2, 0,
+				info[idx++] = (struct field_modify_info){2, 4,
 						MLX5_MODI_OUT_SMAC_15_0};
-			info[idx] = (struct field_modify_info){4, off,
+			info[idx] = (struct field_modify_info){4, 0,
 						MLX5_MODI_OUT_SMAC_47_16};
 		}
 		break;
@@ -1584,8 +1581,7 @@ mlx5_flow_field_id_to_modify_info
 	case RTE_FLOW_FIELD_IPV6_SRC:
 		if (mask) {
 			if (data->offset < 32) {
-				info[idx] = (struct field_modify_info){4,
-						4 * idx,
+				info[idx] = (struct field_modify_info){4, 12,
 						MLX5_MODI_OUT_SIPV6_31_0};
 				if (width < 32) {
 					mask[idx] =
@@ -1601,8 +1597,7 @@ mlx5_flow_field_id_to_modify_info
 				++idx;
 			}
 			if (data->offset < 64) {
-				info[idx] = (struct field_modify_info){4,
-						4 * idx,
+				info[idx] = (struct field_modify_info){4, 8,
 						MLX5_MODI_OUT_SIPV6_63_32};
 				if (width < 32) {
 					mask[idx] =
@@ -1618,8 +1613,7 @@ mlx5_flow_field_id_to_modify_info
 				++idx;
 			}
 			if (data->offset < 96) {
-				info[idx] = (struct field_modify_info){4,
-						4 * idx,
+				info[idx] = (struct field_modify_info){4, 4,
 						MLX5_MODI_OUT_SIPV6_95_64};
 				if (width < 32) {
 					mask[idx] =
@@ -1634,19 +1628,19 @@ mlx5_flow_field_id_to_modify_info
 					break;
 				++idx;
 			}
-			info[idx] = (struct field_modify_info){4, 4 * idx,
+			info[idx] = (struct field_modify_info){4, 0,
 						MLX5_MODI_OUT_SIPV6_127_96};
 			mask[idx] = rte_cpu_to_be_32(0xffffffff >>
 						     (32 - width));
 		} else {
 			if (data->offset < 32)
-				info[idx++] = (struct field_modify_info){4, 0,
+				info[idx++] = (struct field_modify_info){4, 12,
 						MLX5_MODI_OUT_SIPV6_31_0};
 			if (data->offset < 64)
-				info[idx++] = (struct field_modify_info){4, 0,
+				info[idx++] = (struct field_modify_info){4, 8,
 						MLX5_MODI_OUT_SIPV6_63_32};
 			if (data->offset < 96)
-				info[idx++] = (struct field_modify_info){4, 0,
+				info[idx++] = (struct field_modify_info){4, 4,
 						MLX5_MODI_OUT_SIPV6_95_64};
 			if (data->offset < 128)
 				info[idx++] = (struct field_modify_info){4, 0,
@@ -1656,8 +1650,7 @@ mlx5_flow_field_id_to_modify_info
 	case RTE_FLOW_FIELD_IPV6_DST:
 		if (mask) {
 			if (data->offset < 32) {
-				info[idx] = (struct field_modify_info){4,
-						4 * idx,
+				info[idx] = (struct field_modify_info){4, 12,
 						MLX5_MODI_OUT_DIPV6_31_0};
 				if (width < 32) {
 					mask[idx] =
@@ -1673,8 +1666,7 @@ mlx5_flow_field_id_to_modify_info
 				++idx;
 			}
 			if (data->offset < 64) {
-				info[idx] = (struct field_modify_info){4,
-						4 * idx,
+				info[idx] = (struct field_modify_info){4, 8,
 						MLX5_MODI_OUT_DIPV6_63_32};
 				if (width < 32) {
 					mask[idx] =
@@ -1690,8 +1682,7 @@ mlx5_flow_field_id_to_modify_info
 				++idx;
 			}
 			if (data->offset < 96) {
-				info[idx] = (struct field_modify_info){4,
-						4 * idx,
+				info[idx] = (struct field_modify_info){4, 4,
 						MLX5_MODI_OUT_DIPV6_95_64};
 				if (width < 32) {
 					mask[idx] =
@@ -1706,19 +1697,19 @@ mlx5_flow_field_id_to_modify_info
 					break;
 				++idx;
 			}
-			info[idx] = (struct field_modify_info){4, 4 * idx,
+			info[idx] = (struct field_modify_info){4, 0,
 						MLX5_MODI_OUT_DIPV6_127_96};
 			mask[idx] = rte_cpu_to_be_32(0xffffffff >>
 						     (32 - width));
 		} else {
 			if (data->offset < 32)
-				info[idx++] = (struct field_modify_info){4, 0,
+				info[idx++] = (struct field_modify_info){4, 12,
 						MLX5_MODI_OUT_DIPV6_31_0};
 			if (data->offset < 64)
-				info[idx++] = (struct field_modify_info){4, 0,
+				info[idx++] = (struct field_modify_info){4, 8,
 						MLX5_MODI_OUT_DIPV6_63_32};
 			if (data->offset < 96)
-				info[idx++] = (struct field_modify_info){4, 0,
+				info[idx++] = (struct field_modify_info){4, 4,
 						MLX5_MODI_OUT_DIPV6_95_64};
 			if (data->offset < 128)
 				info[idx++] = (struct field_modify_info){4, 0,
@@ -1838,35 +1829,6 @@ mlx5_flow_field_id_to_modify_info
 		break;
 	case RTE_FLOW_FIELD_POINTER:
 	case RTE_FLOW_FIELD_VALUE:
-		if (data->field == RTE_FLOW_FIELD_POINTER)
-			memcpy(&val, (void *)(uintptr_t)data->value,
-			       sizeof(uint64_t));
-		else
-			val = data->value;
-		for (idx = 0; idx < MLX5_ACT_MAX_MOD_FIELDS; idx++) {
-			if (mask[idx]) {
-				if (dst_width == 48) {
-					/*special case for MAC addresses */
-					value[idx] = rte_cpu_to_be_16(val);
-					val >>= 16;
-					dst_width -= 16;
-				} else if (dst_width > 16) {
-					value[idx] = rte_cpu_to_be_32(val);
-					val >>= 32;
-				} else if (dst_width > 8) {
-					value[idx] = rte_cpu_to_be_16(val);
-					val >>= 16;
-				} else {
-					value[idx] = (uint8_t)val;
-					val >>= 8;
-				}
-				if (*shift)
-					value[idx] <<= *shift;
-				if (!val)
-					break;
-			}
-		}
-		break;
 	default:
 		MLX5_ASSERT(false);
 		break;
@@ -1898,7 +1860,6 @@ flow_dv_convert_action_modify_field
 			 const struct rte_flow_attr *attr,
 			 struct rte_flow_error *error)
 {
-	struct mlx5_priv *priv = dev->data->dev_private;
 	const struct rte_flow_action_modify_field *conf =
 		(const struct rte_flow_action_modify_field *)(action->conf);
 	struct rte_flow_item item;
@@ -1907,33 +1868,29 @@ flow_dv_convert_action_modify_field
 	struct field_modify_info dcopy[MLX5_ACT_MAX_MOD_FIELDS] = {
 								{0, 0, 0} };
 	uint32_t mask[MLX5_ACT_MAX_MOD_FIELDS] = {0, 0, 0, 0, 0};
-	uint32_t value[MLX5_ACT_MAX_MOD_FIELDS] = {0, 0, 0, 0, 0};
 	uint32_t type;
 	uint32_t shift = 0;
-	uint32_t dst_width = mlx5_flow_item_field_width(priv, conf->dst.field);
 
 	if (conf->src.field == RTE_FLOW_FIELD_POINTER ||
-		conf->src.field == RTE_FLOW_FIELD_VALUE) {
+	    conf->src.field == RTE_FLOW_FIELD_VALUE) {
 		type = MLX5_MODIFICATION_TYPE_SET;
 		/** For SET fill the destination field (field) first. */
 		mlx5_flow_field_id_to_modify_info(&conf->dst, field, mask,
-						  value, conf->width, dst_width,
-						  &shift, dev, attr, error);
-		/** Then copy immediate value from source as per mask. */
-		mlx5_flow_field_id_to_modify_info(&conf->src, dcopy, mask,
-						  value, conf->width, dst_width,
-						  &shift, dev, attr, error);
-		item.spec = &value;
+						  conf->width, &shift, dev,
+						  attr, error);
+		item.spec = conf->src.field == RTE_FLOW_FIELD_POINTER ?
+					(void *)(uintptr_t)conf->src.pvalue :
+					(void *)(uintptr_t)&conf->src.value;
 	} else {
 		type = MLX5_MODIFICATION_TYPE_COPY;
 		/** For COPY fill the destination field (dcopy) without mask. */
 		mlx5_flow_field_id_to_modify_info(&conf->dst, dcopy, NULL,
-						  value, conf->width, dst_width,
-						  &shift, dev, attr, error);
+						  conf->width, &shift, dev,
+						  attr, error);
 		/** Then construct the source field (field) with mask. */
 		mlx5_flow_field_id_to_modify_info(&conf->src, field, mask,
-						  value, conf->width, dst_width,
-						  &shift, dev, attr, error);
+						  conf->width, &shift,
+						  dev, attr, error);
 	}
 	item.mask = &mask;
 	return flow_dv_convert_modify_action(&item,
@@ -4874,9 +4831,9 @@ flow_dv_validate_action_modify_field(struct rte_eth_dev *dev,
 	const struct rte_flow_action_modify_field *action_modify_field =
 		action->conf;
 	uint32_t dst_width = mlx5_flow_item_field_width(priv,
-				action_modify_field->dst.field);
+				action_modify_field->dst.field, -1);
 	uint32_t src_width = mlx5_flow_item_field_width(priv,
-				action_modify_field->src.field);
+				action_modify_field->src.field, dst_width);
 
 	ret = flow_dv_validate_action_modify_hdr(action_flags, action, error);
 	if (ret)
-- 
2.18.1

Re: [dpdk-dev] [PATCH v5 0/5] ethdev: update modify field flow action

[Ferruh Yigit]

On 10/12/2021 9:25 PM, Viacheslav Ovsiienko wrote:
> The generic modify field flow action introduced in [1] has
> some issues related to the immediate source operand:
> 
>    - immediate source can be presented either as an unsigned
>      64-bit integer or pointer to data pattern in memory.
>      There was no explicit pointer field defined in the union
> 
>    - the byte ordering for 64-bit integer was not specified.
>      Many fields have lesser lengths and byte ordering
>      is crucial.
> 
>    - how the bit offset is applied to the immediate source
>      field was not defined and documented
> 
>    - 64-bit integer size is not enough to provide MAC and
>      IPv6 addresses
> 
> In order to cover the issues and exclude any ambiguities
> the following is done:
> 
>    - introduce the explicit pointer field
>      in rte_flow_action_modify_data structure
> 
>    - replace the 64-bit unsigned integer with 16-byte array
> 
>    - update the modify field flow action documentation
> 
> Appropriate commit message has been removed.
> 
> [1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
> [2] RFC: http://patches.dpdk.org/project/dpdk/patch/20210910141609.8410-1-viacheslavo@nvidia.com/
> [3] Deprecation notice: http://patches.dpdk.org/project/dpdk/patch/20210803085754.643180-1-orika@nvidia.com/
> [4] v1 - http://patches.dpdk.org/project/dpdk/cover/20211001195223.31909-1-viacheslavo@nvidia.com/
> [5] v2 - http://patches.dpdk.org/project/dpdk/patch/20211010234547.1495-2-viacheslavo@nvidia.com/
> [6] v3 - http://patches.dpdk.org/project/dpdk/cover/20211012080631.28504-1-viacheslavo@nvidia.com/
> [7] v4 - http://patches.dpdk.org/project/dpdk/cover/20211012104919.13145-1-viacheslavo@nvidia.com/
> 
> v2: - comments addressed
>      - documentation updated
>      - typos fixed
>      - mlx5 PMD updated
> 
> v3: - comments addressed
>      - documentation updated
>      - typos fixed
> 
> v4: - removed errorneously added Ack by Ori K. for mlx5 patch
>      - mlx5 patch updated - bug fixes and cleanup
> 
> v5: - fix compilation issue with unused variable in mlx5
> 
> Viacheslav Ovsiienko (5):
>    ethdev: update modify field flow action
>    ethdev: fix missed experimental tag for modify field action
>    app/testpmd: update modify field flow action support
>    app/testpmd: fix hex string parser in flow commands
>    net/mlx5: update modify field action
> 

Hi Viacheslav,

The set reports build error on the CI [1], can you please check if it is a valid error?

[1]
http://mails.dpdk.org/archives/test-report/2021-October/227206.html

FAILED: drivers/libtmp_rte_net_mlx5.a.p/net_mlx5_mlx5_flow_dv.c.o
../drivers/net/mlx5/mlx5_flow_dv.c: In function 'flow_dv_convert_action_modify_field.isra.123':
../drivers/net/mlx5/mlx5_flow_dv.c:526:30: error: 'item.spec' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     data = flow_dv_fetch_field((const uint8_t *)item->spec +
                               ^
../drivers/net/mlx5/mlx5_flow_dv.c:1865:23: note: 'item.spec' was declared here
   struct rte_flow_item item;
                        ^

[dpdk-dev] [PATCH v6 0/5] ethdev: update modify field flow action

[Viacheslav Ovsiienko]

The generic modify field flow action introduced in [1] has
some issues related to the immediate source operand:

  - immediate source can be presented either as an unsigned
    64-bit integer or pointer to data pattern in memory.
    There was no explicit pointer field defined in the union

  - the byte ordering for 64-bit integer was not specified.
    Many fields have lesser lengths and byte ordering
    is crucial.

  - how the bit offset is applied to the immediate source
    field was not defined and documented

  - 64-bit integer size is not enough to provide MAC and
    IPv6 addresses

In order to cover the issues and exclude any ambiguities
the following is done:

  - introduce the explicit pointer field
    in rte_flow_action_modify_data structure

  - replace the 64-bit unsigned integer with 16-byte array

  - update the modify field flow action documentation

Appropriate commit message has been removed.

[1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
[2] RFC: http://patches.dpdk.org/project/dpdk/patch/20210910141609.8410-1-viacheslavo@nvidia.com/
[3] Deprecation notice: http://patches.dpdk.org/project/dpdk/patch/20210803085754.643180-1-orika@nvidia.com/
[4] v1 - http://patches.dpdk.org/project/dpdk/cover/20211001195223.31909-1-viacheslavo@nvidia.com/
[5] v2 - http://patches.dpdk.org/project/dpdk/patch/20211010234547.1495-2-viacheslavo@nvidia.com/
[6] v3 - http://patches.dpdk.org/project/dpdk/cover/20211012080631.28504-1-viacheslavo@nvidia.com/
[7] v4 - http://patches.dpdk.org/project/dpdk/cover/20211012104919.13145-1-viacheslavo@nvidia.com/
[8] v5 - http://patches.dpdk.org/project/dpdk/patch/20211012202557.30295-2-viacheslavo@nvidia.com/

v2: - comments addressed
    - documentation updated
    - typos fixed
    - mlx5 PMD updated

v3: - comments addressed
    - documentation updated
    - typos fixed

v4: - removed errorneously added Ack by Ori K. for mlx5 patch
    - mlx5 patch updated - bug fixes and cleanup

v5: - fix compilation issue with unused variable in mlx5

v6: - fix compilation issue with unused variable in mlx5

Viacheslav Ovsiienko (5):
  ethdev: update modify field flow action
  ethdev: fix missed experimental tag for modify field action
  app/testpmd: update modify field flow action support
  app/testpmd: fix hex string parser in flow commands
  net/mlx5: update modify field action

 app/test-pmd/cmdline_flow.c            |  60 ++++++++----
 doc/guides/prog_guide/rte_flow.rst     |  24 ++++-
 doc/guides/rel_notes/deprecation.rst   |   4 -
 doc/guides/rel_notes/release_21_11.rst |   7 ++
 drivers/net/mlx5/mlx5_flow_dv.c        | 128 +++++++++----------------
 lib/ethdev/rte_flow.h                  |  19 +++-
 6 files changed, 131 insertions(+), 111 deletions(-)

-- 
2.18.1

[dpdk-dev] [PATCH v6 1/5] ethdev: update modify field flow action

[Viacheslav Ovsiienko]

The generic modify field flow action introduced in [1] has
some issues related to the immediate source operand:

  - immediate source can be presented either as an unsigned
    64-bit integer or pointer to data pattern in memory.
    There was no explicit pointer field defined in the union.

  - the byte ordering for 64-bit integer was not specified.
    Many fields have shorter lengths and byte ordering
    is crucial.

  - how the bit offset is applied to the immediate source
    field was not defined and documented.

  - 64-bit integer size is not enough to provide IPv6
    addresses.

In order to cover the issues and exclude any ambiguities
the following is done:

  - introduce the explicit pointer field
    in rte_flow_action_modify_data structure

  - replace the 64-bit unsigned integer with 16-byte array

  - update the modify field flow action documentation

Appropriate deprecation notice has been removed.

[1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")

Fixes: 2ba49b5f3721 ("doc: announce change to ethdev modify action data")

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 doc/guides/prog_guide/rte_flow.rst     | 24 +++++++++++++++++++++++-
 doc/guides/rel_notes/deprecation.rst   |  4 ----
 doc/guides/rel_notes/release_21_11.rst |  7 +++++++
 lib/ethdev/rte_flow.h                  | 16 ++++++++++++----
 4 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 2b42d5ec8c..b08087511f 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -2835,6 +2835,22 @@ a packet to any other part of it.
 ``value`` sets an immediate value to be used as a source or points to a
 location of the value in memory. It is used instead of ``level`` and ``offset``
 for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER`` respectively.
+The data in memory should be presented exactly in the same byte order and
+length as in the relevant flow item, i.e. data for field with type
+``RTE_FLOW_FIELD_MAC_DST`` should follow the conventions of ``dst`` field
+in ``rte_flow_item_eth`` structure, with type ``RTE_FLOW_FIELD_IPV6_SRC`` -
+``rte_flow_item_ipv6`` conventions, and so on. If the field size is larger than
+16 bytes the pattern can be provided as pointer only.
+
+The bitfield extracted from the memory being applied as second operation
+parameter is defined by action width and by the destination field offset.
+Application should provide the data in immediate value memory (either as
+buffer or by pointer) exactly as item field without any applied explicit offset,
+and destination packet field (with specified width and bit offset) will be
+replaced by immediate source bits from the same bit offset. For example,
+to replace the third byte of MAC address with value 0x85, application should
+specify destination width as 8, destination offset as 16, and provide immediate
+value as sequence of bytes {xxx, xxx, 0x85, xxx, xxx, xxx}.
 
 .. _table_rte_flow_action_modify_field:
 
@@ -2865,7 +2881,13 @@ for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER`` respectively.
    +---------------+----------------------------------------------------------+
    | ``offset``    | number of bits to skip at the beginning                  |
    +---------------+----------------------------------------------------------+
-   | ``value``     | immediate value or a pointer to this value               |
+   | ``value``     | immediate value buffer (source field only, not           |
+   |               | applicable to destination) for RTE_FLOW_FIELD_VALUE      |
+   |               | field type                                               |
+   +---------------+----------------------------------------------------------+
+   | ``pvalue``    | pointer to immediate value data (source field only, not  |
+   |               | applicable to destination) for RTE_FLOW_FIELD_POINTER    |
+   |               | field type                                               |
    +---------------+----------------------------------------------------------+
 
 Action: ``CONNTRACK``
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index a2fe766d4b..dee14077a5 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -120,10 +120,6 @@ Deprecation Notices
 * ethdev: Announce moving from dedicated modify function for each field,
   to using the general ``rte_flow_modify_field`` action.
 
-* ethdev: The struct ``rte_flow_action_modify_data`` will be modified
-  to support modifying fields larger than 64 bits.
-  In addition, documentation will be updated to clarify byte order.
-
 * ethdev: Attribute ``shared`` of the ``struct rte_flow_action_count``
   is deprecated and will be removed in DPDK 21.11. Shared counters should
   be managed using shared actions API (``rte_flow_shared_action_create`` etc).
diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index dfc2cbdeed..578c1206e7 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -187,6 +187,13 @@ API Changes
   the crypto/security operation. This field will be used to communicate
   events such as soft expiry with IPsec in lookaside mode.
 
+* ethdev: ``rte_flow_action_modify_data`` structure updated, immediate data
+  array is extended, data pointer field is explicitly added to union, the
+  action behavior is defined in more strict fashion and documentation updated.
+  The immediate value behavior has been changed, the entire immediate field
+  should be provided, and offset for immediate source bitfield is assigned
+  from destination one.
+
 
 ABI Changes
 -----------
diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
index 7b1ed7f110..f14f77772b 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -3217,10 +3217,18 @@ struct rte_flow_action_modify_data {
 			uint32_t offset;
 		};
 		/**
-		 * Immediate value for RTE_FLOW_FIELD_VALUE or
-		 * memory address for RTE_FLOW_FIELD_POINTER.
+		 * Immediate value for RTE_FLOW_FIELD_VALUE, presented in the
+		 * same byte order and length as in relevant rte_flow_item_xxx.
+		 * The immediate source bitfield offset is inherited from
+		 * the destination's one.
 		 */
-		uint64_t value;
+		uint8_t value[16];
+		/**
+		 * Memory address for RTE_FLOW_FIELD_POINTER, memory layout
+		 * should be the same as for relevant field in the
+		 * rte_flow_item_xxx structure.
+		 */
+		void *pvalue;
 	};
 };
 
@@ -3240,7 +3248,7 @@ enum rte_flow_modify_op {
  * RTE_FLOW_ACTION_TYPE_MODIFY_FIELD
  *
  * Modify a destination header field according to the specified
- * operation. Another packet field can be used as a source as well
+ * operation. Another field of the packet can be used as a source as well
  * as tag, mark, metadata, immediate value or a pointer to it.
  */
 struct rte_flow_action_modify_field {
-- 
2.18.1

[dpdk-dev] [PATCH v6 2/5] ethdev: fix missed experimental tag for modify field action

[Viacheslav Ovsiienko]

EXPERIMENTAL tag was missed in rte_flow_action_modify_data
structure description.

Fixes: 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
Cc: stable@dpdk.org

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 lib/ethdev/rte_flow.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
index f14f77772b..8a1eddd0b7 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -3204,6 +3204,9 @@ enum rte_flow_field_id {
 };
 
 /**
+ * @warning
+ * @b EXPERIMENTAL: this structure may change without prior notice
+ *
  * Field description for MODIFY_FIELD action.
  */
 struct rte_flow_action_modify_data {
-- 
2.18.1

[dpdk-dev] [PATCH v6 3/5] app/testpmd: update modify field flow action support

[Viacheslav Ovsiienko]

The testpmd flow create command updates provided:

  - modify field action supports the updated actions
  - pointer type added for action source field
  - pointer and value source field takes hex string
    instead of unsigned int in host endianness

There are some examples of flow with update modified
field action:

1. IPv6 destination address bytes 4-7 assignment:
   0000::1111 - > 0000:xxxx:4455:6677::1111

   flow create 0 egress group 1
     pattern eth / ipv6 dst is 0000::1111 / udp / end
     actions modify_field op set
             dst_type ipv6_dst
	     dst_offset 32
             src_type value
             src_value 0011223344556677
	     width 32 / end

2. Copy second byte of IPv4 destination address to the
   third byte of source address:
     10.0.118.4 -> 192.168.100.1
     10.0.168.4 -> 192.168.100.1

   flow create 0 egress group 1
     pattern eth / ipv4 / udp / end
     actions modify_field op set
             dst_type ipv4_src
             dst_offset 16
	     src_type ipv4_dst
	     src_offset 8
	     width 8 / end

3. Assign METADATA value with 11223344 value from the
   hex string in the linear buffer. Please note, the value
   definition should follow host-endian, example is given
   for x86 (little-endian):

   flow create 0 egress group 1
     pattern eth / ipv4 / end
     actions modify_field op set
             dst_type meta
	     src_type pointer
	     src_ptr 44332211
	     width 32 / end

4. Assign destination MAC with EA:11:0B:AD:0B:ED value:

   flow create 0 egress group 1
     pattern eth / end
     actions modify_field op set
             dst_type mac_dst
	     src_type value
	     src_value EA110BAD0BED
	     width 48 / end

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 55 +++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index bb22294dd3..736029c4fd 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -448,6 +448,7 @@ enum index {
 	ACTION_MODIFY_FIELD_SRC_LEVEL,
 	ACTION_MODIFY_FIELD_SRC_OFFSET,
 	ACTION_MODIFY_FIELD_SRC_VALUE,
+	ACTION_MODIFY_FIELD_SRC_POINTER,
 	ACTION_MODIFY_FIELD_WIDTH,
 	ACTION_CONNTRACK,
 	ACTION_CONNTRACK_UPDATE,
@@ -468,6 +469,14 @@ enum index {
 #define ITEM_RAW_SIZE \
 	(sizeof(struct rte_flow_item_raw) + ITEM_RAW_PATTERN_SIZE)
 
+/** Maximum size for external pattern in struct rte_flow_action_modify_data. */
+#define ACTION_MODIFY_PATTERN_SIZE 32
+
+/** Storage size for struct rte_flow_action_modify_field including pattern. */
+#define ACTION_MODIFY_SIZE \
+	(sizeof(struct rte_flow_action_modify_field) + \
+	ACTION_MODIFY_PATTERN_SIZE)
+
 /** Maximum number of queue indices in struct rte_flow_action_rss. */
 #define ACTION_RSS_QUEUE_NUM 128
 
@@ -1704,6 +1713,7 @@ static const enum index action_modify_field_src[] = {
 	ACTION_MODIFY_FIELD_SRC_LEVEL,
 	ACTION_MODIFY_FIELD_SRC_OFFSET,
 	ACTION_MODIFY_FIELD_SRC_VALUE,
+	ACTION_MODIFY_FIELD_SRC_POINTER,
 	ACTION_MODIFY_FIELD_WIDTH,
 	ZERO,
 };
@@ -4455,8 +4465,7 @@ static const struct token token_list[] = {
 	[ACTION_MODIFY_FIELD] = {
 		.name = "modify_field",
 		.help = "modify destination field with data from source field",
-		.priv = PRIV_ACTION(MODIFY_FIELD,
-			sizeof(struct rte_flow_action_modify_field)),
+		.priv = PRIV_ACTION(MODIFY_FIELD, ACTION_MODIFY_SIZE),
 		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_OP)),
 		.call = parse_vc,
 	},
@@ -4539,11 +4548,26 @@ static const struct token token_list[] = {
 		.name = "src_value",
 		.help = "source immediate value",
 		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_WIDTH),
-			NEXT_ENTRY(COMMON_UNSIGNED)),
-		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_modify_field,
+			     NEXT_ENTRY(COMMON_HEX)),
+		.args = ARGS(ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY(struct rte_flow_action_modify_field,
 					src.value)),
 		.call = parse_vc_conf,
 	},
+	[ACTION_MODIFY_FIELD_SRC_POINTER] = {
+		.name = "src_ptr",
+		.help = "pointer to source immediate value",
+		.next = NEXT(NEXT_ENTRY(ACTION_MODIFY_FIELD_WIDTH),
+			     NEXT_ENTRY(COMMON_HEX)),
+		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_modify_field,
+					src.pvalue),
+			     ARGS_ENTRY_ARB(0, 0),
+			     ARGS_ENTRY_ARB
+				(sizeof(struct rte_flow_action_modify_field),
+				 ACTION_MODIFY_PATTERN_SIZE)),
+		.call = parse_vc_conf,
+	},
 	[ACTION_MODIFY_FIELD_WIDTH] = {
 		.name = "width",
 		.help = "number of bits to copy",
@@ -7830,15 +7854,11 @@ static int
 comp_set_modify_field_op(struct context *ctx, const struct token *token,
 		   unsigned int ent, char *buf, unsigned int size)
 {
-	uint16_t idx = 0;
-
 	RTE_SET_USED(ctx);
 	RTE_SET_USED(token);
-	for (idx = 0; modify_field_ops[idx]; ++idx)
-		;
 	if (!buf)
-		return idx + 1;
-	if (ent < idx)
+		return RTE_DIM(modify_field_ops);
+	if (ent < RTE_DIM(modify_field_ops) - 1)
 		return strlcpy(buf, modify_field_ops[ent], size);
 	return -1;
 }
@@ -7848,16 +7868,17 @@ static int
 comp_set_modify_field_id(struct context *ctx, const struct token *token,
 		   unsigned int ent, char *buf, unsigned int size)
 {
-	uint16_t idx = 0;
+	const char *name;
 
-	RTE_SET_USED(ctx);
 	RTE_SET_USED(token);
-	for (idx = 0; modify_field_ids[idx]; ++idx)
-		;
 	if (!buf)
-		return idx + 1;
-	if (ent < idx)
-		return strlcpy(buf, modify_field_ids[ent], size);
+		return RTE_DIM(modify_field_ids);
+	if (ent >= RTE_DIM(modify_field_ids) - 1)
+		return -1;
+	name = modify_field_ids[ent];
+	if (ctx->curr == ACTION_MODIFY_FIELD_SRC_TYPE ||
+	    (strcmp(name, "pointer") && strcmp(name, "value")))
+		return strlcpy(buf, name, size);
 	return -1;
 }
 
-- 
2.18.1

[dpdk-dev] [PATCH v6 4/5] app/testpmd: fix hex string parser in flow commands

[Viacheslav Ovsiienko]

The hexadecimal string parser does not check the target
field buffer size, buffer overflow happens and might
cause the application failure (segmentation fault
is observed usually).

Fixes: 169a9fed1f4c ("app/testpmd: fix hex string parser support for flow API")
Cc: stable@dpdk.org

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 736029c4fd..6827d9228f 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -7291,10 +7291,13 @@ parse_hex(struct context *ctx, const struct token *token,
 		hexlen -= 2;
 	}
 	if (hexlen > length)
-		return -1;
+		goto error;
 	ret = parse_hex_string(str, hex_tmp, &hexlen);
 	if (ret < 0)
 		goto error;
+	/* Check the converted binary fits into data buffer. */
+	if (hexlen > size)
+		goto error;
 	/* Let parse_int() fill length information first. */
 	ret = snprintf(tmp, sizeof(tmp), "%u", hexlen);
 	if (ret < 0)
-- 
2.18.1

[dpdk-dev] [PATCH v6 5/5] net/mlx5: update modify field action

[Viacheslav Ovsiienko]

Update immediate value/pointer source operand support
for modify field RTE Flow action:

  - source operand data can be presented by byte buffer
    (instead of former uint64_t) or by pointer
  - no host byte ordering is assumed anymore for immediate
    data buffer (not uint64_t anymore)
  - no immediate value offset is expected (the source
    subfield is located at the same offset as in destination)

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow_dv.c | 128 +++++++++++---------------------
 1 file changed, 44 insertions(+), 84 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index c6370cd1d6..552b149041 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -1391,7 +1391,7 @@ flow_dv_convert_action_modify_ipv6_dscp
 
 static int
 mlx5_flow_item_field_width(struct mlx5_priv *priv,
-			   enum rte_flow_field_id field)
+			   enum rte_flow_field_id field, int inherit)
 {
 	switch (field) {
 	case RTE_FLOW_FIELD_START:
@@ -1442,7 +1442,7 @@ mlx5_flow_item_field_width(struct mlx5_priv *priv,
 		return __builtin_popcount(priv->sh->dv_meta_mask);
 	case RTE_FLOW_FIELD_POINTER:
 	case RTE_FLOW_FIELD_VALUE:
-		return 64;
+		return inherit < 0 ? 0 : inherit;
 	default:
 		MLX5_ASSERT(false);
 	}
@@ -1452,17 +1452,14 @@ mlx5_flow_item_field_width(struct mlx5_priv *priv,
 static void
 mlx5_flow_field_id_to_modify_info
 		(const struct rte_flow_action_modify_data *data,
-		 struct field_modify_info *info,
-		 uint32_t *mask, uint32_t *value,
-		 uint32_t width, uint32_t dst_width,
-		 uint32_t *shift, struct rte_eth_dev *dev,
-		 const struct rte_flow_attr *attr,
-		 struct rte_flow_error *error)
+		 struct field_modify_info *info, uint32_t *mask,
+		 uint32_t width, uint32_t *shift, struct rte_eth_dev *dev,
+		 const struct rte_flow_attr *attr, struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
 	uint32_t idx = 0;
 	uint32_t off = 0;
-	uint64_t val = 0;
+
 	switch (data->field) {
 	case RTE_FLOW_FIELD_START:
 		/* not supported yet */
@@ -1472,7 +1469,7 @@ mlx5_flow_field_id_to_modify_info
 		off = data->offset > 16 ? data->offset - 16 : 0;
 		if (mask) {
 			if (data->offset < 16) {
-				info[idx] = (struct field_modify_info){2, 0,
+				info[idx] = (struct field_modify_info){2, 4,
 						MLX5_MODI_OUT_DMAC_15_0};
 				if (width < 16) {
 					mask[idx] = rte_cpu_to_be_16(0xffff >>
@@ -1486,15 +1483,15 @@ mlx5_flow_field_id_to_modify_info
 					break;
 				++idx;
 			}
-			info[idx] = (struct field_modify_info){4, 4 * idx,
+			info[idx] = (struct field_modify_info){4, 0,
 						MLX5_MODI_OUT_DMAC_47_16};
 			mask[idx] = rte_cpu_to_be_32((0xffffffff >>
 						      (32 - width)) << off);
 		} else {
 			if (data->offset < 16)
-				info[idx++] = (struct field_modify_info){2, 0,
+				info[idx++] = (struct field_modify_info){2, 4,
 						MLX5_MODI_OUT_DMAC_15_0};
-			info[idx] = (struct field_modify_info){4, off,
+			info[idx] = (struct field_modify_info){4, 0,
 						MLX5_MODI_OUT_DMAC_47_16};
 		}
 		break;
@@ -1502,7 +1499,7 @@ mlx5_flow_field_id_to_modify_info
 		off = data->offset > 16 ? data->offset - 16 : 0;
 		if (mask) {
 			if (data->offset < 16) {
-				info[idx] = (struct field_modify_info){2, 0,
+				info[idx] = (struct field_modify_info){2, 4,
 						MLX5_MODI_OUT_SMAC_15_0};
 				if (width < 16) {
 					mask[idx] = rte_cpu_to_be_16(0xffff >>
@@ -1516,15 +1513,15 @@ mlx5_flow_field_id_to_modify_info
 					break;
 				++idx;
 			}
-			info[idx] = (struct field_modify_info){4, 4 * idx,
+			info[idx] = (struct field_modify_info){4, 0,
 						MLX5_MODI_OUT_SMAC_47_16};
 			mask[idx] = rte_cpu_to_be_32((0xffffffff >>
 						      (32 - width)) << off);
 		} else {
 			if (data->offset < 16)
-				info[idx++] = (struct field_modify_info){2, 0,
+				info[idx++] = (struct field_modify_info){2, 4,
 						MLX5_MODI_OUT_SMAC_15_0};
-			info[idx] = (struct field_modify_info){4, off,
+			info[idx] = (struct field_modify_info){4, 0,
 						MLX5_MODI_OUT_SMAC_47_16};
 		}
 		break;
@@ -1584,8 +1581,7 @@ mlx5_flow_field_id_to_modify_info
 	case RTE_FLOW_FIELD_IPV6_SRC:
 		if (mask) {
 			if (data->offset < 32) {
-				info[idx] = (struct field_modify_info){4,
-						4 * idx,
+				info[idx] = (struct field_modify_info){4, 12,
 						MLX5_MODI_OUT_SIPV6_31_0};
 				if (width < 32) {
 					mask[idx] =
@@ -1601,8 +1597,7 @@ mlx5_flow_field_id_to_modify_info
 				++idx;
 			}
 			if (data->offset < 64) {
-				info[idx] = (struct field_modify_info){4,
-						4 * idx,
+				info[idx] = (struct field_modify_info){4, 8,
 						MLX5_MODI_OUT_SIPV6_63_32};
 				if (width < 32) {
 					mask[idx] =
@@ -1618,8 +1613,7 @@ mlx5_flow_field_id_to_modify_info
 				++idx;
 			}
 			if (data->offset < 96) {
-				info[idx] = (struct field_modify_info){4,
-						4 * idx,
+				info[idx] = (struct field_modify_info){4, 4,
 						MLX5_MODI_OUT_SIPV6_95_64};
 				if (width < 32) {
 					mask[idx] =
@@ -1634,19 +1628,19 @@ mlx5_flow_field_id_to_modify_info
 					break;
 				++idx;
 			}
-			info[idx] = (struct field_modify_info){4, 4 * idx,
+			info[idx] = (struct field_modify_info){4, 0,
 						MLX5_MODI_OUT_SIPV6_127_96};
 			mask[idx] = rte_cpu_to_be_32(0xffffffff >>
 						     (32 - width));
 		} else {
 			if (data->offset < 32)
-				info[idx++] = (struct field_modify_info){4, 0,
+				info[idx++] = (struct field_modify_info){4, 12,
 						MLX5_MODI_OUT_SIPV6_31_0};
 			if (data->offset < 64)
-				info[idx++] = (struct field_modify_info){4, 0,
+				info[idx++] = (struct field_modify_info){4, 8,
 						MLX5_MODI_OUT_SIPV6_63_32};
 			if (data->offset < 96)
-				info[idx++] = (struct field_modify_info){4, 0,
+				info[idx++] = (struct field_modify_info){4, 4,
 						MLX5_MODI_OUT_SIPV6_95_64};
 			if (data->offset < 128)
 				info[idx++] = (struct field_modify_info){4, 0,
@@ -1656,8 +1650,7 @@ mlx5_flow_field_id_to_modify_info
 	case RTE_FLOW_FIELD_IPV6_DST:
 		if (mask) {
 			if (data->offset < 32) {
-				info[idx] = (struct field_modify_info){4,
-						4 * idx,
+				info[idx] = (struct field_modify_info){4, 12,
 						MLX5_MODI_OUT_DIPV6_31_0};
 				if (width < 32) {
 					mask[idx] =
@@ -1673,8 +1666,7 @@ mlx5_flow_field_id_to_modify_info
 				++idx;
 			}
 			if (data->offset < 64) {
-				info[idx] = (struct field_modify_info){4,
-						4 * idx,
+				info[idx] = (struct field_modify_info){4, 8,
 						MLX5_MODI_OUT_DIPV6_63_32};
 				if (width < 32) {
 					mask[idx] =
@@ -1690,8 +1682,7 @@ mlx5_flow_field_id_to_modify_info
 				++idx;
 			}
 			if (data->offset < 96) {
-				info[idx] = (struct field_modify_info){4,
-						4 * idx,
+				info[idx] = (struct field_modify_info){4, 4,
 						MLX5_MODI_OUT_DIPV6_95_64};
 				if (width < 32) {
 					mask[idx] =
@@ -1706,19 +1697,19 @@ mlx5_flow_field_id_to_modify_info
 					break;
 				++idx;
 			}
-			info[idx] = (struct field_modify_info){4, 4 * idx,
+			info[idx] = (struct field_modify_info){4, 0,
 						MLX5_MODI_OUT_DIPV6_127_96};
 			mask[idx] = rte_cpu_to_be_32(0xffffffff >>
 						     (32 - width));
 		} else {
 			if (data->offset < 32)
-				info[idx++] = (struct field_modify_info){4, 0,
+				info[idx++] = (struct field_modify_info){4, 12,
 						MLX5_MODI_OUT_DIPV6_31_0};
 			if (data->offset < 64)
-				info[idx++] = (struct field_modify_info){4, 0,
+				info[idx++] = (struct field_modify_info){4, 8,
 						MLX5_MODI_OUT_DIPV6_63_32};
 			if (data->offset < 96)
-				info[idx++] = (struct field_modify_info){4, 0,
+				info[idx++] = (struct field_modify_info){4, 4,
 						MLX5_MODI_OUT_DIPV6_95_64};
 			if (data->offset < 128)
 				info[idx++] = (struct field_modify_info){4, 0,
@@ -1838,35 +1829,6 @@ mlx5_flow_field_id_to_modify_info
 		break;
 	case RTE_FLOW_FIELD_POINTER:
 	case RTE_FLOW_FIELD_VALUE:
-		if (data->field == RTE_FLOW_FIELD_POINTER)
-			memcpy(&val, (void *)(uintptr_t)data->value,
-			       sizeof(uint64_t));
-		else
-			val = data->value;
-		for (idx = 0; idx < MLX5_ACT_MAX_MOD_FIELDS; idx++) {
-			if (mask[idx]) {
-				if (dst_width == 48) {
-					/*special case for MAC addresses */
-					value[idx] = rte_cpu_to_be_16(val);
-					val >>= 16;
-					dst_width -= 16;
-				} else if (dst_width > 16) {
-					value[idx] = rte_cpu_to_be_32(val);
-					val >>= 32;
-				} else if (dst_width > 8) {
-					value[idx] = rte_cpu_to_be_16(val);
-					val >>= 16;
-				} else {
-					value[idx] = (uint8_t)val;
-					val >>= 8;
-				}
-				if (*shift)
-					value[idx] <<= *shift;
-				if (!val)
-					break;
-			}
-		}
-		break;
 	default:
 		MLX5_ASSERT(false);
 		break;
@@ -1898,42 +1860,40 @@ flow_dv_convert_action_modify_field
 			 const struct rte_flow_attr *attr,
 			 struct rte_flow_error *error)
 {
-	struct mlx5_priv *priv = dev->data->dev_private;
 	const struct rte_flow_action_modify_field *conf =
 		(const struct rte_flow_action_modify_field *)(action->conf);
-	struct rte_flow_item item;
+	struct rte_flow_item item = {
+		.spec = NULL,
+		.mask = NULL
+	};
 	struct field_modify_info field[MLX5_ACT_MAX_MOD_FIELDS] = {
 								{0, 0, 0} };
 	struct field_modify_info dcopy[MLX5_ACT_MAX_MOD_FIELDS] = {
 								{0, 0, 0} };
 	uint32_t mask[MLX5_ACT_MAX_MOD_FIELDS] = {0, 0, 0, 0, 0};
-	uint32_t value[MLX5_ACT_MAX_MOD_FIELDS] = {0, 0, 0, 0, 0};
 	uint32_t type;
 	uint32_t shift = 0;
-	uint32_t dst_width = mlx5_flow_item_field_width(priv, conf->dst.field);
 
 	if (conf->src.field == RTE_FLOW_FIELD_POINTER ||
-		conf->src.field == RTE_FLOW_FIELD_VALUE) {
+	    conf->src.field == RTE_FLOW_FIELD_VALUE) {
 		type = MLX5_MODIFICATION_TYPE_SET;
 		/** For SET fill the destination field (field) first. */
 		mlx5_flow_field_id_to_modify_info(&conf->dst, field, mask,
-						  value, conf->width, dst_width,
-						  &shift, dev, attr, error);
-		/** Then copy immediate value from source as per mask. */
-		mlx5_flow_field_id_to_modify_info(&conf->src, dcopy, mask,
-						  value, conf->width, dst_width,
-						  &shift, dev, attr, error);
-		item.spec = &value;
+						  conf->width, &shift, dev,
+						  attr, error);
+		item.spec = conf->src.field == RTE_FLOW_FIELD_POINTER ?
+					(void *)(uintptr_t)conf->src.pvalue :
+					(void *)(uintptr_t)&conf->src.value;
 	} else {
 		type = MLX5_MODIFICATION_TYPE_COPY;
 		/** For COPY fill the destination field (dcopy) without mask. */
 		mlx5_flow_field_id_to_modify_info(&conf->dst, dcopy, NULL,
-						  value, conf->width, dst_width,
-						  &shift, dev, attr, error);
+						  conf->width, &shift, dev,
+						  attr, error);
 		/** Then construct the source field (field) with mask. */
 		mlx5_flow_field_id_to_modify_info(&conf->src, field, mask,
-						  value, conf->width, dst_width,
-						  &shift, dev, attr, error);
+						  conf->width, &shift,
+						  dev, attr, error);
 	}
 	item.mask = &mask;
 	return flow_dv_convert_modify_action(&item,
@@ -4874,9 +4834,9 @@ flow_dv_validate_action_modify_field(struct rte_eth_dev *dev,
 	const struct rte_flow_action_modify_field *action_modify_field =
 		action->conf;
 	uint32_t dst_width = mlx5_flow_item_field_width(priv,
-				action_modify_field->dst.field);
+				action_modify_field->dst.field, -1);
 	uint32_t src_width = mlx5_flow_item_field_width(priv,
-				action_modify_field->src.field);
+				action_modify_field->src.field, dst_width);
 
 	ret = flow_dv_validate_action_modify_hdr(action_flags, action, error);
 	if (ret)
-- 
2.18.1

Re: [dpdk-dev] [PATCH v6 1/5] ethdev: update modify field flow action

[Ferruh Yigit]

On 10/13/2021 7:45 PM, Viacheslav Ovsiienko wrote:
> The generic modify field flow action introduced in [1] has
> some issues related to the immediate source operand:
> 
>    - immediate source can be presented either as an unsigned
>      64-bit integer or pointer to data pattern in memory.
>      There was no explicit pointer field defined in the union.
> 
>    - the byte ordering for 64-bit integer was not specified.
>      Many fields have shorter lengths and byte ordering
>      is crucial.
> 
>    - how the bit offset is applied to the immediate source
>      field was not defined and documented.
> 
>    - 64-bit integer size is not enough to provide IPv6
>      addresses.
> 
> In order to cover the issues and exclude any ambiguities
> the following is done:
> 
>    - introduce the explicit pointer field
>      in rte_flow_action_modify_data structure
> 
>    - replace the 64-bit unsigned integer with 16-byte array
> 
>    - update the modify field flow action documentation
> 
> Appropriate deprecation notice has been removed.
> 
> [1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
> 
> Fixes: 2ba49b5f3721 ("doc: announce change to ethdev modify action data")
> 

Hi Viacheslav,

The commit in the 'Fixes' line is the commit that announces the deprecation
notice, sure this patch is not fixing it.

I am removing the above 'Fixes' line.

This patch is breaking the ABI and it can't be backported anyway, but
the 'Fixes' line still can be useful for documentation purpose, so if there
is valid fix commits please share them and I can squash them in next-net.

> Signed-off-by: Viacheslav Ovsiienko<viacheslavo@nvidia.com>
> Acked-by: Ori Kam<orika@nvidia.com>
> Acked-by: Andrew Rybchenko<andrew.rybchenko@oktetlabs.ru>

Re: [dpdk-dev] [PATCH v6 0/5] ethdev: update modify field flow action

[Ferruh Yigit]

On 10/13/2021 7:45 PM, Viacheslav Ovsiienko wrote:
> The generic modify field flow action introduced in [1] has
> some issues related to the immediate source operand:
> 
>    - immediate source can be presented either as an unsigned
>      64-bit integer or pointer to data pattern in memory.
>      There was no explicit pointer field defined in the union
> 
>    - the byte ordering for 64-bit integer was not specified.
>      Many fields have lesser lengths and byte ordering
>      is crucial.
> 
>    - how the bit offset is applied to the immediate source
>      field was not defined and documented
> 
>    - 64-bit integer size is not enough to provide MAC and
>      IPv6 addresses
> 
> In order to cover the issues and exclude any ambiguities
> the following is done:
> 
>    - introduce the explicit pointer field
>      in rte_flow_action_modify_data structure
> 
>    - replace the 64-bit unsigned integer with 16-byte array
> 
>    - update the modify field flow action documentation
> 
> Appropriate commit message has been removed.
> 
> [1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow action")
> [2] RFC: http://patches.dpdk.org/project/dpdk/patch/20210910141609.8410-1-viacheslavo@nvidia.com/
> [3] Deprecation notice: http://patches.dpdk.org/project/dpdk/patch/20210803085754.643180-1-orika@nvidia.com/
> [4] v1 - http://patches.dpdk.org/project/dpdk/cover/20211001195223.31909-1-viacheslavo@nvidia.com/
> [5] v2 - http://patches.dpdk.org/project/dpdk/patch/20211010234547.1495-2-viacheslavo@nvidia.com/
> [6] v3 - http://patches.dpdk.org/project/dpdk/cover/20211012080631.28504-1-viacheslavo@nvidia.com/
> [7] v4 - http://patches.dpdk.org/project/dpdk/cover/20211012104919.13145-1-viacheslavo@nvidia.com/
> [8] v5 - http://patches.dpdk.org/project/dpdk/patch/20211012202557.30295-2-viacheslavo@nvidia.com/
> 
> v2: - comments addressed
>      - documentation updated
>      - typos fixed
>      - mlx5 PMD updated
> 
> v3: - comments addressed
>      - documentation updated
>      - typos fixed
> 
> v4: - removed errorneously added Ack by Ori K. for mlx5 patch
>      - mlx5 patch updated - bug fixes and cleanup
> 
> v5: - fix compilation issue with unused variable in mlx5
> 
> v6: - fix compilation issue with unused variable in mlx5
> 
> Viacheslav Ovsiienko (5):
>    ethdev: update modify field flow action
>    ethdev: fix missed experimental tag for modify field action
>    app/testpmd: update modify field flow action support
>    app/testpmd: fix hex string parser in flow commands
>    net/mlx5: update modify field action
> 

Series applied to dpdk-next-net/main, thanks.

Re: [dpdk-dev] [PATCH v6 1/5] ethdev: update modify field flow action

[Slava Ovsiienko]

There is no actual fixing commits.
I've just followed the practice of removing the deprecation notice in dedicated commit.
So, no extra steps needed, thank you for the patch correction. 

With best regards,
Slava

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, October 14, 2021 15:09
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org
> Cc: Raslan Darawsheh <rasland@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Ori Kam
> <orika@nvidia.com>; Gregory Etelson <getelson@nvidia.com>; NBU-Contact-
> Thomas Monjalon <thomas@monjalon.net>
> Subject: Re: [dpdk-dev] [PATCH v6 1/5] ethdev: update modify field flow
> action
> 
> On 10/13/2021 7:45 PM, Viacheslav Ovsiienko wrote:
> > The generic modify field flow action introduced in [1] has some issues
> > related to the immediate source operand:
> >
> >    - immediate source can be presented either as an unsigned
> >      64-bit integer or pointer to data pattern in memory.
> >      There was no explicit pointer field defined in the union.
> >
> >    - the byte ordering for 64-bit integer was not specified.
> >      Many fields have shorter lengths and byte ordering
> >      is crucial.
> >
> >    - how the bit offset is applied to the immediate source
> >      field was not defined and documented.
> >
> >    - 64-bit integer size is not enough to provide IPv6
> >      addresses.
> >
> > In order to cover the issues and exclude any ambiguities the following
> > is done:
> >
> >    - introduce the explicit pointer field
> >      in rte_flow_action_modify_data structure
> >
> >    - replace the 64-bit unsigned integer with 16-byte array
> >
> >    - update the modify field flow action documentation
> >
> > Appropriate deprecation notice has been removed.
> >
> > [1] commit 73b68f4c54a0 ("ethdev: introduce generic modify flow
> > action")
> >
> > Fixes: 2ba49b5f3721 ("doc: announce change to ethdev modify action
> > data")
> >
> 
> Hi Viacheslav,
> 
> The commit in the 'Fixes' line is the commit that announces the deprecation
> notice, sure this patch is not fixing it.
> 
> I am removing the above 'Fixes' line.
> 
> This patch is breaking the ABI and it can't be backported anyway, but the
> 'Fixes' line still can be useful for documentation purpose, so if there is valid fix
> commits please share them and I can squash them in next-net.
> 
> > Signed-off-by: Viacheslav Ovsiienko<viacheslavo@nvidia.com>
> > Acked-by: Ori Kam<orika@nvidia.com>
> > Acked-by: Andrew Rybchenko<andrew.rybchenko@oktetlabs.ru>