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

[Geert Uytterhoeven]

Hi Eric,

On Tue, Aug 7, 2018 at 4:18 PM Auger Eric <address@hidden> wrote:
> On 07/25/2018 04:34 PM, Geert Uytterhoeven wrote:
> > From: Auger Eric <address@hidden>
> >
> > 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 <address@hidden>
> > [geert: Rebase, set user_creatable=true, use compatible values in sysfs
> >       instead of user-supplied manufacturer/model options, reword]
> > Signed-off-by: Geert Uytterhoeven <address@hidden>
> > ---
> > 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.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- address@hidden

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds