[Qemu-devel] [RFC v2 0/4] AMBA platform device passthrough

[Qemu-devel] [RFC v2 0/4] AMBA platform device passthrough

[Alvise Rigo]

These patches are the next iteration of "[Qemu-devel] [RFC 0/4] AMBA
platform device passthrough" and are based on the latest version of VFIO
for platform devices "[RFC PATCH v5 00/11] VFIO support for platform
devices" and the initial patch series "[Qemu-devel] [RFC v2 0/6] KVM
platform device passthrough".

- [PATCH 1/4] Addition of the exec flag to the DMA mappings. At the
  moment this is mandatory only for the ARM SMMU.
  Change since v1:
  Thanks to the new version of the VFIO API, now we can know from the
  kernel if the IOMMU requires the EXEC_FLAG.  For now, we always add it
  if supported.

- [PATCH 2/4] Possibility to bind a wider class of devices. In
  particular the patch proposes a solution to enhance a bit the device
  tree nodes generation, allowing to specify more than one compatibility
  property (handy for AMBA devices). This was required by the DMA330
  that needs "arm,pl330","arm,primecell" as compatibility string,
  together with a clock source defined in the device tree. In the future
  VFIO will be able to tell if we are dealing with an AMBA device;
  before that, we have to rely on the compatibility string to state
  that.

- [PATCH 3/4] Another approach to handle the EOI, starting from the
  assumption that we know in advance the location of the interrupt clear
  register of the device. This is of course another workaround and not
  the definitive solution until we can rely on a proper callback from
  the VGIC (something that we will see in a future version of VFIO for
  platform devices).
  Change since v1:
  The code is much simpler now and does not require the definition of an
  additional memory region which would be dma-mapped by the memory
  listener vfio_listener_region_add in hw/vfio/common.c. In this regard
  probably soon or later we will need a way to exclude some regions from
  being mapped "blindly" by the memory listener: this is for example
  what happens with the memory region of the device which is dma-mapped
  to itself.

- [PATCH 4/4] If event_notifier_init fails to create a new eventfd, it
  will fallback usign pipe/pipe2 and we end up passing to the kernel a
  wrong file descriptor. This patch force to check if eventfd has been
  used.

In order to easy the test of the VFIO API with platform devices, here:
https://github.com/virtualopensystems/vfio-host-test.git
is possible to find a handy test suite for an ARM host which will print the
devices that could be probed with VFIO and run some basic tests on a chosen
device. If the ARM DMA330 is tested, a device specific code (which was
introduced in the Virtual Open Systems guide
http://virtualopensystems.com/en/solutions/guides/vfio-on-arm) will be run to
verify even further the correct behaviour of the device. This suite has been
tested with ARM FastModels.

Alvise Rigo (4):
  Add EXEC_FLAG to VFIO DMA mappings
  Add AMBA devices support to VFIO
  MemoryRegion with EOI callbacks for VFIO Platform devices
  Always use eventfd as notifying mechanism

 hw/arm/virt.c              | 39 ++++++++++++++++---
 hw/vfio/common.c           |  9 +++++
 hw/vfio/platform.c         | 96 +++++++++++++++++++++++++++++++++++++++++++++-
 hw/vfio/vfio-common.h      |  1 +
 linux-headers/linux/vfio.h |  2 +
 5 files changed, 140 insertions(+), 7 deletions(-)

-- 
1.9.1

[Qemu-devel] [RFC v2 1/4] Add EXEC_FLAG to VFIO DMA mappings

[Alvise Rigo]

The flag is mandatory for the ARM SMMU so we always add it if the MMIO
handles it.

Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
---
 hw/vfio/common.c           | 9 +++++++++
 hw/vfio/vfio-common.h      | 1 +
 linux-headers/linux/vfio.h | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 9d1f723..a805c5d 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -107,6 +107,11 @@ static int vfio_dma_map(VFIOContainer *container, hwaddr iova,
         map.flags |= VFIO_DMA_MAP_FLAG_WRITE;
     }
 
+    /* add exec flag */
+    if (container->iommu_data.has_exec_cap) {
+        map.flags |= VFIO_DMA_MAP_FLAG_EXEC;
+    }
+
     /*
      * Try the mapping, if it fails with EBUSY, unmap the region and try
      * again.  This shouldn't be necessary, but we sometimes see it in
@@ -352,6 +357,10 @@ static int vfio_connect_container(VFIOGroup *group)
             return -errno;
         }
 
+        if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_IOMMU_PROT_EXEC)) {
+            container->iommu_data.has_exec_cap = true;
+        }
+
         container->iommu_data.type1.listener = vfio_memory_listener;
         container->iommu_data.release = vfio_listener_release;
 
diff --git a/hw/vfio/vfio-common.h b/hw/vfio/vfio-common.h
index 21148ef..1abbd1a 100644
--- a/hw/vfio/vfio-common.h
+++ b/hw/vfio/vfio-common.h
@@ -35,6 +35,7 @@ typedef struct VFIOContainer {
         union {
             VFIOType1 type1;
         };
+        bool has_exec_cap; /* support of exec capability by the IOMMU */
         void (*release)(struct VFIOContainer *);
     } iommu_data;
     QLIST_HEAD(, VFIOGroup) group_list;
diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index 17c58e0..95a02c5 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -24,6 +24,7 @@
 #define VFIO_TYPE1_IOMMU		1
 #define VFIO_SPAPR_TCE_IOMMU		2
 
+#define VFIO_IOMMU_PROT_EXEC		5
 /*
  * The IOCTL interface is designed for extensibility by embedding the
  * structure length (argsz) and flags into structures passed between
@@ -392,6 +393,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.9.1

[Qemu-devel] [RFC v2 2/4] Add AMBA devices support to VFIO

[Alvise Rigo]

The impossibility to add more then one compatibility property to the
device tree node was not permitting to bind AMBA devices.
Now we can add an arbitrary number of compatibility property values divided by
the character ";".

If the compatibility string contains the substring "arm,primecell", a
clock property will be added to the device tree node in order to allow the AMBA
bus code to probe the device.

Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
---
 hw/arm/virt.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1fb66ef..dadf5f0 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -344,6 +344,8 @@ static int vfio_init_func(QemuOpts *opts, void *opaque)
     size_t size;
     int i;
     uint32_t *irq_attr;
+    bool is_amba = false;
+    int compat_str_len;
 
     if (!driver) {
         qerror_report(QERR_MISSING_PARAMETER, "driver");
@@ -369,12 +371,31 @@ static int vfio_init_func(QemuOpts *opts, void *opaque)
 
         /*
          * process compatibility property string passed by end-user
-         * replaces / by ,
-         * currently a single property compatibility value is supported!
+         * replaces / by , and ; by NUL character
          */
         corrected_compat = g_strdup(*pcompat);
-        char *slash = strchr(corrected_compat, '/');
-        *slash = ',';
+        /*
+         * the total length of the string has to include also the last
+         * NUL char.
+         */
+        compat_str_len = strlen(corrected_compat) + 1;
+
+        char *str_ptr = corrected_compat;
+        while ((str_ptr = strchr(str_ptr, '/')) != NULL) {
+            *str_ptr = ',';
+        }
+
+        /* check if is an AMBA device */
+        str_ptr = corrected_compat;
+        if (strstr(str_ptr, "arm,primecell") != NULL) {
+            is_amba = true;
+        }
+
+        /* substitute ";" with the NUL char */
+        str_ptr = corrected_compat;
+        while ((str_ptr = strchr(str_ptr, ';')) != NULL) {
+            *str_ptr = '\0';
+        }
 
         sysbus_mmio_map(s, 0, vbi->avail_vfio_base);
 
@@ -383,11 +404,19 @@ static int vfio_init_func(QemuOpts *opts, void *opaque)
         qemu_fdt_add_subnode(vbi->fdt, nodename);
 
         qemu_fdt_setprop(vbi->fdt, nodename, "compatible",
-                             corrected_compat, strlen(corrected_compat));
+                             corrected_compat, compat_str_len);
 
         qemu_fdt_setprop_sized_cells(vbi->fdt, nodename,
                              "reg", 2, vbi->avail_vfio_base, 2, size);
 
+        if (is_amba) {
+            qemu_fdt_setprop_cells(vbi->fdt, nodename, "clocks",
+                                   vbi->clock_phandle);
+            char clock_names[] = "apb_pclk";
+            qemu_fdt_setprop(vbi->fdt, nodename, "clock-names", clock_names,
+                                                       sizeof(clock_names));
+        }
+
         irq_attr = g_malloc0(num_irqs*3*sizeof(uint32_t));
         for (i = 0; i < num_irqs; i++) {
             sysbus_connect_irq(s, i, vbi->pic[irq_start+i]);
-- 
1.9.1

[Qemu-devel] [RFC v2 3/4] MemoryRegion with EOI callbacks for VFIO Platform devices

[Alvise Rigo]

The user can specify the location of the memory region (register) used
by the guest driver to clear the pending interrupt; if enabled, this
mechanism will overlap to the default one (timer based).

The region is provided as command line property "intclr-region" of the
vfio-platform device. The property is a string "region_index;offset;size" where:

    region_index:   is the index of the memory region where the register
                    lives,
    offset:         offset of the register in the region,
    size:           size of the register.

example:
    -device vfio-platform,...,intclr-region="0;0x2c;4"

Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
---
 hw/vfio/platform.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 89 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index c4a4286..ec6a29e 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -40,6 +40,7 @@
 
 #include "vfio-common.h"
 
+/*#define DEBUG_VFIO 1*/
 #ifdef DEBUG_VFIO
 #define DPRINTF(fmt, ...) \
     do { fprintf(stderr, "vfio: %s: " fmt, __func__, ## __VA_ARGS__); } \
@@ -56,6 +57,11 @@
 
 #define TYPE_VFIO_PLATFORM "vfio-platform"
 
+struct intclr_region {
+    hwaddr offset;
+    hwaddr size;
+};
+
 typedef struct VFIORegion {
     off_t fd_offset; /* offset of region within device fd */
     int fd; /* device fd, allows us to pass VFIORegion as opaque data */
@@ -65,6 +71,7 @@ typedef struct VFIORegion {
     size_t size;
     uint32_t flags; /* VFIO region flags (rd/wr/mmap) */
     uint8_t nr; /* cache the region number for debug */
+    struct intclr_region intclr_reg;
 } VFIORegion;
 
 
@@ -99,6 +106,8 @@ typedef struct VFIODevice {
     QLIST_ENTRY(VFIODevice) next;
     struct VFIOGroup *group;
     QLIST_HEAD(, VFIOINTp) intp_list;
+    char *intclr_region_str; /* clear interrupt region string */
+    bool has_intclr_region;
 } VFIODevice;
 
 
@@ -120,6 +129,48 @@ void vfio_get_props(SysBusDevice *s, char **pname,
      *psize = vdev->regions[0].size;
 }
 
+static int parse_clrint_string(const char *intclr_str, uint32_t *id_reg,
+                                    hwaddr *off_reg, uint64_t *size_reg)
+{
+    char *str_ptr = g_strdup(intclr_str);
+    char *idx, *off, *size;
+
+    if (strlen(intclr_str) < 5) {
+        return -1;
+    }
+
+    idx = str_ptr;
+    str_ptr = strchr(str_ptr, ';');
+    if (!str_ptr || idx == str_ptr) {
+        return -1;
+    }
+    *str_ptr = '\0';
+
+    off = ++str_ptr;
+    str_ptr = strchr(str_ptr, ';');
+    if (!str_ptr || off == str_ptr) {
+        return -1;
+    }
+    *str_ptr = '\0';
+
+    size = ++str_ptr;
+    if (!*size) {
+        return -1;
+    }
+
+    *id_reg = strtol(idx, NULL, 10);
+    *off_reg = strtol(off, NULL, 0);
+    *size_reg = strtol(size, NULL, 10);
+
+    if (errno == EINVAL || errno == ERANGE) {
+        return -1;
+    }
+
+    DPRINTF("intclr region - id: %u offset: 0x%"HWADDR_PRIx" size: %"PRIx64"\n",
+                                                *id_reg, *off_reg, *size_reg);
+    return 0;
+}
+
 static void vfio_disable_irqindex(VFIODevice *vdev, int index)
 {
     struct vfio_irq_set irq_set = {
@@ -397,6 +448,8 @@ static void vfio_region_write(void *opaque, hwaddr addr,
                               uint64_t data, unsigned size)
 {
     VFIORegion *region = opaque;
+    VFIODevice *vdev = NULL;
+
     union {
         uint8_t byte;
         uint16_t word;
@@ -427,13 +480,22 @@ static void vfio_region_write(void *opaque, hwaddr addr,
     DPRINTF("(region %d, addr=0x%"HWADDR_PRIx", data= 0x%"PRIx64", %d)\n",
             region->nr, addr, data, size);
 
-    vfio_irq_eoi(container_of(region, VFIODevice, regions[region->nr]));
+    vdev = container_of(region, VFIODevice, regions[region->nr]);
+    struct intclr_region *intr = &region->intclr_reg;
+    /* If an interrupt clear region has been specified we clear the pending
+     * intterrupts only when the memory accesse is inside the region.
+     * */
+    if ((addr >= intr->offset && addr + size <= intr->offset + intr->size)
+                                            || !vdev->has_intclr_region) {
+        vfio_irq_eoi(vdev);
+    }
 
 }
 
 static uint64_t vfio_region_read(void *opaque, hwaddr addr, unsigned size)
 {
     VFIORegion *region = opaque;
+    VFIODevice *vdev = NULL;
     union {
         uint8_t byte;
         uint16_t word;
@@ -466,7 +528,10 @@ static uint64_t vfio_region_read(void *opaque, hwaddr addr, unsigned size)
     DPRINTF("(region %d, addr= 0x%"HWADDR_PRIx", data=%d) = 0x%"PRIx64"\n",
             region->nr, addr, size, data);
 
-    vfio_irq_eoi(container_of(region, VFIODevice, regions[region->nr]));
+    vdev = container_of(region, VFIODevice, regions[region->nr]);
+    if (!vdev->has_intclr_region) {
+        vfio_irq_eoi(vdev);
+    }
 
     return data;
 }
@@ -669,6 +734,17 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
         }
     }
 
+    hwaddr intclr_off = 0;
+    uint64_t intclr_size = 0;
+    uint32_t id = 0;
+    if (vdev->intclr_region_str) {
+        ret = parse_clrint_string(vdev->intclr_region_str, &id, &intclr_off,
+                                                              &intclr_size);
+    } else {
+        ret = -1;
+    }
+    vdev->has_intclr_region = (!ret) ? true : false;
+
     ret = vfio_get_device(group, path, vdev);
     if (ret) {
         error_report("vfio: failed to get device %s", path);
@@ -678,6 +754,16 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
 
     for (i = 0; i < vdev->num_regions; i++) {
         vfio_map_region(vdev, i);
+
+        if (id == i && vdev->has_intclr_region) {
+            struct intclr_region *intclr_reg = NULL;
+
+            intclr_reg = &(vdev->regions[i].intclr_reg);
+
+            intclr_reg->offset = intclr_off;
+            intclr_reg->size = intclr_size;
+        }
+
         sysbus_init_mmio(sbdev, &vdev->regions[i].mem);
     }
 }
@@ -729,6 +815,7 @@ typedef struct VFIOPlatformDeviceClass {
 static Property vfio_platform_dev_properties[] = {
 DEFINE_PROP_STRING("vfio_device", VFIODevice, name),
 DEFINE_PROP_STRING("compat", VFIODevice, compat),
+DEFINE_PROP_STRING("intclr-region", VFIODevice, intclr_region_str),
 DEFINE_PROP_UINT32("mmap-timeout-ms", VFIODevice, mmap_timeout, 1100),
 DEFINE_PROP_END_OF_LIST(),
 };
-- 
1.9.1

[Qemu-devel] [RFC v2 4/4] Always use eventfd as notifying mechanism

[Alvise Rigo]

When eventfd is not configured the method event_notifier_init fallbacks
to the pipe/pipe2 system call, causing an error in VFIO_DEVICE_SET_IRQS
since we pass to the kernel a file descriptor which is not created by
eventfd.

Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
---
 hw/vfio/platform.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index ec6a29e..40109c6 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -324,6 +324,11 @@ static int vfio_enable_intp(VFIODevice *vdev, unsigned int index)
     sysbus_init_irq(sbdev, &intp->qemuirq);
 
     ret = event_notifier_init(&intp->interrupt, 0);
+    if (!ret && (intp->interrupt.rfd != intp->interrupt.wfd)) {
+        /* event_notifier_init created a pipe instead of eventfd */
+        ret = -1;
+    }
+
     if (ret) {
         error_report("vfio: Error: event_notifier_init failed ");
         return ret;
-- 
1.9.1

Re: [Qemu-devel] [RFC v2 1/4] Add EXEC_FLAG to VFIO DMA mappings

[Eric Auger]

On 05/11/2014 07:13 PM, Alvise Rigo wrote:
> The flag is mandatory for the ARM SMMU so we always add it if the MMIO
> handles it.

Hi Alvise,

Refering to the root problem explanation found in
https://lkml.org/lkml/2014/2/8/176, I understand the problem is specific
to devices that fetch instructions from executable memory region
sections (XN =0).

Typically this is not the case of Midway xgmac which does not need
executable regions and hence does not need that change.

in
http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007095.html,
Will says most IOMMU mappings should be XN.

I am not knowledged enough on mem mapping settings to understand the
consequences of always setting XN=0, even for devices that do not need
request it.

Does anyone have an opinion on this?

Best Regards

Eric

> 
> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
> ---
>  hw/vfio/common.c           | 9 +++++++++
>  hw/vfio/vfio-common.h      | 1 +
>  linux-headers/linux/vfio.h | 2 ++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 9d1f723..a805c5d 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -107,6 +107,11 @@ static int vfio_dma_map(VFIOContainer *container, hwaddr iova,
>          map.flags |= VFIO_DMA_MAP_FLAG_WRITE;
>      }
>  
> +    /* add exec flag */
> +    if (container->iommu_data.has_exec_cap) {
> +        map.flags |= VFIO_DMA_MAP_FLAG_EXEC;
> +    }
> +
>      /*
>       * Try the mapping, if it fails with EBUSY, unmap the region and try
>       * again.  This shouldn't be necessary, but we sometimes see it in
> @@ -352,6 +357,10 @@ static int vfio_connect_container(VFIOGroup *group)
>              return -errno;
>          }
>  
> +        if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_IOMMU_PROT_EXEC)) {
> +            container->iommu_data.has_exec_cap = true;
> +        }
> +
>          container->iommu_data.type1.listener = vfio_memory_listener;
>          container->iommu_data.release = vfio_listener_release;
>  
> diff --git a/hw/vfio/vfio-common.h b/hw/vfio/vfio-common.h
> index 21148ef..1abbd1a 100644
> --- a/hw/vfio/vfio-common.h
> +++ b/hw/vfio/vfio-common.h
> @@ -35,6 +35,7 @@ typedef struct VFIOContainer {
>          union {
>              VFIOType1 type1;
>          };
> +        bool has_exec_cap; /* support of exec capability by the IOMMU */
>          void (*release)(struct VFIOContainer *);
>      } iommu_data;
>      QLIST_HEAD(, VFIOGroup) group_list;
> diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
> index 17c58e0..95a02c5 100644
> --- a/linux-headers/linux/vfio.h
> +++ b/linux-headers/linux/vfio.h
> @@ -24,6 +24,7 @@
>  #define VFIO_TYPE1_IOMMU		1
>  #define VFIO_SPAPR_TCE_IOMMU		2
>  
> +#define VFIO_IOMMU_PROT_EXEC		5
>  /*
>   * The IOCTL interface is designed for extensibility by embedding the
>   * structure length (argsz) and flags into structures passed between
> @@ -392,6 +393,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) */
>