[PATCH v2 0/4] VFIO platform reset

[PATCH v2 0/4] VFIO platform reset

[Eric Auger]

In situations where the userspace driver is stopped abnormally and the
VFIO platform device is released, the assigned HW device currently is
left running. As a consequence the HW device might continue issuing IRQs
and performing DMA accesses.

On release, no physical IRQ handler is setup anymore. Also the DMA buffers
are unmapped leading to IOMMU aborts. So there is no serious consequence.

However when assigning that HW device again to another userspace driver,
this latter might face some unexpected IRQs and DMA accesses, which are
the result of the previous assignment.

In virtualization use-case, a VM newly granted with that HW device may be
impacted by the assignment of that device to a previous VM:
- IRQs may be injected very early when booting the new guest, even before
  the guest driver has initialized leading to possible driver state
  inconsistency.
- DMA accesses may hit the newly mapped VM address space at addresses that
  may jeopardize the integrity of the newly installed VM.

Obviously the criticity depends on the assigned HW device.

As opposed to PCI, there is no standard mechanism to reset the platform
device.

This series proposes to implement device specific reset functions in
separate vfio reset modules (in-kernel or external). The vfio-platform
driver holds a whitelist of implemented triplets (compat string, module
name, function name). When the vfio-platform driver is probed it identifies
the fellow reset module/function matching the compat string of the
device, if any, and forces the load of this reset module.

A first reset module is provided: the vfio-platform-calxedaxgmac
module which implements a basic reset for the Calxeda xgmac.

The series can be found at
https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.1-rc6-reset-v2

History:
v1 -> v2:
- much simplified compared to v1 although principle of external modules is
  kept: removed mechanism of dynamic registration of reset functions
- list is replaced by whitelist lookup table
- name of the reset function also stored in the lookup table
- autoload of reset modules

RFC -> PATCH v1:
- solution now based on a lookup list instead of specialized driver


Eric Auger (4):
  VFIO: platform: add reset struct and lookup table
  VFIO: platform: add reset callback
  VFIO: platform: populate the reset function on probe
  VFIO: platform: Calxeda xgmac reset module

 drivers/vfio/platform/Kconfig                      |  2 +
 drivers/vfio/platform/Makefile                     |  2 +
 drivers/vfio/platform/reset/Kconfig                |  7 ++
 drivers/vfio/platform/reset/Makefile               |  5 ++
 .../platform/reset/vfio_platform_calxedaxgmac.c    | 98 ++++++++++++++++++++++
 drivers/vfio/platform/vfio_platform_common.c       | 75 ++++++++++++++++-
 drivers/vfio/platform/vfio_platform_private.h      | 14 ++++
 7 files changed, 200 insertions(+), 3 deletions(-)
 create mode 100644 drivers/vfio/platform/reset/Kconfig
 create mode 100644 drivers/vfio/platform/reset/Makefile
 create mode 100644 drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c

-- 
1.9.1

[PATCH v2 1/4] VFIO: platform: add reset struct and lookup table

[Eric Auger]

This patch introduces the vfio_platform_reset_combo struct that
stores all the information useful to handle the reset modality:
compat string, name of the reset function, name of the module that
implements the reset function. A lookup table of such structures
is added, currently containing a single sentinel element. A new
type field is added in vfio_platform_device to store what kind of
reset is associated to the device, if any.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 drivers/vfio/platform/vfio_platform_common.c  |  6 ++++++
 drivers/vfio/platform/vfio_platform_private.h | 12 ++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index abcff7a..d970776 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -25,6 +25,12 @@
 
 static DEFINE_MUTEX(driver_lock);
 
+static const struct vfio_platform_reset_combo reset_lookup_table[] = {
+	{
+		.type = VFIO_PLATFORM_RESET_TYPE_MAX
+	},
+};
+
 static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
 {
 	int cnt = 0, i;
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 5d31e04..d864124 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -49,6 +49,10 @@ struct vfio_platform_region {
 	void __iomem		*ioaddr;
 };
 
+enum vfio_platform_reset_type {
+	VFIO_PLATFORM_RESET_TYPE_MAX /* last element */,
+};
+
 struct vfio_platform_device {
 	struct vfio_platform_region	*regions;
 	u32				num_regions;
@@ -56,6 +60,7 @@ struct vfio_platform_device {
 	u32				num_irqs;
 	int				refcnt;
 	struct mutex			igate;
+	enum vfio_platform_reset_type	type;
 
 	/*
 	 * These fields should be filled by the bus specific binder
@@ -69,6 +74,13 @@ struct vfio_platform_device {
 	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
 };
 
+struct vfio_platform_reset_combo {
+	enum vfio_platform_reset_type type;
+	char *compat;
+	char *reset_function_name;
+	char *module_name;
+};
+
 extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 				      struct device *dev);
 extern struct vfio_platform_device *vfio_platform_remove_common
-- 
1.9.1

[PATCH v2 2/4] VFIO: platform: add reset callback

[Eric Auger]

A new reset callback is introduced. If this callback is populated,
the reset is invoked on device first open/last close or upon userspace
ioctl.  The modality is exposed on VFIO_DEVICE_GET_INFO.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v1 -> v2:
- reset now is also called on first open on top of last close
- remove IS_ERR_OR_NULL
---
 drivers/vfio/platform/vfio_platform_common.c  | 15 +++++++++++++--
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index d970776..995929b 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -106,6 +106,8 @@ static void vfio_platform_release(void *device_data)
 	mutex_lock(&driver_lock);
 
 	if (!(--vdev->refcnt)) {
+		if (vdev->reset)
+			vdev->reset(vdev);
 		vfio_platform_regions_cleanup(vdev);
 		vfio_platform_irq_cleanup(vdev);
 	}
@@ -133,6 +135,9 @@ static int vfio_platform_open(void *device_data)
 		ret = vfio_platform_irq_init(vdev);
 		if (ret)
 			goto err_irq;
+
+		if (vdev->reset)
+			vdev->reset(vdev);
 	}
 
 	vdev->refcnt++;
@@ -165,6 +170,8 @@ static long vfio_platform_ioctl(void *device_data,
 		if (info.argsz < minsz)
 			return -EINVAL;
 
+		if (vdev->reset)
+			vdev->flags |= VFIO_DEVICE_FLAGS_RESET;
 		info.flags = vdev->flags;
 		info.num_regions = vdev->num_regions;
 		info.num_irqs = vdev->num_irqs;
@@ -258,8 +265,12 @@ static long vfio_platform_ioctl(void *device_data,
 
 		return ret;
 
-	} else if (cmd == VFIO_DEVICE_RESET)
-		return -EINVAL;
+	} else if (cmd == VFIO_DEVICE_RESET) {
+		if (vdev->reset)
+			return vdev->reset(vdev);
+		else
+			return -EINVAL;
+	}
 
 	return -ENOTTY;
 }
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index d864124..cccb3ba 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -72,6 +72,7 @@ struct vfio_platform_device {
 	struct resource*
 		(*get_resource)(struct vfio_platform_device *vdev, int i);
 	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
+	int	(*reset)(struct vfio_platform_device *vdev);
 };
 
 struct vfio_platform_reset_combo {
-- 
1.9.1

[PATCH v2 3/4] VFIO: platform: populate the reset function on probe

[Eric Auger]

The reset function lookup happens on vfio-platform probe. The reset
module load is requested  and a reference to the function symbol is
hold. The reference is released on vfio-platform remove.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v1 -> v2:
- [get,put]_reset now is called once on probe/remove
- use request_module to automatically load the reset module that
  matches the compatibility string
- lookup table is used instead of list
- remove registration mechanism: reset function name is stored in the
  lookup table.
- use device_property_read_string instead of
  device_property_read_string_array
---
 drivers/vfio/platform/vfio_platform_common.c | 48 +++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 995929b..d474d6a 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -31,6 +31,47 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = {
 	},
 };
 
+static int vfio_platform_get_reset(struct vfio_platform_device *vdev,
+				   struct device *dev)
+{
+	const char *compat;
+	const struct vfio_platform_reset_combo *iter = reset_lookup_table;
+	int (*reset)(struct vfio_platform_device *);
+	int ret;
+
+	vdev->type = VFIO_PLATFORM_RESET_TYPE_MAX;
+	ret = device_property_read_string(dev, "compatible", &compat);
+	if (ret)
+		return ret;
+
+	while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
+		if (!strcmp(iter->compat, compat)) {
+			request_module(iter->module_name);
+			reset = __symbol_get(iter->reset_function_name);
+			if (reset) {
+				vdev->type = iter->type;
+				vdev->reset = reset;
+				return 0;
+			}
+		}
+		iter++;
+	}
+	return -1;
+}
+
+static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
+{
+	const struct vfio_platform_reset_combo *iter = reset_lookup_table;
+
+	while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
+		if (iter->type == vdev->type) {
+			__symbol_put(iter->reset_function_name);
+			return;
+		}
+		iter++;
+	}
+}
+
 static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
 {
 	int cnt = 0, i;
@@ -519,6 +560,8 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 		return ret;
 	}
 
+	vfio_platform_get_reset(vdev, dev);
+
 	mutex_init(&vdev->igate);
 
 	return 0;
@@ -530,8 +573,11 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
 	struct vfio_platform_device *vdev;
 
 	vdev = vfio_del_group_dev(dev);
-	if (vdev)
+
+	if (vdev) {
+		vfio_platform_put_reset(vdev);
 		iommu_group_put(dev->iommu_group);
+	}
 
 	return vdev;
 }
-- 
1.9.1

[PATCH v2 4/4] VFIO: platform: Calxeda xgmac reset module

[Eric Auger]

This patch introduces a module that registers and implements a basic
reset function for the Calxeda xgmac device. This latter basically disables
interrupts and stops DMA transfers.

The reset function code is inherited from the native calxeda xgmac driver.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v1 -> v2:
- remove registration mechanism. The module mainly exports the reset
  function. module_init and module_exit now are void.
---
 drivers/vfio/platform/Kconfig                      |  2 +
 drivers/vfio/platform/Makefile                     |  2 +
 drivers/vfio/platform/reset/Kconfig                |  7 ++
 drivers/vfio/platform/reset/Makefile               |  5 ++
 .../platform/reset/vfio_platform_calxedaxgmac.c    | 98 ++++++++++++++++++++++
 drivers/vfio/platform/vfio_platform_common.c       |  6 ++
 drivers/vfio/platform/vfio_platform_private.h      |  1 +
 7 files changed, 121 insertions(+)
 create mode 100644 drivers/vfio/platform/reset/Kconfig
 create mode 100644 drivers/vfio/platform/reset/Makefile
 create mode 100644 drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c

diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
index 9a4403e..1df7477 100644
--- a/drivers/vfio/platform/Kconfig
+++ b/drivers/vfio/platform/Kconfig
@@ -18,3 +18,5 @@ config VFIO_AMBA
 	  framework.
 
 	  If you don't know what to do here, say N.
+
+source "drivers/vfio/platform/reset/Kconfig"
diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
index 81de144..9ce8afe 100644
--- a/drivers/vfio/platform/Makefile
+++ b/drivers/vfio/platform/Makefile
@@ -2,7 +2,9 @@
 vfio-platform-y := vfio_platform.o vfio_platform_common.o vfio_platform_irq.o
 
 obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
+obj-$(CONFIG_VFIO_PLATFORM) += reset/
 
 vfio-amba-y := vfio_amba.o
 
 obj-$(CONFIG_VFIO_AMBA) += vfio-amba.o
+obj-$(CONFIG_VFIO_AMBA) += reset/
diff --git a/drivers/vfio/platform/reset/Kconfig b/drivers/vfio/platform/reset/Kconfig
new file mode 100644
index 0000000..746b96b
--- /dev/null
+++ b/drivers/vfio/platform/reset/Kconfig
@@ -0,0 +1,7 @@
+config VFIO_PLATFORM_CALXEDAXGMAC_RESET
+	tristate "VFIO support for calxeda xgmac reset"
+	depends on VFIO_PLATFORM
+	help
+	  Enables the VFIO platform driver to handle reset for Calxeda xgmac
+
+	  If you don't know what to do here, say N.
diff --git a/drivers/vfio/platform/reset/Makefile b/drivers/vfio/platform/reset/Makefile
new file mode 100644
index 0000000..2a486af
--- /dev/null
+++ b/drivers/vfio/platform/reset/Makefile
@@ -0,0 +1,5 @@
+vfio-platform-calxedaxgmac-y := vfio_platform_calxedaxgmac.o
+
+ccflags-y += -Idrivers/vfio/platform
+
+obj-$(CONFIG_VFIO_PLATFORM_CALXEDAXGMAC_RESET) += vfio-platform-calxedaxgmac.o
diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
new file mode 100644
index 0000000..bdf4c7b
--- /dev/null
+++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
@@ -0,0 +1,98 @@
+/*
+ * VFIO platform driver specialized for Calxeda xgmac reset
+ * reset code is inherited from calxeda xgmac native driver
+ *
+ * Copyright 2010-2011 Calxeda, Inc.
+ * Copyright (c) 2015 Linaro Ltd.
+ *              www.linaro.org
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/io.h>
+
+#include "vfio_platform_private.h"
+
+#define DRIVER_VERSION  "0.1"
+#define DRIVER_AUTHOR   "Eric Auger <eric.auger@linaro.org>"
+#define DRIVER_DESC     "Reset support for Calxeda xgmac vfio platform device"
+
+#define CALXEDAXGMAC_COMPAT "calxeda,hb-xgmac"
+
+/* XGMAC Register definitions */
+#define XGMAC_CONTROL           0x00000000      /* MAC Configuration */
+
+/* DMA Control and Status Registers */
+#define XGMAC_DMA_CONTROL       0x00000f18      /* Ctrl (Operational Mode) */
+#define XGMAC_DMA_INTR_ENA      0x00000f1c      /* Interrupt Enable */
+
+/* DMA Control registe defines */
+#define DMA_CONTROL_ST          0x00002000      /* Start/Stop Transmission */
+#define DMA_CONTROL_SR          0x00000002      /* Start/Stop Receive */
+
+/* Common MAC defines */
+#define MAC_ENABLE_TX           0x00000008      /* Transmitter Enable */
+#define MAC_ENABLE_RX           0x00000004      /* Receiver Enable */
+
+static inline void xgmac_mac_disable(void __iomem *ioaddr)
+{
+	u32 value = readl(ioaddr + XGMAC_DMA_CONTROL);
+
+	value &= ~(DMA_CONTROL_ST | DMA_CONTROL_SR);
+	writel(value, ioaddr + XGMAC_DMA_CONTROL);
+
+	value = readl(ioaddr + XGMAC_CONTROL);
+	value &= ~(MAC_ENABLE_TX | MAC_ENABLE_RX);
+	writel(value, ioaddr + XGMAC_CONTROL);
+}
+
+int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
+{
+	struct vfio_platform_region reg = vdev->regions[0];
+
+	if (!reg.ioaddr) {
+		reg.ioaddr =
+			ioremap_nocache(reg.addr, reg.size);
+		if (!reg.ioaddr)
+			return -ENOMEM;
+	}
+
+	/* disable IRQ */
+	writel(0, reg.ioaddr + XGMAC_DMA_INTR_ENA);
+
+	/* Disable the MAC core */
+	xgmac_mac_disable(reg.ioaddr);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vfio_platform_calxedaxgmac_reset);
+
+static int __init vfio_platform_calxedaxgmac_init(void)
+{
+	return 0;
+}
+
+static void __exit vfio_platform_calxedaxgmac_exit(void)
+{
+}
+
+module_init(vfio_platform_calxedaxgmac_init);
+module_exit(vfio_platform_calxedaxgmac_exit);
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index d474d6a..ffd76d2 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -27,6 +27,12 @@ static DEFINE_MUTEX(driver_lock);
 
 static const struct vfio_platform_reset_combo reset_lookup_table[] = {
 	{
+		.type = VFIO_PLATFORM_RESET_CALXEDA_XGMAC,
+		.compat = "calxeda,hb-xgmac",
+		.reset_function_name = "vfio_platform_calxedaxgmac_reset",
+		.module_name = "vfio-platform-calxedaxgmac",
+	},
+	{
 		.type = VFIO_PLATFORM_RESET_TYPE_MAX
 	},
 };
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index cccb3ba..31f15c7 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -50,6 +50,7 @@ struct vfio_platform_region {
 };
 
 enum vfio_platform_reset_type {
+	VFIO_PLATFORM_RESET_CALXEDA_XGMAC,
 	VFIO_PLATFORM_RESET_TYPE_MAX /* last element */,
 };
 
-- 
1.9.1

[PATCH v2 0/4] VFIO platform reset

[Rob Herring]

On Fri, Jun 5, 2015 at 10:06 AM, Eric Auger <eric.auger@linaro.org> wrote:
> In situations where the userspace driver is stopped abnormally and the
> VFIO platform device is released, the assigned HW device currently is
> left running. As a consequence the HW device might continue issuing IRQs
> and performing DMA accesses.
>
> On release, no physical IRQ handler is setup anymore. Also the DMA buffers
> are unmapped leading to IOMMU aborts. So there is no serious consequence.
>
> However when assigning that HW device again to another userspace driver,
> this latter might face some unexpected IRQs and DMA accesses, which are
> the result of the previous assignment.

In general, shouldn't it just be a requirement that the drivers handle
this condition. You have the same problem with firmware/bootloaders
leaving h/w not in reset state or kexec'ing to a new kernel.

Rob

> In virtualization use-case, a VM newly granted with that HW device may be
> impacted by the assignment of that device to a previous VM:
> - IRQs may be injected very early when booting the new guest, even before
>   the guest driver has initialized leading to possible driver state
>   inconsistency.
> - DMA accesses may hit the newly mapped VM address space at addresses that
>   may jeopardize the integrity of the newly installed VM.
>
> Obviously the criticity depends on the assigned HW device.
>
> As opposed to PCI, there is no standard mechanism to reset the platform
> device.
>
> This series proposes to implement device specific reset functions in
> separate vfio reset modules (in-kernel or external). The vfio-platform
> driver holds a whitelist of implemented triplets (compat string, module
> name, function name). When the vfio-platform driver is probed it identifies
> the fellow reset module/function matching the compat string of the
> device, if any, and forces the load of this reset module.
>
> A first reset module is provided: the vfio-platform-calxedaxgmac
> module which implements a basic reset for the Calxeda xgmac.
>
> The series can be found at
> https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.1-rc6-reset-v2
>
> History:
> v1 -> v2:
> - much simplified compared to v1 although principle of external modules is
>   kept: removed mechanism of dynamic registration of reset functions
> - list is replaced by whitelist lookup table
> - name of the reset function also stored in the lookup table
> - autoload of reset modules
>
> RFC -> PATCH v1:
> - solution now based on a lookup list instead of specialized driver
>
>
> Eric Auger (4):
>   VFIO: platform: add reset struct and lookup table
>   VFIO: platform: add reset callback
>   VFIO: platform: populate the reset function on probe
>   VFIO: platform: Calxeda xgmac reset module
>
>  drivers/vfio/platform/Kconfig                      |  2 +
>  drivers/vfio/platform/Makefile                     |  2 +
>  drivers/vfio/platform/reset/Kconfig                |  7 ++
>  drivers/vfio/platform/reset/Makefile               |  5 ++
>  .../platform/reset/vfio_platform_calxedaxgmac.c    | 98 ++++++++++++++++++++++
>  drivers/vfio/platform/vfio_platform_common.c       | 75 ++++++++++++++++-
>  drivers/vfio/platform/vfio_platform_private.h      | 14 ++++
>  7 files changed, 200 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/vfio/platform/reset/Kconfig
>  create mode 100644 drivers/vfio/platform/reset/Makefile
>  create mode 100644 drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
>
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 0/4] VFIO platform reset

[Scott Wood]

On Fri, 2015-06-05 at 13:05 -0500, Rob Herring wrote:
> On Fri, Jun 5, 2015 at 10:06 AM, Eric Auger <eric.auger@linaro.org> 
> wrote:
> > In situations where the userspace driver is stopped abnormally and 
> > the
> > VFIO platform device is released, the assigned HW device currently 
> > is
> > left running. As a consequence the HW device might continue 
> > issuing IRQs
> > and performing DMA accesses.
> > 
> > On release, no physical IRQ handler is setup anymore. Also the DMA 
> > buffers
> > are unmapped leading to IOMMU aborts. So there is no serious 
> > consequence.
> > 
> > However when assigning that HW device again to another userspace 
> > driver,
> > this latter might face some unexpected IRQs and DMA accesses, 
> > which are
> > the result of the previous assignment.
> 
> In general, shouldn't it just be a requirement that the drivers 
> handle
> this condition. You have the same problem with firmware/bootloaders
> leaving h/w not in reset state or kexec'ing to a new kernel.

It's not the same situation.  Firmware may leave HW in a non-reset 
state but it must not leave the HW doing DMA; there's nothing the OS 
could do about that as the OS could get corrupted before the driver 
has a chance to run (this is not fun to debug).  Leaving interrupts 
potentially asserted would be bad as well, especially if the interrupt 
is shared.

Likewise, with normal kexec drivers are supposed to quiesce the 
hardware first -- and with kdump, the affected DMA buffers are never 
reused.

In order for the driver to handle this, it would need to reset/quiesce 
the device itself before enabling an IOMMU mapping.  How would that 
work for virtualization scenarios where the guest does not see any 
IOMMU, and all vfio mappings are handled by QEMU or equivalent?

-Scott

[PATCH v2 4/4] VFIO: platform: Calxeda xgmac reset module

[Paul Bolle]

On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
> --- /dev/null
> +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c

> +static int __init vfio_platform_calxedaxgmac_init(void)
> +{
> +	return 0;
> +}
> +
> +static void __exit vfio_platform_calxedaxgmac_exit(void)
> +{
> +}
> +
> +module_init(vfio_platform_calxedaxgmac_init);
> +module_exit(vfio_platform_calxedaxgmac_exit);

Just a nit: I think it's OK to leave out these functions if they both do
nothing. See delete_module in kernel/module.c.


Paul Bolle

[PATCH v2 0/4] VFIO platform reset

[Eric Auger]

Hi Rob, Scott,
On 06/05/2015 11:14 PM, Scott Wood wrote:
> On Fri, 2015-06-05 at 13:05 -0500, Rob Herring wrote:
>> On Fri, Jun 5, 2015 at 10:06 AM, Eric Auger <eric.auger@linaro.org> 
>> wrote:
>>> In situations where the userspace driver is stopped abnormally and 
>>> the
>>> VFIO platform device is released, the assigned HW device currently 
>>> is
>>> left running. As a consequence the HW device might continue 
>>> issuing IRQs
>>> and performing DMA accesses.
>>>
>>> On release, no physical IRQ handler is setup anymore. Also the DMA 
>>> buffers
>>> are unmapped leading to IOMMU aborts. So there is no serious 
>>> consequence.
>>>
>>> However when assigning that HW device again to another userspace 
>>> driver,
>>> this latter might face some unexpected IRQs and DMA accesses, 
>>> which are
>>> the result of the previous assignment.
>>
>> In general, shouldn't it just be a requirement that the drivers 
>> handle
>> this condition. You have the same problem with firmware/bootloaders
>> leaving h/w not in reset state or kexec'ing to a new kernel.
> 
> It's not the same situation.  Firmware may leave HW in a non-reset 
> state but it must not leave the HW doing DMA; there's nothing the OS 
> could do about that as the OS could get corrupted before the driver 
> has a chance to run (this is not fun to debug).  Leaving interrupts 
> potentially asserted would be bad as well, especially if the interrupt 
> is shared.
> 
> Likewise, with normal kexec drivers are supposed to quiesce the 
> hardware first -- and with kdump, the affected DMA buffers are never 
> reused.
> 
> In order for the driver to handle this, it would need to reset/quiesce 
> the device itself before enabling an IOMMU mapping.  How would that 
> work for virtualization scenarios where the guest does not see any 
> IOMMU, and all vfio mappings are handled by QEMU or equivalent?

This is also my understanding. In a KVM virtualization use case, the
guest potentially could be corrupted by previously set DMA accesses
before getting the chance to stop DMA/IRQs.

Thanks for your interest.

Best Regards

Eric
> 
> -Scott
> 

[PATCH v2 4/4] VFIO: platform: Calxeda xgmac reset module

[Eric Auger]

Hi Paul,
On 06/06/2015 02:57 PM, Paul Bolle wrote:
> On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
>> --- /dev/null
>> +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
> 
>> +static int __init vfio_platform_calxedaxgmac_init(void)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void __exit vfio_platform_calxedaxgmac_exit(void)
>> +{
>> +}
>> +
>> +module_init(vfio_platform_calxedaxgmac_init);
>> +module_exit(vfio_platform_calxedaxgmac_exit);
> 
> Just a nit: I think it's OK to leave out these functions if they both do
> nothing. See delete_module in kernel/module.c.
Indeed it works fine without. Thank you for the information!

Best Regards

Eric
> 
> 
> Paul Bolle
> 

[PATCH v2 3/4] VFIO: platform: populate the reset function on probe

[Alex Williamson]

On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
> The reset function lookup happens on vfio-platform probe. The reset
> module load is requested  and a reference to the function symbol is
> hold. The reference is released on vfio-platform remove.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v1 -> v2:
> - [get,put]_reset now is called once on probe/remove
> - use request_module to automatically load the reset module that
>   matches the compatibility string
> - lookup table is used instead of list
> - remove registration mechanism: reset function name is stored in the
>   lookup table.
> - use device_property_read_string instead of
>   device_property_read_string_array
> ---
>  drivers/vfio/platform/vfio_platform_common.c | 48 +++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index 995929b..d474d6a 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -31,6 +31,47 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = {
>  	},
>  };
>  
> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev,
> +				   struct device *dev)
> +{
> +	const char *compat;
> +	const struct vfio_platform_reset_combo *iter = reset_lookup_table;
> +	int (*reset)(struct vfio_platform_device *);
> +	int ret;
> +
> +	vdev->type = VFIO_PLATFORM_RESET_TYPE_MAX;
> +	ret = device_property_read_string(dev, "compatible", &compat);
> +	if (ret)
> +		return ret;
> +
> +	while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
> +		if (!strcmp(iter->compat, compat)) {
> +			request_module(iter->module_name);
> +			reset = __symbol_get(iter->reset_function_name);

symbol_get() appears to be the more robust and dominant interface for
this, why use __symbol_get()? 

> +			if (reset) {
> +				vdev->type = iter->type;
> +				vdev->reset = reset;
> +				return 0;
> +			}
> +		}
> +		iter++;
> +	}
> +	return -1;

-ENODEV seems preferable to -1, but shouldn't this really be a void
function?

> +}
> +
> +static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
> +{
> +	const struct vfio_platform_reset_combo *iter = reset_lookup_table;
> +
> +	while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
> +		if (iter->type == vdev->type) {

Again, I don't see the value in storing the enum, since the table is
static, it could just as easily be the array index and avoid this loop,
but we can avoid it anyway with symbol_put_addr().

> +			__symbol_put(iter->reset_function_name);
> +			return;
> +		}
> +		iter++;
> +	}
> +}
> +
>  static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>  {
>  	int cnt = 0, i;
> @@ -519,6 +560,8 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>  		return ret;
>  	}
>  
> +	vfio_platform_get_reset(vdev, dev);
> +
>  	mutex_init(&vdev->igate);
>  
>  	return 0;
> @@ -530,8 +573,11 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
>  	struct vfio_platform_device *vdev;
>  
>  	vdev = vfio_del_group_dev(dev);
> -	if (vdev)
> +
> +	if (vdev) {
> +		vfio_platform_put_reset(vdev);
>  		iommu_group_put(dev->iommu_group);
> +	}
>  
>  	return vdev;
>  }

[PATCH v2 1/4] VFIO: platform: add reset struct and lookup table

[Alex Williamson]

On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
> This patch introduces the vfio_platform_reset_combo struct that
> stores all the information useful to handle the reset modality:
> compat string, name of the reset function, name of the module that
> implements the reset function. A lookup table of such structures
> is added, currently containing a single sentinel element. A new
> type field is added in vfio_platform_device to store what kind of
> reset is associated to the device, if any.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> ---
>  drivers/vfio/platform/vfio_platform_common.c  |  6 ++++++
>  drivers/vfio/platform/vfio_platform_private.h | 12 ++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index abcff7a..d970776 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -25,6 +25,12 @@
>  
>  static DEFINE_MUTEX(driver_lock);
>  
> +static const struct vfio_platform_reset_combo reset_lookup_table[] = {
> +	{
> +		.type = VFIO_PLATFORM_RESET_TYPE_MAX
> +	},
> +};
> +
>  static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>  {
>  	int cnt = 0, i;
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 5d31e04..d864124 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -49,6 +49,10 @@ struct vfio_platform_region {
>  	void __iomem		*ioaddr;
>  };
>  
> +enum vfio_platform_reset_type {
> +	VFIO_PLATFORM_RESET_TYPE_MAX /* last element */,
> +};
> +
>  struct vfio_platform_device {
>  	struct vfio_platform_region	*regions;
>  	u32				num_regions;
> @@ -56,6 +60,7 @@ struct vfio_platform_device {
>  	u32				num_irqs;
>  	int				refcnt;
>  	struct mutex			igate;
> +	enum vfio_platform_reset_type	type;
>  
>  	/*
>  	 * These fields should be filled by the bus specific binder
> @@ -69,6 +74,13 @@ struct vfio_platform_device {
>  	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
>  };
>  
> +struct vfio_platform_reset_combo {
> +	enum vfio_platform_reset_type type;
> +	char *compat;
> +	char *reset_function_name;
> +	char *module_name;
> +};
> +

I don't really understand the benefit of vfio_platform_reset_type, what
does it add?  If we want an array end marker, we could just use NULL,
but if we're dealing with a static table, we could always use ARRAY_SIZE
and avoid an end marker altogether.  If the concern is matching the
symbol to put when the device is released, we could just use
symbol_put_addr() and avoid any sort of lookup.  Seems like there could
also be a smattering of "const" in this struct definition too.

>  extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>  				      struct device *dev);
>  extern struct vfio_platform_device *vfio_platform_remove_common

[PATCH v2 3/4] VFIO: platform: populate the reset function on probe

[Eric Auger]

On 06/09/2015 08:26 PM, Alex Williamson wrote:
> On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
>> The reset function lookup happens on vfio-platform probe. The reset
>> module load is requested  and a reference to the function symbol is
>> hold. The reference is released on vfio-platform remove.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v1 -> v2:
>> - [get,put]_reset now is called once on probe/remove
>> - use request_module to automatically load the reset module that
>>   matches the compatibility string
>> - lookup table is used instead of list
>> - remove registration mechanism: reset function name is stored in the
>>   lookup table.
>> - use device_property_read_string instead of
>>   device_property_read_string_array
>> ---
>>  drivers/vfio/platform/vfio_platform_common.c | 48 +++++++++++++++++++++++++++-
>>  1 file changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index 995929b..d474d6a 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -31,6 +31,47 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = {
>>  	},
>>  };
>>  
>> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev,
>> +				   struct device *dev)
>> +{
>> +	const char *compat;
>> +	const struct vfio_platform_reset_combo *iter = reset_lookup_table;
>> +	int (*reset)(struct vfio_platform_device *);
>> +	int ret;
>> +
>> +	vdev->type = VFIO_PLATFORM_RESET_TYPE_MAX;
>> +	ret = device_property_read_string(dev, "compatible", &compat);
>> +	if (ret)
>> +		return ret;
>> +
>> +	while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
>> +		if (!strcmp(iter->compat, compat)) {
>> +			request_module(iter->module_name);
>> +			reset = __symbol_get(iter->reset_function_name);
> 
> symbol_get() appears to be the more robust and dominant interface for
> this, why use __symbol_get()? 
I used this because it takes a const char * as an argument and this is
what I use as a datatype for storing the reset function name. Symbol_get
is provided with the symbol directly? It is also used
drivers/mtd/chips/gen_probe.c.
> 
>> +			if (reset) {
>> +				vdev->type = iter->type;
>> +				vdev->reset = reset;
>> +				return 0;
>> +			}
>> +		}
>> +		iter++;
>> +	}
>> +	return -1;
> 
> -ENODEV seems preferable to -1, but shouldn't this really be a void
> function?
yes indeed
> 
>> +}
>> +
>> +static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>> +{
>> +	const struct vfio_platform_reset_combo *iter = reset_lookup_table;
>> +
>> +	while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
>> +		if (iter->type == vdev->type) {
> 
> Again, I don't see the value in storing the enum, since the table is
> static, it could just as easily be the array index and avoid this loop,
> but we can avoid it anyway with symbol_put_addr().
yes you're definitively right!

Thanks

Eric
> 
>> +			__symbol_put(iter->reset_function_name);
>> +			return;
>> +		}
>> +		iter++;
>> +	}
>> +}
>> +
>>  static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>>  {
>>  	int cnt = 0, i;
>> @@ -519,6 +560,8 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>  		return ret;
>>  	}
>>  
>> +	vfio_platform_get_reset(vdev, dev);
>> +
>>  	mutex_init(&vdev->igate);
>>  
>>  	return 0;
>> @@ -530,8 +573,11 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
>>  	struct vfio_platform_device *vdev;
>>  
>>  	vdev = vfio_del_group_dev(dev);
>> -	if (vdev)
>> +
>> +	if (vdev) {
>> +		vfio_platform_put_reset(vdev);
>>  		iommu_group_put(dev->iommu_group);
>> +	}
>>  
>>  	return vdev;
>>  }
> 
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH v2 1/4] VFIO: platform: add reset struct and lookup table

[Eric Auger]

Hi Alex,
On 06/09/2015 08:26 PM, Alex Williamson wrote:
> On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
>> This patch introduces the vfio_platform_reset_combo struct that
>> stores all the information useful to handle the reset modality:
>> compat string, name of the reset function, name of the module that
>> implements the reset function. A lookup table of such structures
>> is added, currently containing a single sentinel element. A new
>> type field is added in vfio_platform_device to store what kind of
>> reset is associated to the device, if any.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>> ---
>>  drivers/vfio/platform/vfio_platform_common.c  |  6 ++++++
>>  drivers/vfio/platform/vfio_platform_private.h | 12 ++++++++++++
>>  2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index abcff7a..d970776 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -25,6 +25,12 @@
>>  
>>  static DEFINE_MUTEX(driver_lock);
>>  
>> +static const struct vfio_platform_reset_combo reset_lookup_table[] = {
>> +	{
>> +		.type = VFIO_PLATFORM_RESET_TYPE_MAX
>> +	},
>> +};
>> +
>>  static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>>  {
>>  	int cnt = 0, i;
>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
>> index 5d31e04..d864124 100644
>> --- a/drivers/vfio/platform/vfio_platform_private.h
>> +++ b/drivers/vfio/platform/vfio_platform_private.h
>> @@ -49,6 +49,10 @@ struct vfio_platform_region {
>>  	void __iomem		*ioaddr;
>>  };
>>  
>> +enum vfio_platform_reset_type {
>> +	VFIO_PLATFORM_RESET_TYPE_MAX /* last element */,
>> +};
>> +
>>  struct vfio_platform_device {
>>  	struct vfio_platform_region	*regions;
>>  	u32				num_regions;
>> @@ -56,6 +60,7 @@ struct vfio_platform_device {
>>  	u32				num_irqs;
>>  	int				refcnt;
>>  	struct mutex			igate;
>> +	enum vfio_platform_reset_type	type;
>>  
>>  	/*
>>  	 * These fields should be filled by the bus specific binder
>> @@ -69,6 +74,13 @@ struct vfio_platform_device {
>>  	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
>>  };
>>  
>> +struct vfio_platform_reset_combo {
>> +	enum vfio_platform_reset_type type;
>> +	char *compat;
>> +	char *reset_function_name;
>> +	char *module_name;
>> +};
>> +
> 
> I don't really understand the benefit of vfio_platform_reset_type, what
> does it add?  If we want an array end marker, we could just use NULL,
> but if we're dealing with a static table, we could always use ARRAY_SIZE
> and avoid an end marker altogether.  If the concern is matching the
> symbol to put when the device is released, we could just use
> symbol_put_addr() and avoid any sort of lookup.  Seems like there could
> also be a smattering of "const" in this struct definition too.

Agreed. I will respin accordingly.

Best Regards

Eric
> 
>>  extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>  				      struct device *dev);
>>  extern struct vfio_platform_device *vfio_platform_remove_common
> 
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH v2 3/4] VFIO: platform: populate the reset function on probe

[Alex Williamson]

On Wed, 2015-06-10 at 13:44 +0200, Eric Auger wrote:
> On 06/09/2015 08:26 PM, Alex Williamson wrote:
> > On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
> >> The reset function lookup happens on vfio-platform probe. The reset
> >> module load is requested  and a reference to the function symbol is
> >> hold. The reference is released on vfio-platform remove.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> >>
> >> ---
> >>
> >> v1 -> v2:
> >> - [get,put]_reset now is called once on probe/remove
> >> - use request_module to automatically load the reset module that
> >>   matches the compatibility string
> >> - lookup table is used instead of list
> >> - remove registration mechanism: reset function name is stored in the
> >>   lookup table.
> >> - use device_property_read_string instead of
> >>   device_property_read_string_array
> >> ---
> >>  drivers/vfio/platform/vfio_platform_common.c | 48 +++++++++++++++++++++++++++-
> >>  1 file changed, 47 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> >> index 995929b..d474d6a 100644
> >> --- a/drivers/vfio/platform/vfio_platform_common.c
> >> +++ b/drivers/vfio/platform/vfio_platform_common.c
> >> @@ -31,6 +31,47 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = {
> >>  	},
> >>  };
> >>  
> >> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev,
> >> +				   struct device *dev)
> >> +{
> >> +	const char *compat;
> >> +	const struct vfio_platform_reset_combo *iter = reset_lookup_table;
> >> +	int (*reset)(struct vfio_platform_device *);
> >> +	int ret;
> >> +
> >> +	vdev->type = VFIO_PLATFORM_RESET_TYPE_MAX;
> >> +	ret = device_property_read_string(dev, "compatible", &compat);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
> >> +		if (!strcmp(iter->compat, compat)) {
> >> +			request_module(iter->module_name);
> >> +			reset = __symbol_get(iter->reset_function_name);
> > 
> > symbol_get() appears to be the more robust and dominant interface for
> > this, why use __symbol_get()? 
> I used this because it takes a const char * as an argument and this is
> what I use as a datatype for storing the reset function name. Symbol_get
> is provided with the symbol directly? It is also used
> drivers/mtd/chips/gen_probe.c.

But symbol_get() is just some macro wrappers around __symbol_get() that
handles tool chains that precede symbols with underscores.  I don't
really know if that's still a concern, but are you sure it doesn't work?

> > 
> >> +			if (reset) {
> >> +				vdev->type = iter->type;
> >> +				vdev->reset = reset;
> >> +				return 0;
> >> +			}
> >> +		}
> >> +		iter++;
> >> +	}
> >> +	return -1;
> > 
> > -ENODEV seems preferable to -1, but shouldn't this really be a void
> > function?
> yes indeed
> > 
> >> +}
> >> +
> >> +static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
> >> +{
> >> +	const struct vfio_platform_reset_combo *iter = reset_lookup_table;
> >> +
> >> +	while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
> >> +		if (iter->type == vdev->type) {
> > 
> > Again, I don't see the value in storing the enum, since the table is
> > static, it could just as easily be the array index and avoid this loop,
> > but we can avoid it anyway with symbol_put_addr().
> yes you're definitively right!
> 
> Thanks
> 
> Eric
> > 
> >> +			__symbol_put(iter->reset_function_name);
> >> +			return;
> >> +		}
> >> +		iter++;
> >> +	}
> >> +}
> >> +
> >>  static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
> >>  {
> >>  	int cnt = 0, i;
> >> @@ -519,6 +560,8 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
> >>  		return ret;
> >>  	}
> >>  
> >> +	vfio_platform_get_reset(vdev, dev);
> >> +
> >>  	mutex_init(&vdev->igate);
> >>  
> >>  	return 0;
> >> @@ -530,8 +573,11 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
> >>  	struct vfio_platform_device *vdev;
> >>  
> >>  	vdev = vfio_del_group_dev(dev);
> >> -	if (vdev)
> >> +
> >> +	if (vdev) {
> >> +		vfio_platform_put_reset(vdev);
> >>  		iommu_group_put(dev->iommu_group);
> >> +	}
> >>  
> >>  	return vdev;
> >>  }
> > 
> > 
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> 

[PATCH v2 3/4] VFIO: platform: populate the reset function on probe

[Eric Auger]

On 06/10/2015 05:10 PM, Alex Williamson wrote:
> On Wed, 2015-06-10 at 13:44 +0200, Eric Auger wrote:
>> On 06/09/2015 08:26 PM, Alex Williamson wrote:
>>> On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
>>>> The reset function lookup happens on vfio-platform probe. The reset
>>>> module load is requested  and a reference to the function symbol is
>>>> hold. The reference is released on vfio-platform remove.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>> - [get,put]_reset now is called once on probe/remove
>>>> - use request_module to automatically load the reset module that
>>>>   matches the compatibility string
>>>> - lookup table is used instead of list
>>>> - remove registration mechanism: reset function name is stored in the
>>>>   lookup table.
>>>> - use device_property_read_string instead of
>>>>   device_property_read_string_array
>>>> ---
>>>>  drivers/vfio/platform/vfio_platform_common.c | 48 +++++++++++++++++++++++++++-
>>>>  1 file changed, 47 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>>>> index 995929b..d474d6a 100644
>>>> --- a/drivers/vfio/platform/vfio_platform_common.c
>>>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>>>> @@ -31,6 +31,47 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = {
>>>>  	},
>>>>  };
>>>>  
>>>> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev,
>>>> +				   struct device *dev)
>>>> +{
>>>> +	const char *compat;
>>>> +	const struct vfio_platform_reset_combo *iter = reset_lookup_table;
>>>> +	int (*reset)(struct vfio_platform_device *);
>>>> +	int ret;
>>>> +
>>>> +	vdev->type = VFIO_PLATFORM_RESET_TYPE_MAX;
>>>> +	ret = device_property_read_string(dev, "compatible", &compat);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
>>>> +		if (!strcmp(iter->compat, compat)) {
>>>> +			request_module(iter->module_name);
>>>> +			reset = __symbol_get(iter->reset_function_name);
>>>
>>> symbol_get() appears to be the more robust and dominant interface for
>>> this, why use __symbol_get()? 
>> I used this because it takes a const char * as an argument and this is
>> what I use as a datatype for storing the reset function name. Symbol_get
>> is provided with the symbol directly? It is also used
>> drivers/mtd/chips/gen_probe.c.
> 
> But symbol_get() is just some macro wrappers around __symbol_get() that
> handles tool chains that precede symbols with underscores.  I don't
> really know if that's still a concern, but are you sure it doesn't work?
> 
>>>
>>>> +			if (reset) {
>>>> +				vdev->type = iter->type;
>>>> +				vdev->reset = reset;
>>>> +				return 0;
>>>> +			}
>>>> +		}
>>>> +		iter++;
>>>> +	}
>>>> +	return -1;
>>>
>>> -ENODEV seems preferable to -1, but shouldn't this really be a void
>>> function?
>> yes indeed
>>>
>>>> +}
>>>> +
>>>> +static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>>>> +{
>>>> +	const struct vfio_platform_reset_combo *iter = reset_lookup_table;
>>>> +
>>>> +	while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
>>>> +		if (iter->type == vdev->type) {
>>>
>>> Again, I don't see the value in storing the enum, since the table is
>>> static, it could just as easily be the array index and avoid this loop,
>>> but we can avoid it anyway with symbol_put_addr().
>> yes you're definitively right!
to be honest I don't remember. I Will try anyway ;-)

Eric
>>
>> Thanks
>>
>> Eric
>>>
>>>> +			__symbol_put(iter->reset_function_name);
>>>> +			return;
>>>> +		}
>>>> +		iter++;
>>>> +	}
>>>> +}
>>>> +
>>>>  static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>>>>  {
>>>>  	int cnt = 0, i;
>>>> @@ -519,6 +560,8 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>>>  		return ret;
>>>>  	}
>>>>  
>>>> +	vfio_platform_get_reset(vdev, dev);
>>>> +
>>>>  	mutex_init(&vdev->igate);
>>>>  
>>>>  	return 0;
>>>> @@ -530,8 +573,11 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
>>>>  	struct vfio_platform_device *vdev;
>>>>  
>>>>  	vdev = vfio_del_group_dev(dev);
>>>> -	if (vdev)
>>>> +
>>>> +	if (vdev) {
>>>> +		vfio_platform_put_reset(vdev);
>>>>  		iommu_group_put(dev->iommu_group);
>>>> +	}
>>>>  
>>>>  	return vdev;
>>>>  }
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
> 
> 
> 

[PATCH v2 3/4] VFIO: platform: populate the reset function on probe

[Eric Auger]

Hi Alex,
On 06/10/2015 05:10 PM, Alex Williamson wrote:
> On Wed, 2015-06-10 at 13:44 +0200, Eric Auger wrote:
>> On 06/09/2015 08:26 PM, Alex Williamson wrote:
>>> On Fri, 2015-06-05 at 17:06 +0200, Eric Auger wrote:
>>>> The reset function lookup happens on vfio-platform probe. The reset
>>>> module load is requested  and a reference to the function symbol is
>>>> hold. The reference is released on vfio-platform remove.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>> - [get,put]_reset now is called once on probe/remove
>>>> - use request_module to automatically load the reset module that
>>>>   matches the compatibility string
>>>> - lookup table is used instead of list
>>>> - remove registration mechanism: reset function name is stored in the
>>>>   lookup table.
>>>> - use device_property_read_string instead of
>>>>   device_property_read_string_array
>>>> ---
>>>>  drivers/vfio/platform/vfio_platform_common.c | 48 +++++++++++++++++++++++++++-
>>>>  1 file changed, 47 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>>>> index 995929b..d474d6a 100644
>>>> --- a/drivers/vfio/platform/vfio_platform_common.c
>>>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>>>> @@ -31,6 +31,47 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = {
>>>>  	},
>>>>  };
>>>>  
>>>> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev,
>>>> +				   struct device *dev)
>>>> +{
>>>> +	const char *compat;
>>>> +	const struct vfio_platform_reset_combo *iter = reset_lookup_table;
>>>> +	int (*reset)(struct vfio_platform_device *);
>>>> +	int ret;
>>>> +
>>>> +	vdev->type = VFIO_PLATFORM_RESET_TYPE_MAX;
>>>> +	ret = device_property_read_string(dev, "compatible", &compat);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
>>>> +		if (!strcmp(iter->compat, compat)) {
>>>> +			request_module(iter->module_name);
>>>> +			reset = __symbol_get(iter->reset_function_name);
>>>
>>> symbol_get() appears to be the more robust and dominant interface for
>>> this, why use __symbol_get()? 
>> I used this because it takes a const char * as an argument and this is
>> what I use as a datatype for storing the reset function name. Symbol_get
>> is provided with the symbol directly? It is also used
>> drivers/mtd/chips/gen_probe.c.
> 
> But symbol_get() is just some macro wrappers around __symbol_get() that
> handles tool chains that precede symbols with underscores.  I don't
> really know if that's still a concern, but are you sure it doesn't work?
I confirm it doesn't work due the (typeof(&x)) which in my case matches
a const char *.

drivers/vfio/platform/vfio_platform_common.c:45:10: warning: assignment
from incompatible pointer type [enabled by default]
    reset = symbol_get(

#define symbol_get(x) ((typeof(&x))(__symbol_get(VMLINUX_SYMBOL_STR(x))))

Best Regards

Eric
> 
>>>
>>>> +			if (reset) {
>>>> +				vdev->type = iter->type;
>>>> +				vdev->reset = reset;
>>>> +				return 0;
>>>> +			}
>>>> +		}
>>>> +		iter++;
>>>> +	}
>>>> +	return -1;
>>>
>>> -ENODEV seems preferable to -1, but shouldn't this really be a void
>>> function?
>> yes indeed
>>>
>>>> +}
>>>> +
>>>> +static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>>>> +{
>>>> +	const struct vfio_platform_reset_combo *iter = reset_lookup_table;
>>>> +
>>>> +	while (iter->type < VFIO_PLATFORM_RESET_TYPE_MAX) {
>>>> +		if (iter->type == vdev->type) {
>>>
>>> Again, I don't see the value in storing the enum, since the table is
>>> static, it could just as easily be the array index and avoid this loop,
>>> but we can avoid it anyway with symbol_put_addr().
>> yes you're definitively right!
>>
>> Thanks
>>
>> Eric
>>>
>>>> +			__symbol_put(iter->reset_function_name);
>>>> +			return;
>>>> +		}
>>>> +		iter++;
>>>> +	}
>>>> +}
>>>> +
>>>>  static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>>>>  {
>>>>  	int cnt = 0, i;
>>>> @@ -519,6 +560,8 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>>>  		return ret;
>>>>  	}
>>>>  
>>>> +	vfio_platform_get_reset(vdev, dev);
>>>> +
>>>>  	mutex_init(&vdev->igate);
>>>>  
>>>>  	return 0;
>>>> @@ -530,8 +573,11 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
>>>>  	struct vfio_platform_device *vdev;
>>>>  
>>>>  	vdev = vfio_del_group_dev(dev);
>>>> -	if (vdev)
>>>> +
>>>> +	if (vdev) {
>>>> +		vfio_platform_put_reset(vdev);
>>>>  		iommu_group_put(dev->iommu_group);
>>>> +	}
>>>>  
>>>>  	return vdev;
>>>>  }
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
> 
> 
>