Re: [Xen-devel] [PATCH 0/2] libxl: add PV display device driver interface

[Juergen Gross <jgross@xxxxxxxx>]

On 23/03/17 15:23, al1img . wrote:
> This example is clear. But still wrapper macro is required to make it
> visible for libxen client (xl):
> 
> #define LIBXL_DEFINE_DEVICE_LIST_FREE(type)
>     void libxl_device_##type##_list_free(libxl_device_##type* list, int nr)
>     {
>         libxl__device_list_free(libxl__##type##_devtype, list, nr);
>     }

Either this or we could switch to a more generic way avoiding the need
to add multiple very similar functions to libxl:

#define LIBXL_DEVTYPE_VTPM "vtpm"

int libxl_device_list_free(const char *type, void *list, int nr)
{
    int i;

    for (i = 0; device_type_tbl[i]; i++) {
        if (!strcmp(type, device_type_tbl[i]->type)) {
           libxl__device_list_free(device_type_tbl[i], list, nr);
           return 0;
        }
    }
    return ERROR_INVAL;
}

and let xl call it via libxl_device_list_free(LIBXL_DEVTYPE_VTPM, ...)

This is subject to an Ack from the tools maintainers, of course.

> 
> Also where should this function (libxl__device_list_free) be located?
> libxl_device.c?

I think so.

> 
> Another case is getting this list:
> 
> From one side each device should have its own implementation, from
> other side for PV devices
> there is common part like getting devId and backend domId and unique
> part like getting
> device specific parameters (uuid for vtpm). In this case I can do following:
> 
> Implement void *libxl__device_list(struct libxl_device_type *dt,
> libxl_ctx *ctx, uint32_t domid, int *num)
> where I will get devId and backend domId. Then add get_params callback
> to libxl_device_type to get device specific
> parameters:
> 
> void *libxl__device_pv_list(struct libxl_device_type *dt, libxl_ctx
> *ctx, uint32_t domid, int *num)
> {
>     ...
>     if (dt->get_patams) {
>          dt->get_params(...);
>     }
> }
> 
> And vtpm get list may look like:
> 
> libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t
> domid, int *num)
> {
>     return libxl__device_list(&libxl__vtpm_devtype, ctx, domid, num);
> }
> 
> DEFINE_DEVICE_TYPE_STRUCT(vtpm,
>     .update_config = libxl_device_vtpm_update_config
>     .get_params = libxl_device_vtpm_get_params
> );
> 
> Correct?

Right. Possibly using the same trick as above.

BTW: Please don't top-post.


Juergen

> 
> 
> 2017-03-23 14:08 GMT+02:00 Juergen Gross <jgross@xxxxxxxx>:
>> On 23/03/17 12:32, al1img . wrote:
>>> Hi Juergen,
>>>
>>> I've checked the mentioned commits. And I don't see how it can be done.
>>> The duplication I see it is in libxl_device_type.add and
>>> libxl_device_type.list functions
>>> for different PV devices. These functions have a lot of common code
>>> which I've tried
>>> to move to macros in my patches.
>>
>> Okay, here an example for replacement of LIBXL_DEFINE_DEVICE_LIST_FREE()
>>
>> void libxl_device_list_free(struct libxl_device_type *dt,
>>                             void *list, int nr)
>> {
>>     int i;
>>
>>     for (i = 0; i < nr; i++)
>>         dt->dispose(list + i * dt->dev_elem_size);
>>     free(list);
>> }
>>
>> BTW: exactly this pattern is to be found at the very end of
>> libxl_retrieve_domain_configuration().
>>
>> The others should be doable, too.
>>
>>
>> Juergen
>>
>>>
>>> 2017-03-23 12:21 GMT+02:00 Juergen Gross <jgross@xxxxxxxx>:
>>>> On 23/03/17 11:10, Oleksandr Grytsov wrote:
>>>>> From: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
>>>>>
>>>>> Hi all,
>>>>>
>>>>> We are working on series of PV drivers (display, sound, input etc.) and
>>>>> would like to add their support to libxl and xl. These patches add PV 
>>>>> display
>>>>> device. PV display is based on [1] protocol.
>>>>>
>>>>> During implementation I see a lot of code duplication. This problem was
>>>>> mentioned in [2]. One of the solutions was to implement common parts in 
>>>>> IDL
>>>>> and make them autogenerated. On my side, to minimize the copy/pasting
>>>>> I've moved common parts into macro functions: LIBXL_DEFINE_DEVICE_COMMIT,
>>>>> LIBXL_DEFINE_DEVICE_LIST_GET, LIBXL_DEFINE_DEVICE_GETINFO etc.
>>>>> Existing PV devices implementations can be reworked to use these macros as
>>>>> well. Any other proposals to avoid the duplications are welcome.
>>>>
>>>> Did you look into the device type framework I introduced with commit
>>>> 74e857c6c7f9 and some followups? Maybe it is possible to expand this
>>>> framework by adding more callbacks to struct libxl_device_type and
>>>> have some common function(s) in libxl_device.c?
>>>>
>>>> Juergen
>>>
>>>
>>>
>>
> 
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel