ethdev: simplify representor info get API

* Background on rte_eth_representor_info_get()

rte_eth_representor_info_get() experimental function was added to ethdev public API in 21.05 DPDK release [1].

Its purpose is to provide representor IDs in form of ID ranges. Driver can return the following example info:

	info = (struct rte_eth_representor_info){
		.controller = 0, /* Controller ID of caller device. */
		.pf = 0, /* Physical function ID of caller device. */
		.nb_ranges = 2,
		.ranges = {
			{
				.type = RTE_ETH_REPRESENTOR_VF,
				.controller = 0,
				.pf = 0,
				.vf = 0,
				.id_base = 100,
				.id_end = 199,
				.name = "pfvf"
			},
			{
				.type = RTE_ETH_REPRESENTOR_SF,
				.controller = 0,
				.pf = 0,
				.sf = 0,
				.id_base = 200,
				.id_end = 299,
				.name = "pfsf"
			},
		}
	};

This roughly means that, representors on PF 0 will have IDs:

- PF0VF0 -> ID == 100
- PF0VF1 -> ID == 101
- ...
- PF0VF99 -> ID == 199
- PF0SF0 -> ID == 200
- PF0SF1 -> ID == 201
- ...
- PF0SF99 -> ID == 299

All representors are related to controller with ID equal to 0.

Since the introduction, this API was implemented by 3 drivers:

- mlx5
- sfc
- cnxk

and resulting implementations suggest that some clean up is needed to address the following issues.

** Issue #1: All drivers return degenerate ranges

All 3 drivers implementing rte_eth_representor_info_get() return degenerate ranges i.e.:

- one struct rte_eth_representor_range is returned per representor
- each has id_base == id_end

Due to this, current range format seems to be an overkill.

** Issue #2: Representor ID is not directly usable by DPDK users

rte_eth_representor_info_get() is part of the public ethdev API. Applications can, for example, use it to answer a question:

What representor ID will have a representor of VF 3 on PF 0?

The problem is that applications have no clear way to correlate an ethdev port with a representor ID.

Representor ID itself is stored in rte_eth_dev_data.representor_id, but this field is not exposed to the user.
This field is used internally in ethdev to implement iteration over matching representors. This is exposed to applications through rte_eth_iterator and related macros.

Drivers implementing rte_eth_representor_info_get() reuse the same representor ID in rte_eth_switch_info.port_id. However whether these values should be treated the same way, is not part of the API contract.

Regardless of that, representor ID alone does not bring any useful information to the user.

* Proposal

To address the 2 issues above, I would like to propose simplification of rte_eth_representor_info_get():

- Function should return information about
what a given ethdev port represents:
- representor type
- PCI controller index
- PF index
- VF/SF index (for VF/SF type)
- Function will return information only for provided port.
- If driver does not provide representor_info_get() in its dev_ops, then ethdev library will handle it for the driver in the following way:

    # for representors
    type = RTE_ETH_REPRESENTOR_VF
    controller_valid = false
    PF_valid = false
    VF = dev_info.switch_info.port_id

    # for non-representors
    type = RTE_ETH_REPRESENTOR_NONE
    controller_valid = false
    PF_valid = false

This behavior would be consistent with how rte_eth_representor_id_get() calculates representor_id for each driver.
- Otherwise, driver is responsible for filling representor info.

As a result rte_eth_representor_info_get() would be implemented for all drivers.

Iteration over representors can be implemented
in terms of this new implementation by:

- iterating over ethdev ports with matching rte_device
- for ethdev ports which are representors
- convert devargs to rte_eth_representor_info output
- compare converted info with rte_eth_representor_info_get() output for port

Comparison mentioned above is provided by internal rte_eth_representor_info_match() function provided in this RFC.

Changes in eth_representor_cmp() showcase rough algorithm of that new logic.

This change would also deprecate rte_eth_representor_id_get() internal function.

[1]: https://inbox.dpdk.org/dev/[email protected]/

---
 lib/ethdev/ethdev_driver.c | 89 ++++++++++++++++++++++++++++++++++++++
 lib/ethdev/ethdev_driver.h |  4 ++
 lib/ethdev/rte_class_eth.c | 35 ++++++++++-----
 lib/ethdev/rte_ethdev.c    | 32 ++++++++++++--
 lib/ethdev/rte_ethdev.h    | 47 +++++---------------
 5 files changed, 157 insertions(+), 50 deletions(-)

index eab5c15d12..514c8fa29d 100644
@@ -902,6 +902,95 @@ rte_eth_recycle_rx_descriptors_refill_dummy(void *queue __rte_unused,
 	/* No action. */
 }
 
+static bool controller_match(const struct rte_eth_representor_info *rep_info,
+			     const struct rte_eth_representor_info *backer_info,
+			     const struct rte_eth_representor_info *other)
+{
+	/* If device has controller, but devargs do not have one, then use backer's controller as default. */
+	if (rep_info->controller_valid && !other->controller_valid) {
+		if (rep_info->controller != backer_info->controller)
+			return false;
+	}
+
+	/* If devargs have controller, but device does not have one, then no way to compare. */
+	if (!rep_info->controller_valid && other->controller_valid)
+		return false;
+
+	/* If both device and devargs have controller, then they must match. */
+	if (rep_info->controller_valid && other->controller_valid && rep_info->controller != other->controller)
+		return false;
+
+	/* If both device and devargs do not have controller, then no need to compare. */
+
+	return true;
+}
+
+static bool pf_match(const struct rte_eth_representor_info *rep_info,
+			     const struct rte_eth_representor_info *backer_info,
+			     const struct rte_eth_representor_info *other)
+{
+	/* If device has pf, but devargs do not have one, then use backer's pf as default. */
+	if (rep_info->pf_valid && !other->pf_valid) {
+		if (rep_info->pf != backer_info->pf)
+			return false;
+	}
+
+	/* If devargs have pf, but device does not have one, then no way to compare. */
+	if (!rep_info->pf_valid && other->pf_valid)
+		return false;
+
+	/* If both device and devargs have pf, then they must match. */
+	if (rep_info->pf_valid && other->pf_valid && rep_info->pf != other->pf)
+		return false;
+
+	/* If both device and devargs do not have pf, then no need to compare. */
+
+	return true;
+}
+
+RTE_EXPORT_INTERNAL_SYMBOL(rte_eth_representor_id_get)
+bool
+rte_eth_representor_info_match(const struct rte_eth_dev *dev, const struct rte_eth_representor_info *other)
+{
+	struct rte_eth_representor_info info;
+	struct rte_eth_representor_info backer_info;
+	int ret;
+
+	memset(&info, 0, sizeof(info));
+	memset(&backer_info, 0, sizeof(backer_info));
+
+	ret = rte_eth_representor_info_get(dev->data->port_id, &info);
+	if (ret < 0)
+		return false;
+	ret = rte_eth_representor_info_get(dev->data->backer_port_id, &backer_info);
+	if (ret < 0)
+		return false;
+
+	if (!controller_match(&info, &backer_info, other))
+		return false;
+
+	if (!pf_match(&info, &backer_info, other))
+		return false;
+
+	if (info.type != other->type)
+		return false;
+
+	switch (info.type) {
+	case RTE_ETH_REPRESENTOR_VF:
+		if (info.vf != other->vf)
+			return false;
+		break;
+	case RTE_ETH_REPRESENTOR_SF:
+		if (info.sf != other->sf)
+			return false;
+		break;
+	default:
+		break;
+	}
+
+	return true;
+}
+
 RTE_EXPORT_INTERNAL_SYMBOL(rte_eth_representor_id_get)
 int
 rte_eth_representor_id_get(uint16_t port_id,

index 0f336f9567..349b8bbc71 100644
@@ -2045,6 +2045,10 @@ struct rte_eth_devargs {
 	enum rte_eth_representor_type type; /* type of representor */
 };
 
+__rte_internal
+bool
+rte_eth_representor_info_match(const struct rte_eth_dev *dev, const struct rte_eth_representor_info *other);
+
 /**
  * PMD helper function to get representor ID from location detail.
  *

index a8d01e2595..8235ac1d28 100644
@@ -93,19 +93,34 @@ eth_representor_cmp(const char *key __rte_unused,
 
 	/* Return 0 if representor ID is matching one of the values. */
 	for (i = 0; i < nc * np * nf; ++i) {
+		struct rte_eth_representor_info info;
+
 		c = i / (np * nf);
 		p = (i / nf) % np;
 		f = i % nf;
-		if (rte_eth_representor_id_get(edev->data->backer_port_id,
-			eth_da.type,
-			eth_da.nb_mh_controllers == 0 ? -1 :
-					eth_da.mh_controllers[c],
-			eth_da.nb_ports == 0 ? -1 : eth_da.ports[p],
-			eth_da.nb_representor_ports == 0 ? -1 :
-					eth_da.representor_ports[f],
-			&id) < 0)
-			continue;
-		if (data->representor_id == id)
+
+		memset(&info, 0, sizeof(info));
+		info.type = eth_da.type;
+		if (eth_da.nb_mh_controllers > 0) {
+			info.controller = eth_da.mh_controllers[c];
+			info.controller_valid = true;
+		}
+		if (eth_da.nb_ports > 0) {
+			info.pf = eth_da.ports[p];
+			info.pf = true;
+		}
+		switch (info.type) {
+		case RTE_ETH_REPRESENTOR_VF:
+			info.vf = eth_da.representor_ports[f];
+			break;
+		case RTE_ETH_REPRESENTOR_SF:
+			info.sf = eth_da.representor_ports[f];
+			break;
+		default:
+			break;
+		}
+
+		if (rte_eth_representor_info_match(edev, &info))
 			return 0;
 	}
 	return -1; /* no match */

index 9efeaf77cb..5ec08810fd 100644
@@ -7155,17 +7155,41 @@ rte_eth_representor_info_get(uint16_t port_id,
 			     struct rte_eth_representor_info *info)
 {
 	struct rte_eth_dev *dev;
-	int ret;
+	int ret = 0;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
-	if (dev->dev_ops->representor_info_get == NULL)
-		return -ENOTSUP;
+	if (info == NULL)
+		return -EINVAL;
+
+	if (dev->dev_ops->representor_info_get == NULL) {
+		if (!rte_eth_dev_is_repr(dev)) {
+			info->type = RTE_ETH_REPRESENTOR_NONE;
+		} else {
+			struct rte_eth_dev_info dev_info = {};
+
+			ret = rte_eth_dev_info_get(port_id, &dev_info);
+			if (ret != 0) {
+				RTE_ETHDEV_LOG_LINE(ERR, "rte_eth_dev_info_get() failed for port %u",
+						port_id);
+				goto finish;
+			}
+
+			info->type = RTE_ETH_REPRESENTOR_VF;
+			info->vf = dev_info.switch_info.port_id;
+		}
+
+		info->controller_valid = false;
+		info->pf_valid = false;
+
+		goto finish;
+	}
+
 	ret = eth_err(port_id, dev->dev_ops->representor_info_get(dev, info));
 
+finish:
 	rte_eth_trace_representor_info_get(port_id, info, ret);
-
 	return ret;
 }
 

2.43.0

You know the API shouldn't have stayed experimental so long.
There is no good reason for API to stay experimental across multiple LTS releases!

The code would be much simpler if you use sentinel values rather than carrying around all the XX_valid flag values.

With int fields and -1 (or a named RTE_ETH_REPRESENTOR_UNSPEC),
the whole comparator collapses to something like:


static bool field_match(int a, int b)

{
	return a == -1 || b == -1 || a == b;
}

return field_match(info.controller, other->controller) &&

       field_match(info.pf, other->pf) &&
       info.type == other->type &&
       (info.type != RTE_ETH_REPRESENTOR_VF || field_match(info.vf, other->vf)) &&

Ack, I can apply that in the next version of the patches.

My only worry would be that mlx5 DPDK driver gets these values (controller numbers, VF/SF numbers) from phys_port_name exported through sysfs.
phys_port_name is based on devlink port attributes [1] with the following types:

- controller number - u32
- port number - u32
- Used for physical port representors in mlx5 DPDK driver. These are of type RTE_ETH_REPRESENTOR_PF.
- VF number - u16
- SF number - u32

Using int fields would not cover values
which can be returned by kernel.

What do you think about going with int64_t for these fields?

[1]: https://github.com/torvalds/linux/blob/v7.2/include/uapi/linux/devlink.h#L412
Relevant attributes are:
- DEVLINK_ATTR_PORT_CONTROLLER_NUMBER
- DEVLINK_ATTR_PORT_NUMBER
- DEVLINK_ATTR_PORT_PCI_PF_NUMBER
- DEVLINK_ATTR_PORT_PCI_VF_NUMBER
- DEVLINK_ATTR_PORT_PCI_SF_NUMBER