[PATCH 1/3] net/mlx5: fix validation for conntrack indirect action

[PATCH 1/3] net/mlx5: fix validation for conntrack indirect action

[Alexander Kozyrev]

From: Bing Zhao <bingz@nvidia.com>

After rte_flow_shared_action_* API was replaced with
rte_flow_action_handle_* API, one input parameter of the update
interface was also changed. A generic pointer was used instead of the
"const struct rte_flow_action *" pointer.

In the entrance of mlx5 PMD callback for update, the validation is
called for all indirect actions. But for conntrack type, the pointer
is no longer with rte_flow_action pointer type and it will cause an
incorrect casting and error.

The content for updating should only be validated when needed. It
should skip the validation in the entrance. Right now, the content
was already added before updating the hardware by WQE. So the type
of the indirect action should be checked before calling the action
validate function.

When creating a new conntrack object, the validation is still needed
since all the content will be used to update the hardware context.

Fixes: 40327aab1491 ("ethdev: introduce indirect flow action")

Signed-off-by: Bing Zhao <bingz@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 4b567f61dd..9bd92f28fd 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -10753,9 +10753,18 @@ mlx5_action_handle_update(struct rte_eth_dev *dev,
 	const struct mlx5_flow_driver_ops *fops =
 			flow_get_drv_ops(flow_get_drv_type(dev, &attr));
 	int ret;
+	uint32_t act_idx = (uint32_t)(uintptr_t)handle;
+	uint32_t type = act_idx >> MLX5_INDIRECT_ACTION_TYPE_OFFSET;
 
-	ret = flow_drv_action_validate(dev, NULL,
-			(const struct rte_flow_action *)update, fops, error);
+	switch (type) {
+	case MLX5_INDIRECT_ACTION_TYPE_CT:
+		ret = 0;
+		break;
+	default:
+		ret = flow_drv_action_validate(dev, NULL,
+				(const struct rte_flow_action *)update,
+				fops, error);
+	}
 	if (ret)
 		return ret;
 	return flow_drv_action_update(dev, handle, update, fops,
-- 
2.18.2

[PATCH 2/3] net/mlx5: fix handle validation for meter mark

[Alexander Kozyrev]

Skip the METER_MARK validation for the indirect action update.
The old synchronous indirect action update was left out
during the METER_MARK implementation in favor of the async way.
Allow the sync method of doing this with relaxed validation.

Fixes: 48fbb0e93d06 ("net/mlx5: support flow meter mark indirect action with HWS")

Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 9bd92f28fd..e91eb636d0 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -10758,6 +10758,7 @@ mlx5_action_handle_update(struct rte_eth_dev *dev,
 
 	switch (type) {
 	case MLX5_INDIRECT_ACTION_TYPE_CT:
+	case MLX5_INDIRECT_ACTION_TYPE_METER_MARK:
 		ret = 0;
 		break;
 	default:
-- 
2.18.2

[PATCH 3/3] app/testpmd: fix meter mark handle update

[Alexander Kozyrev]

The indirect action handle update for the METER_MERK action
was implemented only for the async RTE Flow API.
Allow updating the METER_MARK parameters via the old sync method.

Fixes: 9c4a0c1859a3 ("ethdev: add meter color mark flow action")

Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
---
 app/test-pmd/config.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index c4784b7f2c..58ac1576b7 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1943,6 +1943,7 @@ port_action_handle_update(portid_t port_id, uint32_t id,
 	struct rte_flow_error error;
 	struct rte_flow_action_handle *action_handle;
 	struct port_indirect_action *pia;
+	struct rte_flow_update_meter_mark mtr_update;
 	const void *update;
 
 	action_handle = port_action_handle_get_by_id(port_id, id);
@@ -1956,6 +1957,17 @@ port_action_handle_update(portid_t port_id, uint32_t id,
 	case RTE_FLOW_ACTION_TYPE_CONNTRACK:
 		update = action->conf;
 		break;
+	case RTE_FLOW_ACTION_TYPE_METER_MARK:
+		rte_memcpy(&mtr_update.meter_mark, action->conf,
+			sizeof(struct rte_flow_action_meter_mark));
+		if (mtr_update.meter_mark.profile)
+			mtr_update.profile_valid = 1;
+		if (mtr_update.meter_mark.policy)
+			mtr_update.policy_valid = 1;
+		mtr_update.color_mode_valid = 1;
+		mtr_update.state_valid = 1;
+		update = &mtr_update;
+		break;
 	default:
 		update = action;
 		break;
@@ -3162,8 +3174,10 @@ port_queue_action_handle_update(portid_t port_id,
 	case RTE_FLOW_ACTION_TYPE_METER_MARK:
 		rte_memcpy(&mtr_update.meter_mark, action->conf,
 			sizeof(struct rte_flow_action_meter_mark));
-		mtr_update.profile_valid = 1;
-		mtr_update.policy_valid = 1;
+		if (mtr_update.meter_mark.profile)
+			mtr_update.profile_valid = 1;
+		if (mtr_update.meter_mark.policy)
+			mtr_update.policy_valid = 1;
 		mtr_update.color_mode_valid = 1;
 		mtr_update.init_color_valid = 1;
 		mtr_update.state_valid = 1;
-- 
2.18.2

Re: [PATCH 3/3] app/testpmd: fix meter mark handle update

[Stephen Hemminger]

On Tue, 11 Jul 2023 18:20:44 +0300
Alexander Kozyrev <akozyrev@nvidia.com> wrote:

> +	case RTE_FLOW_ACTION_TYPE_METER_MARK:
> +		rte_memcpy(&mtr_update.meter_mark, action->conf,
> +			sizeof(struct rte_flow_action_meter_mark));

Why use rte_memcpy? memcpy of fixed size is faster.
Also, more static checkers know what memcpy is.

RE: [PATCH 1/3] net/mlx5: fix validation for conntrack indirect action

[Suanming Mou]

Hi,

> -----Original Message-----
> From: Alexander Kozyrev <akozyrev@nvidia.com>
> Sent: Tuesday, July 11, 2023 11:21 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>; Bing Zhao
> <bingz@nvidia.com>
> Subject: [PATCH 1/3] net/mlx5: fix validation for conntrack indirect action
> 
> From: Bing Zhao <bingz@nvidia.com>
> 
> After rte_flow_shared_action_* API was replaced with
> rte_flow_action_handle_* API, one input parameter of the update interface was
> also changed. A generic pointer was used instead of the "const struct
> rte_flow_action *" pointer.
> 
> In the entrance of mlx5 PMD callback for update, the validation is called for all
> indirect actions. But for conntrack type, the pointer is no longer with
> rte_flow_action pointer type and it will cause an incorrect casting and error.
> 
> The content for updating should only be validated when needed. It should skip the
> validation in the entrance. Right now, the content was already added before
> updating the hardware by WQE. So the type of the indirect action should be
> checked before calling the action validate function.
> 
> When creating a new conntrack object, the validation is still needed since all the
> content will be used to update the hardware context.
> 
> Fixes: 40327aab1491 ("ethdev: introduce indirect flow action")
> 
> Signed-off-by: Bing Zhao <bingz@nvidia.com>

Acked-by: Suanming Mou <suanmingm@nvidia.com>

RE: [PATCH 2/3] net/mlx5: fix handle validation for meter mark

[Suanming Mou]

Hi,

> -----Original Message-----
> From: Alexander Kozyrev <akozyrev@nvidia.com>
> Sent: Tuesday, July 11, 2023 11:21 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>
> Subject: [PATCH 2/3] net/mlx5: fix handle validation for meter mark
> 
> Skip the METER_MARK validation for the indirect action update.
> The old synchronous indirect action update was left out during the METER_MARK
> implementation in favor of the async way.
> Allow the sync method of doing this with relaxed validation.
> 
> Fixes: 48fbb0e93d06 ("net/mlx5: support flow meter mark indirect action with
> HWS")
> 
> Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>

Acked-by: Suanming Mou <suanmingm@nvidia.com>

Thanks.

RE: [PATCH 3/3] app/testpmd: fix meter mark handle update

[Alexander Kozyrev]

> On Tuesday, July 11, 2023 12:15 PM
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> On Tue, 11 Jul 2023 18:20:44 +0300
> Alexander Kozyrev <akozyrev@nvidia.com> wrote:
> 
> > +	case RTE_FLOW_ACTION_TYPE_METER_MARK:
> > +		rte_memcpy(&mtr_update.meter_mark, action->conf,
> > +			sizeof(struct rte_flow_action_meter_mark));
> 
> Why use rte_memcpy? memcpy of fixed size is faster.
> Also, more static checkers know what memcpy is.

Ok, not a problem, switching to memcpy.

[PATCH v2 1/3] net/mlx5: fix validation for conntrack indirect action

[Alexander Kozyrev]

From: Bing Zhao <bingz@nvidia.com>

After rte_flow_shared_action_* API was replaced with
rte_flow_action_handle_* API, one input parameter of the update
interface was also changed. A generic pointer was used instead of the
"const struct rte_flow_action *" pointer.

In the entrance of mlx5 PMD callback for update, the validation is
called for all indirect actions. But for conntrack type, the pointer
is no longer with rte_flow_action pointer type and it will cause an
incorrect casting and error.

The content for updating should only be validated when needed. It
should skip the validation in the entrance. Right now, the content
was already added before updating the hardware by WQE. So the type
of the indirect action should be checked before calling the action
validate function.

When creating a new conntrack object, the validation is still needed
since all the content will be used to update the hardware context.

Fixes: 40327aab1491 ("ethdev: introduce indirect flow action")

Signed-off-by: Bing Zhao <bingz@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 4b567f61dd..9bd92f28fd 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -10753,9 +10753,18 @@ mlx5_action_handle_update(struct rte_eth_dev *dev,
 	const struct mlx5_flow_driver_ops *fops =
 			flow_get_drv_ops(flow_get_drv_type(dev, &attr));
 	int ret;
+	uint32_t act_idx = (uint32_t)(uintptr_t)handle;
+	uint32_t type = act_idx >> MLX5_INDIRECT_ACTION_TYPE_OFFSET;
 
-	ret = flow_drv_action_validate(dev, NULL,
-			(const struct rte_flow_action *)update, fops, error);
+	switch (type) {
+	case MLX5_INDIRECT_ACTION_TYPE_CT:
+		ret = 0;
+		break;
+	default:
+		ret = flow_drv_action_validate(dev, NULL,
+				(const struct rte_flow_action *)update,
+				fops, error);
+	}
 	if (ret)
 		return ret;
 	return flow_drv_action_update(dev, handle, update, fops,
-- 
2.18.2

[PATCH v2 2/3] net/mlx5: fix handle validation for meter mark

[Alexander Kozyrev]

Skip the METER_MARK validation for the indirect action update.
The old synchronous indirect action update was left out
during the METER_MARK implementation in favor of the async way.
Allow the sync method of doing this with relaxed validation.

Fixes: 48fbb0e93d06 ("net/mlx5: support flow meter mark indirect action with HWS")

Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 9bd92f28fd..e91eb636d0 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -10758,6 +10758,7 @@ mlx5_action_handle_update(struct rte_eth_dev *dev,
 
 	switch (type) {
 	case MLX5_INDIRECT_ACTION_TYPE_CT:
+	case MLX5_INDIRECT_ACTION_TYPE_METER_MARK:
 		ret = 0;
 		break;
 	default:
-- 
2.18.2

[PATCH v2 3/3] app/testpmd: fix meter mark handle update

[Alexander Kozyrev]

The indirect action handle update for the METER_MERK action
was implemented only for the async RTE Flow API.
Allow updating the METER_MARK parameters via the old sync method.

Fixes: 9c4a0c1859a3 ("ethdev: add meter color mark flow action")

Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
---
 app/test-pmd/config.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index c4784b7f2c..11f3a22048 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1943,6 +1943,7 @@ port_action_handle_update(portid_t port_id, uint32_t id,
 	struct rte_flow_error error;
 	struct rte_flow_action_handle *action_handle;
 	struct port_indirect_action *pia;
+	struct rte_flow_update_meter_mark mtr_update;
 	const void *update;
 
 	action_handle = port_action_handle_get_by_id(port_id, id);
@@ -1956,6 +1957,17 @@ port_action_handle_update(portid_t port_id, uint32_t id,
 	case RTE_FLOW_ACTION_TYPE_CONNTRACK:
 		update = action->conf;
 		break;
+	case RTE_FLOW_ACTION_TYPE_METER_MARK:
+		memcpy(&mtr_update.meter_mark, action->conf,
+		       sizeof(struct rte_flow_action_meter_mark));
+		if (mtr_update.meter_mark.profile)
+			mtr_update.profile_valid = 1;
+		if (mtr_update.meter_mark.policy)
+			mtr_update.policy_valid = 1;
+		mtr_update.color_mode_valid = 1;
+		mtr_update.state_valid = 1;
+		update = &mtr_update;
+		break;
 	default:
 		update = action;
 		break;
@@ -3162,8 +3174,10 @@ port_queue_action_handle_update(portid_t port_id,
 	case RTE_FLOW_ACTION_TYPE_METER_MARK:
 		rte_memcpy(&mtr_update.meter_mark, action->conf,
 			sizeof(struct rte_flow_action_meter_mark));
-		mtr_update.profile_valid = 1;
-		mtr_update.policy_valid = 1;
+		if (mtr_update.meter_mark.profile)
+			mtr_update.profile_valid = 1;
+		if (mtr_update.meter_mark.policy)
+			mtr_update.policy_valid = 1;
 		mtr_update.color_mode_valid = 1;
 		mtr_update.init_color_valid = 1;
 		mtr_update.state_valid = 1;
-- 
2.18.2

RE: [PATCH v2 1/3] net/mlx5: fix validation for conntrack indirect action

[Ori Kam]

> -----Original Message-----
> From: Alexander Kozyrev <akozyrev@nvidia.com>
> Sent: Tuesday, July 18, 2023 4:32 PM
> 
> From: Bing Zhao <bingz@nvidia.com>
> 
> After rte_flow_shared_action_* API was replaced with
> rte_flow_action_handle_* API, one input parameter of the update
> interface was also changed. A generic pointer was used instead of the
> "const struct rte_flow_action *" pointer.
> 
> In the entrance of mlx5 PMD callback for update, the validation is
> called for all indirect actions. But for conntrack type, the pointer
> is no longer with rte_flow_action pointer type and it will cause an
> incorrect casting and error.
> 
> The content for updating should only be validated when needed. It
> should skip the validation in the entrance. Right now, the content
> was already added before updating the hardware by WQE. So the type
> of the indirect action should be checked before calling the action
> validate function.
> 
> When creating a new conntrack object, the validation is still needed
> since all the content will be used to update the hardware context.
> 
> Fixes: 40327aab1491 ("ethdev: introduce indirect flow action")
> 
> Signed-off-by: Bing Zhao <bingz@nvidia.com>
> ---

Acked-by: Ori Kam <orika@nvidia.com>
Best,
Ori

RE: [PATCH v2 2/3] net/mlx5: fix handle validation for meter mark

[Ori Kam]

> -----Original Message-----
> From: Alexander Kozyrev <akozyrev@nvidia.com>
> Sent: Tuesday, July 18, 2023 4:32 PM
> 
> Skip the METER_MARK validation for the indirect action update.
> The old synchronous indirect action update was left out
> during the METER_MARK implementation in favor of the async way.
> Allow the sync method of doing this with relaxed validation.
> 
> Fixes: 48fbb0e93d06 ("net/mlx5: support flow meter mark indirect action with
> HWS")
> 
> Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> ---
>  drivers/net/mlx5/mlx5_flow.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 9bd92f28fd..e91eb636d0 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -10758,6 +10758,7 @@ mlx5_action_handle_update(struct rte_eth_dev
> *dev,
> 
>  	switch (type) {
>  	case MLX5_INDIRECT_ACTION_TYPE_CT:
> +	case MLX5_INDIRECT_ACTION_TYPE_METER_MARK:
>  		ret = 0;
>  		break;
>  	default:
> --
> 2.18.2

Acked-by: Ori Kam <orika@nvidia.com>
Best,
Ori

RE: [PATCH v2 3/3] app/testpmd: fix meter mark handle update

[Ori Kam]

> -----Original Message-----
> From: Alexander Kozyrev <akozyrev@nvidia.com>
> Sent: Tuesday, July 18, 2023 4:32 PM
> 
> The indirect action handle update for the METER_MERK action
> was implemented only for the async RTE Flow API.
> Allow updating the METER_MARK parameters via the old sync method.
> 
> Fixes: 9c4a0c1859a3 ("ethdev: add meter color mark flow action")
> 
> Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> ---

Acked-by: Ori Kam <orika@nvidia.com>
Best,
Ori

RE: [PATCH v2 1/3] net/mlx5: fix validation for conntrack indirect action

[Raslan Darawsheh]

Hi,

> -----Original Message-----
> From: Alexander Kozyrev <akozyrev@nvidia.com>
> Sent: Tuesday, July 18, 2023 4:32 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Slava
> Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>;
> stephen@networkplumber.org; Bing Zhao <bingz@nvidia.com>
> Subject: [PATCH v2 1/3] net/mlx5: fix validation for conntrack indirect action
> 
> From: Bing Zhao <bingz@nvidia.com>
> 
> After rte_flow_shared_action_* API was replaced with
> rte_flow_action_handle_* API, one input parameter of the update interface
> was also changed. A generic pointer was used instead of the "const struct
> rte_flow_action *" pointer.
> 
> In the entrance of mlx5 PMD callback for update, the validation is called for all
> indirect actions. But for conntrack type, the pointer is no longer with
> rte_flow_action pointer type and it will cause an incorrect casting and error.
> 
> The content for updating should only be validated when needed. It should skip
> the validation in the entrance. Right now, the content was already added
> before updating the hardware by WQE. So the type of the indirect action
> should be checked before calling the action validate function.
> 
> When creating a new conntrack object, the validation is still needed since all
> the content will be used to update the hardware context.
> 
> Fixes: 40327aab1491 ("ethdev: introduce indirect flow action")
> 
> Signed-off-by: Bing Zhao <bingz@nvidia.com>

Series applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh