Re: [Qemu-devel] [PATCH v3 1/4] vfio/platform: Make the vfio-platform device non-abstract

[Auger Eric <eric.auger@redhat.com>]

Hi Geert,

On 08/07/2018 05:00 PM, Geert Uytterhoeven wrote:
> Hi Eric,
> 
> On Tue, Aug 7, 2018 at 4:18 PM Auger Eric <eric.auger@redhat.com> wrote:
>> On 07/25/2018 04:34 PM, Geert Uytterhoeven wrote:
>>> From: Auger Eric <eric.auger@redhat.com>
>>>
>>> Up to now the vfio-platform device has been abstract and could not be
>>> instantiated.  The integration of a new vfio platform device required
>>> creating a dummy derived device which only set the compatible string.
>>>
>>> Following the few vfio-platform device integrations we have seen the
>>> actual requested adaptation happens on device tree node creation
>>> (sysbus-fdt).
>>>
>>> Hence remove the abstract setting, and read the list of compatible
>>> values from sysfs if not set by a derived device.
>>>
>>> Update the amd-xgbe and calxeda-xgmac drivers to fill in the number of
>>> compatible values, as there can now be more than one.
>>>
>>> Note that sysbus-fdt does not support the instantiation of the
>>> vfio-platform device yet.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> [geert: Rebase, set user_creatable=true, use compatible values in sysfs
>>>       instead of user-supplied manufacturer/model options, reword]
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>> v3:
>>>   - Read all compatible values from sysfs instead of using user-supplied
>>>     manufacturer and model options,
>>>   - Reword patch description,
>>>   - Drop RFC state,
>>>
>>> v2:
>>>   - No changes,
>>>
>>> v1:
>>>   - Rebase, Set user_creatable=true,
>>>
>>> v0:
>>>   - Original version from Eric.
> 
>>> --- a/hw/vfio/platform.c
>>> +++ b/hw/vfio/platform.c
>>> @@ -655,6 +655,25 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
>>>          goto out;
>>>      }
>>>
>>> +    if (!vdev->compat) {
>>> +        gchar *contents;
>>> +        gsize length;
>>> +        char *tmp;
>>> +
>>> +        tmp = g_strdup_printf("%s/of_node/compatible", vbasedev->sysfsdev);
>>> +        if (!g_file_get_contents(tmp, &contents, &length, NULL)) {
>>> +            error_report("failed to load \"%s\"", tmp);
>>> +            exit(1);
>> You should set errp instead so that the error gets properly propagated.
> 
> Thanks, will do.
> 
>>> --- a/include/hw/vfio/vfio-platform.h
>>> +++ b/include/hw/vfio/vfio-platform.h
>>> @@ -54,7 +54,8 @@ typedef struct VFIOPlatformDevice {
>>>      QLIST_HEAD(, VFIOINTp) intp_list; /* list of IRQs */
>>>      /* queue of pending IRQs */
>>>      QSIMPLEQ_HEAD(pending_intp_queue, VFIOINTp) pending_intp_queue;
>>> -    char *compat; /* compatibility string */
>>> +    char *compat; /* DT compatible values, separated by NUL */
>> by NULL characters?
> 
> "NUL" is the character ('\0'), "NULL" is the pointer.
Ah OK ;-)

Thanks

Eric
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>