Re: [dpdk-dev] Making rte_eal_pci_probe() in rte_eal_init() optional?

["Roger B. Melton" <rmelton@cisco.com>]

David/Thomas,  in-line  -Roger


On 11/18/15 5:13 PM, Roger B. Melton wrote:
> Hi Thomas, in-line  -Roger
>
> On 11/17/15 10:46 AM, Thomas Monjalon wrote:
>> 2015-11-17 08:56, Roger B. Melton:
>>> Hi David,  in-line -Roger
>>>
>>> On 11/16/15 4:46 AM, David Marchand wrote:
>>>> Hello Roger,
>>>>
>>>> On Sun, Nov 15, 2015 at 3:45 PM, Roger B. Melton <rmelton@cisco.com
>>>> <mailto:rmelton@cisco.com>> wrote:
>>>>
>>>>      I like the "-b all" and "-w none" idea, but I think it might be
>>>>      complicated to implement it the way we would need it to work.  
>>>> The
>>>>      existing -b and -w options  persist for the duration of the
>>>>      application, and we would need the "-b all"/"-w none" to persists
>>>>      only through rte_eal_init() time.  Otherwise our attempt to to
>>>>      attach a device at a later time would be blocked by the option.
>>>>
>>>> I agree, the black/white lists should only apply to initial scan.
>>>> I forgot about this problem ...
>>>> I had started some cleanup in the pci scan / attach code but this is
>>>> too late for 2.2, I will post this in the next merge window.
>>>>
>>>>
>>>>      Wouldn't it be simpler to have an option to disable the
>>>>      rte_eal_init() time the probe.  Would that address the issue with
>>>>      VFIO, prevent automatically attaching to devices while permitting
>>>>      on demand attach?
>>>>
>>>>
>>>> I suppose we can do this yes (I think Thomas once proposed off-list an
>>>> option like --no-pci-scan).
>>>> Do you think you can send a patch ?
>>> What about --no-pci-init-probe?  I know it's long, but it is more
>>> descriptive of it's purpose to disable only the init time pci probe.
>> Why not a "-b all"?
>> Making it work would also solve the case where you to scan only part of
>> the devices and initialize the blacklisted ones later.
>> .
>>
> Do you envision "-b all" setting a flag that would be used to block 
> rte_eal_init() invocation of rte_eal_pci_probe()?  e.g. If we have a 
> new API, *rte_eal_pci_blacklist_all_get()* that returns a non-zero 
> value if "-b all" was specified, then in rte_eal_init() we would have 
> something like:
>
> ...
>     /* Probe & Initialize PCI devices */
> *    if (!rte_eal_pci_blacklist_all_get())**  <--- New check*
>         if (rte_eal_pci_probe())
>             rte_panic("Cannot probe PCI\n");
> ...
>
>
> Or setting a flag that would be checked in rte_eal_probe_one() similar 
> to the existing per device blacklist check?  e.g. Again with a new 
> API, *rte_eal_pci_blacklist_all_get()* that returns a non-zero value 
> if "-b all" was specified, then in rte_eal_pci_probe_one_driver() we 
> would have something like:
>
>         /* no initialization when blacklisted, return without error */
>         if (*rte_eal_pci_blacklist_all_get() ||  <--- New check*
>             (dev->devargs != NULL &&
>              dev->devargs->type == RTE_DEVTYPE_BLACKLISTED_PCI)) {
>             RTE_LOG(DEBUG, EAL, "  Device is blacklisted, not 
> initializing\n");
>             return 1;
>         }
>
> The former would work, but I think it would be confusing for "-b all" 
> to only apply to init.
>
> The latter would be consistent with how "-b <PCI DBDF>" works, but in 
> order to initialize devices at a later time, we would need a way to 
> clear the blacklist all state at run time so that 
> *rte_eal_pci_blacklist_all()* would return false. For example, 
> something like *rte_eal_pci_blacklist_all_clear()*.
>
> Or do you have something else in mind entirely?
>
> .
>

How about something like this:

  * Add pci_blacklist_all member to eal internal config.
  * When non-zero, pci_blacklist_all will prevent PCI probing. Result is
    that probes will find no match.
  * pci_blacklist_all is temporal
      o Can be set at rte_eal_init() time with "-b all" switch.
      o Can be set or cleared at run time with
        rte_eal_pci_blacklist_all_set() or
        rte_eal_pci_blacklist_all_clear() respectively.
      o State can be queried with rte_eal_pci_blacklist_all_get()
  * Blacklist and whitelist switches with PCI DBDF are still permitted
    as before, but they are used only when the blacklist all override is
    cleared.

I've tested the implementation from the diff below in our application 
and it works well.  Note our application falls under linuxapp.  I 
haven't tested bsdapp yet.

If it looks good to you, I will update test application and update if 
necessary with logic to test the capability, and complete testing of the 
bsdapp.

Thanks,  -Roger

On branch DPDK2.2Integration
Changes not staged for commit:
   (use "git add <file>..." to update what will be committed)
   (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   lib/librte_eal/bsdapp/eal/eal.c
	modified:   lib/librte_eal/common/eal_common_options.c
	modified:   lib/librte_eal/common/eal_common_pci.c
	modified:   lib/librte_eal/common/include/rte_eal.h
	modified:   lib/librte_eal/linuxapp/eal/eal.c

no changes added to commit (use "git add" and/or "git commit -a")
  .



diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index b64bbfc..0218434 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -118,6 +118,31 @@ rte_eal_get_configuration(void)
  	return &rte_config;
  }
  
+/* Get PCI blacklist all state */
+uint32_t
+rte_eal_pci_blacklist_all_get (void)
+{
+	return internal_config.pci_blacklist_all;
+}
+
+/* Clear PCI blacklist all state */
+void
+rte_eal_pci_blacklist_all_clear (void)
+{
+	internal_config.pci_blacklist_all = 0;
+	RTE_LOG(DEBUG, EAL, "PCI blacklist all override removed\n");
+	return;
+}
+
+/* Set PCI blacklist all state */
+void
+rte_eal_pci_blacklist_all_set (void)
+{
+	internal_config.pci_blacklist_all = 1;
+	RTE_LOG(DEBUG, EAL, "PCI blacklist all override applied\n");
+	return;
+}
+
  /* parse a sysfs (or other) file containing one integer value */
  int
  eal_parse_sysfs_value(const char *filename, unsigned long *val)
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 79db608..c01c315 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -655,7 +655,9 @@ eal_parse_common_option(int opt, const char *optarg,
  	switch (opt) {
  	/* blacklist */
  	case 'b':
-		if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI,
+		if (!strcmp(optarg,"all")) {
+			rte_eal_pci_blacklist_all_set();
+		} else if (rte_eal_devargs_add(RTE_DEVTYPE_BLACKLISTED_PCI,
  				optarg) < 0) {
  			return -1;
  		}
@@ -886,6 +888,7 @@ eal_common_usage(void)
  	       "  -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"
+	       "                      '-b all' is a special case used to blacklist all devices\n"
  	       "  -w, --"OPT_PCI_WHITELIST" Add a PCI device in white list.\n"
  	       "                      Only use the specified PCI devices. The argument format\n"
  	       "                      is <[domain:]bus:devid.func>. This option can be present\n"
diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index dcfe947..2cd789e 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -147,6 +147,10 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
  	int ret;
  	const struct rte_pci_id *id_table;
  
+	/* If all PCI devices are blacklisted, don't probe */
+	if (rte_eal_pci_blacklist_all_get())
+		return 1;
+
  	for (id_table = dr->id_table; id_table->vendor_id != 0; id_table++) {
  
  		/* check if device's identifiers match the driver's ones */
diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
index d2816a8..92216aa 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -100,6 +100,31 @@ struct rte_config {
  struct rte_config *rte_eal_get_configuration(void);
  
  /**
+ * Get PCI blacklist all state
+ *
+ * @ return
+ *   0 if PCI blacklist all is disabled.
+ *   Non-zero if PCI blacklist all is enabled.
+ */
+uint32_t rte_eal_pci_blacklist_all_get (void);
+
+/**
+ * Clear PCI blacklist all state
+ *
+ * @ return
+ * void
+ */
+void rte_eal_pci_blacklist_all_clear (void);
+
+/**
+ * Set PCI blacklist all state
+ *
+ * @ return
+ * void
+ */
+void rte_eal_pci_blacklist_all_set (void);
+
+/**
   * Get a lcore's role.
   *
   * @param lcore_id
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 18fe19b..7ad776b 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -139,6 +139,31 @@ rte_eal_get_configuration(void)
  	return &rte_config;
  }
  
+/* Get PCI blacklist all state */
+uint32_t
+rte_eal_pci_blacklist_all_get (void)
+{
+	return internal_config.pci_blacklist_all;
+}
+
+/* Clear PCI blacklist all state */
+void
+rte_eal_pci_blacklist_all_clear (void)
+{
+	internal_config.pci_blacklist_all = 0;
+	RTE_LOG(DEBUG, EAL, "PCI blacklist all override removed\n");
+	return;
+}
+
+/* Set PCI blacklist all state */
+void
+rte_eal_pci_blacklist_all_set (void)
+{
+	internal_config.pci_blacklist_all = 1;
+	RTE_LOG(DEBUG, EAL, "PCI blacklist all override applied\n");
+	return;
+}
+
  /* parse a sysfs (or other) file containing one integer value */
  int
  eal_parse_sysfs_value(const char *filename, unsigned long *val)
.