Re: [dpdk-dev] [PATCH v8 8/9] ethdev: representor iterator compare complete info

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 3/4/21 5:30 PM, Xueming Li wrote:
> The NIC can have multiple PCIe links and can be attached to multiple
> hosts, for example the same single NIC can be shared for multiple server
> units in the rack. On each PCIe link NIC can provide multiple PFs and
> VFs/SFs based on these ones. The full representor identifier consists of
> three indices - controller index, PF index, and VF or SF index (if any).
> 
> SR-IOV and SubFunction are created on top of PF. PF index is introduced
> because there might be multiple PFs in the bonding configuration and
> only bonding device is probed.
> 
> In eth representor comparator callback, ethdev representor ID was
> compared with devarg. Since controller index and PF index not compared,
> callback returned representor from other PF or controller.
> 
> This patch adds new API to get representor ID from controller, pf and
> vf/sf index. Representor comparer callback get representor ID then
> compare with device representor ID.
> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

One minor suggestions below to make it a bit more robust
against errors in PMDs.

Make thanks for careful review notes processing.

[snip]

> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 3d4ec8ad5c..2dc464a7ad 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -5609,6 +5609,94 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
>  	return result;
>  }
>  
> +int
> +rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
> +			   enum rte_eth_representor_type type,
> +			   int controller, int pf, int representor_port,
> +			   uint16_t *repr_id)
> +{
> +	int ret, n, i, count;
> +	struct rte_eth_representor_info *info = NULL;
> +	size_t size;
> +
> +	if (type == RTE_ETH_REPRESENTOR_NONE)
> +		return 0;
> +	if (repr_id == NULL)
> +		return -EINVAL;
> +
> +	/* Get PMD representor range info. */
> +	ret = rte_eth_representor_info_get(ethdev->data->port_id, NULL);
> +	if (ret < 0) {
> +		if (type == RTE_ETH_REPRESENTOR_VF && controller == -1 &&
> +		    pf == -1) {
> +			/* Direct mapping for legacy VF representor. */
> +			*repr_id = representor_port;
> +			return 0;
> +		} else {
> +			return ret;
> +		}
> +	}
> +	n = ret;
> +	size = sizeof(*info) + n * sizeof(info->ranges[0]);
> +	info = calloc(1, size);
> +	if (info == NULL)
> +		return -ENOMEM;
> +	ret = rte_eth_representor_info_get(ethdev->data->port_id, info);
> +	if (ret < 0)
> +		goto out;
> +
> +	/* Default controller and pf to caller. */
> +	if (controller == -1)
> +		controller = info->controller;
> +	if (pf == -1)
> +		pf = info->pf;
> +
> +	/* Locate representor ID. */
> +	ret = -ENOENT;
> +	for (i = 0; i < n; ++i) {
> +		if (info->ranges[i].type != type)
> +			continue;
> +		if (info->ranges[i].controller != controller)
> +			continue;
> +		count = info->ranges[i].id_end - info->ranges[i].id_base + 1;

I think it would be useful to sanity test these values to
ensure that we have no overflow here.
Let's check above, log error and skip the range if id_end is
smaller than id_base.

> +		switch (info->ranges[i].type) {
> +		case RTE_ETH_REPRESENTOR_PF:
> +			if (pf < info->ranges[i].pf ||
> +			    pf >= info->ranges[i].pf + count)
> +				continue;
> +			*repr_id = info->ranges[i].id_base +
> +				   (pf - info->ranges[i].pf);
> +			ret = 0;
> +			goto out;
> +		case RTE_ETH_REPRESENTOR_VF:
> +			if (info->ranges[i].pf != pf)
> +				continue;
> +			if (representor_port < info->ranges[i].vf ||
> +			    representor_port >= info->ranges[i].vf + count)
> +				continue;
> +			*repr_id = info->ranges[i].id_base +
> +				   (representor_port - info->ranges[i].vf);
> +			ret = 0;
> +			goto out;
> +		case RTE_ETH_REPRESENTOR_SF:
> +			if (info->ranges[i].pf != pf)
> +				continue;
> +			if (representor_port < info->ranges[i].sf ||
> +			    representor_port >= info->ranges[i].sf + count)
> +				continue;
> +			*repr_id = info->ranges[i].id_base +
> +			      (representor_port - info->ranges[i].sf);
> +			ret = 0;
> +			goto out;
> +		default:
> +			break;
> +		}
> +	}
> +out:
> +	free(info);
> +	return ret;
> +}
> +
>  static int
>  eth_dev_handle_port_list(const char *cmd __rte_unused,
>  		const char *params __rte_unused,

[snip]