< From: Slava Ovsiienko <viachesl...@nvidia.com> on Wednesday, January 27, 2021
6:37
>
> Hi, Alexander
>
> Please, see my minor comments below.
>
> Besides this:
> Acked-by: Viacheslav Ovsiienko <viachesl...@nvidia.com>
>
> > -----Original Message-----
> > From: Alexander Kozyrev <akozy...@nvidia.com>
> > Sent: Wednesday, January 27, 2021 4:49
> > To: dev@dpdk.org
> > Cc: Raslan Darawsheh <rasl...@nvidia.com>; Slava Ovsiienko
> > <viachesl...@nvidia.com>; Matan Azrad <ma...@nvidia.com>; Ori Kam
> > <or...@nvidia.com>
> > Subject: [PATCH] net/mlx5: support modify field rte flow action
> >
> > Add support for new MODIFY_FIELD action to the Mellanox PMD.
> > This is the generic API that allows to manipulate any packet header field by
> > copying data from another packet field or mark, metadata, tag, or
> immediate
> > value (or pointer to it).
> >
> > Since the API is generic and covers a lot of action under its umbrella it
> makes
> > sense to implement all the mechanics gradually in order to move to this API
> > for any packet field manipulations in the future. This is first step of RTE
> flows
> Typo: "the first"
>
> > consolidation.
> >
> > The modify field RTE flow supports three operations: set, add and sub. This
> Missed API|action? Should it be "The modify field RTE flow action supports"
> ?
Thanks, will fix typos.
> > patch brings to live only the "set" operation.
> > Support is provided for any packet header field as well as meta/tag/mark
> and
> > immediate value can be used as a source.
> >
> > There are few limitations for this first version of API support:
> > - encapsulation levels are not supported, just outermost header can be
> > manipulated for now.
> > - offsets can only be 4-bytes aligned: 32, 64 and 96 for IPv6.
> > - the special ITEM_START ID is not supported as we do not allow to cross
> > packet header field boundaries yet.
> >
> > Signed-off-by: Alexander Kozyrev <akozy...@nvidia.com>
> > ---
> > drivers/net/mlx5/mlx5_flow.h | 4 +-
> > drivers/net/mlx5/mlx5_flow_dv.c | 507
> > +++++++++++++++++++++++++++++++-
> > 2 files changed, 508 insertions(+), 3 deletions(-)
> >
> mlx5 documentation update?
> release notes update?
Let me add them.
> > diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> > index 2178a04e3a..6f6828c6a1 100644
> > --- a/drivers/net/mlx5/mlx5_flow.h
> > +++ b/drivers/net/mlx5/mlx5_flow.h
> > @@ -219,6 +219,7 @@ enum mlx5_feature_name { #define
> > MLX5_FLOW_ACTION_SAMPLE (1ull << 36) #define
> > MLX5_FLOW_ACTION_TUNNEL_SET (1ull << 37) #define
> > MLX5_FLOW_ACTION_TUNNEL_MATCH (1ull << 38)
> > +#define MLX5_FLOW_ACTION_MODIFY_FIELD (1ull << 39)
> >
> > #define MLX5_FLOW_FATE_ACTIONS \
> > (MLX5_FLOW_ACTION_DROP | MLX5_FLOW_ACTION_QUEUE | \
> > @@ -249,7 +250,8 @@ enum mlx5_feature_name {
> > MLX5_FLOW_ACTION_MARK_EXT | \
> > MLX5_FLOW_ACTION_SET_META | \
> > MLX5_FLOW_ACTION_SET_IPV4_DSCP | \
> > - MLX5_FLOW_ACTION_SET_IPV6_DSCP)
> > + MLX5_FLOW_ACTION_SET_IPV6_DSCP | \
> > + MLX5_FLOW_ACTION_MODIFY_FIELD)
> >
> > #define MLX5_FLOW_VLAN_ACTIONS
> (MLX5_FLOW_ACTION_OF_POP_VLAN
> > | \
> > MLX5_FLOW_ACTION_OF_PUSH_VLAN)
> > diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> > b/drivers/net/mlx5/mlx5_flow_dv.c index 1a0c0be680..d842dc9887 100644
> > --- a/drivers/net/mlx5/mlx5_flow_dv.c
> > +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> > @@ -209,6 +209,8 @@ rte_col_2_mlx5_col(enum rte_color rcol)
> > return MLX5_FLOW_COLOR_UNDEFINED;
> > }
> >
> > +#define MLX5DV_FLOW_MAX_MOD_FIELDS 5
> Let's move to the mlx5_flow.h, closer to other #defines
Ok.
> > +
> > struct field_modify_info {
> > uint32_t size; /* Size of field in protocol header, in bytes. */
> > uint32_t offset; /* Offset of field in protocol header, in bytes. */ @@
> -
> > 431,6 +433,9 @@ flow_dv_convert_modify_action(struct rte_flow_item
> > *item,
> > (int)dcopy->offset < 0 ? off_b : dcopy-
> >offset;
> > /* Convert entire record to big-endian format. */
> > actions[i].data1 =
> rte_cpu_to_be_32(actions[i].data1);
> > + ++dcopy;
> > + if (!dcopy)
> > + break;
> Sorry, I do not follow. dcopy is the structure pointer (we assume it points to
> the array).
> How pointer can be NULL after update ++dcopy ?
Just a precaution if we go past the array due to some bug.
> > } else {
> > MLX5_ASSERT(item->spec);
> > data = flow_dv_fetch_field((const uint8_t *)item-
> > >spec + @@ -1324,6 +1329,339 @@
> > flow_dv_convert_action_modify_ipv6_dscp
> > MLX5_MODIFICATION_TYPE_SET,
> > error); }
> >
> > +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,
> > + struct rte_eth_dev *dev,
> > + const struct rte_flow_attr *attr,
> > + struct rte_flow_error *error)
> > +{
> > + uint32_t idx = 0;
> > + switch (data->field) {
> > + case RTE_FLOW_FIELD_START:
> > + /* not supported yet */
> > + break;
> Let's move to default? To catch validation gap?
Ok.
> > + case RTE_FLOW_FIELD_MAC_DST:
> > + if (mask) {
> > + info[idx] = (struct field_modify_info){4, 0,
> > +
> > MLX5_MODI_OUT_DMAC_47_16};
> > + mask[idx] = (data->offset == 32) ? 0x0 : 0xffffffff;
> With zero mask we could just skip the element here
> (not critical - anyway, it will be dropped at conversion stage to HW actions)
That is the ides - to skip element at the conversion stage.
