[PATCH V7 0/9] vfio, platform: add ACPI support

[PATCH V7 0/9] vfio, platform: add ACPI support

[Sinan Kaya]

The current code only supports the device tree based platforms.
The code checks for the presence of a reset driver and calls the reset
function pointer by looking up the reset driver as a module.

ACPI defines _RST method to perform device level reset. After the _RST
method is executed, the OS can resume using the device.

The patchset is moving the device tree specific pieces out of the code
to common functions so that ACPI support is added without impacting the
rest of the code.

During probe, the ACPI HID of the object will be saved and will be used to
determine if this is an ACPI capable platform or not. If acpihid is NULL
then, device tree functions are called.

In addition to plumbing ACPI support, reset functionality is now a
requirement by default.

The code was allowing platform devices to be used without a supporting
VFIO reset driver. The hardware can be left in some inconsistent state
after a guest machine abort.

The reset driver will put the hardware back to safe state and disable
interrupts before returning the control back to the host machine.

Adding a new reset_required kernel module option to AMBA and platform
VFIO drivers with a default value of true.

New requirements are:
1. A reset function needs to be implemented by the corresponding driver
via DT/ACPI.
2. The reset function needs to be discovered via DT/ACPI.

The probe of the driver will fail if any of the above conditions are
not satisfied.


Changes from V6:
1. Clarify when reset method presence is checked vs. when the method is
executed.
2. Clean up the IRQs if reset call fails in open.

Sinan Kaya (9):
  vfio: platform: rename reset function
  vfio: platform: move reset call to a common function
  vfio: platform: determine reset capability
  vfio: platform: add support for ACPI probe
  vfio: platform: add extra debug info argument to call reset
  vfio: platform: call _RST method when using ACPI
  vfio, platform: make reset driver a requirement by default
  vfio: platform: check reset call return code during open
  vfio: platform: check reset call return code during release

 drivers/vfio/platform/vfio_amba.c             |   5 +
 drivers/vfio/platform/vfio_platform.c         |   5 +
 drivers/vfio/platform/vfio_platform_common.c  | 195 +++++++++++++++++++++-----
 drivers/vfio/platform/vfio_platform_private.h |   8 +-
 4 files changed, 176 insertions(+), 37 deletions(-)

-- 
1.8.2.1

[PATCH V7 1/9] vfio: platform: rename reset function

[Sinan Kaya]

Renaming the reset function to of_reset as it is only used
by the device tree based platforms.

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

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index e65b142..08fd7c2 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -41,7 +41,7 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
 		if (!strcmp(iter->compat, compat) &&
 			try_module_get(iter->owner)) {
 			*module = iter->owner;
-			reset_fn = iter->reset;
+			reset_fn = iter->of_reset;
 			break;
 		}
 	}
@@ -51,18 +51,18 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
 
 static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
 {
-	vdev->reset = vfio_platform_lookup_reset(vdev->compat,
-						&vdev->reset_module);
-	if (!vdev->reset) {
+	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
+						    &vdev->reset_module);
+	if (!vdev->of_reset) {
 		request_module("vfio-reset:%s", vdev->compat);
-		vdev->reset = vfio_platform_lookup_reset(vdev->compat,
-							 &vdev->reset_module);
+		vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
+							&vdev->reset_module);
 	}
 }
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
 {
-	if (vdev->reset)
+	if (vdev->of_reset)
 		module_put(vdev->reset_module);
 }
 
@@ -141,9 +141,9 @@ static void vfio_platform_release(void *device_data)
 	mutex_lock(&driver_lock);
 
 	if (!(--vdev->refcnt)) {
-		if (vdev->reset) {
+		if (vdev->of_reset) {
 			dev_info(vdev->device, "reset\n");
-			vdev->reset(vdev);
+			vdev->of_reset(vdev);
 		} else {
 			dev_warn(vdev->device, "no reset function found!\n");
 		}
@@ -175,9 +175,9 @@ static int vfio_platform_open(void *device_data)
 		if (ret)
 			goto err_irq;
 
-		if (vdev->reset) {
+		if (vdev->of_reset) {
 			dev_info(vdev->device, "reset\n");
-			vdev->reset(vdev);
+			vdev->of_reset(vdev);
 		} else {
 			dev_warn(vdev->device, "no reset function found!\n");
 		}
@@ -213,7 +213,7 @@ static long vfio_platform_ioctl(void *device_data,
 		if (info.argsz < minsz)
 			return -EINVAL;
 
-		if (vdev->reset)
+		if (vdev->of_reset)
 			vdev->flags |= VFIO_DEVICE_FLAGS_RESET;
 		info.flags = vdev->flags;
 		info.num_regions = vdev->num_regions;
@@ -312,8 +312,8 @@ static long vfio_platform_ioctl(void *device_data,
 		return ret;
 
 	} else if (cmd == VFIO_DEVICE_RESET) {
-		if (vdev->reset)
-			return vdev->reset(vdev);
+		if (vdev->of_reset)
+			return vdev->of_reset(vdev);
 		else
 			return -EINVAL;
 	}
@@ -611,7 +611,7 @@ void vfio_platform_unregister_reset(const char *compat,
 
 	mutex_lock(&driver_lock);
 	list_for_each_entry_safe(iter, temp, &reset_list, link) {
-		if (!strcmp(iter->compat, compat) && (iter->reset == fn)) {
+		if (!strcmp(iter->compat, compat) && (iter->of_reset == fn)) {
 			list_del(&iter->link);
 			break;
 		}
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 42816dd..71ed7d1 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -71,7 +71,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);
+	int	(*of_reset)(struct vfio_platform_device *vdev);
 };
 
 typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
@@ -80,7 +80,7 @@ struct vfio_platform_reset_node {
 	struct list_head link;
 	char *compat;
 	struct module *owner;
-	vfio_platform_reset_fn_t reset;
+	vfio_platform_reset_fn_t of_reset;
 };
 
 extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
@@ -103,7 +103,7 @@ extern void vfio_platform_unregister_reset(const char *compat,
 static struct vfio_platform_reset_node __reset ## _node = {	\
 	.owner = THIS_MODULE,					\
 	.compat = __compat,					\
-	.reset = __reset,					\
+	.of_reset = __reset,					\
 };								\
 __vfio_platform_register_reset(&__reset ## _node)
 
-- 
1.8.2.1

[PATCH V7 2/9] vfio: platform: move reset call to a common function

[Sinan Kaya]

The reset call sequence seems to replicate itself multiple times
across the file. Grouping them together for maintenance reasons.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Eric Auger <eric.auger@linaro.org>
---
 drivers/vfio/platform/vfio_platform_common.c | 30 +++++++++++++---------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 08fd7c2..3e2a7c0 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -134,6 +134,17 @@ static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
 	kfree(vdev->regions);
 }
 
+static int vfio_platform_call_reset(struct vfio_platform_device *vdev)
+{
+	if (vdev->of_reset) {
+		dev_info(vdev->device, "reset\n");
+		return vdev->of_reset(vdev);
+	}
+
+	dev_warn(vdev->device, "no reset function found!\n");
+	return -EINVAL;
+}
+
 static void vfio_platform_release(void *device_data)
 {
 	struct vfio_platform_device *vdev = device_data;
@@ -141,12 +152,7 @@ static void vfio_platform_release(void *device_data)
 	mutex_lock(&driver_lock);
 
 	if (!(--vdev->refcnt)) {
-		if (vdev->of_reset) {
-			dev_info(vdev->device, "reset\n");
-			vdev->of_reset(vdev);
-		} else {
-			dev_warn(vdev->device, "no reset function found!\n");
-		}
+		vfio_platform_call_reset(vdev);
 		vfio_platform_regions_cleanup(vdev);
 		vfio_platform_irq_cleanup(vdev);
 	}
@@ -175,12 +181,7 @@ static int vfio_platform_open(void *device_data)
 		if (ret)
 			goto err_irq;
 
-		if (vdev->of_reset) {
-			dev_info(vdev->device, "reset\n");
-			vdev->of_reset(vdev);
-		} else {
-			dev_warn(vdev->device, "no reset function found!\n");
-		}
+		vfio_platform_call_reset(vdev);
 	}
 
 	vdev->refcnt++;
@@ -312,10 +313,7 @@ static long vfio_platform_ioctl(void *device_data,
 		return ret;
 
 	} else if (cmd == VFIO_DEVICE_RESET) {
-		if (vdev->of_reset)
-			return vdev->of_reset(vdev);
-		else
-			return -EINVAL;
+		return vfio_platform_call_reset(vdev);
 	}
 
 	return -ENOTTY;
-- 
1.8.2.1

[PATCH V7 3/9] vfio: platform: determine reset capability

[Sinan Kaya]

Creating a new function to determine if this driver supports reset
function or not. This is an attempt to abstract device tree calls
from the rest of the code.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Eric Auger <eric.auger@linaro.org>
---
 drivers/vfio/platform/vfio_platform_common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 3e2a7c0..6be92c3 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -49,6 +49,11 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
 	return reset_fn;
 }
 
+static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
+{
+	return vdev->of_reset ? true : false;
+}
+
 static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
 {
 	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
@@ -214,7 +219,7 @@ static long vfio_platform_ioctl(void *device_data,
 		if (info.argsz < minsz)
 			return -EINVAL;
 
-		if (vdev->of_reset)
+		if (vfio_platform_has_reset(vdev))
 			vdev->flags |= VFIO_DEVICE_FLAGS_RESET;
 		info.flags = vdev->flags;
 		info.num_regions = vdev->num_regions;
-- 
1.8.2.1

[PATCH V7 4/9] vfio: platform: add support for ACPI probe

[Sinan Kaya]

The code is using the compatible DT string to associate a reset driver
with the actual device itself. The compatible string does not exist on
ACPI based systems. HID is the unique identifier for a device driver
instead.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 drivers/vfio/platform/vfio_platform_common.c  | 57 ++++++++++++++++++++++++---
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 2 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 6be92c3..fbf4565 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/acpi.h>
 #include <linux/iommu.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -49,6 +50,37 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
 	return reset_fn;
 }
 
+#ifdef CONFIG_ACPI
+static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
+				    struct device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	if (!adev) {
+		pr_err("VFIO: ACPI companion device not found for %s\n",
+			vdev->name);
+		return -ENODEV;
+	}
+
+	vdev->acpihid = acpi_device_hid(adev);
+	if (!vdev->acpihid) {
+		pr_err("VFIO: cannot find ACPI HID for %s\n",
+		       vdev->name);
+		return -ENODEV;
+	}
+	return 0;
+}
+#else
+static inline int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
+					   struct device *dev)
+{
+	return -ENOENT;
+}
+#endif
+
 static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
 {
 	return vdev->of_reset ? true : false;
@@ -547,6 +579,20 @@ static const struct vfio_device_ops vfio_platform_ops = {
 	.mmap		= vfio_platform_mmap,
 };
 
+int vfio_platform_of_probe(struct vfio_platform_device *vdev,
+			   struct device *dev)
+{
+	int ret;
+
+	ret = device_property_read_string(dev, "compatible",
+					  &vdev->compat);
+	if (ret)
+		pr_err("VFIO: cannot retrieve compat for %s\n",
+			vdev->name);
+
+	return ret;
+}
+
 int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 			       struct device *dev)
 {
@@ -556,11 +602,12 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 	if (!vdev)
 		return -EINVAL;
 
-	ret = device_property_read_string(dev, "compatible", &vdev->compat);
-	if (ret) {
-		pr_err("VFIO: cannot retrieve compat for %s\n", vdev->name);
-		return -EINVAL;
-	}
+	ret = vfio_platform_acpi_probe(vdev, dev);
+	if (ret)
+		ret = vfio_platform_of_probe(vdev, dev);
+
+	if (ret)
+		return ret;
 
 	vdev->device = dev;
 
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 71ed7d1..ba9e4f8 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -58,6 +58,7 @@ struct vfio_platform_device {
 	struct mutex			igate;
 	struct module			*parent_module;
 	const char			*compat;
+	const char			*acpihid;
 	struct module			*reset_module;
 	struct device			*device;
 
-- 
1.8.2.1

[PATCH V7 5/9] vfio: platform: add extra debug info argument to call reset

[Sinan Kaya]

Getting ready to bring out extra debug information to the caller
so that more verbose information can be printed when an error is
observed.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/vfio/platform/vfio_platform_common.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index fbf4565..e7ce2c2 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -171,7 +171,8 @@ static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
 	kfree(vdev->regions);
 }
 
-static int vfio_platform_call_reset(struct vfio_platform_device *vdev)
+static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
+				    const char **extra_dbg)
 {
 	if (vdev->of_reset) {
 		dev_info(vdev->device, "reset\n");
@@ -189,7 +190,7 @@ static void vfio_platform_release(void *device_data)
 	mutex_lock(&driver_lock);
 
 	if (!(--vdev->refcnt)) {
-		vfio_platform_call_reset(vdev);
+		vfio_platform_call_reset(vdev, NULL);
 		vfio_platform_regions_cleanup(vdev);
 		vfio_platform_irq_cleanup(vdev);
 	}
@@ -218,7 +219,7 @@ static int vfio_platform_open(void *device_data)
 		if (ret)
 			goto err_irq;
 
-		vfio_platform_call_reset(vdev);
+		vfio_platform_call_reset(vdev, NULL);
 	}
 
 	vdev->refcnt++;
@@ -350,7 +351,7 @@ static long vfio_platform_ioctl(void *device_data,
 		return ret;
 
 	} else if (cmd == VFIO_DEVICE_RESET) {
-		return vfio_platform_call_reset(vdev);
+		return vfio_platform_call_reset(vdev, NULL);
 	}
 
 	return -ENOTTY;
-- 
1.8.2.1

[PATCH V7 6/9] vfio: platform: call _RST method when using ACPI

[Sinan Kaya]

The device tree code checks for the presence of a reset driver and calls
the of_reset function pointer by looking up the reset driver as a module.

ACPI defines _RST method to perform device level reset. After the _RST
method is executed, the OS can resume using the device. _RST method is
expected to stop DMA transfers and IRQs.

This patch introduces two functions as vfio_platform_acpi_has_reset and
vfio_platform_acpi_call_reset. The has reset method is used to declare
reset capability via the ioctl flag VFIO_DEVICE_FLAGS_RESET. The call
reset function is used to execute the _RST ACPI method.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Eric Auger <eric.auger@linaro.org>
---
 drivers/vfio/platform/vfio_platform_common.c | 51 ++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index e7ce2c2..0ea8c26 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -73,21 +73,66 @@ static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
 	}
 	return 0;
 }
+
+static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev,
+					 const char **extra_dbg)
+{
+	struct device *dev = vdev->device;
+	acpi_handle handle = ACPI_HANDLE(dev);
+	acpi_status acpi_ret;
+	unsigned long long val;
+
+	acpi_ret = acpi_evaluate_integer(handle, "_RST", NULL, &val);
+	if (ACPI_FAILURE(acpi_ret)) {
+		if (extra_dbg)
+			*extra_dbg = acpi_format_exception(acpi_ret);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static bool vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
+{
+	struct device *dev = vdev->device;
+	acpi_handle handle = ACPI_HANDLE(dev);
+
+	return acpi_has_method(handle, "_RST");
+}
 #else
 static inline int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
 					   struct device *dev)
 {
 	return -ENOENT;
 }
+
+static inline
+int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev,
+				  const char **extra_dbg)
+{
+	return -ENOENT;
+}
+
+static inline
+bool vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
+{
+	return false;
+}
 #endif
 
 static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
 {
+	if (vdev->acpihid)
+		return vfio_platform_acpi_has_reset(vdev);
+
 	return vdev->of_reset ? true : false;
 }
 
 static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
 {
+	if (vdev->acpihid)
+		return;
+
 	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
 						    &vdev->reset_module);
 	if (!vdev->of_reset) {
@@ -99,6 +144,9 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
 {
+	if (vdev->acpihid)
+		return;
+
 	if (vdev->of_reset)
 		module_put(vdev->reset_module);
 }
@@ -177,6 +225,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
 	if (vdev->of_reset) {
 		dev_info(vdev->device, "reset\n");
 		return vdev->of_reset(vdev);
+	} else if (vdev->acpihid) {
+		dev_info(vdev->device, "reset\n");
+		return vfio_platform_acpi_call_reset(vdev, extra_dbg);
 	}
 
 	dev_warn(vdev->device, "no reset function found!\n");
-- 
1.8.2.1

[PATCH V7 6/9] vfio: platform: call _RST method when using ACPI

[Alex Williamson]

On Mon, 13 Jun 2016 00:26:38 -0400
Sinan Kaya <okaya@codeaurora.org> wrote:

> The device tree code checks for the presence of a reset driver and calls
> the of_reset function pointer by looking up the reset driver as a module.
> 
> ACPI defines _RST method to perform device level reset. After the _RST
> method is executed, the OS can resume using the device. _RST method is
> expected to stop DMA transfers and IRQs.
> 
> This patch introduces two functions as vfio_platform_acpi_has_reset and
> vfio_platform_acpi_call_reset. The has reset method is used to declare
> reset capability via the ioctl flag VFIO_DEVICE_FLAGS_RESET. The call
> reset function is used to execute the _RST ACPI method.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> Reviewed-by: Eric Auger <eric.auger@linaro.org>
> ---
>  drivers/vfio/platform/vfio_platform_common.c | 51 ++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index e7ce2c2..0ea8c26 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -73,21 +73,66 @@ static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
>  	}
>  	return 0;
>  }
> +
> +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev,
> +					 const char **extra_dbg)
> +{
> +	struct device *dev = vdev->device;
> +	acpi_handle handle = ACPI_HANDLE(dev);
> +	acpi_status acpi_ret;
> +	unsigned long long val;
> +
> +	acpi_ret = acpi_evaluate_integer(handle, "_RST", NULL, &val);
> +	if (ACPI_FAILURE(acpi_ret)) {
> +		if (extra_dbg)
> +			*extra_dbg = acpi_format_exception(acpi_ret);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static bool vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
> +{
> +	struct device *dev = vdev->device;
> +	acpi_handle handle = ACPI_HANDLE(dev);
> +
> +	return acpi_has_method(handle, "_RST");
> +}
>  #else
>  static inline int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
>  					   struct device *dev)
>  {
>  	return -ENOENT;
>  }
> +
> +static inline
> +int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev,
> +				  const char **extra_dbg)
> +{
> +	return -ENOENT;
> +}
> +
> +static inline
> +bool vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
> +{
> +	return false;
> +}
>  #endif
>  
>  static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
>  {
> +	if (vdev->acpihid)
> +		return vfio_platform_acpi_has_reset(vdev);
> +
>  	return vdev->of_reset ? true : false;
>  }
>  
>  static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
>  {
> +	if (vdev->acpihid)
> +		return;
> +
>  	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>  						    &vdev->reset_module);
>  	if (!vdev->of_reset) {
> @@ -99,6 +144,9 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
>  
>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>  {
> +	if (vdev->acpihid)
> +		return;
> +
>  	if (vdev->of_reset)
>  		module_put(vdev->reset_module);
>  }
> @@ -177,6 +225,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
>  	if (vdev->of_reset) {
>  		dev_info(vdev->device, "reset\n");
>  		return vdev->of_reset(vdev);
> +	} else if (vdev->acpihid) {
> +		dev_info(vdev->device, "reset\n");
> +		return vfio_platform_acpi_call_reset(vdev, extra_dbg);


nit, all the cases until this one prioritize ACPI over DT, here we have
the opposite.  Granted we should never have of_reset set when acpihid
is set.  A macro like

#define vfio_platform_is_acpi(vdev) (vdev->acpihid != NULL)

might be useful.


>  	}
>  
>  	dev_warn(vdev->device, "no reset function found!\n");

[PATCH V7 6/9] vfio: platform: call _RST method when using ACPI

[Alex Williamson]

On Thu, 23 Jun 2016 12:59:48 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Mon, 13 Jun 2016 00:26:38 -0400
> Sinan Kaya <okaya@codeaurora.org> wrote:
> 
> > The device tree code checks for the presence of a reset driver and calls
> > the of_reset function pointer by looking up the reset driver as a module.
> > 
> > ACPI defines _RST method to perform device level reset. After the _RST
> > method is executed, the OS can resume using the device. _RST method is
> > expected to stop DMA transfers and IRQs.
> > 
> > This patch introduces two functions as vfio_platform_acpi_has_reset and
> > vfio_platform_acpi_call_reset. The has reset method is used to declare
> > reset capability via the ioctl flag VFIO_DEVICE_FLAGS_RESET. The call
> > reset function is used to execute the _RST ACPI method.
> > 
> > Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> > Reviewed-by: Eric Auger <eric.auger@linaro.org>
> > ---
> >  drivers/vfio/platform/vfio_platform_common.c | 51 ++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> > 
> > diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> > index e7ce2c2..0ea8c26 100644
> > --- a/drivers/vfio/platform/vfio_platform_common.c
> > +++ b/drivers/vfio/platform/vfio_platform_common.c
> > @@ -73,21 +73,66 @@ static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> >  	}
> >  	return 0;
> >  }
> > +
> > +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev,
> > +					 const char **extra_dbg)
> > +{
> > +	struct device *dev = vdev->device;
> > +	acpi_handle handle = ACPI_HANDLE(dev);
> > +	acpi_status acpi_ret;
> > +	unsigned long long val;
> > +
> > +	acpi_ret = acpi_evaluate_integer(handle, "_RST", NULL, &val);
> > +	if (ACPI_FAILURE(acpi_ret)) {
> > +		if (extra_dbg)
> > +			*extra_dbg = acpi_format_exception(acpi_ret);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static bool vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
> > +{
> > +	struct device *dev = vdev->device;
> > +	acpi_handle handle = ACPI_HANDLE(dev);
> > +
> > +	return acpi_has_method(handle, "_RST");
> > +}
> >  #else
> >  static inline int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> >  					   struct device *dev)
> >  {
> >  	return -ENOENT;
> >  }
> > +
> > +static inline
> > +int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev,
> > +				  const char **extra_dbg)
> > +{
> > +	return -ENOENT;
> > +}
> > +
> > +static inline
> > +bool vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
> > +{
> > +	return false;
> > +}
> >  #endif
> >  
> >  static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
> >  {
> > +	if (vdev->acpihid)
> > +		return vfio_platform_acpi_has_reset(vdev);
> > +
> >  	return vdev->of_reset ? true : false;
> >  }
> >  
> >  static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
> >  {
> > +	if (vdev->acpihid)
> > +		return;
> > +
> >  	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
> >  						    &vdev->reset_module);
> >  	if (!vdev->of_reset) {
> > @@ -99,6 +144,9 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
> >  
> >  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
> >  {
> > +	if (vdev->acpihid)
> > +		return;
> > +
> >  	if (vdev->of_reset)
> >  		module_put(vdev->reset_module);
> >  }
> > @@ -177,6 +225,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
> >  	if (vdev->of_reset) {
> >  		dev_info(vdev->device, "reset\n");
> >  		return vdev->of_reset(vdev);
> > +	} else if (vdev->acpihid) {
> > +		dev_info(vdev->device, "reset\n");
> > +		return vfio_platform_acpi_call_reset(vdev, extra_dbg);  
> 
> 
> nit, all the cases until this one prioritize ACPI over DT, here we have
> the opposite.  Granted we should never have of_reset set when acpihid
> is set.  A macro like
> 
> #define vfio_platform_is_acpi(vdev) (vdev->acpihid != NULL)
> 
> might be useful.

Oops, I accidentally picked the v7 version to reply to, but the nit
exists in v8 as well.  Thanks,

Alex

[PATCH V7 6/9] vfio: platform: call _RST method when using ACPI

[Sinan Kaya]

On 6/23/2016 3:07 PM, Alex Williamson wrote:
>> might be useful.
> Oops, I accidentally picked the v7 version to reply to, but the nit
> exists in v8 as well.  Thanks,

No problem. I'll work on your comments and post a follow up. 

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V7 6/9] vfio: platform: call _RST method when using ACPI

[Sinan Kaya]

On 6/23/2016 2:59 PM, Alex Williamson wrote:
> On Mon, 13 Jun 2016 00:26:38 -0400
> Sinan Kaya <okaya@codeaurora.org> wrote:
> 
>> The device tree code checks for the presence of a reset driver and calls
>> the of_reset function pointer by looking up the reset driver as a module.
>>
>> ACPI defines _RST method to perform device level reset. After the _RST
>> method is executed, the OS can resume using the device. _RST method is
>> expected to stop DMA transfers and IRQs.
>>
>> This patch introduces two functions as vfio_platform_acpi_has_reset and
>> vfio_platform_acpi_call_reset. The has reset method is used to declare
>> reset capability via the ioctl flag VFIO_DEVICE_FLAGS_RESET. The call
>> reset function is used to execute the _RST ACPI method.
>>
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> Reviewed-by: Eric Auger <eric.auger@linaro.org>
>> ---
>>  drivers/vfio/platform/vfio_platform_common.c | 51 ++++++++++++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index e7ce2c2..0ea8c26 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -73,21 +73,66 @@ static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
>>  	}
>>  	return 0;
>>  }
>> +
>> +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev,
>> +					 const char **extra_dbg)
>> +{
>> +	struct device *dev = vdev->device;
>> +	acpi_handle handle = ACPI_HANDLE(dev);
>> +	acpi_status acpi_ret;
>> +	unsigned long long val;
>> +
>> +	acpi_ret = acpi_evaluate_integer(handle, "_RST", NULL, &val);
>> +	if (ACPI_FAILURE(acpi_ret)) {
>> +		if (extra_dbg)
>> +			*extra_dbg = acpi_format_exception(acpi_ret);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static bool vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
>> +{
>> +	struct device *dev = vdev->device;
>> +	acpi_handle handle = ACPI_HANDLE(dev);
>> +
>> +	return acpi_has_method(handle, "_RST");
>> +}
>>  #else
>>  static inline int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
>>  					   struct device *dev)
>>  {
>>  	return -ENOENT;
>>  }
>> +
>> +static inline
>> +int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev,
>> +				  const char **extra_dbg)
>> +{
>> +	return -ENOENT;
>> +}
>> +
>> +static inline
>> +bool vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
>> +{
>> +	return false;
>> +}
>>  #endif
>>  
>>  static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
>>  {
>> +	if (vdev->acpihid)
>> +		return vfio_platform_acpi_has_reset(vdev);
>> +
>>  	return vdev->of_reset ? true : false;
>>  }
>>  
>>  static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
>>  {
>> +	if (vdev->acpihid)
>> +		return;
>> +
>>  	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>>  						    &vdev->reset_module);
>>  	if (!vdev->of_reset) {
>> @@ -99,6 +144,9 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
>>  
>>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>>  {
>> +	if (vdev->acpihid)
>> +		return;
>> +
>>  	if (vdev->of_reset)
>>  		module_put(vdev->reset_module);
>>  }
>> @@ -177,6 +225,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
>>  	if (vdev->of_reset) {
>>  		dev_info(vdev->device, "reset\n");
>>  		return vdev->of_reset(vdev);
>> +	} else if (vdev->acpihid) {
>> +		dev_info(vdev->device, "reset\n");
>> +		return vfio_platform_acpi_call_reset(vdev, extra_dbg);
> 
> 
> nit, all the cases until this one prioritize ACPI over DT, here we have
> the opposite. 

I'll reorder to be consistent.

> Granted we should never have of_reset set when acpihid
> is set.  A macro like
> 
> #define vfio_platform_is_acpi(vdev) (vdev->acpihid != NULL)
> 
> might be useful.

I like your idea. If you don't mind, I like capitals in macros. 

> 
> 
>>  	}
>>  
>>  	dev_warn(vdev->device, "no reset function found!\n");
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V7 6/9] vfio: platform: call _RST method when using ACPI

[Alex Williamson]

On Wed, 13 Jul 2016 15:39:25 -0400
Sinan Kaya <okaya@codeaurora.org> wrote:

> On 6/23/2016 2:59 PM, Alex Williamson wrote:
> > On Mon, 13 Jun 2016 00:26:38 -0400
> > Sinan Kaya <okaya@codeaurora.org> wrote:
> >   
> >> The device tree code checks for the presence of a reset driver and calls
> >> the of_reset function pointer by looking up the reset driver as a module.
> >>
> >> ACPI defines _RST method to perform device level reset. After the _RST
> >> method is executed, the OS can resume using the device. _RST method is
> >> expected to stop DMA transfers and IRQs.
> >>
> >> This patch introduces two functions as vfio_platform_acpi_has_reset and
> >> vfio_platform_acpi_call_reset. The has reset method is used to declare
> >> reset capability via the ioctl flag VFIO_DEVICE_FLAGS_RESET. The call
> >> reset function is used to execute the _RST ACPI method.
> >>
> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> >> Reviewed-by: Eric Auger <eric.auger@linaro.org>
> >> ---
> >>  drivers/vfio/platform/vfio_platform_common.c | 51 ++++++++++++++++++++++++++++
> >>  1 file changed, 51 insertions(+)
> >>
> >> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> >> index e7ce2c2..0ea8c26 100644
> >> --- a/drivers/vfio/platform/vfio_platform_common.c
> >> +++ b/drivers/vfio/platform/vfio_platform_common.c
> >> @@ -73,21 +73,66 @@ static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> >>  	}
> >>  	return 0;
> >>  }
> >> +
> >> +static int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev,
> >> +					 const char **extra_dbg)
> >> +{
> >> +	struct device *dev = vdev->device;
> >> +	acpi_handle handle = ACPI_HANDLE(dev);
> >> +	acpi_status acpi_ret;
> >> +	unsigned long long val;
> >> +
> >> +	acpi_ret = acpi_evaluate_integer(handle, "_RST", NULL, &val);
> >> +	if (ACPI_FAILURE(acpi_ret)) {
> >> +		if (extra_dbg)
> >> +			*extra_dbg = acpi_format_exception(acpi_ret);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static bool vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
> >> +{
> >> +	struct device *dev = vdev->device;
> >> +	acpi_handle handle = ACPI_HANDLE(dev);
> >> +
> >> +	return acpi_has_method(handle, "_RST");
> >> +}
> >>  #else
> >>  static inline int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
> >>  					   struct device *dev)
> >>  {
> >>  	return -ENOENT;
> >>  }
> >> +
> >> +static inline
> >> +int vfio_platform_acpi_call_reset(struct vfio_platform_device *vdev,
> >> +				  const char **extra_dbg)
> >> +{
> >> +	return -ENOENT;
> >> +}
> >> +
> >> +static inline
> >> +bool vfio_platform_acpi_has_reset(struct vfio_platform_device *vdev)
> >> +{
> >> +	return false;
> >> +}
> >>  #endif
> >>  
> >>  static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
> >>  {
> >> +	if (vdev->acpihid)
> >> +		return vfio_platform_acpi_has_reset(vdev);
> >> +
> >>  	return vdev->of_reset ? true : false;
> >>  }
> >>  
> >>  static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
> >>  {
> >> +	if (vdev->acpihid)
> >> +		return;
> >> +
> >>  	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
> >>  						    &vdev->reset_module);
> >>  	if (!vdev->of_reset) {
> >> @@ -99,6 +144,9 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
> >>  
> >>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
> >>  {
> >> +	if (vdev->acpihid)
> >> +		return;
> >> +
> >>  	if (vdev->of_reset)
> >>  		module_put(vdev->reset_module);
> >>  }
> >> @@ -177,6 +225,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
> >>  	if (vdev->of_reset) {
> >>  		dev_info(vdev->device, "reset\n");
> >>  		return vdev->of_reset(vdev);
> >> +	} else if (vdev->acpihid) {
> >> +		dev_info(vdev->device, "reset\n");
> >> +		return vfio_platform_acpi_call_reset(vdev, extra_dbg);  
> > 
> > 
> > nit, all the cases until this one prioritize ACPI over DT, here we have
> > the opposite.   
> 
> I'll reorder to be consistent.
> 
> > Granted we should never have of_reset set when acpihid
> > is set.  A macro like
> > 
> > #define vfio_platform_is_acpi(vdev) (vdev->acpihid != NULL)
> > 
> > might be useful.  
> 
> I like your idea. If you don't mind, I like capitals in macros. 

Sure.  Thanks,

Alex

[PATCH V7 7/9] vfio, platform: make reset driver a requirement by default

[Sinan Kaya]

The code was allowing platform devices to be used without a supporting
VFIO reset driver. The hardware can be left in some inconsistent state
after a guest machine abort.

The reset driver will put the hardware back to safe state and disable
interrupts before returning the control back to the host machine.

Adding a new reset_required kernel module option to AMBA and platform
VFIO drivers with a default value of true.

New requirements are:
1. A reset function needs to be implemented by the corresponding driver
via DT/ACPI.
2. The reset function needs to be discovered via DT/ACPI.

The probe of the driver will fail if any of the above conditions are
not satisfied.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 drivers/vfio/platform/vfio_amba.c             |  5 +++++
 drivers/vfio/platform/vfio_platform.c         |  5 +++++
 drivers/vfio/platform/vfio_platform_common.c  | 22 +++++++++++++++-------
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 4 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c
index a66479b..7585902 100644
--- a/drivers/vfio/platform/vfio_amba.c
+++ b/drivers/vfio/platform/vfio_amba.c
@@ -23,6 +23,10 @@
 #define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
 #define DRIVER_DESC     "VFIO for AMBA devices - User Level meta-driver"
 
+static bool reset_required = true;
+module_param(reset_required, bool, 0644);
+MODULE_PARM_DESC(reset_required, "override reset requirement (default: 1)");
+
 /* probing devices from the AMBA bus */
 
 static struct resource *get_amba_resource(struct vfio_platform_device *vdev,
@@ -68,6 +72,7 @@ static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id)
 	vdev->get_resource = get_amba_resource;
 	vdev->get_irq = get_amba_irq;
 	vdev->parent_module = THIS_MODULE;
+	vdev->reset_required = reset_required;
 
 	ret = vfio_platform_probe_common(vdev, &adev->dev);
 	if (ret) {
diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index b1cc3a7..ef89146 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -23,6 +23,10 @@
 #define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
 #define DRIVER_DESC     "VFIO for platform devices - User Level meta-driver"
 
+static bool reset_required = true;
+module_param(reset_required, bool, 0644);
+MODULE_PARM_DESC(reset_required, "override reset requirement (default: 1)");
+
 /* probing devices from the linux platform bus */
 
 static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
@@ -66,6 +70,7 @@ static int vfio_platform_probe(struct platform_device *pdev)
 	vdev->get_resource = get_platform_resource;
 	vdev->get_irq = get_platform_irq;
 	vdev->parent_module = THIS_MODULE;
+	vdev->reset_required = reset_required;
 
 	ret = vfio_platform_probe_common(vdev, &pdev->dev);
 	if (ret)
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 0ea8c26..d84c399 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -128,10 +128,10 @@ static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
 	return vdev->of_reset ? true : false;
 }
 
-static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
+static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
 {
 	if (vdev->acpihid)
-		return;
+		return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
 
 	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
 						    &vdev->reset_module);
@@ -140,6 +140,8 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
 		vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
 							&vdev->reset_module);
 	}
+
+	return vdev->of_reset ? 0 : -ENOENT;
 }
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
@@ -670,16 +672,22 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 	}
 
 	ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev);
-	if (ret) {
-		iommu_group_put(group);
-		return ret;
-	}
+	if (ret)
+		goto out;
 
-	vfio_platform_get_reset(vdev);
+	ret = vfio_platform_get_reset(vdev);
+	if (ret && vdev->reset_required) {
+		pr_err("vfio: no reset function found for device %s\n",
+		       vdev->name);
+		goto out;
+	}
 
 	mutex_init(&vdev->igate);
 
 	return 0;
+out:
+	iommu_group_put(group);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(vfio_platform_probe_common);
 
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index ba9e4f8..68fbc00 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 {
 };
 
 struct vfio_platform_device {
+	bool				reset_required;
 	struct vfio_platform_region	*regions;
 	u32				num_regions;
 	struct vfio_platform_irq	*irqs;
-- 
1.8.2.1

[PATCH V7 7/9] vfio, platform: make reset driver a requirement by default

[Auger Eric]

Hi Sinan,

On 13/06/2016 06:26, Sinan Kaya wrote:
> The code was allowing platform devices to be used without a supporting
> VFIO reset driver. The hardware can be left in some inconsistent state
> after a guest machine abort.
> 
> The reset driver will put the hardware back to safe state and disable
> interrupts before returning the control back to the host machine.
> 
> Adding a new reset_required kernel module option to AMBA and platform
> VFIO drivers with a default value of true.
> 
> New requirements are:
> 1. A reset function needs to be implemented by the corresponding driver
> via DT/ACPI.
> 2. The reset function needs to be discovered via DT/ACPI.
> 
> The probe of the driver will fail if any of the above conditions are
> not satisfied.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> ---
>  drivers/vfio/platform/vfio_amba.c             |  5 +++++
>  drivers/vfio/platform/vfio_platform.c         |  5 +++++
>  drivers/vfio/platform/vfio_platform_common.c  | 22 +++++++++++++++-------
>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>  4 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c
> index a66479b..7585902 100644
> --- a/drivers/vfio/platform/vfio_amba.c
> +++ b/drivers/vfio/platform/vfio_amba.c
> @@ -23,6 +23,10 @@
>  #define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
>  #define DRIVER_DESC     "VFIO for AMBA devices - User Level meta-driver"
>  
> +static bool reset_required = true;
> +module_param(reset_required, bool, 0644);
> +MODULE_PARM_DESC(reset_required, "override reset requirement (default: 1)");
> +
>  /* probing devices from the AMBA bus */
>  
>  static struct resource *get_amba_resource(struct vfio_platform_device *vdev,
> @@ -68,6 +72,7 @@ static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id)
>  	vdev->get_resource = get_amba_resource;
>  	vdev->get_irq = get_amba_irq;
>  	vdev->parent_module = THIS_MODULE;
> +	vdev->reset_required = reset_required;
>  
>  	ret = vfio_platform_probe_common(vdev, &adev->dev);
>  	if (ret) {
> diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
> index b1cc3a7..ef89146 100644
> --- a/drivers/vfio/platform/vfio_platform.c
> +++ b/drivers/vfio/platform/vfio_platform.c
> @@ -23,6 +23,10 @@
>  #define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
>  #define DRIVER_DESC     "VFIO for platform devices - User Level meta-driver"
>  
> +static bool reset_required = true;
> +module_param(reset_required, bool, 0644);
> +MODULE_PARM_DESC(reset_required, "override reset requirement (default: 1)");
> +
>  /* probing devices from the linux platform bus */
>  
>  static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
> @@ -66,6 +70,7 @@ static int vfio_platform_probe(struct platform_device *pdev)
>  	vdev->get_resource = get_platform_resource;
>  	vdev->get_irq = get_platform_irq;
>  	vdev->parent_module = THIS_MODULE;
> +	vdev->reset_required = reset_required;
>  
>  	ret = vfio_platform_probe_common(vdev, &pdev->dev);
>  	if (ret)
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index 0ea8c26..d84c399 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -128,10 +128,10 @@ static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
>  	return vdev->of_reset ? true : false;
>  }
>  
> -static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>  {
>  	if (vdev->acpihid)
> -		return;
> +		return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
>  
>  	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>  						    &vdev->reset_module);
> @@ -140,6 +140,8 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
>  		vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>  							&vdev->reset_module);
>  	}
> +
> +	return vdev->of_reset ? 0 : -ENOENT;
>  }
>  
>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
> @@ -670,16 +672,22 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>  	}
>  
>  	ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev);
> -	if (ret) {
> -		iommu_group_put(group);
> -		return ret;
> -	}
> +	if (ret)
> +		goto out;
>  
> -	vfio_platform_get_reset(vdev);
> +	ret = vfio_platform_get_reset(vdev);
> +	if (ret && vdev->reset_required) {
> +		pr_err("vfio: no reset function found for device %s\n",
> +		       vdev->name);
> +		goto out;
> +	}
Unfortunately this causes a crash when the reset module is not
available. You should do the vfio_add_group_dev at the end. Something
like below.

Best Regards

Eric

---
 drivers/vfio/platform/vfio_platform_common.c | 26
++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c
b/drivers/vfio/platform/vfio_platform_common.c
index d84c399..0928f7d 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -664,6 +664,14 @@ int vfio_platform_probe_common(struct
vfio_platform_device *vdev,
                return ret;

        vdev->device = dev;
+       mutex_init(&vdev->igate);
+
+       ret = vfio_platform_get_reset(vdev);
+       if (ret && vdev->reset_required) {
+               pr_err("vfio: no reset function found for device %s\n",
+                      vdev->name);
+               return ret;
+       }

        group = iommu_group_get(dev);
        if (!group) {
@@ -672,22 +680,12 @@ int vfio_platform_probe_common(struct
vfio_platform_device *vdev,
        }

        ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev);
-       if (ret)
-               goto out;
-
-       ret = vfio_platform_get_reset(vdev);
-       if (ret && vdev->reset_required) {
-               pr_err("vfio: no reset function found for device %s\n",
-                      vdev->name);
-               goto out;
-       }
-
-       mutex_init(&vdev->igate);
+       if (ret) {
+               iommu_group_put(group);
+               return ret;
+       }

        return 0;
-out:
-       iommu_group_put(group);
-       return ret;
 }
 EXPORT_SYMBOL_GPL(vfio_platform_probe_common);



>  
>  	mutex_init(&vdev->igate);
>  
>  	return 0;
> +out:
> +	iommu_group_put(group);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(vfio_platform_probe_common);
>  
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index ba9e4f8..68fbc00 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 {
>  };
>  
>  struct vfio_platform_device {
> +	bool				reset_required;
>  	struct vfio_platform_region	*regions;
>  	u32				num_regions;
>  	struct vfio_platform_irq	*irqs;
> 

[PATCH V7 7/9] vfio, platform: make reset driver a requirement by default

[Sinan Kaya]

On 6/16/2016 4:15 AM, Auger Eric wrote:

> Unfortunately this causes a crash when the reset module is not
> available. You should do the vfio_add_group_dev at the end. Something
> like below.
> 

Thanks for testing. I'll post v8 with this change. 


> Best Regards
> 
> Eric
> 
> ---
>  drivers/vfio/platform/vfio_platform_common.c | 26
> ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c
> b/drivers/vfio/platform/vfio_platform_common.c
> index d84c399..0928f7d 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -664,6 +664,14 @@ int vfio_platform_probe_common(struct
> vfio_platform_device *vdev,
>                 return ret;
> 
>         vdev->device = dev;
> +       mutex_init(&vdev->igate);
> +
> +       ret = vfio_platform_get_reset(vdev);
> +       if (ret && vdev->reset_required) {
> +               pr_err("vfio: no reset function found for device %s\n",
> +                      vdev->name);
> +               return ret;
> +       }
> 
>         group = iommu_group_get(dev);
>         if (!group) {
> @@ -672,22 +680,12 @@ int vfio_platform_probe_common(struct
> vfio_platform_device *vdev,
>         }
> 
>         ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev);
> -       if (ret)
> -               goto out;
> -
> -       ret = vfio_platform_get_reset(vdev);
> -       if (ret && vdev->reset_required) {
> -               pr_err("vfio: no reset function found for device %s\n",
> -                      vdev->name);
> -               goto out;
> -       }
> -
> -       mutex_init(&vdev->igate);
> +       if (ret) {
> +               iommu_group_put(group);
> +               return ret;
> +       }
> 
>         return 0;
> -out:
> -       iommu_group_put(group);
> -       return ret;
>  }
>  EXPORT_SYMBOL_GPL(vfio_platform_probe_common);
> 
> 
> 
>>  
>>  	mutex_init(&vdev->igate);
>>  
>>  	return 0;
>> +out:
>> +	iommu_group_put(group);
>> +	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(vfio_platform_probe_common);
>>  
>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
>> index ba9e4f8..68fbc00 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 {
>>  };
>>  
>>  struct vfio_platform_device {
>> +	bool				reset_required;
>>  	struct vfio_platform_region	*regions;
>>  	u32				num_regions;
>>  	struct vfio_platform_irq	*irqs;
>>


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V7 8/9] vfio: platform: check reset call return code during open

[Sinan Kaya]

Open call is ignoring the return code from reset call and can
potentially continue even though reset call failed.

If reset_required module parameter is set, this patch is going
to validate the return code and will abort open if reset fails.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/vfio/platform/vfio_platform_common.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index d84c399..d3141e7 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -264,6 +264,8 @@ static int vfio_platform_open(void *device_data)
 	mutex_lock(&driver_lock);
 
 	if (!vdev->refcnt) {
+		const char *extra_dbg = NULL;
+
 		ret = vfio_platform_regions_init(vdev);
 		if (ret)
 			goto err_reg;
@@ -272,7 +274,12 @@ static int vfio_platform_open(void *device_data)
 		if (ret)
 			goto err_irq;
 
-		vfio_platform_call_reset(vdev, NULL);
+		ret = vfio_platform_call_reset(vdev, &extra_dbg);
+		if (ret && vdev->reset_required) {
+			dev_warn(vdev->device, "reset driver is required and reset call failed in open (%d) %s\n",
+				 ret, extra_dbg ? extra_dbg : "");
+			goto err_rst;
+		}
 	}
 
 	vdev->refcnt++;
@@ -280,6 +287,8 @@ static int vfio_platform_open(void *device_data)
 	mutex_unlock(&driver_lock);
 	return 0;
 
+err_rst:
+	vfio_platform_irq_cleanup(vdev);
 err_irq:
 	vfio_platform_regions_cleanup(vdev);
 err_reg:
-- 
1.8.2.1

[PATCH V7 8/9] vfio: platform: check reset call return code during open

[Auger Eric]

Hi Sinan,

On 13/06/2016 06:26, Sinan Kaya wrote:
> Open call is ignoring the return code from reset call and can
> potentially continue even though reset call failed.
> 
> If reset_required module parameter is set, this patch is going
> to validate the return code and will abort open if reset fails.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/vfio/platform/vfio_platform_common.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index d84c399..d3141e7 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -264,6 +264,8 @@ static int vfio_platform_open(void *device_data)
>  	mutex_lock(&driver_lock);
>  
>  	if (!vdev->refcnt) {
> +		const char *extra_dbg = NULL;
> +
>  		ret = vfio_platform_regions_init(vdev);
>  		if (ret)
>  			goto err_reg;
> @@ -272,7 +274,12 @@ static int vfio_platform_open(void *device_data)
>  		if (ret)
>  			goto err_irq;
>  
> -		vfio_platform_call_reset(vdev, NULL);
> +		ret = vfio_platform_call_reset(vdev, &extra_dbg);
> +		if (ret && vdev->reset_required) {
> +			dev_warn(vdev->device, "reset driver is required and reset call failed in open (%d) %s\n",
> +				 ret, extra_dbg ? extra_dbg : "");
> +			goto err_rst;
> +		}
>  	}
>  
>  	vdev->refcnt++;
> @@ -280,6 +287,8 @@ static int vfio_platform_open(void *device_data)
>  	mutex_unlock(&driver_lock);
>  	return 0;
>  
> +err_rst:
> +	vfio_platform_irq_cleanup(vdev);
>  err_irq:
>  	vfio_platform_regions_cleanup(vdev);
>  err_reg:
> 

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

[PATCH V7 9/9] vfio: platform: check reset call return code during release

[Sinan Kaya]

Release call is ignoring the return code from reset call and can
potentially continue even though reset call failed.

If reset_required module parameter is set, this patch is going
to validate the return code and will cause stack dump with
WARN_ON and warn the user of failure.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 drivers/vfio/platform/vfio_platform_common.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index d3141e7..5035c3f 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -243,7 +243,15 @@ static void vfio_platform_release(void *device_data)
 	mutex_lock(&driver_lock);
 
 	if (!(--vdev->refcnt)) {
-		vfio_platform_call_reset(vdev, NULL);
+		const char *extra_dbg = NULL;
+		int ret;
+
+		ret = vfio_platform_call_reset(vdev, &extra_dbg);
+		if (ret && vdev->reset_required) {
+			dev_warn(vdev->device, "reset driver is required and reset call failed in release (%d) %s\n",
+				 ret, extra_dbg ? extra_dbg : "");
+			WARN_ON(1);
+		}
 		vfio_platform_regions_cleanup(vdev);
 		vfio_platform_irq_cleanup(vdev);
 	}
-- 
1.8.2.1

[PATCH V7 0/9] vfio, platform: add ACPI support

[Sinan Kaya]

Hi Baptiste,

On 6/13/2016 12:26 AM, Sinan Kaya wrote:
> 
> Sinan Kaya (9):
>   vfio: platform: rename reset function
>   vfio: platform: move reset call to a common function
>   vfio: platform: determine reset capability
>   vfio: platform: add support for ACPI probe
>   vfio: platform: add extra debug info argument to call reset
>   vfio: platform: call _RST method when using ACPI
>   vfio, platform: make reset driver a requirement by default
>   vfio: platform: check reset call return code during open
>   vfio: platform: check reset call return code during release
> 
>  drivers/vfio/platform/vfio_amba.c             |   5 +
>  drivers/vfio/platform/vfio_platform.c         |   5 +
>  drivers/vfio/platform/vfio_platform_common.c  | 195 +++++++++++++++++++++-----
>  drivers/vfio/platform/vfio_platform_private.h |   8 +-
>  4 files changed, 176 insertions(+), 37 deletions(-)
> 
Cov and I exchanged some emails with Alex last week. I think
there is a general expectation for you to review the series since
you are the subsystem maintainer.

I'm looking forward to hear your feedback on the series.
Sinan

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V7 0/9] vfio, platform: add ACPI support

[Baptiste Reynal]

Hi Sinan,

Although I don't have the hardware to test them, I reviewed your
patches and they look fine to me.

Regards,
Baptiste

On Mon, Jun 13, 2016 at 6:08 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> Hi Baptiste,
>
> On 6/13/2016 12:26 AM, Sinan Kaya wrote:
>>
>> Sinan Kaya (9):
>>   vfio: platform: rename reset function
>>   vfio: platform: move reset call to a common function
>>   vfio: platform: determine reset capability
>>   vfio: platform: add support for ACPI probe
>>   vfio: platform: add extra debug info argument to call reset
>>   vfio: platform: call _RST method when using ACPI
>>   vfio, platform: make reset driver a requirement by default
>>   vfio: platform: check reset call return code during open
>>   vfio: platform: check reset call return code during release
>>
>>  drivers/vfio/platform/vfio_amba.c             |   5 +
>>  drivers/vfio/platform/vfio_platform.c         |   5 +
>>  drivers/vfio/platform/vfio_platform_common.c  | 195 +++++++++++++++++++++-----
>>  drivers/vfio/platform/vfio_platform_private.h |   8 +-
>>  4 files changed, 176 insertions(+), 37 deletions(-)
>>
> Cov and I exchanged some emails with Alex last week. I think
> there is a general expectation for you to review the series since
> you are the subsystem maintainer.
>
> I'm looking forward to hear your feedback on the series.
> Sinan
>
> --
> Sinan Kaya
> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V7 0/9] vfio, platform: add ACPI support

[Sinan Kaya]

On 6/14/2016 11:29 AM, Baptiste Reynal wrote:
> Hi Sinan,
> 
> Although I don't have the hardware to test them, I reviewed your
> patches and they look fine to me.
> 
> Regards,
> Baptiste

Thanks, I tested ACPI. Eric is testing DT in order not to break
existing functionality.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project