[dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev args

[dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev args

[Pavan Nikhilesh]

Currently, the only way of supplying device argument to a pci device is
to whitelist it i.e. -w 000X:00:0X.0,self_test=1. This is not a very
feasible method as whitelisting a device has its own side effects i.e
only the whitelisted pci devices are probed.

Add a new eal command line option --pci-args to pass device args without
the need to whitelist the devices.
		--pci-args 000X:00:0X.0,self_test=1

Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
---
 v2 Changes:
 - Document the option usage in eal_common_usage.
 - Update commit log to be more informative.

 lib/librte_eal/common/eal_common_devargs.c  | 3 +++
 lib/librte_eal/common/eal_common_options.c  | 9 +++++++++
 lib/librte_eal/common/eal_options.h         | 2 ++
 lib/librte_eal/common/include/rte_dev.h     | 1 +
 lib/librte_eal/common/include/rte_devargs.h | 1 +
 5 files changed, 16 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index b0434158b..a56bfeea0 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -156,6 +156,9 @@ rte_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	bus = devargs->bus;
 	if (devargs->type == RTE_DEVTYPE_BLACKLISTED_PCI)
 		devargs->policy = RTE_DEV_BLACKLISTED;
+	else if (devargs->type == RTE_DEVTYPE_WHITELISTED_PCI)
+		devargs->policy = RTE_DEV_WHITELISTED;
+
 	if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
 		if (devargs->policy == RTE_DEV_WHITELISTED)
 			bus->conf.scan_mode = RTE_BUS_SCAN_WHITELIST;
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index ecebb2923..31eebaa53 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -76,6 +76,7 @@ eal_long_options[] = {
 	{OPT_VMWARE_TSC_MAP,    0, NULL, OPT_VMWARE_TSC_MAP_NUM   },
 	{OPT_LEGACY_MEM,        0, NULL, OPT_LEGACY_MEM_NUM       },
 	{OPT_SINGLE_FILE_SEGMENTS, 0, NULL, OPT_SINGLE_FILE_SEGMENTS_NUM},
+	{OPT_PCI_DEVARGS,       1, NULL, OPT_PCI_DEVARGS_NUM},
 	{0,                     0, NULL, 0                        }
 };

@@ -1224,6 +1225,12 @@ eal_parse_common_option(int opt, const char *optarg,
 	case OPT_SINGLE_FILE_SEGMENTS_NUM:
 		conf->single_file_segments = 1;
 		break;
+	case OPT_PCI_DEVARGS_NUM:
+		if (eal_option_device_add(RTE_DEVTYPE_NORMAL,
+				optarg) < 0) {
+			return -1;
+		}
+		break;

 	/* don't know what to do, leave this to caller */
 	default:
@@ -1360,6 +1367,8 @@ eal_common_usage(void)
 	       "  --"OPT_VDEV"              Add a virtual device.\n"
 	       "                      The argument format is <driver><id>[,key=val,...]\n"
 	       "                      (ex: --vdev=net_pcap0,iface=eth2).\n"
+	       "  --"OPT_PCI_DEVARGS"          Pass key-value arguments to a pci device\n"
+	       "                      The argument format is <domain:bus:devid.func>[,key=val,...]\n"
 	       "  -d LIB.so|DIR       Add a driver or driver directory\n"
 	       "                      (can be used multiple times)\n"
 	       "  --"OPT_VMWARE_TSC_MAP"    Use VMware TSC map instead of native RDTSC\n"
diff --git a/lib/librte_eal/common/eal_options.h b/lib/librte_eal/common/eal_options.h
index 211ae06ae..d52d38e32 100644
--- a/lib/librte_eal/common/eal_options.h
+++ b/lib/librte_eal/common/eal_options.h
@@ -59,6 +59,8 @@ enum {
 	OPT_LEGACY_MEM_NUM,
 #define OPT_SINGLE_FILE_SEGMENTS    "single-file-segments"
 	OPT_SINGLE_FILE_SEGMENTS_NUM,
+#define OPT_PCI_DEVARGS       "pci-args"
+	OPT_PCI_DEVARGS_NUM,
 	OPT_LONG_MAX_NUM
 };

diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index 3879ff3ca..fb3e5033f 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -124,6 +124,7 @@ enum rte_kernel_driver {
  * Device policies.
  */
 enum rte_dev_policy {
+	RTE_DEV_NORMAL,
 	RTE_DEV_WHITELISTED,
 	RTE_DEV_BLACKLISTED,
 };
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 58fbd90a2..78c600bf2 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -29,6 +29,7 @@ extern "C" {
  * Type of generic device
  */
 enum rte_devtype {
+	RTE_DEVTYPE_NORMAL, /* Normal dev with special pci args */
 	RTE_DEVTYPE_WHITELISTED_PCI,
 	RTE_DEVTYPE_BLACKLISTED_PCI,
 	RTE_DEVTYPE_VIRTUAL,
--
2.17.1

Re: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev args

[Shahaf Shuler]

Hi Pavan,

Friday, June 15, 2018 7:44 AM, Pavan Nikhilesh:
> Subject: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev
> args
> 
> Currently, the only way of supplying device argument to a pci device is to
> whitelist it i.e. -w 000X:00:0X.0,self_test=1. This is not a very feasible method
> as whitelisting a device has its own side effects i.e only the whitelisted pci
> devices are probed.
> 
> Add a new eal command line option --pci-args to pass device args without the
> need to whitelist the devices.
> 		--pci-args 000X:00:0X.0,self_test=1
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>

Tested-by: Shahaf Shuler <shahafs@mellanox.com>

It seems to work. 
Please see small comments below

> ---
>  v2 Changes:
>  - Document the option usage in eal_common_usage.
>  - Update commit log to be more informative.
> 
>  lib/librte_eal/common/eal_common_devargs.c  | 3 +++
> lib/librte_eal/common/eal_common_options.c  | 9 +++++++++
>  lib/librte_eal/common/eal_options.h         | 2 ++
>  lib/librte_eal/common/include/rte_dev.h     | 1 +
>  lib/librte_eal/common/include/rte_devargs.h | 1 +
>  5 files changed, 16 insertions(+)

Should we also update the manual of testpmd (doc/guides/testpmd_app_ug/run_app.rst ) for the new eal arg?

> 
> diff --git a/lib/librte_eal/common/eal_common_devargs.c
> b/lib/librte_eal/common/eal_common_devargs.c
> index b0434158b..a56bfeea0 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -156,6 +156,9 @@ rte_devargs_add(enum rte_devtype devtype, const
> char *devargs_str)
>  	bus = devargs->bus;
>  	if (devargs->type == RTE_DEVTYPE_BLACKLISTED_PCI)
>  		devargs->policy = RTE_DEV_BLACKLISTED;
> +	else if (devargs->type == RTE_DEVTYPE_WHITELISTED_PCI)
> +		devargs->policy = RTE_DEV_WHITELISTED;
> +
>  	if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
>  		if (devargs->policy == RTE_DEV_WHITELISTED)
>  			bus->conf.scan_mode = RTE_BUS_SCAN_WHITELIST;
> diff --git a/lib/librte_eal/common/eal_common_options.c
> b/lib/librte_eal/common/eal_common_options.c
> index ecebb2923..31eebaa53 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -76,6 +76,7 @@ eal_long_options[] = {
>  	{OPT_VMWARE_TSC_MAP,    0, NULL,
> OPT_VMWARE_TSC_MAP_NUM   },
>  	{OPT_LEGACY_MEM,        0, NULL, OPT_LEGACY_MEM_NUM       },
>  	{OPT_SINGLE_FILE_SEGMENTS, 0, NULL,
> OPT_SINGLE_FILE_SEGMENTS_NUM},
> +	{OPT_PCI_DEVARGS,       1, NULL, OPT_PCI_DEVARGS_NUM},
>  	{0,                     0, NULL, 0                        }
>  };
> 
> @@ -1224,6 +1225,12 @@ eal_parse_common_option(int opt, const char
> *optarg,
>  	case OPT_SINGLE_FILE_SEGMENTS_NUM:
>  		conf->single_file_segments = 1;
>  		break;
> +	case OPT_PCI_DEVARGS_NUM:
> +		if (eal_option_device_add(RTE_DEVTYPE_NORMAL,
> +				optarg) < 0) {
> +			return -1;
> +		}
> +		break;
> 
>  	/* don't know what to do, leave this to caller */
>  	default:
> @@ -1360,6 +1367,8 @@ eal_common_usage(void)
>  	       "  --"OPT_VDEV"              Add a virtual device.\n"
>  	       "                      The argument format is <driver><id>[,key=val,...]\n"
>  	       "                      (ex: --vdev=net_pcap0,iface=eth2).\n"
> +	       "  --"OPT_PCI_DEVARGS"          Pass key-value arguments to a pci
> device\n"
> +	       "                      The argument format is
> <domain:bus:devid.func>[,key=val,...]\n"
>  	       "  -d LIB.so|DIR       Add a driver or driver directory\n"
>  	       "                      (can be used multiple times)\n"
>  	       "  --"OPT_VMWARE_TSC_MAP"    Use VMware TSC map instead
> of native RDTSC\n"
> diff --git a/lib/librte_eal/common/eal_options.h
> b/lib/librte_eal/common/eal_options.h
> index 211ae06ae..d52d38e32 100644
> --- a/lib/librte_eal/common/eal_options.h
> +++ b/lib/librte_eal/common/eal_options.h
> @@ -59,6 +59,8 @@ enum {
>  	OPT_LEGACY_MEM_NUM,
>  #define OPT_SINGLE_FILE_SEGMENTS    "single-file-segments"
>  	OPT_SINGLE_FILE_SEGMENTS_NUM,
> +#define OPT_PCI_DEVARGS       "pci-args"
> +	OPT_PCI_DEVARGS_NUM,
>  	OPT_LONG_MAX_NUM
>  };
> 
> diff --git a/lib/librte_eal/common/include/rte_dev.h
> b/lib/librte_eal/common/include/rte_dev.h
> index 3879ff3ca..fb3e5033f 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -124,6 +124,7 @@ enum rte_kernel_driver {
>   * Device policies.
>   */
>  enum rte_dev_policy {
> +	RTE_DEV_NORMAL,
>  	RTE_DEV_WHITELISTED,
>  	RTE_DEV_BLACKLISTED,
>  };
> diff --git a/lib/librte_eal/common/include/rte_devargs.h
> b/lib/librte_eal/common/include/rte_devargs.h
> index 58fbd90a2..78c600bf2 100644
> --- a/lib/librte_eal/common/include/rte_devargs.h
> +++ b/lib/librte_eal/common/include/rte_devargs.h
> @@ -29,6 +29,7 @@ extern "C" {
>   * Type of generic device
>   */
>  enum rte_devtype {
> +	RTE_DEVTYPE_NORMAL, /* Normal dev with special pci args */

What is "Normal" device? Can we find a better name? 

>  	RTE_DEVTYPE_WHITELISTED_PCI,
>  	RTE_DEVTYPE_BLACKLISTED_PCI,
>  	RTE_DEVTYPE_VIRTUAL,
> --
> 2.17.1

Re: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev args

[Ferruh Yigit]

On 6/26/2018 1:48 PM, Shahaf Shuler wrote:
> Hi Pavan,
> 
> Friday, June 15, 2018 7:44 AM, Pavan Nikhilesh:
>> Subject: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev
>> args
>>
>> Currently, the only way of supplying device argument to a pci device is to
>> whitelist it i.e. -w 000X:00:0X.0,self_test=1. This is not a very feasible method
>> as whitelisting a device has its own side effects i.e only the whitelisted pci
>> devices are probed.
>>
>> Add a new eal command line option --pci-args to pass device args without the
>> need to whitelist the devices.
>> 		--pci-args 000X:00:0X.0,self_test=1
>>
>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> 
> Tested-by: Shahaf Shuler <shahafs@mellanox.com>
> 
> It seems to work. 
> Please see small comments below

Isn't this conflict with Gaetan's devarg work which has wider scope?

> 
>> ---
>>  v2 Changes:
>>  - Document the option usage in eal_common_usage.
>>  - Update commit log to be more informative.
>>
>>  lib/librte_eal/common/eal_common_devargs.c  | 3 +++
>> lib/librte_eal/common/eal_common_options.c  | 9 +++++++++
>>  lib/librte_eal/common/eal_options.h         | 2 ++
>>  lib/librte_eal/common/include/rte_dev.h     | 1 +
>>  lib/librte_eal/common/include/rte_devargs.h | 1 +
>>  5 files changed, 16 insertions(+)
> 
> Should we also update the manual of testpmd (doc/guides/testpmd_app_ug/run_app.rst ) for the new eal arg?
> 
>>
>> diff --git a/lib/librte_eal/common/eal_common_devargs.c
>> b/lib/librte_eal/common/eal_common_devargs.c
>> index b0434158b..a56bfeea0 100644
>> --- a/lib/librte_eal/common/eal_common_devargs.c
>> +++ b/lib/librte_eal/common/eal_common_devargs.c
>> @@ -156,6 +156,9 @@ rte_devargs_add(enum rte_devtype devtype, const
>> char *devargs_str)
>>  	bus = devargs->bus;
>>  	if (devargs->type == RTE_DEVTYPE_BLACKLISTED_PCI)
>>  		devargs->policy = RTE_DEV_BLACKLISTED;
>> +	else if (devargs->type == RTE_DEVTYPE_WHITELISTED_PCI)
>> +		devargs->policy = RTE_DEV_WHITELISTED;
>> +
>>  	if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
>>  		if (devargs->policy == RTE_DEV_WHITELISTED)
>>  			bus->conf.scan_mode = RTE_BUS_SCAN_WHITELIST;
>> diff --git a/lib/librte_eal/common/eal_common_options.c
>> b/lib/librte_eal/common/eal_common_options.c
>> index ecebb2923..31eebaa53 100644
>> --- a/lib/librte_eal/common/eal_common_options.c
>> +++ b/lib/librte_eal/common/eal_common_options.c
>> @@ -76,6 +76,7 @@ eal_long_options[] = {
>>  	{OPT_VMWARE_TSC_MAP,    0, NULL,
>> OPT_VMWARE_TSC_MAP_NUM   },
>>  	{OPT_LEGACY_MEM,        0, NULL, OPT_LEGACY_MEM_NUM       },
>>  	{OPT_SINGLE_FILE_SEGMENTS, 0, NULL,
>> OPT_SINGLE_FILE_SEGMENTS_NUM},
>> +	{OPT_PCI_DEVARGS,       1, NULL, OPT_PCI_DEVARGS_NUM},
>>  	{0,                     0, NULL, 0                        }
>>  };
>>
>> @@ -1224,6 +1225,12 @@ eal_parse_common_option(int opt, const char
>> *optarg,
>>  	case OPT_SINGLE_FILE_SEGMENTS_NUM:
>>  		conf->single_file_segments = 1;
>>  		break;
>> +	case OPT_PCI_DEVARGS_NUM:
>> +		if (eal_option_device_add(RTE_DEVTYPE_NORMAL,
>> +				optarg) < 0) {
>> +			return -1;
>> +		}
>> +		break;
>>
>>  	/* don't know what to do, leave this to caller */
>>  	default:
>> @@ -1360,6 +1367,8 @@ eal_common_usage(void)
>>  	       "  --"OPT_VDEV"              Add a virtual device.\n"
>>  	       "                      The argument format is <driver><id>[,key=val,...]\n"
>>  	       "                      (ex: --vdev=net_pcap0,iface=eth2).\n"
>> +	       "  --"OPT_PCI_DEVARGS"          Pass key-value arguments to a pci
>> device\n"
>> +	       "                      The argument format is
>> <domain:bus:devid.func>[,key=val,...]\n"
>>  	       "  -d LIB.so|DIR       Add a driver or driver directory\n"
>>  	       "                      (can be used multiple times)\n"
>>  	       "  --"OPT_VMWARE_TSC_MAP"    Use VMware TSC map instead
>> of native RDTSC\n"
>> diff --git a/lib/librte_eal/common/eal_options.h
>> b/lib/librte_eal/common/eal_options.h
>> index 211ae06ae..d52d38e32 100644
>> --- a/lib/librte_eal/common/eal_options.h
>> +++ b/lib/librte_eal/common/eal_options.h
>> @@ -59,6 +59,8 @@ enum {
>>  	OPT_LEGACY_MEM_NUM,
>>  #define OPT_SINGLE_FILE_SEGMENTS    "single-file-segments"
>>  	OPT_SINGLE_FILE_SEGMENTS_NUM,
>> +#define OPT_PCI_DEVARGS       "pci-args"
>> +	OPT_PCI_DEVARGS_NUM,
>>  	OPT_LONG_MAX_NUM
>>  };
>>
>> diff --git a/lib/librte_eal/common/include/rte_dev.h
>> b/lib/librte_eal/common/include/rte_dev.h
>> index 3879ff3ca..fb3e5033f 100644
>> --- a/lib/librte_eal/common/include/rte_dev.h
>> +++ b/lib/librte_eal/common/include/rte_dev.h
>> @@ -124,6 +124,7 @@ enum rte_kernel_driver {
>>   * Device policies.
>>   */
>>  enum rte_dev_policy {
>> +	RTE_DEV_NORMAL,
>>  	RTE_DEV_WHITELISTED,
>>  	RTE_DEV_BLACKLISTED,
>>  };
>> diff --git a/lib/librte_eal/common/include/rte_devargs.h
>> b/lib/librte_eal/common/include/rte_devargs.h
>> index 58fbd90a2..78c600bf2 100644
>> --- a/lib/librte_eal/common/include/rte_devargs.h
>> +++ b/lib/librte_eal/common/include/rte_devargs.h
>> @@ -29,6 +29,7 @@ extern "C" {
>>   * Type of generic device
>>   */
>>  enum rte_devtype {
>> +	RTE_DEVTYPE_NORMAL, /* Normal dev with special pci args */
> 
> What is "Normal" device? Can we find a better name? 
> 
>>  	RTE_DEVTYPE_WHITELISTED_PCI,
>>  	RTE_DEVTYPE_BLACKLISTED_PCI,
>>  	RTE_DEVTYPE_VIRTUAL,
>> --
>> 2.17.1
> 

Re: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev args

[Gaëtan Rivet]

Hi Ferruh, Pavan,

sorry for the delay,

On Tue, Jun 26, 2018 at 04:40:21PM +0100, Ferruh Yigit wrote:
> On 6/26/2018 1:48 PM, Shahaf Shuler wrote:
> > Hi Pavan,
> > 
> > Friday, June 15, 2018 7:44 AM, Pavan Nikhilesh:
> >> Subject: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev
> >> args
> >>
> >> Currently, the only way of supplying device argument to a pci device is to
> >> whitelist it i.e. -w 000X:00:0X.0,self_test=1. This is not a very feasible method
> >> as whitelisting a device has its own side effects i.e only the whitelisted pci
> >> devices are probed.
> >>
> >> Add a new eal command line option --pci-args to pass device args without the
> >> need to whitelist the devices.
> >> 		--pci-args 000X:00:0X.0,self_test=1
> >>
> >> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > 
> > Tested-by: Shahaf Shuler <shahafs@mellanox.com>
> > 
> > It seems to work. 
> > Please see small comments below
> 
> Isn't this conflict with Gaetan's devarg work which has wider scope?
> 

Indeed it does.

Pavan, I have submitted a new version of a series adding generic kvargs
to several layers (bus, class, driver).

It does cover this exact use-case.

However, while writing it, I wasn't able to find PCI bus specific
parameters, that could showcase the functionality.

It would help the development if you could provide which parameter you
wanted to implement, I could add it in my own series, which would
streamline all of this.

Regards,
-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev args

[Pavan Nikhilesh]

Hi Gaëtan,

On Wed, Jun 27, 2018 at 10:39:59AM +0200, Gaëtan Rivet wrote:
> Hi Ferruh, Pavan,
>
> sorry for the delay,
>
> On Tue, Jun 26, 2018 at 04:40:21PM +0100, Ferruh Yigit wrote:
> > On 6/26/2018 1:48 PM, Shahaf Shuler wrote:
> > > Hi Pavan,
> > >
> > > Friday, June 15, 2018 7:44 AM, Pavan Nikhilesh:
> > >> Subject: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev
> > >> args
> > >>
> > >> Currently, the only way of supplying device argument to a pci device is to
> > >> whitelist it i.e. -w 000X:00:0X.0,self_test=1. This is not a very feasible method
> > >> as whitelisting a device has its own side effects i.e only the whitelisted pci
> > >> devices are probed.
> > >>
> > >> Add a new eal command line option --pci-args to pass device args without the
> > >> need to whitelist the devices.
> > >>            --pci-args 000X:00:0X.0,self_test=1
> > >>
> > >> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > >
> > > Tested-by: Shahaf Shuler <shahafs@mellanox.com>
> > >
> > > It seems to work.
> > > Please see small comments below
> >
> > Isn't this conflict with Gaetan's devarg work which has wider scope?
> >
>
> Indeed it does.
>
> Pavan, I have submitted a new version of a series adding generic kvargs
> to several layers (bus, class, driver).
>
> It does cover this exact use-case.
>
> However, while writing it, I wasn't able to find PCI bus specific
> parameters, that could showcase the functionality.

The idea of the patch is to avoid whitelising a device when we want to
supply kvargs to it, I tried mapping it to devargs rework patchset but couldn't
do it at a glance. For example, the following patch[1] reads kvargs through
whitelisting which should be avoided.

[1]http://patches.dpdk.org/patch/41223/

>
> It would help the development if you could provide which parameter you
> wanted to implement, I could add it in my own series, which would
> streamline all of this.

+1

>
> Regards,
> --
> Gaëtan Rivet
> 6WIND

Thanks,
Pavan.

Re: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev args

[Gaëtan Rivet]

On Wed, Jun 27, 2018 at 02:25:30PM +0530, Pavan Nikhilesh wrote:
> Hi Gaëtan,
> 
> On Wed, Jun 27, 2018 at 10:39:59AM +0200, Gaëtan Rivet wrote:
> > Hi Ferruh, Pavan,
> >
> > sorry for the delay,
> >
> > On Tue, Jun 26, 2018 at 04:40:21PM +0100, Ferruh Yigit wrote:
> > > On 6/26/2018 1:48 PM, Shahaf Shuler wrote:
> > > > Hi Pavan,
> > > >
> > > > Friday, June 15, 2018 7:44 AM, Pavan Nikhilesh:
> > > >> Subject: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev
> > > >> args
> > > >>
> > > >> Currently, the only way of supplying device argument to a pci device is to
> > > >> whitelist it i.e. -w 000X:00:0X.0,self_test=1. This is not a very feasible method
> > > >> as whitelisting a device has its own side effects i.e only the whitelisted pci
> > > >> devices are probed.
> > > >>
> > > >> Add a new eal command line option --pci-args to pass device args without the
> > > >> need to whitelist the devices.
> > > >>            --pci-args 000X:00:0X.0,self_test=1
> > > >>
> > > >> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > > >
> > > > Tested-by: Shahaf Shuler <shahafs@mellanox.com>
> > > >
> > > > It seems to work.
> > > > Please see small comments below
> > >
> > > Isn't this conflict with Gaetan's devarg work which has wider scope?
> > >
> >
> > Indeed it does.
> >
> > Pavan, I have submitted a new version of a series adding generic kvargs
> > to several layers (bus, class, driver).
> >
> > It does cover this exact use-case.
> >
> > However, while writing it, I wasn't able to find PCI bus specific
> > parameters, that could showcase the functionality.
> 
> The idea of the patch is to avoid whitelising a device when we want to
> supply kvargs to it, I tried mapping it to devargs rework patchset but couldn't
> do it at a glance. For example, the following patch[1] reads kvargs through
> whitelisting which should be avoided.
> 
> [1]http://patches.dpdk.org/patch/41223/
> 

I see.

Actually, your use-case won't be covered by the devargs rework.

I am still dumbfounded by how this blacklist/whitelist mode stuff is
kept against all odds. But that's not the time to deal with it.

The issue is that the two features "declaring a device" and
"configuring a bus" are currently awkwardly merged. You are piling stuff
on the "declaring a device" part to enhance the "configuring a bus"
feature.

Instead of going this way, I would advise to separate the two features.

If buses could be configured with a generic EAL option
"--blacklist=pci,vdev" for example, then you could provide devargs as
much as you want, the buses themselves would stay properly configured.

This means removing devargs policy, device types and rewriting bus logic
about it.

Regards,
-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev args

[Pavan Nikhilesh]

Hi Gaëtan,Ferruh,

On Wed, Jun 27, 2018 at 11:57:36AM +0200, Gaëtan Rivet wrote:
> On Wed, Jun 27, 2018 at 02:25:30PM +0530, Pavan Nikhilesh wrote:
> > Hi Gaëtan,
> >
> > On Wed, Jun 27, 2018 at 10:39:59AM +0200, Gaëtan Rivet wrote:
> > > Hi Ferruh, Pavan,
> > >
> > > sorry for the delay,
> > >
> > > On Tue, Jun 26, 2018 at 04:40:21PM +0100, Ferruh Yigit wrote:
> > > > On 6/26/2018 1:48 PM, Shahaf Shuler wrote:
> > > > > Hi Pavan,
> > > > >
> > > > > Friday, June 15, 2018 7:44 AM, Pavan Nikhilesh:
> > > > >> Subject: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev
> > > > >> args
> > > > >>
> > > > >> Currently, the only way of supplying device argument to a pci device is to
> > > > >> whitelist it i.e. -w 000X:00:0X.0,self_test=1. This is not a very feasible method
> > > > >> as whitelisting a device has its own side effects i.e only the whitelisted pci
> > > > >> devices are probed.
> > > > >>
> > > > >> Add a new eal command line option --pci-args to pass device args without the
> > > > >> need to whitelist the devices.
> > > > >>            --pci-args 000X:00:0X.0,self_test=1
> > > > >>
> > > > >> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > > > >
> > > > > Tested-by: Shahaf Shuler <shahafs@mellanox.com>
> > > > >
> > > > > It seems to work.
> > > > > Please see small comments below
> > > >
> > > > Isn't this conflict with Gaetan's devarg work which has wider scope?
> > > >
> > >
> > > Indeed it does.
> > >
> > > Pavan, I have submitted a new version of a series adding generic kvargs
> > > to several layers (bus, class, driver).
> > >
> > > It does cover this exact use-case.
> > >
> > > However, while writing it, I wasn't able to find PCI bus specific
> > > parameters, that could showcase the functionality.
> >
> > The idea of the patch is to avoid whitelising a device when we want to
> > supply kvargs to it, I tried mapping it to devargs rework patchset but couldn't
> > do it at a glance. For example, the following patch[1] reads kvargs through
> > whitelisting which should be avoided.
> >
> > [1]http://patches.dpdk.org/patch/41223/
> >
>
> I see.
>
> Actually, your use-case won't be covered by the devargs rework.
>
> I am still dumbfounded by how this blacklist/whitelist mode stuff is
> kept against all odds. But that's not the time to deal with it.
>
> The issue is that the two features "declaring a device" and
> "configuring a bus" are currently awkwardly merged. You are piling stuff
> on the "declaring a device" part to enhance the "configuring a bus"
> feature.

The feature is very much needed to avoid polluting the cmdline args when we are
trying to configure a device at probe (for now).

>
> Instead of going this way, I would advise to separate the two features.
>
> If buses could be configured with a generic EAL option
> "--blacklist=pci,vdev" for example, then you could provide devargs as
> much as you want, the buses themselves would stay properly configured.
>
> This means removing devargs policy, device types and rewriting bus logic
> about it.

I think this can be done as a future work and is not in the scope of this
patch.

@Ferruh,
As Gaëtan mentioned this patch is not related to devargs rework can we make
some forward progress.

>
> Regards,
> --
> Gaëtan Rivet
> 6WIND

Thanks,
Pavan.

Re: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev args

[Thomas Monjalon]

10/07/2018 12:19, Pavan Nikhilesh:
> Hi Gaëtan,Ferruh,
> 
> On Wed, Jun 27, 2018 at 11:57:36AM +0200, Gaëtan Rivet wrote:
> > On Wed, Jun 27, 2018 at 02:25:30PM +0530, Pavan Nikhilesh wrote:
> > > Hi Gaëtan,
> > >
> > > On Wed, Jun 27, 2018 at 10:39:59AM +0200, Gaëtan Rivet wrote:
> > > > Hi Ferruh, Pavan,
> > > >
> > > > sorry for the delay,
> > > >
> > > > On Tue, Jun 26, 2018 at 04:40:21PM +0100, Ferruh Yigit wrote:
> > > > > On 6/26/2018 1:48 PM, Shahaf Shuler wrote:
> > > > > > Hi Pavan,
> > > > > >
> > > > > > Friday, June 15, 2018 7:44 AM, Pavan Nikhilesh:
> > > > > >> Subject: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev
> > > > > >> args
> > > > > >>
> > > > > >> Currently, the only way of supplying device argument to a pci device is to
> > > > > >> whitelist it i.e. -w 000X:00:0X.0,self_test=1. This is not a very feasible method
> > > > > >> as whitelisting a device has its own side effects i.e only the whitelisted pci
> > > > > >> devices are probed.
> > > > > >>
> > > > > >> Add a new eal command line option --pci-args to pass device args without the
> > > > > >> need to whitelist the devices.
> > > > > >>            --pci-args 000X:00:0X.0,self_test=1
> > > > > >>
> > > > > >> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > > > > >
> > > > > > Tested-by: Shahaf Shuler <shahafs@mellanox.com>
> > > > > >
> > > > > > It seems to work.
> > > > > > Please see small comments below
> > > > >
> > > > > Isn't this conflict with Gaetan's devarg work which has wider scope?
> > > > >
> > > >
> > > > Indeed it does.
> > > >
> > > > Pavan, I have submitted a new version of a series adding generic kvargs
> > > > to several layers (bus, class, driver).
> > > >
> > > > It does cover this exact use-case.
> > > >
> > > > However, while writing it, I wasn't able to find PCI bus specific
> > > > parameters, that could showcase the functionality.
> > >
> > > The idea of the patch is to avoid whitelising a device when we want to
> > > supply kvargs to it, I tried mapping it to devargs rework patchset but couldn't
> > > do it at a glance. For example, the following patch[1] reads kvargs through
> > > whitelisting which should be avoided.
> > >
> > > [1]http://patches.dpdk.org/patch/41223/
> > >
> >
> > I see.
> >
> > Actually, your use-case won't be covered by the devargs rework.
> >
> > I am still dumbfounded by how this blacklist/whitelist mode stuff is
> > kept against all odds. But that's not the time to deal with it.
> >
> > The issue is that the two features "declaring a device" and
> > "configuring a bus" are currently awkwardly merged. You are piling stuff
> > on the "declaring a device" part to enhance the "configuring a bus"
> > feature.
> 
> The feature is very much needed to avoid polluting the cmdline args when we are
> trying to configure a device at probe (for now).
> 
> >
> > Instead of going this way, I would advise to separate the two features.
> >
> > If buses could be configured with a generic EAL option
> > "--blacklist=pci,vdev" for example, then you could provide devargs as
> > much as you want, the buses themselves would stay properly configured.
> >
> > This means removing devargs policy, device types and rewriting bus logic
> > about it.
> 
> I think this can be done as a future work and is not in the scope of this
> patch.
> 
> @Ferruh,
> As Gaëtan mentioned this patch is not related to devargs rework can we make
> some forward progress.

No, we should not add a new parameter just to fix one use case for one bus.
The work of Gaetan is opening the door to a generic syntax which can be
used for device matching (like for whitelisting), or for settings
(what you need) of any bus, any device class or any driver.
We can discuss about which option to add for generic device settings,
and whether or not it should be mixed with whitelisting,
but please let's work on a generic solution.

Re: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev args

[Pavan Nikhilesh]

On Mon, Jul 16, 2018 at 12:25:29AM +0200, Thomas Monjalon wrote:
> External Email
>
> 10/07/2018 12:19, Pavan Nikhilesh:
> > Hi Gaëtan,Ferruh,
> >
> > On Wed, Jun 27, 2018 at 11:57:36AM +0200, Gaëtan Rivet wrote:
> > > On Wed, Jun 27, 2018 at 02:25:30PM +0530, Pavan Nikhilesh wrote:
> > > > Hi Gaëtan,
> > > >
> > > > On Wed, Jun 27, 2018 at 10:39:59AM +0200, Gaëtan Rivet wrote:
> > > > > Hi Ferruh, Pavan,
> > > > >
> > > > > sorry for the delay,
> > > > >
> > > > > On Tue, Jun 26, 2018 at 04:40:21PM +0100, Ferruh Yigit wrote:
> > > > > > On 6/26/2018 1:48 PM, Shahaf Shuler wrote:
> > > > > > > Hi Pavan,
> > > > > > >
> > > > > > > Friday, June 15, 2018 7:44 AM, Pavan Nikhilesh:
> > > > > > >> Subject: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev
> > > > > > >> args
> > > > > > >>
> > > > > > >> Currently, the only way of supplying device argument to a pci device is to
> > > > > > >> whitelist it i.e. -w 000X:00:0X.0,self_test=1. This is not a very feasible method
> > > > > > >> as whitelisting a device has its own side effects i.e only the whitelisted pci
> > > > > > >> devices are probed.
> > > > > > >>
> > > > > > >> Add a new eal command line option --pci-args to pass device args without the
> > > > > > >> need to whitelist the devices.
> > > > > > >>            --pci-args 000X:00:0X.0,self_test=1
> > > > > > >>
> > > > > > >> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > > > > > >
> > > > > > > Tested-by: Shahaf Shuler <shahafs@mellanox.com>
> > > > > > >
> > > > > > > It seems to work.
> > > > > > > Please see small comments below
> > > > > >
> > > > > > Isn't this conflict with Gaetan's devarg work which has wider scope?
> > > > > >
> > > > >
> > > > > Indeed it does.
> > > > >
> > > > > Pavan, I have submitted a new version of a series adding generic kvargs
> > > > > to several layers (bus, class, driver).
> > > > >
> > > > > It does cover this exact use-case.
> > > > >
> > > > > However, while writing it, I wasn't able to find PCI bus specific
> > > > > parameters, that could showcase the functionality.
> > > >
> > > > The idea of the patch is to avoid whitelising a device when we want to
> > > > supply kvargs to it, I tried mapping it to devargs rework patchset but couldn't
> > > > do it at a glance. For example, the following patch[1] reads kvargs through
> > > > whitelisting which should be avoided.
> > > >
> > > > [1]http://patches.dpdk.org/patch/41223/
> > > >
> > >
> > > I see.
> > >
> > > Actually, your use-case won't be covered by the devargs rework.
> > >
> > > I am still dumbfounded by how this blacklist/whitelist mode stuff is
> > > kept against all odds. But that's not the time to deal with it.
> > >
> > > The issue is that the two features "declaring a device" and
> > > "configuring a bus" are currently awkwardly merged. You are piling stuff
> > > on the "declaring a device" part to enhance the "configuring a bus"
> > > feature.
> >
> > The feature is very much needed to avoid polluting the cmdline args when we are
> > trying to configure a device at probe (for now).
> >
> > >
> > > Instead of going this way, I would advise to separate the two features.
> > >
> > > If buses could be configured with a generic EAL option
> > > "--blacklist=pci,vdev" for example, then you could provide devargs as
> > > much as you want, the buses themselves would stay properly configured.
> > >
> > > This means removing devargs policy, device types and rewriting bus logic
> > > about it.
> >
> > I think this can be done as a future work and is not in the scope of this
> > patch.
> >
> > @Ferruh,
> > As Gaëtan mentioned this patch is not related to devargs rework can we make
> > some forward progress.
>
> No, we should not add a new parameter just to fix one use case for one bus.
> The work of Gaetan is opening the door to a generic syntax which can be
> used for device matching (like for whitelisting), or for settings
> (what you need) of any bus, any device class or any driver.
> We can discuss about which option to add for generic device settings,
> and whether or not it should be mixed with whitelisting,
> but please let's work on a generic solution.

Ok, I guess this can be taken up once Gaëtan devarsg patches are completely
merged. If we split the work into a smaller list we could load balance the
work and work towards 18.11?.

>
>

Re: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev args

[Thomas Monjalon]

16/07/2018 13:05, Pavan Nikhilesh:
> On Mon, Jul 16, 2018 at 12:25:29AM +0200, Thomas Monjalon wrote:
> > External Email
> >
> > 10/07/2018 12:19, Pavan Nikhilesh:
> > > Hi Gaëtan,Ferruh,
> > >
> > > On Wed, Jun 27, 2018 at 11:57:36AM +0200, Gaëtan Rivet wrote:
> > > > On Wed, Jun 27, 2018 at 02:25:30PM +0530, Pavan Nikhilesh wrote:
> > > > > Hi Gaëtan,
> > > > >
> > > > > On Wed, Jun 27, 2018 at 10:39:59AM +0200, Gaëtan Rivet wrote:
> > > > > > Hi Ferruh, Pavan,
> > > > > >
> > > > > > sorry for the delay,
> > > > > >
> > > > > > On Tue, Jun 26, 2018 at 04:40:21PM +0100, Ferruh Yigit wrote:
> > > > > > > On 6/26/2018 1:48 PM, Shahaf Shuler wrote:
> > > > > > > > Hi Pavan,
> > > > > > > >
> > > > > > > > Friday, June 15, 2018 7:44 AM, Pavan Nikhilesh:
> > > > > > > >> Subject: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev
> > > > > > > >> args
> > > > > > > >>
> > > > > > > >> Currently, the only way of supplying device argument to a pci device is to
> > > > > > > >> whitelist it i.e. -w 000X:00:0X.0,self_test=1. This is not a very feasible method
> > > > > > > >> as whitelisting a device has its own side effects i.e only the whitelisted pci
> > > > > > > >> devices are probed.
> > > > > > > >>
> > > > > > > >> Add a new eal command line option --pci-args to pass device args without the
> > > > > > > >> need to whitelist the devices.
> > > > > > > >>            --pci-args 000X:00:0X.0,self_test=1
> > > > > > > >>
> > > > > > > >> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > > > > > > >
> > > > > > > > Tested-by: Shahaf Shuler <shahafs@mellanox.com>
> > > > > > > >
> > > > > > > > It seems to work.
> > > > > > > > Please see small comments below
> > > > > > >
> > > > > > > Isn't this conflict with Gaetan's devarg work which has wider scope?
> > > > > > >
> > > > > >
> > > > > > Indeed it does.
> > > > > >
> > > > > > Pavan, I have submitted a new version of a series adding generic kvargs
> > > > > > to several layers (bus, class, driver).
> > > > > >
> > > > > > It does cover this exact use-case.
> > > > > >
> > > > > > However, while writing it, I wasn't able to find PCI bus specific
> > > > > > parameters, that could showcase the functionality.
> > > > >
> > > > > The idea of the patch is to avoid whitelising a device when we want to
> > > > > supply kvargs to it, I tried mapping it to devargs rework patchset but couldn't
> > > > > do it at a glance. For example, the following patch[1] reads kvargs through
> > > > > whitelisting which should be avoided.
> > > > >
> > > > > [1]http://patches.dpdk.org/patch/41223/
> > > > >
> > > >
> > > > I see.
> > > >
> > > > Actually, your use-case won't be covered by the devargs rework.
> > > >
> > > > I am still dumbfounded by how this blacklist/whitelist mode stuff is
> > > > kept against all odds. But that's not the time to deal with it.
> > > >
> > > > The issue is that the two features "declaring a device" and
> > > > "configuring a bus" are currently awkwardly merged. You are piling stuff
> > > > on the "declaring a device" part to enhance the "configuring a bus"
> > > > feature.
> > >
> > > The feature is very much needed to avoid polluting the cmdline args when we are
> > > trying to configure a device at probe (for now).
> > >
> > > >
> > > > Instead of going this way, I would advise to separate the two features.
> > > >
> > > > If buses could be configured with a generic EAL option
> > > > "--blacklist=pci,vdev" for example, then you could provide devargs as
> > > > much as you want, the buses themselves would stay properly configured.
> > > >
> > > > This means removing devargs policy, device types and rewriting bus logic
> > > > about it.
> > >
> > > I think this can be done as a future work and is not in the scope of this
> > > patch.
> > >
> > > @Ferruh,
> > > As Gaëtan mentioned this patch is not related to devargs rework can we make
> > > some forward progress.
> >
> > No, we should not add a new parameter just to fix one use case for one bus.
> > The work of Gaetan is opening the door to a generic syntax which can be
> > used for device matching (like for whitelisting), or for settings
> > (what you need) of any bus, any device class or any driver.
> > We can discuss about which option to add for generic device settings,
> > and whether or not it should be mixed with whitelisting,
> > but please let's work on a generic solution.
> 
> Ok, I guess this can be taken up once Gaëtan devarsg patches are completely
> merged. If we split the work into a smaller list we could load balance the
> work and work towards 18.11?.

Yes, we must split and share the work.
Some patches from Gaetan are in 18.08 in order to provide some parsing helpers.
What we must do next:
- implement identification by bus properties
- implement identification by device class properties
- choose how we want to manage whitelist/blacklist
- choose syntax to set some properties (to be used by API or command line or config file)

Re: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev args

[Pavan Nikhilesh]

Hi Shahaf,

On Tue, Jun 26, 2018 at 12:48:49PM +0000, Shahaf Shuler wrote:
> Hi Pavan,
>
> Friday, June 15, 2018 7:44 AM, Pavan Nikhilesh:
> > Subject: [dpdk-dev] [PATCH v2] eal/devargs: add option to supply PCI dev
> > args
> >
> > Currently, the only way of supplying device argument to a pci device is to
> > whitelist it i.e. -w 000X:00:0X.0,self_test=1. This is not a very feasible method
> > as whitelisting a device has its own side effects i.e only the whitelisted pci
> > devices are probed.
> >
> > Add a new eal command line option --pci-args to pass device args without the
> > need to whitelist the devices.
> >               --pci-args 000X:00:0X.0,self_test=1
> >
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
>
> Tested-by: Shahaf Shuler <shahafs@mellanox.com>
>
> It seems to work.
> Please see small comments below
>
> > ---
> >  v2 Changes:
> >  - Document the option usage in eal_common_usage.
> >  - Update commit log to be more informative.
> >
> >  lib/librte_eal/common/eal_common_devargs.c  | 3 +++
> > lib/librte_eal/common/eal_common_options.c  | 9 +++++++++
> >  lib/librte_eal/common/eal_options.h         | 2 ++
> >  lib/librte_eal/common/include/rte_dev.h     | 1 +
> >  lib/librte_eal/common/include/rte_devargs.h | 1 +
> >  5 files changed, 16 insertions(+)
>
> Should we also update the manual of testpmd (doc/guides/testpmd_app_ug/run_app.rst ) for the new eal arg?

I was wondering where exactly this has to be documented, Thanks for pointing it
out I will add it in the next version.

>
> >
> >  };
> > diff --git a/lib/librte_eal/common/include/rte_devargs.h
> > b/lib/librte_eal/common/include/rte_devargs.h
> > index 58fbd90a2..78c600bf2 100644
> > --- a/lib/librte_eal/common/include/rte_devargs.h
> > +++ b/lib/librte_eal/common/include/rte_devargs.h
> > @@ -29,6 +29,7 @@ extern "C" {
> >   * Type of generic device
> >   */
> >  enum rte_devtype {
> > +     RTE_DEVTYPE_NORMAL, /* Normal dev with special pci args */
>
> What is "Normal" device? Can we find a better name?

Maybe something like RTE_DEVTYPE_PCI would fit in?. Let me know if you have any
suggestions.

>
> >       RTE_DEVTYPE_WHITELISTED_PCI,
> >       RTE_DEVTYPE_BLACKLISTED_PCI,
> >       RTE_DEVTYPE_VIRTUAL,
> > --
> > 2.17.1
>

Thanks,
Pavan.