diff mbox

[v5,10/10] hw/arm/dyn_sysbus_devtree: enable simple VFIO dynamic instantiation

Message ID 1407594349-9291-11-git-send-email-eric.auger@linaro.org
State New
Headers show

Commit Message

Auger Eric Aug. 9, 2014, 2:25 p.m. UTC 
Generates the device node of VFIO devices, if any are invoked in
-device option. In case VFIO devices require more complex node
generation, they can be handled before.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 hw/arm/dyn_sysbus_devtree.c | 138 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 138 insertions(+)

Comments

Alexander Graf Aug. 11, 2014, 9:40 a.m. UTC | #1 
On 09.08.14 16:25, Eric Auger wrote:
> Generates the device node of VFIO devices, if any are invoked in
> -device option. In case VFIO devices require more complex node
> generation, they can be handled before.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> ---
>   hw/arm/dyn_sysbus_devtree.c | 138 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 138 insertions(+)
>
> diff --git a/hw/arm/dyn_sysbus_devtree.c b/hw/arm/dyn_sysbus_devtree.c
> index 56af62f..ac34f07 100644
> --- a/hw/arm/dyn_sysbus_devtree.c
> +++ b/hw/arm/dyn_sysbus_devtree.c
> @@ -1,6 +1,139 @@
>   #include "hw/arm/dyn_sysbus_devtree.h"
>   #include "qemu/error-report.h"
>   #include "sysemu/device_tree.h"
> +#include "hw/vfio/vfio-platform.h"
> +
> +static void vfio_fdt_add_device_node(SysBusDevice *sbdev, void *opaque);
> +
> +static char *format_compat(char * compat)
> +{
> +    char *str_ptr, *corrected_compat;
> +    /*
> +     * process compatibility property string passed by end-user
> +     * replaces / by , and ; by NUL character
> +     */
> +    corrected_compat = g_strdup(compat);
> +    /*
> +     * the total length of the string has to include also the last
> +     * NUL char.
> +     */
> +
> +    str_ptr = corrected_compat;
> +    while ((str_ptr = strchr(str_ptr, '/')) != NULL) {
> +        *str_ptr = ',';
> +    }
> +
> +    /* substitute ";" with the NUL char */
> +    str_ptr = corrected_compat;
> +    while ((str_ptr = strchr(str_ptr, ';')) != NULL) {
> +        *str_ptr = '\0';
> +    }
> +
> +    return corrected_compat;
> +}
> +
> +static void wrap_vfio_fdt_add_node(SysBusDevice *sbdev, void *opaque)
> +{
> +    PlatformDevtreeData *data = opaque;
> +    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
> +    VFIODevice *vbasedev = &vdev->vbasedev;
> +    gchar irq_number_prop[8];
> +    Object *obj = OBJECT(sbdev);
> +    char *corrected_compat;
> +    uint64_t irq_number;
> +    int compat_str_len = strlen(vdev->compat)+1;
> +    int i;
> +
> +    corrected_compat = format_compat(vdev->compat);
> +    snprintf(vdev->compat, compat_str_len, "%s", corrected_compat);
> +    g_free(corrected_compat);
> +
> +    vfio_fdt_add_device_node(sbdev, opaque);
> +
> +    for (i = 0; i < vbasedev->num_irqs; i++) {
> +        snprintf(irq_number_prop, sizeof(irq_number_prop), "irq[%d]", i);
> +        irq_number = object_property_get_int(obj, irq_number_prop, NULL)
> +                                                 + data->irq_start;
> +        /*
> +         * for setting irqfd up we must provide the virtual IRQ number
> +         * which is the sum of irq_start and actual platform bus irq
> +         * index. At realize point we do not have this info.
> +         */
> +        if (vdev->irqfd_allowed) {
> +            vfio_setup_irqfd(sbdev, i, irq_number);
> +        }
> +    }
> +}
> +
> +static void vfio_fdt_add_device_node(SysBusDevice *sbdev, void *opaque)
> +{
> +    PlatformDevtreeData *data = opaque;
> +    void *fdt = data->fdt;
> +    const char *parent_node = data->node;
> +    int compat_str_len;
> +    char *nodename;
> +    int i, ret;
> +    uint32_t *irq_attr;
> +    uint64_t *reg_attr;
> +    uint64_t mmio_base;
> +    uint64_t irq_number;
> +    gchar mmio_base_prop[8];
> +    gchar irq_number_prop[8];
> +    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);
> +
> +    compat_str_len = strlen(vdev->compat) + 1;
> +    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++) {
> +        snprintf(mmio_base_prop, sizeof(mmio_base_prop), "mmio[%d]", i);
> +        mmio_base = object_property_get_int(obj, mmio_base_prop, NULL);
> +        reg_attr[2*i] = 1;
> +        reg_attr[2*i+1] = mmio_base;
> +        reg_attr[2*i+2] = 1;
> +        reg_attr[2*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);
> +    if (ret < 0) {
> +        error_report("could not set reg property of node %s", nodename);
> +    }
> +
> +    irq_attr = g_new(uint32_t, vbasedev->num_irqs*3);
> +
> +    for (i = 0; i < vbasedev->num_irqs; i++) {
> +        snprintf(irq_number_prop, sizeof(irq_number_prop), "irq[%d]", i);
> +        irq_number = object_property_get_int(obj, irq_number_prop, NULL)
> +                                                 + 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);
> +    }
> +
> +    g_free(nodename);
> +    g_free(irq_attr);
> +    g_free(reg_attr);
> +}
>   
>   int sysbus_device_create_devtree(Object *obj, void *opaque)
>   {
> @@ -17,6 +150,11 @@ int sysbus_device_create_devtree(Object *obj, void *opaque)
>           return object_child_foreach(obj, sysbus_device_create_devtree, data);
>       }
>   
> +    if (object_dynamic_cast(obj, TYPE_VFIO_PLATFORM)) {

You should only ever check for specific VFIO device types. A generic 
"vfio-platform" device type will not work, since you won't have enough 
auxiliary information once devices become more complicated.


Alex
Auger Eric Aug. 11, 2014, 11:55 a.m. UTC | #2 
On 08/11/2014 11:40 AM, Alexander Graf wrote:
> 
> On 09.08.14 16:25, Eric Auger wrote:
>> Generates the device node of VFIO devices, if any are invoked in
>> -device option. In case VFIO devices require more complex node
>> generation, they can be handled before.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>> ---
>>   hw/arm/dyn_sysbus_devtree.c | 138
>> ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 138 insertions(+)
>>
>> diff --git a/hw/arm/dyn_sysbus_devtree.c b/hw/arm/dyn_sysbus_devtree.c
>> index 56af62f..ac34f07 100644
>> --- a/hw/arm/dyn_sysbus_devtree.c
>> +++ b/hw/arm/dyn_sysbus_devtree.c
>> @@ -1,6 +1,139 @@
>>   #include "hw/arm/dyn_sysbus_devtree.h"
>>   #include "qemu/error-report.h"
>>   #include "sysemu/device_tree.h"
>> +#include "hw/vfio/vfio-platform.h"
>> +
>> +static void vfio_fdt_add_device_node(SysBusDevice *sbdev, void *opaque);
>> +
>> +static char *format_compat(char * compat)
>> +{
>> +    char *str_ptr, *corrected_compat;
>> +    /*
>> +     * process compatibility property string passed by end-user
>> +     * replaces / by , and ; by NUL character
>> +     */
>> +    corrected_compat = g_strdup(compat);
>> +    /*
>> +     * the total length of the string has to include also the last
>> +     * NUL char.
>> +     */
>> +
>> +    str_ptr = corrected_compat;
>> +    while ((str_ptr = strchr(str_ptr, '/')) != NULL) {
>> +        *str_ptr = ',';
>> +    }
>> +
>> +    /* substitute ";" with the NUL char */
>> +    str_ptr = corrected_compat;
>> +    while ((str_ptr = strchr(str_ptr, ';')) != NULL) {
>> +        *str_ptr = '\0';
>> +    }
>> +
>> +    return corrected_compat;
>> +}
>> +
>> +static void wrap_vfio_fdt_add_node(SysBusDevice *sbdev, void *opaque)
>> +{
>> +    PlatformDevtreeData *data = opaque;
>> +    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
>> +    VFIODevice *vbasedev = &vdev->vbasedev;
>> +    gchar irq_number_prop[8];
>> +    Object *obj = OBJECT(sbdev);
>> +    char *corrected_compat;
>> +    uint64_t irq_number;
>> +    int compat_str_len = strlen(vdev->compat)+1;
>> +    int i;
>> +
>> +    corrected_compat = format_compat(vdev->compat);
>> +    snprintf(vdev->compat, compat_str_len, "%s", corrected_compat);
>> +    g_free(corrected_compat);
>> +
>> +    vfio_fdt_add_device_node(sbdev, opaque);
>> +
>> +    for (i = 0; i < vbasedev->num_irqs; i++) {
>> +        snprintf(irq_number_prop, sizeof(irq_number_prop), "irq[%d]",
>> i);
>> +        irq_number = object_property_get_int(obj, irq_number_prop, NULL)
>> +                                                 + data->irq_start;
>> +        /*
>> +         * for setting irqfd up we must provide the virtual IRQ number
>> +         * which is the sum of irq_start and actual platform bus irq
>> +         * index. At realize point we do not have this info.
>> +         */
>> +        if (vdev->irqfd_allowed) {
>> +            vfio_setup_irqfd(sbdev, i, irq_number);
>> +        }
>> +    }
>> +}
>> +
>> +static void vfio_fdt_add_device_node(SysBusDevice *sbdev, void *opaque)
>> +{
>> +    PlatformDevtreeData *data = opaque;
>> +    void *fdt = data->fdt;
>> +    const char *parent_node = data->node;
>> +    int compat_str_len;
>> +    char *nodename;
>> +    int i, ret;
>> +    uint32_t *irq_attr;
>> +    uint64_t *reg_attr;
>> +    uint64_t mmio_base;
>> +    uint64_t irq_number;
>> +    gchar mmio_base_prop[8];
>> +    gchar irq_number_prop[8];
>> +    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);
>> +
>> +    compat_str_len = strlen(vdev->compat) + 1;
>> +    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++) {
>> +        snprintf(mmio_base_prop, sizeof(mmio_base_prop), "mmio[%d]", i);
>> +        mmio_base = object_property_get_int(obj, mmio_base_prop, NULL);
>> +        reg_attr[2*i] = 1;
>> +        reg_attr[2*i+1] = mmio_base;
>> +        reg_attr[2*i+2] = 1;
>> +        reg_attr[2*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);
>> +    if (ret < 0) {
>> +        error_report("could not set reg property of node %s", nodename);
>> +    }
>> +
>> +    irq_attr = g_new(uint32_t, vbasedev->num_irqs*3);
>> +
>> +    for (i = 0; i < vbasedev->num_irqs; i++) {
>> +        snprintf(irq_number_prop, sizeof(irq_number_prop), "irq[%d]",
>> i);
>> +        irq_number = object_property_get_int(obj, irq_number_prop, NULL)
>> +                                                 + 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);
>> +    }
>> +
>> +    g_free(nodename);
>> +    g_free(irq_attr);
>> +    g_free(reg_attr);
>> +}
>>     int sysbus_device_create_devtree(Object *obj, void *opaque)
>>   {
>> @@ -17,6 +150,11 @@ int sysbus_device_create_devtree(Object *obj, void
>> *opaque)
>>           return object_child_foreach(obj,
>> sysbus_device_create_devtree, data);
>>       }
>>   +    if (object_dynamic_cast(obj, TYPE_VFIO_PLATFORM)) {
> 
> You should only ever check for specific VFIO device types. A generic
> "vfio-platform" device type will not work, since you won't have enough
> auxiliary information once devices become more complicated.

Hi Alex,

I will re-submit this week with calxeda-xgmac derived device back again
and abstract class too.

Thanks

Eric
> 
> 
> Alex
>
Joel Schopp Aug. 18, 2014, 9:54 p.m. UTC | #3 
+static void vfio_fdt_add_device_node(SysBusDevice *sbdev, void *opaque)
+{
+    PlatformDevtreeData *data = opaque;
+    void *fdt = data->fdt;
+    const char *parent_node = data->node;
+    int compat_str_len;
+    char *nodename;
+    int i, ret;
+    uint32_t *irq_attr;
+    uint64_t *reg_attr;
+    uint64_t mmio_base;
+    uint64_t irq_number;
+    gchar mmio_base_prop[8];
+    gchar irq_number_prop[8];
+    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);
+
+    compat_str_len = strlen(vdev->compat) + 1;
+    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++) {
+        snprintf(mmio_base_prop, sizeof(mmio_base_prop), "mmio[%d]", i);
+        mmio_base = object_property_get_int(obj, mmio_base_prop, NULL);
+        reg_attr[2*i] = 1;
+        reg_attr[2*i+1] = mmio_base;
+        reg_attr[2*i+2] = 1;
+        reg_attr[2*i+3] = memory_region_size(&vdev->regions[i]->mem);
+    }

This should be 4 instead of 2. 
Also, to support 64 bit systems I think this should be 2 instead of 1.
Peter Maydell Aug. 18, 2014, 10:11 p.m. UTC | #4 
On 18 August 2014 22:54, Joel Schopp <joel.schopp@amd.com> wrote:
>
> +static void vfio_fdt_add_device_node(SysBusDevice *sbdev, void *opaque)
> +{
> +    PlatformDevtreeData *data = opaque;
> +    void *fdt = data->fdt;
> +    const char *parent_node = data->node;
> +    int compat_str_len;
> +    char *nodename;
> +    int i, ret;
> +    uint32_t *irq_attr;
> +    uint64_t *reg_attr;
> +    uint64_t mmio_base;
> +    uint64_t irq_number;
> +    gchar mmio_base_prop[8];
> +    gchar irq_number_prop[8];
> +    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);
> +
> +    compat_str_len = strlen(vdev->compat) + 1;

At this point you've already substituted the NULs in,
so you can't call strlen(), I think.

> +    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++) {
> +        snprintf(mmio_base_prop, sizeof(mmio_base_prop), "mmio[%d]", i);
> +        mmio_base = object_property_get_int(obj, mmio_base_prop, NULL);
> +        reg_attr[2*i] = 1;
> +        reg_attr[2*i+1] = mmio_base;
> +        reg_attr[2*i+2] = 1;
> +        reg_attr[2*i+3] = memory_region_size(&vdev->regions[i]->mem);
> +    }
>
> This should be 4 instead of 2.
> Also, to support 64 bit systems I think this should be 2 instead of 1.

Actually it depends entirely on what the board has done to
create the device tree node that we're inserting this child
node into. For ARM boot.c sets both #address-cells and
#size-cells to 2 regardless of whether the system is 32
or 64 bits, for simplicity. I imagine PPC does something
different. If we're editing a dtb that the user passed in (which
I think would be pretty lunatic so we shouldn't do this)
we'd have to actually walk the dtb to try to figure out what
the semantics of the reg property should be.

thanks
-- PMM
Joel Schopp Aug. 18, 2014, 10:26 p.m. UTC | #5 
On 08/18/2014 05:11 PM, Peter Maydell wrote:
> On 18 August 2014 22:54, Joel Schopp <joel.schopp@amd.com> wrote:
>> +static void vfio_fdt_add_device_node(SysBusDevice *sbdev, void *opaque)
>> +{
>> +    PlatformDevtreeData *data = opaque;
>> +    void *fdt = data->fdt;
>> +    const char *parent_node = data->node;
>> +    int compat_str_len;
>> +    char *nodename;
>> +    int i, ret;
>> +    uint32_t *irq_attr;
>> +    uint64_t *reg_attr;
>> +    uint64_t mmio_base;
>> +    uint64_t irq_number;
>> +    gchar mmio_base_prop[8];
>> +    gchar irq_number_prop[8];
>> +    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);
>> +
>> +    compat_str_len = strlen(vdev->compat) + 1;
> At this point you've already substituted the NULs in,
> so you can't call strlen(), I think.
>
>> +    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++) {
>> +        snprintf(mmio_base_prop, sizeof(mmio_base_prop), "mmio[%d]", i);
>> +        mmio_base = object_property_get_int(obj, mmio_base_prop, NULL);
>> +        reg_attr[2*i] = 1;
>> +        reg_attr[2*i+1] = mmio_base;
>> +        reg_attr[2*i+2] = 1;
>> +        reg_attr[2*i+3] = memory_region_size(&vdev->regions[i]->mem);
>> +    }
>>
>> This should be 4 instead of 2.
>> Also, to support 64 bit systems I think this should be 2 instead of 1.
> Actually it depends entirely on what the board has done to
> create the device tree node that we're inserting this child
> node into. For ARM boot.c sets both #address-cells and
> #size-cells to 2 regardless of whether the system is 32
> or 64 bits, for simplicity. I imagine PPC does something
> different. If we're editing a dtb that the user passed in (which
> I think would be pretty lunatic so we shouldn't do this)
> we'd have to actually walk the dtb to try to figure out what
> the semantics of the reg property should be.
For the index [2*i],[2*i+1], etc is clearly a bug as when i = 1 it will
overwrite two of the values.  Changing that to [4*i],[4*i+1],etc fixes it.

I think you are right on the size.  I also wonder if the user doesn't
pass in a dtb if qemu should try to recreate the device-tree entry from
the platform device entry in the host kernel?  If so would that best be
done by recreating the values from /proc/device-tree ?

I also wish that qemu had a flag to output the generated dtb to a file
much like lkvm (kvmtool) has.
Auger Eric Aug. 19, 2014, 6:59 a.m. UTC | #6 
On 08/18/2014 11:54 PM, Joel Schopp wrote:
> 
> +static void vfio_fdt_add_device_node(SysBusDevice *sbdev, void *opaque)
> +{
> +    PlatformDevtreeData *data = opaque;
> +    void *fdt = data->fdt;
> +    const char *parent_node = data->node;
> +    int compat_str_len;
> +    char *nodename;
> +    int i, ret;
> +    uint32_t *irq_attr;
> +    uint64_t *reg_attr;
> +    uint64_t mmio_base;
> +    uint64_t irq_number;
> +    gchar mmio_base_prop[8];
> +    gchar irq_number_prop[8];
> +    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);
> +
> +    compat_str_len = strlen(vdev->compat) + 1;
> +    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++) {
> +        snprintf(mmio_base_prop, sizeof(mmio_base_prop), "mmio[%d]", i);
> +        mmio_base = object_property_get_int(obj, mmio_base_prop, NULL);
> +        reg_attr[2*i] = 1;
> +        reg_attr[2*i+1] = mmio_base;
> +        reg_attr[2*i+2] = 1;
> +        reg_attr[2*i+3] = memory_region_size(&vdev->regions[i]->mem);
> +    }
> 
> This should be 4 instead of 2. 
Hi Joel,

Yes definitively! Forgot to restore the original value after trying
different qemu_fdt_setprop_* functions. sorry for that.

Best Regards

Eric
> Also, to support 64 bit systems I think this should be 2 instead of 1.
>
Auger Eric Aug. 19, 2014, 7:24 a.m. UTC | #7 
On 08/19/2014 12:11 AM, Peter Maydell wrote:
> On 18 August 2014 22:54, Joel Schopp <joel.schopp@amd.com> wrote:
>>
>> +static void vfio_fdt_add_device_node(SysBusDevice *sbdev, void *opaque)
>> +{
>> +    PlatformDevtreeData *data = opaque;
>> +    void *fdt = data->fdt;
>> +    const char *parent_node = data->node;
>> +    int compat_str_len;
>> +    char *nodename;
>> +    int i, ret;
>> +    uint32_t *irq_attr;
>> +    uint64_t *reg_attr;
>> +    uint64_t mmio_base;
>> +    uint64_t irq_number;
>> +    gchar mmio_base_prop[8];
>> +    gchar irq_number_prop[8];
>> +    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);
>> +
>> +    compat_str_len = strlen(vdev->compat) + 1;
> 
> At this point you've already substituted the NULs in,
> so you can't call strlen(), I think.
Hi Peter,

yes you're right. Thanks
> 
>> +    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++) {
>> +        snprintf(mmio_base_prop, sizeof(mmio_base_prop), "mmio[%d]", i);
>> +        mmio_base = object_property_get_int(obj, mmio_base_prop, NULL);
>> +        reg_attr[2*i] = 1;
>> +        reg_attr[2*i+1] = mmio_base;
>> +        reg_attr[2*i+2] = 1;
>> +        reg_attr[2*i+3] = memory_region_size(&vdev->regions[i]->mem);
>> +    }
>>
>> This should be 4 instead of 2.
>> Also, to support 64 bit systems I think this should be 2 instead of 1.
> 
> Actually it depends entirely on what the board has done to
> create the device tree node that we're inserting this child
> node into. For ARM boot.c sets both #address-cells and
> #size-cells to 2 regardless of whether the system is 32
> or 64 bits, for simplicity. I imagine PPC does something
> different. If we're editing a dtb that the user passed in (which
> I think would be pretty lunatic so we shouldn't do this)
> we'd have to actually walk the dtb to try to figure out what
> the semantics of the reg property should be.

Putting size=1 was the only solution I found to use an offset relative
to the parent bus instead of an absolute base address. I would explain
this because, in platform_bus_create_devtree, the function that creates
the "platform bus" node, #address-cells and #size-cells currently are
set to 1. I assume the motivation was that bus size was supposed to be
smaller than 4GB. Then I guess the problem is shifted to the inclusion
of the platform bus in any ARM platform.

Thanks

Eric
> 
> thanks
> -- PMM
>
Auger Eric Aug. 19, 2014, 7:32 a.m. UTC | #8 
On 08/19/2014 12:26 AM, Joel Schopp wrote:
> 
> On 08/18/2014 05:11 PM, Peter Maydell wrote:
>> On 18 August 2014 22:54, Joel Schopp <joel.schopp@amd.com> wrote:
>>> +static void vfio_fdt_add_device_node(SysBusDevice *sbdev, void *opaque)
>>> +{
>>> +    PlatformDevtreeData *data = opaque;
>>> +    void *fdt = data->fdt;
>>> +    const char *parent_node = data->node;
>>> +    int compat_str_len;
>>> +    char *nodename;
>>> +    int i, ret;
>>> +    uint32_t *irq_attr;
>>> +    uint64_t *reg_attr;
>>> +    uint64_t mmio_base;
>>> +    uint64_t irq_number;
>>> +    gchar mmio_base_prop[8];
>>> +    gchar irq_number_prop[8];
>>> +    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);
>>> +
>>> +    compat_str_len = strlen(vdev->compat) + 1;
>> At this point you've already substituted the NULs in,
>> so you can't call strlen(), I think.
>>
>>> +    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++) {
>>> +        snprintf(mmio_base_prop, sizeof(mmio_base_prop), "mmio[%d]", i);
>>> +        mmio_base = object_property_get_int(obj, mmio_base_prop, NULL);
>>> +        reg_attr[2*i] = 1;
>>> +        reg_attr[2*i+1] = mmio_base;
>>> +        reg_attr[2*i+2] = 1;
>>> +        reg_attr[2*i+3] = memory_region_size(&vdev->regions[i]->mem);
>>> +    }
>>>
>>> This should be 4 instead of 2.
>>> Also, to support 64 bit systems I think this should be 2 instead of 1.
>> Actually it depends entirely on what the board has done to
>> create the device tree node that we're inserting this child
>> node into. For ARM boot.c sets both #address-cells and
>> #size-cells to 2 regardless of whether the system is 32
>> or 64 bits, for simplicity. I imagine PPC does something
>> different. If we're editing a dtb that the user passed in (which
>> I think would be pretty lunatic so we shouldn't do this)
>> we'd have to actually walk the dtb to try to figure out what
>> the semantics of the reg property should be.
> For the index [2*i],[2*i+1], etc is clearly a bug as when i = 1 it will
> overwrite two of the values.  Changing that to [4*i],[4*i+1],etc fixes it.
> 
> I think you are right on the size.  I also wonder if the user doesn't
> pass in a dtb if qemu should try to recreate the device-tree entry from
> the platform device entry in the host kernel?  If so would that best be
> done by recreating the values from /proc/device-tree ?
Antonios recently submitted a patch to retrieve dt info from the vfio
platform device.
[RFC 0/4] VFIO: PLATFORM: Return device tree info for a platform device node
https://www.mail-archive.com/kvm@vger.kernel.org/msg106282.html

Best Regards

Eric
> 
> I also wish that qemu had a flag to output the generated dtb to a file
> much like lkvm (kvmtool) has.
>
Peter Maydell Aug. 19, 2014, 8:17 a.m. UTC | #9 
On 19 August 2014 08:24, Eric Auger <eric.auger@linaro.org> wrote:
> Putting size=1 was the only solution I found to use an offset relative
> to the parent bus instead of an absolute base address. I would explain
> this because, in platform_bus_create_devtree, the function that creates
> the "platform bus" node, #address-cells and #size-cells currently are
> set to 1. I assume the motivation was that bus size was supposed to be
> smaller than 4GB. Then I guess the problem is shifted to the inclusion
> of the platform bus in any ARM platform.

Ah, I see. Yes, if the containing node is setting addr/size to 1
then 1 is correct, and the limitation then is just the 4GB max.

thanks
-- PMM
Alexander Graf Aug. 19, 2014, 10:59 a.m. UTC | #10 
On 19.08.14 00:26, Joel Schopp wrote:
> 
> On 08/18/2014 05:11 PM, Peter Maydell wrote:
>> On 18 August 2014 22:54, Joel Schopp <joel.schopp@amd.com> wrote:
>>> +static void vfio_fdt_add_device_node(SysBusDevice *sbdev, void *opaque)
>>> +{
>>> +    PlatformDevtreeData *data = opaque;
>>> +    void *fdt = data->fdt;
>>> +    const char *parent_node = data->node;
>>> +    int compat_str_len;
>>> +    char *nodename;
>>> +    int i, ret;
>>> +    uint32_t *irq_attr;
>>> +    uint64_t *reg_attr;
>>> +    uint64_t mmio_base;
>>> +    uint64_t irq_number;
>>> +    gchar mmio_base_prop[8];
>>> +    gchar irq_number_prop[8];
>>> +    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);
>>> +
>>> +    compat_str_len = strlen(vdev->compat) + 1;
>> At this point you've already substituted the NULs in,
>> so you can't call strlen(), I think.
>>
>>> +    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++) {
>>> +        snprintf(mmio_base_prop, sizeof(mmio_base_prop), "mmio[%d]", i);
>>> +        mmio_base = object_property_get_int(obj, mmio_base_prop, NULL);
>>> +        reg_attr[2*i] = 1;
>>> +        reg_attr[2*i+1] = mmio_base;
>>> +        reg_attr[2*i+2] = 1;
>>> +        reg_attr[2*i+3] = memory_region_size(&vdev->regions[i]->mem);
>>> +    }
>>>
>>> This should be 4 instead of 2.
>>> Also, to support 64 bit systems I think this should be 2 instead of 1.
>> Actually it depends entirely on what the board has done to
>> create the device tree node that we're inserting this child
>> node into. For ARM boot.c sets both #address-cells and
>> #size-cells to 2 regardless of whether the system is 32
>> or 64 bits, for simplicity. I imagine PPC does something
>> different. If we're editing a dtb that the user passed in (which
>> I think would be pretty lunatic so we shouldn't do this)
>> we'd have to actually walk the dtb to try to figure out what
>> the semantics of the reg property should be.
> For the index [2*i],[2*i+1], etc is clearly a bug as when i = 1 it will
> overwrite two of the values.  Changing that to [4*i],[4*i+1],etc fixes it.
> 
> I think you are right on the size.  I also wonder if the user doesn't
> pass in a dtb if qemu should try to recreate the device-tree entry from
> the platform device entry in the host kernel?  If so would that best be
> done by recreating the values from /proc/device-tree ?
> 
> I also wish that qemu had a flag to output the generated dtb to a file
> much like lkvm (kvmtool) has.

It does. "qemu-system-foo -machine dumpdtb=mydtb.dtb" should dump the
generated dtb into a file called mydtb.dtb.


Alex
Joel Schopp Aug. 19, 2014, 2:15 p.m. UTC | #11 
>> For the index [2*i],[2*i+1], etc is clearly a bug as when i = 1 it will
>> overwrite two of the values.  Changing that to [4*i],[4*i+1],etc fixes it.
>>
>> I think you are right on the size.  I also wonder if the user doesn't
>> pass in a dtb if qemu should try to recreate the device-tree entry from
>> the platform device entry in the host kernel?  If so would that best be
>> done by recreating the values from /proc/device-tree ?
>>
>> I also wish that qemu had a flag to output the generated dtb to a file
>> much like lkvm (kvmtool) has.
> It does. "qemu-system-foo -machine dumpdtb=mydtb.dtb" should dump the
> generated dtb into a file called mydtb.dtb.

Would a patch that adds this output to --help be welcomed?
Alexander Graf Aug. 19, 2014, 2:29 p.m. UTC | #12 
On 19.08.14 16:15, Joel Schopp wrote:
> 
>>> For the index [2*i],[2*i+1], etc is clearly a bug as when i = 1 it will
>>> overwrite two of the values.  Changing that to [4*i],[4*i+1],etc fixes it.
>>>
>>> I think you are right on the size.  I also wonder if the user doesn't
>>> pass in a dtb if qemu should try to recreate the device-tree entry from
>>> the platform device entry in the host kernel?  If so would that best be
>>> done by recreating the values from /proc/device-tree ?
>>>
>>> I also wish that qemu had a flag to output the generated dtb to a file
>>> much like lkvm (kvmtool) has.
>> It does. "qemu-system-foo -machine dumpdtb=mydtb.dtb" should dump the
>> generated dtb into a file called mydtb.dtb.
> 
> Would a patch that adds this output to --help be welcomed?

Not sure -help is the right place for these debugging options. But I
would definitely love to see all -machine options properly documented in
the man page!


Alex
diff mbox

Patch

diff --git a/hw/arm/dyn_sysbus_devtree.c b/hw/arm/dyn_sysbus_devtree.c
index 56af62f..ac34f07 100644
--- a/hw/arm/dyn_sysbus_devtree.c
+++ b/hw/arm/dyn_sysbus_devtree.c
@@ -1,6 +1,139 @@ 
 #include "hw/arm/dyn_sysbus_devtree.h"
 #include "qemu/error-report.h"
 #include "sysemu/device_tree.h"
+#include "hw/vfio/vfio-platform.h"
+
+static void vfio_fdt_add_device_node(SysBusDevice *sbdev, void *opaque);
+
+static char *format_compat(char * compat)
+{
+    char *str_ptr, *corrected_compat;
+    /*
+     * process compatibility property string passed by end-user
+     * replaces / by , and ; by NUL character
+     */
+    corrected_compat = g_strdup(compat);
+    /*
+     * the total length of the string has to include also the last
+     * NUL char.
+     */
+
+    str_ptr = corrected_compat;
+    while ((str_ptr = strchr(str_ptr, '/')) != NULL) {
+        *str_ptr = ',';
+    }
+
+    /* substitute ";" with the NUL char */
+    str_ptr = corrected_compat;
+    while ((str_ptr = strchr(str_ptr, ';')) != NULL) {
+        *str_ptr = '\0';
+    }
+
+    return corrected_compat;
+}
+
+static void wrap_vfio_fdt_add_node(SysBusDevice *sbdev, void *opaque)
+{
+    PlatformDevtreeData *data = opaque;
+    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
+    VFIODevice *vbasedev = &vdev->vbasedev;
+    gchar irq_number_prop[8];
+    Object *obj = OBJECT(sbdev);
+    char *corrected_compat;
+    uint64_t irq_number;
+    int compat_str_len = strlen(vdev->compat)+1;
+    int i;
+
+    corrected_compat = format_compat(vdev->compat);
+    snprintf(vdev->compat, compat_str_len, "%s", corrected_compat);
+    g_free(corrected_compat);
+
+    vfio_fdt_add_device_node(sbdev, opaque);
+
+    for (i = 0; i < vbasedev->num_irqs; i++) {
+        snprintf(irq_number_prop, sizeof(irq_number_prop), "irq[%d]", i);
+        irq_number = object_property_get_int(obj, irq_number_prop, NULL)
+                                                 + data->irq_start;
+        /*
+         * for setting irqfd up we must provide the virtual IRQ number
+         * which is the sum of irq_start and actual platform bus irq
+         * index. At realize point we do not have this info.
+         */
+        if (vdev->irqfd_allowed) {
+            vfio_setup_irqfd(sbdev, i, irq_number);
+        }
+    }
+}
+
+static void vfio_fdt_add_device_node(SysBusDevice *sbdev, void *opaque)
+{
+    PlatformDevtreeData *data = opaque;
+    void *fdt = data->fdt;
+    const char *parent_node = data->node;
+    int compat_str_len;
+    char *nodename;
+    int i, ret;
+    uint32_t *irq_attr;
+    uint64_t *reg_attr;
+    uint64_t mmio_base;
+    uint64_t irq_number;
+    gchar mmio_base_prop[8];
+    gchar irq_number_prop[8];
+    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);
+
+    compat_str_len = strlen(vdev->compat) + 1;
+    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++) {
+        snprintf(mmio_base_prop, sizeof(mmio_base_prop), "mmio[%d]", i);
+        mmio_base = object_property_get_int(obj, mmio_base_prop, NULL);
+        reg_attr[2*i] = 1;
+        reg_attr[2*i+1] = mmio_base;
+        reg_attr[2*i+2] = 1;
+        reg_attr[2*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);
+    if (ret < 0) {
+        error_report("could not set reg property of node %s", nodename);
+    }
+
+    irq_attr = g_new(uint32_t, vbasedev->num_irqs*3);
+
+    for (i = 0; i < vbasedev->num_irqs; i++) {
+        snprintf(irq_number_prop, sizeof(irq_number_prop), "irq[%d]", i);
+        irq_number = object_property_get_int(obj, irq_number_prop, NULL)
+                                                 + 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);
+    }
+
+    g_free(nodename);
+    g_free(irq_attr);
+    g_free(reg_attr);
+}
 
 int sysbus_device_create_devtree(Object *obj, void *opaque)
 {
@@ -17,6 +150,11 @@  int sysbus_device_create_devtree(Object *obj, void *opaque)
         return object_child_foreach(obj, sysbus_device_create_devtree, data);
     }
 
+    if (object_dynamic_cast(obj, TYPE_VFIO_PLATFORM)) {
+        wrap_vfio_fdt_add_node(sbdev, data);
+        matched = true;
+    }
+
     if (!matched) {
         error_report("Device %s is not supported by this machine yet.",
                      qdev_fw_name(DEVICE(dev)));