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: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.
>