[Qemu-devel] [RFC PATCH v2 0/3] [RFC PATCH 0/3] VFIO support for AMBA devices

[Qemu-devel] [RFC PATCH v2 0/3] [RFC PATCH 0/3] VFIO support for AMBA devices

[Baptiste Reynal]

The following series add VFIO support for AMBA devices.

It introduces multiple compatible string support to deal with arm,primecell
compatible string.

The VFIOPlatformDevice now checks for this string and performs amba specific
operations if it is present (change path of the device, add clock in the fd).

This patch applies on top of http://git.linaro.org/people/eric.auger/qemu.git
on branch vfio_integ_v9_rc6_official_dynsysbus_v6 for irqfd support.

TODO:
- The IRQ forwarding doesn't work.
- VFIO_GET_DEVTREE_INFO integration.

The series has been tested using PL330 device, irq forwarding doesn't work.
(The test hangs on the first IRQ).
The test performed relies on a special guest kernel, which can be found here
http://github.com/virtualopensystems/linux-kvm-arm.git on branch
vfio-platform-v6-guide, with CONFIG_KVM, CONFIG_ARM_SMMU, CONFIG_PL330_DMA
and CONFIG_VOSYS_DMATEST enabled.

Run qemu using this command:
./qemu-system-arm -M virt -m 512M -enable-kvm \
        -device vfio-pl330,host="2c0a0000.dma",id=dma,x-irqfd=false,x-forward=false \
        -append "earlyprintk console=ttyAMA0" \
        -kernel zImage -nographic

Then run DMA test:
mount -t debugfs none /sys/kernel/debug
echo 1 > /sys/kernel/debug/vosys_dmatest/start

Results can be checked using dmesg.

Changes since v1:
- Remove add_generic_fdt_node
- Refactor add_calxeda_midway_xgmac_fdt_node for code reuse
- Add add_arm_pl330_fdt_node

Baptiste Reynal (3):
  hw/vfio/sysbus-fdt: refactoring
  hw/vfio: amba device support
  hw/vfio: add pl330 device support

 hw/arm/sysbus-fdt.c           | 180 ++++++++++++++++++++++++++++++++++++------
 hw/vfio/Makefile.objs         |   1 +
 hw/vfio/pl330.c               |  41 ++++++++++
 hw/vfio/platform.c            |  15 +++-
 include/hw/vfio/vfio-common.h |   1 +
 include/hw/vfio/vfio-pl330.h  |  26 ++++++
 6 files changed, 235 insertions(+), 29 deletions(-)
 create mode 100644 hw/vfio/pl330.c
 create mode 100644 include/hw/vfio/vfio-pl330.h

-- 
2.2.1

[Qemu-devel] [RFC PATCH v2 1/3] hw/vfio/sysbus-fdt: refactoring

[Baptiste Reynal]

Creates set_interrupts_fdt_node and set_regions_fdt_node
for code reusability.

Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 hw/arm/sysbus-fdt.c | 102 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 73 insertions(+), 29 deletions(-)

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 15bb50c..f6ff8a7 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -58,6 +58,76 @@ typedef struct NodeCreationPair {
 /* Device Specific Code */
 
 /**
+ * set_interrupts_fdt_node
+ *
+ * Helper function, generate interrupts property for a given node
+ */
+static int set_interrupts_fdt_node(char *nodename, SysBusDevice *sbdev,
+        void *opaque, int type, int flags)
+{
+    PlatformBusFdtData *data = opaque;
+    PlatformBusDevice *pbus = data->pbus;
+    void *fdt = data->fdt;
+    uint32_t *irq_attr;
+    uint64_t irq_number;
+    int i, ret;
+    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
+    VFIODevice *vbasedev = &vdev->vbasedev;
+
+    irq_attr = g_new(uint32_t, vbasedev->num_irqs*3);
+
+    for (i = 0; i < vbasedev->num_irqs; i++) {
+        irq_number = platform_bus_get_irqn(pbus, sbdev , i)
+                         + data->irq_start;
+        irq_attr[3*i] = cpu_to_be32(type);
+        irq_attr[3*i+1] = cpu_to_be32(irq_number);
+        irq_attr[3*i+2] = cpu_to_be32(flags);
+    }
+
+   ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
+                     irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t));
+
+    g_free(irq_attr);
+
+   return ret;
+}
+
+/**
+ * set_regions_fdt_node
+ *
+ * Helper function, generate reg property for a given node
+ */
+static int set_regions_fdt_node(char *nodename, SysBusDevice *sbdev,
+        void *opaque)
+{
+    PlatformBusFdtData *data = opaque;
+    PlatformBusDevice *pbus = data->pbus;
+    void *fdt = data->fdt;
+    uint64_t *reg_attr;
+    uint64_t mmio_base;
+    int i, ret;
+    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
+    VFIODevice *vbasedev = &vdev->vbasedev;
+
+    reg_attr = g_new(uint64_t, vbasedev->num_regions*4);
+
+    for (i = 0; i < vbasedev->num_regions; i++) {
+        mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i);
+        reg_attr[4*i] = 1;
+        reg_attr[4*i+1] = mmio_base;
+        reg_attr[4*i+2] = 1;
+        reg_attr[4*i+3] = memory_region_size(&vdev->regions[i]->mem);
+    }
+
+    ret = qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "reg",
+                     vbasedev->num_regions*2, reg_attr);
+
+    g_free(reg_attr);
+
+    return ret;
+}
+
+/**
  * add_calxeda_midway_xgmac_fdt_node
  *
  * Generates a very simple node with following properties:
@@ -66,16 +136,12 @@ typedef struct NodeCreationPair {
 static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
 {
     PlatformBusFdtData *data = opaque;
-    PlatformBusDevice *pbus = data->pbus;
     void *fdt = data->fdt;
     const char *parent_node = data->pbus_node_name;
     int compat_str_len;
     char *nodename;
-    int i, ret;
-    uint32_t *irq_attr;
-    uint64_t *reg_attr;
+    int ret;
     uint64_t mmio_base;
-    uint64_t irq_number;
     VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
     VFIODevice *vbasedev = &vdev->vbasedev;
     Object *obj = OBJECT(sbdev);
@@ -92,35 +158,15 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
     qemu_fdt_setprop(fdt, nodename, "compatible",
                           vdev->compat, compat_str_len);
 
-    reg_attr = g_new(uint64_t, vbasedev->num_regions*4);
-
-    for (i = 0; i < vbasedev->num_regions; i++) {
-        mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i);
-        reg_attr[4*i] = 1;
-        reg_attr[4*i+1] = mmio_base;
-        reg_attr[4*i+2] = 1;
-        reg_attr[4*i+3] = memory_region_size(&vdev->regions[i]->mem);
-    }
+    ret = set_regions_fdt_node(nodename, sbdev, opaque);
 
-    ret = qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "reg",
-                     vbasedev->num_regions*2, reg_attr);
     if (ret < 0) {
         error_report("could not set reg property of node %s", nodename);
         goto fail;
     }
 
-    irq_attr = g_new(uint32_t, vbasedev->num_irqs*3);
+    ret = set_interrupts_fdt_node(nodename, sbdev, opaque, 0, 0x4);
 
-    for (i = 0; i < vbasedev->num_irqs; i++) {
-        irq_number = platform_bus_get_irqn(pbus, sbdev , i)
-                         + data->irq_start;
-        irq_attr[3*i] = cpu_to_be32(0);
-        irq_attr[3*i+1] = cpu_to_be32(irq_number);
-        irq_attr[3*i+2] = cpu_to_be32(0x4);
-    }
-
-   ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
-                     irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t));
     if (ret < 0) {
         error_report("could not set interrupts property of node %s",
                      nodename);
@@ -128,8 +174,6 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
     }
 
     g_free(nodename);
-    g_free(irq_attr);
-    g_free(reg_attr);
 
     return 0;
 
-- 
2.2.1

[Qemu-devel] [RFC PATCH v2 2/3] hw/vfio: amba device support

[Baptiste Reynal]

Add VFIO_DEVICE_TYPE_AMBA.
Differentiate amba and platform devices according to compatible string.

Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 hw/vfio/platform.c            | 15 ++++++++++++---
 include/hw/vfio/vfio-common.h |  1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 39eef08..0a96178 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -563,8 +563,13 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
     }
 
     /* Check that the host device exists */
-    snprintf(path, sizeof(path), "/sys/bus/platform/devices/%s/",
-             vbasedev->name);
+    if (vbasedev->type == VFIO_DEVICE_TYPE_AMBA) {
+        snprintf(path, sizeof(path), "/sys/bus/amba/devices/%s/",
+                vbasedev->name);
+    } else {
+        snprintf(path, sizeof(path), "/sys/bus/platform/devices/%s/",
+                vbasedev->name);
+    }
 
     if (stat(path, &st) < 0) {
         error_report("vfio: error: no such host device: %s", path);
@@ -661,7 +666,11 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
     VFIODevice *vbasedev = &vdev->vbasedev;
     int i, ret;
 
-    vbasedev->type = VFIO_DEVICE_TYPE_PLATFORM;
+    if (strstr(vdev->compat, "arm,primecell")) {
+        vbasedev->type = VFIO_DEVICE_TYPE_AMBA;
+    } else {
+        vbasedev->type = VFIO_DEVICE_TYPE_PLATFORM;
+    }
     vbasedev->ops = &vfio_platform_ops;
 
 #ifdef CONFIG_KVM
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 58fd786..2f1b09c 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -49,6 +49,7 @@ extern int vfio_kvm_device_fd;
 enum {
     VFIO_DEVICE_TYPE_PCI = 0,
     VFIO_DEVICE_TYPE_PLATFORM = 1,
+    VFIO_DEVICE_TYPE_AMBA = 2,
 };
 
 typedef struct VFIORegion {
-- 
2.2.1

[Qemu-devel] [RFC PATCH v2 3/3] hw/vfio: add pl330 device support

[Baptiste Reynal]

Create a meta-device for PL330 DMA.
Add add_arm_pl330_fdt_node function, with multiple compatible string
and clocks support.

Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 hw/arm/sysbus-fdt.c          | 84 ++++++++++++++++++++++++++++++++++++++++++++
 hw/vfio/Makefile.objs        |  1 +
 hw/vfio/pl330.c              | 41 +++++++++++++++++++++
 include/hw/vfio/vfio-pl330.h | 26 ++++++++++++++
 4 files changed, 152 insertions(+)
 create mode 100644 hw/vfio/pl330.c
 create mode 100644 include/hw/vfio/vfio-pl330.h

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index f6ff8a7..efdeea7 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -28,6 +28,9 @@
 #include "sysemu/sysemu.h"
 #include "hw/vfio/vfio-platform.h"
 #include "hw/vfio/vfio-calxeda-xgmac.h"
+#include "hw/vfio/vfio-pl330.h"
+
+#include <libfdt.h>
 
 /*
  * internal struct that contains the information to create dynamic
@@ -182,9 +185,90 @@ fail:
    return -1;
 }
 
+/**
+ * add_arm_pl330_fdt_node
+ *
+ * Generates a very simple node with following properties:
+ * compatible string, regs, interrupts, clocks, clock-names
+ */
+static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque)
+{
+    PlatformBusFdtData *data = opaque;
+    void *fdt = data->fdt;
+    const char *parent_node = data->pbus_node_name;
+    int compat_str_len;
+    char *nodename;
+    int ret;
+    uint64_t mmio_base;
+    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
+    VFIODevice *vbasedev = &vdev->vbasedev;
+    Object *obj = OBJECT(sbdev);
+
+    mmio_base = object_property_get_int(obj, "mmio[0]", NULL);
+
+    nodename = g_strdup_printf("%s/%s@%" PRIx64, parent_node,
+                               vbasedev->name,
+                               mmio_base);
+
+    qemu_fdt_add_subnode(fdt, nodename);
+
+    /*
+     * Process compatible string to deal with multiple strings
+     * (; is replaced by \0)
+     */
+    char *compat = g_strdup(vdev->compat);
+    compat_str_len = strlen(compat) + 1;
+
+    char *semicolon = compat;
+    while ((semicolon = strchr(semicolon, ';')) != NULL) {
+        *semicolon = '\0';
+    }
+
+    qemu_fdt_setprop(fdt, nodename, "compatible",
+                          compat, compat_str_len);
+
+    /* Setup clocks for AMBA device */
+    /* Check clock existence */
+    ret = fdt_path_offset(fdt, "/apb-pclk");
+
+    if (ret < 0) {
+        error_report("could not set clocks property of node %s", nodename);
+    } else {
+        qemu_fdt_setprop_cells(fdt, nodename, "clocks",
+                qemu_fdt_getprop_cell(fdt, "/apb-pclk", "phandle"));
+        char clock_names[] = "apb_pclk";
+        qemu_fdt_setprop(fdt, nodename, "clock-names", clock_names,
+                sizeof(clock_names));
+    }
+
+    ret = set_regions_fdt_node(nodename, sbdev, opaque);
+
+    if (ret < 0) {
+        error_report("could not set reg property of node %s", nodename);
+        goto fail;
+    }
+
+    ret = set_interrupts_fdt_node(nodename, sbdev, opaque, 0, 0x4);
+
+    if (ret < 0) {
+        error_report("could not set interrupts property of node %s",
+                     nodename);
+        goto fail;
+    }
+
+    g_free(nodename);
+
+    return 0;
+
+fail:
+
+   return -1;
+}
+
 /* list of supported dynamic sysbus devices */
 static const NodeCreationPair add_fdt_node_functions[] = {
 {TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
+{TYPE_VFIO_PL330, add_arm_pl330_fdt_node},
 {"", NULL}, /*last element*/
 };
 
diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
index 913ab14..be3023b 100644
--- a/hw/vfio/Makefile.objs
+++ b/hw/vfio/Makefile.objs
@@ -3,4 +3,5 @@ obj-$(CONFIG_SOFTMMU) += common.o
 obj-$(CONFIG_PCI) += pci.o
 obj-$(CONFIG_SOFTMMU) += platform.o
 obj-$(CONFIG_SOFTMMU) += calxeda_xgmac.o
+obj-$(CONFIG_SOFTMMU) += pl330.o
 endif
diff --git a/hw/vfio/pl330.c b/hw/vfio/pl330.c
new file mode 100644
index 0000000..a409024
--- /dev/null
+++ b/hw/vfio/pl330.c
@@ -0,0 +1,41 @@
+#include "hw/vfio/vfio-pl330.h"
+
+static void pl330_realize(DeviceState *dev, Error **errp)
+{
+    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(dev);
+    VFIOPl330DeviceClass *k = VFIO_PL330_DEVICE_GET_CLASS(dev);
+
+    vdev->compat = g_strdup("arm,pl330;arm,primecell");
+
+    k->parent_realize(dev, errp);
+}
+
+static const VMStateDescription vfio_platform_vmstate = {
+    .name = TYPE_VFIO_PL330,
+    .unmigratable = 1,
+};
+
+static void vfio_pl330_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VFIOPl330DeviceClass *vpc =
+        VFIO_PL330_DEVICE_CLASS(klass);
+    vpc->parent_realize = dc->realize;
+    dc->realize = pl330_realize;
+    dc->desc = "VFIO PL330";
+}
+
+static const TypeInfo vfio_pl330_dev_info = {
+    .name = TYPE_VFIO_PL330,
+    .parent = TYPE_VFIO_PLATFORM,
+    .instance_size = sizeof(VFIOPl330Device),
+    .class_init = vfio_pl330_class_init,
+    .class_size = sizeof(VFIOPl330DeviceClass),
+};
+
+static void register_pl330_dev_type(void)
+{
+    type_register_static(&vfio_pl330_dev_info);
+}
+
+type_init(register_pl330_dev_type)
diff --git a/include/hw/vfio/vfio-pl330.h b/include/hw/vfio/vfio-pl330.h
new file mode 100644
index 0000000..1cdf039
--- /dev/null
+++ b/include/hw/vfio/vfio-pl330.h
@@ -0,0 +1,26 @@
+#ifndef HW_VFIO_VFIO_PL330
+#define HW_VFIO_VFIO_PL330
+
+#include "hw/vfio/vfio-platform.h"
+
+#define TYPE_VFIO_PL330 "vfio-pl330"
+
+typedef struct VFIOPl330Device {
+    VFIOPlatformDevice vdev;
+} VFIOPl330Device;
+
+typedef struct VFIOPl330DeviceClass {
+    VFIOPlatformDeviceClass parent_class;
+    DeviceRealize parent_realize;
+} VFIOPl330DeviceClass;
+
+#define VFIO_PL330_DEVICE(obj) \
+     OBJECT_CHECK(VFIOPl330Device, (obj), TYPE_VFIO_PL330)
+#define VFIO_PL330_DEVICE_CLASS(klass) \
+     OBJECT_CLASS_CHECK(VFIOPl330DeviceClass, (klass), \
+                        TYPE_VFIO_PL330)
+#define VFIO_PL330_DEVICE_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(VFIOPl330DeviceClass, (obj), \
+                        TYPE_VFIO_PL330)
+
+#endif
-- 
2.2.1

Re: [Qemu-devel] [RFC PATCH v2 1/3] hw/vfio/sysbus-fdt: refactoring

[Eric Auger]

Hi Baptiste,

On 12/22/2014 05:23 PM, Baptiste Reynal wrote:
> Creates set_interrupts_fdt_node and set_regions_fdt_node
> for code reusability.
> 
> Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> ---
>  hw/arm/sysbus-fdt.c | 102 +++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 73 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 15bb50c..f6ff8a7 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -58,6 +58,76 @@ typedef struct NodeCreationPair {
>  /* Device Specific Code */
>  
>  /**
> + * set_interrupts_fdt_node
> + *
> + * Helper function, generate interrupts property for a given node
> + */
> +static int set_interrupts_fdt_node(char *nodename, SysBusDevice *sbdev,
> +        void *opaque, int type, int flags)
> +{
> +    PlatformBusFdtData *data = opaque;
> +    PlatformBusDevice *pbus = data->pbus;
> +    void *fdt = data->fdt;
> +    uint32_t *irq_attr;
> +    uint64_t irq_number;
> +    int i, ret;
> +    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
> +    VFIODevice *vbasedev = &vdev->vbasedev;
> +
> +    irq_attr = g_new(uint32_t, vbasedev->num_irqs*3);
> +
> +    for (i = 0; i < vbasedev->num_irqs; i++) {
> +        irq_number = platform_bus_get_irqn(pbus, sbdev , i)
> +                         + data->irq_start;
> +        irq_attr[3*i] = cpu_to_be32(type);
> +        irq_attr[3*i+1] = cpu_to_be32(irq_number);
> +        irq_attr[3*i+2] = cpu_to_be32(flags);
> +    }
> +
> +   ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
> +                     irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t));
> +
> +    g_free(irq_attr);
> +
> +   return ret;
> +}
> +
> +/**
> + * set_regions_fdt_node
> + *
> + * Helper function, generate reg property for a given node
> + */
> +static int set_regions_fdt_node(char *nodename, SysBusDevice *sbdev,
> +        void *opaque)
> +{
> +    PlatformBusFdtData *data = opaque;
> +    PlatformBusDevice *pbus = data->pbus;
> +    void *fdt = data->fdt;
> +    uint64_t *reg_attr;
> +    uint64_t mmio_base;
> +    int i, ret;
> +    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
> +    VFIODevice *vbasedev = &vdev->vbasedev;
> +
> +    reg_attr = g_new(uint64_t, vbasedev->num_regions*4);
> +
> +    for (i = 0; i < vbasedev->num_regions; i++) {
> +        mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i);
> +        reg_attr[4*i] = 1;
> +        reg_attr[4*i+1] = mmio_base;
> +        reg_attr[4*i+2] = 1;
> +        reg_attr[4*i+3] = memory_region_size(&vdev->regions[i]->mem);
> +    }
> +
> +    ret = qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "reg",
> +                     vbasedev->num_regions*2, reg_attr);
> +
> +    g_free(reg_attr);
> +
> +    return ret;
> +}
> +
> +/**
>   * add_calxeda_midway_xgmac_fdt_node
>   *
>   * Generates a very simple node with following properties:
> @@ -66,16 +136,12 @@ typedef struct NodeCreationPair {
>  static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
>  {
>      PlatformBusFdtData *data = opaque;
> -    PlatformBusDevice *pbus = data->pbus;
>      void *fdt = data->fdt;
>      const char *parent_node = data->pbus_node_name;
>      int compat_str_len;
>      char *nodename;
> -    int i, ret;
> -    uint32_t *irq_attr;
> -    uint64_t *reg_attr;
> +    int ret;
>      uint64_t mmio_base;
> -    uint64_t irq_number;
>      VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
>      VFIODevice *vbasedev = &vdev->vbasedev;
>      Object *obj = OBJECT(sbdev);
> @@ -92,35 +158,15 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
>      qemu_fdt_setprop(fdt, nodename, "compatible",
>                            vdev->compat, compat_str_len);
>  
> -    reg_attr = g_new(uint64_t, vbasedev->num_regions*4);
> -
> -    for (i = 0; i < vbasedev->num_regions; i++) {
> -        mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i);
> -        reg_attr[4*i] = 1;
> -        reg_attr[4*i+1] = mmio_base;
> -        reg_attr[4*i+2] = 1;
> -        reg_attr[4*i+3] = memory_region_size(&vdev->regions[i]->mem);
> -    }
> +    ret = set_regions_fdt_node(nodename, sbdev, opaque);
>  
> -    ret = qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "reg",
> -                     vbasedev->num_regions*2, reg_attr);
>      if (ret < 0) {
>          error_report("could not set reg property of node %s", nodename);
>          goto fail;
>      }
>  
> -    irq_attr = g_new(uint32_t, vbasedev->num_irqs*3);
> +    ret = set_interrupts_fdt_node(nodename, sbdev, opaque, 0, 0x4);
>  
> -    for (i = 0; i < vbasedev->num_irqs; i++) {
> -        irq_number = platform_bus_get_irqn(pbus, sbdev , i)
> -                         + data->irq_start;
> -        irq_attr[3*i] = cpu_to_be32(0);
> -        irq_attr[3*i+1] = cpu_to_be32(irq_number);
> -        irq_attr[3*i+2] = cpu_to_be32(0x4);
> -    }
> -
> -   ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
> -                     irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t));
>      if (ret < 0) {
>          error_report("could not set interrupts property of node %s",
>                       nodename);
> @@ -128,8 +174,6 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
>      }
>  
>      g_free(nodename);
> -    g_free(irq_attr);
> -    g_free(reg_attr);
>  
>      return 0;
I think we should also take into account intermediate ret values.
Otherwise looks sensible to me.

Also you properly handled irq_attr and reg_attr dealloc here as opposed
to what I did in that version ;-)

maybe the title of your commit may be more explicit?

Best Regards

Eric
>  
> 

Re: [Qemu-devel] [RFC PATCH v2 2/3] hw/vfio: amba device support

[Eric Auger]

Baptiste,
On 12/22/2014 05:23 PM, Baptiste Reynal wrote:
> Add VFIO_DEVICE_TYPE_AMBA.
> Differentiate amba and platform devices according to compatible string.
> 
> Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> ---
>  hw/vfio/platform.c            | 15 ++++++++++++---
>  include/hw/vfio/vfio-common.h |  1 +
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index 39eef08..0a96178 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -563,8 +563,13 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
>      }
>  
>      /* Check that the host device exists */
> -    snprintf(path, sizeof(path), "/sys/bus/platform/devices/%s/",
> -             vbasedev->name);
> +    if (vbasedev->type == VFIO_DEVICE_TYPE_AMBA) {
> +        snprintf(path, sizeof(path), "/sys/bus/amba/devices/%s/",
> +                vbasedev->name);
> +    } else {
> +        snprintf(path, sizeof(path), "/sys/bus/platform/devices/%s/",
> +                vbasedev->name);
> +    }
>  
>      if (stat(path, &st) < 0) {
>          error_report("vfio: error: no such host device: %s", path);
> @@ -661,7 +666,11 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
>      VFIODevice *vbasedev = &vdev->vbasedev;
>      int i, ret;
>  
> -    vbasedev->type = VFIO_DEVICE_TYPE_PLATFORM;
> +    if (strstr(vdev->compat, "arm,primecell")) {
> +        vbasedev->type = VFIO_DEVICE_TYPE_AMBA;
> +    } else {
> +        vbasedev->type = VFIO_DEVICE_TYPE_PLATFORM;
> +    }
>      vbasedev->ops = &vfio_platform_ops;
>  
>  #ifdef CONFIG_KVM
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 58fd786..2f1b09c 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -49,6 +49,7 @@ extern int vfio_kvm_device_fd;
>  enum {
>      VFIO_DEVICE_TYPE_PCI = 0,
>      VFIO_DEVICE_TYPE_PLATFORM = 1,
> +    VFIO_DEVICE_TYPE_AMBA = 2,
>  };
>  
>  typedef struct VFIORegion {
> 
With last version I added a check of the flags in vfio_populate_device
following Alex advice. You will need to check AMBA too.

Best Regards

Eric

Re: [Qemu-devel] [RFC PATCH v2 3/3] hw/vfio: add pl330 device support

[Eric Auger]

On 12/22/2014 05:23 PM, Baptiste Reynal wrote:
> Create a meta-device for PL330 DMA.
> Add add_arm_pl330_fdt_node function, with multiple compatible string
> and clocks support.
> 
> Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> ---
>  hw/arm/sysbus-fdt.c          | 84 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/vfio/Makefile.objs        |  1 +
>  hw/vfio/pl330.c              | 41 +++++++++++++++++++++
>  include/hw/vfio/vfio-pl330.h | 26 ++++++++++++++
>  4 files changed, 152 insertions(+)
>  create mode 100644 hw/vfio/pl330.c
>  create mode 100644 include/hw/vfio/vfio-pl330.h
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index f6ff8a7..efdeea7 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -28,6 +28,9 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/vfio/vfio-platform.h"
>  #include "hw/vfio/vfio-calxeda-xgmac.h"
> +#include "hw/vfio/vfio-pl330.h"
> +
> +#include <libfdt.h>
>  
>  /*
>   * internal struct that contains the information to create dynamic
> @@ -182,9 +185,90 @@ fail:
>     return -1;
>  }
>  
> +/**
> + * add_arm_pl330_fdt_node
> + *
> + * Generates a very simple node with following properties:
> + * compatible string, regs, interrupts, clocks, clock-names
> + */
> +static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque)
> +{
> +    PlatformBusFdtData *data = opaque;
> +    void *fdt = data->fdt;
> +    const char *parent_node = data->pbus_node_name;
> +    int compat_str_len;
> +    char *nodename;
> +    int ret;
> +    uint64_t mmio_base;
> +    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
> +    VFIODevice *vbasedev = &vdev->vbasedev;
> +    Object *obj = OBJECT(sbdev);
> +
> +    mmio_base = object_property_get_int(obj, "mmio[0]", NULL);
> +
> +    nodename = g_strdup_printf("%s/%s@%" PRIx64, parent_node,
> +                               vbasedev->name,
> +                               mmio_base);
> +
> +    qemu_fdt_add_subnode(fdt, nodename);
> +
> +    /*
> +     * Process compatible string to deal with multiple strings
> +     * (; is replaced by \0)
> +     */
> +    char *compat = g_strdup(vdev->compat);
> +    compat_str_len = strlen(compat) + 1;
> +
> +    char *semicolon = compat;
> +    while ((semicolon = strchr(semicolon, ';')) != NULL) {
> +        *semicolon = '\0';
> +    }
> +
> +    qemu_fdt_setprop(fdt, nodename, "compatible",
> +                          compat, compat_str_len);
> +
> +    /* Setup clocks for AMBA device */
> +    /* Check clock existence */
> +    ret = fdt_path_offset(fdt, "/apb-pclk");
> +
> +    if (ret < 0) {
> +        error_report("could not set clocks property of node %s", nodename);
in case apb-clk is not found shouldn't we jump to a fail section?
> +    } else {
> +        qemu_fdt_setprop_cells(fdt, nodename, "clocks",
> +                qemu_fdt_getprop_cell(fdt, "/apb-pclk", "phandle"));
> +        char clock_names[] = "apb_pclk";
> +        qemu_fdt_setprop(fdt, nodename, "clock-names", clock_names,
> +                sizeof(clock_names));
> +    }
> +
> +    ret = set_regions_fdt_node(nodename, sbdev, opaque);
> +
> +    if (ret < 0) {
> +        error_report("could not set reg property of node %s", nodename);
> +        goto fail;
> +    }
> +
> +    ret = set_interrupts_fdt_node(nodename, sbdev, opaque, 0, 0x4);
> +
> +    if (ret < 0) {
> +        error_report("could not set interrupts property of node %s",
> +                     nodename);
> +        goto fail;
> +    }
> +
> +    g_free(nodename);
> +
> +    return 0;
> +
> +fail:
> +
I think we should free nodename too here.

Otherwise, to me, it fits with the original spirit now.

Thanks

Eric
> +   return -1;
> +}
> +
>  /* list of supported dynamic sysbus devices */
>  static const NodeCreationPair add_fdt_node_functions[] = {
>  {TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
> +{TYPE_VFIO_PL330, add_arm_pl330_fdt_node},
>  {"", NULL}, /*last element*/
>  };
>  
> diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
> index 913ab14..be3023b 100644
> --- a/hw/vfio/Makefile.objs
> +++ b/hw/vfio/Makefile.objs
> @@ -3,4 +3,5 @@ obj-$(CONFIG_SOFTMMU) += common.o
>  obj-$(CONFIG_PCI) += pci.o
>  obj-$(CONFIG_SOFTMMU) += platform.o
>  obj-$(CONFIG_SOFTMMU) += calxeda_xgmac.o
> +obj-$(CONFIG_SOFTMMU) += pl330.o
>  endif
> diff --git a/hw/vfio/pl330.c b/hw/vfio/pl330.c
> new file mode 100644
> index 0000000..a409024
> --- /dev/null
> +++ b/hw/vfio/pl330.c
> @@ -0,0 +1,41 @@
> +#include "hw/vfio/vfio-pl330.h"
> +
> +static void pl330_realize(DeviceState *dev, Error **errp)
> +{
> +    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(dev);
> +    VFIOPl330DeviceClass *k = VFIO_PL330_DEVICE_GET_CLASS(dev);
> +
> +    vdev->compat = g_strdup("arm,pl330;arm,primecell");
> +
> +    k->parent_realize(dev, errp);
> +}
> +
> +static const VMStateDescription vfio_platform_vmstate = {
> +    .name = TYPE_VFIO_PL330,
> +    .unmigratable = 1,
> +};
> +
> +static void vfio_pl330_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    VFIOPl330DeviceClass *vpc =
> +        VFIO_PL330_DEVICE_CLASS(klass);
> +    vpc->parent_realize = dc->realize;
> +    dc->realize = pl330_realize;
> +    dc->desc = "VFIO PL330";
> +}
> +
> +static const TypeInfo vfio_pl330_dev_info = {
> +    .name = TYPE_VFIO_PL330,
> +    .parent = TYPE_VFIO_PLATFORM,
> +    .instance_size = sizeof(VFIOPl330Device),
> +    .class_init = vfio_pl330_class_init,
> +    .class_size = sizeof(VFIOPl330DeviceClass),
> +};
> +
> +static void register_pl330_dev_type(void)
> +{
> +    type_register_static(&vfio_pl330_dev_info);
> +}
> +
> +type_init(register_pl330_dev_type)
> diff --git a/include/hw/vfio/vfio-pl330.h b/include/hw/vfio/vfio-pl330.h
> new file mode 100644
> index 0000000..1cdf039
> --- /dev/null
> +++ b/include/hw/vfio/vfio-pl330.h
> @@ -0,0 +1,26 @@
> +#ifndef HW_VFIO_VFIO_PL330
> +#define HW_VFIO_VFIO_PL330
> +
> +#include "hw/vfio/vfio-platform.h"
> +
> +#define TYPE_VFIO_PL330 "vfio-pl330"
> +
> +typedef struct VFIOPl330Device {
> +    VFIOPlatformDevice vdev;
> +} VFIOPl330Device;
> +
> +typedef struct VFIOPl330DeviceClass {
> +    VFIOPlatformDeviceClass parent_class;
> +    DeviceRealize parent_realize;
> +} VFIOPl330DeviceClass;
> +
> +#define VFIO_PL330_DEVICE(obj) \
> +     OBJECT_CHECK(VFIOPl330Device, (obj), TYPE_VFIO_PL330)
> +#define VFIO_PL330_DEVICE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(VFIOPl330DeviceClass, (klass), \
> +                        TYPE_VFIO_PL330)
> +#define VFIO_PL330_DEVICE_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(VFIOPl330DeviceClass, (obj), \
> +                        TYPE_VFIO_PL330)
> +
> +#endif
>