Re: [Qemu-devel] [PATCH v5 10/10] hw/arm/dyn_sysbus_devtree: enable simple VFIO dynamic instantiation

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

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
>