[PATCH V3 1/3] vfio, platform: add support for ACPI while detecting the reset driver

[PATCH V3 1/3] vfio, platform: add support for ACPI while detecting the reset driver

[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.
The change allows a driver to register with DT compatible string or ACPI
HID and then match the object with one of these conditions.

Rules for loading the reset driver are as follow:
- ACPI HID needs match for ACPI systems
- DT compat needs to match for OF systems

Tested-by: Eric Auger <eric.auger@linaro.org> (device tree only)
Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only)
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 .../vfio/platform/reset/vfio_platform_amdxgbe.c    |   4 +-
 .../platform/reset/vfio_platform_calxedaxgmac.c    |   4 +-
 drivers/vfio/platform/vfio_platform_common.c       | 107 +++++++++++++++++----
 drivers/vfio/platform/vfio_platform_private.h      |  43 +++++----
 4 files changed, 120 insertions(+), 38 deletions(-)

diff --git a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
index d4030d0..6d87442 100644
--- a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
+++ b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
@@ -119,7 +119,9 @@ int vfio_platform_amdxgbe_reset(struct vfio_platform_device *vdev)
 	return 0;
 }
 
-module_vfio_reset_handler("amd,xgbe-seattle-v1a", vfio_platform_amdxgbe_reset);
+module_vfio_reset_handler("amd,xgbe-seattle-v1a", NULL,
+			  vfio_platform_amdxgbe_reset);
+MODULE_ALIAS_VFIO_PLATFORM_RESET("amd,xgbe-seattle-v1a");
 
 MODULE_VERSION("0.1");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
index e3d3d94..952dd55 100644
--- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
+++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
@@ -77,7 +77,9 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
 	return 0;
 }
 
-module_vfio_reset_handler("calxeda,hb-xgmac", vfio_platform_calxedaxgmac_reset);
+module_vfio_reset_handler("calxeda,hb-xgmac", NULL,
+			  vfio_platform_calxedaxgmac_reset);
+MODULE_ALIAS_VFIO_PLATFORM_RESET("calxeda,hb-xgmac");
 
 MODULE_VERSION(DRIVER_VERSION);
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index e65b142..c28883a 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>
@@ -31,14 +32,22 @@ static LIST_HEAD(reset_list);
 static DEFINE_MUTEX(driver_lock);
 
 static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
-					struct module **module)
+				  const char *acpihid, struct module **module)
 {
 	struct vfio_platform_reset_node *iter;
 	vfio_platform_reset_fn_t reset_fn = NULL;
 
 	mutex_lock(&driver_lock);
 	list_for_each_entry(iter, &reset_list, link) {
-		if (!strcmp(iter->compat, compat) &&
+		if (acpihid && iter->acpihid &&
+		    !strcmp(iter->acpihid, acpihid) &&
+			try_module_get(iter->owner)) {
+			*module = iter->owner;
+			reset_fn = iter->reset;
+			break;
+		}
+		if (compat && iter->compat &&
+		    !strcmp(iter->compat, compat) &&
 			try_module_get(iter->owner)) {
 			*module = iter->owner;
 			reset_fn = iter->reset;
@@ -49,15 +58,30 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
 	return reset_fn;
 }
 
-static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
+static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
 {
-	vdev->reset = vfio_platform_lookup_reset(vdev->compat,
-						&vdev->reset_module);
-	if (!vdev->reset) {
-		request_module("vfio-reset:%s", vdev->compat);
-		vdev->reset = vfio_platform_lookup_reset(vdev->compat,
-							 &vdev->reset_module);
-	}
+	int rc = -ENODEV;
+
+	vdev->reset = vfio_platform_lookup_reset(vdev->compat, vdev->acpihid,
+						 &vdev->reset_module);
+	if (vdev->reset)
+		return 0;
+
+	if (vdev->acpihid)
+		rc = request_module("vfio-reset:%s", vdev->acpihid);
+
+	if (rc && vdev->compat)
+		rc = request_module("vfio-reset:%s", vdev->compat);
+
+	if (rc)
+		return rc;
+
+	vdev->reset = vfio_platform_lookup_reset(vdev->compat, vdev->acpihid,
+						 &vdev->reset_module);
+	if (vdev->reset)
+		return 0;
+
+	return -ENODEV;
 }
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
@@ -544,6 +568,46 @@ static const struct vfio_device_ops vfio_platform_ops = {
 	.mmap		= vfio_platform_mmap,
 };
 
+#ifdef CONFIG_ACPI
+int vfio_platform_probe_acpi(struct vfio_platform_device *vdev,
+			     struct device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (!adev)
+		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
+int vfio_platform_probe_acpi(struct vfio_platform_device *vdev,
+			     struct device *dev)
+{
+	return -EINVAL;
+}
+#endif
+
+int vfio_platform_probe_of(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;
+	}
+	return 0;
+}
+
 int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 			       struct device *dev)
 {
@@ -553,14 +617,14 @@ 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_probe_acpi(vdev, dev);
+	if (ret)
+		ret = vfio_platform_probe_of(vdev, dev);
 
-	vdev->device = dev;
+	if (ret)
+		return ret;
 
+	vdev->device = dev;
 	group = iommu_group_get(dev);
 	if (!group) {
 		pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
@@ -574,7 +638,6 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 	}
 
 	vfio_platform_get_reset(vdev);
-
 	mutex_init(&vdev->igate);
 
 	return 0;
@@ -605,13 +668,21 @@ void __vfio_platform_register_reset(struct vfio_platform_reset_node *node)
 EXPORT_SYMBOL_GPL(__vfio_platform_register_reset);
 
 void vfio_platform_unregister_reset(const char *compat,
+				    const char *acpihid,
 				    vfio_platform_reset_fn_t fn)
 {
 	struct vfio_platform_reset_node *iter, *temp;
 
 	mutex_lock(&driver_lock);
 	list_for_each_entry_safe(iter, temp, &reset_list, link) {
-		if (!strcmp(iter->compat, compat) && (iter->reset == fn)) {
+		if (acpihid && iter->acpihid &&
+		    !strcmp(iter->acpihid, acpihid) && (iter->reset == fn)) {
+			list_del(&iter->link);
+			break;
+		}
+
+		if (compat && iter->compat &&
+		    !strcmp(iter->compat, compat) && (iter->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..1b95adf 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;
 
@@ -79,6 +80,7 @@ typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
 struct vfio_platform_reset_node {
 	struct list_head link;
 	char *compat;
+	char *acpihid;
 	struct module *owner;
 	vfio_platform_reset_fn_t reset;
 };
@@ -98,27 +100,32 @@ extern int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
 
 extern void __vfio_platform_register_reset(struct vfio_platform_reset_node *n);
 extern void vfio_platform_unregister_reset(const char *compat,
+					   const char *acpihid,
 					   vfio_platform_reset_fn_t fn);
-#define vfio_platform_register_reset(__compat, __reset)		\
-static struct vfio_platform_reset_node __reset ## _node = {	\
-	.owner = THIS_MODULE,					\
-	.compat = __compat,					\
-	.reset = __reset,					\
-};								\
+
+#define vfio_platform_register_reset(__compat, __acpihid, __reset)	\
+static struct vfio_platform_reset_node __reset ## _node = {		\
+	.owner = THIS_MODULE,						\
+	.compat = __compat,						\
+	.acpihid = __acpihid,						\
+	.reset = __reset,						\
+};									\
 __vfio_platform_register_reset(&__reset ## _node)
 
-#define module_vfio_reset_handler(compat, reset)		\
-MODULE_ALIAS("vfio-reset:" compat);				\
-static int __init reset ## _module_init(void)			\
-{								\
-	vfio_platform_register_reset(compat, reset);		\
-	return 0;						\
-};								\
-static void __exit reset ## _module_exit(void)			\
-{								\
-	vfio_platform_unregister_reset(compat, reset);		\
-};								\
-module_init(reset ## _module_init);				\
+#define MODULE_ALIAS_VFIO_PLATFORM_RESET(name)				\
+	MODULE_ALIAS("vfio-reset:" name)
+
+#define module_vfio_reset_handler(compat, acpihid, reset)		\
+static int __init reset ## _module_init(void)				\
+{									\
+	vfio_platform_register_reset(compat, acpihid, reset);		\
+	return 0;							\
+};									\
+static void __exit reset ## _module_exit(void)				\
+{									\
+	vfio_platform_unregister_reset(compat, acpihid, reset);		\
+};									\
+module_init(reset ## _module_init);					\
 module_exit(reset ## _module_exit)
 
 #endif /* VFIO_PLATFORM_PRIVATE_H */
-- 
1.8.2.1

Re: [PATCH V3 1/3] vfio, platform: add support for ACPI while detecting the reset driver

[Arnd Bergmann]

On Monday 28 March 2016 09:35:22 Sinan Kaya wrote:
> 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.
> The change allows a driver to register with DT compatible string or ACPI
> HID and then match the object with one of these conditions.
> 
> Rules for loading the reset driver are as follow:
> - ACPI HID needs match for ACPI systems
> - DT compat needs to match for OF systems
> 
> Tested-by: Eric Auger <eric.auger@linaro.org> (device tree only)
> Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only)
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> 


This really feels wrong for two reasons:

* device assignment of non-PCI devices is really special and doesn't
  seem to make sense on general purpose servers that would be the target
  for ACPI normally

* If there is indeed a requirement for ACPI to handle something like this,
  it should be part of the ACPI spec, with a well-defined method of handling
  reset, rather than having to add a device specific hack for each
  device separately.

	Arnd

Re: [PATCH V3 1/3] vfio, platform: add support for ACPI while detecting the reset driver

[okaya]

On 2016-03-29 05:25, Arnd Bergmann wrote:
> On Monday 28 March 2016 09:35:22 Sinan Kaya wrote:
>> 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.
>> The change allows a driver to register with DT compatible string or 
>> ACPI
>> HID and then match the object with one of these conditions.
>> 
>> Rules for loading the reset driver are as follow:
>> - ACPI HID needs match for ACPI systems
>> - DT compat needs to match for OF systems
>> 
>> Tested-by: Eric Auger <eric.auger@linaro.org> (device tree only)
>> Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only)
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> 
> 
> 
> This really feels wrong for two reasons:
> 
> * device assignment of non-PCI devices is really special and doesn't
>   seem to make sense on general purpose servers that would be the 
> target
>   for ACPI normally


Why is it special? Acpi is not equal to pci. Platform devices are first 
class devices too. Especially, _cls was introduced for this reason.


> 
> * If there is indeed a requirement for ACPI to handle something like 
> this,
>   it should be part of the ACPI spec, with a well-defined method of 
> handling
>   reset, rather than having to add a device specific hack for each
>   device separately.
> 

I see. Normally, this is done by calling _rst method. AFAIK, Linux 
doesn’t support _rst. I can check its presence and call it if it is 
there.



> 	Arnd

Re: [PATCH V3 1/3] vfio, platform: add support for ACPI while detecting the reset driver

[Arnd Bergmann]

On Tuesday 29 March 2016 06:59:15 okaya@codeaurora.org wrote:
> On 2016-03-29 05:25, Arnd Bergmann wrote:
> > On Monday 28 March 2016 09:35:22 Sinan Kaya wrote:
> >> 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.
> >> The change allows a driver to register with DT compatible string or 
> >> ACPI
> >> HID and then match the object with one of these conditions.
> >> 
> >> Rules for loading the reset driver are as follow:
> >> - ACPI HID needs match for ACPI systems
> >> - DT compat needs to match for OF systems
> >> 
> >> Tested-by: Eric Auger <eric.auger@linaro.org> (device tree only)
> >> Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only)
> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> >> 
> > 
> > 
> > This really feels wrong for two reasons:
> > 
> > * device assignment of non-PCI devices is really special and doesn't
> >   seem to make sense on general purpose servers that would be the 
> > target
> >   for ACPI normally
> 
> 
> Why is it special? Acpi is not equal to pci. Platform devices are first 
> class devices too. Especially, _cls was introduced for this reason.

It still feels like a hack. The normal design for a server is to have
all internal devices show up on the PCI host bridge, next to the PCIe
ports, to have a simple way to manage any device, both internal and
off-chip. Putting a device on random MMIO registers outside of the
discoverable buses and have the firmware work around the lack of
discoverability will always be inferior.

> > 
> > * If there is indeed a requirement for ACPI to handle something like 
> > this,
> >   it should be part of the ACPI spec, with a well-defined method of 
> > handling
> >   reset, rather than having to add a device specific hack for each
> >   device separately.
> > 
> 
> I see. Normally, this is done by calling _rst method. AFAIK, Linux 
> doesn’t support _rst. I can check its presence and call it if it is 
> there.

Yes, that sounds reasonable: In patch 2 where you check for the
presence of the reset method, just keep the existing logic for
DT based systems, and use _rst on ACPI based systems instead,
then you can drop both patches 1 and 3.

	Arnd

Re: [PATCH V3 1/3] vfio, platform: add support for ACPI while detecting the reset driver

[okaya]

On 2016-03-29 07:25, Arnd Bergmann wrote:
> On Tuesday 29 March 2016 06:59:15 okaya@codeaurora.org wrote:
>> On 2016-03-29 05:25, Arnd Bergmann wrote:
>> > On Monday 28 March 2016 09:35:22 Sinan Kaya wrote:
>> >> 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.
>> >> The change allows a driver to register with DT compatible string or
>> >> ACPI
>> >> HID and then match the object with one of these conditions.
>> >>
>> >> Rules for loading the reset driver are as follow:
>> >> - ACPI HID needs match for ACPI systems
>> >> - DT compat needs to match for OF systems
>> >>
>> >> Tested-by: Eric Auger <eric.auger@linaro.org> (device tree only)
>> >> Tested-by: Shanker Donthineni <shankerd@codeaurora.org> (ACPI only)
>> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> >>
>> >
>> >
>> > This really feels wrong for two reasons:
>> >
>> > * device assignment of non-PCI devices is really special and doesn't
>> >   seem to make sense on general purpose servers that would be the
>> > target
>> >   for ACPI normally
>> 
>> 
>> Why is it special? Acpi is not equal to pci. Platform devices are 
>> first
>> class devices too. Especially, _cls was introduced for this reason.
> 
> It still feels like a hack. The normal design for a server is to have
> all internal devices show up on the PCI host bridge, next to the PCIe
> ports, to have a simple way to manage any device, both internal and
> off-chip. Putting a device on random MMIO registers outside of the
> discoverable buses and have the firmware work around the lack of
> discoverability will always be inferior.
> 

It is a HW implementation choice. Having everything as pci problem has 
been already solved. I would vote for it when we had SW pci bridge layer 
just to use usb and sata. Not anymore. Especially, _cls solves this 
problem



>> >
>> > * If there is indeed a requirement for ACPI to handle something like
>> > this,
>> >   it should be part of the ACPI spec, with a well-defined method of
>> > handling
>> >   reset, rather than having to add a device specific hack for each
>> >   device separately.
>> >
>> 
>> I see. Normally, this is done by calling _rst method. AFAIK, Linux
>> doesn’t support _rst. I can check its presence and call it if it is
>> there.
> 
> Yes, that sounds reasonable: In patch 2 where you check for the
> presence of the reset method, just keep the existing logic for
> DT based systems, and use _rst on ACPI based systems instead,
> then you can drop both patches 1 and 3.
> 

I can certainly drop patch #3 and push the reset responsibility to acpi.

I never liked having a fragmented sw design across multiple drivers.

I need something for patch #1. Compatible is a DT property not ACPI.but 
then, I won't have a reset driver anymore.

If we think about how vfio pci works, we pass the pci vendor and device 
id to new_id file to find out which pci device needs to be pass thru.

I can go to a similar route. This time we pass the object id through 
new_id and I call reset method on this object.

Let me know what you think?

> 	Arnd

Re: [PATCH V3 1/3] vfio, platform: add support for ACPI while detecting the reset driver

[Arnd Bergmann]

On Tuesday 29 March 2016 08:15:42 okaya@codeaurora.org wrote:
> >> >
> >> > * If there is indeed a requirement for ACPI to handle something like
> >> > this,
> >> >   it should be part of the ACPI spec, with a well-defined method of
> >> > handling
> >> >   reset, rather than having to add a device specific hack for each
> >> >   device separately.
> >> >
> >> 
> >> I see. Normally, this is done by calling _rst method. AFAIK, Linux
> >> doesn’t support _rst. I can check its presence and call it if it is
> >> there.
> > 
> > Yes, that sounds reasonable: In patch 2 where you check for the
> > presence of the reset method, just keep the existing logic for
> > DT based systems, and use _rst on ACPI based systems instead,
> > then you can drop both patches 1 and 3.
> > 
> 
> I can certainly drop patch #3 and push the reset responsibility to acpi.
> 
> I never liked having a fragmented sw design across multiple drivers.
> 
> I need something for patch #1. Compatible is a DT property not ACPI.but 
> then, I won't have a reset driver anymore.
> 
> If we think about how vfio pci works, we pass the pci vendor and device 
> id to new_id file to find out which pci device needs to be pass thru.
> 
> I can go to a similar route. This time we pass the object id through 
> new_id and I call reset method on this object.
> 
> Let me know what you think?

It would certainly be nice to make it work more like PCI VFIO does
here, where you can assign any device as long as it has an IOMMU
(and a _rst method in this case).

	Arnd

Re: [PATCH V3 1/3] vfio, platform: add support for ACPI while detecting the reset driver

[Sinan Kaya]

On 3/29/2016 8:44 AM, Arnd Bergmann wrote:
>>  can certainly drop patch #3 and push the reset responsibility to acpi.
>> > 
>> > I never liked having a fragmented sw design across multiple drivers.
>> > 
>> > I need something for patch #1. Compatible is a DT property not ACPI.but 
>> > then, I won't have a reset driver anymore.
>> > 
>> > If we think about how vfio pci works, we pass the pci vendor and device 
>> > id to new_id file to find out which pci device needs to be pass thru.
>> > 
>> > I can go to a similar route. This time we pass the object id through 
>> > new_id and I call reset method on this object.
>> > 
>> > Let me know what you think?
> It would certainly be nice to make it work more like PCI VFIO does
> here, where you can assign any device as long as it has an IOMMU
> (and a _rst method in this case).
> 
> 	Arnd

I looked at the code today. This doesn't have to be as convoluted as PCI is. 
The VFIO platform driver already has a pointer to the actual object in
vdev->pdev.dev.

This is really as simple as calling _RST method on the passed oject at the last
step below. We don't need to create a dynamic list like PCI does. 

This is how the driver gets called. 

echo vfio-platform | tee -a /sys/bus/platform/devices/QCOM8061:00/driver_override
echo QCOM8061:00 | tee -a  /sys/bus/platform/devices/QCOM8061:00/driver/unbind
echo QCOM8061:00 |tee -a /sys/bus/platform/drivers_probe

I'll post a patch as soon as I test it.

-- 
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 V3 2/3] 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.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/vfio/platform/vfio_amba.c             | 5 +++++
 drivers/vfio/platform/vfio_platform.c         | 5 +++++
 drivers/vfio/platform/vfio_platform_common.c  | 8 +++++++-
 drivers/vfio/platform/vfio_platform_private.h | 1 +
 4 files changed, 18 insertions(+), 1 deletion(-)

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 c28883a..67a3163 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -637,7 +637,13 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 		return ret;
 	}
 
-	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);
+		iommu_group_put(group);
+		return ret;
+	}
 	mutex_init(&vdev->igate);
 
 	return 0;
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 1b95adf..b5a699a 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 V3 3/3] vfio, platform: add QTI HIDMA reset driver

[Sinan Kaya]

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.

This patch is implementing a reset driver for HIDMA platform driver.
This gets called by the VFIO platform reset interface.

Reviewed-by: Eric Auger <eric.auger@linaro.org>
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/vfio/platform/reset/Kconfig                |   9 ++
 drivers/vfio/platform/reset/Makefile               |   2 +
 .../vfio/platform/reset/vfio_platform_qcomhidma.c  | 101 +++++++++++++++++++++
 3 files changed, 112 insertions(+)
 create mode 100644 drivers/vfio/platform/reset/vfio_platform_qcomhidma.c

diff --git a/drivers/vfio/platform/reset/Kconfig b/drivers/vfio/platform/reset/Kconfig
index 70cccc5..d02b3b5 100644
--- a/drivers/vfio/platform/reset/Kconfig
+++ b/drivers/vfio/platform/reset/Kconfig
@@ -13,3 +13,12 @@ config VFIO_PLATFORM_AMDXGBE_RESET
 	  Enables the VFIO platform driver to handle reset for AMD XGBE
 
 	  If you don't know what to do here, say N.
+
+config VFIO_PLATFORM_QCOMHIDMA_RESET
+	tristate "VFIO support for Qualcomm Technologies HIDMA reset"
+	depends on VFIO_PLATFORM
+	help
+	  Enables the VFIO platform driver to handle reset for Qualcomm Technologies
+	  HIDMA Channel.
+
+	  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
index 93f4e23..ec7748a 100644
--- a/drivers/vfio/platform/reset/Makefile
+++ b/drivers/vfio/platform/reset/Makefile
@@ -1,7 +1,9 @@
 vfio-platform-calxedaxgmac-y := vfio_platform_calxedaxgmac.o
 vfio-platform-amdxgbe-y := vfio_platform_amdxgbe.o
+vfio-platform-qcomhidma-y := vfio_platform_qcomhidma.o
 
 ccflags-y += -Idrivers/vfio/platform
 
 obj-$(CONFIG_VFIO_PLATFORM_CALXEDAXGMAC_RESET) += vfio-platform-calxedaxgmac.o
+obj-$(CONFIG_VFIO_PLATFORM_QCOMHIDMA_RESET) += vfio-platform-qcomhidma.o
 obj-$(CONFIG_VFIO_PLATFORM_AMDXGBE_RESET) += vfio-platform-amdxgbe.o
diff --git a/drivers/vfio/platform/reset/vfio_platform_qcomhidma.c b/drivers/vfio/platform/reset/vfio_platform_qcomhidma.c
new file mode 100644
index 0000000..ef3491b
--- /dev/null
+++ b/drivers/vfio/platform/reset/vfio_platform_qcomhidma.c
@@ -0,0 +1,101 @@
+/*
+ * Qualcomm Technologies HIDMA VFIO Reset Driver
+ *
+ * Copyright (c) 2016, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/device.h>
+#include <linux/iopoll.h>
+
+#include "vfio_platform_private.h"
+
+#define TRCA_CTRLSTS_OFFSET		0x000
+#define EVCA_CTRLSTS_OFFSET		0x000
+
+#define CH_CONTROL_MASK		GENMASK(7, 0)
+#define CH_STATE_MASK			GENMASK(7, 0)
+#define CH_STATE_BIT_POS		0x8
+
+#define HIDMA_CH_STATE(val)	\
+	((val >> CH_STATE_BIT_POS) & CH_STATE_MASK)
+
+#define EVCA_IRQ_EN_OFFSET		0x110
+
+#define CH_RESET			9
+#define CH_DISABLED			0
+
+int vfio_platform_qcomhidma_reset(struct vfio_platform_device *vdev)
+{
+	struct vfio_platform_region *trreg;
+	struct vfio_platform_region *evreg;
+	u32 val;
+	int ret;
+
+	if (vdev->num_regions != 2)
+		return -ENODEV;
+
+	trreg = &vdev->regions[0];
+	if (!trreg->ioaddr) {
+		trreg->ioaddr =
+			ioremap_nocache(trreg->addr, trreg->size);
+		if (!trreg->ioaddr)
+			return -ENOMEM;
+	}
+
+	evreg = &vdev->regions[1];
+	if (!evreg->ioaddr) {
+		evreg->ioaddr =
+			ioremap_nocache(evreg->addr, evreg->size);
+		if (!evreg->ioaddr)
+			return -ENOMEM;
+	}
+
+	/* disable IRQ */
+	writel(0, evreg->ioaddr + EVCA_IRQ_EN_OFFSET);
+
+	/* reset both transfer and event channels */
+	val = readl(trreg->ioaddr + TRCA_CTRLSTS_OFFSET);
+	val &= ~(CH_CONTROL_MASK << 16);
+	val |= CH_RESET << 16;
+	writel(val, trreg->ioaddr + TRCA_CTRLSTS_OFFSET);
+
+	ret = readl_poll_timeout(trreg->ioaddr + TRCA_CTRLSTS_OFFSET, val,
+				 HIDMA_CH_STATE(val) == CH_DISABLED, 1000,
+				 10000);
+	if (ret)
+		return ret;
+
+	val = readl(evreg->ioaddr + EVCA_CTRLSTS_OFFSET);
+	val &= ~(CH_CONTROL_MASK << 16);
+	val |= CH_RESET << 16;
+	writel(val, evreg->ioaddr + EVCA_CTRLSTS_OFFSET);
+
+	ret = readl_poll_timeout(evreg->ioaddr + EVCA_CTRLSTS_OFFSET, val,
+				 HIDMA_CH_STATE(val) == CH_DISABLED, 1000,
+				 10000);
+	if (ret)
+		return ret;
+
+	pr_info("HIDMA channel reset\n");
+	return 0;
+}
+module_vfio_reset_handler("qcom,hidma-1.0", "QCOM8061",
+			  vfio_platform_qcomhidma_reset);
+MODULE_ALIAS_VFIO_PLATFORM_RESET("qcom,hidma-1.0");
+MODULE_ALIAS_VFIO_PLATFORM_RESET("QCOM8061");
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Reset support for Qualcomm Technologies HIDMA device");
-- 
1.8.2.1