[PATCH v2 5/5] ethdev: add MPLS header modification support

[Michael Baum]

Hi Ori,

> -----Original Message-----
> From: Ori Kam <orika at nvidia.com>
> Sent: Sunday, 21 May 2023 22:03
> 
> Hi Michael,
> 
> > -----Original Message-----
> > From: Michael Baum <michaelba at nvidia.com>
> > Sent: Thursday, May 18, 2023 8:40 PM
> >
> > Add support for MPLS modify header using "RTE_FLOW_FIELD_MPLS" id.
> >
> > Since MPLS heaser might appear more the one time in
> > inner/outer/tunnel, a new field was added to
> > "rte_flow_action_modify_data" structure in addition to "level" field.
> > The "tag_index" field is the index of the header inside encapsulation
> > level. It is used for modify multiple MPLS headers in same
> > encapsulation level.
> >
> > This addition enables to modify multiple VLAN headers too, so the
> > description of "RTE_FLOW_FIELD_VLAN_XXXX" was updated.
> >
> > Signed-off-by: Michael Baum <michaelba at nvidia.com>
> > ---
> 
> [Snip]
> 
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > index ec812de335..591e43b5ac 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -2925,8 +2925,7 @@ See ``enum rte_flow_field_id`` for the list of
> > supported fields.
> >
> >  ``width`` defines a number of bits to use from ``src`` field.
> >
> > -``level`` is used to access any packet field on any encapsulation
> > level -as well as any tag element in the tag array:
> > +``level`` is used to access any packet field on any encapsulation level:
> >
> >  - ``0`` means the default behaviour. Depending on the packet type,
> >    it can mean outermost, innermost or anything in between.
> > @@ -2934,8 +2933,9 @@ as well as any tag element in the tag array:
> >  - ``2`` and subsequent values requests access to the specified packet
> >    encapsulation level, from outermost to innermost (lower to higher values).
> >
> > -For the tag array (in case of multiple tags are supported and
> > present) -``level`` translates directly into the array index.
> > +``tag_index`` is the index of the header inside encapsulation level.
> > +It is used for modify either ``VLAN`` or ``MPLS`` or ``TAG`` headers
> > +which multiple of them might be supported in same encapsulation level.
> >
> This is an API change and must be documented as such in the release notes.

OK, I'll add it to release notes and send v3.

> I suggest to avoid breaking working apps that it will be documented that for this
> release when using tag the PMD will look at level and tag_id If both equal to 0 -->
> the requested tag is zero If tag_id == 0 and level != 0 PMD will selected the tag
> based on level, but will output a warning If tag_id != 0 and level == 0 PMD will
> select the tag based on the tag_id

I'm preparing patch for it in MLX5 PMD (the only PMD which uses "RTE_FLOW_FIELD_TAG").
The behavior for "RTE_FLOW_FIELD_TAG" type is:
1. (level == 0 && tag_index == 0) -> the requested tag is zero.
2. (level == 0 && tag_index != 0) -> the requested tag is "tag_index".
3. (level != 0 && tag_index == 0) -> the requested tag is "level" + warning.
4. (level != 0 && tag_index != 0) -> action creation failed + error.

My question is where to document it? In rte_flow.rst or for PMD doc?

> >  ``type`` is used to specify (along with ``class_id``) the Geneve
> > option which  is being modified.
> > @@ -3011,7 +3011,9 @@ and provide immediate value 0xXXXX85XX.
> >
> > +=================+============================================
> > ==============+
> >     | ``field``       | ID: packet field, mark, meta, tag, immediate, pointer    |
> >     +-----------------+----------------------------------------------------------+
> > -   | ``level``       | encapsulation level of a packet field or tag array index |
> > +   | ``level``       | encapsulation level of a packet field                    |
> > +   +-----------------+----------------------------------------------------------+
> > +   | ``tag_index``   | tag index inside encapsulation level                     |
> >     +-----------------+----------------------------------------------------------+
> >     | ``type``        | geneve option type                                       |
> >
> > +-----------------+---------------------------------------------------
> > -------+ diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> > index f30d4b033f..34e536d662 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -3740,8 +3740,8 @@ enum rte_flow_field_id {
> >  	RTE_FLOW_FIELD_START = 0,	/**< Start of a packet. */
> >  	RTE_FLOW_FIELD_MAC_DST,		/**< Destination MAC
> > Address. */
> >  	RTE_FLOW_FIELD_MAC_SRC,		/**< Source MAC Address. */
> > -	RTE_FLOW_FIELD_VLAN_TYPE,	/**< 802.1Q Tag Identifier. */
> > -	RTE_FLOW_FIELD_VLAN_ID,		/**< 802.1Q VLAN Identifier.
> > */
> > +	RTE_FLOW_FIELD_VLAN_TYPE,	/**< VLAN Tag Identifier. */
> > +	RTE_FLOW_FIELD_VLAN_ID,		/**< VLAN Identifier. */
> >  	RTE_FLOW_FIELD_MAC_TYPE,	/**< EtherType. */
> >  	RTE_FLOW_FIELD_IPV4_DSCP,	/**< IPv4 DSCP. */
> >  	RTE_FLOW_FIELD_IPV4_TTL,	/**< IPv4 Time To Live. */
> > @@ -3775,7 +3775,8 @@ enum rte_flow_field_id {
> >  	RTE_FLOW_FIELD_HASH_RESULT,	/**< Hash result. */
> >  	RTE_FLOW_FIELD_GENEVE_OPT_TYPE,	/**< GENEVE option type */
> >  	RTE_FLOW_FIELD_GENEVE_OPT_CLASS,/**< GENEVE option class */
> > -	RTE_FLOW_FIELD_GENEVE_OPT_DATA	/**< GENEVE option data */
> > +	RTE_FLOW_FIELD_GENEVE_OPT_DATA,	/**< GENEVE option
> > data */
> > +	RTE_FLOW_FIELD_MPLS		/**< MPLS header. */
> >  };
> >
> >  /**
> > @@ -3789,7 +3790,7 @@ struct rte_flow_action_modify_data {
> >  	RTE_STD_C11
> >  	union {
> >  		struct {
> > -			/** Encapsulation level or tag index or flex item
> > handle. */
> > +			/** Encapsulation level and tag index or flex item
> > handle. */
> >  			union {
> >  				struct {
> >  					/**
> > @@ -3820,20 +3821,36 @@ struct rte_flow_action_modify_data {
> >  					 *
> >  					 * Values other than @p 0 are not
> >  					 * necessarily supported.
> > +					 *
> > +					 * @note that for MPLS field,
> > +					 * encapsulation level also include
> > +					 * tunnel since MPLS may appear in
> > +					 * outer, inner or tunnel.
> >  					 */
> >  					uint8_t level;
> > -					/**
> > -					 * Geneve option type. relevant only
> > -					 * for
> > RTE_FLOW_FIELD_GENEVE_OPT_XXXX
> > -					 * modification type.
> > -					 */
> > -					uint8_t type;
> > -					/**
> > -					 * Geneve option class. relevant only
> > -					 * for
> > RTE_FLOW_FIELD_GENEVE_OPT_XXXX
> > -					 * modification type.
> > -					 */
> > -					rte_be16_t class_id;
> > +					union {
> > +						/**
> > +						 * Tag index array inside
> > +						 * encapsulation level.
> > +						 */
> 
> I think it is also worth noting that this is used also for VLAN and MPLS.

OK

> 
> > +						uint8_t tag_index;
> > +						/**
> > +						 * Geneve option identifier.
> > +						 * relevant only for
> > +						 *
> > RTE_FLOW_FIELD_GENEVE_OPT_XXXX
> > +						 * modification type.
> > +						 */
> > +						struct {
> > +							/**
> > +							 * Geneve option
> > type.
> > +							 */
> > +							uint8_t type;
> > +							/**
> > +							 * Geneve option
> > class.
> > +							 */
> > +							rte_be16_t class_id;
> > +						};
> > +					};
> >  				};
> >  				struct rte_flow_item_flex_handle
> > *flex_handle;
> >  			};
> > --
> > 2.25.1