Hi Ori,
> -----Original Message-----
> From: Ori Kam <or...@nvidia.com>
> Sent: Sunday, 21 May 2023 22:03
>
> Hi Michael,
>
> > -----Original Message-----
> > From: Michael Baum <michae...@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 <michae...@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
