[v8,8/9] ethdev: representor iterator compare complete info
Checks
Commit Message
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>
---
lib/librte_ethdev/ethdev_driver.h | 32 +++++++++++
lib/librte_ethdev/rte_class_eth.c | 38 +++++++++----
lib/librte_ethdev/rte_ethdev.c | 88 +++++++++++++++++++++++++++++++
lib/librte_ethdev/version.map | 1 +
4 files changed, 150 insertions(+), 9 deletions(-)
Comments
On 3/4/2021 2: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>
<...>
> +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) {
This seems to support the legacy format, should the return value checked
explicitly against the '-ENOTSUP', instead of any error?
> + 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;
> + }
> + }
<...>
>-----Original Message-----
>From: Ferruh Yigit <ferruh.yigit@intel.com>
>Sent: Tuesday, March 9, 2021 12:19 AM
>To: Xueming(Steven) Li <xuemingl@nvidia.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf Penso <asafp@nvidia.com>; NBU-Contact-Thomas Monjalon
><thomas@monjalon.net>; Ray Kinsella <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>
>Subject: Re: [PATCH v8 8/9] ethdev: representor iterator compare complete info
>
>On 3/4/2021 2: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>
>
><...>
>
>> +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) {
>
>This seems to support the legacy format, should the return value checked explicitly against the '-ENOTSUP', instead of any error?
Good suggestion, thanks
>
>> + 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;
>> + }
>> + }
>
>
><...>
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]
>-----Original Message-----
>From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>Sent: Tuesday, March 9, 2021 4:20 PM
>To: Xueming(Steven) Li <xuemingl@nvidia.com>
>Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf Penso <asafp@nvidia.com>; NBU-Contact-Thomas Monjalon
><thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Ray Kinsella <mdr@ashroe.eu>; Neil Horman
><nhorman@tuxdriver.com>
>Subject: Re: [PATCH v8 8/9] ethdev: representor iterator compare complete info
>
>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.
Many thanks to your generous and careful review all the time, with patience :)
This is my first long public api patchset, can't do it without your help :)
>
>[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]
@@ -1244,6 +1244,38 @@ struct rte_eth_devargs {
enum rte_eth_representor_type type; /* type of representor */
};
+/**
+ * PMD helper function to get representor ID from location detail.
+ *
+ * Get representor ID from controller, pf and (sf or vf).
+ * The mapping is retrieved from rte_eth_representor_info_get().
+ *
+ * For backward compatibility, if no representor info, direct
+ * map legacy VF (no controller and pf).
+ *
+ * @param ethdev
+ * Handle of ethdev port.
+ * @param type
+ * Representor type.
+ * @param controller
+ * Controller ID, -1 if unspecified.
+ * @param pf
+ * PF port ID, -1 if unspecified.
+ * @param representor_port
+ * VF or SF representor port number, -1 if unspecified.
+ * @param repr_id
+ * Pointer to output representor ID.
+ *
+ * @return
+ * Negative errno value on error, 0 on success.
+ */
+__rte_internal
+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);
+
/**
* PMD helper function to parse ethdev arguments
*
@@ -65,9 +65,10 @@ eth_representor_cmp(const char *key __rte_unused,
{
int ret;
char *values;
- const struct rte_eth_dev_data *data = opaque;
- struct rte_eth_devargs representors;
- uint16_t index;
+ const struct rte_eth_dev *edev = opaque;
+ const struct rte_eth_dev_data *data = edev->data;
+ struct rte_eth_devargs eth_da;
+ uint16_t id, nc, np, nf, i, c, p, f;
if ((data->dev_flags & RTE_ETH_DEV_REPRESENTOR) == 0)
return -1; /* not a representor port */
@@ -76,17 +77,36 @@ eth_representor_cmp(const char *key __rte_unused,
values = strdup(value);
if (values == NULL)
return -1;
- memset(&representors, 0, sizeof(representors));
- ret = rte_eth_devargs_parse_representor_ports(values, &representors);
+ memset(ð_da, 0, sizeof(eth_da));
+ ret = rte_eth_devargs_parse_representor_ports(values, ð_da);
free(values);
if (ret != 0)
return -1; /* invalid devargs value */
+ if (eth_da.nb_mh_controllers == 0 && eth_da.nb_ports == 0 &&
+ eth_da.nb_representor_ports == 0)
+ return -1;
+ nc = eth_da.nb_mh_controllers > 0 ? eth_da.nb_mh_controllers : 1;
+ np = eth_da.nb_ports > 0 ? eth_da.nb_ports : 1;
+ nf = eth_da.nb_representor_ports > 0 ? eth_da.nb_representor_ports : 1;
+
/* Return 0 if representor id is matching one of the values. */
- for (index = 0; index < representors.nb_representor_ports; index++)
- if (data->representor_id ==
- representors.representor_ports[index])
+ for (i = 0; i < nc * np * nf; ++i) {
+ c = i / (np * nf);
+ p = (i / nf) % np;
+ f = i % nf;
+ if (rte_eth_representor_id_get(edev,
+ 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)
return 0;
+ }
return -1; /* no match */
}
@@ -112,7 +132,7 @@ eth_dev_match(const struct rte_eth_dev *edev,
ret = rte_kvargs_process(kvlist,
eth_params_keys[RTE_ETH_PARAM_REPRESENTOR],
- eth_representor_cmp, edev->data);
+ eth_representor_cmp, (void *)(uintptr_t)edev);
if (ret != 0)
return -1;
/* search for representor key */
@@ -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;
+ 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,
@@ -268,6 +268,7 @@ INTERNAL {
rte_eth_hairpin_queue_peer_bind;
rte_eth_hairpin_queue_peer_unbind;
rte_eth_hairpin_queue_peer_update;
+ rte_eth_representor_id_get;
rte_eth_switch_domain_alloc;
rte_eth_switch_domain_free;
};