The metadata register c0 field in the matcher might be split into two independent fields - the source vport index and META item value. These fields have no permanent assigned bits, the configuration is queried from the kernel drivers. MLX5_SET configures the specified 32-bit field as whole entity. For metadata register c0 we should take into account the provided mask in order to configure the specified subfield bits only. Fixes: acfcd5c52f94 ("net/mlx5: update meta register matcher set") Cc: stable@dpdk.org Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com> --- drivers/net/mlx5/mlx5_flow_dv.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c index 4c16281..893db3e 100644 --- a/drivers/net/mlx5/mlx5_flow_dv.c +++ b/drivers/net/mlx5/mlx5_flow_dv.c @@ -5742,6 +5742,7 @@ struct field_modify_info modify_tcp[] = { MLX5_ADDR_OF(fte_match_param, matcher, misc_parameters_2); void *misc2_v = MLX5_ADDR_OF(fte_match_param, key, misc_parameters_2); + uint32_t temp; data &= mask; switch (reg_type) { @@ -5754,8 +5755,18 @@ struct field_modify_info modify_tcp[] = { MLX5_SET(fte_match_set_misc2, misc2_v, metadata_reg_b, data); break; case REG_C_0: - MLX5_SET(fte_match_set_misc2, misc2_m, metadata_reg_c_0, mask); - MLX5_SET(fte_match_set_misc2, misc2_v, metadata_reg_c_0, data); + /* + * The metadata register C0 field might be divided into + * source vport index and META item value, we should set + * this field accorfing to specified mask, not as whole one. + */ + temp = MLX5_GET(fte_match_set_misc2, misc2_m, metadata_reg_c_0); + temp |= mask; + MLX5_SET(fte_match_set_misc2, misc2_m, metadata_reg_c_0, temp); + temp = MLX5_GET(fte_match_set_misc2, misc2_v, metadata_reg_c_0); + temp &= ~mask; + temp |= data; + MLX5_SET(fte_match_set_misc2, misc2_v, metadata_reg_c_0, temp); break; case REG_C_1: MLX5_SET(fte_match_set_misc2, misc2_m, metadata_reg_c_1, mask); -- 1.8.3.1
From: Viacheslav Ovsiienko > The metadata register c0 field in the matcher might be split into two > independent fields - the source vport index and META item value. These > fields have no permanent assigned bits, the configuration is queried from the > kernel drivers. > > MLX5_SET configures the specified 32-bit field as whole entity. > For metadata register c0 we should take into account the provided mask in > order to configure the specified subfield bits only. > > Fixes: acfcd5c52f94 ("net/mlx5: update meta register matcher set") > Cc: stable@dpdk.org > > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com> > --- > drivers/net/mlx5/mlx5_flow_dv.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/mlx5/mlx5_flow_dv.c > b/drivers/net/mlx5/mlx5_flow_dv.c index 4c16281..893db3e 100644 > --- a/drivers/net/mlx5/mlx5_flow_dv.c > +++ b/drivers/net/mlx5/mlx5_flow_dv.c > @@ -5742,6 +5742,7 @@ struct field_modify_info modify_tcp[] = { > MLX5_ADDR_OF(fte_match_param, matcher, > misc_parameters_2); > void *misc2_v = > MLX5_ADDR_OF(fte_match_param, key, > misc_parameters_2); > + uint32_t temp; > > data &= mask; > switch (reg_type) { > @@ -5754,8 +5755,18 @@ struct field_modify_info modify_tcp[] = { > MLX5_SET(fte_match_set_misc2, misc2_v, metadata_reg_b, > data); > break; > case REG_C_0: > - MLX5_SET(fte_match_set_misc2, misc2_m, > metadata_reg_c_0, mask); > - MLX5_SET(fte_match_set_misc2, misc2_v, > metadata_reg_c_0, data); > + /* > + * The metadata register C0 field might be divided into > + * source vport index and META item value, we should set > + * this field accorfing to specified mask, not as whole one. > + */ Typo - accorfing => according. > + temp = MLX5_GET(fte_match_set_misc2, misc2_m, > metadata_reg_c_0); > + temp |= mask; > + MLX5_SET(fte_match_set_misc2, misc2_m, > metadata_reg_c_0, temp); > + temp = MLX5_GET(fte_match_set_misc2, misc2_v, > metadata_reg_c_0); > + temp &= ~mask; > + temp |= data; > + MLX5_SET(fte_match_set_misc2, misc2_v, > metadata_reg_c_0, temp); > break; > case REG_C_1: > MLX5_SET(fte_match_set_misc2, misc2_m, > metadata_reg_c_1, mask); Raslan, please fix the typo in integration. Acked-by: Matan Azrad <matan@mellanox.com>
Hi,
> -----Original Message-----
> From: Matan Azrad <matan@mellanox.com>
> Sent: Monday, January 6, 2020 4:50 PM
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> Cc: Raslan Darawsheh <rasland@mellanox.com>; Ori Kam
> <orika@mellanox.com>; stable@dpdk.org
> Subject: RE: [PATCH] net/mlx5: fix matcher metadata register c0 field setup
>
>
>
> From: Viacheslav Ovsiienko
> > The metadata register c0 field in the matcher might be split into two
> > independent fields - the source vport index and META item value. These
> > fields have no permanent assigned bits, the configuration is queried
> > from the kernel drivers.
> >
> > MLX5_SET configures the specified 32-bit field as whole entity.
> > For metadata register c0 we should take into account the provided mask
> > in order to configure the specified subfield bits only.
> >
> > Fixes: acfcd5c52f94 ("net/mlx5: update meta register matcher set")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > ---
> > drivers/net/mlx5/mlx5_flow_dv.c | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> > b/drivers/net/mlx5/mlx5_flow_dv.c index 4c16281..893db3e 100644
> > --- a/drivers/net/mlx5/mlx5_flow_dv.c
> > +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> > @@ -5742,6 +5742,7 @@ struct field_modify_info modify_tcp[] = {
> > MLX5_ADDR_OF(fte_match_param, matcher,
> misc_parameters_2);
> > void *misc2_v =
> > MLX5_ADDR_OF(fte_match_param, key,
> > misc_parameters_2);
> > + uint32_t temp;
> >
> > data &= mask;
> > switch (reg_type) {
> > @@ -5754,8 +5755,18 @@ struct field_modify_info modify_tcp[] = {
> > MLX5_SET(fte_match_set_misc2, misc2_v, metadata_reg_b,
> data);
> > break;
> > case REG_C_0:
> > - MLX5_SET(fte_match_set_misc2, misc2_m,
> > metadata_reg_c_0, mask);
> > - MLX5_SET(fte_match_set_misc2, misc2_v,
> > metadata_reg_c_0, data);
> > + /*
> > + * The metadata register C0 field might be divided into
> > + * source vport index and META item value, we should set
> > + * this field accorfing to specified mask, not as whole one.
> > + */
>
> Typo - accorfing => according.
>
> > + temp = MLX5_GET(fte_match_set_misc2, misc2_m,
> > metadata_reg_c_0);
> > + temp |= mask;
> > + MLX5_SET(fte_match_set_misc2, misc2_m,
> > metadata_reg_c_0, temp);
> > + temp = MLX5_GET(fte_match_set_misc2, misc2_v,
> > metadata_reg_c_0);
> > + temp &= ~mask;
> > + temp |= data;
> > + MLX5_SET(fte_match_set_misc2, misc2_v,
> > metadata_reg_c_0, temp);
> > break;
> > case REG_C_1:
> > MLX5_SET(fte_match_set_misc2, misc2_m,
> metadata_reg_c_1, mask);
>
> Raslan, please fix the typo in integration.
> Acked-by: Matan Azrad <matan@mellanox.com>
>
Fixed typo issue and applied patch to next-net-mlx.
Kindest regards,
Raslan Darawsheh
On 12/20/2019 7:48 AM, Viacheslav Ovsiienko wrote: > The metadata register c0 field in the matcher might be split > into two independent fields - the source vport index and META > item value. These fields have no permanent assigned bits, the > configuration is queried from the kernel drivers. > > MLX5_SET configures the specified 32-bit field as whole entity. > For metadata register c0 we should take into account the provided > mask in order to configure the specified subfield bits only. Hi Slava, Is there a more human friendly name for the field instead of "C0"? I don't know what "matcher metadata register" is, what is the impact of the fix? Which functionality was broken now fixed? Does it make sense to reflect it in the patch title / commit log? Same comment for the related patches: net/mlx5: fix register c0 usage for metadata entities net/mlx5: fix metadata item endianness conversion > > Fixes: acfcd5c52f94 ("net/mlx5: update meta register matcher set") > Cc: stable@dpdk.org > > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com> > --- > drivers/net/mlx5/mlx5_flow_dv.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c > index 4c16281..893db3e 100644 > --- a/drivers/net/mlx5/mlx5_flow_dv.c > +++ b/drivers/net/mlx5/mlx5_flow_dv.c > @@ -5742,6 +5742,7 @@ struct field_modify_info modify_tcp[] = { > MLX5_ADDR_OF(fte_match_param, matcher, misc_parameters_2); > void *misc2_v = > MLX5_ADDR_OF(fte_match_param, key, misc_parameters_2); > + uint32_t temp; > > data &= mask; > switch (reg_type) { > @@ -5754,8 +5755,18 @@ struct field_modify_info modify_tcp[] = { > MLX5_SET(fte_match_set_misc2, misc2_v, metadata_reg_b, data); > break; > case REG_C_0: > - MLX5_SET(fte_match_set_misc2, misc2_m, metadata_reg_c_0, mask); > - MLX5_SET(fte_match_set_misc2, misc2_v, metadata_reg_c_0, data); > + /* > + * The metadata register C0 field might be divided into > + * source vport index and META item value, we should set > + * this field accorfing to specified mask, not as whole one. > + */ > + temp = MLX5_GET(fte_match_set_misc2, misc2_m, metadata_reg_c_0); > + temp |= mask; > + MLX5_SET(fte_match_set_misc2, misc2_m, metadata_reg_c_0, temp); > + temp = MLX5_GET(fte_match_set_misc2, misc2_v, metadata_reg_c_0); > + temp &= ~mask; > + temp |= data; > + MLX5_SET(fte_match_set_misc2, misc2_v, metadata_reg_c_0, temp); > break; > case REG_C_1: > MLX5_SET(fte_match_set_misc2, misc2_m, metadata_reg_c_1, mask); >
Matcher is flow table related structure providing the flow pattern to be translated directly in hardware controlling data. Some fields in this structure might be split (by software) between multiple items. For example, the metadata register c0 field in the matcher might be split into two independent subfields - the source vport index and META item value. These subfields have no permanent assigned masks, the actual configuration is queried from the kernel drivers in runtime. To handle source vport value (the port of e-Switch which is origin of the packet) the kernel might use the dedicated vport field in the matcher or the part of register c0 field, depending on configuration. To setup the matcher structure fields the macro MLX5_SET is used. MLX5_SET configures the specified 32-bit field as whole entity. For metadata register c0 we should take into account the provided mask in order to configure the specified subfield bits only, otherwise setting vport overrides the META values and vice versa. Fixes: acfcd5c52f94 ("net/mlx5: update meta register matcher set") Cc: stable@dpdk.org Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com> Acked-by: Matan Azrad <matan@mellanox.com> --- v1: - http://patches.dpdk.org/patch/64068/ v2: - update commit message - update headline "net/mlx5: fix matcher metadata register c0 field setup" drivers/net/mlx5/mlx5_flow_dv.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c index 5bd1b1c..2b70788 100644 --- a/drivers/net/mlx5/mlx5_flow_dv.c +++ b/drivers/net/mlx5/mlx5_flow_dv.c @@ -5748,6 +5748,7 @@ struct field_modify_info modify_tcp[] = { MLX5_ADDR_OF(fte_match_param, matcher, misc_parameters_2); void *misc2_v = MLX5_ADDR_OF(fte_match_param, key, misc_parameters_2); + uint32_t temp; data &= mask; switch (reg_type) { @@ -5760,8 +5761,18 @@ struct field_modify_info modify_tcp[] = { MLX5_SET(fte_match_set_misc2, misc2_v, metadata_reg_b, data); break; case REG_C_0: - MLX5_SET(fte_match_set_misc2, misc2_m, metadata_reg_c_0, mask); - MLX5_SET(fte_match_set_misc2, misc2_v, metadata_reg_c_0, data); + /* + * The metadata register C0 field might be divided into + * source vport index and META item value, we should set + * this field according to specified mask, not as whole one. + */ + temp = MLX5_GET(fte_match_set_misc2, misc2_m, metadata_reg_c_0); + temp |= mask; + MLX5_SET(fte_match_set_misc2, misc2_m, metadata_reg_c_0, temp); + temp = MLX5_GET(fte_match_set_misc2, misc2_v, metadata_reg_c_0); + temp &= ~mask; + temp |= data; + MLX5_SET(fte_match_set_misc2, misc2_v, metadata_reg_c_0, temp); break; case REG_C_1: MLX5_SET(fte_match_set_misc2, misc2_m, metadata_reg_c_1, mask); -- 1.8.3.1
Raslan, Ferruh In these patches (merged in mlx5 tree) just the commit messages are updated: http://patchwork.dpdk.org/patch/64831/ http://patchwork.dpdk.org/patch/64832/ http://patchwork.dpdk.org/patch/64852/ With best regards, Slava > -----Original Message----- > From: Ferruh Yigit <ferruh.yigit@intel.com> > Sent: Wednesday, January 8, 2020 16:47 > To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org > Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh > <rasland@mellanox.com>; Ori Kam <orika@mellanox.com>; stable@dpdk.org > Subject: Re: [dpdk-stable] [PATCH] net/mlx5: fix matcher metadata register c0 > field setup > > On 12/20/2019 7:48 AM, Viacheslav Ovsiienko wrote: > > The metadata register c0 field in the matcher might be split into two > > independent fields - the source vport index and META item value. These > > fields have no permanent assigned bits, the configuration is queried > > from the kernel drivers. > > > > MLX5_SET configures the specified 32-bit field as whole entity. > > For metadata register c0 we should take into account the provided mask > > in order to configure the specified subfield bits only. > > Hi Slava, > > Is there a more human friendly name for the field instead of "C0"? > > I don't know what "matcher metadata register" is, what is the impact of the > fix? > Which functionality was broken now fixed? Does it make sense to reflect it in > the patch title / commit log? > > Same comment for the related patches: > net/mlx5: fix register c0 usage for metadata entities > net/mlx5: fix metadata item endianness conversion
On 1/17/2020 11:01 AM, Viacheslav Ovsiienko wrote:
> Matcher is flow table related structure providing the flow pattern
> to be translated directly in hardware controlling data. Some fields
> in this structure might be split (by software) between multiple items.
>
> For example, the metadata register c0 field in the matcher might be
> split into two independent subfields - the source vport index and
> META item value. These subfields have no permanent assigned masks,
> the actual configuration is queried from the kernel drivers in
> runtime. To handle source vport value (the port of e-Switch which
> is origin of the packet) the kernel might use the dedicated vport
> field in the matcher or the part of register c0 field, depending
> on configuration.
>
> To setup the matcher structure fields the macro MLX5_SET is used.
> MLX5_SET configures the specified 32-bit field as whole entity.
> For metadata register c0 we should take into account the provided
> mask in order to configure the specified subfield bits only,
> otherwise setting vport overrides the META values and vice versa.
>
> Fixes: acfcd5c52f94 ("net/mlx5: update meta register matcher set")
> Cc: stable@dpdk.org
>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> Acked-by: Matan Azrad <matan@mellanox.com>
> ---
> v1: - http://patches.dpdk.org/patch/64068/
> v2: - update commit message
> - update headline
> "net/mlx5: fix matcher metadata register c0 field setup"
Applied to dpdk-next-net/master, thanks.