[RFC 0/5] vfio: AMBA devices support

[RFC 0/5] vfio: AMBA devices support

[Antonios Motakis]

This patch series depends on the VFIO for PLATFORM devices patch series,
and implements AMBA device support for VFIO.

This can be used for example with the PL330 DMA Controller, which is an
AMBA device and not a proper platform device. By using the VFIO AMBA driver
introduced here, one can bind VFIO directly to such a device without hacking
the host device tree to make the device look like a platform device to the
system.

Also adding driver_override support to ARM AMBA bus devices, one can
alternate between using the proper host device driver and VFIO without
restarting the host system with a new device tree.

Antonios Motakis (5):
  driver core: amba: add device binding path 'driver_override'
  vfio: platform: decouple code probing devices from the platform bus
  vfio: platform: move code that can be shared with AMBA to a common
    file
  vfio: amba: VFIO support for AMBA devices
  vfio: amba: add the VFIO for AMBA devices module to Kconfig

 drivers/amba/bus.c                            |  43 +++
 drivers/vfio/platform/Kconfig                 |  10 +
 drivers/vfio/platform/Makefile                |   6 +-
 drivers/vfio/platform/vfio_amba.c             | 129 +++++++++
 drivers/vfio/platform/vfio_platform.c         | 355 +-----------------------
 drivers/vfio/platform/vfio_platform_common.c  | 380 ++++++++++++++++++++++++++
 drivers/vfio/platform/vfio_platform_irq.c     |   6 +-
 drivers/vfio/platform/vfio_platform_private.h |   9 +-
 include/linux/amba/bus.h                      |   1 +
 9 files changed, 591 insertions(+), 348 deletions(-)
 create mode 100644 drivers/vfio/platform/vfio_amba.c
 create mode 100644 drivers/vfio/platform/vfio_platform_common.c

-- 
1.8.3.2

[RFC 1/5] driver core: amba: add device binding path 'driver_override'

[Antonios Motakis]

As already demonstrated with PCI [1] and the platform bus [2], a
driver_override property in sysfs can be used to bypass the id matching
of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
device requested by the user.

[1] http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
[2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html

Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
---
 drivers/amba/bus.c       | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/amba/bus.h |  1 +
 2 files changed, 44 insertions(+)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 3cf61a1..8174a9a 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -17,6 +17,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/amba/bus.h>
 #include <linux/sizes.h>
+#include <linux/limits.h>
 
 #include <asm/irq.h>
 
@@ -42,6 +43,10 @@ static int amba_match(struct device *dev, struct device_driver *drv)
 	struct amba_device *pcdev = to_amba_device(dev);
 	struct amba_driver *pcdrv = to_amba_driver(drv);
 
+	/* When driver_override is set, only bind to the matching driver */
+	if (pcdev->driver_override)
+		return !strcmp(pcdev->driver_override, drv->name);
+
 	return amba_lookup(pcdrv->id_table, pcdev) != NULL;
 }
 
@@ -58,6 +63,43 @@ static int amba_uevent(struct device *dev, struct kobj_uevent_env *env)
 	return retval;
 }
 
+static ssize_t driver_override_show(struct device *_dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct amba_device *dev = to_amba_device(_dev);
+	return sprintf(buf, "%s\n", dev->driver_override);
+}
+
+static ssize_t driver_override_store(struct device *_dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct amba_device *dev = to_amba_device(_dev);
+	char *driver_override, *old = dev->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)) {
+	       dev->driver_override = driver_override;
+	} else {
+	       kfree(driver_override);
+	       dev->driver_override = NULL;
+	}
+
+	kfree(old);
+
+	return count;
+}
+
 #define amba_attr_func(name,fmt,arg...)					\
 static ssize_t name##_show(struct device *_dev,				\
 			   struct device_attribute *attr, char *buf)	\
@@ -80,6 +122,7 @@ amba_attr_func(resource, "\t%016llx\t%016llx\t%016lx\n",
 static struct device_attribute amba_dev_attrs[] = {
 	__ATTR_RO(id),
 	__ATTR_RO(resource),
+	__ATTR_RW(driver_override),
 	__ATTR_NULL,
 };
 
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index fdd7e1b..7c011e7 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -32,6 +32,7 @@ struct amba_device {
 	struct clk		*pclk;
 	unsigned int		periphid;
 	unsigned int		irq[AMBA_NR_IRQS];
+	char			*driver_override;
 };
 
 struct amba_driver {
-- 
1.8.3.2

Re: [RFC 1/5] driver core: amba: add device binding path 'driver_override'

[Kim Phillips]

On Fri, 22 Aug 2014 11:01:24 +0200
Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org> wrote:

> As already demonstrated with PCI [1] and the platform bus [2], a
> driver_override property in sysfs can be used to bypass the id matching
> of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
> device requested by the user.
> 
> [1] http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
> [2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html
> 
> Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> ---
>  drivers/amba/bus.c       | 43 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/amba/bus.h |  1 +

missing Documentation/ABI/testing/sysfs-bus-amba entry?

otherwise looks ok.

Thanks,

Kim

Re: [RFC 1/5] driver core: amba: add device binding path 'driver_override'

[Antonios Motakis]

On Mon, Aug 25, 2014 at 10:29 PM, Kim Phillips
<kim.phillips-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> On Fri, 22 Aug 2014 11:01:24 +0200
> Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org> wrote:
>
>> As already demonstrated with PCI [1] and the platform bus [2], a
>> driver_override property in sysfs can be used to bypass the id matching
>> of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
>> device requested by the user.
>>
>> [1] http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
>> [2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html
>>
>> Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
>> ---
>>  drivers/amba/bus.c       | 43 +++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/amba/bus.h |  1 +
>
> missing Documentation/ABI/testing/sysfs-bus-amba entry?
>
> otherwise looks ok.

Yep, it will be included when integrated with the VFIO PLATFORM series.

>
> Thanks,
>
> Kim



-- 
Antonios Motakis
Virtual Open Systems

[RFC 2/5] vfio: platform: decouple code probing devices from the platform bus

[Antonios Motakis]

Hide functionality related to discovering Linux platform device resources
in separate callback functions. This will allow us to implement AMBA support
while reusing a lot of the code.

Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
---
 drivers/vfio/platform/vfio_platform.c         | 28 +++++++++++++++++++++++----
 drivers/vfio/platform/vfio_platform_irq.c     |  6 +++---
 drivers/vfio/platform/vfio_platform_private.h |  7 ++++++-
 3 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index f4c06c6..99aa9af 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -38,7 +38,7 @@ static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
 {
 	int cnt = 0, i;
 
-	while (platform_get_resource(vdev->pdev, IORESOURCE_MEM, cnt))
+	while (vdev->get_resource(vdev, cnt))
 		cnt++;
 
 	vdev->region = kzalloc(sizeof(struct vfio_platform_region) * cnt,
@@ -47,8 +47,7 @@ static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
 		return -ENOMEM;
 
 	for (i = 0; i < cnt;  i++) {
-		struct resource *res =
-			platform_get_resource(vdev->pdev, IORESOURCE_MEM, i);
+		struct resource *res = vdev->get_resource(vdev, i);
 
 		if (!res)
 			goto err;
@@ -385,6 +384,24 @@ static const struct vfio_device_ops vfio_platform_ops = {
 	.mmap		= vfio_platform_mmap,
 };
 
+/* probing devices from the linux platform bus */
+
+static struct resource* get_platform_resource(struct vfio_platform_device *vdev,
+						int i)
+{
+	struct platform_device *pdev = (struct platform_device *) vdev->opaque;
+
+	return platform_get_resource(pdev, IORESOURCE_MEM, i);
+}
+
+static int get_platform_irq(struct vfio_platform_device *vdev, int i)
+{
+	struct platform_device *pdev = (struct platform_device *) vdev->opaque;
+
+	return platform_get_irq(pdev, i);
+}
+
+
 static int vfio_platform_probe(struct platform_device *pdev)
 {
 	struct vfio_platform_device *vdev;
@@ -403,7 +420,10 @@ static int vfio_platform_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	vdev->pdev = pdev;
+	vdev->opaque = (void *) pdev;
+	vdev->get_resource = get_platform_resource;
+	vdev->get_irq = get_platform_irq;
+	vdev->name = pdev->name;
 
 	ret = vfio_add_group_dev(&pdev->dev, &vfio_platform_ops, vdev);
 	if (ret) {
diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
index 6768508..50c8293 100644
--- a/drivers/vfio/platform/vfio_platform_irq.c
+++ b/drivers/vfio/platform/vfio_platform_irq.c
@@ -38,7 +38,7 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev)
 {
 	int cnt = 0, i;
 
-	while (platform_get_irq(vdev->pdev, cnt) > 0)
+	while (vdev->get_irq(vdev, cnt) > 0)
 		cnt++;
 
 	vdev->irq = kzalloc(sizeof(struct vfio_platform_irq) * cnt, GFP_KERNEL);
@@ -46,7 +46,7 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev)
 		return -ENOMEM;
 
 	for (i = 0; i < cnt; i++) {
-		int hwirq = platform_get_irq(vdev->pdev, i);
+		int hwirq = vdev->get_irq(vdev, i);
 
 		if (hwirq < 0)
 			goto err;
@@ -172,7 +172,7 @@ static int vfio_set_trigger(struct vfio_platform_device *vdev,
 		return 0;
 
 	irq->name = kasprintf(GFP_KERNEL, "vfio-irq[%d](%s)",
-						irq->hwirq, vdev->pdev->name);
+						irq->hwirq, vdev->name);
 	if (!irq->name)
 		return -ENOMEM;
 
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 86a9201..b79f485 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -44,11 +44,16 @@ struct vfio_platform_region {
 };
 
 struct vfio_platform_device {
-	struct platform_device		*pdev;
 	struct vfio_platform_region	*region;
 	u32				num_regions;
 	struct vfio_platform_irq	*irq;
 	u32				num_irqs;
+	void				*opaque;
+	const char			*name;
+	/* callbacks to discover device resources */
+	struct resource*
+		(*get_resource)(struct vfio_platform_device *vdev, int i);
+	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
 };
 
 extern int vfio_platform_irq_init(struct vfio_platform_device *vdev);
-- 
1.8.3.2

[RFC 3/5] vfio: platform: move code that can be shared with AMBA to a common file

[Antonios Motakis]

Having decoupled a portion of the VFIO_PLATFORM code from platform bus
specific resource discovery code, we can now move that code separate file.
This will allow us to implement discovering AMBA devices and their resources,
but reuse the a large part of the VFIO_PLATFORM implementation.

Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
---
 drivers/vfio/platform/Makefile                |   2 +-
 drivers/vfio/platform/vfio_platform.c         | 351 ------------------------
 drivers/vfio/platform/vfio_platform_common.c  | 380 ++++++++++++++++++++++++++
 drivers/vfio/platform/vfio_platform_private.h |   2 +
 4 files changed, 383 insertions(+), 352 deletions(-)
 create mode 100644 drivers/vfio/platform/vfio_platform_common.c

diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
index 2c53327..115d1b5 100644
--- a/drivers/vfio/platform/Makefile
+++ b/drivers/vfio/platform/Makefile
@@ -1,4 +1,4 @@
 
-vfio-platform-y := vfio_platform.o vfio_platform_irq.o
+vfio-platform-y := vfio_platform.o vfio_platform_irq.o vfio_platform_common.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 99aa9af..6be305a 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -13,7 +13,6 @@
  */
 
 #include <linux/device.h>
-#include <linux/eventfd.h>
 #include <linux/interrupt.h>
 #include <linux/iommu.h>
 #include <linux/module.h>
@@ -34,356 +33,6 @@
 #define DRIVER_AUTHOR   "Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>"
 #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 (vdev->get_resource(vdev, cnt))
-		cnt++;
-
-	vdev->region = kzalloc(sizeof(struct vfio_platform_region) * cnt,
-				GFP_KERNEL);
-	if (!vdev->region)
-		return -ENOMEM;
-
-	for (i = 0; i < cnt;  i++) {
-		struct resource *res = vdev->get_resource(vdev, i);
-
-		if (!res)
-			goto err;
-
-		vdev->region[i].addr = res->start;
-		vdev->region[i].size = resource_size(res);
-		vdev->region[i].flags = VFIO_REGION_INFO_FLAG_READ
-					| VFIO_REGION_INFO_FLAG_WRITE;
-		/* Only regions addressed with PAGE granularity may be MMAPed
-		 * securely. */
-		if (!(vdev->region[i].addr & ~PAGE_MASK)
-				&& !(vdev->region[i].size & ~PAGE_MASK))
-			vdev->region[i].flags |= VFIO_REGION_INFO_FLAG_MMAP;
-	}
-
-	vdev->num_regions = cnt;
-
-	return 0;
-err:
-	kfree(vdev->region);
-	return -EINVAL;
-}
-
-static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
-{
-	vdev->num_regions = 0;
-	kfree(vdev->region);
-}
-
-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);
-}
-
-static int vfio_platform_open(void *device_data)
-{
-	struct vfio_platform_device *vdev = device_data;
-	int ret;
-
-	if (!try_module_get(THIS_MODULE))
-		return -ENODEV;
-
-	ret = vfio_platform_regions_init(vdev);
-	if (ret)
-		goto err_reg;
-
-	ret = vfio_platform_irq_init(vdev);
-	if (ret)
-		goto err_irq;
-
-	return 0;
-
-err_irq:
-	vfio_platform_regions_cleanup(vdev);
-err_reg:
-	module_put(THIS_MODULE);
-
-	return ret;
-}
-
-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 = vdev->num_regions;
-		info.num_irqs = vdev->num_irqs;
-
-		return copy_to_user((void __user *)arg, &info, minsz);
-
-	} 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) {
-		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) {
-		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)
-		return -EINVAL;
-
-	return -ENOTTY;
-}
-
-static ssize_t vfio_platform_read(void *device_data, char __user *buf,
-			     size_t count, loff_t *ppos)
-{
-	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;
-
-	if (index >= vdev->num_regions)
-		return -EINVAL;
-
-	if (!vdev->region[index].ioaddr) {
-		vdev->region[index].ioaddr =
-			ioremap_nocache(vdev->region[index].addr,
-					vdev->region[index].size);
-
-		if (!vdev->region[index].ioaddr)
-			return -ENOMEM;
-	}
-
-	while (count) {
-		size_t filled;
-
-		if (count >= 4 && !(off % 4)) {
-			u32 val;
-
-			val = ioread32(vdev->region[index].ioaddr + off);
-			if (copy_to_user(buf, &val, 4))
-				goto err;
-
-			filled = 4;
-		} else if (count >= 2 && !(off % 2)) {
-			u16 val;
-
-			val = ioread16(vdev->region[index].ioaddr + off);
-			if (copy_to_user(buf, &val, 2))
-				goto err;
-
-			filled = 2;
-		} else {
-			u8 val;
-
-			val = ioread8(vdev->region[index].ioaddr + off);
-			if (copy_to_user(buf, &val, 1))
-				goto err;
-
-			filled = 1;
-		}
-
-
-		count -= filled;
-		done += filled;
-		off += filled;
-		buf += filled;
-	}
-
-	return done;
-err:
-	return -EFAULT;
-}
-
-static ssize_t vfio_platform_write(void *device_data, const char __user *buf,
-			      size_t count, loff_t *ppos)
-{
-	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;
-
-	if (index >= vdev->num_regions)
-		return -EINVAL;
-
-	if (!vdev->region[index].ioaddr) {
-		vdev->region[index].ioaddr =
-			ioremap_nocache(vdev->region[index].addr,
-					vdev->region[index].size);
-
-		if (!vdev->region[index].ioaddr)
-			return -ENOMEM;
-	}
-
-	while (count) {
-		size_t filled;
-
-		if (count >= 4 && !(off % 4)) {
-			u32 val;
-
-			if (copy_from_user(&val, buf, 4))
-				goto err;
-			iowrite32(val, vdev->region[index].ioaddr + off);
-
-			filled = 4;
-		} else if (count >= 2 && !(off % 2)) {
-			u16 val;
-
-			if (copy_from_user(&val, buf, 2))
-				goto err;
-			iowrite16(val, vdev->region[index].ioaddr + off);
-
-			filled = 2;
-		} else {
-			u8 val;
-
-			if (copy_from_user(&val, buf, 1))
-				goto err;
-			iowrite8(val, vdev->region[index].ioaddr + off);
-
-			filled = 1;
-		}
-
-		count -= filled;
-		done += filled;
-		off += filled;
-		buf += filled;
-	}
-
-	return done;
-err:
-	return -EFAULT;
-}
-
-static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
-{
-	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 = {
-	.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,
-};
-
 /* probing devices from the linux platform bus */
 
 static struct resource* get_platform_resource(struct vfio_platform_device *vdev,
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
new file mode 100644
index 0000000..6e69703
--- /dev/null
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -0,0 +1,380 @@
+/*
+ * Copyright (C) 2013 - Virtual Open Systems
+ * Author: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
+ *
+ * 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/irq.h>
+
+#include "vfio_platform_private.h"
+
+static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
+{
+	int cnt = 0, i;
+
+	while (vdev->get_resource(vdev, cnt))
+		cnt++;
+
+	vdev->region = kzalloc(sizeof(struct vfio_platform_region) * cnt,
+				GFP_KERNEL);
+	if (!vdev->region)
+		return -ENOMEM;
+
+	for (i = 0; i < cnt;  i++) {
+		struct resource *res = vdev->get_resource(vdev, i);
+
+		if (!res)
+			goto err;
+
+		vdev->region[i].addr = res->start;
+		vdev->region[i].size = resource_size(res);
+		vdev->region[i].flags = VFIO_REGION_INFO_FLAG_READ
+					| VFIO_REGION_INFO_FLAG_WRITE;
+		/* Only regions addressed with PAGE granularity may be MMAPed
+		 * securely. */
+		if (!(vdev->region[i].addr & ~PAGE_MASK)
+				&& !(vdev->region[i].size & ~PAGE_MASK))
+			vdev->region[i].flags |= VFIO_REGION_INFO_FLAG_MMAP;
+	}
+
+	vdev->num_regions = cnt;
+
+	return 0;
+err:
+	kfree(vdev->region);
+	return -EINVAL;
+}
+
+static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
+{
+	vdev->num_regions = 0;
+	kfree(vdev->region);
+}
+
+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);
+}
+
+static int vfio_platform_open(void *device_data)
+{
+	struct vfio_platform_device *vdev = device_data;
+	int ret;
+
+	if (!try_module_get(THIS_MODULE))
+		return -ENODEV;
+
+	ret = vfio_platform_regions_init(vdev);
+	if (ret)
+		goto err_reg;
+
+	ret = vfio_platform_irq_init(vdev);
+	if (ret)
+		goto err_irq;
+
+	return 0;
+
+err_irq:
+	vfio_platform_regions_cleanup(vdev);
+err_reg:
+	module_put(THIS_MODULE);
+
+	return ret;
+}
+
+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 = vdev->num_regions;
+		info.num_irqs = vdev->num_irqs;
+
+		return copy_to_user((void __user *)arg, &info, minsz);
+
+	} 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) {
+		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) {
+		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)
+		return -EINVAL;
+
+	return -ENOTTY;
+}
+
+static ssize_t vfio_platform_read(void *device_data, char __user *buf,
+			     size_t count, loff_t *ppos)
+{
+	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;
+
+	if (index >= vdev->num_regions)
+		return -EINVAL;
+
+	if (!vdev->region[index].ioaddr) {
+		vdev->region[index].ioaddr =
+			ioremap_nocache(vdev->region[index].addr,
+					vdev->region[index].size);
+
+		if (!vdev->region[index].ioaddr)
+			return -ENOMEM;
+	}
+
+	while (count) {
+		size_t filled;
+
+		if (count >= 4 && !(off % 4)) {
+			u32 val;
+
+			val = ioread32(vdev->region[index].ioaddr + off);
+			if (copy_to_user(buf, &val, 4))
+				goto err;
+
+			filled = 4;
+		} else if (count >= 2 && !(off % 2)) {
+			u16 val;
+
+			val = ioread16(vdev->region[index].ioaddr + off);
+			if (copy_to_user(buf, &val, 2))
+				goto err;
+
+			filled = 2;
+		} else {
+			u8 val;
+
+			val = ioread8(vdev->region[index].ioaddr + off);
+			if (copy_to_user(buf, &val, 1))
+				goto err;
+
+			filled = 1;
+		}
+
+
+		count -= filled;
+		done += filled;
+		off += filled;
+		buf += filled;
+	}
+
+	return done;
+err:
+	return -EFAULT;
+}
+
+static ssize_t vfio_platform_write(void *device_data, const char __user *buf,
+			      size_t count, loff_t *ppos)
+{
+	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;
+
+	if (index >= vdev->num_regions)
+		return -EINVAL;
+
+	if (!vdev->region[index].ioaddr) {
+		vdev->region[index].ioaddr =
+			ioremap_nocache(vdev->region[index].addr,
+					vdev->region[index].size);
+
+		if (!vdev->region[index].ioaddr)
+			return -ENOMEM;
+	}
+
+	while (count) {
+		size_t filled;
+
+		if (count >= 4 && !(off % 4)) {
+			u32 val;
+
+			if (copy_from_user(&val, buf, 4))
+				goto err;
+			iowrite32(val, vdev->region[index].ioaddr + off);
+
+			filled = 4;
+		} else if (count >= 2 && !(off % 2)) {
+			u16 val;
+
+			if (copy_from_user(&val, buf, 2))
+				goto err;
+			iowrite16(val, vdev->region[index].ioaddr + off);
+
+			filled = 2;
+		} else {
+			u8 val;
+
+			if (copy_from_user(&val, buf, 1))
+				goto err;
+			iowrite8(val, vdev->region[index].ioaddr + off);
+
+			filled = 1;
+		}
+
+		count -= filled;
+		done += filled;
+		off += filled;
+		buf += filled;
+	}
+
+	return done;
+err:
+	return -EFAULT;
+}
+
+static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
+{
+	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);
+}
+
+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,
+};
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index b79f485..45d0bab 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -56,6 +56,8 @@ struct vfio_platform_device {
 	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
 };
 
+extern const struct vfio_device_ops vfio_platform_ops;
+
 extern int vfio_platform_irq_init(struct vfio_platform_device *vdev);
 
 extern void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev);
-- 
1.8.3.2

[RFC 5/5] vfio: amba: add the VFIO for AMBA devices module to Kconfig

[Antonios Motakis]

Enable building the VFIO AMBA driver. VFIO_AMBA depends on VFIO_PLATFORM,
since it is sharing a portion of the code, and it is essentially implemented
as a platform device whose resources are discovered via AMBA specific APIs
in the kernel.

Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
---
 drivers/vfio/platform/Kconfig  | 10 ++++++++++
 drivers/vfio/platform/Makefile |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
index c51af17..8b97786 100644
--- a/drivers/vfio/platform/Kconfig
+++ b/drivers/vfio/platform/Kconfig
@@ -7,3 +7,13 @@ config VFIO_PLATFORM
 	  framework.
 
 	  If you don't know what to do here, say N.
+
+config VFIO_AMBA
+	tristate "VFIO support for AMBA devices"
+	depends on VFIO && VFIO_PLATFORM && EVENTFD && ARM_AMBA
+	help
+	  Support for ARM AMBA devices with VFIO. This is required to make
+	  use of ARM AMBA 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
index 115d1b5..1cb5e94 100644
--- a/drivers/vfio/platform/Makefile
+++ b/drivers/vfio/platform/Makefile
@@ -2,3 +2,7 @@
 vfio-platform-y := vfio_platform.o vfio_platform_irq.o vfio_platform_common.o
 
 obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
+
+vfio-amba-y := vfio_amba.o
+
+obj-$(CONFIG_VFIO_AMBA) += vfio-amba.o
-- 
1.8.3.2

[RFC 4/5] vfio: amba: VFIO support for AMBA devices

[Antonios Motakis]

Add support for discovering AMBA devices with VFIO and handle them
similarly to Linux platform devices.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_amba.c | 129 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 129 insertions(+)
 create mode 100644 drivers/vfio/platform/vfio_amba.c

diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c
new file mode 100644
index 0000000..9fc0ae6
--- /dev/null
+++ b/drivers/vfio/platform/vfio_amba.c
@@ -0,0 +1,129 @@
+/*
+ * 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/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/irq.h>
+#include <linux/amba/bus.h>
+
+#include "vfio_platform_private.h"
+
+#define DRIVER_VERSION  "0.6"
+#define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
+#define DRIVER_DESC     "VFIO for AMBA devices - User Level meta-driver"
+
+/* probing devices from the AMBA bus */
+
+static struct resource* get_amba_resource(struct vfio_platform_device *vdev,
+						int i)
+{
+	struct amba_device *adev = (struct amba_device *) vdev->opaque;
+
+	if (i == 0)
+		return &adev->res;
+	else
+		return NULL;
+}
+
+static int get_amba_irq(struct vfio_platform_device *vdev, int i)
+{
+	struct amba_device *adev = (struct amba_device *) vdev->opaque;
+
+	if (i < AMBA_NR_IRQS)
+		return adev->irq[i];
+	else
+		return 0;
+}
+
+static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id)
+{
+
+	struct vfio_platform_device *vdev;
+	struct iommu_group *group;
+	int ret;
+
+	group = iommu_group_get(&adev->dev);
+	if (!group) {
+		pr_err("VFIO: No IOMMU group for amba device with id%d\n",
+				adev->periphid);
+		return -EINVAL;
+	}
+
+	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
+	if (!vdev) {
+		iommu_group_put(group);
+		return -ENOMEM;
+	}
+
+	vdev->opaque = (void *) adev;
+	vdev->get_resource = get_amba_resource;
+	vdev->get_irq = get_amba_irq;
+	vdev->name = "AMBA device";
+
+	ret = vfio_add_group_dev(&adev->dev, &vfio_platform_ops, vdev);
+	if (ret) {
+		iommu_group_put(group);
+		kfree(vdev);
+	}
+
+	return ret;
+}
+
+static int vfio_amba_remove(struct amba_device *adev)
+{
+
+	struct vfio_platform_device *vdev;
+
+	vdev = vfio_del_group_dev(&adev->dev);
+	if (!vdev)
+		return -EINVAL;
+
+	iommu_group_put(adev->dev.iommu_group);
+	kfree(vdev);
+
+	return 0;
+}
+
+static struct amba_id pl330_ids[] = {
+	{ 0, 0 },
+};
+
+MODULE_DEVICE_TABLE(amba, pl330_ids);
+
+static struct amba_driver vfio_amba_driver = {
+	.probe = vfio_amba_probe,
+	.remove = vfio_amba_remove,
+	.id_table = pl330_ids,
+	.drv = {
+		.name = "vfio-amba",
+		.owner = THIS_MODULE,
+	},
+};
+
+module_amba_driver(vfio_amba_driver);
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
-- 
1.8.3.2

Re: [RFC 4/5] vfio: amba: VFIO support for AMBA devices

[Will Deacon]

Hi Antonios,

On Fri, Aug 22, 2014 at 10:01:27AM +0100, Antonios Motakis wrote:
> Add support for discovering AMBA devices with VFIO and handle them
> similarly to Linux platform devices.

[...]

> +static struct amba_id pl330_ids[] = {
> +	{ 0, 0 },
> +};
> +
> +MODULE_DEVICE_TABLE(amba, pl330_ids);
> +
> +static struct amba_driver vfio_amba_driver = {
> +	.probe = vfio_amba_probe,
> +	.remove = vfio_amba_remove,
> +	.id_table = pl330_ids,
> +	.drv = {
> +		.name = "vfio-amba",
> +		.owner = THIS_MODULE,
> +	},
> +};

I don't understand what you're doing with the IDs here. What's the point in
the empty list?

This also raises a larger question about whether or not it's safe to allow
device passthrough of arbitrary platform devices with VFIO. In the absence
of a bus/device standard like PCI, I really think this should be in opt-in
decision, where certain platform drivers can declare that their device can
be safely used with passthrough.

Thoughts?

Will

Re: [RFC 4/5] vfio: amba: VFIO support for AMBA devices

[Antonios Motakis]

Hello WIll,

On Tue, Aug 26, 2014 at 12:50 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi Antonios,
>
> On Fri, Aug 22, 2014 at 10:01:27AM +0100, Antonios Motakis wrote:
>> Add support for discovering AMBA devices with VFIO and handle them
>> similarly to Linux platform devices.
>
> [...]
>
>> +static struct amba_id pl330_ids[] = {
>> +     { 0, 0 },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(amba, pl330_ids);
>> +
>> +static struct amba_driver vfio_amba_driver = {
>> +     .probe = vfio_amba_probe,
>> +     .remove = vfio_amba_remove,
>> +     .id_table = pl330_ids,
>> +     .drv = {
>> +             .name = "vfio-amba",
>> +             .owner = THIS_MODULE,
>> +     },
>> +};
>
> I don't understand what you're doing with the IDs here. What's the point in
> the empty list?
>

The empty list means VFIO will not bind to any AMBA devices by
default. If the user wants to use an AMBA device with VFIO, the idea
is to use the driver override proposed in [RFC 1/5] driver core: amba:
add device binding path 'driver_override'.

> This also raises a larger question about whether or not it's safe to allow
> device passthrough of arbitrary platform devices with VFIO. In the absence
> of a bus/device standard like PCI, I really think this should be in opt-in
> decision, where certain platform drivers can declare that their device can
> be safely used with passthrough.

In a sense the driver_override is already an opt-in, although under
control of the user. I don't know if we want to take the decision for
the user and whitelist the devices one can safely use, and reject any
other device one might want to try. I would argue that if a user
decides to use driver_override, he should be able to do that (and then
report the consequences to us ;)

Some of the differences between PLATFORM devices, could be handled at
the QEMU level. With the addition of the VFIO device node info patches
we also proposed recently, any VFIO user essentially sees a bunch of
MMIO regions, IRQs and some device node properties that describe the
device. So a VFIO user (e.g. QEMU) has access to more or less the same
stuff a kernel driver does.

I could see this breaking if a device tries to do memory accesses that
don't go through the IOMMU (maybe if it is directly connected with
another device), but in that situation that device is probably
unusable with VFIO even if we special-case it. Any other ideas?

>
> Thoughts?
>
> Will



-- 
Antonios Motakis
Virtual Open Systems

Re: [RFC 4/5] vfio: amba: VFIO support for AMBA devices

[Alex Williamson]

On Tue, 2014-08-26 at 17:39 +0200, Antonios Motakis wrote:
> Hello WIll,
> 
> On Tue, Aug 26, 2014 at 12:50 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> > Hi Antonios,
> >
> > On Fri, Aug 22, 2014 at 10:01:27AM +0100, Antonios Motakis wrote:
> >> Add support for discovering AMBA devices with VFIO and handle them
> >> similarly to Linux platform devices.
> >
> > [...]
> >
> >> +static struct amba_id pl330_ids[] = {
> >> +     { 0, 0 },
> >> +};
> >> +
> >> +MODULE_DEVICE_TABLE(amba, pl330_ids);
> >> +
> >> +static struct amba_driver vfio_amba_driver = {
> >> +     .probe = vfio_amba_probe,
> >> +     .remove = vfio_amba_remove,
> >> +     .id_table = pl330_ids,
> >> +     .drv = {
> >> +             .name = "vfio-amba",
> >> +             .owner = THIS_MODULE,
> >> +     },
> >> +};
> >
> > I don't understand what you're doing with the IDs here. What's the point in
> > the empty list?
> >
> 
> The empty list means VFIO will not bind to any AMBA devices by
> default. If the user wants to use an AMBA device with VFIO, the idea
> is to use the driver override proposed in [RFC 1/5] driver core: amba:
> add device binding path 'driver_override'.
> 
> > This also raises a larger question about whether or not it's safe to allow
> > device passthrough of arbitrary platform devices with VFIO. In the absence
> > of a bus/device standard like PCI, I really think this should be in opt-in
> > decision, where certain platform drivers can declare that their device can
> > be safely used with passthrough.
> 
> In a sense the driver_override is already an opt-in, although under
> control of the user. I don't know if we want to take the decision for
> the user and whitelist the devices one can safely use, and reject any
> other device one might want to try. I would argue that if a user
> decides to use driver_override, he should be able to do that (and then
> report the consequences to us ;)
> 
> Some of the differences between PLATFORM devices, could be handled at
> the QEMU level. With the addition of the VFIO device node info patches
> we also proposed recently, any VFIO user essentially sees a bunch of
> MMIO regions, IRQs and some device node properties that describe the
> device. So a VFIO user (e.g. QEMU) has access to more or less the same
> stuff a kernel driver does.
> 
> I could see this breaking if a device tries to do memory accesses that
> don't go through the IOMMU (maybe if it is directly connected with
> another device), but in that situation that device is probably
> unusable with VFIO even if we special-case it. Any other ideas?

It would be a flaw in the IOMMU group support of the platform to not
expose lack of isolation between such devices.  The platform IOMMU group
code is where decisions should be made about whether a device or set of
devices is safe to expose through VFIO.  If devices are unsafe they
should not have an IOMMU group.  If there is lack of isolation between
devices, they should be members of the same IOMMU group.  Adjust how
IOMMU grouping works and VFIO will follow.  Thanks,

Alex

Re: [RFC 0/5] vfio: AMBA devices support

[Kim Phillips]

On Fri, 22 Aug 2014 11:01:23 +0200
Antonios Motakis <a.motakis@virtualopensystems.com> wrote:

> This patch series depends on the VFIO for PLATFORM devices patch series,
> and implements AMBA device support for VFIO.

...

>  drivers/amba/bus.c                            |  43 +++
>  drivers/vfio/platform/Kconfig                 |  10 +
>  drivers/vfio/platform/Makefile                |   6 +-
>  drivers/vfio/platform/vfio_amba.c             | 129 +++++++++
>  drivers/vfio/platform/vfio_platform.c         | 355 +-----------------------
>  drivers/vfio/platform/vfio_platform_common.c  | 380 ++++++++++++++++++++++++++
>  drivers/vfio/platform/vfio_platform_irq.c     |   6 +-
>  drivers/vfio/platform/vfio_platform_private.h |   9 +-
>  include/linux/amba/bus.h                      |   1 +
>  9 files changed, 591 insertions(+), 348 deletions(-)
>  create mode 100644 drivers/vfio/platform/vfio_amba.c
>  create mode 100644 drivers/vfio/platform/vfio_platform_common.c

I think this patchseries should be merged with its dependent
patchseries, in order to not have to review the common vfio_platform
code twice.

just my 2c.

Thanks,

Kim

Re: [RFC 0/5] vfio: AMBA devices support

[Antonios Motakis]

[-- Attachment #1.1: Type: text/plain, Size: 1509 bytes --]

On Tue, Aug 26, 2014 at 1:56 AM, Kim Phillips <kim.phillips-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
wrote:

> On Fri, 22 Aug 2014 11:01:23 +0200
> Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org> wrote:
>
> > This patch series depends on the VFIO for PLATFORM devices patch series,
> > and implements AMBA device support for VFIO.
>
> ...
>
> >  drivers/amba/bus.c                            |  43 +++
> >  drivers/vfio/platform/Kconfig                 |  10 +
> >  drivers/vfio/platform/Makefile                |   6 +-
> >  drivers/vfio/platform/vfio_amba.c             | 129 +++++++++
> >  drivers/vfio/platform/vfio_platform.c         | 355
> +-----------------------
> >  drivers/vfio/platform/vfio_platform_common.c  | 380
> ++++++++++++++++++++++++++
> >  drivers/vfio/platform/vfio_platform_irq.c     |   6 +-
> >  drivers/vfio/platform/vfio_platform_private.h |   9 +-
> >  include/linux/amba/bus.h                      |   1 +
> >  9 files changed, 591 insertions(+), 348 deletions(-)
> >  create mode 100644 drivers/vfio/platform/vfio_amba.c
> >  create mode 100644 drivers/vfio/platform/vfio_platform_common.c
>
> I think this patchseries should be merged with its dependent
> patchseries, in order to not have to review the common vfio_platform
> code twice.
>
>
Yeah, my intention is to squash them into one series for the next version
of VFIO PLATFORM, so we are in agreement here.


just my 2c.
>
> Thanks,
>
> Kim
>



-- 
Antonios Motakis
Virtual Open Systems

[-- Attachment #1.2: Type: text/html, Size: 2381 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC 0/5] vfio: AMBA devices support

[Antonios Motakis]

On Tue, Aug 26, 2014 at 1:56 AM, Kim Phillips
<kim.phillips-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>
> On Fri, 22 Aug 2014 11:01:23 +0200
> Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org> wrote:
>
> > This patch series depends on the VFIO for PLATFORM devices patch series,
> > and implements AMBA device support for VFIO.
>
> ...
>
> >  drivers/amba/bus.c                            |  43 +++
> >  drivers/vfio/platform/Kconfig                 |  10 +
> >  drivers/vfio/platform/Makefile                |   6 +-
> >  drivers/vfio/platform/vfio_amba.c             | 129 +++++++++
> >  drivers/vfio/platform/vfio_platform.c         | 355 +-----------------------
> >  drivers/vfio/platform/vfio_platform_common.c  | 380 ++++++++++++++++++++++++++
> >  drivers/vfio/platform/vfio_platform_irq.c     |   6 +-
> >  drivers/vfio/platform/vfio_platform_private.h |   9 +-
> >  include/linux/amba/bus.h                      |   1 +
> >  9 files changed, 591 insertions(+), 348 deletions(-)
> >  create mode 100644 drivers/vfio/platform/vfio_amba.c
> >  create mode 100644 drivers/vfio/platform/vfio_platform_common.c
>
> I think this patchseries should be merged with its dependent
> patchseries, in order to not have to review the common vfio_platform
> code twice.

Yeah, my intention is to squash them into one series for the next
version of VFIO PLATFORM, so we are in agreement here.

>
> just my 2c.
>
> Thanks,
>
> Kim




-- 
Antonios Motakis
Virtual Open Systems