diff mbox

[dpdk-dev,v2,12/18] eal: add generic device declaration parameter

Message ID c16edb4815d830679404fa1012b2989e3ba24f9c.1507796100.git.gaetan.rivet@6wind.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail apply patch file failure

Commit Message

Gaëtan Rivet Oct. 12, 2017, 8:21 a.m. UTC 
  Add a new generic device declaration parameter:

   --dev=<device_declaration>

That allows to declare device from any bus. The format is as follows:

device_declaration := <bus><c><device>[,arg_list]

bus      := bus name
c        := arbitrary character separator
device   := device name (PCI location, virtual PMD name, ...)
arg_list := key value list: key1=val1[,key2=val2[,...]]

The bus name is mandatory. The character separator can be anything.
The device name is mandatory. The argument list is optional.

Examples:

    --dev=pci:0000:05:00.0,port=1
    --dev=vdev_net_ring0

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_options.c | 19 +++++++++++++++++++
 lib/librte_eal/common/eal_options.h        |  2 ++
 2 files changed, 21 insertions(+)

Comments

Shreyansh Jain Dec. 13, 2017, 2:26 p.m. UTC | #1 
On Thursday 12 October 2017 01:51 PM, Gaetan Rivet wrote:
> Add a new generic device declaration parameter:
> 
>     --dev=<device_declaration>
> 

[...]

> 
> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index 603df27..b7591fd 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -95,6 +95,7 @@ eal_long_options[] = {
>   	{OPT_PROC_TYPE,         1, NULL, OPT_PROC_TYPE_NUM        },
>   	{OPT_SOCKET_MEM,        1, NULL, OPT_SOCKET_MEM_NUM       },
>   	{OPT_SYSLOG,            1, NULL, OPT_SYSLOG_NUM           },
> +	{OPT_DEV,               1, NULL, OPT_DEV_NUM              },
>   	{OPT_VDEV,              1, NULL, OPT_VDEV_NUM             },
>   	{OPT_VFIO_INTR,         1, NULL, OPT_VFIO_INTR_NUM        },
>   	{OPT_VMWARE_TSC_MAP,    0, NULL, OPT_VMWARE_TSC_MAP_NUM   },
> @@ -1120,6 +1121,21 @@ eal_parse_common_option(int opt, const char *optarg,
>   		}
>   		break;
>   
> +	case OPT_DEV_NUM: {
> +		struct rte_devargs da;
> +		int ret;
> +
> +		if (rte_eal_devargs_parse(&da, optarg) < 0)
> +			return -1;
> +		ret = rte_bus_probe_mode_set(da.bus->name,
> +					RTE_BUS_PROBE_WHITELIST);
> +		if (ret < 0 && ret != -ENOTSUP)
> +			return -1;
> +		if (eal_option_device_add(NULL, optarg) < 0)
> +			return -1;
> +	}

Might be a naive question: Any specific reason why we don't add the 
devices directly into devargs_list here (eal_parse_args -> 
eal_parse_common_option -> OPT_DEV ->) rather than wait for eal to call 
eal_option_device_parse again?

Is it to allow eal_plugins_init() to finish?

> +		break;
> +
>   	case OPT_VDEV_NUM:
>   		if (eal_option_device_add("vdev", optarg) < 0)
>   			return -1;

[...]
Gaëtan Rivet Dec. 13, 2017, 2:47 p.m. UTC | #2 
On Wed, Dec 13, 2017 at 07:56:42PM +0530, Shreyansh Jain wrote:
> On Thursday 12 October 2017 01:51 PM, Gaetan Rivet wrote:
> > Add a new generic device declaration parameter:
> > 
> >     --dev=<device_declaration>
> > 
> 
> [...]
> 
> > 
> > diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> > index 603df27..b7591fd 100644
> > --- a/lib/librte_eal/common/eal_common_options.c
> > +++ b/lib/librte_eal/common/eal_common_options.c
> > @@ -95,6 +95,7 @@ eal_long_options[] = {
> >   	{OPT_PROC_TYPE,         1, NULL, OPT_PROC_TYPE_NUM        },
> >   	{OPT_SOCKET_MEM,        1, NULL, OPT_SOCKET_MEM_NUM       },
> >   	{OPT_SYSLOG,            1, NULL, OPT_SYSLOG_NUM           },
> > +	{OPT_DEV,               1, NULL, OPT_DEV_NUM              },
> >   	{OPT_VDEV,              1, NULL, OPT_VDEV_NUM             },
> >   	{OPT_VFIO_INTR,         1, NULL, OPT_VFIO_INTR_NUM        },
> >   	{OPT_VMWARE_TSC_MAP,    0, NULL, OPT_VMWARE_TSC_MAP_NUM   },
> > @@ -1120,6 +1121,21 @@ eal_parse_common_option(int opt, const char *optarg,
> >   		}
> >   		break;
> > +	case OPT_DEV_NUM: {
> > +		struct rte_devargs da;
> > +		int ret;
> > +
> > +		if (rte_eal_devargs_parse(&da, optarg) < 0)
> > +			return -1;
> > +		ret = rte_bus_probe_mode_set(da.bus->name,
> > +					RTE_BUS_PROBE_WHITELIST);
> > +		if (ret < 0 && ret != -ENOTSUP)
> > +			return -1;
> > +		if (eal_option_device_add(NULL, optarg) < 0)
> > +			return -1;
> > +	}
> 
> Might be a naive question: Any specific reason why we don't add the devices
> directly into devargs_list here (eal_parse_args -> eal_parse_common_option
> -> OPT_DEV ->) rather than wait for eal to call eal_option_device_parse
> again?
> 
> Is it to allow eal_plugins_init() to finish?
> 

Yes. And actually this makes me aware of an issue with this
implementation.

Calling rte_eal_devargs_parse here is premature, and
rte_bus_probe_mode_set as well.

eal_plugins_init() must be executed before calling rte_devargs to allow
for buses introduced as plugins to be able to recognize their devices.

I will reorder a few things in eal_options, thanks for catching this.

> > +		break;
> > +
> >   	case OPT_VDEV_NUM:
> >   		if (eal_option_device_add("vdev", optarg) < 0)
> >   			return -1;
> 
> [...]
>
Shreyansh Jain Dec. 13, 2017, 3:24 p.m. UTC | #3 
On Wednesday 13 December 2017 08:17 PM, Gaëtan Rivet wrote:
> On Wed, Dec 13, 2017 at 07:56:42PM +0530, Shreyansh Jain wrote:
>> On Thursday 12 October 2017 01:51 PM, Gaetan Rivet wrote:
>>> Add a new generic device declaration parameter:
>>>
>>>      --dev=<device_declaration>
>>>
>>
>> [...]
>>
>>>
>>> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
>>> index 603df27..b7591fd 100644
>>> --- a/lib/librte_eal/common/eal_common_options.c
>>> +++ b/lib/librte_eal/common/eal_common_options.c
>>> @@ -95,6 +95,7 @@ eal_long_options[] = {
>>>    	{OPT_PROC_TYPE,         1, NULL, OPT_PROC_TYPE_NUM        },
>>>    	{OPT_SOCKET_MEM,        1, NULL, OPT_SOCKET_MEM_NUM       },
>>>    	{OPT_SYSLOG,            1, NULL, OPT_SYSLOG_NUM           },
>>> +	{OPT_DEV,               1, NULL, OPT_DEV_NUM              },
>>>    	{OPT_VDEV,              1, NULL, OPT_VDEV_NUM             },
>>>    	{OPT_VFIO_INTR,         1, NULL, OPT_VFIO_INTR_NUM        },
>>>    	{OPT_VMWARE_TSC_MAP,    0, NULL, OPT_VMWARE_TSC_MAP_NUM   },
>>> @@ -1120,6 +1121,21 @@ eal_parse_common_option(int opt, const char *optarg,
>>>    		}
>>>    		break;
>>> +	case OPT_DEV_NUM: {
>>> +		struct rte_devargs da;
>>> +		int ret;
>>> +
>>> +		if (rte_eal_devargs_parse(&da, optarg) < 0)
>>> +			return -1;
>>> +		ret = rte_bus_probe_mode_set(da.bus->name,
>>> +					RTE_BUS_PROBE_WHITELIST);
>>> +		if (ret < 0 && ret != -ENOTSUP)
>>> +			return -1;
>>> +		if (eal_option_device_add(NULL, optarg) < 0)
>>> +			return -1;
>>> +	}
>>
>> Might be a naive question: Any specific reason why we don't add the devices
>> directly into devargs_list here (eal_parse_args -> eal_parse_common_option
>> -> OPT_DEV ->) rather than wait for eal to call eal_option_device_parse
>> again?
>>
>> Is it to allow eal_plugins_init() to finish?
>>
> 
> Yes. And actually this makes me aware of an issue with this
> implementation.
> 
> Calling rte_eal_devargs_parse here is premature, and
> rte_bus_probe_mode_set as well.
> 
> eal_plugins_init() must be executed before calling rte_devargs to allow
> for buses introduced as plugins to be able to recognize their devices.

There might be one more catch. Maybe eal_parse_args also finds all the 
plugins to load (-d ...).

> 
> I will reorder a few things in eal_options, thanks for catching this.
> 
>>> +		break;
>>> +
>>>    	case OPT_VDEV_NUM:
>>>    		if (eal_option_device_add("vdev", optarg) < 0)
>>>    			return -1;
>>
>> [...]
>>
>
diff mbox

Patch

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 603df27..b7591fd 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -95,6 +95,7 @@  eal_long_options[] = {
 	{OPT_PROC_TYPE,         1, NULL, OPT_PROC_TYPE_NUM        },
 	{OPT_SOCKET_MEM,        1, NULL, OPT_SOCKET_MEM_NUM       },
 	{OPT_SYSLOG,            1, NULL, OPT_SYSLOG_NUM           },
+	{OPT_DEV,               1, NULL, OPT_DEV_NUM              },
 	{OPT_VDEV,              1, NULL, OPT_VDEV_NUM             },
 	{OPT_VFIO_INTR,         1, NULL, OPT_VFIO_INTR_NUM        },
 	{OPT_VMWARE_TSC_MAP,    0, NULL, OPT_VMWARE_TSC_MAP_NUM   },
@@ -1120,6 +1121,21 @@  eal_parse_common_option(int opt, const char *optarg,
 		}
 		break;
 
+	case OPT_DEV_NUM: {
+		struct rte_devargs da;
+		int ret;
+
+		if (rte_eal_devargs_parse(&da, optarg) < 0)
+			return -1;
+		ret = rte_bus_probe_mode_set(da.bus->name,
+					RTE_BUS_PROBE_WHITELIST);
+		if (ret < 0 && ret != -ENOTSUP)
+			return -1;
+		if (eal_option_device_add(NULL, optarg) < 0)
+			return -1;
+	}
+		break;
+
 	case OPT_VDEV_NUM:
 		if (eal_option_device_add("vdev", optarg) < 0)
 			return -1;
@@ -1271,6 +1287,9 @@  eal_common_usage(void)
 	       "  -n CHANNELS         Number of memory channels\n"
 	       "  -m MB               Memory to allocate (see also --"OPT_SOCKET_MEM")\n"
 	       "  -r RANKS            Force number of memory ranks (don't detect)\n"
+	       "  --"OPT_DEV"         Declare a device.\n"
+	       "                      The argument format is <bus><c><device>[,key=val,...]\n"
+	       "                      ex: pci:00:00.0,key=val\n"
 	       "  -b, --"OPT_PCI_BLACKLIST" Add a PCI device in black list.\n"
 	       "                      Prevent EAL from using this PCI device. The argument\n"
 	       "                      format is <domain:bus:devid.func>.\n"
diff --git a/lib/librte_eal/common/eal_options.h b/lib/librte_eal/common/eal_options.h
index 30e6bb4..d50eff7 100644
--- a/lib/librte_eal/common/eal_options.h
+++ b/lib/librte_eal/common/eal_options.h
@@ -77,6 +77,8 @@  enum {
 	OPT_SOCKET_MEM_NUM,
 #define OPT_SYSLOG            "syslog"
 	OPT_SYSLOG_NUM,
+#define OPT_DEV               "dev"
+	OPT_DEV_NUM,
 #define OPT_VDEV              "vdev"
 	OPT_VDEV_NUM,
 #define OPT_VFIO_INTR         "vfio-intr"