[PATCH v1 0/5] Enable hotplug in vfio

[PATCH v1 0/5] Enable hotplug in vfio

[Jeff Guo]

As we may know that the process of hotplug is different between igb_uio
and vfio. For igb_uio, it could use uevent notification and memory
failure handle mechanism for hotplug. But for vfio, when device is be
hotplug-out, the uevent can not be detected immediately, because of the
vfio kernel module will use a special mechanism to guaranty the pci
device would not be deleted until the user space release the resources,
so it will use another event “req notifier” at first to notify user space
to release resources for hotplug.

This patch will add a new interrupt type of req notifier in eal interrupt,
and add the new interrupt handler in pci device to handle the req device
event. When the req notifier be detected, it can trigger the device event
callback process to process for hotplug. With this mechanism, hotplug
could be enable in vfio.

Jeff Guo (5):
  eal: add a new req notifier to eal interrupt
  eal: add a new req event to device event
  eal: modify device event callback process func
  pci: add req handler field to generic pci device
  vfio: enable vfio hotplug by req notifier handler

 drivers/bus/pci/linux/pci_vfio.c                   | 104 +++++++++++++++++++++
 drivers/bus/pci/pci_common.c                       |  10 ++
 drivers/bus/pci/rte_bus_pci.h                      |   1 +
 lib/librte_eal/common/eal_common_dev.c             |   5 +-
 lib/librte_eal/common/eal_private.h                |  12 ---
 lib/librte_eal/common/include/rte_dev.h            |  20 +++-
 lib/librte_eal/common/include/rte_eal_interrupts.h |   1 +
 lib/librte_eal/linuxapp/eal/eal_dev.c              |   2 +-
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       |  71 ++++++++++++++
 lib/librte_ethdev/rte_ethdev.c                     |   3 +-
 10 files changed, 212 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH v1 1/5] eal: add a new req notifier to eal interrupt

[Jeff Guo]

Add a new req notifier in eal interrupt for enable vfio hotplug.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
 lib/librte_eal/common/include/rte_eal_interrupts.h |  1 +
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 71 ++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_eal_interrupts.h b/lib/librte_eal/common/include/rte_eal_interrupts.h
index 6eb4932..2c47738 100644
--- a/lib/librte_eal/common/include/rte_eal_interrupts.h
+++ b/lib/librte_eal/common/include/rte_eal_interrupts.h
@@ -35,6 +35,7 @@ enum rte_intr_handle_type {
 	RTE_INTR_HANDLE_EXT,          /**< external handler */
 	RTE_INTR_HANDLE_VDEV,         /**< virtual device */
 	RTE_INTR_HANDLE_DEV_EVENT,    /**< device event handle */
+	RTE_INTR_HANDLE_VFIO_REQ,  /**< vfio device handle (req) */
 	RTE_INTR_HANDLE_MAX           /**< count of elements */
 };
 
diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index 4076c6d..7f611b3 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -308,6 +308,64 @@ vfio_disable_msix(const struct rte_intr_handle *intr_handle) {
 
 	return ret;
 }
+
+/* enable req notifier */
+static int
+vfio_enable_req(const struct rte_intr_handle *intr_handle)
+{
+	int len, ret;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	struct vfio_irq_set *irq_set;
+	int *fd_ptr;
+
+	len = sizeof(irq_set_buf);
+
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 1;
+	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
+			 VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
+	irq_set->start = 0;
+	fd_ptr = (int *) &irq_set->data;
+	*fd_ptr = intr_handle->fd;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Error enabling req interrupts for fd %d\n",
+						intr_handle->fd);
+		return -1;
+	}
+
+	return 0;
+}
+
+/* disable req notifier */
+static int
+vfio_disable_req(const struct rte_intr_handle *intr_handle)
+{
+	struct vfio_irq_set *irq_set;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	int len, ret;
+
+	len = sizeof(struct vfio_irq_set);
+
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 0;
+	irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
+	irq_set->start = 0;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret)
+		RTE_LOG(ERR, EAL, "Error disabling req interrupts for fd %d\n",
+			intr_handle->fd);
+
+	return ret;
+}
 #endif
 
 static int
@@ -556,6 +614,10 @@ rte_intr_enable(const struct rte_intr_handle *intr_handle)
 		if (vfio_enable_intx(intr_handle))
 			return -1;
 		break;
+	case RTE_INTR_HANDLE_VFIO_REQ:
+		if (vfio_enable_req(intr_handle))
+			return -1;
+		break;
 #endif
 	/* not used at this moment */
 	case RTE_INTR_HANDLE_DEV_EVENT:
@@ -606,6 +668,11 @@ rte_intr_disable(const struct rte_intr_handle *intr_handle)
 		if (vfio_disable_intx(intr_handle))
 			return -1;
 		break;
+	case RTE_INTR_HANDLE_VFIO_REQ:
+		if (vfio_disable_req(intr_handle))
+			return -1;
+		break;
+
 #endif
 	/* not used at this moment */
 	case RTE_INTR_HANDLE_DEV_EVENT:
@@ -682,6 +749,10 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
 			bytes_read = 0;
 			call = true;
 			break;
+		case RTE_INTR_HANDLE_VFIO_REQ:
+			bytes_read = 0;
+			call = true;
+			break;
 		default:
 			bytes_read = 1;
 			break;
-- 
2.7.4

[PATCH v1 2/5] eal: add a new req event to device event

[Jeff Guo]

Add a new req event in eal device event for vfio hotplug. When the req
request send from the vfio kernel module be detected, vfio userpace
driver could use this event to notify the app to handler it.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
 lib/librte_eal/common/include/rte_dev.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index ff580a0..0324c84 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -30,6 +30,7 @@ extern "C" {
 enum rte_dev_event_type {
 	RTE_DEV_EVENT_ADD,	/**< device being added */
 	RTE_DEV_EVENT_REMOVE,	/**< device being removed */
+	RTE_DEV_EVENT_REQ,	/**< device being removed */
 	RTE_DEV_EVENT_MAX	/**< max value of this enum */
 };
 
-- 
2.7.4

[PATCH v1 3/5] eal: modify device event callback process func

[Jeff Guo]

This patch modify the device event callback process function name to be
more explicit, and exposure the API from private to public. The drivers
and apps would directly use this API to process device event callback.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
 lib/librte_eal/common/eal_common_dev.c  |  5 +++--
 lib/librte_eal/common/eal_private.h     | 12 ------------
 lib/librte_eal/common/include/rte_dev.h | 19 ++++++++++++++++++-
 lib/librte_eal/linuxapp/eal/eal_dev.c   |  2 +-
 lib/librte_ethdev/rte_ethdev.c          |  2 +-
 5 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 678dbca..2d610a4 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -342,8 +342,9 @@ rte_dev_event_callback_unregister(const char *device_name,
 	return ret;
 }
 
-void
-dev_callback_process(char *device_name, enum rte_dev_event_type event)
+void __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event)
 {
 	struct dev_event_callback *cb_lst;
 
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 3cf0357..8758f2b 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -259,18 +259,6 @@ struct rte_bus *rte_bus_find_by_device_name(const char *str);
 int rte_mp_channel_init(void);
 
 /**
- * Internal Executes all the user application registered callbacks for
- * the specific device. It is for DPDK internal user only. User
- * application should not call it directly.
- *
- * @param device_name
- *  The device name.
- * @param event
- *  the device event type.
- */
-void dev_callback_process(char *device_name, enum rte_dev_event_type event);
-
-/**
  * @internal
  * Parse a device string and store its information in an
  * rte_devargs structure.
diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index 0324c84..f35ba22 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -40,7 +40,7 @@ struct rte_dev_event {
 	char *devname;			/**< device name */
 };
 
-typedef void (*rte_dev_event_cb_fn)(char *device_name,
+typedef void (*rte_dev_event_cb_fn)(const char *device_name,
 					enum rte_dev_event_type event,
 					void *cb_arg);
 
@@ -439,6 +439,23 @@ rte_dev_event_callback_unregister(const char *device_name,
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
  *
+ * Internal Executes all the user application registered callbacks for
+ * the specific device. It is for DPDK internal user only. User
+ * application should not call it directly.
+ *
+ * @param device_name
+ *  The device name.
+ * @param event
+ *  the device event type.
+ */
+void  __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
  * Start the device event monitoring.
  *
  * @return
diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c b/lib/librte_eal/linuxapp/eal/eal_dev.c
index fa5cb9b..09079ea 100644
--- a/lib/librte_eal/linuxapp/eal/eal_dev.c
+++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
@@ -270,7 +270,7 @@ dev_uev_handler(__rte_unused void *param)
 				return;
 			}
 		}
-		dev_callback_process(uevent.devname, uevent.type);
+		rte_dev_event_callback_process(uevent.devname, uevent.type);
 	}
 }
 
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index d68bd4c..da1e9a6 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -4418,7 +4418,7 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
 }
 
 static void __rte_experimental
-eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
+eth_dev_event_callback(const char *device_name, enum rte_dev_event_type type,
 		       void *arg)
 {
 	struct rte_eth_dev *eth_dev = (struct rte_eth_dev *)arg;
-- 
2.7.4

[PATCH v1 4/5] pci: add req handler field to generic pci device

[Jeff Guo]

There are some extended interrupt types in vfio pci device except from the
existing interrupts, such as err and req notifier, it could be useful for
device error monitoring. And these corresponding interrupt handler is
different from the other interrupt handler that register in PMDs, so a new
interrupt handler should be added. This patch will add specific req handler
in generic pci device.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
 drivers/bus/pci/rte_bus_pci.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
index 0d1955f..c45a820 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -66,6 +66,7 @@ struct rte_pci_device {
 	uint16_t max_vfs;                   /**< sriov enable if not zero */
 	enum rte_kernel_driver kdrv;        /**< Kernel driver passthrough */
 	char name[PCI_PRI_STR_SIZE+1];      /**< PCI location (ASCII) */
+	struct rte_intr_handle req_notifier_handler;/**< Req notifier handle */
 };
 
 /**
-- 
2.7.4

[PATCH v1 5/5] vfio: enable vfio hotplug by req notifier handler

[Jeff Guo]

This patch will add req notifier processing to enable hotplug for vfio.
When device is be hotplug-out, the vfio kernel module will sent req
notifier to request user space to release the allocated resources. It
could use the bus failure mechanism to trigger the currently hotplug
procedure of user space. After that, vfio kernel module will detect the
device disappear, and then release the kernel resource of the device.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
 drivers/bus/pci/linux/pci_vfio.c | 104 +++++++++++++++++++++++++++++++++++++++
 drivers/bus/pci/pci_common.c     |  10 ++++
 lib/librte_ethdev/rte_ethdev.c   |   1 +
 3 files changed, 115 insertions(+)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 686386d..8a14f92 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -17,6 +17,8 @@
 #include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
 #include <rte_vfio.h>
+#include <rte_eal.h>
+#include <rte_bus.h>
 
 #include "eal_filesystem.h"
 
@@ -277,6 +279,92 @@ pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd)
 	return -1;
 }
 
+static void
+pci_vfio_req_handler(void *param)
+{
+	struct rte_bus *bus;
+	int ret;
+	struct rte_device *device = (struct rte_device *)param;
+
+	bus = rte_bus_find_by_device(device);
+	if (bus == NULL) {
+		RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n",
+			device->name);
+		return;
+	}
+
+	/**
+	 * vfio kernel module request user space to release allocated
+	 * resources before resources be released in kernel, so it can directly
+	 * call the bus failure handler to use the specific mechanism in
+	 * user space to handle it.
+	 */
+	ret = bus->memory_failure_handler(device);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Can not handle failure for "
+			"device (%s)\n", device->name);
+		return;
+	}
+}
+
+/* enable notifier (only enable req now) */
+static int
+pci_vfio_enable_notifier(struct rte_pci_device *dev, int vfio_dev_fd)
+{
+	int ret;
+	int fd = -1;
+
+	/* set up an eventfd for req notifier */
+	fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
+	if (fd < 0) {
+		RTE_LOG(ERR, EAL, "Cannot set up eventfd, "
+				"error %i (%s)\n", errno, strerror(errno));
+		return -1;
+	}
+
+	dev->req_notifier_handler.fd = fd;
+	dev->req_notifier_handler.type = RTE_INTR_HANDLE_VFIO_REQ;
+	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
+	ret = rte_intr_callback_register(&dev->req_notifier_handler,
+					 pci_vfio_req_handler,
+					 (void *)&dev->device);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Fail to register req notifier handler.\n");
+		return -1;
+	}
+
+	ret = rte_intr_enable(&dev->req_notifier_handler);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Fail to enable req notifier.\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+/*disable notifier (only disable req now) */
+static int
+pci_vfio_disable_notifier(struct rte_pci_device *dev)
+{
+	int ret;
+
+	ret = rte_intr_callback_unregister(&dev->req_notifier_handler,
+					 pci_vfio_req_handler,
+					 (void *)&dev->device);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "fail to unregister req notifier handler.\n");
+		return -1;
+	}
+
+	ret = rte_intr_enable(&dev->req_notifier_handler);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "fail to disable req notifier.\n");
+		return -1;
+	}
+
+	return 0;
+}
+
 static int
 pci_vfio_is_ioport_bar(int vfio_dev_fd, int bar_index)
 {
@@ -430,6 +518,7 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
+	dev->req_notifier_handler.fd = -1;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
@@ -521,6 +610,11 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
 		goto err_vfio_res;
 	}
 
+	if (pci_vfio_enable_notifier(dev, vfio_dev_fd) != 0) {
+		RTE_LOG(ERR, EAL, "Error setting up notifier!\n");
+		return -1;
+	}
+
 	TAILQ_INSERT_TAIL(vfio_res_list, vfio_res, next);
 
 	return 0;
@@ -546,6 +640,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
+	dev->req_notifier_handler.fd = -1;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
@@ -586,6 +681,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 
 	/* we need save vfio_dev_fd, so it can be used during release */
 	dev->intr_handle.vfio_dev_fd = vfio_dev_fd;
+	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
 
 	return 0;
 err_vfio_dev_fd:
@@ -658,12 +754,20 @@ pci_vfio_unmap_resource_primary(struct rte_pci_device *dev)
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
 			loc->domain, loc->bus, loc->devid, loc->function);
 
+	pci_vfio_disable_notifier(dev);
+
 	if (close(dev->intr_handle.fd) < 0) {
 		RTE_LOG(INFO, EAL, "Error when closing eventfd file descriptor for %s\n",
 			pci_addr);
 		return -1;
 	}
 
+	if (close(dev->req_notifier_handler.fd) < 0) {
+		RTE_LOG(INFO, EAL, "Error when closing req eventfd file "
+			"descriptor for %s\n", pci_addr);
+		return -1;
+	}
+
 	if (pci_vfio_set_bus_master(dev->intr_handle.vfio_dev_fd, false)) {
 		RTE_LOG(ERR, EAL, "  %s cannot unset bus mastering for PCI device!\n",
 				pci_addr);
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index b8f3244..21a7397 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -446,6 +446,16 @@ pci_memory_failure_handler(struct rte_device *dev)
 		return -1;
 
 	switch (pdev->kdrv) {
+	case RTE_KDRV_VFIO:
+		/**
+		 * vfio kernel module guaranty the pci device would not be
+		 * deleted until the user space release the resource, so no
+		 * need to remap mmio resource here, just directly notify
+		 * the req event to the user space to handle it.
+		 */
+		rte_dev_event_callback_process(dev->name,
+					       RTE_DEV_EVENT_REQ);
+		break;
 	case RTE_KDRV_IGB_UIO:
 	case RTE_KDRV_UIO_GENERIC:
 	case RTE_KDRV_NIC_UIO:
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index da1e9a6..74694c4 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -4424,6 +4424,7 @@ eth_dev_event_callback(const char *device_name, enum rte_dev_event_type type,
 	struct rte_eth_dev *eth_dev = (struct rte_eth_dev *)arg;
 
 	switch (type) {
+	case RTE_DEV_EVENT_REQ:
 	case RTE_DEV_EVENT_REMOVE:
 		RTE_ETHDEV_LOG(INFO, "The device %s has been removed!\n",
 			       device_name);
-- 
2.7.4

Re: [PATCH v1 0/5] Enable hotplug in vfio

[Gaëtan Rivet]

Hi Jeff,

On Fri, Aug 17, 2018 at 06:51:26PM +0800, Jeff Guo wrote:
> As we may know that the process of hotplug is different between igb_uio
> and vfio. For igb_uio, it could use uevent notification and memory
> failure handle mechanism for hotplug. But for vfio, when device is be
> hotplug-out, the uevent can not be detected immediately, because of the
> vfio kernel module will use a special mechanism to guaranty the pci
> device would not be deleted until the user space release the resources,
> so it will use another event “req notifier” at first to notify user space
> to release resources for hotplug.
> 
> This patch will add a new interrupt type of req notifier in eal interrupt,
> and add the new interrupt handler in pci device to handle the req device
> event. When the req notifier be detected, it can trigger the device event
> callback process to process for hotplug. With this mechanism, hotplug
> could be enable in vfio.
> 

The REQ event seems VFIO specific.
Why not leave it within the PCI bus, around the vfio glue, and leave the
EAL unmodified?

> Jeff Guo (5):
>   eal: add a new req notifier to eal interrupt
>   eal: add a new req event to device event
>   eal: modify device event callback process func
>   pci: add req handler field to generic pci device
>   vfio: enable vfio hotplug by req notifier handler
> 
>  drivers/bus/pci/linux/pci_vfio.c                   | 104 +++++++++++++++++++++
>  drivers/bus/pci/pci_common.c                       |  10 ++
>  drivers/bus/pci/rte_bus_pci.h                      |   1 +
>  lib/librte_eal/common/eal_common_dev.c             |   5 +-
>  lib/librte_eal/common/eal_private.h                |  12 ---
>  lib/librte_eal/common/include/rte_dev.h            |  20 +++-
>  lib/librte_eal/common/include/rte_eal_interrupts.h |   1 +
>  lib/librte_eal/linuxapp/eal/eal_dev.c              |   2 +-
>  lib/librte_eal/linuxapp/eal/eal_interrupts.c       |  71 ++++++++++++++
>  lib/librte_ethdev/rte_ethdev.c                     |   3 +-
>  10 files changed, 212 insertions(+), 17 deletions(-)
> 
> -- 
> 2.7.4
> 

-- 
Gaëtan Rivet
6WIND

Re: [PATCH v1 2/5] eal: add a new req event to device event

[Andrew Rybchenko]

On 17.08.2018 13:51, Jeff Guo wrote:
> Add a new req event in eal device event for vfio hotplug. When the req
> request send from the vfio kernel module be detected, vfio userpace
> driver could use this event to notify the app to handler it.
>
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
>   lib/librte_eal/common/include/rte_dev.h | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index ff580a0..0324c84 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -30,6 +30,7 @@ extern "C" {
>   enum rte_dev_event_type {
>   	RTE_DEV_EVENT_ADD,	/**< device being added */
>   	RTE_DEV_EVENT_REMOVE,	/**< device being removed */
> +	RTE_DEV_EVENT_REQ,	/**< device being removed */

Comment is the copy of previous one.

>   	RTE_DEV_EVENT_MAX	/**< max value of this enum */
>   };
>   

Re: [PATCH v1 0/5] Enable hotplug in vfio

[Jeff Guo]

hi, gaetan


On 8/20/2018 5:15 PM, Gaëtan Rivet wrote:
> Hi Jeff,
>
> On Fri, Aug 17, 2018 at 06:51:26PM +0800, Jeff Guo wrote:
>> As we may know that the process of hotplug is different between igb_uio
>> and vfio. For igb_uio, it could use uevent notification and memory
>> failure handle mechanism for hotplug. But for vfio, when device is be
>> hotplug-out, the uevent can not be detected immediately, because of the
>> vfio kernel module will use a special mechanism to guaranty the pci
>> device would not be deleted until the user space release the resources,
>> so it will use another event “req notifier” at first to notify user space
>> to release resources for hotplug.
>>
>> This patch will add a new interrupt type of req notifier in eal interrupt,
>> and add the new interrupt handler in pci device to handle the req device
>> event. When the req notifier be detected, it can trigger the device event
>> callback process to process for hotplug. With this mechanism, hotplug
>> could be enable in vfio.
>>
> The REQ event seems VFIO specific.
> Why not leave it within the PCI bus, around the vfio glue, and leave the
> EAL unmodified?

Sorry i don' t see if there are any problem. Firstly, as we can see the 
eal interrupt type, it cover the ext/uio/vfio/dev event types, which are 
not general for all platform/bus/driver type.
so i think base on the current framework, the interrupt type structure 
should be considerate as combined set, it could extend for other adding 
interrupts case by case. And secondly, i don't know what
is your way about leave it within the PCI bus, because i need to use the 
eal interrupt epoll to process this req interrupt which is familiar with 
other interrupts. What do you think about that, if you still
have better suggestion, please detail it for clarify.

>> Jeff Guo (5):
>>    eal: add a new req notifier to eal interrupt
>>    eal: add a new req event to device event
>>    eal: modify device event callback process func
>>    pci: add req handler field to generic pci device
>>    vfio: enable vfio hotplug by req notifier handler
>>
>>   drivers/bus/pci/linux/pci_vfio.c                   | 104 +++++++++++++++++++++
>>   drivers/bus/pci/pci_common.c                       |  10 ++
>>   drivers/bus/pci/rte_bus_pci.h                      |   1 +
>>   lib/librte_eal/common/eal_common_dev.c             |   5 +-
>>   lib/librte_eal/common/eal_private.h                |  12 ---
>>   lib/librte_eal/common/include/rte_dev.h            |  20 +++-
>>   lib/librte_eal/common/include/rte_eal_interrupts.h |   1 +
>>   lib/librte_eal/linuxapp/eal/eal_dev.c              |   2 +-
>>   lib/librte_eal/linuxapp/eal/eal_interrupts.c       |  71 ++++++++++++++
>>   lib/librte_ethdev/rte_ethdev.c                     |   3 +-
>>   10 files changed, 212 insertions(+), 17 deletions(-)
>>
>> -- 
>> 2.7.4
>>

Re: [PATCH v1 2/5] eal: add a new req event to device event

[Jeff Guo]

hi, andrew


On 8/20/2018 6:37 PM, Andrew Rybchenko wrote:
> On 17.08.2018 13:51, Jeff Guo wrote:
>> Add a new req event in eal device event for vfio hotplug. When the req
>> request send from the vfio kernel module be detected, vfio userpace
>> driver could use this event to notify the app to handler it.
>>
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> ---
>>   lib/librte_eal/common/include/rte_dev.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/lib/librte_eal/common/include/rte_dev.h 
>> b/lib/librte_eal/common/include/rte_dev.h
>> index ff580a0..0324c84 100644
>> --- a/lib/librte_eal/common/include/rte_dev.h
>> +++ b/lib/librte_eal/common/include/rte_dev.h
>> @@ -30,6 +30,7 @@ extern "C" {
>>   enum rte_dev_event_type {
>>       RTE_DEV_EVENT_ADD,    /**< device being added */
>>       RTE_DEV_EVENT_REMOVE,    /**< device being removed */
>> +    RTE_DEV_EVENT_REQ,    /**< device being removed */
>
> Comment is the copy of previous one.
>

You are right here, even we process these type in the same way but 
should be considered it as the different type if we exactly want to add 
new one.
so base on the interpret from the vfio kernel driver, this req event is 
used to require the user space to release the device resources, so it 
should be
interpret it by "device release request". If you object and have other 
better idea, let me know.

>>       RTE_DEV_EVENT_MAX    /**< max value of this enum */
>>   };
>

Re: [PATCH v1 2/5] eal: add a new req event to device event

[Andrew Rybchenko]

On 21.08.2018 09:56, Jeff Guo wrote:
> hi, andrew
>
>
> On 8/20/2018 6:37 PM, Andrew Rybchenko wrote:
>> On 17.08.2018 13:51, Jeff Guo wrote:
>>> Add a new req event in eal device event for vfio hotplug. When the req
>>> request send from the vfio kernel module be detected, vfio userpace
>>> driver could use this event to notify the app to handler it.
>>>
>>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>>> ---
>>>   lib/librte_eal/common/include/rte_dev.h | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/lib/librte_eal/common/include/rte_dev.h 
>>> b/lib/librte_eal/common/include/rte_dev.h
>>> index ff580a0..0324c84 100644
>>> --- a/lib/librte_eal/common/include/rte_dev.h
>>> +++ b/lib/librte_eal/common/include/rte_dev.h
>>> @@ -30,6 +30,7 @@ extern "C" {
>>>   enum rte_dev_event_type {
>>>       RTE_DEV_EVENT_ADD,    /**< device being added */
>>>       RTE_DEV_EVENT_REMOVE,    /**< device being removed */
>>> +    RTE_DEV_EVENT_REQ,    /**< device being removed */
>>
>> Comment is the copy of previous one.
>>
>
> You are right here, even we process these type in the same way but 
> should be considered it as the different type if we exactly want to 
> add new one.
> so base on the interpret from the vfio kernel driver, this req event 
> is used to require the user space to release the device resources, so 
> it should be
> interpret it by "device release request". If you object and have other 
> better idea, let me know.


If so, may be it should be RTE_DEV_EVENT_RELEASE_REQ, since just _REQ is 
really misleading.

>
>>>       RTE_DEV_EVENT_MAX    /**< max value of this enum */
>>>   };
>>

Re: [PATCH v1 2/5] eal: add a new req event to device event

[Jeff Guo]

On 8/21/2018 3:20 PM, Andrew Rybchenko wrote:
> On 21.08.2018 09:56, Jeff Guo wrote:
>> hi, andrew
>>
>>
>> On 8/20/2018 6:37 PM, Andrew Rybchenko wrote:
>>> On 17.08.2018 13:51, Jeff Guo wrote:
>>>> Add a new req event in eal device event for vfio hotplug. When the req
>>>> request send from the vfio kernel module be detected, vfio userpace
>>>> driver could use this event to notify the app to handler it.
>>>>
>>>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>>>> ---
>>>>   lib/librte_eal/common/include/rte_dev.h | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/lib/librte_eal/common/include/rte_dev.h 
>>>> b/lib/librte_eal/common/include/rte_dev.h
>>>> index ff580a0..0324c84 100644
>>>> --- a/lib/librte_eal/common/include/rte_dev.h
>>>> +++ b/lib/librte_eal/common/include/rte_dev.h
>>>> @@ -30,6 +30,7 @@ extern "C" {
>>>>   enum rte_dev_event_type {
>>>>       RTE_DEV_EVENT_ADD,    /**< device being added */
>>>>       RTE_DEV_EVENT_REMOVE,    /**< device being removed */
>>>> +    RTE_DEV_EVENT_REQ,    /**< device being removed */
>>>
>>> Comment is the copy of previous one.
>>>
>>
>> You are right here, even we process these type in the same way but 
>> should be considered it as the different type if we exactly want to 
>> add new one.
>> so base on the interpret from the vfio kernel driver, this req event 
>> is used to require the user space to release the device resources, so 
>> it should be
>> interpret it by "device release request". If you object and have 
>> other better idea, let me know.
>
>
> If so, may be it should be RTE_DEV_EVENT_RELEASE_REQ, since just _REQ 
> is really misleading.
>

accept it , sounds better, thanks.

>>
>>>>       RTE_DEV_EVENT_MAX    /**< max value of this enum */
>>>>   };
>>>
>

Re: [PATCH v1 0/5] Enable hotplug in vfio

[Gaëtan Rivet]

On Tue, Aug 21, 2018 at 02:45:32PM +0800, Jeff Guo wrote:
> hi, gaetan
> 
> 
> On 8/20/2018 5:15 PM, Gaëtan Rivet wrote:
> > Hi Jeff,
> > 
> > On Fri, Aug 17, 2018 at 06:51:26PM +0800, Jeff Guo wrote:
> > > As we may know that the process of hotplug is different between igb_uio
> > > and vfio. For igb_uio, it could use uevent notification and memory
> > > failure handle mechanism for hotplug. But for vfio, when device is be
> > > hotplug-out, the uevent can not be detected immediately, because of the
> > > vfio kernel module will use a special mechanism to guaranty the pci
> > > device would not be deleted until the user space release the resources,
> > > so it will use another event “req notifier” at first to notify user space
> > > to release resources for hotplug.
> > > 
> > > This patch will add a new interrupt type of req notifier in eal interrupt,
> > > and add the new interrupt handler in pci device to handle the req device
> > > event. When the req notifier be detected, it can trigger the device event
> > > callback process to process for hotplug. With this mechanism, hotplug
> > > could be enable in vfio.
> > > 
> > The REQ event seems VFIO specific.
> > Why not leave it within the PCI bus, around the vfio glue, and leave the
> > EAL unmodified?
> 
> Sorry i don' t see if there are any problem. Firstly, as we can see the eal
> interrupt type, it cover the ext/uio/vfio/dev event types, which are not
> general for all platform/bus/driver type.
> so i think base on the current framework, the interrupt type structure
> should be considerate as combined set, it could extend for other adding
> interrupts case by case. And secondly, i don't know what
> is your way about leave it within the PCI bus, because i need to use the eal
> interrupt epoll to process this req interrupt which is familiar with other
> interrupts. What do you think about that, if you still
> have better suggestion, please detail it for clarify.
> 

Ah, yes, unfortunately you're right.
There is a genericization effort going on for the EAL, but these parts
are still depending on the interrupts being reworked. So for now, not
much can be done to avoid it.

> > > Jeff Guo (5):
> > >    eal: add a new req notifier to eal interrupt
> > >    eal: add a new req event to device event
> > >    eal: modify device event callback process func
> > >    pci: add req handler field to generic pci device
> > >    vfio: enable vfio hotplug by req notifier handler
> > > 
> > >   drivers/bus/pci/linux/pci_vfio.c                   | 104 +++++++++++++++++++++
> > >   drivers/bus/pci/pci_common.c                       |  10 ++
> > >   drivers/bus/pci/rte_bus_pci.h                      |   1 +
> > >   lib/librte_eal/common/eal_common_dev.c             |   5 +-
> > >   lib/librte_eal/common/eal_private.h                |  12 ---
> > >   lib/librte_eal/common/include/rte_dev.h            |  20 +++-
> > >   lib/librte_eal/common/include/rte_eal_interrupts.h |   1 +
> > >   lib/librte_eal/linuxapp/eal/eal_dev.c              |   2 +-
> > >   lib/librte_eal/linuxapp/eal/eal_interrupts.c       |  71 ++++++++++++++
> > >   lib/librte_ethdev/rte_ethdev.c                     |   3 +-
> > >   10 files changed, 212 insertions(+), 17 deletions(-)
> > > 
> > > -- 
> > > 2.7.4
> > > 
> 

-- 
Gaëtan Rivet
6WIND

Re: [PATCH v1 3/5] eal: modify device event callback process func

[Burakov, Anatoly]

On 17-Aug-18 11:51 AM, Jeff Guo wrote:
> This patch modify the device event callback process function name to be
> more explicit, and exposure the API from private to public. The drivers
> and apps would directly use this API to process device event callback.
> 
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---

<snip>

> -typedef void (*rte_dev_event_cb_fn)(char *device_name,
> +typedef void (*rte_dev_event_cb_fn)(const char *device_name,
>   					enum rte_dev_event_type event,
>   					void *cb_arg);
>   
> @@ -439,6 +439,23 @@ rte_dev_event_callback_unregister(const char *device_name,
>    * @warning
>    * @b EXPERIMENTAL: this API may change without prior notice
>    *
> + * Internal Executes all the user application registered callbacks for

it probably should say @internal instead of "Internal", with comment 
starting on a new line.

-- 
Thanks,
Anatoly

Re: [PATCH v1 3/5] eal: modify device event callback process func

[Burakov, Anatoly]

On 17-Aug-18 11:51 AM, Jeff Guo wrote:
> This patch modify the device event callback process function name to be
> more explicit, and exposure the API from private to public. The drivers
> and apps would directly use this API to process device event callback.
> 
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---

<snip>

>    *
> + * Internal Executes all the user application registered callbacks for
> + * the specific device. It is for DPDK internal user only. User
> + * application should not call it directly.
> + *
> + * @param device_name
> + *  The device name.
> + * @param event
> + *  the device event type.
> + */
> +void  __rte_experimental
> +rte_dev_event_callback_process(const char *device_name,
> +			       enum rte_dev_event_type event);

Also, you've exported it but haven't added it to the .map file.

-- 
Thanks,
Anatoly

Re: [PATCH v1 4/5] pci: add req handler field to generic pci device

[Burakov, Anatoly]

On 17-Aug-18 11:51 AM, Jeff Guo wrote:
> There are some extended interrupt types in vfio pci device except from the
> existing interrupts, such as err and req notifier, it could be useful for
> device error monitoring. And these corresponding interrupt handler is
> different from the other interrupt handler that register in PMDs, so a new
> interrupt handler should be added. This patch will add specific req handler
> in generic pci device.
> 
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
>   drivers/bus/pci/rte_bus_pci.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
> index 0d1955f..c45a820 100644
> --- a/drivers/bus/pci/rte_bus_pci.h
> +++ b/drivers/bus/pci/rte_bus_pci.h
> @@ -66,6 +66,7 @@ struct rte_pci_device {
>   	uint16_t max_vfs;                   /**< sriov enable if not zero */
>   	enum rte_kernel_driver kdrv;        /**< Kernel driver passthrough */
>   	char name[PCI_PRI_STR_SIZE+1];      /**< PCI location (ASCII) */
> +	struct rte_intr_handle req_notifier_handler;/**< Req notifier handle */
>   };
>   
>   /**
> 

Does this break ABI?

-- 
Thanks,
Anatoly

Re: [PATCH v1 5/5] vfio: enable vfio hotplug by req notifier handler

[Burakov, Anatoly]

On 17-Aug-18 11:51 AM, Jeff Guo wrote:
> This patch will add req notifier processing to enable hotplug for vfio.
> When device is be hotplug-out, the vfio kernel module will sent req
> notifier to request user space to release the allocated resources. It
> could use the bus failure mechanism to trigger the currently hotplug
> procedure of user space. After that, vfio kernel module will detect the
> device disappear, and then release the kernel resource of the device.
> 
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---

<snip>

> +	 * resources before resources be released in kernel, so it can directly
> +	 * call the bus failure handler to use the specific mechanism in
> +	 * user space to handle it.
> +	 */
> +	ret = bus->memory_failure_handler(device);
> +	if (ret) {
> +		RTE_LOG(ERR, EAL, "Can not handle failure for "
> +			"device (%s)\n", device->name);
> +		return;

Return is not needed here :)

Otherwise, LGTM

-- 
Thanks,
Anatoly

Re: [PATCH v1 5/5] vfio: enable vfio hotplug by req notifier handler

[Jeff Guo]

Anatoly, thanks for your review, please see comment as below.

On 9/26/2018 8:28 PM, Burakov, Anatoly wrote:
> On 17-Aug-18 11:51 AM, Jeff Guo wrote:
>> This patch will add req notifier processing to enable hotplug for vfio.
>> When device is be hotplug-out, the vfio kernel module will sent req
>> notifier to request user space to release the allocated resources. It
>> could use the bus failure mechanism to trigger the currently hotplug
>> procedure of user space. After that, vfio kernel module will detect the
>> device disappear, and then release the kernel resource of the device.
>>
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> ---
>
> <snip>
>
>> +     * resources before resources be released in kernel, so it can 
>> directly
>> +     * call the bus failure handler to use the specific mechanism in
>> +     * user space to handle it.
>> +     */
>> +    ret = bus->memory_failure_handler(device);
>> +    if (ret) {
>> +        RTE_LOG(ERR, EAL, "Can not handle failure for "
>> +            "device (%s)\n", device->name);
>> +        return;
>
> Return is not needed here :)
>
> Otherwise, LGTM
>

yes, will modify it.

Re: [PATCH v1 4/5] pci: add req handler field to generic pci device

[Jeff Guo]

On 9/26/2018 8:22 PM, Burakov, Anatoly wrote:
> On 17-Aug-18 11:51 AM, Jeff Guo wrote:
>> There are some extended interrupt types in vfio pci device except 
>> from the
>> existing interrupts, such as err and req notifier, it could be useful 
>> for
>> device error monitoring. And these corresponding interrupt handler is
>> different from the other interrupt handler that register in PMDs, so 
>> a new
>> interrupt handler should be added. This patch will add specific req 
>> handler
>> in generic pci device.
>>
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> ---
>>   drivers/bus/pci/rte_bus_pci.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/bus/pci/rte_bus_pci.h 
>> b/drivers/bus/pci/rte_bus_pci.h
>> index 0d1955f..c45a820 100644
>> --- a/drivers/bus/pci/rte_bus_pci.h
>> +++ b/drivers/bus/pci/rte_bus_pci.h
>> @@ -66,6 +66,7 @@ struct rte_pci_device {
>>       uint16_t max_vfs;                   /**< sriov enable if not 
>> zero */
>>       enum rte_kernel_driver kdrv;        /**< Kernel driver 
>> passthrough */
>>       char name[PCI_PRI_STR_SIZE+1];      /**< PCI location (ASCII) */
>> +    struct rte_intr_handle req_notifier_handler;/**< Req notifier 
>> handle */
>>   };
>>     /**
>>
>
> Does this break ABI?
>

If add a variable in struct would break ABI, it does.

Re: [PATCH v1 3/5] eal: modify device event callback process func

[Jeff Guo]

On 9/26/2018 8:20 PM, Burakov, Anatoly wrote:
> On 17-Aug-18 11:51 AM, Jeff Guo wrote:
>> This patch modify the device event callback process function name to be
>> more explicit, and exposure the API from private to public. The drivers
>> and apps would directly use this API to process device event callback.
>>
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> ---
>
> <snip>
>
>> -typedef void (*rte_dev_event_cb_fn)(char *device_name,
>> +typedef void (*rte_dev_event_cb_fn)(const char *device_name,
>>                       enum rte_dev_event_type event,
>>                       void *cb_arg);
>>   @@ -439,6 +439,23 @@ rte_dev_event_callback_unregister(const char 
>> *device_name,
>>    * @warning
>>    * @b EXPERIMENTAL: this API may change without prior notice
>>    *
>> + * Internal Executes all the user application registered callbacks for
>
> it probably should say @internal instead of "Internal", with comment 
> starting on a new line.
>

yes, you are here, and what i want is make it internal.

Re: [PATCH v1 3/5] eal: modify device event callback process func

[Jeff Guo]

On 9/26/2018 8:20 PM, Burakov, Anatoly wrote:
> On 17-Aug-18 11:51 AM, Jeff Guo wrote:
>> This patch modify the device event callback process function name to be
>> more explicit, and exposure the API from private to public. The drivers
>> and apps would directly use this API to process device event callback.
>>
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> ---
>
> <snip>
>
>>    *
>> + * Internal Executes all the user application registered callbacks for
>> + * the specific device. It is for DPDK internal user only. User
>> + * application should not call it directly.
>> + *
>> + * @param device_name
>> + *  The device name.
>> + * @param event
>> + *  the device event type.
>> + */
>> +void  __rte_experimental
>> +rte_dev_event_callback_process(const char *device_name,
>> +                   enum rte_dev_event_type event);
>
> Also, you've exported it but haven't added it to the .map file.
>

what i want is make it internal use, so i will modify the typo.

[PATCH v2 0/4] Enable hotplug in vfio

[Jeff Guo]

As we may know that the process of hotplug is different between igb_uio
and vfio. For igb_uio, it could use uevent notification and memory
failure handle mechanism for hot-unplug. But for vfio, when device is be
hot-unplugged, the uevent can not be detected immediately, because of the
vfio kernel module will use a special mechanism to guaranty the pci
device would not be deleted until the user space release the resources,
so it will use another event “req notifier” at first to notify user space
to release resources for hotplug.

This patch will add a new interrupt type of req notifier in eal interrupt,
and add the new interrupt handler in pci device to handle the req device
event. When the req notifier be detected, it can trigger the device event
callback process to process for hot-unplug. With this mechanism, hotplug
could be enable in vfio.

patchset history:
v2->v1:
change the rte_dev_event_callback_prcess from internal to external api
for bus or app usage.
change some code logic.

Jeff Guo (4):
  eal: add a new req notifier to eal interrupt
  eal: modify device event callback process func
  pci: add req handler field to generic pci device
  vfio: enable vfio hotplug by req notifier handler

 app/test-pmd/testpmd.c                             |  4 +-
 drivers/bus/pci/linux/pci_vfio.c                   | 95 ++++++++++++++++++++++
 drivers/bus/pci/pci_common.c                       | 10 +++
 drivers/bus/pci/rte_bus_pci.h                      |  1 +
 lib/librte_eal/bsdapp/eal/eal_dev.c                |  8 ++
 lib/librte_eal/common/eal_common_dev.c             |  5 +-
 lib/librte_eal/common/eal_private.h                | 12 ---
 lib/librte_eal/common/include/rte_dev.h            | 18 +++-
 lib/librte_eal/common/include/rte_eal_interrupts.h |  1 +
 lib/librte_eal/linuxapp/eal/eal_dev.c              |  2 +-
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 71 ++++++++++++++++
 lib/librte_eal/rte_eal_version.map                 |  1 +
 12 files changed, 210 insertions(+), 18 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] eal: add a new req notifier to eal interrupt

[Jeff Guo]

Add a new req notifier in eal interrupt for enable vfio hotplug.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v2->v1:
no change
---
 lib/librte_eal/common/include/rte_eal_interrupts.h |  1 +
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 71 ++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_eal_interrupts.h b/lib/librte_eal/common/include/rte_eal_interrupts.h
index 6eb4932..2c47738 100644
--- a/lib/librte_eal/common/include/rte_eal_interrupts.h
+++ b/lib/librte_eal/common/include/rte_eal_interrupts.h
@@ -35,6 +35,7 @@ enum rte_intr_handle_type {
 	RTE_INTR_HANDLE_EXT,          /**< external handler */
 	RTE_INTR_HANDLE_VDEV,         /**< virtual device */
 	RTE_INTR_HANDLE_DEV_EVENT,    /**< device event handle */
+	RTE_INTR_HANDLE_VFIO_REQ,  /**< vfio device handle (req) */
 	RTE_INTR_HANDLE_MAX           /**< count of elements */
 };
 
diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index 4076c6d..7f611b3 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -308,6 +308,64 @@ vfio_disable_msix(const struct rte_intr_handle *intr_handle) {
 
 	return ret;
 }
+
+/* enable req notifier */
+static int
+vfio_enable_req(const struct rte_intr_handle *intr_handle)
+{
+	int len, ret;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	struct vfio_irq_set *irq_set;
+	int *fd_ptr;
+
+	len = sizeof(irq_set_buf);
+
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 1;
+	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
+			 VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
+	irq_set->start = 0;
+	fd_ptr = (int *) &irq_set->data;
+	*fd_ptr = intr_handle->fd;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Error enabling req interrupts for fd %d\n",
+						intr_handle->fd);
+		return -1;
+	}
+
+	return 0;
+}
+
+/* disable req notifier */
+static int
+vfio_disable_req(const struct rte_intr_handle *intr_handle)
+{
+	struct vfio_irq_set *irq_set;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	int len, ret;
+
+	len = sizeof(struct vfio_irq_set);
+
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 0;
+	irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
+	irq_set->start = 0;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret)
+		RTE_LOG(ERR, EAL, "Error disabling req interrupts for fd %d\n",
+			intr_handle->fd);
+
+	return ret;
+}
 #endif
 
 static int
@@ -556,6 +614,10 @@ rte_intr_enable(const struct rte_intr_handle *intr_handle)
 		if (vfio_enable_intx(intr_handle))
 			return -1;
 		break;
+	case RTE_INTR_HANDLE_VFIO_REQ:
+		if (vfio_enable_req(intr_handle))
+			return -1;
+		break;
 #endif
 	/* not used at this moment */
 	case RTE_INTR_HANDLE_DEV_EVENT:
@@ -606,6 +668,11 @@ rte_intr_disable(const struct rte_intr_handle *intr_handle)
 		if (vfio_disable_intx(intr_handle))
 			return -1;
 		break;
+	case RTE_INTR_HANDLE_VFIO_REQ:
+		if (vfio_disable_req(intr_handle))
+			return -1;
+		break;
+
 #endif
 	/* not used at this moment */
 	case RTE_INTR_HANDLE_DEV_EVENT:
@@ -682,6 +749,10 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
 			bytes_read = 0;
 			call = true;
 			break;
+		case RTE_INTR_HANDLE_VFIO_REQ:
+			bytes_read = 0;
+			call = true;
+			break;
 		default:
 			bytes_read = 1;
 			break;
-- 
2.7.4

[PATCH v2 2/4] eal: modify device event callback process func

[Jeff Guo]

This patch modify the device event callback process function name to be
more explicit, change the variable to be const and exposure the API out
from private eal. The bus drivers and eal device would directly use this
API to process device event callback.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v2->v1:
change the rte_dev_event_callback_prcess from internal to external api
for bus or app usage.
---
 app/test-pmd/testpmd.c                  |  4 ++--
 lib/librte_eal/bsdapp/eal/eal_dev.c     |  8 ++++++++
 lib/librte_eal/common/eal_common_dev.c  |  5 +++--
 lib/librte_eal/common/eal_private.h     | 12 ------------
 lib/librte_eal/common/include/rte_dev.h | 18 +++++++++++++++++-
 lib/librte_eal/linuxapp/eal/eal_dev.c   |  2 +-
 lib/librte_eal/rte_eal_version.map      |  1 +
 7 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index bfef483..1313100 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -431,7 +431,7 @@ static void check_all_ports_link_status(uint32_t port_mask);
 static int eth_event_callback(portid_t port_id,
 			      enum rte_eth_event_type type,
 			      void *param, void *ret_param);
-static void eth_dev_event_callback(char *device_name,
+static void eth_dev_event_callback(const char *device_name,
 				enum rte_dev_event_type type,
 				void *param);
 static int eth_dev_event_callback_register(void);
@@ -2249,7 +2249,7 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 
 /* This function is used by the interrupt thread */
 static void
-eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
+eth_dev_event_callback(const char *device_name, enum rte_dev_event_type type,
 			     __rte_unused void *arg)
 {
 	uint16_t port_id;
diff --git a/lib/librte_eal/bsdapp/eal/eal_dev.c b/lib/librte_eal/bsdapp/eal/eal_dev.c
index ae1c558..374e737 100644
--- a/lib/librte_eal/bsdapp/eal/eal_dev.c
+++ b/lib/librte_eal/bsdapp/eal/eal_dev.c
@@ -33,3 +33,11 @@ rte_dev_hotplug_handle_disable(void)
 	RTE_LOG(ERR, EAL, "Device event is not supported for FreeBSD\n");
 	return -1;
 }
+
+void  __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event)
+{
+	RTE_LOG(ERR, EAL, "Device event callback process is not supported "
+		"for FreeBSD\n");
+}
diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 678dbca..2d610a4 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -342,8 +342,9 @@ rte_dev_event_callback_unregister(const char *device_name,
 	return ret;
 }
 
-void
-dev_callback_process(char *device_name, enum rte_dev_event_type event)
+void __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event)
 {
 	struct dev_event_callback *cb_lst;
 
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 637f20d..47e8a33 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -259,18 +259,6 @@ struct rte_bus *rte_bus_find_by_device_name(const char *str);
 int rte_mp_channel_init(void);
 
 /**
- * Internal Executes all the user application registered callbacks for
- * the specific device. It is for DPDK internal user only. User
- * application should not call it directly.
- *
- * @param device_name
- *  The device name.
- * @param event
- *  the device event type.
- */
-void dev_callback_process(char *device_name, enum rte_dev_event_type event);
-
-/**
  * @internal
  * Parse a device string and store its information in an
  * rte_devargs structure.
diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index ff580a0..58fab43 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -39,7 +39,7 @@ struct rte_dev_event {
 	char *devname;			/**< device name */
 };
 
-typedef void (*rte_dev_event_cb_fn)(char *device_name,
+typedef void (*rte_dev_event_cb_fn)(const char *device_name,
 					enum rte_dev_event_type event,
 					void *cb_arg);
 
@@ -438,6 +438,22 @@ rte_dev_event_callback_unregister(const char *device_name,
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
  *
+ * Executes all the user application registered callbacks for
+ * the specific device.
+ *
+ * @param device_name
+ *  The device name.
+ * @param event
+ *  the device event type.
+ */
+void  __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
  * Start the device event monitoring.
  *
  * @return
diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c b/lib/librte_eal/linuxapp/eal/eal_dev.c
index 9f9e1cf..01e3a04 100644
--- a/lib/librte_eal/linuxapp/eal/eal_dev.c
+++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
@@ -271,7 +271,7 @@ dev_uev_handler(__rte_unused void *param)
 				return;
 			}
 		}
-		dev_callback_process(uevent.devname, uevent.type);
+		rte_dev_event_callback_process(uevent.devname, uevent.type);
 	}
 }
 
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index a3255aa..b96da50 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -277,6 +277,7 @@ EXPERIMENTAL {
 	rte_class_register;
 	rte_class_unregister;
 	rte_ctrl_thread_create;
+	rte_dev_event_callback_process;
 	rte_dev_event_callback_register;
 	rte_dev_event_callback_unregister;
 	rte_dev_event_monitor_start;
-- 
2.7.4

[PATCH v2 3/4] pci: add req handler field to generic pci device

[Jeff Guo]

There are some extended interrupt types in vfio pci device except from the
existing interrupts, such as err and req notifier, they could be useful for
device error monitoring. And these corresponding interrupt handler is
different from the other interrupt handler that register in PMDs, so a new
interrupt handler should be added. This patch will add specific req handler
in generic pci device.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v2->v1:
no change
---
 drivers/bus/pci/rte_bus_pci.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
index 0d1955f..c45a820 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -66,6 +66,7 @@ struct rte_pci_device {
 	uint16_t max_vfs;                   /**< sriov enable if not zero */
 	enum rte_kernel_driver kdrv;        /**< Kernel driver passthrough */
 	char name[PCI_PRI_STR_SIZE+1];      /**< PCI location (ASCII) */
+	struct rte_intr_handle req_notifier_handler;/**< Req notifier handle */
 };
 
 /**
-- 
2.7.4

[PATCH v2 4/4] vfio: enable vfio hotplug by req notifier handler

[Jeff Guo]

When device is be hot-unplugged, the vfio kernel module will sent req
notifier to request user space to release the allocated resources at
first. After that, vfio kernel module will detect the device disappear,
and then delete the device in kernel.

This patch aim to add req notifier processing to enable hotplug for vfio.
By enable the req notifier monitoring and register the notifier callback,
when device be hot-unplugged, the hot-unplug handler will be called to
process hotplug for vfio.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v2->v1:
refine some code logic.
---
 drivers/bus/pci/linux/pci_vfio.c | 95 ++++++++++++++++++++++++++++++++++++++++
 drivers/bus/pci/pci_common.c     | 10 +++++
 2 files changed, 105 insertions(+)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 686386d..c780860 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -17,6 +17,8 @@
 #include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
 #include <rte_vfio.h>
+#include <rte_eal.h>
+#include <rte_bus.h>
 
 #include "eal_filesystem.h"
 
@@ -277,6 +279,89 @@ pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd)
 	return -1;
 }
 
+static void
+pci_vfio_req_handler(void *param)
+{
+	struct rte_bus *bus;
+	int ret;
+	struct rte_device *device = (struct rte_device *)param;
+
+	bus = rte_bus_find_by_device(device);
+	if (bus == NULL) {
+		RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n",
+			device->name);
+		return;
+	}
+
+	/**
+	 * vfio kernel module request user space to release allocated
+	 * resources before device be deleted in kernel, so it can directly
+	 * call the vfio bus hot-unplug handler to process it.
+	 */
+	ret = bus->hot_unplug_handler(device);
+	if (ret)
+		RTE_LOG(ERR, EAL, "Can not handle hot-unplug for "
+			"device (%s)\n", device->name);
+}
+
+/* enable notifier (only enable req now) */
+static int
+pci_vfio_enable_notifier(struct rte_pci_device *dev, int vfio_dev_fd)
+{
+	int ret;
+	int fd = -1;
+
+	/* set up an eventfd for req notifier */
+	fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
+	if (fd < 0) {
+		RTE_LOG(ERR, EAL, "Cannot set up eventfd, "
+				"error %i (%s)\n", errno, strerror(errno));
+		return -1;
+	}
+
+	dev->req_notifier_handler.fd = fd;
+	dev->req_notifier_handler.type = RTE_INTR_HANDLE_VFIO_REQ;
+	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
+	ret = rte_intr_callback_register(&dev->req_notifier_handler,
+					 pci_vfio_req_handler,
+					 (void *)&dev->device);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Fail to register req notifier handler.\n");
+		return -1;
+	}
+
+	ret = rte_intr_enable(&dev->req_notifier_handler);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Fail to enable req notifier.\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+/*disable notifier (only disable req now) */
+static int
+pci_vfio_disable_notifier(struct rte_pci_device *dev)
+{
+	int ret;
+
+	ret = rte_intr_disable(&dev->req_notifier_handler);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "fail to disable req notifier.\n");
+		return -1;
+	}
+
+	ret = rte_intr_callback_unregister(&dev->req_notifier_handler,
+					   pci_vfio_req_handler,
+					   (void *)&dev->device);
+	if (ret) {
+		RTE_LOG(ERR, EAL,
+			 "fail to unregister req notifier handler.\n");
+		return -1;
+	}
+	return 0;
+}
+
 static int
 pci_vfio_is_ioport_bar(int vfio_dev_fd, int bar_index)
 {
@@ -430,6 +515,7 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
+	dev->req_notifier_handler.fd = -1;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
@@ -521,6 +607,11 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
 		goto err_vfio_res;
 	}
 
+	if (pci_vfio_enable_notifier(dev, vfio_dev_fd) != 0) {
+		RTE_LOG(ERR, EAL, "Error setting up notifier!\n");
+		return -1;
+	}
+
 	TAILQ_INSERT_TAIL(vfio_res_list, vfio_res, next);
 
 	return 0;
@@ -546,6 +637,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
+	dev->req_notifier_handler.fd = -1;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
@@ -586,6 +678,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 
 	/* we need save vfio_dev_fd, so it can be used during release */
 	dev->intr_handle.vfio_dev_fd = vfio_dev_fd;
+	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
 
 	return 0;
 err_vfio_dev_fd:
@@ -658,6 +751,8 @@ pci_vfio_unmap_resource_primary(struct rte_pci_device *dev)
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
 			loc->domain, loc->bus, loc->devid, loc->function);
 
+	pci_vfio_disable_notifier(dev);
+
 	if (close(dev->intr_handle.fd) < 0) {
 		RTE_LOG(INFO, EAL, "Error when closing eventfd file descriptor for %s\n",
 			pci_addr);
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index f313fe9..2a8e5e9 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -446,6 +446,16 @@ pci_hot_unplug_handler(struct rte_device *dev)
 		return -1;
 
 	switch (pdev->kdrv) {
+	case RTE_KDRV_VFIO:
+		/**
+		 * vfio kernel module guaranty the pci device would not be
+		 * deleted until the user space release the resource, so no
+		 * need to remap BARs resource here, just directly notify
+		 * the req event to the user space to handle it.
+		 */
+		rte_dev_event_callback_process(dev->name,
+					       RTE_DEV_EVENT_REMOVE);
+		break;
 	case RTE_KDRV_IGB_UIO:
 	case RTE_KDRV_UIO_GENERIC:
 	case RTE_KDRV_NIC_UIO:
-- 
2.7.4

Re: [PATCH v1 4/5] pci: add req handler field to generic pci device

[Burakov, Anatoly]

On 29-Sep-18 7:15 AM, Jeff Guo wrote:
> 
> On 9/26/2018 8:22 PM, Burakov, Anatoly wrote:
>> On 17-Aug-18 11:51 AM, Jeff Guo wrote:
>>> There are some extended interrupt types in vfio pci device except 
>>> from the
>>> existing interrupts, such as err and req notifier, it could be useful 
>>> for
>>> device error monitoring. And these corresponding interrupt handler is
>>> different from the other interrupt handler that register in PMDs, so 
>>> a new
>>> interrupt handler should be added. This patch will add specific req 
>>> handler
>>> in generic pci device.
>>>
>>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>>> ---
>>>   drivers/bus/pci/rte_bus_pci.h | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/bus/pci/rte_bus_pci.h 
>>> b/drivers/bus/pci/rte_bus_pci.h
>>> index 0d1955f..c45a820 100644
>>> --- a/drivers/bus/pci/rte_bus_pci.h
>>> +++ b/drivers/bus/pci/rte_bus_pci.h
>>> @@ -66,6 +66,7 @@ struct rte_pci_device {
>>>       uint16_t max_vfs;                   /**< sriov enable if not 
>>> zero */
>>>       enum rte_kernel_driver kdrv;        /**< Kernel driver 
>>> passthrough */
>>>       char name[PCI_PRI_STR_SIZE+1];      /**< PCI location (ASCII) */
>>> +    struct rte_intr_handle req_notifier_handler;/**< Req notifier 
>>> handle */
>>>   };
>>>     /**
>>>
>>
>> Does this break ABI?
>>
> 
> If add a variable in struct would break ABI, it does.
> 
> 

Then it probably does. So, should probably bump PCI driver ABI version?

-- 
Thanks,
Anatoly

Re: [PATCH v2 1/4] eal: add a new req notifier to eal interrupt

[Andrew Rybchenko]

On 9/30/18 5:16 PM, Jeff Guo wrote:
> Add a new req notifier in eal interrupt for enable vfio hotplug.
>
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
> v2->v1:
> no change
> ---
>   lib/librte_eal/common/include/rte_eal_interrupts.h |  1 +
>   lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 71 ++++++++++++++++++++++
>   2 files changed, 72 insertions(+)
>
> diff --git a/lib/librte_eal/common/include/rte_eal_interrupts.h b/lib/librte_eal/common/include/rte_eal_interrupts.h
> index 6eb4932..2c47738 100644
> --- a/lib/librte_eal/common/include/rte_eal_interrupts.h
> +++ b/lib/librte_eal/common/include/rte_eal_interrupts.h
> @@ -35,6 +35,7 @@ enum rte_intr_handle_type {
>   	RTE_INTR_HANDLE_EXT,          /**< external handler */
>   	RTE_INTR_HANDLE_VDEV,         /**< virtual device */
>   	RTE_INTR_HANDLE_DEV_EVENT,    /**< device event handle */
> +	RTE_INTR_HANDLE_VFIO_REQ,  /**< vfio device handle (req) */

Alignment looks inconsistent. Is there any reason?

>   	RTE_INTR_HANDLE_MAX           /**< count of elements */
>   };
>   
> diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> index 4076c6d..7f611b3 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> @@ -308,6 +308,64 @@ vfio_disable_msix(const struct rte_intr_handle *intr_handle) {
>   
>   	return ret;
>   }
> +
> +/* enable req notifier */
> +static int
> +vfio_enable_req(const struct rte_intr_handle *intr_handle)
> +{
> +	int len, ret;
> +	char irq_set_buf[IRQ_SET_BUF_LEN];

I see that it is copied from similar functions in the file, but
I'd suggest to initialize it with zeros using '= {};' just to make
sure that uninitialized on-stack data never go to kernel.

> +	struct vfio_irq_set *irq_set;
> +	int *fd_ptr;
> +
> +	len = sizeof(irq_set_buf);
> +
> +	irq_set = (struct vfio_irq_set *) irq_set_buf;
> +	irq_set->argsz = len;
> +	irq_set->count = 1;
> +	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> +			 VFIO_IRQ_SET_ACTION_TRIGGER;
> +	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;

It looks like it is the only difference (plus error log below) from
vfio_enable_msi(). May be it make sense to restructure code
to avoid duplication. Obviously it is not critical, but please, consider.

Similar comment is applicable to vfio_disable_req() below.

> +	irq_set->start = 0;
> +	fd_ptr = (int *) &irq_set->data;
> +	*fd_ptr = intr_handle->fd;
> +
> +	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
> +
> +	if (ret) {
> +		RTE_LOG(ERR, EAL, "Error enabling req interrupts for fd %d\n",
> +						intr_handle->fd);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +/* disable req notifier */
> +static int
> +vfio_disable_req(const struct rte_intr_handle *intr_handle)
> +{
> +	struct vfio_irq_set *irq_set;
> +	char irq_set_buf[IRQ_SET_BUF_LEN];
> +	int len, ret;
> +
> +	len = sizeof(struct vfio_irq_set);
> +
> +	irq_set = (struct vfio_irq_set *) irq_set_buf;
> +	irq_set->argsz = len;
> +	irq_set->count = 0;
> +	irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER;
> +	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
> +	irq_set->start = 0;
> +
> +	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
> +
> +	if (ret)
> +		RTE_LOG(ERR, EAL, "Error disabling req interrupts for fd %d\n",
> +			intr_handle->fd);
> +
> +	return ret;
> +}
>   #endif
>   
>   static int
> @@ -556,6 +614,10 @@ rte_intr_enable(const struct rte_intr_handle *intr_handle)
>   		if (vfio_enable_intx(intr_handle))
>   			return -1;
>   		break;
> +	case RTE_INTR_HANDLE_VFIO_REQ:
> +		if (vfio_enable_req(intr_handle))
> +			return -1;
> +		break;
>   #endif
>   	/* not used at this moment */
>   	case RTE_INTR_HANDLE_DEV_EVENT:
> @@ -606,6 +668,11 @@ rte_intr_disable(const struct rte_intr_handle *intr_handle)
>   		if (vfio_disable_intx(intr_handle))
>   			return -1;
>   		break;
> +	case RTE_INTR_HANDLE_VFIO_REQ:
> +		if (vfio_disable_req(intr_handle))
> +			return -1;
> +		break;
> +
>   #endif
>   	/* not used at this moment */
>   	case RTE_INTR_HANDLE_DEV_EVENT:
> @@ -682,6 +749,10 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
>   			bytes_read = 0;
>   			call = true;
>   			break;
> +		case RTE_INTR_HANDLE_VFIO_REQ:
> +			bytes_read = 0;
> +			call = true;
> +			break;
>   		default:
>   			bytes_read = 1;
>   			break;

Re: [PATCH v2 2/4] eal: modify device event callback process func

[Andrew Rybchenko]

On 9/30/18 5:16 PM, Jeff Guo wrote:
> This patch modify the device event callback process function name to be
> more explicit, change the variable to be const and exposure the API out
> from private eal. The bus drivers and eal device would directly use this
> API to process device event callback.

Sorry, but from the description it is unclear what has changed to
make it necessary.

> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
> v2->v1:
> change the rte_dev_event_callback_prcess from internal to external api
> for bus or app usage.
> ---
>   app/test-pmd/testpmd.c                  |  4 ++--
>   lib/librte_eal/bsdapp/eal/eal_dev.c     |  8 ++++++++
>   lib/librte_eal/common/eal_common_dev.c  |  5 +++--
>   lib/librte_eal/common/eal_private.h     | 12 ------------
>   lib/librte_eal/common/include/rte_dev.h | 18 +++++++++++++++++-
>   lib/librte_eal/linuxapp/eal/eal_dev.c   |  2 +-
>   lib/librte_eal/rte_eal_version.map      |  1 +
>   7 files changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index bfef483..1313100 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -431,7 +431,7 @@ static void check_all_ports_link_status(uint32_t port_mask);
>   static int eth_event_callback(portid_t port_id,
>   			      enum rte_eth_event_type type,
>   			      void *param, void *ret_param);
> -static void eth_dev_event_callback(char *device_name,
> +static void eth_dev_event_callback(const char *device_name,
>   				enum rte_dev_event_type type,
>   				void *param);
>   static int eth_dev_event_callback_register(void);
> @@ -2249,7 +2249,7 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>   
>   /* This function is used by the interrupt thread */
>   static void
> -eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
> +eth_dev_event_callback(const char *device_name, enum rte_dev_event_type type,
>   			     __rte_unused void *arg)
>   {
>   	uint16_t port_id;
> diff --git a/lib/librte_eal/bsdapp/eal/eal_dev.c b/lib/librte_eal/bsdapp/eal/eal_dev.c
> index ae1c558..374e737 100644
> --- a/lib/librte_eal/bsdapp/eal/eal_dev.c
> +++ b/lib/librte_eal/bsdapp/eal/eal_dev.c
> @@ -33,3 +33,11 @@ rte_dev_hotplug_handle_disable(void)
>   	RTE_LOG(ERR, EAL, "Device event is not supported for FreeBSD\n");
>   	return -1;
>   }
> +
> +void  __rte_experimental
> +rte_dev_event_callback_process(const char *device_name,
> +			       enum rte_dev_event_type event)
> +{
> +	RTE_LOG(ERR, EAL, "Device event callback process is not supported "
> +		"for FreeBSD\n");

Consider to avoid split of log message to simplify search using grep.

> +}
> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> index 678dbca..2d610a4 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -342,8 +342,9 @@ rte_dev_event_callback_unregister(const char *device_name,
>   	return ret;
>   }
>   
> -void
> -dev_callback_process(char *device_name, enum rte_dev_event_type event)
> +void __rte_experimental
> +rte_dev_event_callback_process(const char *device_name,
> +			       enum rte_dev_event_type event)
>   {
>   	struct dev_event_callback *cb_lst;
>   
> diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
> index 637f20d..47e8a33 100644
> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -259,18 +259,6 @@ struct rte_bus *rte_bus_find_by_device_name(const char *str);
>   int rte_mp_channel_init(void);
>   
>   /**
> - * Internal Executes all the user application registered callbacks for
> - * the specific device. It is for DPDK internal user only. User
> - * application should not call it directly.
> - *
> - * @param device_name
> - *  The device name.
> - * @param event
> - *  the device event type.
> - */
> -void dev_callback_process(char *device_name, enum rte_dev_event_type event);
> -
> -/**
>    * @internal
>    * Parse a device string and store its information in an
>    * rte_devargs structure.
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index ff580a0..58fab43 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -39,7 +39,7 @@ struct rte_dev_event {
>   	char *devname;			/**< device name */
>   };
>   
> -typedef void (*rte_dev_event_cb_fn)(char *device_name,
> +typedef void (*rte_dev_event_cb_fn)(const char *device_name,
>   					enum rte_dev_event_type event,
>   					void *cb_arg);
>   
> @@ -438,6 +438,22 @@ rte_dev_event_callback_unregister(const char *device_name,
>    * @warning
>    * @b EXPERIMENTAL: this API may change without prior notice
>    *
> + * Executes all the user application registered callbacks for
> + * the specific device.
> + *
> + * @param device_name
> + *  The device name.
> + * @param event
> + *  the device event type.
> + */
> +void  __rte_experimental
> +rte_dev_event_callback_process(const char *device_name,
> +			       enum rte_dev_event_type event);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
>    * Start the device event monitoring.
>    *
>    * @return
> diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c b/lib/librte_eal/linuxapp/eal/eal_dev.c
> index 9f9e1cf..01e3a04 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_dev.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
> @@ -271,7 +271,7 @@ dev_uev_handler(__rte_unused void *param)
>   				return;
>   			}
>   		}
> -		dev_callback_process(uevent.devname, uevent.type);
> +		rte_dev_event_callback_process(uevent.devname, uevent.type);
>   	}
>   }
>   
> diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
> index a3255aa..b96da50 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -277,6 +277,7 @@ EXPERIMENTAL {
>   	rte_class_register;
>   	rte_class_unregister;
>   	rte_ctrl_thread_create;
> +	rte_dev_event_callback_process;
>   	rte_dev_event_callback_register;
>   	rte_dev_event_callback_unregister;
>   	rte_dev_event_monitor_start;

Re: [PATCH v2 3/4] pci: add req handler field to generic pci device

[Andrew Rybchenko]

On 9/30/18 5:16 PM, Jeff Guo wrote:
> There are some extended interrupt types in vfio pci device except from the
> existing interrupts, such as err and req notifier, they could be useful for
> device error monitoring. And these corresponding interrupt handler is
> different from the other interrupt handler that register in PMDs, so a new
> interrupt handler should be added. This patch will add specific req handler
> in generic pci device.
>
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
> v2->v1:
> no change
> ---
>   drivers/bus/pci/rte_bus_pci.h | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
> index 0d1955f..c45a820 100644
> --- a/drivers/bus/pci/rte_bus_pci.h
> +++ b/drivers/bus/pci/rte_bus_pci.h
> @@ -66,6 +66,7 @@ struct rte_pci_device {
>   	uint16_t max_vfs;                   /**< sriov enable if not zero */
>   	enum rte_kernel_driver kdrv;        /**< Kernel driver passthrough */
>   	char name[PCI_PRI_STR_SIZE+1];      /**< PCI location (ASCII) */
> +	struct rte_intr_handle req_notifier_handler;/**< Req notifier handle */

It is uninitialized now everywhere the structure is used. In the best case
it is initialized with zeros (if the structure is memset or partly 
initialized
on declaration) and zero FD is a valid file descriptor. Is it OK?

>   };
>   
>   /**

Re: [PATCH v2 4/4] vfio: enable vfio hotplug by req notifier handler

[Andrew Rybchenko]

On 9/30/18 5:16 PM, Jeff Guo wrote:
> When device is be hot-unplugged, the vfio kernel module will sent req
> notifier to request user space to release the allocated resources at
> first. After that, vfio kernel module will detect the device disappear,
> and then delete the device in kernel.
>
> This patch aim to add req notifier processing to enable hotplug for vfio.
> By enable the req notifier monitoring and register the notifier callback,
> when device be hot-unplugged, the hot-unplug handler will be called to
> process hotplug for vfio.
>
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
> v2->v1:
> refine some code logic.
> ---
>   drivers/bus/pci/linux/pci_vfio.c | 95 ++++++++++++++++++++++++++++++++++++++++
>   drivers/bus/pci/pci_common.c     | 10 +++++
>   2 files changed, 105 insertions(+)
>
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index 686386d..c780860 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -17,6 +17,8 @@
>   #include <rte_eal_memconfig.h>
>   #include <rte_malloc.h>
>   #include <rte_vfio.h>
> +#include <rte_eal.h>
> +#include <rte_bus.h>
>   
>   #include "eal_filesystem.h"
>   
> @@ -277,6 +279,89 @@ pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd)
>   	return -1;
>   }
>   
> +static void
> +pci_vfio_req_handler(void *param)
> +{
> +	struct rte_bus *bus;
> +	int ret;
> +	struct rte_device *device = (struct rte_device *)param;
> +
> +	bus = rte_bus_find_by_device(device);
> +	if (bus == NULL) {
> +		RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n",
> +			device->name);
> +		return;
> +	}
> +
> +	/**

Why is doxygen style comment used here?

> +	 * vfio kernel module request user space to release allocated
> +	 * resources before device be deleted in kernel, so it can directly
> +	 * call the vfio bus hot-unplug handler to process it.
> +	 */
> +	ret = bus->hot_unplug_handler(device);
> +	if (ret)
> +		RTE_LOG(ERR, EAL, "Can not handle hot-unplug for "
> +			"device (%s)\n", device->name);

Consider to avoid format string split to simplify search using grep.

> +}
> +
> +/* enable notifier (only enable req now) */
> +static int
> +pci_vfio_enable_notifier(struct rte_pci_device *dev, int vfio_dev_fd)
> +{
> +	int ret;
> +	int fd = -1;
> +
> +	/* set up an eventfd for req notifier */
> +	fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
> +	if (fd < 0) {
> +		RTE_LOG(ERR, EAL, "Cannot set up eventfd, "
> +				"error %i (%s)\n", errno, strerror(errno));

Consider to avoid format string split to simplify search using grep.

> +		return -1;
> +	}
> +
> +	dev->req_notifier_handler.fd = fd;
> +	dev->req_notifier_handler.type = RTE_INTR_HANDLE_VFIO_REQ;
> +	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
> +	ret = rte_intr_callback_register(&dev->req_notifier_handler,
> +					 pci_vfio_req_handler,
> +					 (void *)&dev->device);
> +	if (ret) {
> +		RTE_LOG(ERR, EAL, "Fail to register req notifier handler.\n");

I think we should close(fd) here.

> +		return -1;
> +	}
> +
> +	ret = rte_intr_enable(&dev->req_notifier_handler);
> +	if (ret) {
> +		RTE_LOG(ERR, EAL, "Fail to enable req notifier.\n");

I think we should unregister notifier and close(fd) here.

> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +/*disable notifier (only disable req now) */

Space is missing before disable.

> +static int
> +pci_vfio_disable_notifier(struct rte_pci_device *dev)
> +{
> +	int ret;
> +
> +	ret = rte_intr_disable(&dev->req_notifier_handler);
> +	if (ret) {
> +		RTE_LOG(ERR, EAL, "fail to disable req notifier.\n");

I'd like to understand correct way handle errors here. Should we
terminate here or continue and unregister handler and close FD anyway?

> +		return -1;
> +	}
> +
> +	ret = rte_intr_callback_unregister(&dev->req_notifier_handler,
> +					   pci_vfio_req_handler,
> +					   (void *)&dev->device);
> +	if (ret) {
> +		RTE_LOG(ERR, EAL,
> +			 "fail to unregister req notifier handler.\n");
> +		return -1;
> +	}

Shoudn't we close eventfd?

> +	return 0;
> +}
> +
>   static int
>   pci_vfio_is_ioport_bar(int vfio_dev_fd, int bar_index)
>   {
> @@ -430,6 +515,7 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
>   	struct pci_map *maps;
>   
>   	dev->intr_handle.fd = -1;
> +	dev->req_notifier_handler.fd = -1;
>   
>   	/* store PCI address string */
>   	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
> @@ -521,6 +607,11 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
>   		goto err_vfio_res;
>   	}
>   
> +	if (pci_vfio_enable_notifier(dev, vfio_dev_fd) != 0) {
> +		RTE_LOG(ERR, EAL, "Error setting up notifier!\n");
> +		return -1;

I think we should do goto to make required cleanup.

> +	}
> +
>   	TAILQ_INSERT_TAIL(vfio_res_list, vfio_res, next);
>   
>   	return 0;
> @@ -546,6 +637,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
>   	struct pci_map *maps;
>   
>   	dev->intr_handle.fd = -1;
> +	dev->req_notifier_handler.fd = -1;
>   
>   	/* store PCI address string */
>   	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
> @@ -586,6 +678,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
>   
>   	/* we need save vfio_dev_fd, so it can be used during release */
>   	dev->intr_handle.vfio_dev_fd = vfio_dev_fd;
> +	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
>   
>   	return 0;
>   err_vfio_dev_fd:
> @@ -658,6 +751,8 @@ pci_vfio_unmap_resource_primary(struct rte_pci_device *dev)
>   	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
>   			loc->domain, loc->bus, loc->devid, loc->function);
>   
> +	pci_vfio_disable_notifier(dev);
> +

Is it OK to ignore disable failure here? Why? It would be good to see
explanations in comments. Does it leak eventfd now?

>   	if (close(dev->intr_handle.fd) < 0) {
>   		RTE_LOG(INFO, EAL, "Error when closing eventfd file descriptor for %s\n",
>   			pci_addr);
> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
> index f313fe9..2a8e5e9 100644
> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -446,6 +446,16 @@ pci_hot_unplug_handler(struct rte_device *dev)
>   		return -1;
>   
>   	switch (pdev->kdrv) {
> +	case RTE_KDRV_VFIO:
> +		/**

Why is doxygen style comment is used here?

> +		 * vfio kernel module guaranty the pci device would not be
> +		 * deleted until the user space release the resource, so no
> +		 * need to remap BARs resource here, just directly notify
> +		 * the req event to the user space to handle it.
> +		 */
> +		rte_dev_event_callback_process(dev->name,
> +					       RTE_DEV_EVENT_REMOVE);
> +		break;
>   	case RTE_KDRV_IGB_UIO:
>   	case RTE_KDRV_UIO_GENERIC:
>   	case RTE_KDRV_NIC_UIO:

Re: [PATCH v2 1/4] eal: add a new req notifier to eal interrupt

[Jeff Guo]

hi, andrew

thanks for your review again, see comment as below.

On 10/1/2018 5:46 PM, Andrew Rybchenko wrote:
> On 9/30/18 5:16 PM, Jeff Guo wrote:
>> Add a new req notifier in eal interrupt for enable vfio hotplug.
>>
>> Signed-off-by: Jeff Guo<jia.guo@intel.com>
>> ---
>> v2->v1:
>> no change
>> ---
>>   lib/librte_eal/common/include/rte_eal_interrupts.h |  1 +
>>   lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 71 ++++++++++++++++++++++
>>   2 files changed, 72 insertions(+)
>>
>> diff --git a/lib/librte_eal/common/include/rte_eal_interrupts.h b/lib/librte_eal/common/include/rte_eal_interrupts.h
>> index 6eb4932..2c47738 100644
>> --- a/lib/librte_eal/common/include/rte_eal_interrupts.h
>> +++ b/lib/librte_eal/common/include/rte_eal_interrupts.h
>> @@ -35,6 +35,7 @@ enum rte_intr_handle_type {
>>   	RTE_INTR_HANDLE_EXT,          /**< external handler */
>>   	RTE_INTR_HANDLE_VDEV,         /**< virtual device */
>>   	RTE_INTR_HANDLE_DEV_EVENT,    /**< device event handle */
>> +	RTE_INTR_HANDLE_VFIO_REQ,  /**< vfio device handle (req) */
>
> Alignment looks inconsistent. Is there any reason?
>

ok, it should be consistent.


>>   	RTE_INTR_HANDLE_MAX           /**< count of elements */
>>   };
>>   
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
>> index 4076c6d..7f611b3 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
>> @@ -308,6 +308,64 @@ vfio_disable_msix(const struct rte_intr_handle *intr_handle) {
>>   
>>   	return ret;
>>   }
>> +
>> +/* enable req notifier */
>> +static int
>> +vfio_enable_req(const struct rte_intr_handle *intr_handle)
>> +{
>> +	int len, ret;
>> +	char irq_set_buf[IRQ_SET_BUF_LEN];
>
> I see that it is copied from similar functions in the file, but
> I'd suggest to initialize it with zeros using '= {};' just to make
> sure that uninitialized on-stack data never go to kernel.
>
>> +	struct vfio_irq_set *irq_set;
>> +	int *fd_ptr;
>> +
>> +	len = sizeof(irq_set_buf);
>> +
>> +	irq_set = (struct vfio_irq_set *) irq_set_buf;
>> +	irq_set->argsz = len;
>> +	irq_set->count = 1;
>> +	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>> +			 VFIO_IRQ_SET_ACTION_TRIGGER;
>> +	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
>
> It looks like it is the only difference (plus error log below) from
> vfio_enable_msi(). May be it make sense to restructure code
> to avoid duplication. Obviously it is not critical, but please, consider.
>
> Similar comment is applicable to vfio_disable_req() below.
>

make sense, and i found that there are many duplication could be modify 
here, maybe we could just add param index type to

some common function to enable/disable each type of the interrupts/req. 
I suggest to make other patch set which is aim to modify

eal interrupt to do it. The same as the your above comment about char 
array initialize.  Do you agree with that?


>> +	irq_set->start = 0;
>> +	fd_ptr = (int *) &irq_set->data;
>> +	*fd_ptr = intr_handle->fd;
>> +
>> +	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> +
>> +	if (ret) {
>> +		RTE_LOG(ERR, EAL, "Error enabling req interrupts for fd %d\n",
>> +						intr_handle->fd);
>> +		return -1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/* disable req notifier */
>> +static int
>> +vfio_disable_req(const struct rte_intr_handle *intr_handle)
>> +{
>> +	struct vfio_irq_set *irq_set;
>> +	char irq_set_buf[IRQ_SET_BUF_LEN];
>> +	int len, ret;
>> +
>> +	len = sizeof(struct vfio_irq_set);
>> +
>> +	irq_set = (struct vfio_irq_set *) irq_set_buf;
>> +	irq_set->argsz = len;
>> +	irq_set->count = 0;
>> +	irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER;
>> +	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
>> +	irq_set->start = 0;
>> +
>> +	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> +
>> +	if (ret)
>> +		RTE_LOG(ERR, EAL, "Error disabling req interrupts for fd %d\n",
>> +			intr_handle->fd);
>> +
>> +	return ret;
>> +}
>>   #endif
>>   
>>   static int
>> @@ -556,6 +614,10 @@ rte_intr_enable(const struct rte_intr_handle *intr_handle)
>>   		if (vfio_enable_intx(intr_handle))
>>   			return -1;
>>   		break;
>> +	case RTE_INTR_HANDLE_VFIO_REQ:
>> +		if (vfio_enable_req(intr_handle))
>> +			return -1;
>> +		break;
>>   #endif
>>   	/* not used at this moment */
>>   	case RTE_INTR_HANDLE_DEV_EVENT:
>> @@ -606,6 +668,11 @@ rte_intr_disable(const struct rte_intr_handle *intr_handle)
>>   		if (vfio_disable_intx(intr_handle))
>>   			return -1;
>>   		break;
>> +	case RTE_INTR_HANDLE_VFIO_REQ:
>> +		if (vfio_disable_req(intr_handle))
>> +			return -1;
>> +		break;
>> +
>>   #endif
>>   	/* not used at this moment */
>>   	case RTE_INTR_HANDLE_DEV_EVENT:
>> @@ -682,6 +749,10 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
>>   			bytes_read = 0;
>>   			call = true;
>>   			break;
>> +		case RTE_INTR_HANDLE_VFIO_REQ:
>> +			bytes_read = 0;
>> +			call = true;
>> +			break;
>>   		default:
>>   			bytes_read = 1;
>>   			break;
>

Re: [PATCH v2 2/4] eal: modify device event callback process func

[Jeff Guo]

andrew,

On 10/1/2018 5:46 PM, Andrew Rybchenko wrote:
> On 9/30/18 5:16 PM, Jeff Guo wrote:
>> This patch modify the device event callback process function name to be
>> more explicit, change the variable to be const and exposure the API out
>> from private eal. The bus drivers and eal device would directly use this
>> API to process device event callback.
>
> Sorry, but from the description it is unclear what has changed to
> make it necessary.
>

the reason is that not only eal device help will use the callback, but 
also vfio bus will use

the callback to handle hot-unplug. So exposure these API to let vfio 
call it. I will add detail

more at next version if need.


>> Signed-off-by: Jeff Guo<jia.guo@intel.com>
>> ---
>> v2->v1:
>> change the rte_dev_event_callback_prcess from internal to external api
>> for bus or app usage.
>> ---
>>   app/test-pmd/testpmd.c                  |  4 ++--
>>   lib/librte_eal/bsdapp/eal/eal_dev.c     |  8 ++++++++
>>   lib/librte_eal/common/eal_common_dev.c  |  5 +++--
>>   lib/librte_eal/common/eal_private.h     | 12 ------------
>>   lib/librte_eal/common/include/rte_dev.h | 18 +++++++++++++++++-
>>   lib/librte_eal/linuxapp/eal/eal_dev.c   |  2 +-
>>   lib/librte_eal/rte_eal_version.map      |  1 +
>>   7 files changed, 32 insertions(+), 18 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index bfef483..1313100 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -431,7 +431,7 @@ static void check_all_ports_link_status(uint32_t port_mask);
>>   static int eth_event_callback(portid_t port_id,
>>   			      enum rte_eth_event_type type,
>>   			      void *param, void *ret_param);
>> -static void eth_dev_event_callback(char *device_name,
>> +static void eth_dev_event_callback(const char *device_name,
>>   				enum rte_dev_event_type type,
>>   				void *param);
>>   static int eth_dev_event_callback_register(void);
>> @@ -2249,7 +2249,7 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>>   
>>   /* This function is used by the interrupt thread */
>>   static void
>> -eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
>> +eth_dev_event_callback(const char *device_name, enum rte_dev_event_type type,
>>   			     __rte_unused void *arg)
>>   {
>>   	uint16_t port_id;
>> diff --git a/lib/librte_eal/bsdapp/eal/eal_dev.c b/lib/librte_eal/bsdapp/eal/eal_dev.c
>> index ae1c558..374e737 100644
>> --- a/lib/librte_eal/bsdapp/eal/eal_dev.c
>> +++ b/lib/librte_eal/bsdapp/eal/eal_dev.c
>> @@ -33,3 +33,11 @@ rte_dev_hotplug_handle_disable(void)
>>   	RTE_LOG(ERR, EAL, "Device event is not supported for FreeBSD\n");
>>   	return -1;
>>   }
>> +
>> +void  __rte_experimental
>> +rte_dev_event_callback_process(const char *device_name,
>> +			       enum rte_dev_event_type event)
>> +{
>> +	RTE_LOG(ERR, EAL, "Device event callback process is not supported "
>> +		"for FreeBSD\n");
>
> Consider to avoid split of log message to simplify search using grep.
>

i think i got your point, but the 80 limitation a  line and the clearly 
meaning of the log should be the 1 prior to be considerate,

right? It should be considerate but it is inevitable.


>> +}
>> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
>> index 678dbca..2d610a4 100644
>> --- a/lib/librte_eal/common/eal_common_dev.c
>> +++ b/lib/librte_eal/common/eal_common_dev.c
>> @@ -342,8 +342,9 @@ rte_dev_event_callback_unregister(const char *device_name,
>>   	return ret;
>>   }
>>   
>> -void
>> -dev_callback_process(char *device_name, enum rte_dev_event_type event)
>> +void __rte_experimental
>> +rte_dev_event_callback_process(const char *device_name,
>> +			       enum rte_dev_event_type event)
>>   {
>>   	struct dev_event_callback *cb_lst;
>>   
>> diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
>> index 637f20d..47e8a33 100644
>> --- a/lib/librte_eal/common/eal_private.h
>> +++ b/lib/librte_eal/common/eal_private.h
>> @@ -259,18 +259,6 @@ struct rte_bus *rte_bus_find_by_device_name(const char *str);
>>   int rte_mp_channel_init(void);
>>   
>>   /**
>> - * Internal Executes all the user application registered callbacks for
>> - * the specific device. It is for DPDK internal user only. User
>> - * application should not call it directly.
>> - *
>> - * @param device_name
>> - *  The device name.
>> - * @param event
>> - *  the device event type.
>> - */
>> -void dev_callback_process(char *device_name, enum rte_dev_event_type event);
>> -
>> -/**
>>    * @internal
>>    * Parse a device string and store its information in an
>>    * rte_devargs structure.
>> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
>> index ff580a0..58fab43 100644
>> --- a/lib/librte_eal/common/include/rte_dev.h
>> +++ b/lib/librte_eal/common/include/rte_dev.h
>> @@ -39,7 +39,7 @@ struct rte_dev_event {
>>   	char *devname;			/**< device name */
>>   };
>>   
>> -typedef void (*rte_dev_event_cb_fn)(char *device_name,
>> +typedef void (*rte_dev_event_cb_fn)(const char *device_name,
>>   					enum rte_dev_event_type event,
>>   					void *cb_arg);
>>   
>> @@ -438,6 +438,22 @@ rte_dev_event_callback_unregister(const char *device_name,
>>    * @warning
>>    * @b EXPERIMENTAL: this API may change without prior notice
>>    *
>> + * Executes all the user application registered callbacks for
>> + * the specific device.
>> + *
>> + * @param device_name
>> + *  The device name.
>> + * @param event
>> + *  the device event type.
>> + */
>> +void  __rte_experimental
>> +rte_dev_event_callback_process(const char *device_name,
>> +			       enum rte_dev_event_type event);
>> +
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change without prior notice
>> + *
>>    * Start the device event monitoring.
>>    *
>>    * @return
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c b/lib/librte_eal/linuxapp/eal/eal_dev.c
>> index 9f9e1cf..01e3a04 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_dev.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
>> @@ -271,7 +271,7 @@ dev_uev_handler(__rte_unused void *param)
>>   				return;
>>   			}
>>   		}
>> -		dev_callback_process(uevent.devname, uevent.type);
>> +		rte_dev_event_callback_process(uevent.devname, uevent.type);
>>   	}
>>   }
>>   
>> diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
>> index a3255aa..b96da50 100644
>> --- a/lib/librte_eal/rte_eal_version.map
>> +++ b/lib/librte_eal/rte_eal_version.map
>> @@ -277,6 +277,7 @@ EXPERIMENTAL {
>>   	rte_class_register;
>>   	rte_class_unregister;
>>   	rte_ctrl_thread_create;
>> +	rte_dev_event_callback_process;
>>   	rte_dev_event_callback_register;
>>   	rte_dev_event_callback_unregister;
>>   	rte_dev_event_monitor_start;
>

Re: [PATCH v2 4/4] vfio: enable vfio hotplug by req notifier handler

[Jeff Guo]

thanks , andrew.

On 10/1/2018 5:47 PM, Andrew Rybchenko wrote:
> On 9/30/18 5:16 PM, Jeff Guo wrote:
>> When device is be hot-unplugged, the vfio kernel module will sent req
>> notifier to request user space to release the allocated resources at
>> first. After that, vfio kernel module will detect the device disappear,
>> and then delete the device in kernel.
>>
>> This patch aim to add req notifier processing to enable hotplug for vfio.
>> By enable the req notifier monitoring and register the notifier callback,
>> when device be hot-unplugged, the hot-unplug handler will be called to
>> process hotplug for vfio.
>>
>> Signed-off-by: Jeff Guo<jia.guo@intel.com>
>> ---
>> v2->v1:
>> refine some code logic.
>> ---
>>   drivers/bus/pci/linux/pci_vfio.c | 95 ++++++++++++++++++++++++++++++++++++++++
>>   drivers/bus/pci/pci_common.c     | 10 +++++
>>   2 files changed, 105 insertions(+)
>>
>> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
>> index 686386d..c780860 100644
>> --- a/drivers/bus/pci/linux/pci_vfio.c
>> +++ b/drivers/bus/pci/linux/pci_vfio.c
>> @@ -17,6 +17,8 @@
>>   #include <rte_eal_memconfig.h>
>>   #include <rte_malloc.h>
>>   #include <rte_vfio.h>
>> +#include <rte_eal.h>
>> +#include <rte_bus.h>
>>   
>>   #include "eal_filesystem.h"
>>   
>> @@ -277,6 +279,89 @@ pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd)
>>   	return -1;
>>   }
>>   
>> +static void
>> +pci_vfio_req_handler(void *param)
>> +{
>> +	struct rte_bus *bus;
>> +	int ret;
>> +	struct rte_device *device = (struct rte_device *)param;
>> +
>> +	bus = rte_bus_find_by_device(device);
>> +	if (bus == NULL) {
>> +		RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n",
>> +			device->name);
>> +		return;
>> +	}
>> +
>> +	/**
>
> Why is doxygen style comment used here?
>

my fault, thanks.


>> +	 * vfio kernel module request user space to release allocated
>> +	 * resources before device be deleted in kernel, so it can directly
>> +	 * call the vfio bus hot-unplug handler to process it.
>> +	 */
>> +	ret = bus->hot_unplug_handler(device);
>> +	if (ret)
>> +		RTE_LOG(ERR, EAL, "Can not handle hot-unplug for "
>> +			"device (%s)\n", device->name);
>
> Consider to avoid format string split to simplify search using grep.
>

ok, i think i could try more to make it not split.


>> +}
>> +
>> +/* enable notifier (only enable req now) */
>> +static int
>> +pci_vfio_enable_notifier(struct rte_pci_device *dev, int vfio_dev_fd)
>> +{
>> +	int ret;
>> +	int fd = -1;
>> +
>> +	/* set up an eventfd for req notifier */
>> +	fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
>> +	if (fd < 0) {
>> +		RTE_LOG(ERR, EAL, "Cannot set up eventfd, "
>> +				"error %i (%s)\n", errno, strerror(errno));
>
> Consider to avoid format string split to simplify search using grep.
>

ok.


>> +		return -1;
>> +	}
>> +
>> +	dev->req_notifier_handler.fd = fd;
>> +	dev->req_notifier_handler.type = RTE_INTR_HANDLE_VFIO_REQ;
>> +	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
>> +	ret = rte_intr_callback_register(&dev->req_notifier_handler,
>> +					 pci_vfio_req_handler,
>> +					 (void *)&dev->device);
>> +	if (ret) {
>> +		RTE_LOG(ERR, EAL, "Fail to register req notifier handler.\n");
>
> I think we should close(fd) here.
>

you are right here.


>> +		return -1;
>> +	}
>> +
>> +	ret = rte_intr_enable(&dev->req_notifier_handler);
>> +	if (ret) {
>> +		RTE_LOG(ERR, EAL, "Fail to enable req notifier.\n");
>
> I think we should unregister notifier and close(fd) here.
>

ok.


>> +		return -1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/*disable notifier (only disable req now) */
>
> Space is missing before disable.
>

thanks.


>> +static int
>> +pci_vfio_disable_notifier(struct rte_pci_device *dev)
>> +{
>> +	int ret;
>> +
>> +	ret = rte_intr_disable(&dev->req_notifier_handler);
>> +	if (ret) {
>> +		RTE_LOG(ERR, EAL, "fail to disable req notifier.\n");
>
> I'd like to understand correct way handle errors here. Should we
> terminate here or continue and unregister handler and close FD anyway?
>

I think terminate and directly return anyway should be ok, just show the 
disable failure is make sense.


>> +		return -1;
>> +	}
>> +
>> +	ret = rte_intr_callback_unregister(&dev->req_notifier_handler,
>> +					   pci_vfio_req_handler,
>> +					   (void *)&dev->device);
>> +	if (ret) {
>> +		RTE_LOG(ERR, EAL,
>> +			 "fail to unregister req notifier handler.\n");
>> +		return -1;
>> +	}
>
> Shoudn't we close eventfd?
>

It should be, i think.


>> +	return 0;
>> +}
>> +
>>   static int
>>   pci_vfio_is_ioport_bar(int vfio_dev_fd, int bar_index)
>>   {
>> @@ -430,6 +515,7 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
>>   	struct pci_map *maps;
>>   
>>   	dev->intr_handle.fd = -1;
>> +	dev->req_notifier_handler.fd = -1;
>>   
>>   	/* store PCI address string */
>>   	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
>> @@ -521,6 +607,11 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
>>   		goto err_vfio_res;
>>   	}
>>   
>> +	if (pci_vfio_enable_notifier(dev, vfio_dev_fd) != 0) {
>> +		RTE_LOG(ERR, EAL, "Error setting up notifier!\n");
>> +		return -1;
>
> I think we should do goto to make required cleanup.
>

you are definitely right.


>> +	}
>> +
>>   	TAILQ_INSERT_TAIL(vfio_res_list, vfio_res, next);
>>   
>>   	return 0;
>> @@ -546,6 +637,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
>>   	struct pci_map *maps;
>>   
>>   	dev->intr_handle.fd = -1;
>> +	dev->req_notifier_handler.fd = -1;
>>   
>>   	/* store PCI address string */
>>   	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
>> @@ -586,6 +678,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
>>   
>>   	/* we need save vfio_dev_fd, so it can be used during release */
>>   	dev->intr_handle.vfio_dev_fd = vfio_dev_fd;
>> +	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
>>   
>>   	return 0;
>>   err_vfio_dev_fd:
>> @@ -658,6 +751,8 @@ pci_vfio_unmap_resource_primary(struct rte_pci_device *dev)
>>   	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
>>   			loc->domain, loc->bus, loc->devid, loc->function);
>>   
>> +	pci_vfio_disable_notifier(dev);
>> +
>
> Is it OK to ignore disable failure here? Why? It would be good to see
> explanations in comments. Does it leak eventfd now?
>

It might be leak eventfd, but anyway it should not ignore the check here.


>>   	if (close(dev->intr_handle.fd) < 0) {
>>   		RTE_LOG(INFO, EAL, "Error when closing eventfd file descriptor for %s\n",
>>   			pci_addr);
>> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
>> index f313fe9..2a8e5e9 100644
>> --- a/drivers/bus/pci/pci_common.c
>> +++ b/drivers/bus/pci/pci_common.c
>> @@ -446,6 +446,16 @@ pci_hot_unplug_handler(struct rte_device *dev)
>>   		return -1;
>>   
>>   	switch (pdev->kdrv) {
>> +	case RTE_KDRV_VFIO:
>> +		/**
>
> Why is doxygen style comment is used here?
>

ok.


>> +		 * vfio kernel module guaranty the pci device would not be
>> +		 * deleted until the user space release the resource, so no
>> +		 * need to remap BARs resource here, just directly notify
>> +		 * the req event to the user space to handle it.
>> +		 */
>> +		rte_dev_event_callback_process(dev->name,
>> +					       RTE_DEV_EVENT_REMOVE);
>> +		break;
>>   	case RTE_KDRV_IGB_UIO:
>>   	case RTE_KDRV_UIO_GENERIC:
>>   	case RTE_KDRV_NIC_UIO:
>

Re: [PATCH v2 3/4] pci: add req handler field to generic pci device

[Jeff Guo]

On 10/1/2018 5:46 PM, Andrew Rybchenko wrote:
> On 9/30/18 5:16 PM, Jeff Guo wrote:
>> There are some extended interrupt types in vfio pci device except from the
>> existing interrupts, such as err and req notifier, they could be useful for
>> device error monitoring. And these corresponding interrupt handler is
>> different from the other interrupt handler that register in PMDs, so a new
>> interrupt handler should be added. This patch will add specific req handler
>> in generic pci device.
>>
>> Signed-off-by: Jeff Guo<jia.guo@intel.com>
>> ---
>> v2->v1:
>> no change
>> ---
>>   drivers/bus/pci/rte_bus_pci.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
>> index 0d1955f..c45a820 100644
>> --- a/drivers/bus/pci/rte_bus_pci.h
>> +++ b/drivers/bus/pci/rte_bus_pci.h
>> @@ -66,6 +66,7 @@ struct rte_pci_device {
>>   	uint16_t max_vfs;                   /**< sriov enable if not zero */
>>   	enum rte_kernel_driver kdrv;        /**< Kernel driver passthrough */
>>   	char name[PCI_PRI_STR_SIZE+1];      /**< PCI location (ASCII) */
>> +	struct rte_intr_handle req_notifier_handler;/**< Req notifier handle */
>
> It is uninitialized now everywhere the structure is used. In the best case
> it is initialized with zeros (if the structure is memset or partly 
> initialized
> on declaration) and zero FD is a valid file descriptor. Is it OK?
>

The fd should be initialized as -1 as the structure of rte_intr_handler.


>>   };
>>   
>>   /**
>

Re: [PATCH v2 1/4] eal: add a new req notifier to eal interrupt

[Andrew Rybchenko]

On 10/2/18 7:30 AM, Jeff Guo wrote:
> hi, andrew
>
> thanks for your review again, see comment as below.
>
> On 10/1/2018 5:46 PM, Andrew Rybchenko wrote:
>> On 9/30/18 5:16 PM, Jeff Guo wrote:
>>> Add a new req notifier in eal interrupt for enable vfio hotplug.
>>>
>>> Signed-off-by: Jeff Guo<jia.guo@intel.com>
>>> ---
>>> v2->v1:
>>> no change
>>> ---
>>>   lib/librte_eal/common/include/rte_eal_interrupts.h |  1 +
>>>   lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 71 
>>> ++++++++++++++++++++++
>>>   2 files changed, 72 insertions(+)
>>>
>>> diff --git a/lib/librte_eal/common/include/rte_eal_interrupts.h 
>>> b/lib/librte_eal/common/include/rte_eal_interrupts.h
>>> index 6eb4932..2c47738 100644
>>> --- a/lib/librte_eal/common/include/rte_eal_interrupts.h
>>> +++ b/lib/librte_eal/common/include/rte_eal_interrupts.h
>>> @@ -35,6 +35,7 @@ enum rte_intr_handle_type {
>>>       RTE_INTR_HANDLE_EXT,          /**< external handler */
>>>       RTE_INTR_HANDLE_VDEV,         /**< virtual device */
>>>       RTE_INTR_HANDLE_DEV_EVENT,    /**< device event handle */
>>> +    RTE_INTR_HANDLE_VFIO_REQ,  /**< vfio device handle (req) */
>>
>> Alignment looks inconsistent. Is there any reason?
>>
>
> ok, it should be consistent.
>
>
>>>       RTE_INTR_HANDLE_MAX /**< count of elements */
>>>   };
>>>   diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c 
>>> b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
>>> index 4076c6d..7f611b3 100644
>>> --- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
>>> +++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
>>> @@ -308,6 +308,64 @@ vfio_disable_msix(const struct rte_intr_handle 
>>> *intr_handle) {
>>>         return ret;
>>>   }
>>> +
>>> +/* enable req notifier */
>>> +static int
>>> +vfio_enable_req(const struct rte_intr_handle *intr_handle)
>>> +{
>>> +    int len, ret;
>>> +    char irq_set_buf[IRQ_SET_BUF_LEN];
>>
>> I see that it is copied from similar functions in the file, but
>> I'd suggest to initialize it with zeros using '= {};' just to make
>> sure that uninitialized on-stack data never go to kernel.
>>
>>> +    struct vfio_irq_set *irq_set;
>>> +    int *fd_ptr;
>>> +
>>> +    len = sizeof(irq_set_buf);
>>> +
>>> +    irq_set = (struct vfio_irq_set *) irq_set_buf;
>>> +    irq_set->argsz = len;
>>> +    irq_set->count = 1;
>>> +    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>>> +             VFIO_IRQ_SET_ACTION_TRIGGER;
>>> +    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
>>
>> It looks like it is the only difference (plus error log below) from
>> vfio_enable_msi(). May be it make sense to restructure code
>> to avoid duplication. Obviously it is not critical, but please, 
>> consider.
>>
>> Similar comment is applicable to vfio_disable_req() below.
>>
>
> make sense, and i found that there are many duplication could be 
> modify here, maybe we could just add param index type to
>
> some common function to enable/disable each type of the 
> interrupts/req. I suggest to make other patch set which is aim to modify
>
> eal interrupt to do it. The same as the your above comment about char 
> array initialize.  Do you agree with that?

Yes, I agree.

>
>>> +    irq_set->start = 0;
>>> +    fd_ptr = (int *) &irq_set->data;
>>> +    *fd_ptr = intr_handle->fd;
>>> +
>>> +    ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, 
>>> irq_set);
>>> +
>>> +    if (ret) {
>>> +        RTE_LOG(ERR, EAL, "Error enabling req interrupts for fd %d\n",
>>> +                        intr_handle->fd);
>>> +        return -1;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/* disable req notifier */
>>> +static int
>>> +vfio_disable_req(const struct rte_intr_handle *intr_handle)
>>> +{
>>> +    struct vfio_irq_set *irq_set;
>>> +    char irq_set_buf[IRQ_SET_BUF_LEN];
>>> +    int len, ret;
>>> +
>>> +    len = sizeof(struct vfio_irq_set);
>>> +
>>> +    irq_set = (struct vfio_irq_set *) irq_set_buf;
>>> +    irq_set->argsz = len;
>>> +    irq_set->count = 0;
>>> +    irq_set->flags = VFIO_IRQ_SET_DATA_NONE | 
>>> VFIO_IRQ_SET_ACTION_TRIGGER;
>>> +    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
>>> +    irq_set->start = 0;
>>> +
>>> +    ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, 
>>> irq_set);
>>> +
>>> +    if (ret)
>>> +        RTE_LOG(ERR, EAL, "Error disabling req interrupts for fd 
>>> %d\n",
>>> +            intr_handle->fd);
>>> +
>>> +    return ret;
>>> +}
>>>   #endif
>>>     static int
>>> @@ -556,6 +614,10 @@ rte_intr_enable(const struct rte_intr_handle 
>>> *intr_handle)
>>>           if (vfio_enable_intx(intr_handle))
>>>               return -1;
>>>           break;
>>> +    case RTE_INTR_HANDLE_VFIO_REQ:
>>> +        if (vfio_enable_req(intr_handle))
>>> +            return -1;
>>> +        break;
>>>   #endif
>>>       /* not used at this moment */
>>>       case RTE_INTR_HANDLE_DEV_EVENT:
>>> @@ -606,6 +668,11 @@ rte_intr_disable(const struct rte_intr_handle 
>>> *intr_handle)
>>>           if (vfio_disable_intx(intr_handle))
>>>               return -1;
>>>           break;
>>> +    case RTE_INTR_HANDLE_VFIO_REQ:
>>> +        if (vfio_disable_req(intr_handle))
>>> +            return -1;
>>> +        break;
>>> +
>>>   #endif
>>>       /* not used at this moment */
>>>       case RTE_INTR_HANDLE_DEV_EVENT:
>>> @@ -682,6 +749,10 @@ eal_intr_process_interrupts(struct epoll_event 
>>> *events, int nfds)
>>>               bytes_read = 0;
>>>               call = true;
>>>               break;
>>> +        case RTE_INTR_HANDLE_VFIO_REQ:
>>> +            bytes_read = 0;
>>> +            call = true;
>>> +            break;
>>>           default:
>>>               bytes_read = 1;
>>>               break;
>>

Re: [PATCH v2 2/4] eal: modify device event callback process func

[Andrew Rybchenko]

On 10/2/18 7:45 AM, Jeff Guo wrote:
> andrew,
>
> On 10/1/2018 5:46 PM, Andrew Rybchenko wrote:
>> On 9/30/18 5:16 PM, Jeff Guo wrote:
>>> This patch modify the device event callback process function name to be
>>> more explicit, change the variable to be const and exposure the API out
>>> from private eal. The bus drivers and eal device would directly use 
>>> this
>>> API to process device event callback.
>>
>> Sorry, but from the description it is unclear what has changed to
>> make it necessary.
>>
>
> the reason is that not only eal device help will use the callback, but 
> also vfio bus will use
>
> the callback to handle hot-unplug. So exposure these API to let vfio 
> call it. I will add detail
>
> more at next version if need.

Yes, it would be good. Thanks.

>
>
>>> Signed-off-by: Jeff Guo<jia.guo@intel.com>
>>> ---
>>> v2->v1:
>>> change the rte_dev_event_callback_prcess from internal to external api
>>> for bus or app usage.
>>> ---
>>>   app/test-pmd/testpmd.c                  |  4 ++--
>>>   lib/librte_eal/bsdapp/eal/eal_dev.c     |  8 ++++++++
>>>   lib/librte_eal/common/eal_common_dev.c  |  5 +++--
>>>   lib/librte_eal/common/eal_private.h     | 12 ------------
>>>   lib/librte_eal/common/include/rte_dev.h | 18 +++++++++++++++++-
>>>   lib/librte_eal/linuxapp/eal/eal_dev.c   |  2 +-
>>>   lib/librte_eal/rte_eal_version.map      |  1 +
>>>   7 files changed, 32 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>> index bfef483..1313100 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -431,7 +431,7 @@ static void check_all_ports_link_status(uint32_t 
>>> port_mask);
>>>   static int eth_event_callback(portid_t port_id,
>>>                     enum rte_eth_event_type type,
>>>                     void *param, void *ret_param);
>>> -static void eth_dev_event_callback(char *device_name,
>>> +static void eth_dev_event_callback(const char *device_name,
>>>                   enum rte_dev_event_type type,
>>>                   void *param);
>>>   static int eth_dev_event_callback_register(void);
>>> @@ -2249,7 +2249,7 @@ eth_event_callback(portid_t port_id, enum 
>>> rte_eth_event_type type, void *param,
>>>     /* This function is used by the interrupt thread */
>>>   static void
>>> -eth_dev_event_callback(char *device_name, enum rte_dev_event_type 
>>> type,
>>> +eth_dev_event_callback(const char *device_name, enum 
>>> rte_dev_event_type type,
>>>                    __rte_unused void *arg)
>>>   {
>>>       uint16_t port_id;
>>> diff --git a/lib/librte_eal/bsdapp/eal/eal_dev.c 
>>> b/lib/librte_eal/bsdapp/eal/eal_dev.c
>>> index ae1c558..374e737 100644
>>> --- a/lib/librte_eal/bsdapp/eal/eal_dev.c
>>> +++ b/lib/librte_eal/bsdapp/eal/eal_dev.c
>>> @@ -33,3 +33,11 @@ rte_dev_hotplug_handle_disable(void)
>>>       RTE_LOG(ERR, EAL, "Device event is not supported for FreeBSD\n");
>>>       return -1;
>>>   }
>>> +
>>> +void  __rte_experimental
>>> +rte_dev_event_callback_process(const char *device_name,
>>> +                   enum rte_dev_event_type event)
>>> +{
>>> +    RTE_LOG(ERR, EAL, "Device event callback process is not 
>>> supported "
>>> +        "for FreeBSD\n");
>>
>> Consider to avoid split of log message to simplify search using grep.
>>
>
> i think i got your point, but the 80 limitation a  line and the 
> clearly meaning of the log should be the 1 prior to be considerate,
>
> right? It should be considerate but it is inevitable.

It is OK to have line longer than 80 symbols in this case. As far as I 
know checkpatches.sh accepts it and does not generate any errors/warnings.

>>> +}
>>> diff --git a/lib/librte_eal/common/eal_common_dev.c 
>>> b/lib/librte_eal/common/eal_common_dev.c
>>> index 678dbca..2d610a4 100644
>>> --- a/lib/librte_eal/common/eal_common_dev.c
>>> +++ b/lib/librte_eal/common/eal_common_dev.c
>>> @@ -342,8 +342,9 @@ rte_dev_event_callback_unregister(const char 
>>> *device_name,
>>>       return ret;
>>>   }
>>>   -void
>>> -dev_callback_process(char *device_name, enum rte_dev_event_type event)
>>> +void __rte_experimental
>>> +rte_dev_event_callback_process(const char *device_name,
>>> +                   enum rte_dev_event_type event)
>>>   {
>>>       struct dev_event_callback *cb_lst;
>>>   diff --git a/lib/librte_eal/common/eal_private.h 
>>> b/lib/librte_eal/common/eal_private.h
>>> index 637f20d..47e8a33 100644
>>> --- a/lib/librte_eal/common/eal_private.h
>>> +++ b/lib/librte_eal/common/eal_private.h
>>> @@ -259,18 +259,6 @@ struct rte_bus 
>>> *rte_bus_find_by_device_name(const char *str);
>>>   int rte_mp_channel_init(void);
>>>     /**
>>> - * Internal Executes all the user application registered callbacks for
>>> - * the specific device. It is for DPDK internal user only. User
>>> - * application should not call it directly.
>>> - *
>>> - * @param device_name
>>> - *  The device name.
>>> - * @param event
>>> - *  the device event type.
>>> - */
>>> -void dev_callback_process(char *device_name, enum 
>>> rte_dev_event_type event);
>>> -
>>> -/**
>>>    * @internal
>>>    * Parse a device string and store its information in an
>>>    * rte_devargs structure.
>>> diff --git a/lib/librte_eal/common/include/rte_dev.h 
>>> b/lib/librte_eal/common/include/rte_dev.h
>>> index ff580a0..58fab43 100644
>>> --- a/lib/librte_eal/common/include/rte_dev.h
>>> +++ b/lib/librte_eal/common/include/rte_dev.h
>>> @@ -39,7 +39,7 @@ struct rte_dev_event {
>>>       char *devname;            /**< device name */
>>>   };
>>>   -typedef void (*rte_dev_event_cb_fn)(char *device_name,
>>> +typedef void (*rte_dev_event_cb_fn)(const char *device_name,
>>>                       enum rte_dev_event_type event,
>>>                       void *cb_arg);
>>>   @@ -438,6 +438,22 @@ rte_dev_event_callback_unregister(const char 
>>> *device_name,
>>>    * @warning
>>>    * @b EXPERIMENTAL: this API may change without prior notice
>>>    *
>>> + * Executes all the user application registered callbacks for
>>> + * the specific device.
>>> + *
>>> + * @param device_name
>>> + *  The device name.
>>> + * @param event
>>> + *  the device event type.
>>> + */
>>> +void  __rte_experimental
>>> +rte_dev_event_callback_process(const char *device_name,
>>> +                   enum rte_dev_event_type event);
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>> + *
>>>    * Start the device event monitoring.
>>>    *
>>>    * @return
>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c 
>>> b/lib/librte_eal/linuxapp/eal/eal_dev.c
>>> index 9f9e1cf..01e3a04 100644
>>> --- a/lib/librte_eal/linuxapp/eal/eal_dev.c
>>> +++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
>>> @@ -271,7 +271,7 @@ dev_uev_handler(__rte_unused void *param)
>>>                   return;
>>>               }
>>>           }
>>> -        dev_callback_process(uevent.devname, uevent.type);
>>> +        rte_dev_event_callback_process(uevent.devname, uevent.type);
>>>       }
>>>   }
>>>   diff --git a/lib/librte_eal/rte_eal_version.map 
>>> b/lib/librte_eal/rte_eal_version.map
>>> index a3255aa..b96da50 100644
>>> --- a/lib/librte_eal/rte_eal_version.map
>>> +++ b/lib/librte_eal/rte_eal_version.map
>>> @@ -277,6 +277,7 @@ EXPERIMENTAL {
>>>       rte_class_register;
>>>       rte_class_unregister;
>>>       rte_ctrl_thread_create;
>>> +    rte_dev_event_callback_process;
>>>       rte_dev_event_callback_register;
>>>       rte_dev_event_callback_unregister;
>>>       rte_dev_event_monitor_start;
>>

[PATCH v2 0/4] Enable hotplug in vfio

[Jeff Guo]

As we may know that the process of hotplug is different between igb_uio
and vfio. For igb_uio, it could use uevent notification and memory
failure handle mechanism for hot-unplug. But for vfio, when device is be
hot-unplugged, the uevent can not be detected immediately, because of the
vfio kernel module will use a special mechanism to guaranty the pci
device would not be deleted until the user space release the resources,
so it will use another event “req notifier” at first to notify user space
to release resources for hotplug.

This patch will add a new interrupt type of req notifier in eal interrupt,
and add the new interrupt handler in pci device to handle the req device
event. When the req notifier be detected, it can trigger the device event
callback process to process for hot-unplug. With this mechanism, hotplug
could be enable in vfio.

patchset history:
v3->v2:
change some commit log and coding style and typo.

v2->v1:
change the rte_dev_event_callback_prcess from internal to external api
for bus or app usage.
change some code logic.

Jeff Guo (4):
  eal: add a new req notifier to eal interrupt
  eal: modify device event callback process func
  pci: add req handler field to generic pci device
  vfio: enable vfio hotplug by req notifier handler

 app/test-pmd/testpmd.c                             |   4 +-
 drivers/bus/pci/linux/pci_vfio.c                   | 111 +++++++++++++++++++++
 drivers/bus/pci/pci_common.c                       |  10 ++
 drivers/bus/pci/rte_bus_pci.h                      |   1 +
 lib/librte_eal/bsdapp/eal/eal_dev.c                |   8 ++
 lib/librte_eal/common/eal_common_dev.c             |   5 +-
 lib/librte_eal/common/eal_private.h                |  12 ---
 lib/librte_eal/common/include/rte_dev.h            |  18 +++-
 lib/librte_eal/common/include/rte_eal_interrupts.h |   1 +
 lib/librte_eal/linuxapp/eal/eal_dev.c              |   2 +-
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       |  71 +++++++++++++
 lib/librte_eal/rte_eal_version.map                 |   1 +
 12 files changed, 226 insertions(+), 18 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] eal: add a new req notifier to eal interrupt

[Jeff Guo]

Add a new req notifier in eal interrupt for enable vfio hotplug.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v3->v2:
change some code sytle to make consistent.
---
 lib/librte_eal/common/include/rte_eal_interrupts.h |  1 +
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 71 ++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_eal_interrupts.h b/lib/librte_eal/common/include/rte_eal_interrupts.h
index 6eb4932..5204ed4 100644
--- a/lib/librte_eal/common/include/rte_eal_interrupts.h
+++ b/lib/librte_eal/common/include/rte_eal_interrupts.h
@@ -35,6 +35,7 @@ enum rte_intr_handle_type {
 	RTE_INTR_HANDLE_EXT,          /**< external handler */
 	RTE_INTR_HANDLE_VDEV,         /**< virtual device */
 	RTE_INTR_HANDLE_DEV_EVENT,    /**< device event handle */
+	RTE_INTR_HANDLE_VFIO_REQ,     /**< vfio device handle (req) */
 	RTE_INTR_HANDLE_MAX           /**< count of elements */
 };
 
diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index 4076c6d..7f611b3 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -308,6 +308,64 @@ vfio_disable_msix(const struct rte_intr_handle *intr_handle) {
 
 	return ret;
 }
+
+/* enable req notifier */
+static int
+vfio_enable_req(const struct rte_intr_handle *intr_handle)
+{
+	int len, ret;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	struct vfio_irq_set *irq_set;
+	int *fd_ptr;
+
+	len = sizeof(irq_set_buf);
+
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 1;
+	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
+			 VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
+	irq_set->start = 0;
+	fd_ptr = (int *) &irq_set->data;
+	*fd_ptr = intr_handle->fd;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Error enabling req interrupts for fd %d\n",
+						intr_handle->fd);
+		return -1;
+	}
+
+	return 0;
+}
+
+/* disable req notifier */
+static int
+vfio_disable_req(const struct rte_intr_handle *intr_handle)
+{
+	struct vfio_irq_set *irq_set;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	int len, ret;
+
+	len = sizeof(struct vfio_irq_set);
+
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 0;
+	irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
+	irq_set->start = 0;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret)
+		RTE_LOG(ERR, EAL, "Error disabling req interrupts for fd %d\n",
+			intr_handle->fd);
+
+	return ret;
+}
 #endif
 
 static int
@@ -556,6 +614,10 @@ rte_intr_enable(const struct rte_intr_handle *intr_handle)
 		if (vfio_enable_intx(intr_handle))
 			return -1;
 		break;
+	case RTE_INTR_HANDLE_VFIO_REQ:
+		if (vfio_enable_req(intr_handle))
+			return -1;
+		break;
 #endif
 	/* not used at this moment */
 	case RTE_INTR_HANDLE_DEV_EVENT:
@@ -606,6 +668,11 @@ rte_intr_disable(const struct rte_intr_handle *intr_handle)
 		if (vfio_disable_intx(intr_handle))
 			return -1;
 		break;
+	case RTE_INTR_HANDLE_VFIO_REQ:
+		if (vfio_disable_req(intr_handle))
+			return -1;
+		break;
+
 #endif
 	/* not used at this moment */
 	case RTE_INTR_HANDLE_DEV_EVENT:
@@ -682,6 +749,10 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
 			bytes_read = 0;
 			call = true;
 			break;
+		case RTE_INTR_HANDLE_VFIO_REQ:
+			bytes_read = 0;
+			call = true;
+			break;
 		default:
 			bytes_read = 1;
 			break;
-- 
2.7.4

[PATCH v2 2/4] eal: modify device event callback process func

[Jeff Guo]

This patch modify the device event callback process function name to be
more explicit, change the variable to be const. And more, because not only
eal device helper will use the callback, but also vfio bus will use the
callback to handle hot-unplug, so exposure the API out from private eal.
The bus drivers and eal device would directly use this API to process
device event callback.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
modify commit log to be more clear
---
 app/test-pmd/testpmd.c                  |  4 ++--
 lib/librte_eal/bsdapp/eal/eal_dev.c     |  8 ++++++++
 lib/librte_eal/common/eal_common_dev.c  |  5 +++--
 lib/librte_eal/common/eal_private.h     | 12 ------------
 lib/librte_eal/common/include/rte_dev.h | 18 +++++++++++++++++-
 lib/librte_eal/linuxapp/eal/eal_dev.c   |  2 +-
 lib/librte_eal/rte_eal_version.map      |  1 +
 7 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index bfef483..1313100 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -431,7 +431,7 @@ static void check_all_ports_link_status(uint32_t port_mask);
 static int eth_event_callback(portid_t port_id,
 			      enum rte_eth_event_type type,
 			      void *param, void *ret_param);
-static void eth_dev_event_callback(char *device_name,
+static void eth_dev_event_callback(const char *device_name,
 				enum rte_dev_event_type type,
 				void *param);
 static int eth_dev_event_callback_register(void);
@@ -2249,7 +2249,7 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 
 /* This function is used by the interrupt thread */
 static void
-eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
+eth_dev_event_callback(const char *device_name, enum rte_dev_event_type type,
 			     __rte_unused void *arg)
 {
 	uint16_t port_id;
diff --git a/lib/librte_eal/bsdapp/eal/eal_dev.c b/lib/librte_eal/bsdapp/eal/eal_dev.c
index 255d611..3a3a2a5 100644
--- a/lib/librte_eal/bsdapp/eal/eal_dev.c
+++ b/lib/librte_eal/bsdapp/eal/eal_dev.c
@@ -33,3 +33,11 @@ rte_dev_hotplug_handle_disable(void)
 	RTE_LOG(ERR, EAL, "Device event is not supported for FreeBSD\n");
 	return -1;
 }
+
+void  __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event)
+{
+	RTE_LOG(ERR, EAL,
+		"Device event callback process is not supported for FreeBSD.\n");
+}
diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 678dbca..2d610a4 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -342,8 +342,9 @@ rte_dev_event_callback_unregister(const char *device_name,
 	return ret;
 }
 
-void
-dev_callback_process(char *device_name, enum rte_dev_event_type event)
+void __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event)
 {
 	struct dev_event_callback *cb_lst;
 
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 637f20d..47e8a33 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -259,18 +259,6 @@ struct rte_bus *rte_bus_find_by_device_name(const char *str);
 int rte_mp_channel_init(void);
 
 /**
- * Internal Executes all the user application registered callbacks for
- * the specific device. It is for DPDK internal user only. User
- * application should not call it directly.
- *
- * @param device_name
- *  The device name.
- * @param event
- *  the device event type.
- */
-void dev_callback_process(char *device_name, enum rte_dev_event_type event);
-
-/**
  * @internal
  * Parse a device string and store its information in an
  * rte_devargs structure.
diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index ff580a0..58fab43 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -39,7 +39,7 @@ struct rte_dev_event {
 	char *devname;			/**< device name */
 };
 
-typedef void (*rte_dev_event_cb_fn)(char *device_name,
+typedef void (*rte_dev_event_cb_fn)(const char *device_name,
 					enum rte_dev_event_type event,
 					void *cb_arg);
 
@@ -438,6 +438,22 @@ rte_dev_event_callback_unregister(const char *device_name,
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
  *
+ * Executes all the user application registered callbacks for
+ * the specific device.
+ *
+ * @param device_name
+ *  The device name.
+ * @param event
+ *  the device event type.
+ */
+void  __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
  * Start the device event monitoring.
  *
  * @return
diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c b/lib/librte_eal/linuxapp/eal/eal_dev.c
index 14b18d8..7f44251 100644
--- a/lib/librte_eal/linuxapp/eal/eal_dev.c
+++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
@@ -271,7 +271,7 @@ dev_uev_handler(__rte_unused void *param)
 				return;
 			}
 		}
-		dev_callback_process(uevent.devname, uevent.type);
+		rte_dev_event_callback_process(uevent.devname, uevent.type);
 	}
 }
 
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index a3255aa..b96da50 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -277,6 +277,7 @@ EXPERIMENTAL {
 	rte_class_register;
 	rte_class_unregister;
 	rte_ctrl_thread_create;
+	rte_dev_event_callback_process;
 	rte_dev_event_callback_register;
 	rte_dev_event_callback_unregister;
 	rte_dev_event_monitor_start;
-- 
2.7.4

[PATCH v2 3/4] pci: add req handler field to generic pci device

[Jeff Guo]

There are some extended interrupt types in vfio pci device except from the
existing interrupts, such as err and req notifier, they could be useful for
device error monitoring. And these corresponding interrupt handler is
different from the other interrupt handler that register in PMDs, so a new
interrupt handler should be added. This patch will add specific req handler
in generic pci device.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v3->v2:
no change.
---
 drivers/bus/pci/rte_bus_pci.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
index 0d1955f..c45a820 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -66,6 +66,7 @@ struct rte_pci_device {
 	uint16_t max_vfs;                   /**< sriov enable if not zero */
 	enum rte_kernel_driver kdrv;        /**< Kernel driver passthrough */
 	char name[PCI_PRI_STR_SIZE+1];      /**< PCI location (ASCII) */
+	struct rte_intr_handle req_notifier_handler;/**< Req notifier handle */
 };
 
 /**
-- 
2.7.4

[PATCH v2 4/4] vfio: enable vfio hotplug by req notifier handler

[Jeff Guo]

When device is be hot-unplugged, the vfio kernel module will sent req
notifier to request user space to release the allocated resources at
first. After that, vfio kernel module will detect the device disappear,
and then delete the device in kernel.

This patch aim to add req notifier processing to enable hotplug for vfio.
By enable the req notifier monitoring and register the notifier callback,
when device be hot-unplugged, the hot-unplug handler will be called to
process hotplug for vfio.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v3->v2:
change some code style and typo
---
 drivers/bus/pci/linux/pci_vfio.c | 111 +++++++++++++++++++++++++++++++++++++++
 drivers/bus/pci/pci_common.c     |  10 ++++
 2 files changed, 121 insertions(+)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 686386d..5d3d026 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -17,6 +17,8 @@
 #include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
 #include <rte_vfio.h>
+#include <rte_eal.h>
+#include <rte_bus.h>
 
 #include "eal_filesystem.h"
 
@@ -277,6 +279,101 @@ pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd)
 	return -1;
 }
 
+static void
+pci_vfio_req_handler(void *param)
+{
+	struct rte_bus *bus;
+	int ret;
+	struct rte_device *device = (struct rte_device *)param;
+
+	bus = rte_bus_find_by_device(device);
+	if (bus == NULL) {
+		RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n",
+			device->name);
+		return;
+	}
+
+	/*
+	 * vfio kernel module request user space to release allocated
+	 * resources before device be deleted in kernel, so it can directly
+	 * call the vfio bus hot-unplug handler to process it.
+	 */
+	ret = bus->hot_unplug_handler(device);
+	if (ret)
+		RTE_LOG(ERR, EAL,
+			"Can not handle hot-unplug for device (%s)\n",
+			device->name);
+}
+
+/* enable notifier (only enable req now) */
+static int
+pci_vfio_enable_notifier(struct rte_pci_device *dev, int vfio_dev_fd)
+{
+	int ret;
+	int fd = -1;
+
+	/* set up an eventfd for req notifier */
+	fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
+	if (fd < 0) {
+		RTE_LOG(ERR, EAL, "Cannot set up eventfd, error %i (%s)\n",
+			errno, strerror(errno));
+		return -1;
+	}
+
+	dev->req_notifier_handler.fd = fd;
+	dev->req_notifier_handler.type = RTE_INTR_HANDLE_VFIO_REQ;
+	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
+	ret = rte_intr_callback_register(&dev->req_notifier_handler,
+					 pci_vfio_req_handler,
+					 (void *)&dev->device);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Fail to register req notifier handler.\n");
+		goto error;
+	}
+
+	ret = rte_intr_enable(&dev->req_notifier_handler);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Fail to enable req notifier.\n");
+		ret = rte_intr_callback_unregister(&dev->req_notifier_handler,
+						 pci_vfio_req_handler,
+						 (void *)&dev->device);
+		if (ret)
+			RTE_LOG(ERR, EAL,
+				"Fail to unregister req notifier handler.\n");
+		goto error;
+	}
+
+	return 0;
+error:
+	close(fd);
+	return -1;
+}
+
+/* disable notifier (only disable req now) */
+static int
+pci_vfio_disable_notifier(struct rte_pci_device *dev)
+{
+	int ret;
+
+	ret = rte_intr_disable(&dev->req_notifier_handler);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "fail to disable req notifier.\n");
+		return -1;
+	}
+
+	ret = rte_intr_callback_unregister(&dev->req_notifier_handler,
+					   pci_vfio_req_handler,
+					   (void *)&dev->device);
+	if (ret) {
+		RTE_LOG(ERR, EAL,
+			 "fail to unregister req notifier handler.\n");
+		return -1;
+	}
+
+	close(dev->req_notifier_handler.fd);
+	return 0;
+}
+
 static int
 pci_vfio_is_ioport_bar(int vfio_dev_fd, int bar_index)
 {
@@ -430,6 +527,7 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
+	dev->req_notifier_handler.fd = -1;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
@@ -521,6 +619,11 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
 		goto err_vfio_res;
 	}
 
+	if (pci_vfio_enable_notifier(dev, vfio_dev_fd) != 0) {
+		RTE_LOG(ERR, EAL, "Error setting up notifier!\n");
+		goto err_vfio_res;
+	}
+
 	TAILQ_INSERT_TAIL(vfio_res_list, vfio_res, next);
 
 	return 0;
@@ -546,6 +649,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
+	dev->req_notifier_handler.fd = -1;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
@@ -586,6 +690,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 
 	/* we need save vfio_dev_fd, so it can be used during release */
 	dev->intr_handle.vfio_dev_fd = vfio_dev_fd;
+	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
 
 	return 0;
 err_vfio_dev_fd:
@@ -658,6 +763,12 @@ pci_vfio_unmap_resource_primary(struct rte_pci_device *dev)
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
 			loc->domain, loc->bus, loc->devid, loc->function);
 
+	ret = pci_vfio_disable_notifier(dev);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "fail to disable req notifier.\n");
+		return -1;
+	}
+
 	if (close(dev->intr_handle.fd) < 0) {
 		RTE_LOG(INFO, EAL, "Error when closing eventfd file descriptor for %s\n",
 			pci_addr);
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index f313fe9..51e69c1 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -446,6 +446,16 @@ pci_hot_unplug_handler(struct rte_device *dev)
 		return -1;
 
 	switch (pdev->kdrv) {
+	case RTE_KDRV_VFIO:
+		/*
+		 * vfio kernel module guaranty the pci device would not be
+		 * deleted until the user space release the resource, so no
+		 * need to remap BARs resource here, just directly notify
+		 * the req event to the user space to handle it.
+		 */
+		rte_dev_event_callback_process(dev->name,
+					       RTE_DEV_EVENT_REMOVE);
+		break;
 	case RTE_KDRV_IGB_UIO:
 	case RTE_KDRV_UIO_GENERIC:
 	case RTE_KDRV_NIC_UIO:
-- 
2.7.4

[PATCH v3 0/4] Enable hotplug in vfio

[Jeff Guo]

As we may know that the process of hotplug is different between igb_uio
and vfio. For igb_uio, it could use uevent notification and memory
failure handle mechanism for hot-unplug. But for vfio, when device is be
hot-unplugged, the uevent can not be detected immediately, because of the
vfio kernel module will use a special mechanism to guaranty the pci
device would not be deleted until the user space release the resources,
so it will use another event “req notifier” at first to notify user space
to release resources for hotplug.

This patch will add a new interrupt type of req notifier in eal interrupt,
and add the new interrupt handler in pci device to handle the req device
event. When the req notifier be detected, it can trigger the device event
callback process to process for hot-unplug. With this mechanism, hotplug
could be enable in vfio.

patchset history:
v3->v2:
change some commit log and coding style and typo.

v2->v1:
change the rte_dev_event_callback_prcess from internal to external api
for bus or app usage.
change some code logic.

Jeff Guo (4):
  eal: add a new req notifier to eal interrupt
  eal: modify device event callback process func
  pci: add req handler field to generic pci device
  vfio: enable vfio hotplug by req notifier handler

 app/test-pmd/testpmd.c                             |   4 +-
 drivers/bus/pci/linux/pci_vfio.c                   | 111 +++++++++++++++++++++
 drivers/bus/pci/pci_common.c                       |  10 ++
 drivers/bus/pci/rte_bus_pci.h                      |   1 +
 lib/librte_eal/bsdapp/eal/eal_dev.c                |   8 ++
 lib/librte_eal/common/eal_common_dev.c             |   5 +-
 lib/librte_eal/common/eal_private.h                |  12 ---
 lib/librte_eal/common/include/rte_dev.h            |  18 +++-
 lib/librte_eal/common/include/rte_eal_interrupts.h |   1 +
 lib/librte_eal/linuxapp/eal/eal_dev.c              |   2 +-
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       |  71 +++++++++++++
 lib/librte_eal/rte_eal_version.map                 |   1 +
 12 files changed, 226 insertions(+), 18 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] eal: add a new req notifier to eal interrupt

[Jeff Guo]

Add a new req notifier in eal interrupt for enable vfio hotplug.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v3->v2:
change some code sytle to make consistent.
---
 lib/librte_eal/common/include/rte_eal_interrupts.h |  1 +
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 71 ++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_eal_interrupts.h b/lib/librte_eal/common/include/rte_eal_interrupts.h
index 6eb4932..5204ed4 100644
--- a/lib/librte_eal/common/include/rte_eal_interrupts.h
+++ b/lib/librte_eal/common/include/rte_eal_interrupts.h
@@ -35,6 +35,7 @@ enum rte_intr_handle_type {
 	RTE_INTR_HANDLE_EXT,          /**< external handler */
 	RTE_INTR_HANDLE_VDEV,         /**< virtual device */
 	RTE_INTR_HANDLE_DEV_EVENT,    /**< device event handle */
+	RTE_INTR_HANDLE_VFIO_REQ,     /**< vfio device handle (req) */
 	RTE_INTR_HANDLE_MAX           /**< count of elements */
 };
 
diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index 4076c6d..7f611b3 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -308,6 +308,64 @@ vfio_disable_msix(const struct rte_intr_handle *intr_handle) {
 
 	return ret;
 }
+
+/* enable req notifier */
+static int
+vfio_enable_req(const struct rte_intr_handle *intr_handle)
+{
+	int len, ret;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	struct vfio_irq_set *irq_set;
+	int *fd_ptr;
+
+	len = sizeof(irq_set_buf);
+
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 1;
+	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
+			 VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
+	irq_set->start = 0;
+	fd_ptr = (int *) &irq_set->data;
+	*fd_ptr = intr_handle->fd;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Error enabling req interrupts for fd %d\n",
+						intr_handle->fd);
+		return -1;
+	}
+
+	return 0;
+}
+
+/* disable req notifier */
+static int
+vfio_disable_req(const struct rte_intr_handle *intr_handle)
+{
+	struct vfio_irq_set *irq_set;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	int len, ret;
+
+	len = sizeof(struct vfio_irq_set);
+
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 0;
+	irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
+	irq_set->start = 0;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret)
+		RTE_LOG(ERR, EAL, "Error disabling req interrupts for fd %d\n",
+			intr_handle->fd);
+
+	return ret;
+}
 #endif
 
 static int
@@ -556,6 +614,10 @@ rte_intr_enable(const struct rte_intr_handle *intr_handle)
 		if (vfio_enable_intx(intr_handle))
 			return -1;
 		break;
+	case RTE_INTR_HANDLE_VFIO_REQ:
+		if (vfio_enable_req(intr_handle))
+			return -1;
+		break;
 #endif
 	/* not used at this moment */
 	case RTE_INTR_HANDLE_DEV_EVENT:
@@ -606,6 +668,11 @@ rte_intr_disable(const struct rte_intr_handle *intr_handle)
 		if (vfio_disable_intx(intr_handle))
 			return -1;
 		break;
+	case RTE_INTR_HANDLE_VFIO_REQ:
+		if (vfio_disable_req(intr_handle))
+			return -1;
+		break;
+
 #endif
 	/* not used at this moment */
 	case RTE_INTR_HANDLE_DEV_EVENT:
@@ -682,6 +749,10 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
 			bytes_read = 0;
 			call = true;
 			break;
+		case RTE_INTR_HANDLE_VFIO_REQ:
+			bytes_read = 0;
+			call = true;
+			break;
 		default:
 			bytes_read = 1;
 			break;
-- 
2.7.4

[PATCH v3 2/4] eal: modify device event callback process func

[Jeff Guo]

This patch modify the device event callback process function name to be
more explicit, change the variable to be const. And more, because not only
eal device helper will use the callback, but also vfio bus will use the
callback to handle hot-unplug, so exposure the API out from private eal.
The bus drivers and eal device would directly use this API to process
device event callback.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
modify commit log to be more clear
---
 app/test-pmd/testpmd.c                  |  4 ++--
 lib/librte_eal/bsdapp/eal/eal_dev.c     |  8 ++++++++
 lib/librte_eal/common/eal_common_dev.c  |  5 +++--
 lib/librte_eal/common/eal_private.h     | 12 ------------
 lib/librte_eal/common/include/rte_dev.h | 18 +++++++++++++++++-
 lib/librte_eal/linuxapp/eal/eal_dev.c   |  2 +-
 lib/librte_eal/rte_eal_version.map      |  1 +
 7 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index bfef483..1313100 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -431,7 +431,7 @@ static void check_all_ports_link_status(uint32_t port_mask);
 static int eth_event_callback(portid_t port_id,
 			      enum rte_eth_event_type type,
 			      void *param, void *ret_param);
-static void eth_dev_event_callback(char *device_name,
+static void eth_dev_event_callback(const char *device_name,
 				enum rte_dev_event_type type,
 				void *param);
 static int eth_dev_event_callback_register(void);
@@ -2249,7 +2249,7 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 
 /* This function is used by the interrupt thread */
 static void
-eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
+eth_dev_event_callback(const char *device_name, enum rte_dev_event_type type,
 			     __rte_unused void *arg)
 {
 	uint16_t port_id;
diff --git a/lib/librte_eal/bsdapp/eal/eal_dev.c b/lib/librte_eal/bsdapp/eal/eal_dev.c
index 255d611..3a3a2a5 100644
--- a/lib/librte_eal/bsdapp/eal/eal_dev.c
+++ b/lib/librte_eal/bsdapp/eal/eal_dev.c
@@ -33,3 +33,11 @@ rte_dev_hotplug_handle_disable(void)
 	RTE_LOG(ERR, EAL, "Device event is not supported for FreeBSD\n");
 	return -1;
 }
+
+void  __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event)
+{
+	RTE_LOG(ERR, EAL,
+		"Device event callback process is not supported for FreeBSD.\n");
+}
diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 678dbca..2d610a4 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -342,8 +342,9 @@ rte_dev_event_callback_unregister(const char *device_name,
 	return ret;
 }
 
-void
-dev_callback_process(char *device_name, enum rte_dev_event_type event)
+void __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event)
 {
 	struct dev_event_callback *cb_lst;
 
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 637f20d..47e8a33 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -259,18 +259,6 @@ struct rte_bus *rte_bus_find_by_device_name(const char *str);
 int rte_mp_channel_init(void);
 
 /**
- * Internal Executes all the user application registered callbacks for
- * the specific device. It is for DPDK internal user only. User
- * application should not call it directly.
- *
- * @param device_name
- *  The device name.
- * @param event
- *  the device event type.
- */
-void dev_callback_process(char *device_name, enum rte_dev_event_type event);
-
-/**
  * @internal
  * Parse a device string and store its information in an
  * rte_devargs structure.
diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index ff580a0..58fab43 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -39,7 +39,7 @@ struct rte_dev_event {
 	char *devname;			/**< device name */
 };
 
-typedef void (*rte_dev_event_cb_fn)(char *device_name,
+typedef void (*rte_dev_event_cb_fn)(const char *device_name,
 					enum rte_dev_event_type event,
 					void *cb_arg);
 
@@ -438,6 +438,22 @@ rte_dev_event_callback_unregister(const char *device_name,
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
  *
+ * Executes all the user application registered callbacks for
+ * the specific device.
+ *
+ * @param device_name
+ *  The device name.
+ * @param event
+ *  the device event type.
+ */
+void  __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
  * Start the device event monitoring.
  *
  * @return
diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c b/lib/librte_eal/linuxapp/eal/eal_dev.c
index 14b18d8..7f44251 100644
--- a/lib/librte_eal/linuxapp/eal/eal_dev.c
+++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
@@ -271,7 +271,7 @@ dev_uev_handler(__rte_unused void *param)
 				return;
 			}
 		}
-		dev_callback_process(uevent.devname, uevent.type);
+		rte_dev_event_callback_process(uevent.devname, uevent.type);
 	}
 }
 
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index a3255aa..b96da50 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -277,6 +277,7 @@ EXPERIMENTAL {
 	rte_class_register;
 	rte_class_unregister;
 	rte_ctrl_thread_create;
+	rte_dev_event_callback_process;
 	rte_dev_event_callback_register;
 	rte_dev_event_callback_unregister;
 	rte_dev_event_monitor_start;
-- 
2.7.4

[PATCH v3 3/4] pci: add req handler field to generic pci device

[Jeff Guo]

There are some extended interrupt types in vfio pci device except from the
existing interrupts, such as err and req notifier, they could be useful for
device error monitoring. And these corresponding interrupt handler is
different from the other interrupt handler that register in PMDs, so a new
interrupt handler should be added. This patch will add specific req handler
in generic pci device.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v3->v2:
no change.
---
 drivers/bus/pci/rte_bus_pci.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
index 0d1955f..c45a820 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -66,6 +66,7 @@ struct rte_pci_device {
 	uint16_t max_vfs;                   /**< sriov enable if not zero */
 	enum rte_kernel_driver kdrv;        /**< Kernel driver passthrough */
 	char name[PCI_PRI_STR_SIZE+1];      /**< PCI location (ASCII) */
+	struct rte_intr_handle req_notifier_handler;/**< Req notifier handle */
 };
 
 /**
-- 
2.7.4

[PATCH v3 4/4] vfio: enable vfio hotplug by req notifier handler

[Jeff Guo]

When device is be hot-unplugged, the vfio kernel module will sent req
notifier to request user space to release the allocated resources at
first. After that, vfio kernel module will detect the device disappear,
and then delete the device in kernel.

This patch aim to add req notifier processing to enable hotplug for vfio.
By enable the req notifier monitoring and register the notifier callback,
when device be hot-unplugged, the hot-unplug handler will be called to
process hotplug for vfio.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v3->v2:
change some code style and typo
---
 drivers/bus/pci/linux/pci_vfio.c | 111 +++++++++++++++++++++++++++++++++++++++
 drivers/bus/pci/pci_common.c     |  10 ++++
 2 files changed, 121 insertions(+)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 686386d..5d3d026 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -17,6 +17,8 @@
 #include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
 #include <rte_vfio.h>
+#include <rte_eal.h>
+#include <rte_bus.h>
 
 #include "eal_filesystem.h"
 
@@ -277,6 +279,101 @@ pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd)
 	return -1;
 }
 
+static void
+pci_vfio_req_handler(void *param)
+{
+	struct rte_bus *bus;
+	int ret;
+	struct rte_device *device = (struct rte_device *)param;
+
+	bus = rte_bus_find_by_device(device);
+	if (bus == NULL) {
+		RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n",
+			device->name);
+		return;
+	}
+
+	/*
+	 * vfio kernel module request user space to release allocated
+	 * resources before device be deleted in kernel, so it can directly
+	 * call the vfio bus hot-unplug handler to process it.
+	 */
+	ret = bus->hot_unplug_handler(device);
+	if (ret)
+		RTE_LOG(ERR, EAL,
+			"Can not handle hot-unplug for device (%s)\n",
+			device->name);
+}
+
+/* enable notifier (only enable req now) */
+static int
+pci_vfio_enable_notifier(struct rte_pci_device *dev, int vfio_dev_fd)
+{
+	int ret;
+	int fd = -1;
+
+	/* set up an eventfd for req notifier */
+	fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
+	if (fd < 0) {
+		RTE_LOG(ERR, EAL, "Cannot set up eventfd, error %i (%s)\n",
+			errno, strerror(errno));
+		return -1;
+	}
+
+	dev->req_notifier_handler.fd = fd;
+	dev->req_notifier_handler.type = RTE_INTR_HANDLE_VFIO_REQ;
+	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
+	ret = rte_intr_callback_register(&dev->req_notifier_handler,
+					 pci_vfio_req_handler,
+					 (void *)&dev->device);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Fail to register req notifier handler.\n");
+		goto error;
+	}
+
+	ret = rte_intr_enable(&dev->req_notifier_handler);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Fail to enable req notifier.\n");
+		ret = rte_intr_callback_unregister(&dev->req_notifier_handler,
+						 pci_vfio_req_handler,
+						 (void *)&dev->device);
+		if (ret)
+			RTE_LOG(ERR, EAL,
+				"Fail to unregister req notifier handler.\n");
+		goto error;
+	}
+
+	return 0;
+error:
+	close(fd);
+	return -1;
+}
+
+/* disable notifier (only disable req now) */
+static int
+pci_vfio_disable_notifier(struct rte_pci_device *dev)
+{
+	int ret;
+
+	ret = rte_intr_disable(&dev->req_notifier_handler);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "fail to disable req notifier.\n");
+		return -1;
+	}
+
+	ret = rte_intr_callback_unregister(&dev->req_notifier_handler,
+					   pci_vfio_req_handler,
+					   (void *)&dev->device);
+	if (ret) {
+		RTE_LOG(ERR, EAL,
+			 "fail to unregister req notifier handler.\n");
+		return -1;
+	}
+
+	close(dev->req_notifier_handler.fd);
+	return 0;
+}
+
 static int
 pci_vfio_is_ioport_bar(int vfio_dev_fd, int bar_index)
 {
@@ -430,6 +527,7 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
+	dev->req_notifier_handler.fd = -1;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
@@ -521,6 +619,11 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
 		goto err_vfio_res;
 	}
 
+	if (pci_vfio_enable_notifier(dev, vfio_dev_fd) != 0) {
+		RTE_LOG(ERR, EAL, "Error setting up notifier!\n");
+		goto err_vfio_res;
+	}
+
 	TAILQ_INSERT_TAIL(vfio_res_list, vfio_res, next);
 
 	return 0;
@@ -546,6 +649,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
+	dev->req_notifier_handler.fd = -1;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
@@ -586,6 +690,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 
 	/* we need save vfio_dev_fd, so it can be used during release */
 	dev->intr_handle.vfio_dev_fd = vfio_dev_fd;
+	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
 
 	return 0;
 err_vfio_dev_fd:
@@ -658,6 +763,12 @@ pci_vfio_unmap_resource_primary(struct rte_pci_device *dev)
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
 			loc->domain, loc->bus, loc->devid, loc->function);
 
+	ret = pci_vfio_disable_notifier(dev);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "fail to disable req notifier.\n");
+		return -1;
+	}
+
 	if (close(dev->intr_handle.fd) < 0) {
 		RTE_LOG(INFO, EAL, "Error when closing eventfd file descriptor for %s\n",
 			pci_addr);
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index f313fe9..51e69c1 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -446,6 +446,16 @@ pci_hot_unplug_handler(struct rte_device *dev)
 		return -1;
 
 	switch (pdev->kdrv) {
+	case RTE_KDRV_VFIO:
+		/*
+		 * vfio kernel module guaranty the pci device would not be
+		 * deleted until the user space release the resource, so no
+		 * need to remap BARs resource here, just directly notify
+		 * the req event to the user space to handle it.
+		 */
+		rte_dev_event_callback_process(dev->name,
+					       RTE_DEV_EVENT_REMOVE);
+		break;
 	case RTE_KDRV_IGB_UIO:
 	case RTE_KDRV_UIO_GENERIC:
 	case RTE_KDRV_NIC_UIO:
-- 
2.7.4

[PATCH v3 0/4] Enable hotplug in vfio

[Jeff Guo]

As we may know that the process of hotplug is different between igb_uio
and vfio. For igb_uio, it could use uevent notification and memory
failure handle mechanism for hot-unplug. But for vfio, when device is be
hot-unplugged, the uevent can not be detected immediately, because of the
vfio kernel module will use a special mechanism to guaranty the pci
device would not be deleted until the user space release the resources,
so it will use another event “req notifier” at first to notify user space
to release resources for hotplug.

This patch will add a new interrupt type of req notifier in eal interrupt,
and add the new interrupt handler in pci device to handle the req device
event. When the req notifier be detected, it can trigger the device event
callback process to process for hot-unplug. With this mechanism, hotplug
could be enable in vfio.

patchset history:
v3->v2:
change some commit log and coding style and typo.

v2->v1:
change the rte_dev_event_callback_prcess from internal to external api
for bus or app usage.
change some code logic.

Jeff Guo (4):
  eal: add a new req notifier to eal interrupt
  eal: modify device event callback process func
  pci: add req handler field to generic pci device
  vfio: enable vfio hotplug by req notifier handler

 app/test-pmd/testpmd.c                             |   4 +-
 drivers/bus/pci/linux/pci_vfio.c                   | 111 +++++++++++++++++++++
 drivers/bus/pci/pci_common.c                       |  10 ++
 drivers/bus/pci/rte_bus_pci.h                      |   1 +
 lib/librte_eal/bsdapp/eal/eal_dev.c                |   8 ++
 lib/librte_eal/common/eal_common_dev.c             |   5 +-
 lib/librte_eal/common/eal_private.h                |  12 ---
 lib/librte_eal/common/include/rte_dev.h            |  18 +++-
 lib/librte_eal/common/include/rte_eal_interrupts.h |   1 +
 lib/librte_eal/linuxapp/eal/eal_dev.c              |   2 +-
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       |  71 +++++++++++++
 lib/librte_eal/rte_eal_version.map                 |   1 +
 12 files changed, 226 insertions(+), 18 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] eal: add a new req notifier to eal interrupt

[Jeff Guo]

Add a new req notifier in eal interrupt for enable vfio hotplug.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v3->v2:
no change.
---
 lib/librte_eal/common/include/rte_eal_interrupts.h |  1 +
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 71 ++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_eal_interrupts.h b/lib/librte_eal/common/include/rte_eal_interrupts.h
index 6eb4932..5204ed4 100644
--- a/lib/librte_eal/common/include/rte_eal_interrupts.h
+++ b/lib/librte_eal/common/include/rte_eal_interrupts.h
@@ -35,6 +35,7 @@ enum rte_intr_handle_type {
 	RTE_INTR_HANDLE_EXT,          /**< external handler */
 	RTE_INTR_HANDLE_VDEV,         /**< virtual device */
 	RTE_INTR_HANDLE_DEV_EVENT,    /**< device event handle */
+	RTE_INTR_HANDLE_VFIO_REQ,     /**< vfio device handle (req) */
 	RTE_INTR_HANDLE_MAX           /**< count of elements */
 };
 
diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index 4076c6d..7f611b3 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -308,6 +308,64 @@ vfio_disable_msix(const struct rte_intr_handle *intr_handle) {
 
 	return ret;
 }
+
+/* enable req notifier */
+static int
+vfio_enable_req(const struct rte_intr_handle *intr_handle)
+{
+	int len, ret;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	struct vfio_irq_set *irq_set;
+	int *fd_ptr;
+
+	len = sizeof(irq_set_buf);
+
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 1;
+	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
+			 VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
+	irq_set->start = 0;
+	fd_ptr = (int *) &irq_set->data;
+	*fd_ptr = intr_handle->fd;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Error enabling req interrupts for fd %d\n",
+						intr_handle->fd);
+		return -1;
+	}
+
+	return 0;
+}
+
+/* disable req notifier */
+static int
+vfio_disable_req(const struct rte_intr_handle *intr_handle)
+{
+	struct vfio_irq_set *irq_set;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	int len, ret;
+
+	len = sizeof(struct vfio_irq_set);
+
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 0;
+	irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
+	irq_set->start = 0;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret)
+		RTE_LOG(ERR, EAL, "Error disabling req interrupts for fd %d\n",
+			intr_handle->fd);
+
+	return ret;
+}
 #endif
 
 static int
@@ -556,6 +614,10 @@ rte_intr_enable(const struct rte_intr_handle *intr_handle)
 		if (vfio_enable_intx(intr_handle))
 			return -1;
 		break;
+	case RTE_INTR_HANDLE_VFIO_REQ:
+		if (vfio_enable_req(intr_handle))
+			return -1;
+		break;
 #endif
 	/* not used at this moment */
 	case RTE_INTR_HANDLE_DEV_EVENT:
@@ -606,6 +668,11 @@ rte_intr_disable(const struct rte_intr_handle *intr_handle)
 		if (vfio_disable_intx(intr_handle))
 			return -1;
 		break;
+	case RTE_INTR_HANDLE_VFIO_REQ:
+		if (vfio_disable_req(intr_handle))
+			return -1;
+		break;
+
 #endif
 	/* not used at this moment */
 	case RTE_INTR_HANDLE_DEV_EVENT:
@@ -682,6 +749,10 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
 			bytes_read = 0;
 			call = true;
 			break;
+		case RTE_INTR_HANDLE_VFIO_REQ:
+			bytes_read = 0;
+			call = true;
+			break;
 		default:
 			bytes_read = 1;
 			break;
-- 
2.7.4

[PATCH v3 2/4] eal: modify device event callback process func

[Jeff Guo]

This patch modify the device event callback process function name to be
more explicit, change the variable to be const. And more, because not only
eal device helper will use the callback, but also vfio bus will use the
callback to handle hot-unplug, so exposure the API out from private eal.
The bus drivers and eal device would directly use this API to process
device event callback.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v3->v2:
modify commit log to be more clear.
---
 app/test-pmd/testpmd.c                  |  4 ++--
 lib/librte_eal/bsdapp/eal/eal_dev.c     |  8 ++++++++
 lib/librte_eal/common/eal_common_dev.c  |  5 +++--
 lib/librte_eal/common/eal_private.h     | 12 ------------
 lib/librte_eal/common/include/rte_dev.h | 18 +++++++++++++++++-
 lib/librte_eal/linuxapp/eal/eal_dev.c   |  2 +-
 lib/librte_eal/rte_eal_version.map      |  1 +
 7 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index bfef483..1313100 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -431,7 +431,7 @@ static void check_all_ports_link_status(uint32_t port_mask);
 static int eth_event_callback(portid_t port_id,
 			      enum rte_eth_event_type type,
 			      void *param, void *ret_param);
-static void eth_dev_event_callback(char *device_name,
+static void eth_dev_event_callback(const char *device_name,
 				enum rte_dev_event_type type,
 				void *param);
 static int eth_dev_event_callback_register(void);
@@ -2249,7 +2249,7 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 
 /* This function is used by the interrupt thread */
 static void
-eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
+eth_dev_event_callback(const char *device_name, enum rte_dev_event_type type,
 			     __rte_unused void *arg)
 {
 	uint16_t port_id;
diff --git a/lib/librte_eal/bsdapp/eal/eal_dev.c b/lib/librte_eal/bsdapp/eal/eal_dev.c
index 255d611..3a3a2a5 100644
--- a/lib/librte_eal/bsdapp/eal/eal_dev.c
+++ b/lib/librte_eal/bsdapp/eal/eal_dev.c
@@ -33,3 +33,11 @@ rte_dev_hotplug_handle_disable(void)
 	RTE_LOG(ERR, EAL, "Device event is not supported for FreeBSD\n");
 	return -1;
 }
+
+void  __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event)
+{
+	RTE_LOG(ERR, EAL,
+		"Device event callback process is not supported for FreeBSD.\n");
+}
diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 678dbca..2d610a4 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -342,8 +342,9 @@ rte_dev_event_callback_unregister(const char *device_name,
 	return ret;
 }
 
-void
-dev_callback_process(char *device_name, enum rte_dev_event_type event)
+void __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event)
 {
 	struct dev_event_callback *cb_lst;
 
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 637f20d..47e8a33 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -259,18 +259,6 @@ struct rte_bus *rte_bus_find_by_device_name(const char *str);
 int rte_mp_channel_init(void);
 
 /**
- * Internal Executes all the user application registered callbacks for
- * the specific device. It is for DPDK internal user only. User
- * application should not call it directly.
- *
- * @param device_name
- *  The device name.
- * @param event
- *  the device event type.
- */
-void dev_callback_process(char *device_name, enum rte_dev_event_type event);
-
-/**
  * @internal
  * Parse a device string and store its information in an
  * rte_devargs structure.
diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index ff580a0..58fab43 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -39,7 +39,7 @@ struct rte_dev_event {
 	char *devname;			/**< device name */
 };
 
-typedef void (*rte_dev_event_cb_fn)(char *device_name,
+typedef void (*rte_dev_event_cb_fn)(const char *device_name,
 					enum rte_dev_event_type event,
 					void *cb_arg);
 
@@ -438,6 +438,22 @@ rte_dev_event_callback_unregister(const char *device_name,
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
  *
+ * Executes all the user application registered callbacks for
+ * the specific device.
+ *
+ * @param device_name
+ *  The device name.
+ * @param event
+ *  the device event type.
+ */
+void  __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
  * Start the device event monitoring.
  *
  * @return
diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c b/lib/librte_eal/linuxapp/eal/eal_dev.c
index 72fc033..3b834fa 100644
--- a/lib/librte_eal/linuxapp/eal/eal_dev.c
+++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
@@ -271,7 +271,7 @@ dev_uev_handler(__rte_unused void *param)
 				return;
 			}
 		}
-		dev_callback_process(uevent.devname, uevent.type);
+		rte_dev_event_callback_process(uevent.devname, uevent.type);
 	}
 }
 
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index a3255aa..b96da50 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -277,6 +277,7 @@ EXPERIMENTAL {
 	rte_class_register;
 	rte_class_unregister;
 	rte_ctrl_thread_create;
+	rte_dev_event_callback_process;
 	rte_dev_event_callback_register;
 	rte_dev_event_callback_unregister;
 	rte_dev_event_monitor_start;
-- 
2.7.4

[PATCH v3 3/4] pci: add req handler field to generic pci device

[Jeff Guo]

There are some extended interrupt types in vfio pci device except from the
existing interrupts, such as err and req notifier, they could be useful for
device error monitoring. And these corresponding interrupt handler is
different from the other interrupt handler that register in PMDs, so a new
interrupt handler should be added. This patch will add specific req handler
in generic pci device.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v3->v2:
no change.
---
 drivers/bus/pci/rte_bus_pci.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
index 0d1955f..c45a820 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -66,6 +66,7 @@ struct rte_pci_device {
 	uint16_t max_vfs;                   /**< sriov enable if not zero */
 	enum rte_kernel_driver kdrv;        /**< Kernel driver passthrough */
 	char name[PCI_PRI_STR_SIZE+1];      /**< PCI location (ASCII) */
+	struct rte_intr_handle req_notifier_handler;/**< Req notifier handle */
 };
 
 /**
-- 
2.7.4

[PATCH v3 4/4] vfio: enable vfio hotplug by req notifier handler

[Jeff Guo]

When device is be hot-unplugged, the vfio kernel module will sent req
notifier to request user space to release the allocated resources at
first. After that, vfio kernel module will detect the device disappear,
and then delete the device in kernel.

This patch aim to add req notifier processing to enable hotplug for vfio.
By enable the req notifier monitoring and register the notifier callback,
when device be hot-unplugged, the hot-unplug handler will be called to
process hotplug for vfio.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v3->v2:
change some code style and typo.
---
 drivers/bus/pci/linux/pci_vfio.c | 111 +++++++++++++++++++++++++++++++++++++++
 drivers/bus/pci/pci_common.c     |  10 ++++
 2 files changed, 121 insertions(+)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 686386d..5d3d026 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -17,6 +17,8 @@
 #include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
 #include <rte_vfio.h>
+#include <rte_eal.h>
+#include <rte_bus.h>
 
 #include "eal_filesystem.h"
 
@@ -277,6 +279,101 @@ pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd)
 	return -1;
 }
 
+static void
+pci_vfio_req_handler(void *param)
+{
+	struct rte_bus *bus;
+	int ret;
+	struct rte_device *device = (struct rte_device *)param;
+
+	bus = rte_bus_find_by_device(device);
+	if (bus == NULL) {
+		RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n",
+			device->name);
+		return;
+	}
+
+	/*
+	 * vfio kernel module request user space to release allocated
+	 * resources before device be deleted in kernel, so it can directly
+	 * call the vfio bus hot-unplug handler to process it.
+	 */
+	ret = bus->hot_unplug_handler(device);
+	if (ret)
+		RTE_LOG(ERR, EAL,
+			"Can not handle hot-unplug for device (%s)\n",
+			device->name);
+}
+
+/* enable notifier (only enable req now) */
+static int
+pci_vfio_enable_notifier(struct rte_pci_device *dev, int vfio_dev_fd)
+{
+	int ret;
+	int fd = -1;
+
+	/* set up an eventfd for req notifier */
+	fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
+	if (fd < 0) {
+		RTE_LOG(ERR, EAL, "Cannot set up eventfd, error %i (%s)\n",
+			errno, strerror(errno));
+		return -1;
+	}
+
+	dev->req_notifier_handler.fd = fd;
+	dev->req_notifier_handler.type = RTE_INTR_HANDLE_VFIO_REQ;
+	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
+	ret = rte_intr_callback_register(&dev->req_notifier_handler,
+					 pci_vfio_req_handler,
+					 (void *)&dev->device);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Fail to register req notifier handler.\n");
+		goto error;
+	}
+
+	ret = rte_intr_enable(&dev->req_notifier_handler);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Fail to enable req notifier.\n");
+		ret = rte_intr_callback_unregister(&dev->req_notifier_handler,
+						 pci_vfio_req_handler,
+						 (void *)&dev->device);
+		if (ret)
+			RTE_LOG(ERR, EAL,
+				"Fail to unregister req notifier handler.\n");
+		goto error;
+	}
+
+	return 0;
+error:
+	close(fd);
+	return -1;
+}
+
+/* disable notifier (only disable req now) */
+static int
+pci_vfio_disable_notifier(struct rte_pci_device *dev)
+{
+	int ret;
+
+	ret = rte_intr_disable(&dev->req_notifier_handler);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "fail to disable req notifier.\n");
+		return -1;
+	}
+
+	ret = rte_intr_callback_unregister(&dev->req_notifier_handler,
+					   pci_vfio_req_handler,
+					   (void *)&dev->device);
+	if (ret) {
+		RTE_LOG(ERR, EAL,
+			 "fail to unregister req notifier handler.\n");
+		return -1;
+	}
+
+	close(dev->req_notifier_handler.fd);
+	return 0;
+}
+
 static int
 pci_vfio_is_ioport_bar(int vfio_dev_fd, int bar_index)
 {
@@ -430,6 +527,7 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
+	dev->req_notifier_handler.fd = -1;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
@@ -521,6 +619,11 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
 		goto err_vfio_res;
 	}
 
+	if (pci_vfio_enable_notifier(dev, vfio_dev_fd) != 0) {
+		RTE_LOG(ERR, EAL, "Error setting up notifier!\n");
+		goto err_vfio_res;
+	}
+
 	TAILQ_INSERT_TAIL(vfio_res_list, vfio_res, next);
 
 	return 0;
@@ -546,6 +649,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
+	dev->req_notifier_handler.fd = -1;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
@@ -586,6 +690,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 
 	/* we need save vfio_dev_fd, so it can be used during release */
 	dev->intr_handle.vfio_dev_fd = vfio_dev_fd;
+	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
 
 	return 0;
 err_vfio_dev_fd:
@@ -658,6 +763,12 @@ pci_vfio_unmap_resource_primary(struct rte_pci_device *dev)
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
 			loc->domain, loc->bus, loc->devid, loc->function);
 
+	ret = pci_vfio_disable_notifier(dev);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "fail to disable req notifier.\n");
+		return -1;
+	}
+
 	if (close(dev->intr_handle.fd) < 0) {
 		RTE_LOG(INFO, EAL, "Error when closing eventfd file descriptor for %s\n",
 			pci_addr);
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index f313fe9..51e69c1 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -446,6 +446,16 @@ pci_hot_unplug_handler(struct rte_device *dev)
 		return -1;
 
 	switch (pdev->kdrv) {
+	case RTE_KDRV_VFIO:
+		/*
+		 * vfio kernel module guaranty the pci device would not be
+		 * deleted until the user space release the resource, so no
+		 * need to remap BARs resource here, just directly notify
+		 * the req event to the user space to handle it.
+		 */
+		rte_dev_event_callback_process(dev->name,
+					       RTE_DEV_EVENT_REMOVE);
+		break;
 	case RTE_KDRV_IGB_UIO:
 	case RTE_KDRV_UIO_GENERIC:
 	case RTE_KDRV_NIC_UIO:
-- 
2.7.4

Re: [PATCH v3 4/4] vfio: enable vfio hotplug by req notifier handler

[Burakov, Anatoly]

On 02-Oct-18 1:58 PM, Jeff Guo wrote:
> When device is be hot-unplugged, the vfio kernel module will sent req
> notifier to request user space to release the allocated resources at
> first. After that, vfio kernel module will detect the device disappear,
> and then delete the device in kernel.
> 
> This patch aim to add req notifier processing to enable hotplug for vfio.
> By enable the req notifier monitoring and register the notifier callback,
> when device be hot-unplugged, the hot-unplug handler will be called to
> process hotplug for vfio.
> 
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
> v3->v2:
> change some code style and typo.
> ---

<snip>

> +/* enable notifier (only enable req now) */
> +static int
> +pci_vfio_enable_notifier(struct rte_pci_device *dev, int vfio_dev_fd)
> +{
> +	int ret;
> +	int fd = -1;
> +
> +	/* set up an eventfd for req notifier */
> +	fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
> +	if (fd < 0) {
> +		RTE_LOG(ERR, EAL, "Cannot set up eventfd, error %i (%s)\n",
> +			errno, strerror(errno));
> +		return -1;
> +	}
> +
> +	dev->req_notifier_handler.fd = fd;
> +	dev->req_notifier_handler.type = RTE_INTR_HANDLE_VFIO_REQ;
> +	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
> +	ret = rte_intr_callback_register(&dev->req_notifier_handler,
> +					 pci_vfio_req_handler,
> +					 (void *)&dev->device);
> +	if (ret) {
> +		RTE_LOG(ERR, EAL, "Fail to register req notifier handler.\n");
> +		goto error;

At this point (and further down as well), if we go to error, we've 
already modified the req_notifier_handler data. Do we need to overwrite 
it? Maybe just do these operations on a local struct, and then do a 
memcpy into dev->req_notifier_handler on success?

-- 
Thanks,
Anatoly

Re: [PATCH v3 4/4] vfio: enable vfio hotplug by req notifier handler

[Jeff Guo]

On 10/2/2018 10:15 PM, Burakov, Anatoly wrote:
> On 02-Oct-18 1:58 PM, Jeff Guo wrote:
>> When device is be hot-unplugged, the vfio kernel module will sent req
>> notifier to request user space to release the allocated resources at
>> first. After that, vfio kernel module will detect the device disappear,
>> and then delete the device in kernel.
>>
>> This patch aim to add req notifier processing to enable hotplug for 
>> vfio.
>> By enable the req notifier monitoring and register the notifier 
>> callback,
>> when device be hot-unplugged, the hot-unplug handler will be called to
>> process hotplug for vfio.
>>
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> ---
>> v3->v2:
>> change some code style and typo.
>> ---
>
> <snip>
>
>> +/* enable notifier (only enable req now) */
>> +static int
>> +pci_vfio_enable_notifier(struct rte_pci_device *dev, int vfio_dev_fd)
>> +{
>> +    int ret;
>> +    int fd = -1;
>> +
>> +    /* set up an eventfd for req notifier */
>> +    fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
>> +    if (fd < 0) {
>> +        RTE_LOG(ERR, EAL, "Cannot set up eventfd, error %i (%s)\n",
>> +            errno, strerror(errno));
>> +        return -1;
>> +    }
>> +
>> +    dev->req_notifier_handler.fd = fd;
>> +    dev->req_notifier_handler.type = RTE_INTR_HANDLE_VFIO_REQ;
>> +    dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
>> +    ret = rte_intr_callback_register(&dev->req_notifier_handler,
>> +                     pci_vfio_req_handler,
>> +                     (void *)&dev->device);
>> +    if (ret) {
>> +        RTE_LOG(ERR, EAL, "Fail to register req notifier handler.\n");
>> +        goto error;
>
> At this point (and further down as well), if we go to error, we've 
> already modified the req_notifier_handler data. Do we need to 
> overwrite it? Maybe just do these operations on a local struct, and 
> then do a memcpy into dev->req_notifier_handler on success?
>

I think just overwrite it in error process should be clear and simple.

[PATCH v4 0/4] Enable hotplug in vfio

[Jeff Guo]

As we may know that the process of hotplug is different between igb_uio
and vfio. For igb_uio, it could use uevent notification and memory
failure handle mechanism for hot-unplug. But for vfio, when device is be
hot-unplugged, the uevent can not be detected immediately, because of the
vfio kernel module will use a special mechanism to guaranty the pci
device would not be deleted until the user space release the resources,
so it will use another req notifier event at first to notify user space
to release resources for hotplug.

This patch will add a new interrupt type of req notifier in eal interrupt,
and add the new interrupt handler in pci device to handle the req device
event. When the req notifier be detected, it can trigger the device event
callback process to process for hot-unplug. With this mechanism, hotplug
could be enable in vfio.

patchset history:
v4->v3:
add some part of uninitialize for req handler.

v3->v2:
change some commit log and coding style and typo.

v2->v1:
change the rte_dev_event_callback_prcess from internal to external api
for bus or app usage.
change some code logic.

Jeff Guo (4):
  eal: add a new req notifier to eal interrupt
  eal: modify device event callback process func
  pci: add req handler field to generic pci device
  vfio: enable vfio hotplug by req notifier handler

 app/test-pmd/testpmd.c                             |   4 +-
 drivers/bus/pci/linux/pci_vfio.c                   | 122 +++++++++++++++++++++
 drivers/bus/pci/pci_common.c                       |  10 ++
 drivers/bus/pci/rte_bus_pci.h                      |   1 +
 lib/librte_eal/bsdapp/eal/eal_dev.c                |   8 ++
 lib/librte_eal/common/eal_common_dev.c             |   5 +-
 lib/librte_eal/common/eal_private.h                |  12 --
 lib/librte_eal/common/include/rte_dev.h            |  18 ++-
 lib/librte_eal/common/include/rte_eal_interrupts.h |   1 +
 lib/librte_eal/linuxapp/eal/eal_dev.c              |   2 +-
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       |  71 ++++++++++++
 lib/librte_eal/rte_eal_version.map                 |   1 +
 12 files changed, 237 insertions(+), 18 deletions(-)

-- 
2.7.4

[PATCH v4 1/4] eal: add a new req notifier to eal interrupt

[Jeff Guo]

Add a new req notifier in eal interrupt for enable vfio hotplug.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v4->v3:
no change.
---
 lib/librte_eal/common/include/rte_eal_interrupts.h |  1 +
 lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 71 ++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_eal_interrupts.h b/lib/librte_eal/common/include/rte_eal_interrupts.h
index 6eb4932..5204ed4 100644
--- a/lib/librte_eal/common/include/rte_eal_interrupts.h
+++ b/lib/librte_eal/common/include/rte_eal_interrupts.h
@@ -35,6 +35,7 @@ enum rte_intr_handle_type {
 	RTE_INTR_HANDLE_EXT,          /**< external handler */
 	RTE_INTR_HANDLE_VDEV,         /**< virtual device */
 	RTE_INTR_HANDLE_DEV_EVENT,    /**< device event handle */
+	RTE_INTR_HANDLE_VFIO_REQ,     /**< vfio device handle (req) */
 	RTE_INTR_HANDLE_MAX           /**< count of elements */
 };
 
diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index 4076c6d..7f611b3 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -308,6 +308,64 @@ vfio_disable_msix(const struct rte_intr_handle *intr_handle) {
 
 	return ret;
 }
+
+/* enable req notifier */
+static int
+vfio_enable_req(const struct rte_intr_handle *intr_handle)
+{
+	int len, ret;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	struct vfio_irq_set *irq_set;
+	int *fd_ptr;
+
+	len = sizeof(irq_set_buf);
+
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 1;
+	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
+			 VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
+	irq_set->start = 0;
+	fd_ptr = (int *) &irq_set->data;
+	*fd_ptr = intr_handle->fd;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Error enabling req interrupts for fd %d\n",
+						intr_handle->fd);
+		return -1;
+	}
+
+	return 0;
+}
+
+/* disable req notifier */
+static int
+vfio_disable_req(const struct rte_intr_handle *intr_handle)
+{
+	struct vfio_irq_set *irq_set;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	int len, ret;
+
+	len = sizeof(struct vfio_irq_set);
+
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 0;
+	irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
+	irq_set->start = 0;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret)
+		RTE_LOG(ERR, EAL, "Error disabling req interrupts for fd %d\n",
+			intr_handle->fd);
+
+	return ret;
+}
 #endif
 
 static int
@@ -556,6 +614,10 @@ rte_intr_enable(const struct rte_intr_handle *intr_handle)
 		if (vfio_enable_intx(intr_handle))
 			return -1;
 		break;
+	case RTE_INTR_HANDLE_VFIO_REQ:
+		if (vfio_enable_req(intr_handle))
+			return -1;
+		break;
 #endif
 	/* not used at this moment */
 	case RTE_INTR_HANDLE_DEV_EVENT:
@@ -606,6 +668,11 @@ rte_intr_disable(const struct rte_intr_handle *intr_handle)
 		if (vfio_disable_intx(intr_handle))
 			return -1;
 		break;
+	case RTE_INTR_HANDLE_VFIO_REQ:
+		if (vfio_disable_req(intr_handle))
+			return -1;
+		break;
+
 #endif
 	/* not used at this moment */
 	case RTE_INTR_HANDLE_DEV_EVENT:
@@ -682,6 +749,10 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
 			bytes_read = 0;
 			call = true;
 			break;
+		case RTE_INTR_HANDLE_VFIO_REQ:
+			bytes_read = 0;
+			call = true;
+			break;
 		default:
 			bytes_read = 1;
 			break;
-- 
2.7.4

[PATCH v4 2/4] eal: modify device event callback process func

[Jeff Guo]

This patch modify the device event callback process function name to be
more explicit, change the variable to be const. And more, because not only
eal device helper will use the callback, but also vfio bus will use the
callback to handle hot-unplug, so exposure the API out from private eal.
The bus drivers and eal device would directly use this API to process
device event callback.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v4->v3:
no change.
---
 app/test-pmd/testpmd.c                  |  4 ++--
 lib/librte_eal/bsdapp/eal/eal_dev.c     |  8 ++++++++
 lib/librte_eal/common/eal_common_dev.c  |  5 +++--
 lib/librte_eal/common/eal_private.h     | 12 ------------
 lib/librte_eal/common/include/rte_dev.h | 18 +++++++++++++++++-
 lib/librte_eal/linuxapp/eal/eal_dev.c   |  2 +-
 lib/librte_eal/rte_eal_version.map      |  1 +
 7 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index f3f8e44..14a6ab0 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -431,7 +431,7 @@ static void check_all_ports_link_status(uint32_t port_mask);
 static int eth_event_callback(portid_t port_id,
 			      enum rte_eth_event_type type,
 			      void *param, void *ret_param);
-static void eth_dev_event_callback(char *device_name,
+static void eth_dev_event_callback(const char *device_name,
 				enum rte_dev_event_type type,
 				void *param);
 
@@ -2216,7 +2216,7 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 
 /* This function is used by the interrupt thread */
 static void
-eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
+eth_dev_event_callback(const char *device_name, enum rte_dev_event_type type,
 			     __rte_unused void *arg)
 {
 	uint16_t port_id;
diff --git a/lib/librte_eal/bsdapp/eal/eal_dev.c b/lib/librte_eal/bsdapp/eal/eal_dev.c
index 255d611..3a3a2a5 100644
--- a/lib/librte_eal/bsdapp/eal/eal_dev.c
+++ b/lib/librte_eal/bsdapp/eal/eal_dev.c
@@ -33,3 +33,11 @@ rte_dev_hotplug_handle_disable(void)
 	RTE_LOG(ERR, EAL, "Device event is not supported for FreeBSD\n");
 	return -1;
 }
+
+void  __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event)
+{
+	RTE_LOG(ERR, EAL,
+		"Device event callback process is not supported for FreeBSD.\n");
+}
diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 678dbca..2d610a4 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -342,8 +342,9 @@ rte_dev_event_callback_unregister(const char *device_name,
 	return ret;
 }
 
-void
-dev_callback_process(char *device_name, enum rte_dev_event_type event)
+void __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event)
 {
 	struct dev_event_callback *cb_lst;
 
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 637f20d..47e8a33 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -259,18 +259,6 @@ struct rte_bus *rte_bus_find_by_device_name(const char *str);
 int rte_mp_channel_init(void);
 
 /**
- * Internal Executes all the user application registered callbacks for
- * the specific device. It is for DPDK internal user only. User
- * application should not call it directly.
- *
- * @param device_name
- *  The device name.
- * @param event
- *  the device event type.
- */
-void dev_callback_process(char *device_name, enum rte_dev_event_type event);
-
-/**
  * @internal
  * Parse a device string and store its information in an
  * rte_devargs structure.
diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index ff580a0..58fab43 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -39,7 +39,7 @@ struct rte_dev_event {
 	char *devname;			/**< device name */
 };
 
-typedef void (*rte_dev_event_cb_fn)(char *device_name,
+typedef void (*rte_dev_event_cb_fn)(const char *device_name,
 					enum rte_dev_event_type event,
 					void *cb_arg);
 
@@ -438,6 +438,22 @@ rte_dev_event_callback_unregister(const char *device_name,
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
  *
+ * Executes all the user application registered callbacks for
+ * the specific device.
+ *
+ * @param device_name
+ *  The device name.
+ * @param event
+ *  the device event type.
+ */
+void  __rte_experimental
+rte_dev_event_callback_process(const char *device_name,
+			       enum rte_dev_event_type event);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
  * Start the device event monitoring.
  *
  * @return
diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c b/lib/librte_eal/linuxapp/eal/eal_dev.c
index 4695fcb..b95d9ee 100644
--- a/lib/librte_eal/linuxapp/eal/eal_dev.c
+++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
@@ -277,7 +277,7 @@ dev_uev_handler(__rte_unused void *param)
 				return;
 			}
 		}
-		dev_callback_process(uevent.devname, uevent.type);
+		rte_dev_event_callback_process(uevent.devname, uevent.type);
 	}
 }
 
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index b167b8f..7ae11d8 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -277,6 +277,7 @@ EXPERIMENTAL {
 	rte_class_register;
 	rte_class_unregister;
 	rte_ctrl_thread_create;
+	rte_dev_event_callback_process;
 	rte_dev_event_callback_register;
 	rte_dev_event_callback_unregister;
 	rte_dev_event_monitor_start;
-- 
2.7.4

[PATCH v4 3/4] pci: add req handler field to generic pci device

[Jeff Guo]

There are some extended interrupt types in vfio pci device except from the
existing interrupts, such as err and req notifier, they could be useful for
device error monitoring. And these corresponding interrupt handler is
different from the other interrupt handler that register in PMDs, so a new
interrupt handler should be added. This patch will add specific req handler
in generic pci device.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v4->v3:
no change.
---
 drivers/bus/pci/rte_bus_pci.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
index 0d1955f..c45a820 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -66,6 +66,7 @@ struct rte_pci_device {
 	uint16_t max_vfs;                   /**< sriov enable if not zero */
 	enum rte_kernel_driver kdrv;        /**< Kernel driver passthrough */
 	char name[PCI_PRI_STR_SIZE+1];      /**< PCI location (ASCII) */
+	struct rte_intr_handle req_notifier_handler;/**< Req notifier handle */
 };
 
 /**
-- 
2.7.4

[PATCH v4 4/4] vfio: enable vfio hotplug by req notifier handler

[Jeff Guo]

When device is be hot-unplugged, the vfio kernel module will sent req
notifier to request user space to release the allocated resources at
first. After that, vfio kernel module will detect the device disappear,
and then delete the device in kernel.

This patch aim to add req notifier processing to enable hotplug for vfio.
By enable the req notifier monitoring and register the notifier callback,
when device be hot-unplugged, the hot-unplug handler will be called to
process hotplug for vfio.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v4->v3:
add some part of uninitialize for req handler.
---
 drivers/bus/pci/linux/pci_vfio.c | 122 +++++++++++++++++++++++++++++++++++++++
 drivers/bus/pci/pci_common.c     |  10 ++++
 2 files changed, 132 insertions(+)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 686386d..32ba3e0 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -17,6 +17,8 @@
 #include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
 #include <rte_vfio.h>
+#include <rte_eal.h>
+#include <rte_bus.h>
 
 #include "eal_filesystem.h"
 
@@ -277,6 +279,112 @@ pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd)
 	return -1;
 }
 
+static void
+pci_vfio_req_handler(void *param)
+{
+	struct rte_bus *bus;
+	int ret;
+	struct rte_device *device = (struct rte_device *)param;
+
+	bus = rte_bus_find_by_device(device);
+	if (bus == NULL) {
+		RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n",
+			device->name);
+		return;
+	}
+
+	/*
+	 * vfio kernel module request user space to release allocated
+	 * resources before device be deleted in kernel, so it can directly
+	 * call the vfio bus hot-unplug handler to process it.
+	 */
+	ret = bus->hot_unplug_handler(device);
+	if (ret)
+		RTE_LOG(ERR, EAL,
+			"Can not handle hot-unplug for device (%s)\n",
+			device->name);
+}
+
+/* enable notifier (only enable req now) */
+static int
+pci_vfio_enable_notifier(struct rte_pci_device *dev, int vfio_dev_fd)
+{
+	int ret;
+	int fd = -1;
+
+	/* set up an eventfd for req notifier */
+	fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
+	if (fd < 0) {
+		RTE_LOG(ERR, EAL, "Cannot set up eventfd, error %i (%s)\n",
+			errno, strerror(errno));
+		return -1;
+	}
+
+	dev->req_notifier_handler.fd = fd;
+	dev->req_notifier_handler.type = RTE_INTR_HANDLE_VFIO_REQ;
+	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
+
+	ret = rte_intr_callback_register(&dev->req_notifier_handler,
+					 pci_vfio_req_handler,
+					 (void *)&dev->device);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Fail to register req notifier handler.\n");
+		goto error;
+	}
+
+	ret = rte_intr_enable(&dev->req_notifier_handler);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Fail to enable req notifier.\n");
+		ret = rte_intr_callback_unregister(&dev->req_notifier_handler,
+						 pci_vfio_req_handler,
+						 (void *)&dev->device);
+		if (ret)
+			RTE_LOG(ERR, EAL,
+				"Fail to unregister req notifier handler.\n");
+		goto error;
+	}
+
+	return 0;
+error:
+	close(fd);
+
+	dev->req_notifier_handler.fd = -1;
+	dev->req_notifier_handler.type = RTE_INTR_HANDLE_UNKNOWN;
+	dev->req_notifier_handler.vfio_dev_fd = -1;
+
+	return -1;
+}
+
+/* disable notifier (only disable req now) */
+static int
+pci_vfio_disable_notifier(struct rte_pci_device *dev)
+{
+	int ret;
+
+	ret = rte_intr_disable(&dev->req_notifier_handler);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "fail to disable req notifier.\n");
+		return -1;
+	}
+
+	ret = rte_intr_callback_unregister(&dev->req_notifier_handler,
+					   pci_vfio_req_handler,
+					   (void *)&dev->device);
+	if (ret) {
+		RTE_LOG(ERR, EAL,
+			 "fail to unregister req notifier handler.\n");
+		return -1;
+	}
+
+	close(dev->req_notifier_handler.fd);
+
+	dev->req_notifier_handler.fd = -1;
+	dev->req_notifier_handler.type = RTE_INTR_HANDLE_UNKNOWN;
+	dev->req_notifier_handler.vfio_dev_fd = -1;
+
+	return 0;
+}
+
 static int
 pci_vfio_is_ioport_bar(int vfio_dev_fd, int bar_index)
 {
@@ -430,6 +538,7 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
+	dev->req_notifier_handler.fd = -1;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
@@ -521,6 +630,11 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
 		goto err_vfio_res;
 	}
 
+	if (pci_vfio_enable_notifier(dev, vfio_dev_fd) != 0) {
+		RTE_LOG(ERR, EAL, "Error setting up notifier!\n");
+		goto err_vfio_res;
+	}
+
 	TAILQ_INSERT_TAIL(vfio_res_list, vfio_res, next);
 
 	return 0;
@@ -546,6 +660,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
+	dev->req_notifier_handler.fd = -1;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
@@ -586,6 +701,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 
 	/* we need save vfio_dev_fd, so it can be used during release */
 	dev->intr_handle.vfio_dev_fd = vfio_dev_fd;
+	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
 
 	return 0;
 err_vfio_dev_fd:
@@ -658,6 +774,12 @@ pci_vfio_unmap_resource_primary(struct rte_pci_device *dev)
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
 			loc->domain, loc->bus, loc->devid, loc->function);
 
+	ret = pci_vfio_disable_notifier(dev);
+	if (ret) {
+		RTE_LOG(ERR, EAL, "fail to disable req notifier.\n");
+		return -1;
+	}
+
 	if (close(dev->intr_handle.fd) < 0) {
 		RTE_LOG(INFO, EAL, "Error when closing eventfd file descriptor for %s\n",
 			pci_addr);
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index f313fe9..51e69c1 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -446,6 +446,16 @@ pci_hot_unplug_handler(struct rte_device *dev)
 		return -1;
 
 	switch (pdev->kdrv) {
+	case RTE_KDRV_VFIO:
+		/*
+		 * vfio kernel module guaranty the pci device would not be
+		 * deleted until the user space release the resource, so no
+		 * need to remap BARs resource here, just directly notify
+		 * the req event to the user space to handle it.
+		 */
+		rte_dev_event_callback_process(dev->name,
+					       RTE_DEV_EVENT_REMOVE);
+		break;
 	case RTE_KDRV_IGB_UIO:
 	case RTE_KDRV_UIO_GENERIC:
 	case RTE_KDRV_NIC_UIO:
-- 
2.7.4

Re: [PATCH v4 0/4] Enable hotplug in vfio

[Burakov, Anatoly]

On 04-Oct-18 7:44 AM, Jeff Guo wrote:
> As we may know that the process of hotplug is different between igb_uio
> and vfio. For igb_uio, it could use uevent notification and memory
> failure handle mechanism for hot-unplug. But for vfio, when device is be
> hot-unplugged, the uevent can not be detected immediately, because of the
> vfio kernel module will use a special mechanism to guaranty the pci
> device would not be deleted until the user space release the resources,
> so it will use another req notifier event at first to notify user space
> to release resources for hotplug.
> 
> This patch will add a new interrupt type of req notifier in eal interrupt,
> and add the new interrupt handler in pci device to handle the req device
> event. When the req notifier be detected, it can trigger the device event
> callback process to process for hot-unplug. With this mechanism, hotplug
> could be enable in vfio.
> 

LGTM

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [PATCH v4 3/4] pci: add req handler field to generic pci device

[Thomas Monjalon]

04/10/2018 08:44, Jeff Guo:
> There are some extended interrupt types in vfio pci device except from the
> existing interrupts, such as err and req notifier, they could be useful for
> device error monitoring. And these corresponding interrupt handler is
> different from the other interrupt handler that register in PMDs, so a new
> interrupt handler should be added. This patch will add specific req handler
> in generic pci device.
> 
> Signed-off-by: Jeff Guo <jia.guo@intel.com>

> --- a/drivers/bus/pci/rte_bus_pci.h
> +++ b/drivers/bus/pci/rte_bus_pci.h
> @@ -66,6 +66,7 @@ struct rte_pci_device {
>  	uint16_t max_vfs;                   /**< sriov enable if not zero */
>  	enum rte_kernel_driver kdrv;        /**< Kernel driver passthrough */
>  	char name[PCI_PRI_STR_SIZE+1];      /**< PCI location (ASCII) */
> +	struct rte_intr_handle req_notifier_handler;/**< Req notifier handle */

We had intr_handle, should we name this one req_notifier_handle?
May we improve the comment to better explain? (and add a space before)

Maybe we need to update the comment of the other handle?
    struct rte_intr_handle intr_handle; /**< Interrupt handle */

Re: [PATCH v4 3/4] pci: add req handler field to generic pci device

[Thomas Monjalon]

15/10/2018 11:12, Thomas Monjalon:
> 04/10/2018 08:44, Jeff Guo:
> > There are some extended interrupt types in vfio pci device except from the
> > existing interrupts, such as err and req notifier, they could be useful for
> > device error monitoring. And these corresponding interrupt handler is
> > different from the other interrupt handler that register in PMDs, so a new
> > interrupt handler should be added. This patch will add specific req handler
> > in generic pci device.
> > 
> > Signed-off-by: Jeff Guo <jia.guo@intel.com>
> 
> > --- a/drivers/bus/pci/rte_bus_pci.h
> > +++ b/drivers/bus/pci/rte_bus_pci.h
> > @@ -66,6 +66,7 @@ struct rte_pci_device {
> >  	uint16_t max_vfs;                   /**< sriov enable if not zero */
> >  	enum rte_kernel_driver kdrv;        /**< Kernel driver passthrough */
> >  	char name[PCI_PRI_STR_SIZE+1];      /**< PCI location (ASCII) */
> > +	struct rte_intr_handle req_notifier_handler;/**< Req notifier handle */
> 
> We had intr_handle, should we name this one req_notifier_handle?
> May we improve the comment to better explain? (and add a space before)

On IRC, we agreed on this name/comment:

struct rte_intr_handle vfio_req_intr_handle; /**< Handler of VFIO request interrupt */

> Maybe we need to update the comment of the other handle?
>     struct rte_intr_handle intr_handle; /**< Interrupt handle */

OK to keep this one as-is.

Re: [PATCH v4 0/4] Enable hotplug in vfio

[Thomas Monjalon]

04/10/2018 11:11, Burakov, Anatoly:
> On 04-Oct-18 7:44 AM, Jeff Guo wrote:
> > As we may know that the process of hotplug is different between igb_uio
> > and vfio. For igb_uio, it could use uevent notification and memory
> > failure handle mechanism for hot-unplug. But for vfio, when device is be
> > hot-unplugged, the uevent can not be detected immediately, because of the
> > vfio kernel module will use a special mechanism to guaranty the pci
> > device would not be deleted until the user space release the resources,
> > so it will use another req notifier event at first to notify user space
> > to release resources for hotplug.
> > 
> > This patch will add a new interrupt type of req notifier in eal interrupt,
> > and add the new interrupt handler in pci device to handle the req device
> > event. When the req notifier be detected, it can trigger the device event
> > callback process to process for hot-unplug. With this mechanism, hotplug
> > could be enable in vfio.
> 
> LGTM
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Applied, thanks