[RFC PATCH v5 01/11] driver core: platform: add device binding path 'driver_override'

[RFC PATCH v5 01/11] driver core: platform: add device binding path 'driver_override'

[Antonios Motakis]

From: Kim Phillips <kim.phillips@freescale.com>

Needed by platform device drivers, such as the vfio-platform driver [1],
in order to bypass the existing OF, ACPI, id_table and name string matches,
and successfully be able to be bound to any device, like so:

echo vfio-platform > /sys/bus/platform/devices/fff51000.ethernet/driver_override
echo fff51000.ethernet > /sys/bus/platform/devices/fff51000.ethernet/driver/unbind
echo fff51000.ethernet > /sys/bus/platform/drivers_probe

This mimics "PCI: Introduce new device binding path using
pci_dev.driver_override" [2], which is an interface enhancement
for more deterministic PCI device binding, e.g., when in the
presence of hotplug.

[1] http://lkml.iu.edu/hypermail/linux/kernel/1402.1/00177.html
[2] http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html

Suggested-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
---
 Documentation/ABI/testing/sysfs-bus-platform | 20 ++++++++++++
 drivers/base/platform.c                      | 46 ++++++++++++++++++++++++++++
 include/linux/platform_device.h              |  1 +
 3 files changed, 67 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-platform

diff --git a/Documentation/ABI/testing/sysfs-bus-platform b/Documentation/ABI/testing/sysfs-bus-platform
new file mode 100644
index 0000000..5172a61
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-platform
@@ -0,0 +1,20 @@
+What:		/sys/bus/platform/devices/.../driver_override
+Date:		April 2014
+Contact:	Kim Phillips <kim.phillips@freescale.com>
+Description:
+		This file allows the driver for a device to be specified which
+		will override standard OF, ACPI, ID table, and name matching.
+		When specified, only a driver with a name matching the value
+		written to driver_override will have an opportunity to bind
+		to the device.  The override is specified by writing a string
+		to the driver_override file (echo vfio-platform > \
+		driver_override) and may be cleared with an empty string
+		(echo > driver_override).  This returns the device to standard
+		matching rules binding.  Writing to driver_override does not
+		automatically unbind the device from its current driver or make
+		any attempt to automatically load the specified driver.  If no
+		driver with a matching name is currently loaded in the kernel,
+		the device will not bind to any driver.  This also allows
+		devices to opt-out of driver binding using a driver_override
+		name such as "none".  Only a single driver may be specified in
+		the override, there is no support for parsing delimiters.
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index bc78848..6db9fc2 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -22,6 +22,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/idr.h>
 #include <linux/acpi.h>
+#include <linux/limits.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -693,8 +694,49 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
 }
 static DEVICE_ATTR_RO(modalias);
 
+static ssize_t driver_override_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	char *driver_override, *old = pdev->driver_override, *cp;
+
+	if (count > PATH_MAX)
+		return -EINVAL;
+
+	driver_override = kstrndup(buf, count, GFP_KERNEL);
+	if (!driver_override)
+		return -ENOMEM;
+
+	cp = strchr(driver_override, '\n');
+	if (cp)
+		*cp = '\0';
+
+	if (strlen(driver_override)) {
+		pdev->driver_override = driver_override;
+	} else {
+		kfree(driver_override);
+		pdev->driver_override = NULL;
+	}
+
+	kfree(old);
+
+	return count;
+}
+
+static ssize_t driver_override_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+
+	return sprintf(buf, "%s\n", pdev->driver_override);
+}
+static DEVICE_ATTR_RW(driver_override);
+
+
 static struct attribute *platform_dev_attrs[] = {
 	&dev_attr_modalias.attr,
+	&dev_attr_driver_override.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(platform_dev);
@@ -750,6 +792,10 @@ static int platform_match(struct device *dev, struct device_driver *drv)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct platform_driver *pdrv = to_platform_driver(drv);
 
+	/* When driver_override is set, only bind to the matching driver */
+	if (pdev->driver_override)
+		return !strcmp(pdev->driver_override, drv->name);
+
 	/* Attempt an OF style match first */
 	if (of_driver_match_device(dev, drv))
 		return 1;
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 16f6654..153d303 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -28,6 +28,7 @@ struct platform_device {
 	struct resource	*resource;
 
 	const struct platform_device_id	*id_entry;
+	char *driver_override; /* Driver name to force a match */
 
 	/* MFD cell pointer */
 	struct mfd_cell *mfd_cell;
-- 
1.8.3.2

[RFC PATCH v5_v2 01/11] driver core: platform: add device binding path 'driver_override'

[Kim Phillips]

From: Kim Phillips <kim.phillips@freescale.com>

Needed by platform device drivers, such as the vfio-platform driver
later in series, in order to bypass the existing OF, ACPI, id_table and
name string matches, and successfully be able to be bound to any
device, like so:

echo vfio-platform > /sys/bus/platform/devices/fff51000.ethernet/driver_override
echo fff51000.ethernet > /sys/bus/platform/devices/fff51000.ethernet/driver/unbind
echo fff51000.ethernet > /sys/bus/platform/drivers_probe

This mimics "PCI: Introduce new device binding path using
pci_dev.driver_override" [1], which is an interface enhancement
for more deterministic PCI device binding, e.g., when in the
presence of hotplug.

[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009527.html

Suggested-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
---
changes in v2 patch of v5 of this patchseries:
- rebased onto today's Linus' ToT
- added kfree to match PCI counterpart fix, as Alex Williamson
  just posted a v3 of the patch (thanks Christoffer for the
  notification)
- in the commit text, replaced vfio platform driver reference with
  'later in series', and updated the PCI version mailing list reference
  to the v3 version.

Is it safe to assume this patch will continue to as part of the VFIO
platform driver patchseries, and be submitted by Antonis?  If so, can
we start collecting some {Reviewed,Acked}-bys?   Thanks, Kim.

 Documentation/ABI/testing/sysfs-bus-platform | 20 ++++++++++++
 drivers/base/platform.c                      | 47 ++++++++++++++++++++++++++++
 include/linux/platform_device.h              |  1 +
 3 files changed, 68 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-platform

diff --git a/Documentation/ABI/testing/sysfs-bus-platform b/Documentation/ABI/testing/sysfs-bus-platform
new file mode 100644
index 0000000..5172a61
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-platform
@@ -0,0 +1,20 @@
+What:		/sys/bus/platform/devices/.../driver_override
+Date:		April 2014
+Contact:	Kim Phillips <kim.phillips@freescale.com>
+Description:
+		This file allows the driver for a device to be specified which
+		will override standard OF, ACPI, ID table, and name matching.
+		When specified, only a driver with a name matching the value
+		written to driver_override will have an opportunity to bind
+		to the device.  The override is specified by writing a string
+		to the driver_override file (echo vfio-platform > \
+		driver_override) and may be cleared with an empty string
+		(echo > driver_override).  This returns the device to standard
+		matching rules binding.  Writing to driver_override does not
+		automatically unbind the device from its current driver or make
+		any attempt to automatically load the specified driver.  If no
+		driver with a matching name is currently loaded in the kernel,
+		the device will not bind to any driver.  This also allows
+		devices to opt-out of driver binding using a driver_override
+		name such as "none".  Only a single driver may be specified in
+		the override, there is no support for parsing delimiters.
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 5b47210..4f47563 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -23,6 +23,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/idr.h>
 #include <linux/acpi.h>
+#include <linux/limits.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -188,6 +189,7 @@ static void platform_device_release(struct device *dev)
 	kfree(pa->pdev.dev.platform_data);
 	kfree(pa->pdev.mfd_cell);
 	kfree(pa->pdev.resource);
+	kfree(pa->pdev.driver_override);
 	kfree(pa);
 }
 
@@ -695,8 +697,49 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
 }
 static DEVICE_ATTR_RO(modalias);
 
+static ssize_t driver_override_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	char *driver_override, *old = pdev->driver_override, *cp;
+
+	if (count > PATH_MAX)
+		return -EINVAL;
+
+	driver_override = kstrndup(buf, count, GFP_KERNEL);
+	if (!driver_override)
+		return -ENOMEM;
+
+	cp = strchr(driver_override, '\n');
+	if (cp)
+		*cp = '\0';
+
+	if (strlen(driver_override)) {
+		pdev->driver_override = driver_override;
+	} else {
+		kfree(driver_override);
+		pdev->driver_override = NULL;
+	}
+
+	kfree(old);
+
+	return count;
+}
+
+static ssize_t driver_override_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+
+	return sprintf(buf, "%s\n", pdev->driver_override);
+}
+static DEVICE_ATTR_RW(driver_override);
+
+
 static struct attribute *platform_dev_attrs[] = {
 	&dev_attr_modalias.attr,
+	&dev_attr_driver_override.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(platform_dev);
@@ -752,6 +795,10 @@ static int platform_match(struct device *dev, struct device_driver *drv)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct platform_driver *pdrv = to_platform_driver(drv);
 
+	/* When driver_override is set, only bind to the matching driver */
+	if (pdev->driver_override)
+		return !strcmp(pdev->driver_override, drv->name);
+
 	/* Attempt an OF style match first */
 	if (of_driver_match_device(dev, drv))
 		return 1;
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 16f6654..153d303 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -28,6 +28,7 @@ struct platform_device {
 	struct resource	*resource;
 
 	const struct platform_device_id	*id_entry;
+	char *driver_override; /* Driver name to force a match */
 
 	/* MFD cell pointer */
 	struct mfd_cell *mfd_cell;
-- 
1.9.3

Re: [RFC PATCH v5_v2 01/11] driver core: platform: add device binding path 'driver_override'

[Alex Williamson]

On Tue, 2014-05-20 at 19:25 -0500, Kim Phillips wrote:
> From: Kim Phillips <kim.phillips@freescale.com>
> 
> Needed by platform device drivers, such as the vfio-platform driver
> later in series, in order to bypass the existing OF, ACPI, id_table and
> name string matches, and successfully be able to be bound to any
> device, like so:
> 
> echo vfio-platform > /sys/bus/platform/devices/fff51000.ethernet/driver_override
> echo fff51000.ethernet > /sys/bus/platform/devices/fff51000.ethernet/driver/unbind
> echo fff51000.ethernet > /sys/bus/platform/drivers_probe
> 
> This mimics "PCI: Introduce new device binding path using
> pci_dev.driver_override" [1], which is an interface enhancement
> for more deterministic PCI device binding, e.g., when in the
> presence of hotplug.
> 
> [1] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009527.html
> 
> Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Kim Phillips <kim.phillips@freescale.com>

Looks largely identical to the PCI version of the same that has been
accepted for v3.16 and ack'd by GregKH.

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

> ---
> changes in v2 patch of v5 of this patchseries:
> - rebased onto today's Linus' ToT
> - added kfree to match PCI counterpart fix, as Alex Williamson
>   just posted a v3 of the patch (thanks Christoffer for the
>   notification)
> - in the commit text, replaced vfio platform driver reference with
>   'later in series', and updated the PCI version mailing list reference
>   to the v3 version.
> 
> Is it safe to assume this patch will continue to as part of the VFIO
> platform driver patchseries, and be submitted by Antonis?  If so, can
> we start collecting some {Reviewed,Acked}-bys?   Thanks, Kim.
> 
>  Documentation/ABI/testing/sysfs-bus-platform | 20 ++++++++++++
>  drivers/base/platform.c                      | 47 ++++++++++++++++++++++++++++
>  include/linux/platform_device.h              |  1 +
>  3 files changed, 68 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-platform
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-platform b/Documentation/ABI/testing/sysfs-bus-platform
> new file mode 100644
> index 0000000..5172a61
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-platform
> @@ -0,0 +1,20 @@
> +What:		/sys/bus/platform/devices/.../driver_override
> +Date:		April 2014
> +Contact:	Kim Phillips <kim.phillips@freescale.com>
> +Description:
> +		This file allows the driver for a device to be specified which
> +		will override standard OF, ACPI, ID table, and name matching.
> +		When specified, only a driver with a name matching the value
> +		written to driver_override will have an opportunity to bind
> +		to the device.  The override is specified by writing a string
> +		to the driver_override file (echo vfio-platform > \
> +		driver_override) and may be cleared with an empty string
> +		(echo > driver_override).  This returns the device to standard
> +		matching rules binding.  Writing to driver_override does not
> +		automatically unbind the device from its current driver or make
> +		any attempt to automatically load the specified driver.  If no
> +		driver with a matching name is currently loaded in the kernel,
> +		the device will not bind to any driver.  This also allows
> +		devices to opt-out of driver binding using a driver_override
> +		name such as "none".  Only a single driver may be specified in
> +		the override, there is no support for parsing delimiters.
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 5b47210..4f47563 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -23,6 +23,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/idr.h>
>  #include <linux/acpi.h>
> +#include <linux/limits.h>
>  
>  #include "base.h"
>  #include "power/power.h"
> @@ -188,6 +189,7 @@ static void platform_device_release(struct device *dev)
>  	kfree(pa->pdev.dev.platform_data);
>  	kfree(pa->pdev.mfd_cell);
>  	kfree(pa->pdev.resource);
> +	kfree(pa->pdev.driver_override);
>  	kfree(pa);
>  }
>  
> @@ -695,8 +697,49 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
>  }
>  static DEVICE_ATTR_RO(modalias);
>  
> +static ssize_t driver_override_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	char *driver_override, *old = pdev->driver_override, *cp;
> +
> +	if (count > PATH_MAX)
> +		return -EINVAL;
> +
> +	driver_override = kstrndup(buf, count, GFP_KERNEL);
> +	if (!driver_override)
> +		return -ENOMEM;
> +
> +	cp = strchr(driver_override, '\n');
> +	if (cp)
> +		*cp = '\0';
> +
> +	if (strlen(driver_override)) {
> +		pdev->driver_override = driver_override;
> +	} else {
> +		kfree(driver_override);
> +		pdev->driver_override = NULL;
> +	}
> +
> +	kfree(old);
> +
> +	return count;
> +}
> +
> +static ssize_t driver_override_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +
> +	return sprintf(buf, "%s\n", pdev->driver_override);
> +}
> +static DEVICE_ATTR_RW(driver_override);
> +
> +
>  static struct attribute *platform_dev_attrs[] = {
>  	&dev_attr_modalias.attr,
> +	&dev_attr_driver_override.attr,
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(platform_dev);
> @@ -752,6 +795,10 @@ static int platform_match(struct device *dev, struct device_driver *drv)
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct platform_driver *pdrv = to_platform_driver(drv);
>  
> +	/* When driver_override is set, only bind to the matching driver */
> +	if (pdev->driver_override)
> +		return !strcmp(pdev->driver_override, drv->name);
> +
>  	/* Attempt an OF style match first */
>  	if (of_driver_match_device(dev, drv))
>  		return 1;
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 16f6654..153d303 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -28,6 +28,7 @@ struct platform_device {
>  	struct resource	*resource;
>  
>  	const struct platform_device_id	*id_entry;
> +	char *driver_override; /* Driver name to force a match */
>  
>  	/* MFD cell pointer */
>  	struct mfd_cell *mfd_cell;

Re: [RFC PATCH v5_v2 01/11] driver core: platform: add device binding path 'driver_override'

[Alexander Graf]

On 29.05.14 21:43, Alex Williamson wrote:
> On Tue, 2014-05-20 at 19:25 -0500, Kim Phillips wrote:
>> From: Kim Phillips <kim.phillips@freescale.com>
>>
>> Needed by platform device drivers, such as the vfio-platform driver
>> later in series, in order to bypass the existing OF, ACPI, id_table and
>> name string matches, and successfully be able to be bound to any
>> device, like so:
>>
>> echo vfio-platform > /sys/bus/platform/devices/fff51000.ethernet/driver_override
>> echo fff51000.ethernet > /sys/bus/platform/devices/fff51000.ethernet/driver/unbind
>> echo fff51000.ethernet > /sys/bus/platform/drivers_probe
>>
>> This mimics "PCI: Introduce new device binding path using
>> pci_dev.driver_override" [1], which is an interface enhancement
>> for more deterministic PCI device binding, e.g., when in the
>> presence of hotplug.
>>
>> [1] https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009527.html
>>
>> Suggested-by: Alex Williamson <alex.williamson@redhat.com>
>> Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
> Looks largely identical to the PCI version of the same that has been
> accepted for v3.16 and ack'd by GregKH.
>
> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

Yup, would be great to have feature parity for device binding on 
platform and PCI.

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

[PATCH] driver core: platform: add device binding path 'driver_override'

[Kim Phillips]

Needed by platform device drivers, such as the upcoming
vfio-platform driver, in order to bypass the existing OF, ACPI,
id_table and name string matches, and successfully be able to be
bound to any device, like so:

echo vfio-platform > /sys/bus/platform/devices/fff51000.ethernet/driver_override
echo fff51000.ethernet > /sys/bus/platform/devices/fff51000.ethernet/driver/unbind
echo fff51000.ethernet > /sys/bus/platform/drivers_probe

This mimics "PCI: Introduce new device binding path using
pci_dev.driver_override", which is an interface enhancement
for more deterministic PCI device binding, e.g., when in the
presence of hotplug.

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
Reviewed-by: Alexander Graf <agraf@suse.de>
Reviewed-by: Stuart Yoder <stuart.yoder@freescale.com>
Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
---
Greg,

This is largely identical to the PCI version of the same that has
been accepted for v3.16 and ack'd by you:

https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009674.html

and applied to Bjorn Helgaas' PCI tree:

https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/virtualization&id=782a985d7af26db39e86070d28f987cad21313c0

You are the platform driver core maintainer: can you apply this to
your driver-core tree now?

Thanks,

Kim

changes since previous version (v2 of the v5 vfio-platform driver
series):
- commit text: replaced vfio platform driver reference with
  'upcoming', and removed the PCI version mailing list reference
  since it has now been accepted.
- added Alex W., Alex G., and Stuart's Reviewed-by's.

changes in v2 patch of v5 of this patchseries:
- rebased onto today's Linus' ToT
- added kfree to match PCI counterpart fix, as Alex Williamson
  just posted a v3 of the patch (thanks Christoffer for the
  notification)
- in the commit text, replaced vfio platform driver reference with
  'later in series', and updated the PCI version mailing list reference
  to the v3 version.

 Documentation/ABI/testing/sysfs-bus-platform | 20 ++++++++++++
 drivers/base/platform.c                      | 47 ++++++++++++++++++++++++++++
 include/linux/platform_device.h              |  1 +
 3 files changed, 68 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-platform

diff --git a/Documentation/ABI/testing/sysfs-bus-platform b/Documentation/ABI/testing/sysfs-bus-platform
new file mode 100644
index 0000000..5172a61
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-platform
@@ -0,0 +1,20 @@
+What:		/sys/bus/platform/devices/.../driver_override
+Date:		April 2014
+Contact:	Kim Phillips <kim.phillips@freescale.com>
+Description:
+		This file allows the driver for a device to be specified which
+		will override standard OF, ACPI, ID table, and name matching.
+		When specified, only a driver with a name matching the value
+		written to driver_override will have an opportunity to bind
+		to the device.  The override is specified by writing a string
+		to the driver_override file (echo vfio-platform > \
+		driver_override) and may be cleared with an empty string
+		(echo > driver_override).  This returns the device to standard
+		matching rules binding.  Writing to driver_override does not
+		automatically unbind the device from its current driver or make
+		any attempt to automatically load the specified driver.  If no
+		driver with a matching name is currently loaded in the kernel,
+		the device will not bind to any driver.  This also allows
+		devices to opt-out of driver binding using a driver_override
+		name such as "none".  Only a single driver may be specified in
+		the override, there is no support for parsing delimiters.
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 5b47210..4f47563 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -23,6 +23,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/idr.h>
 #include <linux/acpi.h>
+#include <linux/limits.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -188,6 +189,7 @@ static void platform_device_release(struct device *dev)
 	kfree(pa->pdev.dev.platform_data);
 	kfree(pa->pdev.mfd_cell);
 	kfree(pa->pdev.resource);
+	kfree(pa->pdev.driver_override);
 	kfree(pa);
 }
 
@@ -695,8 +697,49 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
 }
 static DEVICE_ATTR_RO(modalias);
 
+static ssize_t driver_override_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	char *driver_override, *old = pdev->driver_override, *cp;
+
+	if (count > PATH_MAX)
+		return -EINVAL;
+
+	driver_override = kstrndup(buf, count, GFP_KERNEL);
+	if (!driver_override)
+		return -ENOMEM;
+
+	cp = strchr(driver_override, '\n');
+	if (cp)
+		*cp = '\0';
+
+	if (strlen(driver_override)) {
+		pdev->driver_override = driver_override;
+	} else {
+		kfree(driver_override);
+		pdev->driver_override = NULL;
+	}
+
+	kfree(old);
+
+	return count;
+}
+
+static ssize_t driver_override_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+
+	return sprintf(buf, "%s\n", pdev->driver_override);
+}
+static DEVICE_ATTR_RW(driver_override);
+
+
 static struct attribute *platform_dev_attrs[] = {
 	&dev_attr_modalias.attr,
+	&dev_attr_driver_override.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(platform_dev);
@@ -752,6 +795,10 @@ static int platform_match(struct device *dev, struct device_driver *drv)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct platform_driver *pdrv = to_platform_driver(drv);
 
+	/* When driver_override is set, only bind to the matching driver */
+	if (pdev->driver_override)
+		return !strcmp(pdev->driver_override, drv->name);
+
 	/* Attempt an OF style match first */
 	if (of_driver_match_device(dev, drv))
 		return 1;
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 16f6654..153d303 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -28,6 +28,7 @@ struct platform_device {
 	struct resource	*resource;
 
 	const struct platform_device_id	*id_entry;
+	char *driver_override; /* Driver name to force a match */
 
 	/* MFD cell pointer */
 	struct mfd_cell *mfd_cell;
-- 
1.9.3

Re: [PATCH] driver core: platform: add device binding path 'driver_override'

[Greg KH]

On Mon, Jun 02, 2014 at 07:42:58PM -0500, Kim Phillips wrote:
> Needed by platform device drivers, such as the upcoming
> vfio-platform driver, in order to bypass the existing OF, ACPI,
> id_table and name string matches, and successfully be able to be
> bound to any device, like so:
> 
> echo vfio-platform > /sys/bus/platform/devices/fff51000.ethernet/driver_override
> echo fff51000.ethernet > /sys/bus/platform/devices/fff51000.ethernet/driver/unbind
> echo fff51000.ethernet > /sys/bus/platform/drivers_probe
> 
> This mimics "PCI: Introduce new device binding path using
> pci_dev.driver_override", which is an interface enhancement
> for more deterministic PCI device binding, e.g., when in the
> presence of hotplug.
> 
> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> Reviewed-by: Alexander Graf <agraf@suse.de>
> Reviewed-by: Stuart Yoder <stuart.yoder@freescale.com>
> Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
> ---
> Greg,
> 
> This is largely identical to the PCI version of the same that has
> been accepted for v3.16 and ack'd by you:
> 
> https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009674.html
> 
> and applied to Bjorn Helgaas' PCI tree:
> 
> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/virtualization&id=782a985d7af26db39e86070d28f987cad21313c0
> 
> You are the platform driver core maintainer: can you apply this to
> your driver-core tree now?

Yes, I will after this merge window ends, it's too late for 3.16-rc1
with the window opening up a week early, sorry.

greg k-h

Re: [PATCH] driver core: platform: add device binding path 'driver_override'

[Kim Phillips]

On Mon, 2 Jun 2014 21:28:42 -0700
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Mon, Jun 02, 2014 at 07:42:58PM -0500, Kim Phillips wrote:
> > You are the platform driver core maintainer: can you apply this to
> > your driver-core tree now?
> 
> Yes, I will after this merge window ends, it's too late for 3.16-rc1
> with the window opening up a week early, sorry.

How about now?  fwiw, I just checked: it still applies cleanly.

Thanks,

Kim

Re: [PATCH] driver core: platform: add device binding path 'driver_override'

[Kim Phillips]

On Mon, 2 Jun 2014 21:28:42 -0700
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Mon, Jun 02, 2014 at 07:42:58PM -0500, Kim Phillips wrote:
> > You are the platform driver core maintainer: can you apply this to
> > your driver-core tree now?
> 
> Yes, I will after this merge window ends, it's too late for 3.16-rc1
> with the window opening up a week early, sorry.

Hi Greg,

The merge window has ended, and I see commit activity on your
driver-core tree's driver-core-next branch, but this patch is still
missing:  Do you think you can get around to applying it before the
next merge window?

Thanks,

Kim

Re: [PATCH] driver core: platform: add device binding path 'driver_override'

[Greg KH]

On Mon, Jun 02, 2014 at 07:42:58PM -0500, Kim Phillips wrote:
> Needed by platform device drivers, such as the upcoming
> vfio-platform driver, in order to bypass the existing OF, ACPI,
> id_table and name string matches, and successfully be able to be
> bound to any device, like so:
> 
> echo vfio-platform > /sys/bus/platform/devices/fff51000.ethernet/driver_override
> echo fff51000.ethernet > /sys/bus/platform/devices/fff51000.ethernet/driver/unbind
> echo fff51000.ethernet > /sys/bus/platform/drivers_probe
> 
> This mimics "PCI: Introduce new device binding path using
> pci_dev.driver_override", which is an interface enhancement
> for more deterministic PCI device binding, e.g., when in the
> presence of hotplug.
> 
> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> Reviewed-by: Alexander Graf <agraf@suse.de>
> Reviewed-by: Stuart Yoder <stuart.yoder@freescale.com>
> Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
> ---
> Greg,
> 
> This is largely identical to the PCI version of the same that has
> been accepted for v3.16 and ack'd by you:
> 
> https://lists.cs.columbia.edu/pipermail/kvmarm/2014-May/009674.html
> 
> and applied to Bjorn Helgaas' PCI tree:
> 
> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/virtualization&id=782a985d7af26db39e86070d28f987cad21313c0
> 
> You are the platform driver core maintainer: can you apply this to
> your driver-core tree now?

Sorry for the very long delay, it's now merged in my tree.

Thanks for being persistant.

greg k-h

[RFC PATCH v5 02/11] ARM SMMU: Add capability IOMMU_CAP_DMA_EXEC

[Antonios Motakis]

The ARM SMMU can take an IOMMU_EXEC protection flag in addition to
IOMMU_READ and IOMMU_WRITE. Expose this as an IOMMU capability.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/iommu/arm-smmu.c | 2 ++
 include/linux/iommu.h    | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 1d9ab39..abf802f 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1509,6 +1509,8 @@ static int arm_smmu_domain_has_cap(struct iommu_domain *domain,
 	if (smmu_domain->root_cfg.smmu->features & ARM_SMMU_FEAT_COHERENT_WALK)
 		caps |= IOMMU_CAP_CACHE_COHERENCY;
 
+	caps |= IOMMU_CAP_DMA_EXEC;
+
 	return !!(cap & caps);
 }
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index b96a5b2..4f547f3 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -57,8 +57,9 @@ struct iommu_domain {
 	struct iommu_domain_geometry geometry;
 };
 
-#define IOMMU_CAP_CACHE_COHERENCY	0x1
-#define IOMMU_CAP_INTR_REMAP		0x2	/* isolates device intrs */
+#define IOMMU_CAP_CACHE_COHERENCY	(1 << 0)
+#define IOMMU_CAP_INTR_REMAP		(1 << 1) /* isolates device intrs */
+#define IOMMU_CAP_DMA_EXEC		(1 << 2) /* EXEC protection flag */
 
 /*
  * Following constraints are specifc to FSL_PAMUV1:
-- 
1.8.3.2

Re: [RFC PATCH v5 02/11] ARM SMMU: Add capability IOMMU_CAP_DMA_EXEC

[Will Deacon]

On Mon, Apr 28, 2014 at 04:52:42PM +0100, Antonios Motakis wrote:
> The ARM SMMU can take an IOMMU_EXEC protection flag in addition to
> IOMMU_READ and IOMMU_WRITE. Expose this as an IOMMU capability.

The other way of handling this would be to negate the capability and
advertise a NOEXEC cap instead. That would need the IOMMU_EXEC flag to
become IOMMU_NOEXEC and the ARM SMMU driver updating accordingly, but it
might make more sense if people don't object to mixing positive and negative
logic in the IOMMU_* flags.

Any thoughts?

Will

Re: [RFC PATCH v5 02/11] ARM SMMU: Add capability IOMMU_CAP_DMA_EXEC

[Alex Williamson]

On Mon, 2014-04-28 at 20:37 +0100, Will Deacon wrote:
> On Mon, Apr 28, 2014 at 04:52:42PM +0100, Antonios Motakis wrote:
> > The ARM SMMU can take an IOMMU_EXEC protection flag in addition to
> > IOMMU_READ and IOMMU_WRITE. Expose this as an IOMMU capability.
> 
> The other way of handling this would be to negate the capability and
> advertise a NOEXEC cap instead. That would need the IOMMU_EXEC flag to
> become IOMMU_NOEXEC and the ARM SMMU driver updating accordingly, but it
> might make more sense if people don't object to mixing positive and negative
> logic in the IOMMU_* flags.
> 
> Any thoughts?

A benefit of doing that would be that the flag becomes enforceable.  As
written in this draft, if a user does not specify EXEC, the mapping may
or may not be executable, depending on the IOMMU capability (assuming
that if EXEC is not supported that it follows READ).  If the flag
changes to NOEXEC, then all the domains in the container should support
it or else the mapping should fail.  We could also avoid the test in
vfio code when doing a mapping and just let the IOMMU driver fail the
map if NOEXEC is unsupported.  Thanks,

Alex

Re: [RFC PATCH v5 02/11] ARM SMMU: Add capability IOMMU_CAP_DMA_EXEC

[Antonios Motakis]

On Mon, Apr 28, 2014 at 9:37 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Mon, Apr 28, 2014 at 04:52:42PM +0100, Antonios Motakis wrote:
>> The ARM SMMU can take an IOMMU_EXEC protection flag in addition to
>> IOMMU_READ and IOMMU_WRITE. Expose this as an IOMMU capability.
>
> The other way of handling this would be to negate the capability and
> advertise a NOEXEC cap instead. That would need the IOMMU_EXEC flag to
> become IOMMU_NOEXEC and the ARM SMMU driver updating accordingly, but it
> might make more sense if people don't object to mixing positive and negative
> logic in the IOMMU_* flags.
>
> Any thoughts?
>

I agree, that would be simpler for the user. As it is now we have to
worry about whether the EXEC flac is available just to use the PL330,
with NOEXEC the device would always be usable by default.


> Will



-- 
Antonios Motakis
Virtual Open Systems

[RFC PATCH v5 03/11] VFIO_IOMMU_TYPE1 for platform bus devices on ARM

[Antonios Motakis]

This allows to make use of the VFIO_IOMMU_TYPE1 driver with platform
devices on ARM in addition to PCI. This is required in order to use the
Exynos SMMU, or ARM SMMU driver with VFIO_IOMMU_TYPE1.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/Kconfig            | 2 +-
 drivers/vfio/vfio_iommu_type1.c | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index af7b204..3a598ed 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -11,7 +11,7 @@ config VFIO_IOMMU_SPAPR_TCE
 menuconfig VFIO
 	tristate "VFIO Non-Privileged userspace driver framework"
 	depends on IOMMU_API
-	select VFIO_IOMMU_TYPE1 if X86
+	select VFIO_IOMMU_TYPE1 if X86 || ARM
 	select VFIO_IOMMU_SPAPR_TCE if (PPC_POWERNV || PPC_PSERIES)
 	select ANON_INODES
 	help
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 1f90344..4dc989d 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -30,6 +30,7 @@
 #include <linux/iommu.h>
 #include <linux/module.h>
 #include <linux/mm.h>
+#include <linux/pci.h>		/* pci_bus_type */
 #include <linux/rbtree.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
@@ -721,13 +722,15 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	INIT_LIST_HEAD(&domain->group_list);
 	list_add(&group->next, &domain->group_list);
 
-	if (!allow_unsafe_interrupts &&
+#ifdef CONFIG_PCI
+	if (bus == &pci_bus_type && !allow_unsafe_interrupts &&
 	    !iommu_domain_has_cap(domain->domain, IOMMU_CAP_INTR_REMAP)) {
 		pr_warn("%s: No interrupt remapping support.  Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
 		       __func__);
 		ret = -EPERM;
 		goto out_detach;
 	}
+#endif
 
 	if (iommu_domain_has_cap(domain->domain, IOMMU_CAP_CACHE_COHERENCY))
 		domain->prot |= IOMMU_CACHE;
-- 
1.8.3.2

Re: [RFC PATCH v5 03/11] VFIO_IOMMU_TYPE1 for platform bus devices on ARM

[Alex Williamson]

On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
> This allows to make use of the VFIO_IOMMU_TYPE1 driver with platform
> devices on ARM in addition to PCI. This is required in order to use the
> Exynos SMMU, or ARM SMMU driver with VFIO_IOMMU_TYPE1.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/vfio/Kconfig            | 2 +-
>  drivers/vfio/vfio_iommu_type1.c | 5 ++++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> index af7b204..3a598ed 100644
> --- a/drivers/vfio/Kconfig
> +++ b/drivers/vfio/Kconfig
> @@ -11,7 +11,7 @@ config VFIO_IOMMU_SPAPR_TCE
>  menuconfig VFIO
>  	tristate "VFIO Non-Privileged userspace driver framework"
>  	depends on IOMMU_API
> -	select VFIO_IOMMU_TYPE1 if X86
> +	select VFIO_IOMMU_TYPE1 if X86 || ARM
>  	select VFIO_IOMMU_SPAPR_TCE if (PPC_POWERNV || PPC_PSERIES)
>  	select ANON_INODES
>  	help
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 1f90344..4dc989d 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -30,6 +30,7 @@
>  #include <linux/iommu.h>
>  #include <linux/module.h>
>  #include <linux/mm.h>
> +#include <linux/pci.h>		/* pci_bus_type */
>  #include <linux/rbtree.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> @@ -721,13 +722,15 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	INIT_LIST_HEAD(&domain->group_list);
>  	list_add(&group->next, &domain->group_list);
>  
> -	if (!allow_unsafe_interrupts &&
> +#ifdef CONFIG_PCI
> +	if (bus == &pci_bus_type && !allow_unsafe_interrupts &&
>  	    !iommu_domain_has_cap(domain->domain, IOMMU_CAP_INTR_REMAP)) {
>  		pr_warn("%s: No interrupt remapping support.  Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
>  		       __func__);
>  		ret = -EPERM;
>  		goto out_detach;
>  	}
> +#endif
>  
>  	if (iommu_domain_has_cap(domain->domain, IOMMU_CAP_CACHE_COHERENCY))
>  		domain->prot |= IOMMU_CACHE;

This is not a PCI specific requirement.  Anything that can support MSI
needs an IOMMU that can provide isolation for both DMA and interrupts.
I think the IOMMU should still be telling us that it has this feature.
Thanks,

Alex

Re: [RFC PATCH v5 03/11] VFIO_IOMMU_TYPE1 for platform bus devices on ARM

[Will Deacon]

Hi Alex,

On Mon, Apr 28, 2014 at 05:43:41PM +0100, Alex Williamson wrote:
> On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
> > This allows to make use of the VFIO_IOMMU_TYPE1 driver with platform
> > devices on ARM in addition to PCI. This is required in order to use the
> > Exynos SMMU, or ARM SMMU driver with VFIO_IOMMU_TYPE1.

[...]

> > @@ -721,13 +722,15 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> >  	INIT_LIST_HEAD(&domain->group_list);
> >  	list_add(&group->next, &domain->group_list);
> >  
> > -	if (!allow_unsafe_interrupts &&
> > +#ifdef CONFIG_PCI
> > +	if (bus == &pci_bus_type && !allow_unsafe_interrupts &&
> >  	    !iommu_domain_has_cap(domain->domain, IOMMU_CAP_INTR_REMAP)) {
> >  		pr_warn("%s: No interrupt remapping support.  Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
> >  		       __func__);
> >  		ret = -EPERM;
> >  		goto out_detach;
> >  	}
> > +#endif
> >  
> >  	if (iommu_domain_has_cap(domain->domain, IOMMU_CAP_CACHE_COHERENCY))
> >  		domain->prot |= IOMMU_CACHE;
> 
> This is not a PCI specific requirement.  Anything that can support MSI
> needs an IOMMU that can provide isolation for both DMA and interrupts.
> I think the IOMMU should still be telling us that it has this feature.

Please excuse any ignorance on part here (I'm not at all familiar with the
Intel IOMMU), but shouldn't this really be a property of the interrupt
controller itself? On ARM with GICv3, there is a separate block called the
ITS (interrupt translation service) which is part of the interrupt
controller. The ITS provides a doorbell page which the SMMU can map into a
guest operating system to provide MSI for passthrough devices, but this
isn't something the SMMU is aware of -- it will just see the iommu_map
request for a non-cacheable mapping.

Will

Re: [RFC PATCH v5 03/11] VFIO_IOMMU_TYPE1 for platform bus devices on ARM

[Alex Williamson]

On Mon, 2014-04-28 at 20:19 +0100, Will Deacon wrote:
> Hi Alex,
> 
> On Mon, Apr 28, 2014 at 05:43:41PM +0100, Alex Williamson wrote:
> > On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
> > > This allows to make use of the VFIO_IOMMU_TYPE1 driver with platform
> > > devices on ARM in addition to PCI. This is required in order to use the
> > > Exynos SMMU, or ARM SMMU driver with VFIO_IOMMU_TYPE1.
> 
> [...]
> 
> > > @@ -721,13 +722,15 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> > >  	INIT_LIST_HEAD(&domain->group_list);
> > >  	list_add(&group->next, &domain->group_list);
> > >  
> > > -	if (!allow_unsafe_interrupts &&
> > > +#ifdef CONFIG_PCI
> > > +	if (bus == &pci_bus_type && !allow_unsafe_interrupts &&
> > >  	    !iommu_domain_has_cap(domain->domain, IOMMU_CAP_INTR_REMAP)) {
> > >  		pr_warn("%s: No interrupt remapping support.  Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
> > >  		       __func__);
> > >  		ret = -EPERM;
> > >  		goto out_detach;
> > >  	}
> > > +#endif
> > >  
> > >  	if (iommu_domain_has_cap(domain->domain, IOMMU_CAP_CACHE_COHERENCY))
> > >  		domain->prot |= IOMMU_CACHE;
> > 
> > This is not a PCI specific requirement.  Anything that can support MSI
> > needs an IOMMU that can provide isolation for both DMA and interrupts.
> > I think the IOMMU should still be telling us that it has this feature.
> 
> Please excuse any ignorance on part here (I'm not at all familiar with the
> Intel IOMMU), but shouldn't this really be a property of the interrupt
> controller itself? On ARM with GICv3, there is a separate block called the
> ITS (interrupt translation service) which is part of the interrupt
> controller. The ITS provides a doorbell page which the SMMU can map into a
> guest operating system to provide MSI for passthrough devices, but this
> isn't something the SMMU is aware of -- it will just see the iommu_map
> request for a non-cacheable mapping.

Hi Will,

I don't know the history of why this is an IOMMU domain capability on
x86, it's sort of a paradox.  An MSI from a device is conceptually just
a DMA write and is therefore logically co-located in the IOMMU hardware,
but x86 doesn't allow it to be mapped via the IOMMU API interfaces.  For
compatibility, interrupt remapping support is buried deep in the
request_irq interface and effectively invisible other than having this
path to query it.  Therefore this flag is effectively just saying "MSI
isolation support is present and enabled".  IOW, the host is protected
from interrupt injection attacks from malicious devices.  If there is
some property of your platform that makes this always the case, then the
IOMMU driver can always export this capability as true.

With PCI, MSI is configured via spec defined configuration space
registers, so we emulate these registers and prevent user access to them
so that we don't need to allow the user a way to setup an interrupt
remapping entry.  It's done for them via request_irq.

IIRC, the Freescale devices have a limited number of MSI pages and can
therefore create some instances with isolation while others may require
sharing.  In that case I would expect this flag to indicate whether the
domain has an exclusive or shared page.

In any case, I suspect keying on the bus_type here is not the correct
way to go.  Thanks,

Alex

Re: [RFC PATCH v5 03/11] VFIO_IOMMU_TYPE1 for platform bus devices on ARM

[Will Deacon]

On Mon, Apr 28, 2014 at 09:08:10PM +0100, Alex Williamson wrote:
> On Mon, 2014-04-28 at 20:19 +0100, Will Deacon wrote:
> > Please excuse any ignorance on part here (I'm not at all familiar with the
> > Intel IOMMU), but shouldn't this really be a property of the interrupt
> > controller itself? On ARM with GICv3, there is a separate block called the
> > ITS (interrupt translation service) which is part of the interrupt
> > controller. The ITS provides a doorbell page which the SMMU can map into a
> > guest operating system to provide MSI for passthrough devices, but this
> > isn't something the SMMU is aware of -- it will just see the iommu_map
> > request for a non-cacheable mapping.
> 
> I don't know the history of why this is an IOMMU domain capability on
> x86, it's sort of a paradox.  An MSI from a device is conceptually just
> a DMA write and is therefore logically co-located in the IOMMU hardware,
> but x86 doesn't allow it to be mapped via the IOMMU API interfaces.  For
> compatibility, interrupt remapping support is buried deep in the
> request_irq interface and effectively invisible other than having this
> path to query it.  Therefore this flag is effectively just saying "MSI
> isolation support is present and enabled".  IOW, the host is protected
> from interrupt injection attacks from malicious devices.  If there is
> some property of your platform that makes this always the case, then the
> IOMMU driver can always export this capability as true.

Thanks for the explanation. On ARM, the SMMU does indeed see the MSI write
just like a normal write, so it can be mapped via iommu_map() to point at
the interrupt controller doorbell page. I guess that means we can enable
this capability for all MSI-capable devices upstream of the SMMU, providing
that the IRQ controller doesn't have any horrible quirks.

> With PCI, MSI is configured via spec defined configuration space
> registers, so we emulate these registers and prevent user access to them
> so that we don't need to allow the user a way to setup an interrupt
> remapping entry.  It's done for them via request_irq.
> 
> IIRC, the Freescale devices have a limited number of MSI pages and can
> therefore create some instances with isolation while others may require
> sharing.  In that case I would expect this flag to indicate whether the
> domain has an exclusive or shared page.
> 
> In any case, I suspect keying on the bus_type here is not the correct
> way to go.  Thanks,

Agreed, I was more intrigued by the meaning of the flag.

Thanks,

Will

[RFC PATCH v5 04/11] VFIO_IOMMU_TYPE1: Introduce the VFIO_DMA_MAP_FLAG_EXEC flag

[Antonios Motakis]

The ARM SMMU driver expects the IOMMU_EXEC flag, otherwise it will
set the page tables for a device as XN (execute never). This affects
devices such as the ARM PL330 DMA Controller, which fails to operate
if the XN flag is set on the memory it tries to fetch its instructions
from.

We introduce the VFIO_DMA_MAP_FLAG_EXEC to VFIO, and use it in
VFIO_IOMMU_TYPE1 to set the IOMMU_EXEC flag. This way the user can
control whether the XN flag will be set on the requested mappings. If
the IOMMU_EXEC flag is available for at least one IOMMU of a container,
the new capability VFIO_IOMMU_PROT_EXEC will be exposed.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/vfio_iommu_type1.c | 34 +++++++++++++++++++++++++++++++---
 include/uapi/linux/vfio.h       |  2 ++
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 4dc989d..6ce32bf 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -498,12 +498,15 @@ static int map_try_harder(struct vfio_domain *domain, dma_addr_t iova,
 }
 
 static int vfio_iommu_map(struct vfio_iommu *iommu, dma_addr_t iova,
-			  unsigned long pfn, long npage, int prot)
+			  unsigned long pfn, long npage, int prot, bool exec)
 {
 	struct vfio_domain *d;
 	int ret;
 
 	list_for_each_entry(d, &iommu->domain_list, next) {
+		if (exec && iommu_domain_has_cap(d->domain, IOMMU_CAP_DMA_EXEC))
+			prot |= IOMMU_EXEC;
+
 		ret = iommu_map(d->domain, iova, (phys_addr_t)pfn << PAGE_SHIFT,
 				npage << PAGE_SHIFT, prot | d->prot);
 		if (ret) {
@@ -530,6 +533,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 	size_t size = map->size;
 	long npage;
 	int ret = 0, prot = 0;
+	bool prot_exec = false;
 	uint64_t mask;
 	struct vfio_dma *dma;
 	unsigned long pfn;
@@ -543,6 +547,8 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 		prot |= IOMMU_WRITE;
 	if (map->flags & VFIO_DMA_MAP_FLAG_READ)
 		prot |= IOMMU_READ;
+	if (map->flags & VFIO_DMA_MAP_FLAG_EXEC)
+		prot_exec = true;
 
 	if (!prot)
 		return -EINVAL; /* No READ/WRITE? */
@@ -595,7 +601,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 		}
 
 		/* Map it! */
-		ret = vfio_iommu_map(iommu, iova, pfn, npage, prot);
+		ret = vfio_iommu_map(iommu, iova, pfn, npage, prot, prot_exec);
 		if (ret) {
 			vfio_unpin_pages(pfn, npage, prot, true);
 			break;
@@ -887,6 +893,23 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
 	return ret;
 }
 
+static int vfio_domains_have_iommu_exec(struct vfio_iommu *iommu)
+{
+	struct vfio_domain *d;
+	int ret = 0;
+
+	mutex_lock(&iommu->lock);
+	list_for_each_entry(d, &iommu->domain_list, next) {
+		if (iommu_domain_has_cap(d->domain, IOMMU_CAP_DMA_EXEC)) {
+			ret = 1;
+			break;
+		}
+	}
+	mutex_unlock(&iommu->lock);
+
+	return ret;
+}
+
 static long vfio_iommu_type1_ioctl(void *iommu_data,
 				   unsigned int cmd, unsigned long arg)
 {
@@ -902,6 +925,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 			if (!iommu)
 				return 0;
 			return vfio_domains_have_iommu_cache(iommu);
+		case VFIO_IOMMU_PROT_EXEC:
+			if (!iommu)
+				return 0;
+			return vfio_domains_have_iommu_exec(iommu);
 		default:
 			return 0;
 		}
@@ -925,7 +952,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 	} else if (cmd == VFIO_IOMMU_MAP_DMA) {
 		struct vfio_iommu_type1_dma_map map;
 		uint32_t mask = VFIO_DMA_MAP_FLAG_READ |
-				VFIO_DMA_MAP_FLAG_WRITE;
+				VFIO_DMA_MAP_FLAG_WRITE |
+				VFIO_DMA_MAP_FLAG_EXEC;
 
 		minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
 
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index cb9023d..0847b29 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -29,6 +29,7 @@
  * capability is subject to change as groups are added or removed.
  */
 #define VFIO_DMA_CC_IOMMU		4
+#define VFIO_IOMMU_PROT_EXEC		5
 
 /*
  * The IOCTL interface is designed for extensibility by embedding the
@@ -398,6 +399,7 @@ struct vfio_iommu_type1_dma_map {
 	__u32	flags;
 #define VFIO_DMA_MAP_FLAG_READ (1 << 0)		/* readable from device */
 #define VFIO_DMA_MAP_FLAG_WRITE (1 << 1)	/* writable from device */
+#define VFIO_DMA_MAP_FLAG_EXEC (1 << 2)		/* executable from device */
 	__u64	vaddr;				/* Process virtual address */
 	__u64	iova;				/* IO virtual address */
 	__u64	size;				/* Size of mapping (bytes) */
-- 
1.8.3.2

Re: [RFC PATCH v5 04/11] VFIO_IOMMU_TYPE1: Introduce the VFIO_DMA_MAP_FLAG_EXEC flag

[Antonios Motakis]

On Mon, Apr 28, 2014 at 6:53 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
>> The ARM SMMU driver expects the IOMMU_EXEC flag, otherwise it will
>> set the page tables for a device as XN (execute never). This affects
>> devices such as the ARM PL330 DMA Controller, which fails to operate
>> if the XN flag is set on the memory it tries to fetch its instructions
>> from.
>>
>> We introduce the VFIO_DMA_MAP_FLAG_EXEC to VFIO, and use it in
>> VFIO_IOMMU_TYPE1 to set the IOMMU_EXEC flag. This way the user can
>> control whether the XN flag will be set on the requested mappings. If
>> the IOMMU_EXEC flag is available for at least one IOMMU of a container,
>> the new capability VFIO_IOMMU_PROT_EXEC will be exposed.
>>
>> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 34 +++++++++++++++++++++++++++++++---
>>  include/uapi/linux/vfio.h       |  2 ++
>>  2 files changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 4dc989d..6ce32bf 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -498,12 +498,15 @@ static int map_try_harder(struct vfio_domain *domain, dma_addr_t iova,
>>  }
>>
>>  static int vfio_iommu_map(struct vfio_iommu *iommu, dma_addr_t iova,
>> -                       unsigned long pfn, long npage, int prot)
>> +                       unsigned long pfn, long npage, int prot, bool exec)
>>  {
>>       struct vfio_domain *d;
>>       int ret;
>>
>>       list_for_each_entry(d, &iommu->domain_list, next) {
>> +             if (exec && iommu_domain_has_cap(d->domain, IOMMU_CAP_DMA_EXEC))
>> +                     prot |= IOMMU_EXEC;
>> +
>>               ret = iommu_map(d->domain, iova, (phys_addr_t)pfn << PAGE_SHIFT,
>>                               npage << PAGE_SHIFT, prot | d->prot);
>>               if (ret) {
>> @@ -530,6 +533,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>>       size_t size = map->size;
>>       long npage;
>>       int ret = 0, prot = 0;
>> +     bool prot_exec = false;
>>       uint64_t mask;
>>       struct vfio_dma *dma;
>>       unsigned long pfn;
>> @@ -543,6 +547,8 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>>               prot |= IOMMU_WRITE;
>>       if (map->flags & VFIO_DMA_MAP_FLAG_READ)
>>               prot |= IOMMU_READ;
>> +     if (map->flags & VFIO_DMA_MAP_FLAG_EXEC)
>> +             prot_exec = true;
>
> Is it really necessary for this to be a new parameter rather than prot |
> = IOMMU_EXEC here?  It could simply be masked out in vfio_iommu_map()
> when not supported.

It seemed more readable to me at that point to mask it in, but if you
prefer we can mask it out instead.

If we replace EXEC with NOEXEC as suggested in the other thread, then
we might just check if all domains in the container support the flag
and avoid the trouble altogether.

> When IOMMU_CAP_DMA_EXEC is not supported, does
> IOMMU_EXEC effectively follow IOMMU_READ?  If so, does that imply we
> need to error on a request for (IOMMU_WRITE | IOMMU_EXEC)?  Thanks,
>

This is probably a good idea, I imagine there won't ever be a platform
where EXEC without READ makes sense.

> Alex
>
>>
>>       if (!prot)
>>               return -EINVAL; /* No READ/WRITE? */
>> @@ -595,7 +601,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>>               }
>>
>>               /* Map it! */
>> -             ret = vfio_iommu_map(iommu, iova, pfn, npage, prot);
>> +             ret = vfio_iommu_map(iommu, iova, pfn, npage, prot, prot_exec);
>>               if (ret) {
>>                       vfio_unpin_pages(pfn, npage, prot, true);
>>                       break;
>> @@ -887,6 +893,23 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
>>       return ret;
>>  }
>>
>> +static int vfio_domains_have_iommu_exec(struct vfio_iommu *iommu)
>> +{
>> +     struct vfio_domain *d;
>> +     int ret = 0;
>> +
>> +     mutex_lock(&iommu->lock);
>> +     list_for_each_entry(d, &iommu->domain_list, next) {
>> +             if (iommu_domain_has_cap(d->domain, IOMMU_CAP_DMA_EXEC)) {
>> +                     ret = 1;
>> +                     break;
>> +             }
>> +     }
>> +     mutex_unlock(&iommu->lock);
>> +
>> +     return ret;
>> +}
>> +
>>  static long vfio_iommu_type1_ioctl(void *iommu_data,
>>                                  unsigned int cmd, unsigned long arg)
>>  {
>> @@ -902,6 +925,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>>                       if (!iommu)
>>                               return 0;
>>                       return vfio_domains_have_iommu_cache(iommu);
>> +             case VFIO_IOMMU_PROT_EXEC:
>> +                     if (!iommu)
>> +                             return 0;
>> +                     return vfio_domains_have_iommu_exec(iommu);
>>               default:
>>                       return 0;
>>               }
>> @@ -925,7 +952,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>>       } else if (cmd == VFIO_IOMMU_MAP_DMA) {
>>               struct vfio_iommu_type1_dma_map map;
>>               uint32_t mask = VFIO_DMA_MAP_FLAG_READ |
>> -                             VFIO_DMA_MAP_FLAG_WRITE;
>> +                             VFIO_DMA_MAP_FLAG_WRITE |
>> +                             VFIO_DMA_MAP_FLAG_EXEC;
>>
>>               minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
>>
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index cb9023d..0847b29 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -29,6 +29,7 @@
>>   * capability is subject to change as groups are added or removed.
>>   */
>>  #define VFIO_DMA_CC_IOMMU            4
>> +#define VFIO_IOMMU_PROT_EXEC         5
>>
>>  /*
>>   * The IOCTL interface is designed for extensibility by embedding the
>> @@ -398,6 +399,7 @@ struct vfio_iommu_type1_dma_map {
>>       __u32   flags;
>>  #define VFIO_DMA_MAP_FLAG_READ (1 << 0)              /* readable from device */
>>  #define VFIO_DMA_MAP_FLAG_WRITE (1 << 1)     /* writable from device */
>> +#define VFIO_DMA_MAP_FLAG_EXEC (1 << 2)              /* executable from device */
>>       __u64   vaddr;                          /* Process virtual address */
>>       __u64   iova;                           /* IO virtual address */
>>       __u64   size;                           /* Size of mapping (bytes) */
>
>
>



-- 
Antonios Motakis
Virtual Open Systems

[RFC PATCH v5 05/11] VFIO_PLATFORM: Initial skeleton of VFIO support for platform devices

[Antonios Motakis]

This patch forms the skeleton for platform devices support with VFIO.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/Kconfig                          |   1 +
 drivers/vfio/Makefile                         |   1 +
 drivers/vfio/platform/Kconfig                 |   9 ++
 drivers/vfio/platform/Makefile                |   4 +
 drivers/vfio/platform/vfio_platform.c         | 172 ++++++++++++++++++++++++++
 drivers/vfio/platform/vfio_platform_private.h |  22 ++++
 include/uapi/linux/vfio.h                     |   1 +
 7 files changed, 210 insertions(+)
 create mode 100644 drivers/vfio/platform/Kconfig
 create mode 100644 drivers/vfio/platform/Makefile
 create mode 100644 drivers/vfio/platform/vfio_platform.c
 create mode 100644 drivers/vfio/platform/vfio_platform_private.h

diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index 3a598ed..a484d72 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -21,3 +21,4 @@ menuconfig VFIO
 	  If you don't know what to do here, say N.
 
 source "drivers/vfio/pci/Kconfig"
+source "drivers/vfio/platform/Kconfig"
diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index 72bfabc..b5e4a33 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_VFIO) += vfio.o
 obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
 obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
 obj-$(CONFIG_VFIO_PCI) += pci/
+obj-$(CONFIG_VFIO_PLATFORM) += platform/
diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
new file mode 100644
index 0000000..42b0022
--- /dev/null
+++ b/drivers/vfio/platform/Kconfig
@@ -0,0 +1,9 @@
+config VFIO_PLATFORM
+	tristate "VFIO support for platform devices"
+	depends on VFIO && EVENTFD
+	help
+	  Support for platform devices with VFIO. This is required to make
+	  use of platform devices present on the system using the VFIO
+	  framework.
+
+	  If you don't know what to do here, say N.
diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
new file mode 100644
index 0000000..df3a014
--- /dev/null
+++ b/drivers/vfio/platform/Makefile
@@ -0,0 +1,4 @@
+
+vfio-platform-y := vfio_platform.o
+
+obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
new file mode 100644
index 0000000..1661746
--- /dev/null
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -0,0 +1,172 @@
+/*
+ * Copyright (C) 2013 - Virtual Open Systems
+ * Author: Antonios Motakis <a.motakis@virtualopensystems.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, 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/device.h>
+#include <linux/eventfd.h>
+#include <linux/interrupt.h>
+#include <linux/iommu.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/vfio.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/irq.h>
+
+#include "vfio_platform_private.h"
+
+#define DRIVER_VERSION  "0.5"
+#define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
+#define DRIVER_DESC     "VFIO for platform devices - User Level meta-driver"
+
+static void vfio_platform_release(void *device_data)
+{
+	module_put(THIS_MODULE);
+}
+
+static int vfio_platform_open(void *device_data)
+{
+	if (!try_module_get(THIS_MODULE))
+		return -ENODEV;
+
+	return 0;
+}
+
+static long vfio_platform_ioctl(void *device_data,
+			   unsigned int cmd, unsigned long arg)
+{
+	struct vfio_platform_device *vdev = device_data;
+	unsigned long minsz;
+
+	if (cmd == VFIO_DEVICE_GET_INFO) {
+		struct vfio_device_info info;
+
+		minsz = offsetofend(struct vfio_device_info, num_irqs);
+
+		if (copy_from_user(&info, (void __user *)arg, minsz))
+			return -EFAULT;
+
+		if (info.argsz < minsz)
+			return -EINVAL;
+
+		info.flags = VFIO_DEVICE_FLAGS_PLATFORM;
+		info.num_regions = 0;
+		info.num_irqs = 0;
+
+		return copy_to_user((void __user *)arg, &info, minsz);
+
+	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
+		return -EINVAL;
+
+	else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
+		return -EINVAL;
+
+	else if (cmd == VFIO_DEVICE_SET_IRQS)
+		return -EINVAL;
+
+	else if (cmd == VFIO_DEVICE_RESET)
+		return -EINVAL;
+
+	return -ENOTTY;
+}
+
+static ssize_t vfio_platform_read(void *device_data, char __user *buf,
+			     size_t count, loff_t *ppos)
+{
+	return 0;
+}
+
+static ssize_t vfio_platform_write(void *device_data, const char __user *buf,
+			      size_t count, loff_t *ppos)
+{
+	return 0;
+}
+
+static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
+{
+	return -EINVAL;
+}
+
+static const struct vfio_device_ops vfio_platform_ops = {
+	.name		= "vfio-platform",
+	.open		= vfio_platform_open,
+	.release	= vfio_platform_release,
+	.ioctl		= vfio_platform_ioctl,
+	.read		= vfio_platform_read,
+	.write		= vfio_platform_write,
+	.mmap		= vfio_platform_mmap,
+};
+
+static int vfio_platform_probe(struct platform_device *pdev)
+{
+	struct vfio_platform_device *vdev;
+	struct iommu_group *group;
+	int ret;
+
+	group = iommu_group_get(&pdev->dev);
+	if (!group) {
+		pr_err("VFIO: No IOMMU group for device %s\n", pdev->name);
+		return -EINVAL;
+	}
+
+	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
+	if (!vdev) {
+		iommu_group_put(group);
+		return -ENOMEM;
+	}
+
+	vdev->pdev = pdev;
+
+	ret = vfio_add_group_dev(&pdev->dev, &vfio_platform_ops, vdev);
+	if (ret) {
+		iommu_group_put(group);
+		kfree(vdev);
+	}
+
+	return ret;
+}
+
+static int vfio_platform_remove(struct platform_device *pdev)
+{
+	struct vfio_platform_device *vdev;
+
+	vdev = vfio_del_group_dev(&pdev->dev);
+	if (!vdev)
+		return -EINVAL;
+
+	iommu_group_put(pdev->dev.iommu_group);
+	kfree(vdev);
+
+	return 0;
+}
+
+static struct platform_driver vfio_platform_driver = {
+	.probe		= vfio_platform_probe,
+	.remove		= vfio_platform_remove,
+	.driver	= {
+		.name	= "vfio-platform",
+		.owner	= THIS_MODULE,
+	},
+};
+
+module_platform_driver(vfio_platform_driver);
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
new file mode 100644
index 0000000..4ae88f8
--- /dev/null
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -0,0 +1,22 @@
+/*
+ * Copyright (C) 2013 - Virtual Open Systems
+ * Author: Antonios Motakis <a.motakis@virtualopensystems.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, 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.
+ */
+
+#ifndef VFIO_PLATFORM_PRIVATE_H
+#define VFIO_PLATFORM_PRIVATE_H
+
+struct vfio_platform_device {
+	struct platform_device		*pdev;
+};
+
+#endif /* VFIO_PLATFORM_PRIVATE_H */
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 0847b29..b1a8de8 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -155,6 +155,7 @@ struct vfio_device_info {
 	__u32	flags;
 #define VFIO_DEVICE_FLAGS_RESET	(1 << 0)	/* Device supports reset */
 #define VFIO_DEVICE_FLAGS_PCI	(1 << 1)	/* vfio-pci device */
+#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)	/* vfio-platform device */
 	__u32	num_regions;	/* Max region index + 1 */
 	__u32	num_irqs;	/* Max IRQ index + 1 */
 };
-- 
1.8.3.2

Re: [RFC PATCH v5 05/11] VFIO_PLATFORM: Initial skeleton of VFIO support for platform devices

[Antonios Motakis]

On Mon, Apr 28, 2014 at 7:04 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
>> This patch forms the skeleton for platform devices support with VFIO.
>>
>> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>> ---
>>  drivers/vfio/Kconfig                          |   1 +
>>  drivers/vfio/Makefile                         |   1 +
>>  drivers/vfio/platform/Kconfig                 |   9 ++
>>  drivers/vfio/platform/Makefile                |   4 +
>>  drivers/vfio/platform/vfio_platform.c         | 172 ++++++++++++++++++++++++++
>>  drivers/vfio/platform/vfio_platform_private.h |  22 ++++
>>  include/uapi/linux/vfio.h                     |   1 +
>>  7 files changed, 210 insertions(+)
>>  create mode 100644 drivers/vfio/platform/Kconfig
>>  create mode 100644 drivers/vfio/platform/Makefile
>>  create mode 100644 drivers/vfio/platform/vfio_platform.c
>>  create mode 100644 drivers/vfio/platform/vfio_platform_private.h
>>
>> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
>> index 3a598ed..a484d72 100644
>> --- a/drivers/vfio/Kconfig
>> +++ b/drivers/vfio/Kconfig
>> @@ -21,3 +21,4 @@ menuconfig VFIO
>>         If you don't know what to do here, say N.
>>
>>  source "drivers/vfio/pci/Kconfig"
>> +source "drivers/vfio/platform/Kconfig"
>> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
>> index 72bfabc..b5e4a33 100644
>> --- a/drivers/vfio/Makefile
>> +++ b/drivers/vfio/Makefile
>> @@ -2,3 +2,4 @@ obj-$(CONFIG_VFIO) += vfio.o
>>  obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
>>  obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
>>  obj-$(CONFIG_VFIO_PCI) += pci/
>> +obj-$(CONFIG_VFIO_PLATFORM) += platform/
>> diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
>> new file mode 100644
>> index 0000000..42b0022
>> --- /dev/null
>> +++ b/drivers/vfio/platform/Kconfig
>> @@ -0,0 +1,9 @@
>> +config VFIO_PLATFORM
>> +     tristate "VFIO support for platform devices"
>> +     depends on VFIO && EVENTFD
>
> VFIO_PCI also depends on PCI, is there no CONFIG_PLATFORM?  What benefit
> would I get from this if I enable it on my x86 box?

That's a good point. Unfortunately I don't know of an obvious config
we can depend on, other than known target platforms (i.e. ARM).

Useless on an x86 PC, but maybe someone has a weird x86 based non-PC
based architecture somewhere :)

>
>> +     help
>> +       Support for platform devices with VFIO. This is required to make
>> +       use of platform devices present on the system using the VFIO
>> +       framework.
>> +
>> +       If you don't know what to do here, say N.
>> diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
>> new file mode 100644
>> index 0000000..df3a014
>> --- /dev/null
>> +++ b/drivers/vfio/platform/Makefile
>> @@ -0,0 +1,4 @@
>> +
>> +vfio-platform-y := vfio_platform.o
>> +
>> +obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
>> diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
>> new file mode 100644
>> index 0000000..1661746
>> --- /dev/null
>> +++ b/drivers/vfio/platform/vfio_platform.c
>> @@ -0,0 +1,172 @@
>> +/*
>> + * Copyright (C) 2013 - Virtual Open Systems
>> + * Author: Antonios Motakis <a.motakis@virtualopensystems.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License, 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/device.h>
>> +#include <linux/eventfd.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/iommu.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/notifier.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/vfio.h>
>> +#include <linux/io.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/irq.h>
>> +
>> +#include "vfio_platform_private.h"
>> +
>> +#define DRIVER_VERSION  "0.5"
>> +#define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
>> +#define DRIVER_DESC     "VFIO for platform devices - User Level meta-driver"
>> +
>> +static void vfio_platform_release(void *device_data)
>> +{
>> +     module_put(THIS_MODULE);
>> +}
>> +
>> +static int vfio_platform_open(void *device_data)
>> +{
>> +     if (!try_module_get(THIS_MODULE))
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>> +static long vfio_platform_ioctl(void *device_data,
>> +                        unsigned int cmd, unsigned long arg)
>> +{
>> +     struct vfio_platform_device *vdev = device_data;
>> +     unsigned long minsz;
>> +
>> +     if (cmd == VFIO_DEVICE_GET_INFO) {
>> +             struct vfio_device_info info;
>> +
>> +             minsz = offsetofend(struct vfio_device_info, num_irqs);
>> +
>> +             if (copy_from_user(&info, (void __user *)arg, minsz))
>> +                     return -EFAULT;
>> +
>> +             if (info.argsz < minsz)
>> +                     return -EINVAL;
>> +
>> +             info.flags = VFIO_DEVICE_FLAGS_PLATFORM;
>> +             info.num_regions = 0;
>> +             info.num_irqs = 0;
>> +
>> +             return copy_to_user((void __user *)arg, &info, minsz);
>> +
>> +     } else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
>> +             return -EINVAL;
>> +
>> +     else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
>> +             return -EINVAL;
>> +
>> +     else if (cmd == VFIO_DEVICE_SET_IRQS)
>> +             return -EINVAL;
>> +
>> +     else if (cmd == VFIO_DEVICE_RESET)
>> +             return -EINVAL;
>> +
>> +     return -ENOTTY;
>> +}
>> +
>> +static ssize_t vfio_platform_read(void *device_data, char __user *buf,
>> +                          size_t count, loff_t *ppos)
>> +{
>> +     return 0;
>> +}
>> +
>> +static ssize_t vfio_platform_write(void *device_data, const char __user *buf,
>> +                           size_t count, loff_t *ppos)
>> +{
>> +     return 0;
>> +}
>
> I know these are just stubs, but I suspect they should probably return
> error.  Thanks,
>

Ok!

> Alex
>
>> +
>> +static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
>> +{
>> +     return -EINVAL;
>> +}
>> +
>> +static const struct vfio_device_ops vfio_platform_ops = {
>> +     .name           = "vfio-platform",
>> +     .open           = vfio_platform_open,
>> +     .release        = vfio_platform_release,
>> +     .ioctl          = vfio_platform_ioctl,
>> +     .read           = vfio_platform_read,
>> +     .write          = vfio_platform_write,
>> +     .mmap           = vfio_platform_mmap,
>> +};
>> +
>> +static int vfio_platform_probe(struct platform_device *pdev)
>> +{
>> +     struct vfio_platform_device *vdev;
>> +     struct iommu_group *group;
>> +     int ret;
>> +
>> +     group = iommu_group_get(&pdev->dev);
>> +     if (!group) {
>> +             pr_err("VFIO: No IOMMU group for device %s\n", pdev->name);
>> +             return -EINVAL;
>> +     }
>> +
>> +     vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
>> +     if (!vdev) {
>> +             iommu_group_put(group);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     vdev->pdev = pdev;
>> +
>> +     ret = vfio_add_group_dev(&pdev->dev, &vfio_platform_ops, vdev);
>> +     if (ret) {
>> +             iommu_group_put(group);
>> +             kfree(vdev);
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +static int vfio_platform_remove(struct platform_device *pdev)
>> +{
>> +     struct vfio_platform_device *vdev;
>> +
>> +     vdev = vfio_del_group_dev(&pdev->dev);
>> +     if (!vdev)
>> +             return -EINVAL;
>> +
>> +     iommu_group_put(pdev->dev.iommu_group);
>> +     kfree(vdev);
>> +
>> +     return 0;
>> +}
>> +
>> +static struct platform_driver vfio_platform_driver = {
>> +     .probe          = vfio_platform_probe,
>> +     .remove         = vfio_platform_remove,
>> +     .driver = {
>> +             .name   = "vfio-platform",
>> +             .owner  = THIS_MODULE,
>> +     },
>> +};
>> +
>> +module_platform_driver(vfio_platform_driver);
>> +
>> +MODULE_VERSION(DRIVER_VERSION);
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR(DRIVER_AUTHOR);
>> +MODULE_DESCRIPTION(DRIVER_DESC);
>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
>> new file mode 100644
>> index 0000000..4ae88f8
>> --- /dev/null
>> +++ b/drivers/vfio/platform/vfio_platform_private.h
>> @@ -0,0 +1,22 @@
>> +/*
>> + * Copyright (C) 2013 - Virtual Open Systems
>> + * Author: Antonios Motakis <a.motakis@virtualopensystems.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License, 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.
>> + */
>> +
>> +#ifndef VFIO_PLATFORM_PRIVATE_H
>> +#define VFIO_PLATFORM_PRIVATE_H
>> +
>> +struct vfio_platform_device {
>> +     struct platform_device          *pdev;
>> +};
>> +
>> +#endif /* VFIO_PLATFORM_PRIVATE_H */
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 0847b29..b1a8de8 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -155,6 +155,7 @@ struct vfio_device_info {
>>       __u32   flags;
>>  #define VFIO_DEVICE_FLAGS_RESET      (1 << 0)        /* Device supports reset */
>>  #define VFIO_DEVICE_FLAGS_PCI        (1 << 1)        /* vfio-pci device */
>> +#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)  /* vfio-platform device */
>>       __u32   num_regions;    /* Max region index + 1 */
>>       __u32   num_irqs;       /* Max IRQ index + 1 */
>>  };
>
>
>



-- 
Antonios Motakis
Virtual Open Systems

[RFC PATCH v5 06/11] VFIO_PLATFORM: Return info for device and its memory mapped IO regions

[Antonios Motakis]

A VFIO userspace driver will start by opening the VFIO device
that corresponds to an IOMMU group, and will use the ioctl interface
to get the basic device info, such as number of memory regions and
interrupts, and their properties.

This patch enables the IOCTLs:
 - VFIO_DEVICE_GET_INFO
 - VFIO_DEVICE_GET_REGION_INFO

 IRQ info is provided by one of the latter patches.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform.c         | 77 ++++++++++++++++++++++++---
 drivers/vfio/platform/vfio_platform_private.h | 17 ++++++
 2 files changed, 88 insertions(+), 6 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index 1661746..5430cbe 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -34,15 +34,62 @@
 #define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
 #define DRIVER_DESC     "VFIO for platform devices - User Level meta-driver"
 
+static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
+{
+	int cnt = 0, i;
+
+	while (platform_get_resource(vdev->pdev, IORESOURCE_MEM, cnt))
+		cnt++;
+
+	vdev->num_regions = cnt;
+
+	vdev->region = kzalloc(sizeof(struct vfio_platform_region) * cnt,
+				GFP_KERNEL);
+	if (!vdev->region)
+		return -ENOMEM;
+
+	for (i = 0; i < cnt;  i++) {
+		struct vfio_platform_region region;
+		struct resource *res =
+			platform_get_resource(vdev->pdev, IORESOURCE_MEM, i);
+
+		region.addr = res->start;
+		region.size = resource_size(res);
+		region.flags = 0;
+
+		vdev->region[i] = region;
+	}
+
+	return 0;
+}
+
+static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
+{
+	kfree(vdev->region);
+}
+
 static void vfio_platform_release(void *device_data)
 {
+	struct vfio_platform_device *vdev = device_data;
+
+	vfio_platform_regions_cleanup(vdev);
+
 	module_put(THIS_MODULE);
 }
 
 static int vfio_platform_open(void *device_data)
 {
-	if (!try_module_get(THIS_MODULE))
+	struct vfio_platform_device *vdev = device_data;
+	int ret;
+
+	ret = vfio_platform_regions_init(vdev);
+	if (ret)
+		return ret;
+
+	if (!try_module_get(THIS_MODULE)) {
+		vfio_platform_regions_cleanup(vdev);
 		return -ENODEV;
+	}
 
 	return 0;
 }
@@ -65,18 +112,36 @@ static long vfio_platform_ioctl(void *device_data,
 			return -EINVAL;
 
 		info.flags = VFIO_DEVICE_FLAGS_PLATFORM;
-		info.num_regions = 0;
+		info.num_regions = vdev->num_regions;
 		info.num_irqs = 0;
 
 		return copy_to_user((void __user *)arg, &info, minsz);
 
-	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
-		return -EINVAL;
+	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
+		struct vfio_region_info info;
+
+		minsz = offsetofend(struct vfio_region_info, offset);
+
+		if (copy_from_user(&info, (void __user *)arg, minsz))
+			return -EFAULT;
+
+		if (info.argsz < minsz)
+			return -EINVAL;
+
+		if (info.index >= vdev->num_regions)
+			return -EINVAL;
+
+		/* map offset to the physical address  */
+		info.offset = VFIO_PLATFORM_INDEX_TO_OFFSET(info.index);
+		info.size = vdev->region[info.index].size;
+		info.flags = vdev->region[info.index].flags;
+
+		return copy_to_user((void __user *)arg, &info, minsz);
 
-	else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
+	} else if (cmd == VFIO_DEVICE_GET_IRQ_INFO) {
 		return -EINVAL;
 
-	else if (cmd == VFIO_DEVICE_SET_IRQS)
+	} else if (cmd == VFIO_DEVICE_SET_IRQS)
 		return -EINVAL;
 
 	else if (cmd == VFIO_DEVICE_RESET)
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 4ae88f8..3448f918 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -15,8 +15,25 @@
 #ifndef VFIO_PLATFORM_PRIVATE_H
 #define VFIO_PLATFORM_PRIVATE_H
 
+#define VFIO_PLATFORM_OFFSET_SHIFT   40
+#define VFIO_PLATFORM_OFFSET_MASK (((u64)(1) << VFIO_PLATFORM_OFFSET_SHIFT) - 1)
+
+#define VFIO_PLATFORM_OFFSET_TO_INDEX(off)	\
+	(off >> VFIO_PLATFORM_OFFSET_SHIFT)
+
+#define VFIO_PLATFORM_INDEX_TO_OFFSET(index)	\
+	((u64)(index) << VFIO_PLATFORM_OFFSET_SHIFT)
+
+struct vfio_platform_region {
+	u64			addr;
+	resource_size_t		size;
+	u32			flags;
+};
+
 struct vfio_platform_device {
 	struct platform_device		*pdev;
+	struct vfio_platform_region	*region;
+	u32				num_regions;
 };
 
 #endif /* VFIO_PLATFORM_PRIVATE_H */
-- 
1.8.3.2

Re: [RFC PATCH v5 06/11] VFIO_PLATFORM: Return info for device and its memory mapped IO regions

[Alex Williamson]

On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
> A VFIO userspace driver will start by opening the VFIO device
> that corresponds to an IOMMU group, and will use the ioctl interface
> to get the basic device info, such as number of memory regions and
> interrupts, and their properties.
> 
> This patch enables the IOCTLs:
>  - VFIO_DEVICE_GET_INFO
>  - VFIO_DEVICE_GET_REGION_INFO
> 
>  IRQ info is provided by one of the latter patches.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/vfio/platform/vfio_platform.c         | 77 ++++++++++++++++++++++++---
>  drivers/vfio/platform/vfio_platform_private.h | 17 ++++++
>  2 files changed, 88 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
> index 1661746..5430cbe 100644
> --- a/drivers/vfio/platform/vfio_platform.c
> +++ b/drivers/vfio/platform/vfio_platform.c
> @@ -34,15 +34,62 @@
>  #define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
>  #define DRIVER_DESC     "VFIO for platform devices - User Level meta-driver"
>  
> +static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
> +{
> +	int cnt = 0, i;
> +
> +	while (platform_get_resource(vdev->pdev, IORESOURCE_MEM, cnt))
> +		cnt++;
> +
> +	vdev->num_regions = cnt;
> +
> +	vdev->region = kzalloc(sizeof(struct vfio_platform_region) * cnt,
> +				GFP_KERNEL);
> +	if (!vdev->region)

Should vdev->num_regions be cleared here or set at the end to avoid
possibly walking a null pointer later?

> +		return -ENOMEM;
> +
> +	for (i = 0; i < cnt;  i++) {
> +		struct vfio_platform_region region;
> +		struct resource *res =
> +			platform_get_resource(vdev->pdev, IORESOURCE_MEM, i);
> +
> +		region.addr = res->start;
> +		region.size = resource_size(res);
> +		region.flags = 0;
> +
> +		vdev->region[i] = region;

nit, the local variable with copy at the end seems rather unnecessary
here.

> +	}
> +
> +	return 0;
> +}
> +
> +static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
> +{
> +	kfree(vdev->region);

Makes me nervous again that we have vdev->num_regions still set to a
value.  Maybe just paranoia.

> +}
> +
>  static void vfio_platform_release(void *device_data)
>  {
> +	struct vfio_platform_device *vdev = device_data;
> +
> +	vfio_platform_regions_cleanup(vdev);
> +
>  	module_put(THIS_MODULE);
>  }
>  
>  static int vfio_platform_open(void *device_data)
>  {
> -	if (!try_module_get(THIS_MODULE))
> +	struct vfio_platform_device *vdev = device_data;
> +	int ret;
> +
> +	ret = vfio_platform_regions_init(vdev);
> +	if (ret)
> +		return ret;
> +
> +	if (!try_module_get(THIS_MODULE)) {
> +		vfio_platform_regions_cleanup(vdev);
>  		return -ENODEV;
> +	}

Getting a reference to the module seems like it should be step 1 here.
Thanks,

Alex

>  
>  	return 0;
>  }
> @@ -65,18 +112,36 @@ static long vfio_platform_ioctl(void *device_data,
>  			return -EINVAL;
>  
>  		info.flags = VFIO_DEVICE_FLAGS_PLATFORM;
> -		info.num_regions = 0;
> +		info.num_regions = vdev->num_regions;
>  		info.num_irqs = 0;
>  
>  		return copy_to_user((void __user *)arg, &info, minsz);
>  
> -	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
> -		return -EINVAL;
> +	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
> +		struct vfio_region_info info;
> +
> +		minsz = offsetofend(struct vfio_region_info, offset);
> +
> +		if (copy_from_user(&info, (void __user *)arg, minsz))
> +			return -EFAULT;
> +
> +		if (info.argsz < minsz)
> +			return -EINVAL;
> +
> +		if (info.index >= vdev->num_regions)
> +			return -EINVAL;
> +
> +		/* map offset to the physical address  */
> +		info.offset = VFIO_PLATFORM_INDEX_TO_OFFSET(info.index);
> +		info.size = vdev->region[info.index].size;
> +		info.flags = vdev->region[info.index].flags;
> +
> +		return copy_to_user((void __user *)arg, &info, minsz);
>  
> -	else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
> +	} else if (cmd == VFIO_DEVICE_GET_IRQ_INFO) {
>  		return -EINVAL;
>  
> -	else if (cmd == VFIO_DEVICE_SET_IRQS)
> +	} else if (cmd == VFIO_DEVICE_SET_IRQS)
>  		return -EINVAL;
>  
>  	else if (cmd == VFIO_DEVICE_RESET)
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 4ae88f8..3448f918 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -15,8 +15,25 @@
>  #ifndef VFIO_PLATFORM_PRIVATE_H
>  #define VFIO_PLATFORM_PRIVATE_H
>  
> +#define VFIO_PLATFORM_OFFSET_SHIFT   40
> +#define VFIO_PLATFORM_OFFSET_MASK (((u64)(1) << VFIO_PLATFORM_OFFSET_SHIFT) - 1)
> +
> +#define VFIO_PLATFORM_OFFSET_TO_INDEX(off)	\
> +	(off >> VFIO_PLATFORM_OFFSET_SHIFT)
> +
> +#define VFIO_PLATFORM_INDEX_TO_OFFSET(index)	\
> +	((u64)(index) << VFIO_PLATFORM_OFFSET_SHIFT)
> +
> +struct vfio_platform_region {
> +	u64			addr;
> +	resource_size_t		size;
> +	u32			flags;
> +};
> +
>  struct vfio_platform_device {
>  	struct platform_device		*pdev;
> +	struct vfio_platform_region	*region;
> +	u32				num_regions;
>  };
>  
>  #endif /* VFIO_PLATFORM_PRIVATE_H */

[RFC PATCH v5 07/11] VFIO_PLATFORM: Read and write support for the device fd

[Antonios Motakis]

VFIO returns a file descriptor which we can use to manipulate the memory
regions of the device. Since some memory regions we cannot mmap due to
security concerns, we also allow to read and write to this file descriptor
directly.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform.c | 110 +++++++++++++++++++++++++++++++++-
 1 file changed, 107 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index 5430cbe..7c01ced 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -55,7 +55,8 @@ static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
 
 		region.addr = res->start;
 		region.size = resource_size(res);
-		region.flags = 0;
+		region.flags = VFIO_REGION_INFO_FLAG_READ
+				| VFIO_REGION_INFO_FLAG_WRITE;
 
 		vdev->region[i] = region;
 	}
@@ -153,13 +154,116 @@ static long vfio_platform_ioctl(void *device_data,
 static ssize_t vfio_platform_read(void *device_data, char __user *buf,
 			     size_t count, loff_t *ppos)
 {
-	return 0;
+	struct vfio_platform_device *vdev = device_data;
+	unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
+	loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
+	unsigned int done = 0;
+	void __iomem *io;
+
+	if (index >= vdev->num_regions)
+		return -EINVAL;
+
+	io = ioremap_nocache(vdev->region[index].addr,
+			     vdev->region[index].size);
+
+	while (count) {
+		size_t filled;
+
+		if (count >= 4 && !(off % 4)) {
+			u32 val;
+
+			val = ioread32(io + off);
+			if (copy_to_user(buf, &val, 4))
+				goto err;
+
+			filled = 4;
+		} else if (count >= 2 && !(off % 2)) {
+			u16 val;
+
+			val = ioread16(io + off);
+			if (copy_to_user(buf, &val, 2))
+				goto err;
+
+			filled = 2;
+		} else {
+			u8 val;
+
+			val = ioread8(io + off);
+			if (copy_to_user(buf, &val, 1))
+				goto err;
+
+			filled = 1;
+		}
+
+
+		count -= filled;
+		done += filled;
+		off += filled;
+		buf += filled;
+	}
+
+	iounmap(io);
+	return done;
+err:
+	iounmap(io);
+	return -EFAULT;
 }
 
 static ssize_t vfio_platform_write(void *device_data, const char __user *buf,
 			      size_t count, loff_t *ppos)
 {
-	return 0;
+	struct vfio_platform_device *vdev = device_data;
+	unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
+	loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
+	unsigned int done = 0;
+	void __iomem *io;
+
+	if (index >= vdev->num_regions)
+		return -EINVAL;
+
+	io = ioremap_nocache(vdev->region[index].addr,
+			     vdev->region[index].size);
+
+	while (count) {
+		size_t filled;
+
+		if (count >= 4 && !(off % 4)) {
+			u32 val;
+
+			if (copy_from_user(&val, buf, 4))
+				goto err;
+			iowrite32(val, io + off);
+
+			filled = 4;
+		} else if (count >= 2 && !(off % 2)) {
+			u16 val;
+
+			if (copy_from_user(&val, buf, 2))
+				goto err;
+			iowrite16(val, io + off);
+
+			filled = 2;
+		} else {
+			u8 val;
+
+			if (copy_from_user(&val, buf, 1))
+				goto err;
+			iowrite8(val, io + off);
+
+			filled = 1;
+		}
+
+		count -= filled;
+		done += filled;
+		off += filled;
+		buf += filled;
+	}
+
+	iounmap(io);
+	return done;
+err:
+	iounmap(io);
+	return -EFAULT;
 }
 
 static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
-- 
1.8.3.2

Re: [RFC PATCH v5 07/11] VFIO_PLATFORM: Read and write support for the device fd

[Alex Williamson]

On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
> VFIO returns a file descriptor which we can use to manipulate the memory
> regions of the device. Since some memory regions we cannot mmap due to
> security concerns, we also allow to read and write to this file descriptor
> directly.

If there are regions we cannot mmap for security, why do we provide full
read/write access to them?

> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/vfio/platform/vfio_platform.c | 110 +++++++++++++++++++++++++++++++++-
>  1 file changed, 107 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
> index 5430cbe..7c01ced 100644
> --- a/drivers/vfio/platform/vfio_platform.c
> +++ b/drivers/vfio/platform/vfio_platform.c
> @@ -55,7 +55,8 @@ static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>  
>  		region.addr = res->start;
>  		region.size = resource_size(res);
> -		region.flags = 0;
> +		region.flags = VFIO_REGION_INFO_FLAG_READ
> +				| VFIO_REGION_INFO_FLAG_WRITE;
>  
>  		vdev->region[i] = region;
>  	}
> @@ -153,13 +154,116 @@ static long vfio_platform_ioctl(void *device_data,
>  static ssize_t vfio_platform_read(void *device_data, char __user *buf,
>  			     size_t count, loff_t *ppos)
>  {
> -	return 0;
> +	struct vfio_platform_device *vdev = device_data;
> +	unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
> +	loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
> +	unsigned int done = 0;
> +	void __iomem *io;
> +
> +	if (index >= vdev->num_regions)
> +		return -EINVAL;
> +
> +	io = ioremap_nocache(vdev->region[index].addr,
> +			     vdev->region[index].size);

I haven't looked at ioremap on arm, but if it's remotely non-trivially,
it's probably a good idea to do this only on first access, cache it, and
unmap it on region cleanup.  Thanks,

Alex

> +
> +	while (count) {
> +		size_t filled;
> +
> +		if (count >= 4 && !(off % 4)) {
> +			u32 val;
> +
> +			val = ioread32(io + off);
> +			if (copy_to_user(buf, &val, 4))
> +				goto err;
> +
> +			filled = 4;
> +		} else if (count >= 2 && !(off % 2)) {
> +			u16 val;
> +
> +			val = ioread16(io + off);
> +			if (copy_to_user(buf, &val, 2))
> +				goto err;
> +
> +			filled = 2;
> +		} else {
> +			u8 val;
> +
> +			val = ioread8(io + off);
> +			if (copy_to_user(buf, &val, 1))
> +				goto err;
> +
> +			filled = 1;
> +		}
> +
> +
> +		count -= filled;
> +		done += filled;
> +		off += filled;
> +		buf += filled;
> +	}
> +
> +	iounmap(io);
> +	return done;
> +err:
> +	iounmap(io);
> +	return -EFAULT;
>  }
>  
>  static ssize_t vfio_platform_write(void *device_data, const char __user *buf,
>  			      size_t count, loff_t *ppos)
>  {
> -	return 0;
> +	struct vfio_platform_device *vdev = device_data;
> +	unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
> +	loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
> +	unsigned int done = 0;
> +	void __iomem *io;
> +
> +	if (index >= vdev->num_regions)
> +		return -EINVAL;
> +
> +	io = ioremap_nocache(vdev->region[index].addr,
> +			     vdev->region[index].size);
> +
> +	while (count) {
> +		size_t filled;
> +
> +		if (count >= 4 && !(off % 4)) {
> +			u32 val;
> +
> +			if (copy_from_user(&val, buf, 4))
> +				goto err;
> +			iowrite32(val, io + off);
> +
> +			filled = 4;
> +		} else if (count >= 2 && !(off % 2)) {
> +			u16 val;
> +
> +			if (copy_from_user(&val, buf, 2))
> +				goto err;
> +			iowrite16(val, io + off);
> +
> +			filled = 2;
> +		} else {
> +			u8 val;
> +
> +			if (copy_from_user(&val, buf, 1))
> +				goto err;
> +			iowrite8(val, io + off);
> +
> +			filled = 1;
> +		}
> +
> +		count -= filled;
> +		done += filled;
> +		off += filled;
> +		buf += filled;
> +	}
> +
> +	iounmap(io);
> +	return done;
> +err:
> +	iounmap(io);
> +	return -EFAULT;
>  }
>  
>  static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)

Re: [RFC PATCH v5 07/11] VFIO_PLATFORM: Read and write support for the device fd

[Antonios Motakis]

On Mon, Apr 28, 2014 at 7:24 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
>> VFIO returns a file descriptor which we can use to manipulate the memory
>> regions of the device. Since some memory regions we cannot mmap due to
>> security concerns, we also allow to read and write to this file descriptor
>> directly.
>
> If there are regions we cannot mmap for security, why do we provide full
> read/write access to them?

What I refer to here is to the problem of mmaping an MMIO region that
is not page-sized. For example if it is just a small number of
registers which are not aligned on a page boundary, mmaping them would
still make the whole page accessible to the guest, with unpredictable
outcomes. But in these cases read/write is still OK.

>
>> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>> ---
>>  drivers/vfio/platform/vfio_platform.c | 110 +++++++++++++++++++++++++++++++++-
>>  1 file changed, 107 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
>> index 5430cbe..7c01ced 100644
>> --- a/drivers/vfio/platform/vfio_platform.c
>> +++ b/drivers/vfio/platform/vfio_platform.c
>> @@ -55,7 +55,8 @@ static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>>
>>               region.addr = res->start;
>>               region.size = resource_size(res);
>> -             region.flags = 0;
>> +             region.flags = VFIO_REGION_INFO_FLAG_READ
>> +                             | VFIO_REGION_INFO_FLAG_WRITE;
>>
>>               vdev->region[i] = region;
>>       }
>> @@ -153,13 +154,116 @@ static long vfio_platform_ioctl(void *device_data,
>>  static ssize_t vfio_platform_read(void *device_data, char __user *buf,
>>                            size_t count, loff_t *ppos)
>>  {
>> -     return 0;
>> +     struct vfio_platform_device *vdev = device_data;
>> +     unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
>> +     loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
>> +     unsigned int done = 0;
>> +     void __iomem *io;
>> +
>> +     if (index >= vdev->num_regions)
>> +             return -EINVAL;
>> +
>> +     io = ioremap_nocache(vdev->region[index].addr,
>> +                          vdev->region[index].size);
>
> I haven't looked at ioremap on arm, but if it's remotely non-trivially,
> it's probably a good idea to do this only on first access, cache it, and
> unmap it on region cleanup.  Thanks,

Ack.

>
> Alex
>
>> +
>> +     while (count) {
>> +             size_t filled;
>> +
>> +             if (count >= 4 && !(off % 4)) {
>> +                     u32 val;
>> +
>> +                     val = ioread32(io + off);
>> +                     if (copy_to_user(buf, &val, 4))
>> +                             goto err;
>> +
>> +                     filled = 4;
>> +             } else if (count >= 2 && !(off % 2)) {
>> +                     u16 val;
>> +
>> +                     val = ioread16(io + off);
>> +                     if (copy_to_user(buf, &val, 2))
>> +                             goto err;
>> +
>> +                     filled = 2;
>> +             } else {
>> +                     u8 val;
>> +
>> +                     val = ioread8(io + off);
>> +                     if (copy_to_user(buf, &val, 1))
>> +                             goto err;
>> +
>> +                     filled = 1;
>> +             }
>> +
>> +
>> +             count -= filled;
>> +             done += filled;
>> +             off += filled;
>> +             buf += filled;
>> +     }
>> +
>> +     iounmap(io);
>> +     return done;
>> +err:
>> +     iounmap(io);
>> +     return -EFAULT;
>>  }
>>
>>  static ssize_t vfio_platform_write(void *device_data, const char __user *buf,
>>                             size_t count, loff_t *ppos)
>>  {
>> -     return 0;
>> +     struct vfio_platform_device *vdev = device_data;
>> +     unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
>> +     loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
>> +     unsigned int done = 0;
>> +     void __iomem *io;
>> +
>> +     if (index >= vdev->num_regions)
>> +             return -EINVAL;
>> +
>> +     io = ioremap_nocache(vdev->region[index].addr,
>> +                          vdev->region[index].size);
>> +
>> +     while (count) {
>> +             size_t filled;
>> +
>> +             if (count >= 4 && !(off % 4)) {
>> +                     u32 val;
>> +
>> +                     if (copy_from_user(&val, buf, 4))
>> +                             goto err;
>> +                     iowrite32(val, io + off);
>> +
>> +                     filled = 4;
>> +             } else if (count >= 2 && !(off % 2)) {
>> +                     u16 val;
>> +
>> +                     if (copy_from_user(&val, buf, 2))
>> +                             goto err;
>> +                     iowrite16(val, io + off);
>> +
>> +                     filled = 2;
>> +             } else {
>> +                     u8 val;
>> +
>> +                     if (copy_from_user(&val, buf, 1))
>> +                             goto err;
>> +                     iowrite8(val, io + off);
>> +
>> +                     filled = 1;
>> +             }
>> +
>> +             count -= filled;
>> +             done += filled;
>> +             off += filled;
>> +             buf += filled;
>> +     }
>> +
>> +     iounmap(io);
>> +     return done;
>> +err:
>> +     iounmap(io);
>> +     return -EFAULT;
>>  }
>>
>>  static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
>
>
>



-- 
Antonios Motakis
Virtual Open Systems

[RFC PATCH v5 08/11] VFIO_PLATFORM: Support MMAP of MMIO regions

[Antonios Motakis]

Allow to memory map the MMIO regions of the device so userspace can
directly access them.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform.c | 40 ++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index 7c01ced..37beff3 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -58,6 +58,11 @@ static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
 		region.flags = VFIO_REGION_INFO_FLAG_READ
 				| VFIO_REGION_INFO_FLAG_WRITE;
 
+		/* Only regions addressed with PAGE granularity can be MMAPed
+		 * securely. */
+		if (!(region.addr & ~PAGE_MASK) && !(region.size & ~PAGE_MASK))
+			region.flags |= VFIO_REGION_INFO_FLAG_MMAP;
+
 		vdev->region[i] = region;
 	}
 
@@ -268,7 +273,40 @@ err:
 
 static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
 {
-	return -EINVAL;
+	struct vfio_platform_device *vdev = device_data;
+	unsigned int index;
+	u64 req_len, pgoff, req_start;
+	struct vfio_platform_region region;
+
+	index = vma->vm_pgoff >> (VFIO_PLATFORM_OFFSET_SHIFT - PAGE_SHIFT);
+
+	if (vma->vm_end < vma->vm_start)
+		return -EINVAL;
+	if ((vma->vm_flags & VM_SHARED) == 0)
+		return -EINVAL;
+	if (index >= vdev->num_regions)
+		return -EINVAL;
+	if (vma->vm_start & ~PAGE_MASK)
+		return -EINVAL;
+	if (vma->vm_end & ~PAGE_MASK)
+		return -EINVAL;
+
+	region = vdev->region[index];
+
+	req_len = vma->vm_end - vma->vm_start;
+	pgoff = vma->vm_pgoff &
+		((1U << (VFIO_PLATFORM_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
+	req_start = pgoff << PAGE_SHIFT;
+
+	if (region.size < PAGE_SIZE || req_start + req_len > region.size)
+		return -EINVAL;
+
+	vma->vm_private_data = vdev;
+	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+	vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;
+
+	return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
+			       req_len, vma->vm_page_prot);
 }
 
 static const struct vfio_device_ops vfio_platform_ops = {
-- 
1.8.3.2

Re: [RFC PATCH v5 08/11] VFIO_PLATFORM: Support MMAP of MMIO regions

[Alex Williamson]

On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
> Allow to memory map the MMIO regions of the device so userspace can
> directly access them.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/vfio/platform/vfio_platform.c | 40 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
> index 7c01ced..37beff3 100644
> --- a/drivers/vfio/platform/vfio_platform.c
> +++ b/drivers/vfio/platform/vfio_platform.c
> @@ -58,6 +58,11 @@ static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>  		region.flags = VFIO_REGION_INFO_FLAG_READ
>  				| VFIO_REGION_INFO_FLAG_WRITE;
>  
> +		/* Only regions addressed with PAGE granularity can be MMAPed
> +		 * securely. */
> +		if (!(region.addr & ~PAGE_MASK) && !(region.size & ~PAGE_MASK))
> +			region.flags |= VFIO_REGION_INFO_FLAG_MMAP;
> +

Ok, so the security is only that we need page alignment.  Maybe that
could be made more clear in the previous patch.  PCI has regions like
the MSI-X table that we need to protect from userspace access beyond
page alignment requirements.  Thanks,

Alex

>  		vdev->region[i] = region;
>  	}
>  
> @@ -268,7 +273,40 @@ err:
>  
>  static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
>  {
> -	return -EINVAL;
> +	struct vfio_platform_device *vdev = device_data;
> +	unsigned int index;
> +	u64 req_len, pgoff, req_start;
> +	struct vfio_platform_region region;
> +
> +	index = vma->vm_pgoff >> (VFIO_PLATFORM_OFFSET_SHIFT - PAGE_SHIFT);
> +
> +	if (vma->vm_end < vma->vm_start)
> +		return -EINVAL;
> +	if ((vma->vm_flags & VM_SHARED) == 0)
> +		return -EINVAL;
> +	if (index >= vdev->num_regions)
> +		return -EINVAL;
> +	if (vma->vm_start & ~PAGE_MASK)
> +		return -EINVAL;
> +	if (vma->vm_end & ~PAGE_MASK)
> +		return -EINVAL;
> +
> +	region = vdev->region[index];
> +
> +	req_len = vma->vm_end - vma->vm_start;
> +	pgoff = vma->vm_pgoff &
> +		((1U << (VFIO_PLATFORM_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
> +	req_start = pgoff << PAGE_SHIFT;
> +
> +	if (region.size < PAGE_SIZE || req_start + req_len > region.size)
> +		return -EINVAL;
> +
> +	vma->vm_private_data = vdev;
> +	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +	vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;
> +
> +	return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> +			       req_len, vma->vm_page_prot);
>  }
>  
>  static const struct vfio_device_ops vfio_platform_ops = {

Re: [RFC PATCH v5 08/11] VFIO_PLATFORM: Support MMAP of MMIO regions

[Antonios Motakis]

On Mon, Apr 28, 2014 at 7:27 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
>> Allow to memory map the MMIO regions of the device so userspace can
>> directly access them.
>>
>> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>> ---
>>  drivers/vfio/platform/vfio_platform.c | 40 ++++++++++++++++++++++++++++++++++-
>>  1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
>> index 7c01ced..37beff3 100644
>> --- a/drivers/vfio/platform/vfio_platform.c
>> +++ b/drivers/vfio/platform/vfio_platform.c
>> @@ -58,6 +58,11 @@ static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>>               region.flags = VFIO_REGION_INFO_FLAG_READ
>>                               | VFIO_REGION_INFO_FLAG_WRITE;
>>
>> +             /* Only regions addressed with PAGE granularity can be MMAPed
>> +              * securely. */
>> +             if (!(region.addr & ~PAGE_MASK) && !(region.size & ~PAGE_MASK))
>> +                     region.flags |= VFIO_REGION_INFO_FLAG_MMAP;
>> +
>
> Ok, so the security is only that we need page alignment.  Maybe that
> could be made more clear in the previous patch.  PCI has regions like
> the MSI-X table that we need to protect from userspace access beyond
> page alignment requirements.  Thanks,

Ack, I will make the previous description more clear.

>
> Alex
>
>>               vdev->region[i] = region;
>>       }
>>
>> @@ -268,7 +273,40 @@ err:
>>
>>  static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
>>  {
>> -     return -EINVAL;
>> +     struct vfio_platform_device *vdev = device_data;
>> +     unsigned int index;
>> +     u64 req_len, pgoff, req_start;
>> +     struct vfio_platform_region region;
>> +
>> +     index = vma->vm_pgoff >> (VFIO_PLATFORM_OFFSET_SHIFT - PAGE_SHIFT);
>> +
>> +     if (vma->vm_end < vma->vm_start)
>> +             return -EINVAL;
>> +     if ((vma->vm_flags & VM_SHARED) == 0)
>> +             return -EINVAL;
>> +     if (index >= vdev->num_regions)
>> +             return -EINVAL;
>> +     if (vma->vm_start & ~PAGE_MASK)
>> +             return -EINVAL;
>> +     if (vma->vm_end & ~PAGE_MASK)
>> +             return -EINVAL;
>> +
>> +     region = vdev->region[index];
>> +
>> +     req_len = vma->vm_end - vma->vm_start;
>> +     pgoff = vma->vm_pgoff &
>> +             ((1U << (VFIO_PLATFORM_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
>> +     req_start = pgoff << PAGE_SHIFT;
>> +
>> +     if (region.size < PAGE_SIZE || req_start + req_len > region.size)
>> +             return -EINVAL;
>> +
>> +     vma->vm_private_data = vdev;
>> +     vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>> +     vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;
>> +
>> +     return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
>> +                            req_len, vma->vm_page_prot);
>>  }
>>
>>  static const struct vfio_device_ops vfio_platform_ops = {
>
>
>



-- 
Antonios Motakis
Virtual Open Systems

[RFC PATCH v5 09/11] VFIO_PLATFORM: Return IRQ info

[Antonios Motakis]

Return information for the interrupts exposed by the device.
This patch extends VFIO_DEVICE_GET_INFO with the number of IRQs
and enables VFIO_DEVICE_GET_IRQ_INFO

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/Makefile                |  2 +-
 drivers/vfio/platform/vfio_platform.c         | 35 +++++++++++++--
 drivers/vfio/platform/vfio_platform_irq.c     | 63 +++++++++++++++++++++++++++
 drivers/vfio/platform/vfio_platform_private.h | 11 +++++
 4 files changed, 106 insertions(+), 5 deletions(-)
 create mode 100644 drivers/vfio/platform/vfio_platform_irq.c

diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
index df3a014..2c53327 100644
--- a/drivers/vfio/platform/Makefile
+++ b/drivers/vfio/platform/Makefile
@@ -1,4 +1,4 @@
 
-vfio-platform-y := vfio_platform.o
+vfio-platform-y := vfio_platform.o vfio_platform_irq.o
 
 obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index 37beff3..2e16595 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -79,6 +79,7 @@ static void vfio_platform_release(void *device_data)
 	struct vfio_platform_device *vdev = device_data;
 
 	vfio_platform_regions_cleanup(vdev);
+	vfio_platform_irq_cleanup(vdev);
 
 	module_put(THIS_MODULE);
 }
@@ -92,12 +93,22 @@ static int vfio_platform_open(void *device_data)
 	if (ret)
 		return ret;
 
+	ret = vfio_platform_irq_init(vdev);
+	if (ret)
+		goto err_irq;
+
 	if (!try_module_get(THIS_MODULE)) {
-		vfio_platform_regions_cleanup(vdev);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto err_mod;
 	}
 
 	return 0;
+
+err_mod:
+	vfio_platform_irq_cleanup(vdev);
+err_irq:
+	vfio_platform_regions_cleanup(vdev);
+	return ret;
 }
 
 static long vfio_platform_ioctl(void *device_data,
@@ -119,7 +130,7 @@ static long vfio_platform_ioctl(void *device_data,
 
 		info.flags = VFIO_DEVICE_FLAGS_PLATFORM;
 		info.num_regions = vdev->num_regions;
-		info.num_irqs = 0;
+		info.num_irqs = vdev->num_irqs;
 
 		return copy_to_user((void __user *)arg, &info, minsz);
 
@@ -145,7 +156,23 @@ static long vfio_platform_ioctl(void *device_data,
 		return copy_to_user((void __user *)arg, &info, minsz);
 
 	} else if (cmd == VFIO_DEVICE_GET_IRQ_INFO) {
-		return -EINVAL;
+		struct vfio_irq_info info;
+
+		minsz = offsetofend(struct vfio_irq_info, count);
+
+		if (copy_from_user(&info, (void __user *)arg, minsz))
+			return -EFAULT;
+
+		if (info.argsz < minsz)
+			return -EINVAL;
+
+		if (info.index >= vdev->num_irqs)
+			return -EINVAL;
+
+		info.flags = vdev->irq[info.index].flags;
+		info.count = vdev->irq[info.index].count;
+
+		return copy_to_user((void __user *)arg, &info, minsz);
 
 	} else if (cmd == VFIO_DEVICE_SET_IRQS)
 		return -EINVAL;
diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
new file mode 100644
index 0000000..075c401
--- /dev/null
+++ b/drivers/vfio/platform/vfio_platform_irq.c
@@ -0,0 +1,63 @@
+/*
+ * VFIO platform devices interrupt handling
+ *
+ * Copyright (C) 2013 - Virtual Open Systems
+ * Author: Antonios Motakis <a.motakis@virtualopensystems.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, 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/device.h>
+#include <linux/eventfd.h>
+#include <linux/interrupt.h>
+#include <linux/iommu.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/vfio.h>
+#include <linux/platform_device.h>
+#include <linux/irq.h>
+
+#include "vfio_platform_private.h"
+
+int vfio_platform_irq_init(struct vfio_platform_device *vdev)
+{
+	int cnt = 0, i;
+
+	while (platform_get_irq(vdev->pdev, cnt) > 0)
+		cnt++;
+
+	vdev->num_irqs = cnt;
+
+	vdev->irq = kzalloc(sizeof(struct vfio_platform_irq) * vdev->num_irqs,
+				GFP_KERNEL);
+	if (!vdev->irq)
+		return -ENOMEM;
+
+	for (i = 0; i < cnt; i++) {
+		struct vfio_platform_irq irq;
+
+		irq.flags = 0;
+		irq.count = 1;
+
+		vdev->irq[i] = irq;
+	}
+
+	return 0;
+}
+
+void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev)
+{
+	kfree(vdev->irq);
+}
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 3448f918..9776cff 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -24,6 +24,11 @@
 #define VFIO_PLATFORM_INDEX_TO_OFFSET(index)	\
 	((u64)(index) << VFIO_PLATFORM_OFFSET_SHIFT)
 
+struct vfio_platform_irq {
+	u32			flags;
+	u32			count;
+};
+
 struct vfio_platform_region {
 	u64			addr;
 	resource_size_t		size;
@@ -34,6 +39,12 @@ struct vfio_platform_device {
 	struct platform_device		*pdev;
 	struct vfio_platform_region	*region;
 	u32				num_regions;
+	struct vfio_platform_irq	*irq;
+	u32				num_irqs;
 };
 
+extern int vfio_platform_irq_init(struct vfio_platform_device *vdev);
+
+extern void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev);
+
 #endif /* VFIO_PLATFORM_PRIVATE_H */
-- 
1.8.3.2

Re: [RFC PATCH v5 09/11] VFIO_PLATFORM: Return IRQ info

[Alex Williamson]

On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
> Return information for the interrupts exposed by the device.
> This patch extends VFIO_DEVICE_GET_INFO with the number of IRQs
> and enables VFIO_DEVICE_GET_IRQ_INFO
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/vfio/platform/Makefile                |  2 +-
>  drivers/vfio/platform/vfio_platform.c         | 35 +++++++++++++--
>  drivers/vfio/platform/vfio_platform_irq.c     | 63 +++++++++++++++++++++++++++
>  drivers/vfio/platform/vfio_platform_private.h | 11 +++++
>  4 files changed, 106 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/vfio/platform/vfio_platform_irq.c
> 
> diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
> index df3a014..2c53327 100644
> --- a/drivers/vfio/platform/Makefile
> +++ b/drivers/vfio/platform/Makefile
> @@ -1,4 +1,4 @@
>  
> -vfio-platform-y := vfio_platform.o
> +vfio-platform-y := vfio_platform.o vfio_platform_irq.o
>  
>  obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
> diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
> index 37beff3..2e16595 100644
> --- a/drivers/vfio/platform/vfio_platform.c
> +++ b/drivers/vfio/platform/vfio_platform.c
> @@ -79,6 +79,7 @@ static void vfio_platform_release(void *device_data)
>  	struct vfio_platform_device *vdev = device_data;
>  
>  	vfio_platform_regions_cleanup(vdev);
> +	vfio_platform_irq_cleanup(vdev);
>  
>  	module_put(THIS_MODULE);
>  }
> @@ -92,12 +93,22 @@ static int vfio_platform_open(void *device_data)
>  	if (ret)
>  		return ret;
>  
> +	ret = vfio_platform_irq_init(vdev);
> +	if (ret)
> +		goto err_irq;
> +
>  	if (!try_module_get(THIS_MODULE)) {
> -		vfio_platform_regions_cleanup(vdev);
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto err_mod;
>  	}


Same comments from REGION_INFO patch apply here.  Ordering above,
paranoia about keeping num_irqs consistent with allocation, nit about
local variable use in initialization loop.  Thanks,

Alex
>  
>  	return 0;
> +
> +err_mod:
> +	vfio_platform_irq_cleanup(vdev);
> +err_irq:
> +	vfio_platform_regions_cleanup(vdev);
> +	return ret;
>  }
>  
>  static long vfio_platform_ioctl(void *device_data,
> @@ -119,7 +130,7 @@ static long vfio_platform_ioctl(void *device_data,
>  
>  		info.flags = VFIO_DEVICE_FLAGS_PLATFORM;
>  		info.num_regions = vdev->num_regions;
> -		info.num_irqs = 0;
> +		info.num_irqs = vdev->num_irqs;
>  
>  		return copy_to_user((void __user *)arg, &info, minsz);
>  
> @@ -145,7 +156,23 @@ static long vfio_platform_ioctl(void *device_data,
>  		return copy_to_user((void __user *)arg, &info, minsz);
>  
>  	} else if (cmd == VFIO_DEVICE_GET_IRQ_INFO) {
> -		return -EINVAL;
> +		struct vfio_irq_info info;
> +
> +		minsz = offsetofend(struct vfio_irq_info, count);
> +
> +		if (copy_from_user(&info, (void __user *)arg, minsz))
> +			return -EFAULT;
> +
> +		if (info.argsz < minsz)
> +			return -EINVAL;
> +
> +		if (info.index >= vdev->num_irqs)
> +			return -EINVAL;
> +
> +		info.flags = vdev->irq[info.index].flags;
> +		info.count = vdev->irq[info.index].count;
> +
> +		return copy_to_user((void __user *)arg, &info, minsz);
>  
>  	} else if (cmd == VFIO_DEVICE_SET_IRQS)
>  		return -EINVAL;
> diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
> new file mode 100644
> index 0000000..075c401
> --- /dev/null
> +++ b/drivers/vfio/platform/vfio_platform_irq.c
> @@ -0,0 +1,63 @@
> +/*
> + * VFIO platform devices interrupt handling
> + *
> + * Copyright (C) 2013 - Virtual Open Systems
> + * Author: Antonios Motakis <a.motakis@virtualopensystems.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, 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/device.h>
> +#include <linux/eventfd.h>
> +#include <linux/interrupt.h>
> +#include <linux/iommu.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/notifier.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +#include <linux/vfio.h>
> +#include <linux/platform_device.h>
> +#include <linux/irq.h>
> +
> +#include "vfio_platform_private.h"
> +
> +int vfio_platform_irq_init(struct vfio_platform_device *vdev)
> +{
> +	int cnt = 0, i;
> +
> +	while (platform_get_irq(vdev->pdev, cnt) > 0)
> +		cnt++;
> +
> +	vdev->num_irqs = cnt;
> +
> +	vdev->irq = kzalloc(sizeof(struct vfio_platform_irq) * vdev->num_irqs,
> +				GFP_KERNEL);
> +	if (!vdev->irq)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < cnt; i++) {
> +		struct vfio_platform_irq irq;
> +
> +		irq.flags = 0;
> +		irq.count = 1;
> +
> +		vdev->irq[i] = irq;
> +	}
> +
> +	return 0;
> +}
> +
> +void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev)
> +{
> +	kfree(vdev->irq);
> +}
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 3448f918..9776cff 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -24,6 +24,11 @@
>  #define VFIO_PLATFORM_INDEX_TO_OFFSET(index)	\
>  	((u64)(index) << VFIO_PLATFORM_OFFSET_SHIFT)
>  
> +struct vfio_platform_irq {
> +	u32			flags;
> +	u32			count;
> +};
> +
>  struct vfio_platform_region {
>  	u64			addr;
>  	resource_size_t		size;
> @@ -34,6 +39,12 @@ struct vfio_platform_device {
>  	struct platform_device		*pdev;
>  	struct vfio_platform_region	*region;
>  	u32				num_regions;
> +	struct vfio_platform_irq	*irq;
> +	u32				num_irqs;
>  };
>  
> +extern int vfio_platform_irq_init(struct vfio_platform_device *vdev);
> +
> +extern void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev);
> +
>  #endif /* VFIO_PLATFORM_PRIVATE_H */

[RFC PATCH v5 10/11] VFIO_PLATFORM: Initial interrupts support

[Antonios Motakis]

This patch allows to set an eventfd for a patform device's interrupt,
and also to trigger the interrupt eventfd from userspace for testing.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform.c         |  36 +++++++-
 drivers/vfio/platform/vfio_platform_irq.c     | 123 +++++++++++++++++++++++++-
 drivers/vfio/platform/vfio_platform_private.h |   7 ++
 3 files changed, 162 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index 2e16595..8aa5d10 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -174,10 +174,40 @@ static long vfio_platform_ioctl(void *device_data,
 
 		return copy_to_user((void __user *)arg, &info, minsz);
 
-	} else if (cmd == VFIO_DEVICE_SET_IRQS)
-		return -EINVAL;
+	} else if (cmd == VFIO_DEVICE_SET_IRQS) {
+		struct vfio_irq_set hdr;
+		int ret = 0;
+
+		minsz = offsetofend(struct vfio_irq_set, count);
+
+		if (copy_from_user(&hdr, (void __user *)arg, minsz))
+			return -EFAULT;
+
+		if (hdr.argsz < minsz)
+			return -EINVAL;
+
+		if (hdr.index >= vdev->num_irqs)
+			return -EINVAL;
+
+		if (hdr.start != 0 || hdr.count > 1)
+			return -EINVAL;
+
+		if (hdr.count == 0 &&
+			(!(hdr.flags & VFIO_IRQ_SET_DATA_NONE) ||
+			 !(hdr.flags & VFIO_IRQ_SET_ACTION_TRIGGER)))
+			return -EINVAL;
+
+		if (hdr.flags & ~(VFIO_IRQ_SET_DATA_TYPE_MASK |
+				  VFIO_IRQ_SET_ACTION_TYPE_MASK))
+			return -EINVAL;
+
+		ret = vfio_platform_set_irqs_ioctl(vdev, hdr.flags, hdr.index,
+						   hdr.start, hdr.count,
+						   (void *)arg+minsz);
+
+		return ret;
 
-	else if (cmd == VFIO_DEVICE_RESET)
+	} else if (cmd == VFIO_DEVICE_RESET)
 		return -EINVAL;
 
 	return -ENOTTY;
diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
index 075c401..433edc1 100644
--- a/drivers/vfio/platform/vfio_platform_irq.c
+++ b/drivers/vfio/platform/vfio_platform_irq.c
@@ -31,6 +31,9 @@
 
 #include "vfio_platform_private.h"
 
+static int vfio_set_trigger(struct vfio_platform_device *vdev,
+			    int index, int fd);
+
 int vfio_platform_irq_init(struct vfio_platform_device *vdev)
 {
 	int cnt = 0, i;
@@ -47,9 +50,11 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev)
 
 	for (i = 0; i < cnt; i++) {
 		struct vfio_platform_irq irq;
+		int hwirq = platform_get_irq(vdev->pdev, i);
 
-		irq.flags = 0;
+		irq.flags = VFIO_IRQ_INFO_EVENTFD;
 		irq.count = 1;
+		irq.hwirq = hwirq;
 
 		vdev->irq[i] = irq;
 	}
@@ -59,5 +64,121 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev)
 
 void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev)
 {
+	int i;
+
+	for (i = 0; i < vdev->num_irqs; i++)
+		vfio_set_trigger(vdev, i, -1);
+
 	kfree(vdev->irq);
 }
+
+static irqreturn_t vfio_irq_handler(int irq, void *dev_id)
+{
+	struct eventfd_ctx *trigger = dev_id;
+
+	eventfd_signal(trigger, 1);
+
+	return IRQ_HANDLED;
+}
+
+static int vfio_set_trigger(struct vfio_platform_device *vdev,
+			    int index, int fd)
+{
+	struct vfio_platform_irq *irq = &vdev->irq[index];
+	struct eventfd_ctx *trigger;
+	int ret;
+
+	if (irq->trigger) {
+		free_irq(irq->hwirq, irq);
+		kfree(irq->name);
+		eventfd_ctx_put(irq->trigger);
+		irq->trigger = NULL;
+	}
+
+	if (fd < 0) /* Disable only */
+		return 0;
+
+	irq->name = kasprintf(GFP_KERNEL, "vfio-irq[%d](%s)",
+						irq->hwirq, vdev->pdev->name);
+	if (!irq->name)
+		return -ENOMEM;
+
+	trigger = eventfd_ctx_fdget(fd);
+	if (IS_ERR(trigger)) {
+		kfree(irq->name);
+		return PTR_ERR(trigger);
+	}
+
+	irq->trigger = trigger;
+
+	ret = request_irq(irq->hwirq, vfio_irq_handler, 0, irq->name, irq);
+	if (ret) {
+		kfree(irq->name);
+		eventfd_ctx_put(trigger);
+		irq->trigger = NULL;
+		return ret;
+	}
+
+	return 0;
+}
+
+static int vfio_platform_set_irq_trigger(struct vfio_platform_device *vdev,
+				     unsigned index, unsigned start,
+				     unsigned count, uint32_t flags, void *data)
+{
+	struct vfio_platform_irq *irq = &vdev->irq[index];
+	uint8_t arr;
+	int32_t fd;
+
+	switch (flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
+	case VFIO_IRQ_SET_DATA_NONE:
+		if (count == 0)
+			return vfio_set_trigger(vdev, index, -1);
+
+		vfio_irq_handler(irq->hwirq, irq);
+		return 0;
+
+	case VFIO_IRQ_SET_DATA_BOOL:
+		if (copy_from_user(&arr, data, sizeof(uint8_t)))
+			return -EFAULT;
+
+		if (arr == 0x1) {
+			vfio_irq_handler(irq->hwirq, irq);
+			return 0;
+		}
+
+		return -EINVAL;
+
+	case VFIO_IRQ_SET_DATA_EVENTFD:
+		if (copy_from_user(&fd, data, sizeof(int32_t)))
+			return -EFAULT;
+
+		return vfio_set_trigger(vdev, index, fd);
+	}
+
+	return -EFAULT;
+}
+
+int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
+				 uint32_t flags, unsigned index, unsigned start,
+				 unsigned count, void *data)
+{
+	int (*func)(struct vfio_platform_device *vdev, unsigned index,
+		    unsigned start, unsigned count, uint32_t flags,
+		    void *data) = NULL;
+
+	switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
+	case VFIO_IRQ_SET_ACTION_MASK:
+	case VFIO_IRQ_SET_ACTION_UNMASK:
+		/* XXX not implemented */
+		break;
+	case VFIO_IRQ_SET_ACTION_TRIGGER:
+		func = vfio_platform_set_irq_trigger;
+		break;
+	}
+
+	if (!func)
+		return -ENOTTY;
+
+	return func(vdev, index, start, count, flags, data);
+}
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 9776cff..e21c15a 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -25,8 +25,11 @@
 	((u64)(index) << VFIO_PLATFORM_OFFSET_SHIFT)
 
 struct vfio_platform_irq {
+	struct eventfd_ctx	*trigger;
 	u32			flags;
 	u32			count;
+	int			hwirq;
+	char			*name;
 };
 
 struct vfio_platform_region {
@@ -47,4 +50,8 @@ extern int vfio_platform_irq_init(struct vfio_platform_device *vdev);
 
 extern void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev);
 
+extern int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
+			uint32_t flags, unsigned index, unsigned start,
+			unsigned count, void *data);
+
 #endif /* VFIO_PLATFORM_PRIVATE_H */
-- 
1.8.3.2

[RFC PATCH v5 11/11] VFIO_PLATFORM: Support for maskable and automasked interrupts

[Antonios Motakis]

Adds support to mask interrupts, and also for automasked interrupts.
Level sensitive interrupts are exposed as automasked interrupts and
are masked and disabled automatically when they fire.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_irq.c     | 117 ++++++++++++++++++++++++--
 drivers/vfio/platform/vfio_platform_private.h |   2 +
 2 files changed, 113 insertions(+), 6 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
index 433edc1..e38982f 100644
--- a/drivers/vfio/platform/vfio_platform_irq.c
+++ b/drivers/vfio/platform/vfio_platform_irq.c
@@ -52,9 +52,16 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev)
 		struct vfio_platform_irq irq;
 		int hwirq = platform_get_irq(vdev->pdev, i);
 
-		irq.flags = VFIO_IRQ_INFO_EVENTFD;
+		spin_lock_init(&irq.lock);
+
+		irq.flags = VFIO_IRQ_INFO_EVENTFD | VFIO_IRQ_INFO_MASKABLE;
+
+		if (irq_get_trigger_type(hwirq) & IRQ_TYPE_LEVEL_MASK)
+			irq.flags |= VFIO_IRQ_INFO_AUTOMASKED;
+
 		irq.count = 1;
 		irq.hwirq = hwirq;
+		irq.masked = false;
 
 		vdev->irq[i] = irq;
 	}
@@ -66,19 +73,39 @@ void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev)
 {
 	int i;
 
-	for (i = 0; i < vdev->num_irqs; i++)
+	for (i = 0; i < vdev->num_irqs; i++) {
 		vfio_set_trigger(vdev, i, -1);
 
+		if (vdev->irq[i].masked)
+			enable_irq(vdev->irq[i].hwirq);
+	}
+
 	kfree(vdev->irq);
 }
 
 static irqreturn_t vfio_irq_handler(int irq, void *dev_id)
 {
-	struct eventfd_ctx *trigger = dev_id;
+	struct vfio_platform_irq *irq_ctx = dev_id;
+	unsigned long flags;
+	int ret = IRQ_NONE;
+
+	spin_lock_irqsave(&irq_ctx->lock, flags);
+
+	if (!irq_ctx->masked) {
+		ret = IRQ_HANDLED;
+
+		if (irq_ctx->flags & VFIO_IRQ_INFO_AUTOMASKED) {
+			disable_irq_nosync(irq_ctx->hwirq);
+			irq_ctx->masked = true;
+		}
+	}
 
-	eventfd_signal(trigger, 1);
+	spin_unlock_irqrestore(&irq_ctx->lock, flags);
 
-	return IRQ_HANDLED;
+	if (ret == IRQ_HANDLED)
+		eventfd_signal(irq_ctx->trigger, 1);
+
+	return ret;
 }
 
 static int vfio_set_trigger(struct vfio_platform_device *vdev,
@@ -159,6 +186,82 @@ static int vfio_platform_set_irq_trigger(struct vfio_platform_device *vdev,
 	return -EFAULT;
 }
 
+static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev,
+				    unsigned index, unsigned start,
+				    unsigned count, uint32_t flags, void *data)
+{
+	uint8_t arr;
+
+	if (start != 0 || count != 1)
+		return -EINVAL;
+
+	switch (flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
+	case VFIO_IRQ_SET_DATA_BOOL:
+		if (copy_from_user(&arr, data, sizeof(uint8_t)))
+			return -EFAULT;
+
+		if (arr != 0x1)
+			return -EINVAL;
+
+	case VFIO_IRQ_SET_DATA_NONE:
+
+		spin_lock_irq(&vdev->irq[index].lock);
+
+		if (vdev->irq[index].masked) {
+			enable_irq(vdev->irq[index].hwirq);
+			vdev->irq[index].masked = false;
+		}
+
+		spin_unlock_irq(&vdev->irq[index].lock);
+
+		return 0;
+
+	case VFIO_IRQ_SET_DATA_EVENTFD: /* XXX not implemented yet */
+	default:
+		return -ENOTTY;
+	}
+
+	return 0;
+}
+
+static int vfio_platform_set_irq_mask(struct vfio_platform_device *vdev,
+				    unsigned index, unsigned start,
+				    unsigned count, uint32_t flags, void *data)
+{
+	uint8_t arr;
+
+	if (start != 0 || count != 1)
+		return -EINVAL;
+
+	switch (flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
+	case VFIO_IRQ_SET_DATA_BOOL:
+		if (copy_from_user(&arr, data, sizeof(uint8_t)))
+			return -EFAULT;
+
+		if (arr != 0x1)
+			return -EINVAL;
+
+	case VFIO_IRQ_SET_DATA_NONE:
+
+		spin_lock_irq(&vdev->irq[index].lock);
+
+		if (!vdev->irq[index].masked) {
+			disable_irq(vdev->irq[index].hwirq);
+			vdev->irq[index].masked = true;
+		}
+
+		spin_unlock_irq(&vdev->irq[index].lock);
+
+		return 0;
+
+	case VFIO_IRQ_SET_DATA_EVENTFD: /* XXX not implemented yet */
+	default:
+		return -ENOTTY;
+	}
+
+	return 0;
+}
+
 int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
 				 uint32_t flags, unsigned index, unsigned start,
 				 unsigned count, void *data)
@@ -169,8 +272,10 @@ int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
 
 	switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
 	case VFIO_IRQ_SET_ACTION_MASK:
+		func = vfio_platform_set_irq_mask;
+		break;
 	case VFIO_IRQ_SET_ACTION_UNMASK:
-		/* XXX not implemented */
+		func = vfio_platform_set_irq_unmask;
 		break;
 	case VFIO_IRQ_SET_ACTION_TRIGGER:
 		func = vfio_platform_set_irq_trigger;
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index e21c15a..e283171 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -30,6 +30,8 @@ struct vfio_platform_irq {
 	u32			count;
 	int			hwirq;
 	char			*name;
+	bool			masked;
+	spinlock_t		lock;
 };
 
 struct vfio_platform_region {
-- 
1.8.3.2

Re: [RFC PATCH v5 11/11] VFIO_PLATFORM: Support for maskable and automasked interrupts

[Alex Williamson]

On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
> Adds support to mask interrupts, and also for automasked interrupts.
> Level sensitive interrupts are exposed as automasked interrupts and
> are masked and disabled automatically when they fire.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/vfio/platform/vfio_platform_irq.c     | 117 ++++++++++++++++++++++++--
>  drivers/vfio/platform/vfio_platform_private.h |   2 +
>  2 files changed, 113 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
> index 433edc1..e38982f 100644
> --- a/drivers/vfio/platform/vfio_platform_irq.c
> +++ b/drivers/vfio/platform/vfio_platform_irq.c
> @@ -52,9 +52,16 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev)
>  		struct vfio_platform_irq irq;
>  		int hwirq = platform_get_irq(vdev->pdev, i);
>  
> -		irq.flags = VFIO_IRQ_INFO_EVENTFD;
> +		spin_lock_init(&irq.lock);
> +
> +		irq.flags = VFIO_IRQ_INFO_EVENTFD | VFIO_IRQ_INFO_MASKABLE;
> +
> +		if (irq_get_trigger_type(hwirq) & IRQ_TYPE_LEVEL_MASK)
> +			irq.flags |= VFIO_IRQ_INFO_AUTOMASKED;
> +
>  		irq.count = 1;
>  		irq.hwirq = hwirq;
> +		irq.masked = false;
>  
>  		vdev->irq[i] = irq;
>  	}
> @@ -66,19 +73,39 @@ void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev)
>  {
>  	int i;
>  
> -	for (i = 0; i < vdev->num_irqs; i++)
> +	for (i = 0; i < vdev->num_irqs; i++) {
>  		vfio_set_trigger(vdev, i, -1);
>  
> +		if (vdev->irq[i].masked)
> +			enable_irq(vdev->irq[i].hwirq);

This looks suspicious.  set_trigger(,, -1) calls free_irq() and here we
enable_irq().  Shouldn't the nexe user's call to request_irq() be
sufficient to re-enable it?  Thanks,

Alex

> +	}
> +
>  	kfree(vdev->irq);
>  }
>  
>  static irqreturn_t vfio_irq_handler(int irq, void *dev_id)
>  {
> -	struct eventfd_ctx *trigger = dev_id;
> +	struct vfio_platform_irq *irq_ctx = dev_id;
> +	unsigned long flags;
> +	int ret = IRQ_NONE;
> +
> +	spin_lock_irqsave(&irq_ctx->lock, flags);
> +
> +	if (!irq_ctx->masked) {
> +		ret = IRQ_HANDLED;
> +
> +		if (irq_ctx->flags & VFIO_IRQ_INFO_AUTOMASKED) {
> +			disable_irq_nosync(irq_ctx->hwirq);
> +			irq_ctx->masked = true;
> +		}
> +	}
>  
> -	eventfd_signal(trigger, 1);
> +	spin_unlock_irqrestore(&irq_ctx->lock, flags);
>  
> -	return IRQ_HANDLED;
> +	if (ret == IRQ_HANDLED)
> +		eventfd_signal(irq_ctx->trigger, 1);
> +
> +	return ret;
>  }
>  
>  static int vfio_set_trigger(struct vfio_platform_device *vdev,
> @@ -159,6 +186,82 @@ static int vfio_platform_set_irq_trigger(struct vfio_platform_device *vdev,
>  	return -EFAULT;
>  }
>  
> +static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev,
> +				    unsigned index, unsigned start,
> +				    unsigned count, uint32_t flags, void *data)
> +{
> +	uint8_t arr;
> +
> +	if (start != 0 || count != 1)
> +		return -EINVAL;
> +
> +	switch (flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
> +	case VFIO_IRQ_SET_DATA_BOOL:
> +		if (copy_from_user(&arr, data, sizeof(uint8_t)))
> +			return -EFAULT;
> +
> +		if (arr != 0x1)
> +			return -EINVAL;
> +
> +	case VFIO_IRQ_SET_DATA_NONE:
> +
> +		spin_lock_irq(&vdev->irq[index].lock);
> +
> +		if (vdev->irq[index].masked) {
> +			enable_irq(vdev->irq[index].hwirq);
> +			vdev->irq[index].masked = false;
> +		}
> +
> +		spin_unlock_irq(&vdev->irq[index].lock);
> +
> +		return 0;
> +
> +	case VFIO_IRQ_SET_DATA_EVENTFD: /* XXX not implemented yet */
> +	default:
> +		return -ENOTTY;
> +	}
> +
> +	return 0;
> +}
> +
> +static int vfio_platform_set_irq_mask(struct vfio_platform_device *vdev,
> +				    unsigned index, unsigned start,
> +				    unsigned count, uint32_t flags, void *data)
> +{
> +	uint8_t arr;
> +
> +	if (start != 0 || count != 1)
> +		return -EINVAL;
> +
> +	switch (flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
> +	case VFIO_IRQ_SET_DATA_BOOL:
> +		if (copy_from_user(&arr, data, sizeof(uint8_t)))
> +			return -EFAULT;
> +
> +		if (arr != 0x1)
> +			return -EINVAL;
> +
> +	case VFIO_IRQ_SET_DATA_NONE:
> +
> +		spin_lock_irq(&vdev->irq[index].lock);
> +
> +		if (!vdev->irq[index].masked) {
> +			disable_irq(vdev->irq[index].hwirq);
> +			vdev->irq[index].masked = true;
> +		}
> +
> +		spin_unlock_irq(&vdev->irq[index].lock);
> +
> +		return 0;
> +
> +	case VFIO_IRQ_SET_DATA_EVENTFD: /* XXX not implemented yet */
> +	default:
> +		return -ENOTTY;
> +	}
> +
> +	return 0;
> +}
> +
>  int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
>  				 uint32_t flags, unsigned index, unsigned start,
>  				 unsigned count, void *data)
> @@ -169,8 +272,10 @@ int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
>  
>  	switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
>  	case VFIO_IRQ_SET_ACTION_MASK:
> +		func = vfio_platform_set_irq_mask;
> +		break;
>  	case VFIO_IRQ_SET_ACTION_UNMASK:
> -		/* XXX not implemented */
> +		func = vfio_platform_set_irq_unmask;
>  		break;
>  	case VFIO_IRQ_SET_ACTION_TRIGGER:
>  		func = vfio_platform_set_irq_trigger;
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index e21c15a..e283171 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -30,6 +30,8 @@ struct vfio_platform_irq {
>  	u32			count;
>  	int			hwirq;
>  	char			*name;
> +	bool			masked;
> +	spinlock_t		lock;
>  };
>  
>  struct vfio_platform_region {

Re: [RFC PATCH v5 11/11] VFIO_PLATFORM: Support for maskable and automasked interrupts

[Antonios Motakis]

On Mon, Apr 28, 2014 at 7:46 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
> > Adds support to mask interrupts, and also for automasked interrupts.
> > Level sensitive interrupts are exposed as automasked interrupts and
> > are masked and disabled automatically when they fire.
> >
> > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > ---
> >  drivers/vfio/platform/vfio_platform_irq.c     | 117 ++++++++++++++++++++++++--
> >  drivers/vfio/platform/vfio_platform_private.h |   2 +
> >  2 files changed, 113 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
> > index 433edc1..e38982f 100644
> > --- a/drivers/vfio/platform/vfio_platform_irq.c
> > +++ b/drivers/vfio/platform/vfio_platform_irq.c
> > @@ -52,9 +52,16 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev)
> >               struct vfio_platform_irq irq;
> >               int hwirq = platform_get_irq(vdev->pdev, i);
> >
> > -             irq.flags = VFIO_IRQ_INFO_EVENTFD;
> > +             spin_lock_init(&irq.lock);
> > +
> > +             irq.flags = VFIO_IRQ_INFO_EVENTFD | VFIO_IRQ_INFO_MASKABLE;
> > +
> > +             if (irq_get_trigger_type(hwirq) & IRQ_TYPE_LEVEL_MASK)
> > +                     irq.flags |= VFIO_IRQ_INFO_AUTOMASKED;
> > +
> >               irq.count = 1;
> >               irq.hwirq = hwirq;
> > +             irq.masked = false;
> >
> >               vdev->irq[i] = irq;
> >       }
> > @@ -66,19 +73,39 @@ void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev)
> >  {
> >       int i;
> >
> > -     for (i = 0; i < vdev->num_irqs; i++)
> > +     for (i = 0; i < vdev->num_irqs; i++) {
> >               vfio_set_trigger(vdev, i, -1);
> >
> > +             if (vdev->irq[i].masked)
> > +                     enable_irq(vdev->irq[i].hwirq);
>
> This looks suspicious.  set_trigger(,, -1) calls free_irq() and here we
> enable_irq().  Shouldn't the nexe user's call to request_irq() be
> sufficient to re-enable it?  Thanks,
>

This is actually called when we unbind from the device, so it is
indeed not needed.

> Alex
>