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

[Eric Auger <eric.auger@linaro.org>]

On 11/05/2014 11:59 AM, Alexander Graf wrote:
> 
> 
> 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?

sure
> 
>>  
>> @@ -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?
My purpose here was absolutely not to come back again on a proposal
where we could have a generic node creation. I understand that it is not
realistic. I rather tried to put some common property creation in this
function but you're right even the interrupt prop depend on the device.

About your question, I think the specialized VFIO device would set its
compat string including the various substrings. This was done in the
past for PL330 which required arm,pl330;arm,primecell.

> 
>> +
>> +    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?
address-cells? since the bus is < 4GB, 1 32b reg is required to specify
the base address. But since you put #size-cells already in the parent
node maybe I can remove it.

> 
>> +        reg_attr[4*i+1] = mmio_base;
>> +        reg_attr[4*i+2] = 1;
> 
> and here?
size-cells for this reg. same remark as above
> 
>> +        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?

this is indeed device specific. In the future I might be able to read
the host dt using Antonios patch but I did not want to add new feature
now and add extra dependencies as per the discussion we had with Alex W.
> 
> 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?

yes I will.
> 
> 
> 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
>>