[dpdk-dev] [PATCH 1/3] common/sfc_efx/base: fix MAE match spec validation helper

[dpdk-dev] [PATCH 1/3] common/sfc_efx/base: fix MAE match spec validation helper

[Ivan Malov]

A particular FW version is aware of some set of match fields.
Depending on FW configuration and match specification type, a
known field may not necessarily be allowed to have a non-zero
mask. FW communicates such restrictions via field capabilities
MCDI. Newer FW may be aware of more fields. For such fields,
older FW simply does not report any capabilities.

A situation may occur when libefx is aware of a match field
which the FW is unaware of (eg., older FW), that is, FW does
not report capability status for this field. In this case,
libefx must consider such field as unsupported and demand
all-zeros mask for it when validating match specifications.

Currently, the helper in question simply exits and reports
that the specification is valid when it encounters a field
with no capability status available. This is clearly wrong.

Introduce the missing check to fix the problem.

Fixes: 34285fd0891d ("common/sfc_efx/base: add match spec validate API")
Cc: stable@dpdk.org

Reviewed-by: Andy Moreton <amoreton@xilinx.com>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
---
 drivers/common/sfc_efx/base/efx_mae.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/common/sfc_efx/base/efx_mae.c b/drivers/common/sfc_efx/base/efx_mae.c
index a54d5f6e6..ef15deb10 100644
--- a/drivers/common/sfc_efx/base/efx_mae.c
+++ b/drivers/common/sfc_efx/base/efx_mae.c
@@ -885,8 +885,17 @@ efx_mae_match_spec_is_valid(
 		if (m_size == 0)
 			continue; /* Skip array gap */
 
-		if ((unsigned int)field_cap_id >= field_ncaps)
-			break;
+		if ((unsigned int)field_cap_id >= field_ncaps) {
+			/*
+			 * The FW has not reported capability status for
+			 * this field. Make sure that its mask is zeroed.
+			 */
+			is_valid = efx_mask_is_all_zeros(m_size, m_buf);
+			if (is_valid != B_FALSE)
+				continue;
+			else
+				break;
+		}
 
 		switch (field_caps[field_cap_id].emfc_support) {
 		case MAE_FIELD_SUPPORTED_MATCH_MASK:
-- 
2.20.1

[dpdk-dev] [PATCH 2/3] common/sfc_efx/base: fix MAE match spec class comparison API

[Ivan Malov]

The helper exits once it encounters a field which hasn't its
capability status reported by the FW. Handle the corner case
when the two mask-value pairs match for the field, which, in
the absence of capability information, is sufficient to deem
the class unaffected by the field. Explain this in a comment.

Fixes: bb71f7e0a35a ("common/sfc_efx/base: add match specs class comparison API")
Cc: stable@dpdk.org

Reviewed-by: Andy Moreton <amoreton@xilinx.com>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
---
 drivers/common/sfc_efx/base/efx.h     |  5 ++++
 drivers/common/sfc_efx/base/efx_mae.c | 34 +++++++++++++++++----------
 2 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/common/sfc_efx/base/efx.h b/drivers/common/sfc_efx/base/efx.h
index 3b40e28b4..ccf9c7ab8 100644
--- a/drivers/common/sfc_efx/base/efx.h
+++ b/drivers/common/sfc_efx/base/efx.h
@@ -4283,6 +4283,11 @@ efx_mae_action_set_specs_equal(
  * Conduct a comparison to check whether two match specifications
  * of equal rule type (action / outer) and priority would map to
  * the very same rule class from the firmware's standpoint.
+ *
+ * For match specification fields that are not supported by firmware,
+ * the rule class only matches if the mask/value pairs for that field
+ * are equal. Clients should use efx_mae_match_spec_is_valid() before
+ * calling this API to detect usage of unsupported fields.
  */
 LIBEFX_API
 extern	__checkReturn			efx_rc_t
diff --git a/drivers/common/sfc_efx/base/efx_mae.c b/drivers/common/sfc_efx/base/efx_mae.c
index ef15deb10..c1717d7b0 100644
--- a/drivers/common/sfc_efx/base/efx_mae.c
+++ b/drivers/common/sfc_efx/base/efx_mae.c
@@ -1408,18 +1408,32 @@ efx_mae_match_specs_class_cmp(
 	     ++field_id) {
 		const efx_mae_mv_desc_t *descp = &desc_setp[field_id];
 		efx_mae_field_cap_id_t field_cap_id = descp->emmd_field_cap_id;
-
-		if (descp->emmd_mask_size == 0)
+		const uint8_t *lmaskp = mvpl + descp->emmd_mask_offset;
+		const uint8_t *rmaskp = mvpr + descp->emmd_mask_offset;
+		size_t mask_size = descp->emmd_mask_size;
+		const uint8_t *lvalp = mvpl + descp->emmd_value_offset;
+		const uint8_t *rvalp = mvpr + descp->emmd_value_offset;
+		size_t value_size = descp->emmd_value_size;
+
+		if (mask_size == 0)
 			continue; /* Skip array gap */
 
-		if ((unsigned int)field_cap_id >= field_ncaps)
-			break;
+		if ((unsigned int)field_cap_id >= field_ncaps) {
+			/*
+			 * The FW has not reported capability status for this
+			 * field. It's unknown whether any difference between
+			 * the two masks / values affects the class. The only
+			 * case when the class must be the same is when these
+			 * mask-value pairs match. Otherwise, report mismatch.
+			 */
+			if ((memcmp(lmaskp, rmaskp, mask_size) == 0) &&
+			    (memcmp(lvalp, rvalp, value_size) == 0))
+				continue;
+			else
+				break;
+		}
 
 		if (field_caps[field_cap_id].emfc_mask_affects_class) {
-			const uint8_t *lmaskp = mvpl + descp->emmd_mask_offset;
-			const uint8_t *rmaskp = mvpr + descp->emmd_mask_offset;
-			size_t mask_size = descp->emmd_mask_size;
-
 			if (memcmp(lmaskp, rmaskp, mask_size) != 0) {
 				have_same_class = B_FALSE;
 				break;
@@ -1427,10 +1441,6 @@ efx_mae_match_specs_class_cmp(
 		}
 
 		if (field_caps[field_cap_id].emfc_match_affects_class) {
-			const uint8_t *lvalp = mvpl + descp->emmd_value_offset;
-			const uint8_t *rvalp = mvpr + descp->emmd_value_offset;
-			size_t value_size = descp->emmd_value_size;
-
 			if (memcmp(lvalp, rvalp, value_size) != 0) {
 				have_same_class = B_FALSE;
 				break;
-- 
2.20.1

[dpdk-dev] [PATCH 3/3] common/sfc_efx/base: enhance field ID check in field set API

[Ivan Malov]

A field ID passed to the API may point to a gap in the array
of field descriptors. Turn down such invocations as improper.

Fixes: 370ed675a952 ("common/sfc_efx/base: support setting PPORT in match spec")
Cc: stable@dpdk.org

Reviewed-by: Andy Moreton <amoreton@xilinx.com>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
---
 drivers/common/sfc_efx/base/efx_mae.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/common/sfc_efx/base/efx_mae.c b/drivers/common/sfc_efx/base/efx_mae.c
index c1717d7b0..f4a529f5a 100644
--- a/drivers/common/sfc_efx/base/efx_mae.c
+++ b/drivers/common/sfc_efx/base/efx_mae.c
@@ -678,16 +678,22 @@ efx_mae_match_spec_field_set(
 		goto fail2;
 	}
 
-	if (value_size != descp->emmd_value_size) {
+	if (descp->emmd_mask_size == 0) {
+		/* The ID points to a gap in the array of field descriptors. */
 		rc = EINVAL;
 		goto fail3;
 	}
 
-	if (mask_size != descp->emmd_mask_size) {
+	if (value_size != descp->emmd_value_size) {
 		rc = EINVAL;
 		goto fail4;
 	}
 
+	if (mask_size != descp->emmd_mask_size) {
+		rc = EINVAL;
+		goto fail5;
+	}
+
 	if (descp->emmd_endianness == EFX_MAE_FIELD_BE) {
 		/*
 		 * The mask/value are in network (big endian) order.
@@ -729,6 +735,8 @@ efx_mae_match_spec_field_set(
 
 	return (0);
 
+fail5:
+	EFSYS_PROBE(fail5);
 fail4:
 	EFSYS_PROBE(fail4);
 fail3:
-- 
2.20.1

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/3] common/sfc_efx/base: fix MAE match spec validation helper

[Ferruh Yigit]

On 1/6/2021 10:05 AM, Ivan Malov wrote:
> A particular FW version is aware of some set of match fields.
> Depending on FW configuration and match specification type, a
> known field may not necessarily be allowed to have a non-zero
> mask. FW communicates such restrictions via field capabilities
> MCDI. Newer FW may be aware of more fields. For such fields,
> older FW simply does not report any capabilities.
> 
> A situation may occur when libefx is aware of a match field
> which the FW is unaware of (eg., older FW), that is, FW does
> not report capability status for this field. In this case,
> libefx must consider such field as unsupported and demand
> all-zeros mask for it when validating match specifications.
> 
> Currently, the helper in question simply exits and reports
> that the specification is valid when it encounters a field
> with no capability status available. This is clearly wrong.
> 
> Introduce the missing check to fix the problem.
> 
> Fixes: 34285fd0891d ("common/sfc_efx/base: add match spec validate API")
> Cc: stable@dpdk.org
> 
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>

Series applied to dpdk-next-net/main, thanks.