[v3,4/5] bus: add device arguments name parsing API
Checks
Commit Message
To use Global Device Syntax as devargs, name is required for device
management.
In legacy parsing API, devargs name was extracted after bus name:
bus:name,kv_params,,,
To parse new Global Device Syntax, this patch introduces new bus API to
parse devargs and update name, different bus driver might choose
different keys from parameters with unified formating, example:
-a bus=pci,addr=83:00.0/class=eth/driver=mlx5,...
name: 0000:03:00.0
-a bus=vdev,name=pcap0/class=eth/driver=pcap,...
name:pcap0
Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
drivers/bus/pci/pci_common.c | 1 +
drivers/bus/pci/pci_params.c | 48 ++++++++++++++++++++++
drivers/bus/pci/private.h | 14 +++++++
drivers/bus/vdev/vdev.c | 1 +
drivers/bus/vdev/vdev_params.c | 43 +++++++++++++++++++
drivers/bus/vdev/vdev_private.h | 15 +++++++
lib/librte_eal/common/eal_common_devargs.c | 6 +++
lib/librte_eal/include/rte_bus.h | 19 +++++++++
8 files changed, 147 insertions(+)
Comments
The commit log should start by explaining it is adding a callback
to the bus drivers for the new devargs syntax.
30/03/2021 14:15, Xueming Li:
> To use Global Device Syntax as devargs, name is required for device
> management.
Context is missing.
You mean the argument "name" for the vdev bus?
>
> In legacy parsing API, devargs name was extracted after bus name:
> bus:name,kv_params,,,
>
> To parse new Global Device Syntax, this patch introduces new bus API to
> parse devargs and update name, different bus driver might choose
> different keys from parameters with unified formating, example:
> -a bus=pci,addr=83:00.0/class=eth/driver=mlx5,...
> name: 0000:03:00.0
> -a bus=vdev,name=pcap0/class=eth/driver=pcap,...
> name:pcap0
Only PCI and vdev buses are implemented.
What can be the plan for others?
We should track the progress somewhere, maybe with TODO comments.
This commit log could also state what is the status
of the global syntax support, talking about class and device drivers.
We could update this comment in ethdev:
* A new syntax is in development (not yet supported):
* - bus=X,paramX=x/class=Y,paramY=y/driver=Z,paramZ=z
[...]
> +int
> +rte_pci_devargs_parse(struct rte_devargs *da)
> +{
> + struct rte_kvargs *kvargs;
> + const char *addr_str;
> + struct rte_pci_addr addr;
> + int ret;
> +
> + if (da == NULL)
> + return 0;
> + RTE_ASSERT(da->bus_str != NULL);
> +
> + kvargs = rte_kvargs_parse(da->bus_str, NULL);
> + if (kvargs == NULL) {
> + RTE_LOG(ERR, EAL, "cannot parse argument list: %s\n",
> + da->bus_str);
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + addr_str = rte_kvargs_get(kvargs, pci_params_keys[RTE_PCI_PARAM_ADDR]);
> + if (addr_str == NULL) {
> + RTE_LOG(ERR, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
> + pci_params_keys[RTE_PCI_PARAM_ADDR], da->bus_str);
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + ret = rte_pci_addr_parse(addr_str, &addr);
> + if (ret != 0) {
> + RTE_LOG(ERR, EAL, "PCI address invalid: %s\n", da->bus_str);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + rte_pci_device_name(&addr, da->name, sizeof(da->name));
> +
> + /* TODO: class parse -> driver parse */
Please could you give a longer explanation of what is missing?
>-----Original Message-----
>From: Thomas Monjalon <thomas@monjalon.net>
>Sent: Wednesday, March 31, 2021 6:20 PM
>To: Xueming(Steven) Li <xuemingl@nvidia.com>
>Cc: Gaetan Rivet <gaetanr@nvidia.com>; dev@dpdk.org; Asaf Penso <asafp@nvidia.com>; david.marchand@redhat.com;
>ferruh.yigit@intel.com; andrew.rybchenko@oktetlabs.ru; hemant.agrawal@nxp.com; stephen@networkplumber.org;
>rosen.xu@intel.com; ajit.khaparde@broadcom.com; jerinj@marvell.com
>Subject: Re: [dpdk-dev] [PATCH v3 4/5] bus: add device arguments name parsing API
>
>The commit log should start by explaining it is adding a callback to the bus drivers for the new devargs syntax.
OK.
>
>30/03/2021 14:15, Xueming Li:
>> To use Global Device Syntax as devargs, name is required for device
>> management.
>
>Context is missing.
>You mean the argument "name" for the vdev bus?
Devargs.name, it is used by probe and device iterator. To locate a device from a devargs, devargs.name is compared
agains name of probed devices. This not an issue for legacy syntax, since the string after "bus:" is saved as name.
>
>>
>> In legacy parsing API, devargs name was extracted after bus name:
>> bus:name,kv_params,,,
>>
>> To parse new Global Device Syntax, this patch introduces new bus API
>> to parse devargs and update name, different bus driver might choose
>> different keys from parameters with unified formating, example:
>> -a bus=pci,addr=83:00.0/class=eth/driver=mlx5,...
>> name: 0000:03:00.0
>> -a bus=vdev,name=pcap0/class=eth/driver=pcap,...
>> name:pcap0
>
>Only PCI and vdev buses are implemented.
>What can be the plan for others?
>We should track the progress somewhere, maybe with TODO comments.
Like legacy parser, how about using "name" as default name key, the new syntax parser can resolve it by default.
Then PCI bus overrides by using "addr" key in new bus API,
vdev and other bus drivers simply use default implementation, i.e. using "name" as key..
>
>This commit log could also state what is the status of the global syntax support, talking about class and device drivers.
Yes
>
>We could update this comment in ethdev:
> * A new syntax is in development (not yet supported):
> * - bus=X,paramX=x/class=Y,paramY=y/driver=Z,paramZ=z
Will do it in next patch - enable new syntax
>
>[...]
>> +int
>> +rte_pci_devargs_parse(struct rte_devargs *da) {
>> + struct rte_kvargs *kvargs;
>> + const char *addr_str;
>> + struct rte_pci_addr addr;
>> + int ret;
>> +
>> + if (da == NULL)
>> + return 0;
>> + RTE_ASSERT(da->bus_str != NULL);
>> +
>> + kvargs = rte_kvargs_parse(da->bus_str, NULL);
>> + if (kvargs == NULL) {
>> + RTE_LOG(ERR, EAL, "cannot parse argument list: %s\n",
>> + da->bus_str);
>> + ret = -ENODEV;
>> + goto out;
>> + }
>> +
>> + addr_str = rte_kvargs_get(kvargs, pci_params_keys[RTE_PCI_PARAM_ADDR]);
>> + if (addr_str == NULL) {
>> + RTE_LOG(ERR, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
>> + pci_params_keys[RTE_PCI_PARAM_ADDR], da->bus_str);
>> + ret = -ENODEV;
>> + goto out;
>> + }
>> +
>> + ret = rte_pci_addr_parse(addr_str, &addr);
>> + if (ret != 0) {
>> + RTE_LOG(ERR, EAL, "PCI address invalid: %s\n", da->bus_str);
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + rte_pci_device_name(&addr, da->name, sizeof(da->name));
>> +
>> + /* TODO: class parse -> driver parse */
>
>Please could you give a longer explanation of what is missing?
Just an option to give class driver and device driver to override name parsing.
But this should happen in new devargs parser, not here if needed.
I'll remove this line, and mention it in commit log.
>
01/04/2021 17:13, Xueming(Steven) Li:
>From: Thomas Monjalon <thomas@monjalon.net>
> >30/03/2021 14:15, Xueming Li:
> >> To use Global Device Syntax as devargs, name is required for device
> >> management.
> >
> >Context is missing.
> >You mean the argument "name" for the vdev bus?
>
> Devargs.name, it is used by probe and device iterator.
I think we could avoid having a name with the new syntax.
In my understanding, this is for compatibility with the legacy syntax.
> To locate a device from a devargs, devargs.name is compared
> agains name of probed devices.
> This not an issue for legacy syntax,
> since the string after "bus:" is saved as name.
It would be interesting to explain where the name is parsed
for the legacy syntax: rte_devargs_parse
and for the new syntax: rte_devargs_layers_parse called
in rte_devargs_parse in the next patch.
> >> In legacy parsing API, devargs name was extracted after bus name:
> >> bus:name,kv_params,,,
> >>
> >> To parse new Global Device Syntax, this patch introduces new bus API
> >> to parse devargs and update name, different bus driver might choose
> >> different keys from parameters with unified formating, example:
> >> -a bus=pci,addr=83:00.0/class=eth/driver=mlx5,...
> >> name: 0000:03:00.0
> >> -a bus=vdev,name=pcap0/class=eth/driver=pcap,...
> >> name:pcap0
> >
> >Only PCI and vdev buses are implemented.
> >What can be the plan for others?
> >We should track the progress somewhere, maybe with TODO comments.
>
> Like legacy parser, how about using "name" as default name key, the new syntax parser can resolve it by default.
> Then PCI bus overrides by using "addr" key in new bus API,
> vdev and other bus drivers simply use default implementation, i.e. using "name" as key..
Yes, you mean if devargs_parse is not implemented by the bus driver,
the default is to parse the name property,
while the PCI implementation fills the devargs name with the addr property.
> >[...]
> >> +int
> >> +rte_pci_devargs_parse(struct rte_devargs *da) {
> >> + struct rte_kvargs *kvargs;
> >> + const char *addr_str;
> >> + struct rte_pci_addr addr;
> >> + int ret;
> >> +
> >> + if (da == NULL)
> >> + return 0;
> >> + RTE_ASSERT(da->bus_str != NULL);
> >> +
> >> + kvargs = rte_kvargs_parse(da->bus_str, NULL);
> >> + if (kvargs == NULL) {
> >> + RTE_LOG(ERR, EAL, "cannot parse argument list: %s\n",
> >> + da->bus_str);
> >> + ret = -ENODEV;
> >> + goto out;
> >> + }
> >> +
> >> + addr_str = rte_kvargs_get(kvargs, pci_params_keys[RTE_PCI_PARAM_ADDR]);
> >> + if (addr_str == NULL) {
> >> + RTE_LOG(ERR, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
> >> + pci_params_keys[RTE_PCI_PARAM_ADDR], da->bus_str);
> >> + ret = -ENODEV;
> >> + goto out;
> >> + }
> >> +
> >> + ret = rte_pci_addr_parse(addr_str, &addr);
> >> + if (ret != 0) {
> >> + RTE_LOG(ERR, EAL, "PCI address invalid: %s\n", da->bus_str);
> >> + ret = -EINVAL;
> >> + goto out;
> >> + }
> >> +
> >> + rte_pci_device_name(&addr, da->name, sizeof(da->name));
> >> +
> >> + /* TODO: class parse -> driver parse */
> >
> >Please could you give a longer explanation of what is missing?
>
> Just an option to give class driver and device driver to override name parsing.
> But this should happen in new devargs parser, not here if needed.
>
> I'll remove this line, and mention it in commit log.
What would be the benefit of overriding devargs name parsing?
I feel it would be better to not rely on the devargs name
with the new syntax if we have such need.
@@ -760,6 +760,7 @@ struct rte_pci_bus rte_pci_bus = {
.dev_iterate = rte_pci_dev_iterate,
.hot_unplug_handler = pci_hot_unplug_handler,
.sigbus_handler = pci_sigbus_handler,
+ .devargs_parse = rte_pci_devargs_parse,
},
.device_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.device_list),
.driver_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.driver_list),
@@ -8,6 +8,8 @@
#include <rte_errno.h>
#include <rte_kvargs.h>
#include <rte_pci.h>
+#include <rte_devargs.h>
+#include <rte_debug.h>
#include "private.h"
@@ -76,3 +78,49 @@ rte_pci_dev_iterate(const void *start,
rte_kvargs_free(kvargs);
return dev;
}
+
+int
+rte_pci_devargs_parse(struct rte_devargs *da)
+{
+ struct rte_kvargs *kvargs;
+ const char *addr_str;
+ struct rte_pci_addr addr;
+ int ret;
+
+ if (da == NULL)
+ return 0;
+ RTE_ASSERT(da->bus_str != NULL);
+
+ kvargs = rte_kvargs_parse(da->bus_str, NULL);
+ if (kvargs == NULL) {
+ RTE_LOG(ERR, EAL, "cannot parse argument list: %s\n",
+ da->bus_str);
+ ret = -ENODEV;
+ goto out;
+ }
+
+ addr_str = rte_kvargs_get(kvargs, pci_params_keys[RTE_PCI_PARAM_ADDR]);
+ if (addr_str == NULL) {
+ RTE_LOG(ERR, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
+ pci_params_keys[RTE_PCI_PARAM_ADDR], da->bus_str);
+ ret = -ENODEV;
+ goto out;
+ }
+
+ ret = rte_pci_addr_parse(addr_str, &addr);
+ if (ret != 0) {
+ RTE_LOG(ERR, EAL, "PCI address invalid: %s\n", da->bus_str);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ rte_pci_device_name(&addr, da->name, sizeof(da->name));
+
+ /* TODO: class parse -> driver parse */
+out:
+ if (kvargs != NULL)
+ rte_kvargs_free(kvargs);
+ if (ret != 0)
+ rte_errno = -ret;
+ return ret;
+}
@@ -267,4 +267,18 @@ rte_pci_dev_iterate(const void *start,
const char *str,
const struct rte_dev_iterator *it);
+/*
+ * Parse device arguments and update name.
+ *
+ * @param da
+ * device arguments to parse.
+ *
+ * @return
+ * 0 on success.
+ * -EINVAL: kvargs string is invalid and cannot be parsed.
+ * -ENODEV: no key matching a device ID is found in the kv list.
+ */
+int
+rte_pci_devargs_parse(struct rte_devargs *da);
+
#endif /* _PCI_PRIVATE_H_ */
@@ -634,6 +634,7 @@ static struct rte_bus rte_vdev_bus = {
.dma_unmap = vdev_dma_unmap,
.get_iommu_class = vdev_get_iommu_class,
.dev_iterate = rte_vdev_dev_iterate,
+ .devargs_parse = rte_vdev_devargs_parse,
};
RTE_REGISTER_BUS(vdev, rte_vdev_bus);
@@ -8,6 +8,9 @@
#include <rte_bus.h>
#include <rte_kvargs.h>
#include <rte_errno.h>
+#include <rte_devargs.h>
+#include <rte_string_fns.h>
+#include <rte_debug.h>
#include "vdev_logs.h"
#include "vdev_private.h"
@@ -64,3 +67,43 @@ rte_vdev_dev_iterate(const void *start,
rte_kvargs_free(kvargs);
return dev;
}
+
+int
+rte_vdev_devargs_parse(struct rte_devargs *da)
+{
+ struct rte_kvargs *kvargs;
+ const char *name;
+ int ret = 0;
+
+ if (da == NULL)
+ return 0;
+ RTE_ASSERT(da->bus_str != NULL);
+
+ kvargs = rte_kvargs_parse(da->bus_str, NULL);
+ if (kvargs == NULL) {
+ RTE_LOG(ERR, EAL, "cannot parse argument list: %s\n",
+ da->bus_str);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ name = rte_kvargs_get(kvargs, vdev_params_keys[RTE_VDEV_PARAM_NAME]);
+ if (name == NULL) {
+ RTE_LOG(ERR, EAL, "name not found: %s\n", da->bus_str);
+ ret = -ENODEV;
+ goto out;
+ }
+
+ if (rte_strscpy(da->name, name, sizeof(da->name)) < 0) {
+ RTE_LOG(ERR, EAL, "device name is too long: %s\n", name);
+ ret = -E2BIG;
+ }
+
+ /* TODO: class parse -> driver parse */
+out:
+ if (kvargs != NULL)
+ rte_kvargs_free(kvargs);
+ if (ret != 0)
+ rte_errno = -ret;
+ return ret;
+}
@@ -19,6 +19,21 @@ rte_vdev_dev_iterate(const void *start,
const char *str,
const struct rte_dev_iterator *it);
+/*
+ * Parse device argument and update name.
+ *
+ * @param da
+ * device argument to parse.
+ *
+ * @return
+ * 0 on success.
+ * -EINVAL: kvargs string is invalid and cannot be parsed.
+ * -ENODEV: no key matching a device ID is found in the kv list.
+ * -E2BIG: device name is too long.
+ */
+int
+rte_vdev_devargs_parse(struct rte_devargs *da);
+
#ifdef __cplusplus
}
#endif
@@ -118,6 +118,8 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
if (layers[i].kvlist == NULL)
continue;
kv = &layers[i].kvlist->pairs[0];
+ if (!kv->key)
+ continue;
if (strcmp(kv->key, "bus") == 0) {
bus = rte_bus_find_by_name(kv->value);
if (bus == NULL) {
@@ -160,6 +162,10 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
}
}
+ /* Parse device name, optional for iterator filter. */
+ if (bus && bus->devargs_parse)
+ bus->devargs_parse(devargs);
+
get_out:
for (i = 0; i < RTE_DIM(layers); i++) {
if (layers[i].kvlist)
@@ -210,6 +210,24 @@ typedef int (*rte_bus_hot_unplug_handler_t)(struct rte_device *dev);
*/
typedef int (*rte_bus_sigbus_handler_t)(const void *failure_addr);
+/**
+ * Parse device arguments, setting the device name in the devargs as a result.
+ * A bus could use the class and driver layers to resolve name furthermore.
+ *
+ * On error rte_errno is set.
+ *
+ * @param da
+ * Pointer to the devargs to parse.
+ * The 'bus_str' field must be set.
+ *
+ * @return
+ * 0 on successful parsing.
+ * -EINVAL: on parsing error.
+ * -ENODEV: if no key matching a device argument is specified.
+ * -E2BIG: device name is too long.
+ */
+typedef int (*rte_bus_devargs_parse_t)(struct rte_devargs *da);
+
/**
* Bus scan policies
*/
@@ -267,6 +285,7 @@ struct rte_bus {
/**< handle hot-unplug failure on the bus */
rte_bus_sigbus_handler_t sigbus_handler;
/**< handle sigbus error on the bus */
+ rte_bus_devargs_parse_t devargs_parse; /**< Parse device arguments */
};