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 <viachesl...@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;
> };
> };
>
>
