[PATCH v1 0/8] Bus control framework

[PATCH v1 0/8] Bus control framework

[Gaetan Rivet]

Probing policy was introduced in the previous release as a configuration item.
It was thus added to the generic bus structure, breaking its ABI.

In this release, the IOVA mode can be read from a bus to configure the
EAL. This new configuration element also broke the bus ABI when it was
added.

As new operators had to be implemented for the probe policy item, these
patches were developed to help mitigate this issue.

This control framework allows to expand the rte_bus API without breaking
its ABI. It is meant to be used with configuration elements that may
only be valid for a few buses, while the others would remain untouched
and unaware of the evolution.

A central control operator is used, similarly to the working of rte_flow
API in the ether layer. Each driver thus chooses to expose a set of
operators relevant to its implementation. The caller is then free to use
those if they are available.

Both Probe mode and IOVA mode operators are implemented for the PCI bus.

This patchset depends on:

Move PCI away from the EAL
http://dpdk.org/ml/archives/dev/2017-August/073512.html

Gaetan Rivet (8):
  bus: rename scan policy as probe policy
  bus: introduce opaque control framework
  bus: remove probe mode configuration structure
  bus: add probe mode setter
  bus/pci: implement ctrl operator
  bus: add IOVA mode as a ctrl operation
  bus/pci: implement IOVA mode getter
  bus: remove redundant IOVA mode getter

 drivers/bus/pci/bsd/pci.c                       |   9 +-
 drivers/bus/pci/include/rte_bus_pci.h           |  12 +--
 drivers/bus/pci/linux/pci.c                     |  20 ++--
 drivers/bus/pci/pci_common.c                    |  53 +++++++++-
 drivers/bus/pci/private.h                       |  13 +++
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |   1 -
 lib/librte_eal/common/eal_common_bus.c          |  57 +++++++++--
 lib/librte_eal/common/eal_common_devargs.c      |   8 --
 lib/librte_eal/common/eal_common_options.c      |  17 +---
 lib/librte_eal/common/include/rte_bus.h         | 127 ++++++++++++++++--------
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |   1 -
 11 files changed, 222 insertions(+), 96 deletions(-)

-- 
2.1.4

[PATCH v1 1/8] bus: rename scan policy as probe policy

[Gaetan Rivet]

This bus configuration item is misnamed, as it actually refers to the
probing process.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 drivers/bus/pci/pci_common.c               |  2 +-
 lib/librte_eal/common/eal_common_devargs.c |  6 +++---
 lib/librte_eal/common/include/rte_bus.h    | 12 ++++++------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index d7a1c05..cc23a39 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -373,7 +373,7 @@ rte_pci_probe(void)
 	int probe_all = 0;
 	int ret = 0;
 
-	if (rte_pci_bus.bus.conf.scan_mode != RTE_BUS_SCAN_WHITELIST)
+	if (rte_pci_bus.bus.conf.probe_mode != RTE_BUS_PROBE_WHITELIST)
 		probe_all = 1;
 
 	FOREACH_DEVICE_ON_PCIBUS(dev) {
diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 6ac88d6..f5ef913 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -170,11 +170,11 @@ rte_eal_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;
-	if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) {
+	if (bus->conf.probe_mode == RTE_BUS_PROBE_UNDEFINED) {
 		if (devargs->policy == RTE_DEV_WHITELISTED)
-			bus->conf.scan_mode = RTE_BUS_SCAN_WHITELIST;
+			bus->conf.probe_mode = RTE_BUS_PROBE_WHITELIST;
 		else if (devargs->policy == RTE_DEV_BLACKLISTED)
-			bus->conf.scan_mode = RTE_BUS_SCAN_BLACKLIST;
+			bus->conf.probe_mode = RTE_BUS_PROBE_BLACKLIST;
 	}
 	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
 	return 0;
diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
index 6fb0834..331d954 100644
--- a/lib/librte_eal/common/include/rte_bus.h
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -168,19 +168,19 @@ typedef int (*rte_bus_unplug_t)(struct rte_device *dev);
 typedef int (*rte_bus_parse_t)(const char *name, void *addr);
 
 /**
- * Bus scan policies
+ * Bus probe policies
  */
-enum rte_bus_scan_mode {
-	RTE_BUS_SCAN_UNDEFINED,
-	RTE_BUS_SCAN_WHITELIST,
-	RTE_BUS_SCAN_BLACKLIST,
+enum rte_bus_probe_mode {
+	RTE_BUS_PROBE_UNDEFINED,
+	RTE_BUS_PROBE_WHITELIST,
+	RTE_BUS_PROBE_BLACKLIST,
 };
 
 /**
  * A structure used to configure bus operations.
  */
 struct rte_bus_conf {
-	enum rte_bus_scan_mode scan_mode; /**< Scan policy. */
+	enum rte_bus_probe_mode probe_mode; /**< Probe policy. */
 };
 
 
-- 
2.1.4

[PATCH v1 2/8] bus: introduce opaque control framework

[Gaetan Rivet]

New configuration elements are added to the buses. They make the ABI
unstable and will continue to do so.

This new control scheme allows to add new bus operators without
breaking the ABI and by only expanding the API.

This helps having more stability in core EAL subsystems, while allowing
flexibility for future evolutions.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_bus.c  |  9 +++++++
 lib/librte_eal/common/include/rte_bus.h | 46 +++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
index 3c66a02..65d7229 100644
--- a/lib/librte_eal/common/eal_common_bus.c
+++ b/lib/librte_eal/common/eal_common_bus.c
@@ -42,6 +42,13 @@
 struct rte_bus_list rte_bus_list =
 	TAILQ_HEAD_INITIALIZER(rte_bus_list);
 
+static rte_bus_ctrl_t
+rte_bus_default_ctrl(enum rte_bus_ctrl_op op __rte_unused,
+		     enum rte_bus_ctrl_item item __rte_unused)
+{
+	return NULL;
+}
+
 void
 rte_bus_register(struct rte_bus *bus)
 {
@@ -53,6 +60,8 @@ rte_bus_register(struct rte_bus *bus)
 	RTE_VERIFY(bus->find_device);
 	/* Buses supporting driver plug also require unplug. */
 	RTE_VERIFY(!bus->plug || bus->unplug);
+	if (bus->ctrl == NULL)
+		bus->ctrl = &rte_bus_default_ctrl;
 
 	TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
 	RTE_LOG(DEBUG, EAL, "Registered [%s] bus.\n", bus->name);
diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
index 331d954..bd3c28e 100644
--- a/lib/librte_eal/common/include/rte_bus.h
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -183,6 +183,51 @@ struct rte_bus_conf {
 	enum rte_bus_probe_mode probe_mode; /**< Probe policy. */
 };
 
+/**
+ * Bus configuration items.
+ */
+enum rte_bus_ctrl_item {
+	RTE_BUS_CTRL_PROBE_MODE = 0,
+	RTE_BUS_CTRL_ITEM_MAX,
+};
+
+/**
+ * Bus configuration operations.
+ */
+enum rte_bus_ctrl_op {
+	RTE_BUS_CTRL_GET = 0,
+	RTE_BUS_CTRL_SET,
+	RTE_BUS_CTRL_RESET,
+	RTE_BUS_CTRL_OP_MAX,
+};
+
+/**
+ * Operator for a particular rte_bus configuration item.
+ *
+ * @param arg
+ *    Operation parameter.
+ *
+ * @return
+ *	0 on success
+ *	!0 otherwise
+ */
+typedef int (*rte_bus_ctrl_t)(void *arg);
+
+/**
+ * Accessor to bus configuration operators.
+ *
+ * @param op
+ *	Operation type.
+ *
+ * @param item
+ *	Operation element.
+ *
+ * @return
+ *	Operator function on success.
+ *	NULL if this item is not supported.
+ */
+typedef rte_bus_ctrl_t (*rte_bus_ctrl_get_t)(enum rte_bus_ctrl_op op,
+					     enum rte_bus_ctrl_item item);
 
 /**
  * Get common iommu class of the all the devices on the bus. The bus may
@@ -211,6 +256,7 @@ struct rte_bus {
 	rte_bus_parse_t parse;       /**< Parse a device name */
 	struct rte_bus_conf conf;    /**< Bus configuration */
 	rte_bus_get_iommu_class_t get_iommu_class; /**< Get iommu class */
+	rte_bus_ctrl_get_t ctrl;     /**< Get control operators */
 };
 
 /**
-- 
2.1.4

[PATCH v1 3/8] bus: remove probe mode configuration structure

[Gaetan Rivet]

This configuration item will be implemented within the new flexible
framework.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 drivers/bus/pci/pci_common.c               | 5 +----
 lib/librte_eal/common/eal_common_devargs.c | 8 --------
 lib/librte_eal/common/include/rte_bus.h    | 8 --------
 3 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index cc23a39..dc69113 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -370,12 +370,9 @@ rte_pci_probe(void)
 	struct rte_pci_device *dev = NULL;
 	size_t probed = 0, failed = 0;
 	struct rte_devargs *devargs;
-	int probe_all = 0;
+	int probe_all = 1;
 	int ret = 0;
 
-	if (rte_pci_bus.bus.conf.probe_mode != RTE_BUS_PROBE_WHITELIST)
-		probe_all = 1;
-
 	FOREACH_DEVICE_ON_PCIBUS(dev) {
 		probed++;
 
diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index f5ef913..e371456 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -156,7 +156,6 @@ int
 rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 {
 	struct rte_devargs *devargs = NULL;
-	struct rte_bus *bus = NULL;
 	const char *dev = devargs_str;
 
 	/* use calloc instead of rte_zmalloc as it's called early at init */
@@ -167,15 +166,8 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	if (rte_eal_devargs_parse(dev, devargs))
 		goto fail;
 	devargs->type = devtype;
-	bus = devargs->bus;
 	if (devargs->type == RTE_DEVTYPE_BLACKLISTED_PCI)
 		devargs->policy = RTE_DEV_BLACKLISTED;
-	if (bus->conf.probe_mode == RTE_BUS_PROBE_UNDEFINED) {
-		if (devargs->policy == RTE_DEV_WHITELISTED)
-			bus->conf.probe_mode = RTE_BUS_PROBE_WHITELIST;
-		else if (devargs->policy == RTE_DEV_BLACKLISTED)
-			bus->conf.probe_mode = RTE_BUS_PROBE_BLACKLIST;
-	}
 	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
 	return 0;
 
diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
index bd3c28e..a8fb6b1 100644
--- a/lib/librte_eal/common/include/rte_bus.h
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -177,13 +177,6 @@ enum rte_bus_probe_mode {
 };
 
 /**
- * A structure used to configure bus operations.
- */
-struct rte_bus_conf {
-	enum rte_bus_probe_mode probe_mode; /**< Probe policy. */
-};
-
-/**
  * Bus configuration items.
  */
 enum rte_bus_ctrl_item {
@@ -254,7 +247,6 @@ struct rte_bus {
 	rte_bus_plug_t plug;         /**< Probe single device for drivers */
 	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
 	rte_bus_parse_t parse;       /**< Parse a device name */
-	struct rte_bus_conf conf;    /**< Bus configuration */
 	rte_bus_get_iommu_class_t get_iommu_class; /**< Get iommu class */
 	rte_bus_ctrl_get_t ctrl;     /**< Get control operators */
 };
-- 
2.1.4

[PATCH v1 4/8] bus: add probe mode setter

[Gaetan Rivet]

Introduce new rte_bus operation to configure the probe policy.

Implementation is required from buses interested in supporting
this configuration element.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_bus.c     | 24 ++++++++++++++++++++++++
 lib/librte_eal/common/eal_common_options.c | 17 ++++-------------
 lib/librte_eal/common/include/rte_bus.h    | 16 ++++++++++++++++
 3 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
index 65d7229..5b155c6 100644
--- a/lib/librte_eal/common/eal_common_bus.c
+++ b/lib/librte_eal/common/eal_common_bus.c
@@ -120,6 +120,30 @@ rte_bus_probe(void)
 	return 0;
 }
 
+/* Configure the probing policy of a bus */
+int
+rte_bus_probe_mode_set(const char *busname,
+		       enum rte_bus_probe_mode mode)
+{
+	struct rte_bus *bus;
+	rte_bus_ctrl_t probe_mode_set;
+
+	bus = rte_bus_find_by_name(busname);
+	if (bus == NULL) {
+		RTE_LOG(ERR, EAL, "Bus %s not found.\n",
+			busname);
+		return -EFAULT;
+	}
+	probe_mode_set = bus->ctrl(RTE_BUS_CTRL_SET,
+				   RTE_BUS_CTRL_PROBE_MODE);
+	if (probe_mode_set == NULL) {
+		RTE_LOG(ERR, EAL, "Bus %s: probe policy cannot be configured.\n",
+			busname);
+		return -ENOTSUP;
+	}
+	return probe_mode_set(&mode);
+}
+
 /* Dump information of a single bus */
 static int
 bus_dump_one(FILE *f, struct rte_bus *bus)
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index e40c049..630c9d2 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -997,29 +997,24 @@ int
 eal_parse_common_option(int opt, const char *optarg,
 			struct internal_config *conf)
 {
-	static int b_used;
-	static int w_used;
-
 	switch (opt) {
 	/* blacklist */
 	case 'b':
-		if (w_used)
-			goto bw_used;
+		if (rte_bus_probe_mode_set("pci", RTE_BUS_PROBE_BLACKLIST) < 0)
+			return -1;
 		if (eal_option_device_add(RTE_DEVTYPE_BLACKLISTED_PCI,
 				optarg) < 0) {
 			return -1;
 		}
-		b_used = 1;
 		break;
 	/* whitelist */
 	case 'w':
-		if (b_used)
-			goto bw_used;
+		if (rte_bus_probe_mode_set("pci", RTE_BUS_PROBE_WHITELIST) < 0)
+			return -1;
 		if (eal_option_device_add(RTE_DEVTYPE_WHITELISTED_PCI,
 				optarg) < 0) {
 			return -1;
 		}
-		w_used = 1;
 		break;
 	/* coremask */
 	case 'c':
@@ -1165,10 +1160,6 @@ eal_parse_common_option(int opt, const char *optarg,
 	}
 
 	return 0;
-bw_used:
-	RTE_LOG(ERR, EAL, "Options blacklist (-b) and whitelist (-w) "
-		"cannot be used at the same time\n");
-	return -1;
 }
 
 static void
diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
index a8fb6b1..93108ce 100644
--- a/lib/librte_eal/common/include/rte_bus.h
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -289,6 +289,22 @@ int rte_bus_scan(void);
 int rte_bus_probe(void);
 
 /**
+ * Configure bus probe policy.
+ *
+ * @param busname
+ *	Name of the bus to configure.
+ *
+ * @param mode
+ *	Configure the designated bus probe policy to this value.
+ *
+ * @return
+ *	0 on success
+ *	!0 otherwise
+ */
+int rte_bus_probe_mode_set(const char *busname,
+			   enum rte_bus_probe_mode mode);
+
+/**
  * Dump information of all the buses registered with EAL.
  *
  * @param f
-- 
2.1.4

[PATCH v1 5/8] bus/pci: implement ctrl operator

[Gaetan Rivet]

Add the PCI bus control operator.

This operator gives access to the probe policy setting, allowing to
read and write this configuration item. The previous existing
functionality is thus restored to the same level.

Probe policy is blacklist mode by default for the PCI bus. Configuration
is allowed once, and is then considered immutable.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 drivers/bus/pci/include/rte_bus_pci.h |  1 +
 drivers/bus/pci/pci_common.c          | 52 ++++++++++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/include/rte_bus_pci.h b/drivers/bus/pci/include/rte_bus_pci.h
index e6a7998..f662705 100644
--- a/drivers/bus/pci/include/rte_bus_pci.h
+++ b/drivers/bus/pci/include/rte_bus_pci.h
@@ -155,6 +155,7 @@ struct rte_pci_driver {
  */
 struct rte_pci_bus {
 	struct rte_bus bus;               /**< Inherit the generic class */
+	enum rte_bus_probe_mode probe_mode; /**< Probe policy */
 	struct rte_pci_device_list device_list;  /**< List of PCI devices */
 	struct rte_pci_driver_list driver_list;  /**< List of PCI drivers */
 };
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index dc69113..358e232 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -370,9 +370,12 @@ rte_pci_probe(void)
 	struct rte_pci_device *dev = NULL;
 	size_t probed = 0, failed = 0;
 	struct rte_devargs *devargs;
-	int probe_all = 1;
+	int probe_all = 0;
 	int ret = 0;
 
+	if (rte_pci_bus.probe_mode != RTE_BUS_PROBE_WHITELIST)
+		probe_all = 1;
+
 	FOREACH_DEVICE_ON_PCIBUS(dev) {
 		probed++;
 
@@ -515,6 +518,51 @@ pci_unplug(struct rte_device *dev)
 	return ret;
 }
 
+static int
+pci_probe_mode_get(void *_mode)
+{
+	enum rte_bus_probe_mode *mode = _mode;
+
+	*mode = rte_pci_bus.probe_mode;
+	return 0;
+}
+
+static int
+pci_probe_mode_set(void *_mode)
+{
+	enum rte_bus_probe_mode *mode = _mode;
+	static int conf_done;
+
+	if (conf_done &&
+	    *mode != rte_pci_bus.probe_mode) {
+		RTE_LOG(ERR, EAL, "Cannot set PCI to %s mode, bus is already configured.\n",
+			 (*mode == RTE_BUS_PROBE_BLACKLIST) ?
+			 "blacklist" : "whitelist");
+		return -1;
+	}
+	rte_pci_bus.probe_mode = *mode;
+	conf_done = 1;
+	return 0;
+}
+
+static rte_bus_ctrl_t pci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = {
+	[RTE_BUS_CTRL_PROBE_MODE] = {
+		[RTE_BUS_CTRL_GET] = pci_probe_mode_get,
+		[RTE_BUS_CTRL_SET] = pci_probe_mode_set,
+	},
+};
+
+static rte_bus_ctrl_t
+pci_ctrl(enum rte_bus_ctrl_op op,
+	 enum rte_bus_ctrl_item item)
+{
+	if (item > RTE_DIM(pci_ctrl_ops))
+		return NULL;
+	if (op > RTE_DIM(pci_ctrl_ops[item]))
+		return NULL;
+	return pci_ctrl_ops[item][op];
+}
+
 struct rte_pci_bus rte_pci_bus = {
 	.bus = {
 		.scan = rte_pci_scan,
@@ -524,7 +572,9 @@ struct rte_pci_bus rte_pci_bus = {
 		.unplug = pci_unplug,
 		.parse = pci_parse,
 		.get_iommu_class = rte_pci_get_iommu_class,
+		.ctrl = pci_ctrl,
 	},
+	.probe_mode = RTE_BUS_PROBE_BLACKLIST,
 	.device_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.device_list),
 	.driver_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.driver_list),
 };
-- 
2.1.4

[PATCH v1 6/8] bus: add IOVA mode as a ctrl operation

[Gaetan Rivet]

Leverage the new bus control framework for the IOVA mode
configuration item.

The previous version is left for the transition in drivers
implementation and will be removed afterward.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 -
 lib/librte_eal/common/eal_common_bus.c          | 24 +++++++++++------
 lib/librte_eal/common/include/rte_bus.h         | 34 ++++++++++++++-----------
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 -
 4 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 97b3918..573869a 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -228,7 +228,6 @@ DPDK_17.11 {
 	rte_eal_iova_mode;
 	rte_eal_mbuf_default_mempool_ops;
 	rte_lcore_has_role;
-	rte_pci_get_iommu_class;
 	rte_pci_match;
 
 } DPDK_17.08;
diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
index 5b155c6..3627733 100644
--- a/lib/librte_eal/common/eal_common_bus.c
+++ b/lib/librte_eal/common/eal_common_bus.c
@@ -258,18 +258,26 @@ rte_bus_find_by_device_name(const char *str)
 enum rte_iova_mode
 rte_bus_get_iommu_class(void)
 {
-	int mode = RTE_IOVA_DC;
+	enum rte_iova_mode result;
 	struct rte_bus *bus;
 
+	result = RTE_IOVA_DC;
 	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		rte_bus_ctrl_t iova_mode_get;
+		enum rte_iova_mode mode;
 
-		if (bus->get_iommu_class)
-			mode |= bus->get_iommu_class();
+		iova_mode_get = bus->ctrl(RTE_BUS_CTRL_GET,
+					RTE_BUS_CTRL_IOVA_MODE);
+		if (iova_mode_get != NULL) {
+			if (iova_mode_get(&mode))
+				RTE_LOG(ERR, EAL, "Bus %s: error reading IOMMU class\n",
+					bus->name);
+			else
+				result |= mode;
+		}
 	}
-
-	if (mode != RTE_IOVA_VA) {
+	if (result != RTE_IOVA_VA)
 		/* Use default IOVA mode */
-		mode = RTE_IOVA_PA;
-	}
-	return mode;
+		result = RTE_IOVA_PA;
+	return result;
 }
diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
index 93108ce..bb02d9d 100644
--- a/lib/librte_eal/common/include/rte_bus.h
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -55,21 +55,6 @@ extern "C" {
 /** Double linked list of buses */
 TAILQ_HEAD(rte_bus_list, rte_bus);
 
-
-/**
- * IOVA mapping mode.
- *
- * IOVA mapping mode is iommu programming mode of a device.
- * That device (for example: IOMMU backed DMA device) based
- * on rte_iova_mode will generate physical or virtual address.
- *
- */
-enum rte_iova_mode {
-	RTE_IOVA_DC = 0,	/* Don't care mode */
-	RTE_IOVA_PA = (1 << 0), /* DMA using physical address */
-	RTE_IOVA_VA = (1 << 1)  /* DMA using virtual address */
-};
-
 /**
  * Bus specific scan for devices attached on the bus.
  * For each bus object, the scan would be responsible for finding devices and
@@ -177,10 +162,29 @@ enum rte_bus_probe_mode {
 };
 
 /**
+ * IOVA mapping mode.
+ *
+ * IOVA mapping mode is iommu programming mode of a device.
+ * That device (for example: IOMMU backed DMA device) based
+ * on rte_iova_mode will generate physical or virtual address.
+ *
+ */
+enum rte_iova_mode {
+	RTE_IOVA_DC = 0,	/* Don't care mode */
+	RTE_IOVA_PA = (1 << 0), /* DMA using physical address */
+	RTE_IOVA_VA = (1 << 1)  /* DMA using virtual address */
+};
+
+/**
  * Bus configuration items.
  */
 enum rte_bus_ctrl_item {
 	RTE_BUS_CTRL_PROBE_MODE = 0,
+	/**
+	 * IOMMU class common to all devices on the bus.
+	 * If irrelevant, the bus may return RTE_IOVA_DC.
+	 */
+	RTE_BUS_CTRL_IOVA_MODE,
 	RTE_BUS_CTRL_ITEM_MAX,
 };
 
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index a8ea4ea..a2709e3 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -232,7 +232,6 @@ DPDK_17.11 {
 	rte_eal_iova_mode;
 	rte_eal_mbuf_default_mempool_ops;
 	rte_lcore_has_role;
-	rte_pci_get_iommu_class;
 	rte_pci_match;
 
 } DPDK_17.08;
-- 
2.1.4

[PATCH v1 7/8] bus/pci: implement IOVA mode getter

[Gaetan Rivet]

Implement the ctrl operator for the IOVA mode configuration item.
The previous functionality is kept identical, only the new control
framework is used.

All operators are made private as there is no reason to expose them.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 drivers/bus/pci/bsd/pci.c             |  9 ++++++---
 drivers/bus/pci/include/rte_bus_pci.h | 11 -----------
 drivers/bus/pci/linux/pci.c           | 20 +++++++++++++-------
 drivers/bus/pci/pci_common.c          |  4 +++-
 drivers/bus/pci/private.h             | 13 +++++++++++++
 5 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
index 753d914..ffb159f 100644
--- a/drivers/bus/pci/bsd/pci.c
+++ b/drivers/bus/pci/bsd/pci.c
@@ -407,11 +407,14 @@ rte_pci_scan(void)
 /*
  * Get iommu class of PCI devices on the bus.
  */
-enum rte_iova_mode
-rte_pci_get_iommu_class(void)
+int
+pci_iommu_class_get(void *_mode)
 {
+	enum rte_iova_mode *mode = _mode;
+
 	/* Supports only RTE_KDRV_NIC_UIO */
-	return RTE_IOVA_PA;
+	*mode = RTE_IOVA_PA;
+	return 0;
 }
 
 int
diff --git a/drivers/bus/pci/include/rte_bus_pci.h b/drivers/bus/pci/include/rte_bus_pci.h
index f662705..a120b70 100644
--- a/drivers/bus/pci/include/rte_bus_pci.h
+++ b/drivers/bus/pci/include/rte_bus_pci.h
@@ -205,17 +205,6 @@ int
 rte_pci_match(const struct rte_pci_driver *pci_drv,
 	      const struct rte_pci_device *pci_dev);
 
-
-/**
- * Get iommu class of PCI devices on the bus.
- * And return their preferred iova mapping mode.
- *
- * @return
- *   - enum rte_iova_mode.
- */
-enum rte_iova_mode
-rte_pci_get_iommu_class(void);
-
 /**
  * Map the PCI device resources in user space virtual memory address
  *
diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 422579f..b711cdf 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -553,17 +553,20 @@ pci_one_device_has_iova_va(void)
 /*
  * Get iommu class of PCI devices on the bus.
  */
-enum rte_iova_mode
-rte_pci_get_iommu_class(void)
+int
+pci_iommu_class_get(void *_mode)
 {
+	enum rte_iova_mode *mode = _mode;
 	bool is_bound;
 	bool is_vfio_noiommu_enabled = true;
 	bool has_iova_va;
 	bool is_bound_uio;
 
 	is_bound = pci_one_device_is_bound();
-	if (!is_bound)
-		return RTE_IOVA_DC;
+	if (!is_bound) {
+		*mode = RTE_IOVA_DC;
+		return 0;
+	}
 
 	has_iova_va = pci_one_device_has_iova_va();
 	is_bound_uio = pci_one_device_bound_uio();
@@ -572,8 +575,10 @@ rte_pci_get_iommu_class(void)
 					true : false;
 #endif
 
-	if (has_iova_va && !is_bound_uio && !is_vfio_noiommu_enabled)
-		return RTE_IOVA_VA;
+	if (has_iova_va && !is_bound_uio && !is_vfio_noiommu_enabled) {
+		*mode = RTE_IOVA_VA;
+		return 0;
+	}
 
 	if (has_iova_va) {
 		RTE_LOG(WARNING, EAL, "Some devices want iova as va but pa will be used because.. ");
@@ -583,7 +588,8 @@ rte_pci_get_iommu_class(void)
 			RTE_LOG(WARNING, EAL, "few device bound to UIO\n");
 	}
 
-	return RTE_IOVA_PA;
+	*mode = RTE_IOVA_PA;
+	return 0;
 }
 
 /* Read PCI config space. */
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 358e232..bbe862b 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -550,6 +550,9 @@ static rte_bus_ctrl_t pci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = {
 		[RTE_BUS_CTRL_GET] = pci_probe_mode_get,
 		[RTE_BUS_CTRL_SET] = pci_probe_mode_set,
 	},
+	[RTE_BUS_CTRL_IOVA_MODE] = {
+		[RTE_BUS_CTRL_GET] = pci_iommu_class_get,
+	},
 };
 
 static rte_bus_ctrl_t
@@ -571,7 +574,6 @@ struct rte_pci_bus rte_pci_bus = {
 		.plug = pci_plug,
 		.unplug = pci_unplug,
 		.parse = pci_parse,
-		.get_iommu_class = rte_pci_get_iommu_class,
 		.ctrl = pci_ctrl,
 	},
 	.probe_mode = RTE_BUS_PROBE_BLACKLIST,
diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
index fdc2c81..ee13855 100644
--- a/drivers/bus/pci/private.h
+++ b/drivers/bus/pci/private.h
@@ -171,4 +171,17 @@ void pci_uio_free_resource(struct rte_pci_device *dev,
 int pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 		struct mapped_pci_resource *uio_res, int map_idx);
 
+/**
+ * Get iommu class of PCI devices on the bus.
+ * Return their preferred iova mapping mode.
+ *
+ * @param _mode
+ *   Generic address to an (enum rte_iova_mode)
+ * @return
+ *   0 on success
+ *   !0 otherwise
+ */
+int
+pci_iommu_class_get(void *_mode);
+
 #endif /* _PCI_PRIVATE_H_ */
-- 
2.1.4

[PATCH v1 8/8] bus: remove redundant IOVA mode getter

[Gaetan Rivet]

This configuration element is now accessible through the bus control
framework and can be removed from the generic bus structure.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/include/rte_bus.h | 33 +++++++++------------------------
 1 file changed, 9 insertions(+), 24 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
index bb02d9d..1cae96e 100644
--- a/lib/librte_eal/common/include/rte_bus.h
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -227,19 +227,6 @@ typedef rte_bus_ctrl_t (*rte_bus_ctrl_get_t)(enum rte_bus_ctrl_op op,
 					     enum rte_bus_ctrl_item item);
 
 /**
- * Get common iommu class of the all the devices on the bus. The bus may
- * check that those devices are attached to iommu driver.
- * If no devices are attached to the bus. The bus may return with don't care
- * (_DC) value.
- * Otherwise, The bus will return appropriate _pa or _va iova mode.
- *
- * @return
- *      enum rte_iova_mode value.
- */
-typedef enum rte_iova_mode (*rte_bus_get_iommu_class_t)(void);
-
-
-/**
  * A structure describing a generic bus.
  */
 struct rte_bus {
@@ -251,7 +238,6 @@ struct rte_bus {
 	rte_bus_plug_t plug;         /**< Probe single device for drivers */
 	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
 	rte_bus_parse_t parse;       /**< Parse a device name */
-	rte_bus_get_iommu_class_t get_iommu_class; /**< Get iommu class */
 	rte_bus_ctrl_get_t ctrl;     /**< Get control operators */
 };
 
@@ -309,6 +295,15 @@ int rte_bus_probe_mode_set(const char *busname,
 			   enum rte_bus_probe_mode mode);
 
 /**
+ * Get the common iommu class of devices bound on to buses available in the
+ * system. The default mode is PA.
+ *
+ * @return
+ *     enum rte_iova_mode value.
+ */
+enum rte_iova_mode rte_bus_get_iommu_class(void);
+
+/**
  * Dump information of all the buses registered with EAL.
  *
  * @param f
@@ -368,16 +363,6 @@ struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);
  */
 struct rte_bus *rte_bus_find_by_name(const char *busname);
 
-
-/**
- * Get the common iommu class of devices bound on to buses available in the
- * system. The default mode is PA.
- *
- * @return
- *     enum rte_iova_mode value.
- */
-enum rte_iova_mode rte_bus_get_iommu_class(void);
-
 /**
  * Helper for Bus registration.
  * The constructor has higher priority than PMD constructors.
-- 
2.1.4

Re: [PATCH v1 0/8] Bus control framework

[Shreyansh Jain]

Hello Gaetan,

(I am assuming that this series is still valid for 18.02 and you will 
spin a new version of this.)

On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote:
> Probing policy was introduced in the previous release as a configuration item.
> It was thus added to the generic bus structure, breaking its ABI.
> 
> In this release, the IOVA mode can be read from a bus to configure the
> EAL. This new configuration element also broke the bus ABI when it was
> added.
> 
> As new operators had to be implemented for the probe policy item, these
> patches were developed to help mitigate this issue.
> 
> This control framework allows to expand the rte_bus API without breaking
> its ABI. It is meant to be used with configuration elements that may
> only be valid for a few buses, while the others would remain untouched
> and unaware of the evolution.
> 
> A central control operator is used, similarly to the working of rte_flow
> API in the ether layer. Each driver thus chooses to expose a set of
> operators relevant to its implementation. The caller is then free to use
> those if they are available.

I like the overall idea - similar to an ioctl.
It would help extend the control knobs of buses (drivers?) without 
adding additional dependency on ABI/API.

+1

> 
> Both Probe mode and IOVA mode operators are implemented for the PCI bus. >
[...]

Re: [PATCH v1 2/8] bus: introduce opaque control framework

[Shreyansh Jain]

On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote:
> New configuration elements are added to the buses. They make the ABI
> unstable and will continue to do so.
> 
> This new control scheme allows to add new bus operators without
> breaking the ABI and by only expanding the API.
> 
> This helps having more stability in core EAL subsystems, while allowing
> flexibility for future evolutions.
> 
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>   lib/librte_eal/common/eal_common_bus.c  |  9 +++++++
>   lib/librte_eal/common/include/rte_bus.h | 46 +++++++++++++++++++++++++++++++++
>   2 files changed, 55 insertions(+)
> 
> diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
> index 3c66a02..65d7229 100644
> --- a/lib/librte_eal/common/eal_common_bus.c
> +++ b/lib/librte_eal/common/eal_common_bus.c
> @@ -42,6 +42,13 @@
>   struct rte_bus_list rte_bus_list =
>   	TAILQ_HEAD_INITIALIZER(rte_bus_list);
>   
> +static rte_bus_ctrl_t
> +rte_bus_default_ctrl(enum rte_bus_ctrl_op op __rte_unused,
> +		     enum rte_bus_ctrl_item item __rte_unused)
> +{
> +	return NULL;
> +}
> +
>   void
>   rte_bus_register(struct rte_bus *bus)
>   {
> @@ -53,6 +60,8 @@ rte_bus_register(struct rte_bus *bus)
>   	RTE_VERIFY(bus->find_device);
>   	/* Buses supporting driver plug also require unplug. */
>   	RTE_VERIFY(!bus->plug || bus->unplug);
> +	if (bus->ctrl == NULL)
> +		bus->ctrl = &rte_bus_default_ctrl;
>   
>   	TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
>   	RTE_LOG(DEBUG, EAL, "Registered [%s] bus.\n", bus->name);
> diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
> index 331d954..bd3c28e 100644
> --- a/lib/librte_eal/common/include/rte_bus.h
> +++ b/lib/librte_eal/common/include/rte_bus.h
> @@ -183,6 +183,51 @@ struct rte_bus_conf {
>   	enum rte_bus_probe_mode probe_mode; /**< Probe policy. */
>   };
>   
> +/**
> + * Bus configuration items.
> + */
> +enum rte_bus_ctrl_item {
> +	RTE_BUS_CTRL_PROBE_MODE = 0,
> +	RTE_BUS_CTRL_ITEM_MAX,
> +};

I am assuming that a driver implementation can take more than ITEM_MAX 
control knobs. It is opaque to the library. Are we on same page?

For example, a bus driver can implement:

rte_bus_XXX_ctrl_item {
	<Leaving space for allowing rte_bus.h implementations>
	RTE_BUS_XYZ_KNOB_1 = 100,
	RTE_BUS_XYZ_KNOB_2,
	RTE_BUS_XYZ_KNOB_3,
};

without the library knowing or restricting the API to RTE_BUS_CTRL_ITEM_MAX.

I see that in your code for PCI (Patch 5/8: pci_ctrl) you have 
restricted the control knob to RTE_BUS_CTRL_ITEM_MAX.
I hope that such restrictions would not float to library layer.

If we are on same page, should this be documented as a code comment 
somewhere?
if not, do you think what I am stating makes sense?

> +
> +/**
> + * Bus configuration operations.
> + */
> +enum rte_bus_ctrl_op {
> +	RTE_BUS_CTRL_GET = 0,
> +	RTE_BUS_CTRL_SET,
> +	RTE_BUS_CTRL_RESET,
> +	RTE_BUS_CTRL_OP_MAX,
> +};

Similarly, the driver implementation can choose to implement a operation 
which is not defined in the above structures. Obviously, the application 
is expected to know - it being a custom knob.

[...]

Re: [PATCH v1 4/8] bus: add probe mode setter

[Shreyansh Jain]

On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote:
> Introduce new rte_bus operation to configure the probe policy.
> 
> Implementation is required from buses interested in supporting
> this configuration element.
> 

[...]

> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index e40c049..630c9d2 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -997,29 +997,24 @@ int
>   eal_parse_common_option(int opt, const char *optarg,
>   			struct internal_config *conf)
>   {
> -	static int b_used;
> -	static int w_used;
> -
>   	switch (opt) {
>   	/* blacklist */
>   	case 'b':
> -		if (w_used)
> -			goto bw_used;
> +		if (rte_bus_probe_mode_set("pci", RTE_BUS_PROBE_BLACKLIST) < 0)
> +			return -1;

Generic layer shouldn't be concerned about "pci" or other bus.
Problem would be to find which bus this option needs to be set.

What I can think of as options is:
1. Storing this configuration until we can parse the argument for which 
<bus> the argument has been created. That would mean changing the way 
the "-b" and "-w" are passed and to allow non-PCI device identifier to 
be passed.
2. Call each bus bus->ctrl and let it decide what to do based on the args.
  - so, have a wrapper over rte_bus_probe_mode_set for all buses rather 
than taking any one bus as option.

(2) sounds most plausible for now as the application will not send the 
bus name as argument.
And if brute force is not required, we need to split the argument to 
know the bus - after making the device naming standardized (<bus>:<...>)

Even before that, we need to agree that "-w' and "-b" are not more valid 
only for PCIs.

Above is more of loud thinging - I don't have concrete thought on this 
for now. I'll revisit this after reviewing the patches in this series.

>   		if (eal_option_device_add(RTE_DEVTYPE_BLACKLISTED_PCI,
>   				optarg) < 0) {
>   			return -1;
>   		}
> -		b_used = 1;
>   		break;
>   	/* whitelist */
>   	case 'w':
> -		if (b_used)
> -			goto bw_used;
> +		if (rte_bus_probe_mode_set("pci", RTE_BUS_PROBE_WHITELIST) < 0)
> +			return -1;
>   		if (eal_option_device_add(RTE_DEVTYPE_WHITELISTED_PCI,
>   				optarg) < 0) {
>   			return -1;
>   		}
> -		w_used = 1;
>   		break;
>   	/* coremask */
>   	case 'c':
> @@ -1165,10 +1160,6 @@ eal_parse_common_option(int opt, const char *optarg,

[...]

Re: [PATCH v1 4/8] bus: add probe mode setter

[Shreyansh Jain]

On Monday 11 December 2017 06:09 PM, Shreyansh Jain wrote:
> On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote:
>> Introduce new rte_bus operation to configure the probe policy.
>>
>> Implementation is required from buses interested in supporting
>> this configuration element.
>>
> 
> [...]
> 
>> diff --git a/lib/librte_eal/common/eal_common_options.c 
>> b/lib/librte_eal/common/eal_common_options.c
>> index e40c049..630c9d2 100644
>> --- a/lib/librte_eal/common/eal_common_options.c
>> +++ b/lib/librte_eal/common/eal_common_options.c
>> @@ -997,29 +997,24 @@ int
>>   eal_parse_common_option(int opt, const char *optarg,
>>               struct internal_config *conf)
>>   {
>> -    static int b_used;
>> -    static int w_used;
>> -
>>       switch (opt) {
>>       /* blacklist */
>>       case 'b':
>> -        if (w_used)
>> -            goto bw_used;
>> +        if (rte_bus_probe_mode_set("pci", RTE_BUS_PROBE_BLACKLIST) < 0)
>> +            return -1;
> 
> Generic layer shouldn't be concerned about "pci" or other bus.
> Problem would be to find which bus this option needs to be set.
> 
> What I can think of as options is:
> 1. Storing this configuration until we can parse the argument for which 
> <bus> the argument has been created. That would mean changing the way 
> the "-b" and "-w" are passed and to allow non-PCI device identifier to 
> be passed.
> 2. Call each bus bus->ctrl and let it decide what to do based on the args.
>   - so, have a wrapper over rte_bus_probe_mode_set for all buses rather 
> than taking any one bus as option.
> 
> (2) sounds most plausible for now as the application will not send the 
> bus name as argument.
> And if brute force is not required, we need to split the argument to 
> know the bus - after making the device naming standardized (<bus>:<...>)
> 
> Even before that, we need to agree that "-w' and "-b" are not more valid 
> only for PCIs.
> 
> Above is more of loud thinging - I don't have concrete thought on this 
> for now. I'll revisit this after reviewing the patches in this series.

Apologies. I see that some work has been done for this in devargs series 
- I was too quick to reply on this.
Let me come back to you on this after reading through that series.

> 
>>           if (eal_option_device_add(RTE_DEVTYPE_BLACKLISTED_PCI,
>>                   optarg) < 0) {
>>               return -1;
>>           }
>> -        b_used = 1;
>>           break;
>>       /* whitelist */
>>       case 'w':
>> -        if (b_used)
>> -            goto bw_used;
>> +        if (rte_bus_probe_mode_set("pci", RTE_BUS_PROBE_WHITELIST) < 0)
>> +            return -1;
>>           if (eal_option_device_add(RTE_DEVTYPE_WHITELISTED_PCI,
>>                   optarg) < 0) {
>>               return -1;
>>           }
>> -        w_used = 1;
>>           break;
>>       /* coremask */
>>       case 'c':
>> @@ -1165,10 +1160,6 @@ eal_parse_common_option(int opt, const char 
>> *optarg,
> 
> [...]
> 

Re: [PATCH v1 2/8] bus: introduce opaque control framework

[Gaëtan Rivet]

On Mon, Dec 11, 2017 at 05:30:16PM +0530, Shreyansh Jain wrote:
> On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote:
> > New configuration elements are added to the buses. They make the ABI
> > unstable and will continue to do so.
> > 
> > This new control scheme allows to add new bus operators without
> > breaking the ABI and by only expanding the API.
> > 
> > This helps having more stability in core EAL subsystems, while allowing
> > flexibility for future evolutions.
> > 
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> >   lib/librte_eal/common/eal_common_bus.c  |  9 +++++++
> >   lib/librte_eal/common/include/rte_bus.h | 46 +++++++++++++++++++++++++++++++++
> >   2 files changed, 55 insertions(+)
> > 
> > diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
> > index 3c66a02..65d7229 100644
> > --- a/lib/librte_eal/common/eal_common_bus.c
> > +++ b/lib/librte_eal/common/eal_common_bus.c
> > @@ -42,6 +42,13 @@
> >   struct rte_bus_list rte_bus_list =
> >   	TAILQ_HEAD_INITIALIZER(rte_bus_list);
> > +static rte_bus_ctrl_t
> > +rte_bus_default_ctrl(enum rte_bus_ctrl_op op __rte_unused,
> > +		     enum rte_bus_ctrl_item item __rte_unused)
> > +{
> > +	return NULL;
> > +}
> > +
> >   void
> >   rte_bus_register(struct rte_bus *bus)
> >   {
> > @@ -53,6 +60,8 @@ rte_bus_register(struct rte_bus *bus)
> >   	RTE_VERIFY(bus->find_device);
> >   	/* Buses supporting driver plug also require unplug. */
> >   	RTE_VERIFY(!bus->plug || bus->unplug);
> > +	if (bus->ctrl == NULL)
> > +		bus->ctrl = &rte_bus_default_ctrl;
> >   	TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
> >   	RTE_LOG(DEBUG, EAL, "Registered [%s] bus.\n", bus->name);
> > diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
> > index 331d954..bd3c28e 100644
> > --- a/lib/librte_eal/common/include/rte_bus.h
> > +++ b/lib/librte_eal/common/include/rte_bus.h
> > @@ -183,6 +183,51 @@ struct rte_bus_conf {
> >   	enum rte_bus_probe_mode probe_mode; /**< Probe policy. */
> >   };
> > +/**
> > + * Bus configuration items.
> > + */
> > +enum rte_bus_ctrl_item {
> > +	RTE_BUS_CTRL_PROBE_MODE = 0,
> > +	RTE_BUS_CTRL_ITEM_MAX,
> > +};
> 
> I am assuming that a driver implementation can take more than ITEM_MAX
> control knobs. It is opaque to the library. Are we on same page?
> 
> For example, a bus driver can implement:
> 
> rte_bus_XXX_ctrl_item {
> 	<Leaving space for allowing rte_bus.h implementations>
> 	RTE_BUS_XYZ_KNOB_1 = 100,
> 	RTE_BUS_XYZ_KNOB_2,
> 	RTE_BUS_XYZ_KNOB_3,
> };
> 
> without the library knowing or restricting the API to RTE_BUS_CTRL_ITEM_MAX.
> 
> I see that in your code for PCI (Patch 5/8: pci_ctrl) you have restricted
> the control knob to RTE_BUS_CTRL_ITEM_MAX.
> I hope that such restrictions would not float to library layer.
> 
> If we are on same page, should this be documented as a code comment
> somewhere?
> if not, do you think what I am stating makes sense?
> 

I see what you mean, but I'm not sure it would be a good thing.
Actually, I think proposing this ITEM_MAX was a mistake.

Regarding the specific bus knobs:

- If a single bus needs this knob, then it would be better for the dev
  to add it as part of the bus' public API, following the correct
  library versioning processes. This would not break this bus control
  structure ABI.

- If more than one bus implement this knob, then it should be proposed
  as part of the library API. Buses adding this new knob would break
  their ABI, other buses would be left untouched.

This makes me realize that proposing this ITEM_MAX value is not good to
the intended purpose of this patchset:

- If a bus implementation use a reference to ITEM_MAX, then the control
  structure ABI would be broken by any new control knob added, even if the
  bus does not implement it. Granted, it would not break the driver
  structure itself, but still. My PCI implementation is thus incorrect.

Therefore I think that it would be best to remove this ITEM_MAX altogether,
forcing bus developpers to use other ways that would not break their
ABIs every other release.

-- 
Gaëtan Rivet
6WIND

Re: [PATCH v1 2/8] bus: introduce opaque control framework

[Shreyansh Jain]

On Monday 11 December 2017 06:13 PM, Gaëtan Rivet wrote:
> On Mon, Dec 11, 2017 at 05:30:16PM +0530, Shreyansh Jain wrote:
>> On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote:

[...]

>>> diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
>>> index 331d954..bd3c28e 100644
>>> --- a/lib/librte_eal/common/include/rte_bus.h
>>> +++ b/lib/librte_eal/common/include/rte_bus.h
>>> @@ -183,6 +183,51 @@ struct rte_bus_conf {
>>>    	enum rte_bus_probe_mode probe_mode; /**< Probe policy. */
>>>    };
>>> +/**
>>> + * Bus configuration items.
>>> + */
>>> +enum rte_bus_ctrl_item {
>>> +	RTE_BUS_CTRL_PROBE_MODE = 0,
>>> +	RTE_BUS_CTRL_ITEM_MAX,
>>> +};
>>
>> I am assuming that a driver implementation can take more than ITEM_MAX
>> control knobs. It is opaque to the library. Are we on same page?
>>
>> For example, a bus driver can implement:
>>
>> rte_bus_XXX_ctrl_item {
>> 	<Leaving space for allowing rte_bus.h implementations>
>> 	RTE_BUS_XYZ_KNOB_1 = 100,
>> 	RTE_BUS_XYZ_KNOB_2,
>> 	RTE_BUS_XYZ_KNOB_3,
>> };
>>
>> without the library knowing or restricting the API to RTE_BUS_CTRL_ITEM_MAX.
>>
>> I see that in your code for PCI (Patch 5/8: pci_ctrl) you have restricted
>> the control knob to RTE_BUS_CTRL_ITEM_MAX.
>> I hope that such restrictions would not float to library layer.
>>
>> If we are on same page, should this be documented as a code comment
>> somewhere?
>> if not, do you think what I am stating makes sense?
>>
> 
> I see what you mean, but I'm not sure it would be a good thing.
> Actually, I think proposing this ITEM_MAX was a mistake.
> 
> Regarding the specific bus knobs:
> 
> - If a single bus needs this knob, then it would be better for the dev
>    to add it as part of the bus' public API, following the correct
>    library versioning processes. This would not break this bus control
>    structure ABI.

Sorry, but can you elaborate on "...add it as part of bus' public API"?

This is what I had in mind:

ctrl_fn = rte_bus_control_get(busname, RTE_BUS_XYZ_KNOB_1);

(unlike specific functions like probe_mode_get/set and iova_mode_get/set)

Where ctrl_fn would then point to a method specific to bus for KNOB_1 
configuration parameter.
Thereafter, ctrl_fn(KNOB_1, void *arg).

What other public API method are you hinting at?


> 
> - If more than one bus implement this knob, then it should be proposed
>    as part of the library API. Buses adding this new knob would break
>    their ABI, other buses would be left untouched.

Agree, if more than one bus implements same operation.

> 
> This makes me realize that proposing this ITEM_MAX value is not good to
> the intended purpose of this patchset:
> 
> - If a bus implementation use a reference to ITEM_MAX, then the control
>    structure ABI would be broken by any new control knob added, even if the
>    bus does not implement it. Granted, it would not break the driver
>    structure itself, but still. My PCI implementation is thus incorrect.

Changes to enum wouldn't break ABI as far as I understand. Adding a new 
entry only expands it to a new declaration without impacting its size or 
signature.

> 
> Therefore I think that it would be best to remove this ITEM_MAX altogether,
> forcing bus developpers to use other ways that would not break their
> ABIs every other release.
> 

Removing ITEM_MAX is OK from my side. It doesn't serve much purpose. 
But, not for the "ABI break" reason.

Re: [PATCH v1 2/8] bus: introduce opaque control framework

[Gaëtan Rivet]

On Mon, Dec 11, 2017 at 07:06:55PM +0530, Shreyansh Jain wrote:
> On Monday 11 December 2017 06:13 PM, Gaëtan Rivet wrote:
> > On Mon, Dec 11, 2017 at 05:30:16PM +0530, Shreyansh Jain wrote:
> > > On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote:
> 
> [...]
> 
> > > > diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
> > > > index 331d954..bd3c28e 100644
> > > > --- a/lib/librte_eal/common/include/rte_bus.h
> > > > +++ b/lib/librte_eal/common/include/rte_bus.h
> > > > @@ -183,6 +183,51 @@ struct rte_bus_conf {
> > > >    	enum rte_bus_probe_mode probe_mode; /**< Probe policy. */
> > > >    };
> > > > +/**
> > > > + * Bus configuration items.
> > > > + */
> > > > +enum rte_bus_ctrl_item {
> > > > +	RTE_BUS_CTRL_PROBE_MODE = 0,
> > > > +	RTE_BUS_CTRL_ITEM_MAX,
> > > > +};
> > > 
> > > I am assuming that a driver implementation can take more than ITEM_MAX
> > > control knobs. It is opaque to the library. Are we on same page?
> > > 
> > > For example, a bus driver can implement:
> > > 
> > > rte_bus_XXX_ctrl_item {
> > > 	<Leaving space for allowing rte_bus.h implementations>
> > > 	RTE_BUS_XYZ_KNOB_1 = 100,
> > > 	RTE_BUS_XYZ_KNOB_2,
> > > 	RTE_BUS_XYZ_KNOB_3,
> > > };
> > > 
> > > without the library knowing or restricting the API to RTE_BUS_CTRL_ITEM_MAX.
> > > 
> > > I see that in your code for PCI (Patch 5/8: pci_ctrl) you have restricted
> > > the control knob to RTE_BUS_CTRL_ITEM_MAX.
> > > I hope that such restrictions would not float to library layer.
> > > 
> > > If we are on same page, should this be documented as a code comment
> > > somewhere?
> > > if not, do you think what I am stating makes sense?
> > > 
> > 
> > I see what you mean, but I'm not sure it would be a good thing.
> > Actually, I think proposing this ITEM_MAX was a mistake.
> > 
> > Regarding the specific bus knobs:
> > 
> > - If a single bus needs this knob, then it would be better for the dev
> >    to add it as part of the bus' public API, following the correct
> >    library versioning processes. This would not break this bus control
> >    structure ABI.
> 
> Sorry, but can you elaborate on "...add it as part of bus' public API"?
> 
> This is what I had in mind:
> 
> ctrl_fn = rte_bus_control_get(busname, RTE_BUS_XYZ_KNOB_1);
> 
> (unlike specific functions like probe_mode_get/set and iova_mode_get/set)
> 
> Where ctrl_fn would then point to a method specific to bus for KNOB_1
> configuration parameter.
> Thereafter, ctrl_fn(KNOB_1, void *arg).
> 
> What other public API method are you hinting at?
> 
> 

I was thinking that buses would simply expose a function

rte_busname_xyz_knob1(void *arg);

as part of their public API. This would not require an ABI break for
this bus, as it would only be an API extension and would not use
callbacks within the bus structure.

Thus, I think that for buses tempted to propose a specific API, this would be
the cleanest way.

The bus proposing it as part of a custom control section would only be
interesting when the operation is expected to become a standard API for
other buses but was not yet accepted. Applications would be able to use
the interface and the ITEM could be added later. But I doubt this is
encouraging best practices as far as API evolution go.

> > 
> > - If more than one bus implement this knob, then it should be proposed
> >    as part of the library API. Buses adding this new knob would break
> >    their ABI, other buses would be left untouched.
> 
> Agree, if more than one bus implements same operation.
> 
> > 
> > This makes me realize that proposing this ITEM_MAX value is not good to
> > the intended purpose of this patchset:
> > 
> > - If a bus implementation use a reference to ITEM_MAX, then the control
> >    structure ABI would be broken by any new control knob added, even if the
> >    bus does not implement it. Granted, it would not break the driver
> >    structure itself, but still. My PCI implementation is thus incorrect.
> 
> Changes to enum wouldn't break ABI as far as I understand. Adding a new
> entry only expands it to a new declaration without impacting its size or
> signature.
> 
> > 
> > Therefore I think that it would be best to remove this ITEM_MAX altogether,
> > forcing bus developpers to use other ways that would not break their
> > ABIs every other release.
> > 
> 
> Removing ITEM_MAX is OK from my side. It doesn't serve much purpose. But,
> not for the "ABI break" reason.

Adding the enum would not break ABI indeed, but I was thinking about the
way the bus control structure would be declared.

However, upon second inspection on the my PCI implementation, I did not
actually use ITEM_MAX:

> static rte_bus_ctrl_t pci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = {
> »      [RTE_BUS_CTRL_PROBE_MODE] = {
> »      »       [RTE_BUS_CTRL_GET] = pci_probe_mode_get,
> »      »       [RTE_BUS_CTRL_SET] = pci_probe_mode_set,
> »      },
> };

I just thought I had used RTE_BUS_CTRL_ITEM_MAX instead of RTE_BUS_CTRL_OP_MAX.
So my line of thought was simply that if any new item was declared, the control
structure would then change size.

But I was mistaken, so that's actually not a problem :)

Having ITEM_MAX available would still make those kind of mistakes
possible however. It might be better to prevent it completely by
removing it. This would however also prevent a custom control section.

Do you think this would be useful enough to justify the slightly more
complex maintenance and review of bus implementations?

-- 
Gaëtan Rivet
6WIND

Re: [PATCH v1 2/8] bus: introduce opaque control framework

[Shreyansh Jain]

On Monday 11 December 2017 08:08 PM, Gaëtan Rivet wrote:
> On Mon, Dec 11, 2017 at 07:06:55PM +0530, Shreyansh Jain wrote:
>> On Monday 11 December 2017 06:13 PM, Gaëtan Rivet wrote:
>>> On Mon, Dec 11, 2017 at 05:30:16PM +0530, Shreyansh Jain wrote:
>>>> On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote:
>>
>> [...]
>>
>>>>> diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
>>>>> index 331d954..bd3c28e 100644
>>>>> --- a/lib/librte_eal/common/include/rte_bus.h
>>>>> +++ b/lib/librte_eal/common/include/rte_bus.h
>>>>> @@ -183,6 +183,51 @@ struct rte_bus_conf {
>>>>>     	enum rte_bus_probe_mode probe_mode; /**< Probe policy. */
>>>>>     };
>>>>> +/**
>>>>> + * Bus configuration items.
>>>>> + */
>>>>> +enum rte_bus_ctrl_item {
>>>>> +	RTE_BUS_CTRL_PROBE_MODE = 0,
>>>>> +	RTE_BUS_CTRL_ITEM_MAX,
>>>>> +};
>>>>
>>>> I am assuming that a driver implementation can take more than ITEM_MAX
>>>> control knobs. It is opaque to the library. Are we on same page?
>>>>
>>>> For example, a bus driver can implement:
>>>>
>>>> rte_bus_XXX_ctrl_item {
>>>> 	<Leaving space for allowing rte_bus.h implementations>
>>>> 	RTE_BUS_XYZ_KNOB_1 = 100,
>>>> 	RTE_BUS_XYZ_KNOB_2,
>>>> 	RTE_BUS_XYZ_KNOB_3,
>>>> };
>>>>
>>>> without the library knowing or restricting the API to RTE_BUS_CTRL_ITEM_MAX.
>>>>
>>>> I see that in your code for PCI (Patch 5/8: pci_ctrl) you have restricted
>>>> the control knob to RTE_BUS_CTRL_ITEM_MAX.
>>>> I hope that such restrictions would not float to library layer.
>>>>
>>>> If we are on same page, should this be documented as a code comment
>>>> somewhere?
>>>> if not, do you think what I am stating makes sense?
>>>>
>>>
>>> I see what you mean, but I'm not sure it would be a good thing.
>>> Actually, I think proposing this ITEM_MAX was a mistake.
>>>
>>> Regarding the specific bus knobs:
>>>
>>> - If a single bus needs this knob, then it would be better for the dev
>>>     to add it as part of the bus' public API, following the correct
>>>     library versioning processes. This would not break this bus control
>>>     structure ABI.
>>
>> Sorry, but can you elaborate on "...add it as part of bus' public API"?
>>
>> This is what I had in mind:
>>
>> ctrl_fn = rte_bus_control_get(busname, RTE_BUS_XYZ_KNOB_1);
>>
>> (unlike specific functions like probe_mode_get/set and iova_mode_get/set)
>>
>> Where ctrl_fn would then point to a method specific to bus for KNOB_1
>> configuration parameter.
>> Thereafter, ctrl_fn(KNOB_1, void *arg).
>>
>> What other public API method are you hinting at?
>>
>>
> 
> I was thinking that buses would simply expose a function
> 
> rte_busname_xyz_knob1(void *arg);

Yes, that is possible but only for cases where some very specific 
functionality needs to be exposed which is not expected to be 
generalized ever.

> 
> as part of their public API. This would not require an ABI break for
> this bus, as it would only be an API extension and would not use
> callbacks within the bus structure.

Yes, agree with your point.
As such APIs are outside control of DPDK framework, they are something 
which will never impact the library layer.

> 
> Thus, I think that for buses tempted to propose a specific API, this would be
> the cleanest way. >
> The bus proposing it as part of a custom control section would only be
> interesting when the operation is expected to become a standard API for
> other buses but was not yet accepted. Applications would be able to use
> the interface and the ITEM could be added later. But I doubt this is
> encouraging best practices as far as API evolution go.

So, technically both are feasible: 1) having a bus specific API like 
rte_busname_xyz_knob1 and 2) expanding OPS with bus specific values and 
allowing application to use them.

But, in either case, if the APIs can be generalized and/or can be used 
by multiple buses, they can definitely be moved into the library API 
(e.g. rte_bus_probe_mode_set) and/or can be added to rte_bus_ctrl_item.

To summarize, I am OK with your approach.

> 
>>>
>>> - If more than one bus implement this knob, then it should be proposed
>>>     as part of the library API. Buses adding this new knob would break
>>>     their ABI, other buses would be left untouched.
>>
>> Agree, if more than one bus implements same operation.
>>
>>>
>>> This makes me realize that proposing this ITEM_MAX value is not good to
>>> the intended purpose of this patchset:
>>>
>>> - If a bus implementation use a reference to ITEM_MAX, then the control
>>>     structure ABI would be broken by any new control knob added, even if the
>>>     bus does not implement it. Granted, it would not break the driver
>>>     structure itself, but still. My PCI implementation is thus incorrect.
>>
>> Changes to enum wouldn't break ABI as far as I understand. Adding a new
>> entry only expands it to a new declaration without impacting its size or
>> signature.
>>
>>>
>>> Therefore I think that it would be best to remove this ITEM_MAX altogether,
>>> forcing bus developpers to use other ways that would not break their
>>> ABIs every other release.
>>>
>>
>> Removing ITEM_MAX is OK from my side. It doesn't serve much purpose. But,
>> not for the "ABI break" reason.
> 
> Adding the enum would not break ABI indeed, but I was thinking about the
> way the bus control structure would be declared.
> 
> However, upon second inspection on the my PCI implementation, I did not
> actually use ITEM_MAX:
> 
>> static rte_bus_ctrl_t pci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = {
>> »      [RTE_BUS_CTRL_PROBE_MODE] = {
>> »      »       [RTE_BUS_CTRL_GET] = pci_probe_mode_get,
>> »      »       [RTE_BUS_CTRL_SET] = pci_probe_mode_set,
>> »      },
>> };
> 
> I just thought I had used RTE_BUS_CTRL_ITEM_MAX instead of RTE_BUS_CTRL_OP_MAX.
> So my line of thought was simply that if any new item was declared, the control
> structure would then change size.
> 
> But I was mistaken, so that's actually not a problem :)
> 
> Having ITEM_MAX available would still make those kind of mistakes
> possible however. It might be better to prevent it completely by
> removing it. This would however also prevent a custom control section.

It might be a naive question, but, why do you think it would prevent a 
custom control section?

Assuming that only RTE_BUS_CTRL_ITEM_MAX is not available 
(RTE_BUS_CTRL_OPS_MAX is available), Bus driver can still define:

static rte_bus_ctrl_t pci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = {
	[ITEM_1] = {
		{...},
		{...},
	},
	[ITEM_2] = {
		{...},
		{...},
	},
	[ITEM_NOT_IN_RTE_BUS.H] = {
		{...},
		{...},
	},
}

(I know you disagree with third element of above definition - but 
somehow I feel it is a good addition for defining a knob which doesn't 
require an additional API call. Just assume as an example for now, please!)

> 
> Do you think this would be useful enough to justify the slightly more
> complex maintenance and review of bus implementations?
> 

Having RTE_BUS_CTRL_ITEM_MAX is helpful if one has to iterate over all 
ctrl_items - but, that might never be required in my perception. Other 
than that, I don't have a strong reason to say that ITEM_MAX is required.
Though, same is not true for OP_MAX - which is required for definitions 
like above.