[Qemu-devel] [RFC PATCH 0/4] VFIO platform: Use device properties API

[Qemu-devel] [RFC PATCH 0/4] VFIO platform: Use device properties API

[Baptiste Reynal]

This RFC shows the implementation on QEMU side of the device properties interface
presented in kernel patch series:
[RFC PATCH v3 0/3] vfio: platform: return device properties for a platform device
from branch vfio-device-properties-v3 on the repository:
https://github.com/virtualopensystems/linux-kvm-arm.git

When a VFIO device is bound to the VM, properties are queried to the host 
to fill the device tree.

One issue here is wheter a property may change between the host and the guest.
Currently, interrupt numbers and registers change, other information are kept
(including interrupt type and flags).

Regarding the clock, any primecell device requiring a clock is attached to 
apb-pclk.

DMA pl330 is used as an example. For this reason the last patch 
(3/3, arm,pl330 vfio device property) relies on the following
patch series: [RFC PATCH v2 0/3] VFIO support for AMBA devices.

Baptiste Reynal (4):
  linux-headers update
  hw/vfio/common.c : vfio_get_dev_property
  hw/arm/sysbus-fdt: vfio device property for interrupts
  hw/arm/sysbus-fdt: arm,pl330 vfio device property

 hw/arm/sysbus-fdt.c           | 134 ++++++++++++++++++++++++++++++++++++------
 hw/vfio/common.c              |  33 +++++++++++
 include/hw/vfio/vfio-common.h |   2 +
 linux-headers/linux/vfio.h    |  25 ++++++++
 4 files changed, 175 insertions(+), 19 deletions(-)

-- 
2.2.1

[Qemu-devel] [RFC PATCH 1/4] linux-headers update

[Baptiste Reynal]

Add VFIO device property constants

Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 linux-headers/linux/vfio.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index 0f21aa6..0c5f578 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -166,6 +166,31 @@ struct vfio_device_info {
 #define VFIO_DEVICE_GET_INFO		_IO(VFIO_TYPE, VFIO_BASE + 7)
 
 /**
+ * VFIO_DEVICE_GET_DEV_PROPERTY - _IOR(VFIO_TYPE, VFIO_BASE + 16,
+ *						struct vfio_devtree_info)
+ *
+ * Retrieve a device property, e.g. from a device tree if available.
+ * Caller will initialize data[] with a single string with the requested
+ * devicetree property name, and type depending on whether a array of strings
+ * or an array of u32 values is expected. On success, data[] will be extended
+ * with the requested information, either as an array of u32, or with a list
+ * of strings separated by the NULL terminating character.
+ * Return: 0 on success, -errno on failure.
+ */
+struct vfio_dev_property {
+	__u32	argsz;
+	__u32	type;
+#define VFIO_DEV_PROPERTY_TYPE_STRINGS	0
+#define VFIO_DEV_PROPERTY_TYPE_U8	1
+#define VFIO_DEV_PROPERTY_TYPE_U16	2
+#define VFIO_DEV_PROPERTY_TYPE_U32	3
+#define VFIO_DEV_PROPERTY_TYPE_U64	4
+	__u32	length;
+	__u8	data[];
+};
+#define VFIO_DEVICE_GET_DEV_PROPERTY	_IO(VFIO_TYPE, VFIO_BASE + 17)
+
+/**
  * VFIO_DEVICE_GET_REGION_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 8,
  *				       struct vfio_region_info)
  *
-- 
2.2.1

[Qemu-devel] [RFC PATCH 2/4] hw/vfio/common.c : vfio_get_dev_property

[Baptiste Reynal]

Add a function to handle ioctl VFIO_DEVICE_GET_DEV_PROPERTY
to retrieve properties from a VFIO device.

Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 hw/vfio/common.c              | 33 +++++++++++++++++++++++++++++++++
 include/hw/vfio/vfio-common.h |  2 ++
 2 files changed, 35 insertions(+)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index ba00ec9..698d2c4 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -958,3 +958,36 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
 
     return vfio_container_do_ioctl(as, groupid, req, param);
 }
+
+struct vfio_dev_property *vfio_get_dev_property(int device, const char *name,
+        unsigned int type)
+{
+    unsigned int length = 0;
+    struct vfio_dev_property *property = NULL;
+    int ret;
+
+    length = strlen(name) + 1;
+
+    while (1) {
+        unsigned int argsz = sizeof(struct vfio_dev_property) + length;
+        property = realloc(property, argsz);
+        property->argsz = argsz;
+        property->type = type;
+        strcpy((char *) property->data, name);
+
+        ret = ioctl(device, VFIO_DEVICE_GET_DEV_PROPERTY, property);
+
+        if (length < property->length) {
+            length = property->length;
+        } else {
+            break;
+        }
+    }
+
+    if (ret) {
+        g_free(property);
+        property = NULL;
+    }
+
+    return property;
+}
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 2f1b09c..9c649cd 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -149,6 +149,8 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as);
 void vfio_put_group(VFIOGroup *group);
 int vfio_get_device(VFIOGroup *group, const char *name,
                     VFIODevice *vbasedev);
+struct vfio_dev_property *vfio_get_dev_property(int device, const char *name,
+        unsigned int type);
 
 extern const MemoryRegionOps vfio_region_ops;
 extern const MemoryListener vfio_memory_listener;
-- 
2.2.1

Re: [Qemu-devel] [RFC PATCH 2/4] hw/vfio/common.c : vfio_get_dev_property

[Alex Williamson]

On Mon, 2015-01-12 at 14:21 +0100, Baptiste Reynal wrote:
> Add a function to handle ioctl VFIO_DEVICE_GET_DEV_PROPERTY
> to retrieve properties from a VFIO device.
> 
> Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> ---
>  hw/vfio/common.c              | 33 +++++++++++++++++++++++++++++++++
>  include/hw/vfio/vfio-common.h |  2 ++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index ba00ec9..698d2c4 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -958,3 +958,36 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>  
>      return vfio_container_do_ioctl(as, groupid, req, param);
>  }
> +
> +struct vfio_dev_property *vfio_get_dev_property(int device, const char *name,
> +        unsigned int type)
> +{
> +    unsigned int length = 0;
> +    struct vfio_dev_property *property = NULL;
> +    int ret;
> +
> +    length = strlen(name) + 1;
> +
> +    while (1) {
> +        unsigned int argsz = sizeof(struct vfio_dev_property) + length;
> +        property = realloc(property, argsz);

By my read, realloc() doesn't give zero'd memory, so property->length is
uninitialized here.

> +        property->argsz = argsz;
> +        property->type = type;
> +        strcpy((char *) property->data, name);
> +
> +        ret = ioctl(device, VFIO_DEVICE_GET_DEV_PROPERTY, property);

This ioctl might not exit.

> +        if (length < property->length) {

Which means this compares length to random memory and potentially causes
a segfault when trying to realloc.

What types of devices are going to have VFIO_DEVICE_GET_DEV_PROPERTY and
is this appropriate for common?  The error and return here leaves
something to be desired.  Maybe only return for a given error.

> +            length = property->length;
> +        } else {
> +            break;
> +        }
> +    }
> +
> +    if (ret) {
> +        g_free(property);
> +        property = NULL;
> +    }
> +
> +    return property;
> +}
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 2f1b09c..9c649cd 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -149,6 +149,8 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as);
>  void vfio_put_group(VFIOGroup *group);
>  int vfio_get_device(VFIOGroup *group, const char *name,
>                      VFIODevice *vbasedev);
> +struct vfio_dev_property *vfio_get_dev_property(int device, const char *name,
> +        unsigned int type);
>  
>  extern const MemoryRegionOps vfio_region_ops;
>  extern const MemoryListener vfio_memory_listener;

[Qemu-devel] [RFC PATCH 3/4] hw/arm/sysbus-fdt: vfio device property for interrupts

[Baptiste Reynal]

Use the VFIO device property API to retrieve interrupt
information (type and flags) during device node creation.

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

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index efdeea7..087e788 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -31,6 +31,7 @@
 #include "hw/vfio/vfio-pl330.h"
 
 #include <libfdt.h>
+#include <linux/vfio.h>
 
 /*
  * internal struct that contains the information to create dynamic
@@ -66,7 +67,7 @@ typedef struct NodeCreationPair {
  * 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)
+        void *opaque)
 {
     PlatformBusFdtData *data = opaque;
     PlatformBusDevice *pbus = data->pbus;
@@ -76,23 +77,32 @@ static int set_interrupts_fdt_node(char *nodename, SysBusDevice *sbdev,
     int i, ret;
     VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
     VFIODevice *vbasedev = &vdev->vbasedev;
+    struct vfio_dev_property *irq_prop;
+
+    irq_prop = vfio_get_dev_property(vbasedev->fd, "interrupts",
+            VFIO_DEV_PROPERTY_TYPE_U32);
+
+    if (irq_prop == NULL) {
+        return -1;
+    }
 
     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] = cpu_to_be32(((__u32 *) irq_prop->data)[3*i]);
         irq_attr[3*i+1] = cpu_to_be32(irq_number);
-        irq_attr[3*i+2] = cpu_to_be32(flags);
+        irq_attr[3*i+2] = cpu_to_be32(((__u32 *) irq_prop->data)[3*i+2]);
     }
 
-   ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
-                     irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t));
+    ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
+            irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t));
 
     g_free(irq_attr);
+    g_free(irq_prop);
 
-   return ret;
+    return ret;
 }
 
 /**
@@ -168,7 +178,7 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
         goto fail;
     }
 
-    ret = set_interrupts_fdt_node(nodename, sbdev, opaque, 0, 0x4);
+    ret = set_interrupts_fdt_node(nodename, sbdev, opaque);
 
     if (ret < 0) {
         error_report("could not set interrupts property of node %s",
@@ -248,7 +258,7 @@ static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque)
         goto fail;
     }
 
-    ret = set_interrupts_fdt_node(nodename, sbdev, opaque, 0, 0x4);
+    ret = set_interrupts_fdt_node(nodename, sbdev, opaque);
 
     if (ret < 0) {
         error_report("could not set interrupts property of node %s",
-- 
2.2.1

Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/sysbus-fdt: vfio device property for interrupts

[Eric Auger]

Hi Baptiste
On 01/12/2015 02:21 PM, Baptiste Reynal wrote:
> Use the VFIO device property API to retrieve interrupt
> information (type and flags) during device node creation.
> 
> Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> ---
>  hw/arm/sysbus-fdt.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index efdeea7..087e788 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -31,6 +31,7 @@
>  #include "hw/vfio/vfio-pl330.h"
>  
>  #include <libfdt.h>
> +#include <linux/vfio.h>
>  
>  /*
>   * internal struct that contains the information to create dynamic
> @@ -66,7 +67,7 @@ typedef struct NodeCreationPair {
>   * Helper function, generate interrupts property for a given node
might be worth adding something like "based on host device tree node"
>   */
>  static int set_interrupts_fdt_node(char *nodename, SysBusDevice *sbdev,
> -        void *opaque, int type, int flags)
> +        void *opaque)
>  {
>      PlatformBusFdtData *data = opaque;
>      PlatformBusDevice *pbus = data->pbus;
> @@ -76,23 +77,32 @@ static int set_interrupts_fdt_node(char *nodename, SysBusDevice *sbdev,
>      int i, ret;
>      VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
>      VFIODevice *vbasedev = &vdev->vbasedev;
> +    struct vfio_dev_property *irq_prop;
> +
> +    irq_prop = vfio_get_dev_property(vbasedev->fd, "interrupts",
> +            VFIO_DEV_PROPERTY_TYPE_U32);
> +
> +    if (irq_prop == NULL) {
> +        return -1;
> +    }
>  
>      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] = cpu_to_be32(((__u32 *) irq_prop->data)[3*i]);
>          irq_attr[3*i+1] = cpu_to_be32(irq_number);
> -        irq_attr[3*i+2] = cpu_to_be32(flags);
> +        irq_attr[3*i+2] = cpu_to_be32(((__u32 *) irq_prop->data)[3*i+2]);
>      }
>  
> -   ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
> -                     irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t));
> +    ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
> +            irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t));
delta may be removed?
>  
>      g_free(irq_attr);
> +    g_free(irq_prop);
>  
> -   return ret;
> +    return ret;
delta may be removed?
Best Regards
Eric
>  }
>  
>  /**
> @@ -168,7 +178,7 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
>          goto fail;
>      }
>  
> -    ret = set_interrupts_fdt_node(nodename, sbdev, opaque, 0, 0x4);
> +    ret = set_interrupts_fdt_node(nodename, sbdev, opaque);
>  
>      if (ret < 0) {
>          error_report("could not set interrupts property of node %s",
> @@ -248,7 +258,7 @@ static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque)
>          goto fail;
>      }
>  
> -    ret = set_interrupts_fdt_node(nodename, sbdev, opaque, 0, 0x4);
> +    ret = set_interrupts_fdt_node(nodename, sbdev, opaque);
>  
>      if (ret < 0) {
>          error_report("could not set interrupts property of node %s",
> 

[Qemu-devel] [RFC PATCH 4/4] hw/arm/sysbus-fdt: arm, pl330 vfio device property

[Baptiste Reynal]

Adapt arm,pl330 function to use the vfio device property API.

Clock apb-pclk is the default if a clock is needed by the device.

Three optional parameters are taken into account :
- #dma-cells
- #dma-channels
- #dma-requests

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

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 087e788..a0adc50 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -96,13 +96,13 @@ static int set_interrupts_fdt_node(char *nodename, SysBusDevice *sbdev,
         irq_attr[3*i+2] = cpu_to_be32(((__u32 *) irq_prop->data)[3*i+2]);
     }
 
-    ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
-            irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t));
+   ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
+                     irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t));
 
     g_free(irq_attr);
     g_free(irq_prop);
 
-    return ret;
+   return ret;
 }
 
 /**
@@ -140,6 +140,54 @@ static int set_regions_fdt_node(char *nodename, SysBusDevice *sbdev,
     return ret;
 }
 
+static int set_arm_primecell_node(char *nodename, SysBusDevice *sbdev, void
+        *opaque)
+{
+    PlatformBusFdtData *data = opaque;
+    void *fdt = data->fdt;
+    int ret;
+    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
+    VFIODevice *vbasedev = &vdev->vbasedev;
+    struct vfio_dev_property *property;
+
+    /* Optional properties */
+    property = vfio_get_dev_property(vbasedev->fd, "interrupts",
+            VFIO_DEV_PROPERTY_TYPE_U32);
+    if (property != NULL) {
+        ret = set_interrupts_fdt_node(nodename, sbdev, opaque);
+        if (ret < 0) {
+            error_report("could not set interrupts property of node %s",
+                    nodename);
+            goto fail;
+        }
+        g_free(property);
+    }
+
+    property = vfio_get_dev_property(vbasedev->fd, "clock-names",
+            VFIO_DEV_PROPERTY_TYPE_STRINGS);
+    if (property != NULL) {
+        /* If a clock is attached, we rely on apb-pclk */
+        /* Check clock existence */
+        ret = fdt_path_offset(fdt, "/apb-pclk");
+
+        if (ret < 0) {
+            error_report("could not set clocks property of node %s", nodename);
+            goto fail;
+        } 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));
+        }
+        g_free(property);
+    }
+
+    return 0;
+
+fail:
+    return -1;
+}
 /**
  * add_calxeda_midway_xgmac_fdt_node
  *
@@ -213,6 +261,8 @@ static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque)
     VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
     VFIODevice *vbasedev = &vdev->vbasedev;
     Object *obj = OBJECT(sbdev);
+    struct vfio_dev_property *property;
+    int propint;
 
     mmio_base = object_property_get_int(obj, "mmio[0]", NULL);
 
@@ -222,6 +272,13 @@ static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque)
 
     qemu_fdt_add_subnode(fdt, nodename);
 
+    ret = set_arm_primecell_node(nodename, sbdev, opaque);
+    if (ret < 0) {
+        error_report("could not set arm,primecell properties of node %s",
+                nodename);
+        goto fail;
+    }
+
     /*
      * Process compatible string to deal with multiple strings
      * (; is replaced by \0)
@@ -237,20 +294,6 @@ static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque)
     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) {
@@ -266,6 +309,49 @@ static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque)
         goto fail;
     }
 
+    /* Optional properties */
+    property = vfio_get_dev_property(vbasedev->fd, "#dma-cells",
+            VFIO_DEV_PROPERTY_TYPE_U32);
+    if (property != NULL) {
+        propint = cpu_to_be32(((__u32 *) property->data)[0]);
+        ret = qemu_fdt_setprop(fdt, nodename, "#dma-cells", &propint,
+                sizeof(int));
+        if (ret < 0) {
+            error_report("could not set #dma-cells property of node %s",
+                    nodename);
+            goto fail;
+        }
+        g_free(property);
+    }
+
+    property = vfio_get_dev_property(vbasedev->fd, "#dma-channels",
+            VFIO_DEV_PROPERTY_TYPE_U32);
+    if (property != NULL) {
+        propint = cpu_to_be32(((__u32 *) property->data)[0]);
+        ret = qemu_fdt_setprop(fdt, nodename, "#dma-cells", &propint,
+                sizeof(int));
+        if (ret < 0) {
+            error_report("could not set #dma-cells property of node %s",
+                    nodename);
+            goto fail;
+        }
+        g_free(property);
+    }
+
+    property = vfio_get_dev_property(vbasedev->fd, "#dma-requests",
+            VFIO_DEV_PROPERTY_TYPE_U32);
+    if (property != NULL) {
+        propint = cpu_to_be32(((__u32 *) property->data)[0]);
+        ret = qemu_fdt_setprop(fdt, nodename, "#dma-cells", &propint,
+                sizeof(int));
+        if (ret < 0) {
+            error_report("could not set #dma-cells property of node %s",
+                    nodename);
+            goto fail;
+        }
+        g_free(property);
+    }
+
     g_free(nodename);
 
     return 0;
-- 
2.2.1

Re: [Qemu-devel] [RFC PATCH 4/4] hw/arm/sysbus-fdt: arm, pl330 vfio device property

[Eric Auger]

Hi Baptiste,
On 01/12/2015 02:21 PM, Baptiste Reynal wrote:
> Adapt arm,pl330 function to use the vfio device property API.
> 
> Clock apb-pclk is the default if a clock is needed by the device.
> 
> Three optional parameters are taken into account :
> - #dma-cells
> - #dma-channels
> - #dma-requests
> 
> Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> ---
>  hw/arm/sysbus-fdt.c | 120 ++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 103 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 087e788..a0adc50 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -96,13 +96,13 @@ static int set_interrupts_fdt_node(char *nodename, SysBusDevice *sbdev,
>          irq_attr[3*i+2] = cpu_to_be32(((__u32 *) irq_prop->data)[3*i+2]);
>      }
>  
> -    ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
> -            irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t));
> +   ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
> +                     irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t));
delta may be removed
>  
>      g_free(irq_attr);
>      g_free(irq_prop);
>  
> -    return ret;
> +   return ret;
delta may be removed
>  }
>  
>  /**
> @@ -140,6 +140,54 @@ static int set_regions_fdt_node(char *nodename, SysBusDevice *sbdev,
>      return ret;
>  }
>  
may deserve a comment and/or pointer to
Documentation/devicetree/bindings/arm/primecell.txt; don't know whether
this is the reference doc, sorry.
> +static int set_arm_primecell_node(char *nodename, SysBusDevice *sbdev, void
> +        *opaque)
> +{
> +    PlatformBusFdtData *data = opaque;
> +    void *fdt = data->fdt;
> +    int ret;
> +    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
> +    VFIODevice *vbasedev = &vdev->vbasedev;
> +    struct vfio_dev_property *property;
> +
Wouldn't it make sense to handle the required compatible prop here?
> +    /* Optional properties */
> +    property = vfio_get_dev_property(vbasedev->fd, "interrupts",
> +            VFIO_DEV_PROPERTY_TYPE_U32);
> +    if (property != NULL) {
> +        ret = set_interrupts_fdt_node(nodename, sbdev, opaque);
> +        if (ret < 0) {
> +            error_report("could not set interrupts property of node %s",
> +                    nodename);
> +            goto fail;
in case of fail the property is not released.
> +        }
> +        g_free(property);
> +    }
> +
> +    property = vfio_get_dev_property(vbasedev->fd, "clock-names",
> +            VFIO_DEV_PROPERTY_TYPE_STRINGS);
> +    if (property != NULL) {
> +        /* If a clock is attached, we rely on apb-pclk */
> +        /* Check clock existence */
> +        ret = fdt_path_offset(fdt, "/apb-pclk");
> +
> +        if (ret < 0) {
> +            error_report("could not set clocks property of node %s", nodename);
> +            goto fail;
release property
> +        } 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));
> +        }
> +        g_free(property);
do you intend to support other optional properties?
> +    }
> +
> +    return 0;
> +
> +fail:
> +    return -1;
> +}
>  /**
>   * add_calxeda_midway_xgmac_fdt_node
>   *
> @@ -213,6 +261,8 @@ static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque)
>      VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
>      VFIODevice *vbasedev = &vdev->vbasedev;
>      Object *obj = OBJECT(sbdev);
> +    struct vfio_dev_property *property;
> +    int propint;
>  
>      mmio_base = object_property_get_int(obj, "mmio[0]", NULL);
>  
> @@ -222,6 +272,13 @@ static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque)
>  
>      qemu_fdt_add_subnode(fdt, nodename);
>  
> +    ret = set_arm_primecell_node(nodename, sbdev, opaque);
> +    if (ret < 0) {
> +        error_report("could not set arm,primecell properties of node %s",
> +                nodename);
> +        goto fail;
> +    }
> +
>      /*
>       * Process compatible string to deal with multiple strings
>       * (; is replaced by \0)
> @@ -237,20 +294,6 @@ static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque)
>      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) {
> @@ -266,6 +309,49 @@ static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque)
>          goto fail;
>      }
>  
> +    /* Optional properties */
> +    property = vfio_get_dev_property(vbasedev->fd, "#dma-cells",
> +            VFIO_DEV_PROPERTY_TYPE_U32);
> +    if (property != NULL) {
> +        propint = cpu_to_be32(((__u32 *) property->data)[0]);
> +        ret = qemu_fdt_setprop(fdt, nodename, "#dma-cells", &propint,
> +                sizeof(int));
> +        if (ret < 0) {
> +            error_report("could not set #dma-cells property of node %s",
> +                    nodename);
> +            goto fail;
property release
> +        }
> +        g_free(property);
> +    }
> +
> +    property = vfio_get_dev_property(vbasedev->fd, "#dma-channels",
> +            VFIO_DEV_PROPERTY_TYPE_U32);
> +    if (property != NULL) {
> +        propint = cpu_to_be32(((__u32 *) property->data)[0]);
> +        ret = qemu_fdt_setprop(fdt, nodename, "#dma-cells", &propint,
> +                sizeof(int));
> +        if (ret < 0) {
> +            error_report("could not set #dma-cells property of node %s",
> +                    nodename);
> +            goto fail;
> +        }
> +        g_free(property);
> +    }
> +
> +    property = vfio_get_dev_property(vbasedev->fd, "#dma-requests",
> +            VFIO_DEV_PROPERTY_TYPE_U32);
> +    if (property != NULL) {
> +        propint = cpu_to_be32(((__u32 *) property->data)[0]);
> +        ret = qemu_fdt_setprop(fdt, nodename, "#dma-cells", &propint,
> +                sizeof(int));
> +        if (ret < 0) {
> +            error_report("could not set #dma-cells property of node %s",
> +                    nodename);
> +            goto fail;
> +        }
> +        g_free(property);
> +    }
> +
>      g_free(nodename);
>  
>      return 0;
to me it looks a good and interesting illustration of the modality

Best Regards

Eric
>