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

[Qemu-devel] [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.

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.

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

 hw/arm/sysbus-fdt.c           | 34 +++++++++++++++++++++++++++++-----
 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, 110 insertions(+), 8 deletions(-)
 create mode 100644 hw/vfio/pl330.c
 create mode 100644 include/hw/vfio/vfio-pl330.h

-- 
2.1.3

[Qemu-devel] [RFC PATCH 1/3] hw/vfio/sysbus-fdt: generic add_generic_fdt_node

[Baptiste Reynal]

Modify add_calxeda_midway_xgmac_fdt_node to make it more generic.
Add multiple compatible strings support.

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

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 15bb50c..125ba37 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -58,12 +58,12 @@ typedef struct NodeCreationPair {
 /* Device Specific Code */
 
 /**
- * add_calxeda_midway_xgmac_fdt_node
+ * add_device_fdt_node
  *
  * Generates a very simple node with following properties:
  * compatible string, regs, interrupts
  */
-static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
+static int add_generic_fdt_node(SysBusDevice *sbdev, void *opaque)
 {
     PlatformBusFdtData *data = opaque;
     PlatformBusDevice *pbus = data->pbus;
@@ -80,6 +80,18 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
     VFIODevice *vbasedev = &vdev->vbasedev;
     Object *obj = OBJECT(sbdev);
 
+    /*
+     * 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';
+    }
+
     mmio_base = object_property_get_int(obj, "mmio[0]", NULL);
 
     nodename = g_strdup_printf("%s/%s@%" PRIx64, parent_node,
@@ -88,9 +100,8 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
 
     qemu_fdt_add_subnode(fdt, nodename);
 
-    compat_str_len = strlen(vdev->compat) + 1;
     qemu_fdt_setprop(fdt, nodename, "compatible",
-                          vdev->compat, compat_str_len);
+                          compat, compat_str_len);
 
     reg_attr = g_new(uint64_t, vbasedev->num_regions*4);
 
@@ -140,7 +151,7 @@ fail:
 
 /* 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_CALXEDA_XGMAC, add_generic_fdt_node},
 {"", NULL}, /*last element*/
 };
 
-- 
2.1.3

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

[Baptiste Reynal]

Add VFIO_DEVICE_TYPE_AMBA.
Differentiate amba and platform devices according to compatible string.
If the device is amba, clocks description is added in the fd.

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

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 125ba37..41b4c87 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -62,6 +62,8 @@ typedef struct NodeCreationPair {
  *
  * Generates a very simple node with following properties:
  * compatible string, regs, interrupts
+ * And the following properties if it is on an AMBA bus :
+ * clocks, clock-names
  */
 static int add_generic_fdt_node(SysBusDevice *sbdev, void *opaque)
 {
@@ -103,6 +105,15 @@ static int add_generic_fdt_node(SysBusDevice *sbdev, void *opaque)
     qemu_fdt_setprop(fdt, nodename, "compatible",
                           compat, compat_str_len);
 
+    if (vbasedev->type == VFIO_DEVICE_TYPE_AMBA) {
+        /* Setup clocks for AMBA device */
+        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));
+    }
+
     reg_attr = g_new(uint64_t, vbasedev->num_regions*4);
 
     for (i = 0; i < vbasedev->num_regions; i++) {
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.1.3

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

[Baptiste Reynal]

Create a meta-device for PL330 DMA.

Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 hw/arm/sysbus-fdt.c          |  2 ++
 hw/vfio/Makefile.objs        |  1 +
 hw/vfio/pl330.c              | 41 +++++++++++++++++++++++++++++++++++++++++
 include/hw/vfio/vfio-pl330.h | 26 ++++++++++++++++++++++++++
 4 files changed, 70 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 41b4c87..1141185 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -28,6 +28,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/vfio/vfio-platform.h"
 #include "hw/vfio/vfio-calxeda-xgmac.h"
+#include "hw/vfio/vfio-pl330.h"
 
 /*
  * internal struct that contains the information to create dynamic
@@ -163,6 +164,7 @@ fail:
 /* list of supported dynamic sysbus devices */
 static const NodeCreationPair add_fdt_node_functions[] = {
 {TYPE_VFIO_CALXEDA_XGMAC, add_generic_fdt_node},
+{TYPE_VFIO_PL330, add_generic_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.1.3

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

[Eric Auger]

On 12/17/2014 11:09 AM, Baptiste Reynal wrote:
> 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.
> 
> The series has been tested using PL330 device, irq forwarding doesn't work.
> (The test hangs on the first IRQ).

Hi Baptiste,

If am not wrong your branch below does not contain the requested kernel
patches for both irqfd and forwarding. Please take the requested pieces
on my linux git:
last official one can be taken at
http://git.linaro.org/people/eric.auger/linux.git (branch 3.18-rc6-v10)
All the requested kernel dependencies are quoted in
http://lists.gnu.org/archive/html/qemu-devel/2014-11/msg04372.html. That
should help ;-)

Best Regards

Eric


> 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.
> 
> Baptiste Reynal (3):
>   hw/vfio/sysbus-fdt: generic add_generic_fdt_node
>   hw/vfio: amba device support
>   hw/vfio: add pl330 device support
> 
>  hw/arm/sysbus-fdt.c           | 34 +++++++++++++++++++++++++++++-----
>  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, 110 insertions(+), 8 deletions(-)
>  create mode 100644 hw/vfio/pl330.c
>  create mode 100644 include/hw/vfio/vfio-pl330.h
> 

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

[Eric Auger]

On 12/17/2014 11:09 AM, Baptiste Reynal wrote:
> Modify add_calxeda_midway_xgmac_fdt_node to make it more generic.
> Add multiple compatible strings support.
Hi Baptiste,

Although I understand you may be tempted to transform the Calxeda basic
node creation function into something more generic, we came to the
current result after long discussions with Alex Graf. This generic
function was something released in the past but transformed into
something specific at the end.

Typically the fact the IRQ is edge sensitive or level sensitive may
depend on the device and it would be arbitrary here to choose either in
a "generic" node creation.

So the current trend for new devices is to add another entry in the
add_fdt_node_functions array; genericity target was argued in the past
and rejected.

If you want to do something going in the direction of genericity I would
advise you to investigate using Antonios API ([RFC 0/4] VFIO: PLATFORM:
Return device tree info for a platform device node). This could
typically enable to get the egde/level sensitive characteritics.

> 
> Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> ---
>  hw/arm/sysbus-fdt.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 15bb50c..125ba37 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -58,12 +58,12 @@ typedef struct NodeCreationPair {
>  /* Device Specific Code */
>  
>  /**
> - * add_calxeda_midway_xgmac_fdt_node
> + * add_device_fdt_node
>   *
>   * Generates a very simple node with following properties:
>   * compatible string, regs, interrupts
>   */
> -static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
> +static int add_generic_fdt_node(SysBusDevice *sbdev, void *opaque)
>  {
>      PlatformBusFdtData *data = opaque;
>      PlatformBusDevice *pbus = data->pbus;
> @@ -80,6 +80,18 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
>      VFIODevice *vbasedev = &vdev->vbasedev;
>      Object *obj = OBJECT(sbdev);
>  
> +    /*
> +     * 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';
> +    }
why can't you format the string directly in the corresponding VFIO AMBA
device? Formerly we had this problem of conversion since we passed the
format string in qemu command line.

Best Regards

Eric
> +
>      mmio_base = object_property_get_int(obj, "mmio[0]", NULL);
>  
>      nodename = g_strdup_printf("%s/%s@%" PRIx64, parent_node,
> @@ -88,9 +100,8 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
>  
>      qemu_fdt_add_subnode(fdt, nodename);
>  
> -    compat_str_len = strlen(vdev->compat) + 1;
>      qemu_fdt_setprop(fdt, nodename, "compatible",
> -                          vdev->compat, compat_str_len);
> +                          compat, compat_str_len);
>  
>      reg_attr = g_new(uint64_t, vbasedev->num_regions*4);
>  
> @@ -140,7 +151,7 @@ fail:
>  
>  /* 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_CALXEDA_XGMAC, add_generic_fdt_node},
>  {"", NULL}, /*last element*/
>  };
>  
> 

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

[Eric Auger]

On 12/17/2014 11:09 AM, Baptiste Reynal wrote:
> Add VFIO_DEVICE_TYPE_AMBA.
> Differentiate amba and platform devices according to compatible string.
> If the device is amba, clocks description is added in the fd.
> 
> Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> ---
>  hw/arm/sysbus-fdt.c           | 11 +++++++++++
>  hw/vfio/platform.c            | 15 ++++++++++++---
>  include/hw/vfio/vfio-common.h |  1 +
>  3 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 125ba37..41b4c87 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -62,6 +62,8 @@ typedef struct NodeCreationPair {
>   *
>   * Generates a very simple node with following properties:
>   * compatible string, regs, interrupts
> + * And the following properties if it is on an AMBA bus :
> + * clocks, clock-names
>   */
>  static int add_generic_fdt_node(SysBusDevice *sbdev, void *opaque)
>  {
> @@ -103,6 +105,15 @@ static int add_generic_fdt_node(SysBusDevice *sbdev, void *opaque)
>      qemu_fdt_setprop(fdt, nodename, "compatible",
>                            compat, compat_str_len);
>  
> +    if (vbasedev->type == VFIO_DEVICE_TYPE_AMBA) {
> +        /* Setup clocks for AMBA device */
> +        qemu_fdt_setprop_cells(fdt, nodename, "clocks",
> +                qemu_fdt_getprop_cell(fdt, "/apb-pclk", "phandle"));
Shouldn't you test if this clock node exists in the guest device tree?
> +        char clock_names[] = "apb_pclk";
> +        qemu_fdt_setprop(fdt, nodename, "clock-names", clock_names,
> +                sizeof(clock_names));
> +    }
> +
>      reg_attr = g_new(uint64_t, vbasedev->num_regions*4);
>  
>      for (i = 0; i < vbasedev->num_regions; i++) {
> 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;
The plan is to use VFIO_DEVICE_GET_INFO instead. Alex recently requested
we test the flags. We need a header sync for that.

Best Regards

Eric
>  
>  #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 {
> 

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

[Baptiste Reynal]

[-- Attachment #1: Type: text/plain, Size: 4261 bytes --]

On Wed, Dec 17, 2014 at 2:32 PM, Eric Auger <eric.auger@linaro.org> wrote:
>
> On 12/17/2014 11:09 AM, Baptiste Reynal wrote:
> > Modify add_calxeda_midway_xgmac_fdt_node to make it more generic.
> > Add multiple compatible strings support.
> Hi Baptiste,
>
> Although I understand you may be tempted to transform the Calxeda basic
> node creation function into something more generic, we came to the
> current result after long discussions with Alex Graf. This generic
> function was something released in the past but transformed into
> something specific at the end.
>
> Typically the fact the IRQ is edge sensitive or level sensitive may
> depend on the device and it would be arbitrary here to choose either in
> a "generic" node creation.
>
> So the current trend for new devices is to add another entry in the
> add_fdt_node_functions array; genericity target was argued in the past
> and rejected.
>
> If you want to do something going in the direction of genericity I would
> advise you to investigate using Antonios API ([RFC 0/4] VFIO: PLATFORM:
> Return device tree info for a platform device node). This could
> typically enable to get the egde/level sensitive characteritics.
>

I'm not sure I totally understand your point, but creating a new
add_pl330_fdt_node function which will call to
add_calxeda_midway_xgmac_fdt_node and perform amba specific process
(multiple compatible string and clocks) seems to be a good solution for you
?

>
> >
> > Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> > ---
> >  hw/arm/sysbus-fdt.c | 21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> > index 15bb50c..125ba37 100644
> > --- a/hw/arm/sysbus-fdt.c
> > +++ b/hw/arm/sysbus-fdt.c
> > @@ -58,12 +58,12 @@ typedef struct NodeCreationPair {
> >  /* Device Specific Code */
> >
> >  /**
> > - * add_calxeda_midway_xgmac_fdt_node
> > + * add_device_fdt_node
> >   *
> >   * Generates a very simple node with following properties:
> >   * compatible string, regs, interrupts
> >   */
> > -static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void
> *opaque)
> > +static int add_generic_fdt_node(SysBusDevice *sbdev, void *opaque)
> >  {
> >      PlatformBusFdtData *data = opaque;
> >      PlatformBusDevice *pbus = data->pbus;
> > @@ -80,6 +80,18 @@ static int
> add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
> >      VFIODevice *vbasedev = &vdev->vbasedev;
> >      Object *obj = OBJECT(sbdev);
> >
> > +    /*
> > +     * 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';
> > +    }
> why can't you format the string directly in the corresponding VFIO AMBA
> device? Formerly we had this problem of conversion since we passed the
> format string in qemu command line.
>
> Best Regards
>
> Eric
>

>From my point of view, formatting the string directly in VFIO AMBA device
will mess a lot with string function, but I can use an array of string in
VFIOPlatformDevice to avoid it.

Best Regards,

Baptiste

> +
> >      mmio_base = object_property_get_int(obj, "mmio[0]", NULL);
> >
> >      nodename = g_strdup_printf("%s/%s@%" PRIx64, parent_node,
> > @@ -88,9 +100,8 @@ static int
> add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
> >
> >      qemu_fdt_add_subnode(fdt, nodename);
> >
> > -    compat_str_len = strlen(vdev->compat) + 1;
> >      qemu_fdt_setprop(fdt, nodename, "compatible",
> > -                          vdev->compat, compat_str_len);
> > +                          compat, compat_str_len);
> >
> >      reg_attr = g_new(uint64_t, vbasedev->num_regions*4);
> >
> > @@ -140,7 +151,7 @@ fail:
> >
> >  /* 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_CALXEDA_XGMAC, add_generic_fdt_node},
> >  {"", NULL}, /*last element*/
> >  };
> >
> >
>
>

[-- Attachment #2: Type: text/html, Size: 5836 bytes --]

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

[Eric Auger]

On 12/17/2014 03:55 PM, Baptiste Reynal wrote:
> 
> 
> On Wed, Dec 17, 2014 at 2:32 PM, Eric Auger <eric.auger@linaro.org
> <mailto:eric.auger@linaro.org>> wrote:
> 
>     On 12/17/2014 11:09 AM, Baptiste Reynal wrote:
>     > Modify add_calxeda_midway_xgmac_fdt_node to make it more generic.
>     > Add multiple compatible strings support.
>     Hi Baptiste,
> 
>     Although I understand you may be tempted to transform the Calxeda basic
>     node creation function into something more generic, we came to the
>     current result after long discussions with Alex Graf. This generic
>     function was something released in the past but transformed into
>     something specific at the end.
> 
>     Typically the fact the IRQ is edge sensitive or level sensitive may
>     depend on the device and it would be arbitrary here to choose either in
>     a "generic" node creation.
> 
>     So the current trend for new devices is to add another entry in the
>     add_fdt_node_functions array; genericity target was argued in the past
>     and rejected.
> 
>     If you want to do something going in the direction of genericity I would
>     advise you to investigate using Antonios API ([RFC 0/4] VFIO: PLATFORM:
>     Return device tree info for a platform device node). This could
>     typically enable to get the egde/level sensitive characteritics.
> 
> 
> I'm not sure I totally understand your point, but creating a new 
> add_pl330_fdt_node function which will call to
> add_calxeda_midway_xgmac_fdt_node and perform amba specific process
> (multiple compatible string and clocks) seems to be a good solution for
> you ?
> 
No I had in mind the amba node creation function should duplicate the
code, have its own compat string manipulation if needed, add its own
clock properties ...

Maybe we could devise some helper function for building
the reg property and interrupt property?

BR

Eric
> 
>     >
>     > Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com
>     <mailto:b.reynal@virtualopensystems.com>>
>     > ---
>     >  hw/arm/sysbus-fdt.c | 21 ++++++++++++++++-----
>     >  1 file changed, 16 insertions(+), 5 deletions(-)
>     >
>     > diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>     > index 15bb50c..125ba37 100644
>     > --- a/hw/arm/sysbus-fdt.c
>     > +++ b/hw/arm/sysbus-fdt.c
>     > @@ -58,12 +58,12 @@ typedef struct NodeCreationPair {
>     >  /* Device Specific Code */
>     >
>     >  /**
>     > - * add_calxeda_midway_xgmac_fdt_node
>     > + * add_device_fdt_node
>     >   *
>     >   * Generates a very simple node with following properties:
>     >   * compatible string, regs, interrupts
>     >   */
>     > -static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev,
>     void *opaque)
>     > +static int add_generic_fdt_node(SysBusDevice *sbdev, void *opaque)
>     >  {
>     >      PlatformBusFdtData *data = opaque;
>     >      PlatformBusDevice *pbus = data->pbus;
>     > @@ -80,6 +80,18 @@ static int
>     add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
>     >      VFIODevice *vbasedev = &vdev->vbasedev;
>     >      Object *obj = OBJECT(sbdev);
>     >
>     > +    /*
>     > +     * 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';
>     > +    }
>     why can't you format the string directly in the corresponding VFIO AMBA
>     device? Formerly we had this problem of conversion since we passed the
>     format string in qemu command line.
> 
>     Best Regards
> 
>     Eric
> 
> 
> From my point of view, formatting the string directly in VFIO AMBA
> device will mess a lot with string function, but I can use an array of
> string in VFIOPlatformDevice to avoid it.
> 
> Best Regards,
> 
> Baptiste
> 
>     > +
>     >      mmio_base = object_property_get_int(obj, "mmio[0]", NULL);
>     >
>     >      nodename = g_strdup_printf("%s/%s@%" PRIx64, parent_node,
>     > @@ -88,9 +100,8 @@ static int
>     add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
>     >
>     >      qemu_fdt_add_subnode(fdt, nodename);
>     >
>     > -    compat_str_len = strlen(vdev->compat) + 1;
>     >      qemu_fdt_setprop(fdt, nodename, "compatible",
>     > -                          vdev->compat, compat_str_len);
>     > +                          compat, compat_str_len);
>     >
>     >      reg_attr = g_new(uint64_t, vbasedev->num_regions*4);
>     >
>     > @@ -140,7 +151,7 @@ fail:
>     >
>     >  /* 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_CALXEDA_XGMAC, add_generic_fdt_node},
>     >  {"", NULL}, /*last element*/
>     >  };
>     >
>     >
>