diff mbox series

[v3,4/5] bus: add device arguments name parsing API

Message ID 1617106521-25931-5-git-send-email-xuemingl@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series eal: enable global device syntax by default | expand

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Xueming(Steven) Li March 30, 2021, 12:15 p.m. UTC
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

Thomas Monjalon March 31, 2021, 10:19 a.m. UTC | #1
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?
Xueming(Steven) Li April 1, 2021, 3:13 p.m. UTC | #2
>-----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.
>
Thomas Monjalon April 8, 2021, 11:49 p.m. UTC | #3
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.
diff mbox series

Patch

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 9b8d769287..61d3f51452 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -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),
diff --git a/drivers/bus/pci/pci_params.c b/drivers/bus/pci/pci_params.c
index 3192e9c967..7ba9e2650f 100644
--- a/drivers/bus/pci/pci_params.c
+++ b/drivers/bus/pci/pci_params.c
@@ -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;
+}
diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
index f566943f5e..8bc5140e97 100644
--- a/drivers/bus/pci/private.h
+++ b/drivers/bus/pci/private.h
@@ -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_ */
diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index d075409942..d6f651bff2 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -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);
diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c
index 6f74704d1c..3e644ade95 100644
--- a/drivers/bus/vdev/vdev_params.c
+++ b/drivers/bus/vdev/vdev_params.c
@@ -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;
+}
diff --git a/drivers/bus/vdev/vdev_private.h b/drivers/bus/vdev/vdev_private.h
index ba6dc48ff3..257682daac 100644
--- a/drivers/bus/vdev/vdev_private.h
+++ b/drivers/bus/vdev/vdev_private.h
@@ -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
diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index e40b91ea66..b4dcb0099c 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -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)
diff --git a/lib/librte_eal/include/rte_bus.h b/lib/librte_eal/include/rte_bus.h
index 80b154fb98..e006b514de 100644
--- a/lib/librte_eal/include/rte_bus.h
+++ b/lib/librte_eal/include/rte_bus.h
@@ -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 */
 
 };