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

[Oleksandr Grytsov <al1img@gmail.com>]

On 23.03.17 17:55, al1img . wrote:
> On Thu, Mar 23, 2017 at 4:58 PM, Juergen Gross <jgross@suse.com> wrote:
>> 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@suse.com>:
>>>> 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@suse.com>:
>>>>>> On 23/03/17 11:10, Oleksandr Grytsov wrote:
>>>>>>> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
>>>>>>>
>>>>>>> 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
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>>
>>
>
> For me the drawback of more generic way is to cast device_type to
> void* and back which
> may be used by client in improper way.
>
> Thanks.
>

To summarize:

There are two ways to rework the patches:

1. Keep interface between xl an libxl as is and put duplicated code into 
libxl_device_type specific functions.

2. Change interface to call libxl_device_type specific functions 
directly from xl:

libxl_device_vtpm *vtpms;

vtpms = (libxl_device_vtpm*)libxl_device_list_get(LIBXL_DEVTYPE_VTPM, ...);

...

libxl__device_list_free(LIBXL_DEVTYPE_VTPM, vtpms, nr);

But I need feedback from maintainers.

Adding maintainers to CC.


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