Re: [RFC PATCH v4 06/10] VFIO_PLATFORM: Read and write support for the device fd

Re: [RFC PATCH v4 06/10] VFIO_PLATFORM: Read and write support for the device fd

[Eric Auger]

> Date: Sat,  8 Feb 2014 18:29:36 +0100
> 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>
> Tested-by: Alvise Rigo <a.rigo@virtualopensystems.com>
> ---
>  drivers/vfio/platform/vfio_platform.c | 128 +++++++++++++++++++++++++++++++++-
>  1 file changed, 125 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
> index f7db5c0..ee96078 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;
>  	}
> @@ -150,13 +151,134 @@ 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 *io;
> +	int i;
> +
> +	for (i = 0; i < vdev->num_regions; i++) {
> +		struct vfio_platform_region region = vdev->region[i];
> +		unsigned int done = 0;
> +		loff_t off;
> +
> +		if ((*ppos < region.addr)
> +		     || (*ppos + count - 1) >= (region.addr + region.size))
> +			continue;
> +
> +		io = ioremap_nocache(region.addr, region.size);
> +
> +		off = *ppos - region.addr;
> +
> +		while (count) {
> +			size_t filled;
> +
> +			if (count >= 4 && !(off % 4)) {
> +				u32 val;
> +
> +				val = ioread32(io + off);

Hi Antonios,

I suspect there is an issue with the read address. Indeed io being an
unsigned int* the read address is io + off x sizeof (unsigned int) ie.
io+ offx4  whereas we expect to read io + off. declaring io as a void*
corrects the issue (or void __iomem *io). Same issue on write.

Best Regards

Eric

> +				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;
> +	}
> +
> +	return -EFAULT;
> +
> +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 *io;
> +	int i;
> +
> +	for (i = 0; i < vdev->num_regions; i++) {
> +		struct vfio_platform_region region = vdev->region[i];
> +		unsigned int done = 0;
> +		loff_t off;
> +
> +		if ((*ppos < region.addr)
> +		     || (*ppos + count - 1) >= (region.addr + region.size))
> +			continue;
> +
> +		io = ioremap_nocache(region.addr, region.size);
> +
> +		off = *ppos - region.addr;
> +
> +		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;
> +	}
> +
> +	return -EINVAL;
> +
> +err:
> +	iounmap(io);
> +	return -EFAULT;
>  }
>  
>  static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
> 

Re: [RFC PATCH v4 06/10] VFIO_PLATFORM: Read and write support for the device fd

[Alex Williamson]

On Tue, 2014-03-25 at 17:43 +0100, Eric Auger wrote:
> > Date: Sat,  8 Feb 2014 18:29:36 +0100
> > 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-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> > Tested-by: Alvise Rigo <a.rigo-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> > ---
> >  drivers/vfio/platform/vfio_platform.c | 128 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 125 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
> > index f7db5c0..ee96078 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;
> >  	}
> > @@ -150,13 +151,134 @@ 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 *io;
> > +	int i;
> > +
> > +	for (i = 0; i < vdev->num_regions; i++) {
> > +		struct vfio_platform_region region = vdev->region[i];
> > +		unsigned int done = 0;
> > +		loff_t off;
> > +
> > +		if ((*ppos < region.addr)
> > +		     || (*ppos + count - 1) >= (region.addr + region.size))
> > +			continue;
> > +
> > +		io = ioremap_nocache(region.addr, region.size);
> > +
> > +		off = *ppos - region.addr;
> > +
> > +		while (count) {
> > +			size_t filled;
> > +
> > +			if (count >= 4 && !(off % 4)) {
> > +				u32 val;
> > +
> > +				val = ioread32(io + off);
> 
> Hi Antonios,
> 
> I suspect there is an issue with the read address. Indeed io being an
> unsigned int* the read address is io + off x sizeof (unsigned int) ie.
> io+ offx4  whereas we expect to read io + off. declaring io as a void*
> corrects the issue (or void __iomem *io). Same issue on write.

Yep, the same code for vfio/pci uses void __iomem *io.  Thanks,

Alex

[RFC PATCH v4 00/10] VFIO support for platform devices

[Antonios Motakis]

v4 of this series is functionally identical to v3 for VFIO_PLATFORM. The only
change is the inclusion of Kim Phillips' patch to expose driver_probe_device()
and the implementation of a binding mechanism for arbitrary devices via a file
in sysfs. The latter has been folded in the skeleton patch (04/10).

This patch series aims to implement VFIO support for platform devices that
reside behind an IOMMU. Examples of such devices are devices behind an ARM SMMU,
or behind a Samsung Exynos System MMU.

The API used is based on the existing VFIO API that is also used with PCI
devices. Only devices that include a basic set of IRQs and memory regions are
targeted; devices with complex relationships with other devices on a device tree
are not taken into account at this stage.

A copy with all the dependencies applied can be cloned from branch
vfio-platform-v4 at git-9UaJU3cA/F/QT0dZR+AlfA@public.gmane.org:virtualopensystems/linux-kvm-arm.git

This code can also be tested on ARM FastModels using the following test cases:
 - A user space implementation via VFIO for the PL330 on the FastModels:
   git-9UaJU3cA/F/QT0dZR+AlfA@public.gmane.org:virtualopensystems/pl330-vfio-test.git
 - A QEMU prototype, also based on the PL330:
   git-9UaJU3cA/F/QT0dZR+AlfA@public.gmane.org:virtualopensystems/qemu.git pl330-vfio-dev

   We have written detailed instructions on how to build and run these tests:
   http://www.virtualopensystems.com/en/solutions/guides/vfio-on-arm/

The following IOCTLs have been found to be working on FastModels with an
ARM SMMU (MMU400). Testing was based on the ARM PL330 DMA Controller featured
on those models.
 - VFIO_GET_API_VERSION
 - VFIO_CHECK_EXTENSION

The TYPE1 fix proposed here enables the following IOCTLs:
 - VFIO_GROUP_GET_STATUS
 - VFIO_GROUP_SET_CONTAINER
 - VFIO_SET_IOMMU
 - VFIO_IOMMU_GET_INFO
 - VFIO_IOMMU_MAP_DMA
     For this ioctl specifically, a new flag has been added:
     VFIO_DMA_MAP_FLAG_EXEC. This flag is taken into account on systems with an
     ARM SMMU.

The VFIO platform driver proposed here implements the following:
 - VFIO_GROUP_GET_DEVICE_FD
 - VFIO_DEVICE_GET_INFO
 - VFIO_DEVICE_GET_REGION_INFO
 - VFIO_DEVICE_GET_IRQ_INFO
 - VFIO_DEVICE_SET_IRQS
     IRQs are implemented partially using this ioctl. Handling incoming
     interrupts with an eventfd is supported, as is masking and unmasking.
     Level sensitive interrupts are automasked. What is not implemented is
     masking/unmasking via eventfd.

In addition, the VFIO platform driver implements the following through
the VFIO device file descriptor:
 - MMAPing memory regions to the virtual address space of the VFIO user.
 - Read / write of memory regions directly through the file descriptor.

What still needs to be done, includes:
 - Eventfd for masking/unmasking
 - Extend the driver and API for device tree metadata
 - QEMU / KVM integration
 - Device specific functionality (e.g. VFIO_DEVICE_RESET)
 - Improve VFIO_IOMMU_TYPE1 driver to support multiple buses at the same time
 - Bind to ARM AMBA devices
 - IOMMUs with nested page tables (Stage 1 & 2 translation on ARM SMMUs)

Changes since v3:
 - Use Kim Phillips' driver_probe_device()
Changes since v2:
 - Fixed Read/Write and MMAP on device regions
 - Removed dependency on Device Tree
 - Interrupts support
 - Interrupt masking/unmasking
 - Automask level sensitive interrupts
 - Introduced VFIO_DMA_MAP_FLAG_EXEC
 - Code clean ups

Antonios Motakis (9):
  VFIO_IOMMU_TYPE1: Introduce the VFIO_DMA_MAP_FLAG_EXEC flag
  VFIO_IOMMU_TYPE1: workaround to build for platform devices
  VFIO_PLATFORM: Initial skeleton of VFIO support for platform devices
  VFIO_PLATFORM: Return info for device and its memory mapped IO regions
  VFIO_PLATFORM: Read and write support for the device fd
  VFIO_PLATFORM: Support MMAP of MMIO regions
  VFIO_PLATFORM: Return IRQ info
  VFIO_PLATFORM: Initial interrupts support
  VFIO_PLATFORM: Support for maskable and automasked interrupts

Kim Phillips (1):
  driver core: export driver_probe_device()

 drivers/base/base.h                           |   1 -
 drivers/base/dd.c                             |   1 +
 drivers/vfio/Kconfig                          |   3 +-
 drivers/vfio/Makefile                         |   1 +
 drivers/vfio/platform/Kconfig                 |   9 +
 drivers/vfio/platform/Makefile                |   4 +
 drivers/vfio/platform/vfio_platform.c         | 493 ++++++++++++++++++++++++++
 drivers/vfio/platform/vfio_platform_irq.c     | 289 +++++++++++++++
 drivers/vfio/platform/vfio_platform_private.h |  50 +++
 drivers/vfio/vfio_iommu_type1.c               |  27 +-
 include/linux/device.h                        |   1 +
 include/uapi/linux/vfio.h                     |   2 +
 12 files changed, 874 insertions(+), 7 deletions(-)
 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_irq.c
 create mode 100644 drivers/vfio/platform/vfio_platform_private.h

-- 
1.8.3.2

[RFC PATCH v4 06/10] 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-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
Tested-by: Alvise Rigo <a.rigo-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
---
 drivers/vfio/platform/vfio_platform.c | 128 +++++++++++++++++++++++++++++++++-
 1 file changed, 125 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index f7db5c0..ee96078 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;
 	}
@@ -150,13 +151,134 @@ 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 *io;
+	int i;
+
+	for (i = 0; i < vdev->num_regions; i++) {
+		struct vfio_platform_region region = vdev->region[i];
+		unsigned int done = 0;
+		loff_t off;
+
+		if ((*ppos < region.addr)
+		     || (*ppos + count - 1) >= (region.addr + region.size))
+			continue;
+
+		io = ioremap_nocache(region.addr, region.size);
+
+		off = *ppos - region.addr;
+
+		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;
+	}
+
+	return -EFAULT;
+
+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 *io;
+	int i;
+
+	for (i = 0; i < vdev->num_regions; i++) {
+		struct vfio_platform_region region = vdev->region[i];
+		unsigned int done = 0;
+		loff_t off;
+
+		if ((*ppos < region.addr)
+		     || (*ppos + count - 1) >= (region.addr + region.size))
+			continue;
+
+		io = ioremap_nocache(region.addr, region.size);
+
+		off = *ppos - region.addr;
+
+		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;
+	}
+
+	return -EINVAL;
+
+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 v4 06/10] VFIO_PLATFORM: Read and write support for the device fd

[Alex Williamson]

On Sat, 2014-02-08 at 18:29 +0100, 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.
> 
> Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> Tested-by: Alvise Rigo <a.rigo-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> ---
>  drivers/vfio/platform/vfio_platform.c | 128 +++++++++++++++++++++++++++++++++-
>  1 file changed, 125 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
> index f7db5c0..ee96078 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;
>  	}
> @@ -150,13 +151,134 @@ 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 *io;
> +	int i;
> +
> +	for (i = 0; i < vdev->num_regions; i++) {
> +		struct vfio_platform_region region = vdev->region[i];
> +		unsigned int done = 0;
> +		loff_t off;
> +
> +		if ((*ppos < region.addr)
> +		     || (*ppos + count - 1) >= (region.addr + region.size))
> +			continue;

Perhaps there's something to be said for vfio-pci's use of fixed offsets
to have a direct offset to index lookup.

> +
> +		io = ioremap_nocache(region.addr, region.size);

This must incur some overhead per access.

> +
> +		off = *ppos - region.addr;
> +
> +		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;

For vfio-pci we've decided that these interfaces are always little
endian, have you considered whether it makes sense to do something
similar here?  Thanks,

Alex

> +
> +				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;
> +	}
> +
> +	return -EFAULT;
> +
> +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 *io;
> +	int i;
> +
> +	for (i = 0; i < vdev->num_regions; i++) {
> +		struct vfio_platform_region region = vdev->region[i];
> +		unsigned int done = 0;
> +		loff_t off;
> +
> +		if ((*ppos < region.addr)
> +		     || (*ppos + count - 1) >= (region.addr + region.size))
> +			continue;
> +
> +		io = ioremap_nocache(region.addr, region.size);
> +
> +		off = *ppos - region.addr;
> +
> +		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;
> +	}
> +
> +	return -EINVAL;
> +
> +err:
> +	iounmap(io);
> +	return -EFAULT;
>  }
>  
>  static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)

Re: [RFC PATCH v4 06/10] VFIO_PLATFORM: Read and write support for the device fd

[Scott Wood]

On Mon, 2014-02-10 at 15:45 -0700, Alex Williamson wrote:
> On Sat, 2014-02-08 at 18:29 +0100, 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.
> > 
> > Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> > Tested-by: Alvise Rigo <a.rigo-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> > ---
> >  drivers/vfio/platform/vfio_platform.c | 128 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 125 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
> > index f7db5c0..ee96078 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;
> >  	}
> > @@ -150,13 +151,134 @@ 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 *io;
> > +	int i;
> > +
> > +	for (i = 0; i < vdev->num_regions; i++) {
> > +		struct vfio_platform_region region = vdev->region[i];
> > +		unsigned int done = 0;
> > +		loff_t off;
> > +
> > +		if ((*ppos < region.addr)
> > +		     || (*ppos + count - 1) >= (region.addr + region.size))
> > +			continue;
> 
> Perhaps there's something to be said for vfio-pci's use of fixed offsets
> to have a direct offset to index lookup.
> 
> > +
> > +		io = ioremap_nocache(region.addr, region.size);
> 
> This must incur some overhead per access.

There's mmap() if you want fast...  Given the limited ioremap space on
32-bit, I can see not wanting to map everything that the user has open
all the time -- but in that case, wouldn't it be better to just map one
page here rather than the whole region?

> > +
> > +		off = *ppos - region.addr;
> > +
> > +		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;
> 
> For vfio-pci we've decided that these interfaces are always little
> endian, have you considered whether it makes sense to do something
> similar here?  Thanks,

ioread32() is little endian -- but since read() puts its result in the
caller's memory buffer (rather than a register return), I think it makes
more sense to preserve byte-invariance -- similar to the conclusion of
the recent KVM MMIO API clarification discussion.  Then the VFIO user
would use the same type of access (byte swapped or not) to access the
read() buffer that they would have used to access the register directly.

Forcing little endian is a better fit for PCI (which is inherently
little endian) than for platform devices which can be either endianness.

-Scott

Re: [RFC PATCH v4 06/10] VFIO_PLATFORM: Read and write support for the device fd

[Alex Williamson]

On Mon, 2014-02-10 at 17:12 -0600, Scott Wood wrote:
> On Mon, 2014-02-10 at 15:45 -0700, Alex Williamson wrote:
> > On Sat, 2014-02-08 at 18:29 +0100, 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.
> > > 
> > > Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> > > Tested-by: Alvise Rigo <a.rigo-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> > > ---
> > >  drivers/vfio/platform/vfio_platform.c | 128 +++++++++++++++++++++++++++++++++-
> > >  1 file changed, 125 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
> > > index f7db5c0..ee96078 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;
> > >  	}
> > > @@ -150,13 +151,134 @@ 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 *io;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < vdev->num_regions; i++) {
> > > +		struct vfio_platform_region region = vdev->region[i];
> > > +		unsigned int done = 0;
> > > +		loff_t off;
> > > +
> > > +		if ((*ppos < region.addr)
> > > +		     || (*ppos + count - 1) >= (region.addr + region.size))
> > > +			continue;
> > 
> > Perhaps there's something to be said for vfio-pci's use of fixed offsets
> > to have a direct offset to index lookup.
> > 
> > > +
> > > +		io = ioremap_nocache(region.addr, region.size);
> > 
> > This must incur some overhead per access.
> 
> There's mmap() if you want fast...  Given the limited ioremap space on
> 32-bit, I can see not wanting to map everything that the user has open
> all the time -- but in that case, wouldn't it be better to just map one
> page here rather than the whole region?
> 
> > > +
> > > +		off = *ppos - region.addr;
> > > +
> > > +		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;
> > 
> > For vfio-pci we've decided that these interfaces are always little
> > endian, have you considered whether it makes sense to do something
> > similar here?  Thanks,
> 
> ioread32() is little endian -- but since read() puts its result in the
> caller's memory buffer (rather than a register return), I think it makes
> more sense to preserve byte-invariance -- similar to the conclusion of
> the recent KVM MMIO API clarification discussion.  Then the VFIO user
> would use the same type of access (byte swapped or not) to access the
> read() buffer that they would have used to access the register directly.
> 
> Forcing little endian is a better fit for PCI (which is inherently
> little endian) than for platform devices which can be either endianness.

Ok, works for me.  Thanks,

Alex