From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4F59CA034F; Mon, 11 Oct 2021 11:54:53 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 379B1410DA; Mon, 11 Oct 2021 11:54:53 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 253D440E0F for ; Mon, 11 Oct 2021 11:54:51 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id C39CB7F4FD; Mon, 11 Oct 2021 12:54:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru C39CB7F4FD DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1633946090; bh=F4WvQ/jBifhVkB81eOWJtOqzeeY1JytGLFeNkk0k2NQ=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=xAgPbJkh1MvWAt4Pi4a10fxUPk/ONtoVyCFyrQHFsEj6bYCUkR5oGJV8bgGfPNUZB 7zVSlzkIt/ZJ3Xluoi2PEFU0h678Jq2OmGrIgCeMEJxlwZd7KL4TPfVPvccL1I/rqb 9Lrm4G+LYzfaC13Kw1Xb9LSjIqmHM2K9ogq34x/s= To: Viacheslav Ovsiienko , dev@dpdk.org Cc: rasland@nvidia.com, matan@nvidia.com, shahafs@nvidia.com, orika@nvidia.com, getelson@nvidia.com, thomas@monjalon.net References: <20210910141609.8410-1-viacheslavo@nvidia.com> <20211010234547.1495-1-viacheslavo@nvidia.com> <20211010234547.1495-2-viacheslavo@nvidia.com> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <57fd8c07-a807-bf3c-10c0-e560b5089685@oktetlabs.ru> Date: Mon, 11 Oct 2021 12:54:50 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <20211010234547.1495-2-viacheslavo@nvidia.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 1/5] ethdev: update modify field flow action X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > --- > 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; > }; > }; > >