> -----Original Message-----
> From: Slava Ovsiienko <viachesl...@nvidia.com>
> Sent: Wednesday, September 18, 2024 15:46
> To: dev@dpdk.org
> Cc: Matan Azrad <ma...@nvidia.com>; Raslan Darawsheh
> <rasl...@nvidia.com>; Ori Kam <or...@nvidia.com>; Dariusz Sosnowski
> <dsosnow...@nvidia.com>; sta...@dpdk.org
> Subject: [PATCH v2 8/9] net/mlx5: fix non full word sample fields in flex item
>
> If the sample field in flex item did not cover the entire 32-bit word (width
> was not
> verified 32 bits) or was not aligned on the byte boundary the match on this
> sample in flows happened to be ignored or wrongly missed. The field mask "def"
> was build in wrong endianness, and non-byte aligned shifts were wrongly
> performed for the pattern masks and values.
>
> Fixes: 6dac7d7ff2bf ("net/mlx5: translate flex item pattern into matcher")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Viacheslav Ovsiienko <viachesl...@nvidia.com>
> ---
> drivers/net/mlx5/hws/mlx5dr_definer.c | 4 +--
> drivers/net/mlx5/mlx5.h | 5 ++-
> drivers/net/mlx5/mlx5_flow_dv.c | 5 ++-
> drivers/net/mlx5/mlx5_flow_flex.c | 47 +++++++++++++--------------
> 4 files changed, 29 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/mlx5/hws/mlx5dr_definer.c
> b/drivers/net/mlx5/hws/mlx5dr_definer.c
> index 2dfcc5eba6..10b986d66b 100644
> --- a/drivers/net/mlx5/hws/mlx5dr_definer.c
> +++ b/drivers/net/mlx5/hws/mlx5dr_definer.c
> @@ -574,7 +574,7 @@ mlx5dr_definer_flex_parser_set(struct
> mlx5dr_definer_fc *fc,
> idx = fc->fname - MLX5DR_DEFINER_FNAME_FLEX_PARSER_0;
> byte_off -= idx * sizeof(uint32_t);
> ret = mlx5_flex_get_parser_value_per_byte_off(flex, flex->handle,
> byte_off,
> - false, is_inner, &val);
> + is_inner, &val);
> if (ret == -1 || !val)
> return;
>
> @@ -2825,7 +2825,7 @@ mlx5dr_definer_conv_item_flex_parser(struct
> mlx5dr_definer_conv_data *cd,
> for (i = 0; i < MLX5_GRAPH_NODE_SAMPLE_NUM; i++) {
> byte_off = base_off - i * sizeof(uint32_t);
> ret = mlx5_flex_get_parser_value_per_byte_off(m, v->handle,
> byte_off,
> - true, is_inner,
> &mask);
> + is_inner,
> &mask);
> if (ret == -1) {
> rte_errno = EINVAL;
> return rte_errno;
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> b1423b6868..0fb18f7fb1 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -2600,11 +2600,10 @@ void mlx5_flex_flow_translate_item(struct
> rte_eth_dev *dev, void *matcher,
> void *key, const struct rte_flow_item *item,
> bool is_inner);
> int mlx5_flex_get_sample_id(const struct mlx5_flex_item *tp,
> - uint32_t idx, uint32_t *pos,
> - bool is_inner, uint32_t *def);
> + uint32_t idx, uint32_t *pos, bool is_inner);
> int mlx5_flex_get_parser_value_per_byte_off(const struct rte_flow_item_flex
> *item,
> void *flex, uint32_t byte_off,
> - bool is_mask, bool tunnel,
> uint32_t *value);
> + bool tunnel, uint32_t *value);
> int mlx5_flex_get_tunnel_mode(const struct rte_flow_item *item,
> enum rte_flow_item_flex_tunnel_mode
> *tunnel_mode); int mlx5_flex_acquire_index(struct rte_eth_dev *dev, diff
> --git
> a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c index
> b18bb430d7..d2a3f829d5 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -1526,7 +1526,6 @@ mlx5_modify_flex_item(const struct rte_eth_dev
> *dev,
> const struct mlx5_flex_pattern_field *map;
> uint32_t offset = data->offset;
> uint32_t width_left = width;
> - uint32_t def;
> uint32_t cur_width = 0;
> uint32_t tmp_ofs;
> uint32_t idx = 0;
> @@ -1551,7 +1550,7 @@ mlx5_modify_flex_item(const struct rte_eth_dev
> *dev,
> tmp_ofs = pos < data->offset ? data->offset - pos : 0;
> for (j = i; i < flex->mapnum && width_left > 0; ) {
> map = flex->map + i;
> - id = mlx5_flex_get_sample_id(flex, i, &pos, false, &def);
> + id = mlx5_flex_get_sample_id(flex, i, &pos, false);
> if (id == -1) {
> i++;
> /* All left length is dummy */
> @@ -1570,7 +1569,7 @@ mlx5_modify_flex_item(const struct rte_eth_dev
> *dev,
> * 2. Width has been covered.
> */
> for (j = i + 1; j < flex->mapnum; j++) {
> - tmp_id = mlx5_flex_get_sample_id(flex, j,
> &pos, false, &def);
> + tmp_id = mlx5_flex_get_sample_id(flex, j,
> &pos, false);
> if (tmp_id == -1) {
> i = j;
> pos -= flex->map[j].width;
> diff --git a/drivers/net/mlx5/mlx5_flow_flex.c
> b/drivers/net/mlx5/mlx5_flow_flex.c
> index 0c41b956b0..bf38643a23 100644
> --- a/drivers/net/mlx5/mlx5_flow_flex.c
> +++ b/drivers/net/mlx5/mlx5_flow_flex.c
> @@ -118,28 +118,32 @@ mlx5_flex_get_bitfield(const struct
> rte_flow_item_flex *item,
> uint32_t pos, uint32_t width, uint32_t shift) {
> const uint8_t *ptr = item->pattern + pos / CHAR_BIT;
> - uint32_t val, vbits;
> + uint32_t val, vbits, skip = pos % CHAR_BIT;
>
> /* Proceed the bitfield start byte. */
> MLX5_ASSERT(width <= sizeof(uint32_t) * CHAR_BIT && width);
> MLX5_ASSERT(width + shift <= sizeof(uint32_t) * CHAR_BIT);
> if (item->length <= pos / CHAR_BIT)
> return 0;
> - val = *ptr++ >> (pos % CHAR_BIT);
> + /* Bits are enumerated in byte in network order: 01234567 */
> + val = *ptr++;
> vbits = CHAR_BIT - pos % CHAR_BIT;
> - pos = (pos + vbits) / CHAR_BIT;
> + pos = RTE_ALIGN_CEIL(pos, CHAR_BIT) / CHAR_BIT;
> vbits = RTE_MIN(vbits, width);
> - val &= RTE_BIT32(vbits) - 1;
> + /* Load bytes to cover the field width, checking pattern boundary */
> while (vbits < width && pos < item->length) {
> uint32_t part = RTE_MIN(width - vbits, (uint32_t)CHAR_BIT);
> uint32_t tmp = *ptr++;
>
> - pos++;
> - tmp &= RTE_BIT32(part) - 1;
> - val |= tmp << vbits;
> + val |= tmp << RTE_ALIGN_CEIL(vbits, CHAR_BIT);
> vbits += part;
> + pos++;
> }
> - return rte_bswap32(val <<= shift);
> + val = rte_cpu_to_be_32(val);
> + val <<= skip;
> + val >>= shift;
> + val &= (RTE_BIT64(width) - 1) << (sizeof(uint32_t) * CHAR_BIT - shift -
> width);
> + return val;
> }
>
> #define SET_FP_MATCH_SAMPLE_ID(x, def, msk, val, sid) \ @@ -211,21 +215,17
> @@ mlx5_flex_set_match_sample(void *misc4_m, void *misc4_v,
> * Where to search the value and mask.
> * @param[in] is_inner
> * For inner matching or not.
> - * @param[in, def] def
> - * Mask generated by mapping shift and width.
> *
> * @return
> * 0 on success, -1 to ignore.
> */
> int
> mlx5_flex_get_sample_id(const struct mlx5_flex_item *tp,
> - uint32_t idx, uint32_t *pos,
> - bool is_inner, uint32_t *def)
> + uint32_t idx, uint32_t *pos, bool is_inner)
> {
> const struct mlx5_flex_pattern_field *map = tp->map + idx;
> uint32_t id = map->reg_id;
>
> - *def = (RTE_BIT64(map->width) - 1) << map->shift;
> /* Skip placeholders for DUMMY fields. */
> if (id == MLX5_INVALID_SAMPLE_REG_ID) {
> *pos += map->width;
> @@ -252,8 +252,6 @@ mlx5_flex_get_sample_id(const struct mlx5_flex_item
> *tp,
> * Mlx5 flex item sample mapping handle.
> * @param[in] byte_off
> * Mlx5 flex item format_select_dw.
> - * @param[in] is_mask
> - * Spec or mask.
> * @param[in] tunnel
> * Tunnel mode or not.
> * @param[in, def] value
> @@ -265,25 +263,23 @@ mlx5_flex_get_sample_id(const struct mlx5_flex_item
> *tp, int mlx5_flex_get_parser_value_per_byte_off(const struct
> rte_flow_item_flex *item,
> void *flex, uint32_t byte_off,
> - bool is_mask, bool tunnel, uint32_t
> *value)
> + bool tunnel, uint32_t *value)
> {
> struct mlx5_flex_pattern_field *map;
> struct mlx5_flex_item *tp = flex;
> - uint32_t def, i, pos, val;
> + uint32_t i, pos, val;
> int id;
>
> *value = 0;
> for (i = 0, pos = 0; i < tp->mapnum && pos < item->length * CHAR_BIT;
> i++) {
> map = tp->map + i;
> - id = mlx5_flex_get_sample_id(tp, i, &pos, tunnel, &def);
> + id = mlx5_flex_get_sample_id(tp, i, &pos, tunnel);
> if (id == -1)
> continue;
> if (id >= (int)tp->devx_fp->num_samples || id >=
> MLX5_GRAPH_NODE_SAMPLE_NUM)
> return -1;
> if (byte_off == tp->devx_fp->sample_info[id].sample_dw_data *
> sizeof(uint32_t)) {
> val = mlx5_flex_get_bitfield(item, pos, map->width,
> map->shift);
> - if (is_mask)
> - val &= RTE_BE32(def);
> *value |= val;
> }
> pos += map->width;
> @@ -355,10 +351,10 @@ mlx5_flex_flow_translate_item(struct rte_eth_dev
> *dev,
> spec = item->spec;
> mask = item->mask;
> tp = (struct mlx5_flex_item *)spec->handle;
> - for (i = 0; i < tp->mapnum; i++) {
> + for (i = 0; i < tp->mapnum && pos < (spec->length * CHAR_BIT); i++) {
> struct mlx5_flex_pattern_field *map = tp->map + i;
> uint32_t val, msk, def;
> - int id = mlx5_flex_get_sample_id(tp, i, &pos, is_inner, &def);
> + int id = mlx5_flex_get_sample_id(tp, i, &pos, is_inner);
>
> if (id == -1)
> continue;
> @@ -366,11 +362,14 @@ mlx5_flex_flow_translate_item(struct rte_eth_dev
> *dev,
> if (id >= (int)tp->devx_fp->num_samples ||
> id >= MLX5_GRAPH_NODE_SAMPLE_NUM)
> return;
> + def = (uint32_t)(RTE_BIT64(map->width) - 1);
> + def <<= (sizeof(uint32_t) * CHAR_BIT - map->shift - map-
> >width);
> val = mlx5_flex_get_bitfield(spec, pos, map->width, map-
> >shift);
> - msk = mlx5_flex_get_bitfield(mask, pos, map->width, map-
> >shift);
> + msk = pos < (mask->length * CHAR_BIT) ?
> + mlx5_flex_get_bitfield(mask, pos, map->width, map->shift)
> :
> +def;
> sample_id = tp->devx_fp->sample_ids[id];
> mlx5_flex_set_match_sample(misc4_m, misc4_v,
> - def, msk & def, val & msk & def,
> + def, msk, val & msk,
> sample_id, id);
> pos += map->width;
> }
> --
> 2.34.1
Acked-by: Dariusz Sosnowski <dsosnow...@nvidia.com>
Resending the Ack for each patch separately, because patchwork assigned my Ack
for the series to v1, not v2.
Best regards,
Dariusz Sosnowski
