Re: [Qemu-devel] [PATCH v7 12/16] hw/arm/sysbus-fdt: enable vfio-calxeda-xgmac dynamic instantiation

[Alexander Graf <agraf@suse.de>]

On 31.10.14 15:05, Eric Auger wrote:
> vfio-calxeda-xgmac now can be instantiated using the -device option.
> The node creation function generates a very basic dt node composed
> of the compat, reg and interrupts properties
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v6 -> v7:
> - compat string re-formatting removed since compat string is not exposed
>   anymore as a user option
> - VFIO IRQ kick-off removed from sysbus-fdt and moved to VFIO platform
>   device
> ---
>  hw/arm/sysbus-fdt.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index d5476f1..f8b310b 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -27,6 +27,8 @@
>  #include "hw/platform-bus.h"
>  #include "sysemu/sysemu.h"
>  #include "hw/platform-bus.h"
> +#include "hw/vfio/vfio-platform.h"
> +#include "hw/vfio/vfio-calxeda-xgmac.h"
>  
>  /*
>   * internal struct that contains the information to create dynamic
> @@ -54,8 +56,11 @@ typedef struct NodeCreationPair {
>      int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
>  } NodeCreationPair;
>  
> +static int add_basic_vfio_fdt_node(SysBusDevice *sbdev, void *opaque);
> +
>  /* list of supported dynamic sysbus devices */
>  NodeCreationPair add_fdt_node_functions[] = {
> +        {TYPE_VFIO_CALXEDA_XGMAC, add_basic_vfio_fdt_node},
>          {"", NULL}, /*last element*/
>  };

Can you maybe place the list somewhere smartly to make sure we don't
need forward declarations? Either put it in between the "generic" and
"device specific" code or at the end of the file with a single forward
declaration for the array?

>  
> @@ -86,6 +91,89 @@ static int add_fdt_node(SysBusDevice *sbdev, void *opaque)
>  }
>  
>  /**
> + * add_basic_vfio_fdt_node - generates the most basic node for a VFIO node
> + *
> + * set properties are:
> + * - compatible string
> + * - regs
> + * - interrupts
> + */
> +static int add_basic_vfio_fdt_node(SysBusDevice *sbdev, void *opaque)
> +{
> +    PlatformBusFdtData *data = opaque;
> +    PlatformBusDevice *pbus = data->pbus;
> +    void *fdt = data->fdt;
> +    const char *parent_node = data->pbus_node_name;
> +    int compat_str_len;
> +    char *nodename;
> +    int i, ret;
> +    uint32_t *irq_attr;
> +    uint64_t *reg_attr;
> +    uint64_t mmio_base;
> +    uint64_t irq_number;
> +    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);

What if there are multiple compatibles?

> +
> +    reg_attr = g_new(uint64_t, vbasedev->num_regions*4);
> +
> +    for (i = 0; i < vbasedev->num_regions; i++) {
> +        mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i);
> +        reg_attr[4*i] = 1;

What is the 1 here?

> +        reg_attr[4*i+1] = mmio_base;
> +        reg_attr[4*i+2] = 1;

and here?

> +        reg_attr[4*i+3] = memory_region_size(&vdev->regions[i]->mem);
> +    }
> +
> +    ret = qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "reg",
> +                     vbasedev->num_regions*2, reg_attr);
> +    if (ret < 0) {
> +        error_report("could not set reg property of node %s", nodename);
> +        goto fail;
> +    }
> +
> +    irq_attr = g_new(uint32_t, vbasedev->num_irqs*3);
> +
> +    for (i = 0; i < vbasedev->num_irqs; i++) {
> +        irq_number = platform_bus_get_irqn(pbus, sbdev , i)
> +                         + data->irq_start;
> +        irq_attr[3*i] = cpu_to_be32(0);
> +        irq_attr[3*i+1] = cpu_to_be32(irq_number);
> +        irq_attr[3*i+2] = cpu_to_be32(0x4);

Why 0x4? How do you know whether an IRQ is edge or level triggered?

I'm still not convinced we can make anything "generic" on the VFIO path.
How about you call the function xgmac specific for now, but keep the
code as dynamic as it is?


Alex

> +    }
> +
> +   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);
> +        goto fail;
> +    }
> +
> +    g_free(nodename);
> +    g_free(irq_attr);
> +    g_free(reg_attr);
> +
> +    return 0;
> +
> +fail:
> +
> +   return -1;
> +}
> +
> +/**
>   * add_all_platform_bus_fdt_nodes - create all the platform bus nodes
>   *
>   * builds the parent platform bus node and all the nodes of dynamic
>