* [PATCH 0/2] libxl: add PV display device driver interface
@ 2017-03-23 10:10 Oleksandr Grytsov
2017-03-23 10:10 ` [PATCH 1/2] " Oleksandr Grytsov
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Oleksandr Grytsov @ 2017-03-23 10:10 UTC (permalink / raw)
To: xen-devel; +Cc: Oleksandr Grytsov
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.
Thanks.
[1] http://marc.info/?l=xen-devel&m=149000029128972&w=2
[2] http://marc.info/?l=xen-devel&m=145372933919792&w=2
Oleksandr Grytsov (2):
libxl: add PV display device driver interface
xl: add PV display device commands
tools/libxl/Makefile | 2 +-
tools/libxl/libxl.h | 21 ++++
tools/libxl/libxl_create.c | 1 +
tools/libxl/libxl_internal.h | 228 +++++++++++++++++++++++++++++++++++
tools/libxl/libxl_types.idl | 22 +++-
tools/libxl/libxl_types_internal.idl | 1 +
tools/libxl/libxl_utils.h | 4 +
tools/libxl/libxl_vdispl.c | 137 +++++++++++++++++++++
tools/xl/Makefile | 1 +
tools/xl/xl.h | 3 +
tools/xl/xl_cmdtable.c | 16 +++
tools/xl/xl_parse.c | 44 ++++++-
tools/xl/xl_parse.h | 2 +-
tools/xl/xl_vdispl.c | 162 +++++++++++++++++++++++++
14 files changed, 639 insertions(+), 5 deletions(-)
create mode 100644 tools/libxl/libxl_vdispl.c
create mode 100644 tools/xl/xl_vdispl.c
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] libxl: add PV display device driver interface 2017-03-23 10:10 [PATCH 0/2] libxl: add PV display device driver interface Oleksandr Grytsov @ 2017-03-23 10:10 ` Oleksandr Grytsov 2017-03-23 10:10 ` [PATCH 2/2] xl: add PV display device commands Oleksandr Grytsov 2017-03-23 10:21 ` [PATCH 0/2] libxl: add PV display device driver interface Juergen Gross 2 siblings, 0 replies; 11+ messages in thread From: Oleksandr Grytsov @ 2017-03-23 10:10 UTC (permalink / raw) To: xen-devel; +Cc: Oleksandr Grytsov From: Oleksandr Grytsov <oleksandr_grytsov@epam.com> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> --- tools/libxl/Makefile | 2 +- tools/libxl/libxl.h | 21 ++++ tools/libxl/libxl_create.c | 1 + tools/libxl/libxl_internal.h | 228 +++++++++++++++++++++++++++++++++++ tools/libxl/libxl_types.idl | 22 +++- tools/libxl/libxl_types_internal.idl | 1 + tools/libxl/libxl_utils.h | 4 + tools/libxl/libxl_vdispl.c | 137 +++++++++++++++++++++ 8 files changed, 413 insertions(+), 3 deletions(-) create mode 100644 tools/libxl/libxl_vdispl.c diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile index f00d9ef..cb7c17f 100644 --- a/tools/libxl/Makefile +++ b/tools/libxl/Makefile @@ -138,7 +138,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \ libxl_dom_suspend.o libxl_dom_save.o libxl_usb.o \ libxl_vtpm.o libxl_nic.o libxl_disk.o libxl_console.o \ libxl_cpupool.o libxl_mem.o libxl_sched.o libxl_tmem.o \ - libxl_domain.o \ + libxl_domain.o libxl_vdispl.o \ $(LIBXL_OBJS-y) LIBXL_OBJS += libxl_genid.o LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 92f1751..a4fc304 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -1833,6 +1833,27 @@ libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t domid, int *n int libxl_device_vtpm_getinfo(libxl_ctx *ctx, uint32_t domid, libxl_device_vtpm *vtpm, libxl_vtpminfo *vtpminfo); +/* Virtual displays */ +int libxl_device_vdispl_add(libxl_ctx *ctx, uint32_t domid, + libxl_device_vdispl *displ, + const libxl_asyncop_how *ao_how) + LIBXL_EXTERNAL_CALLERS_ONLY; +int libxl_device_vdispl_remove(libxl_ctx *ctx, uint32_t domid, + libxl_device_vdispl *vdispl, + const libxl_asyncop_how *ao_how) + LIBXL_EXTERNAL_CALLERS_ONLY; +int libxl_device_vdispl_destroy(libxl_ctx *ctx, uint32_t domid, + libxl_device_vdispl *vdispl, + const libxl_asyncop_how *ao_how) + LIBXL_EXTERNAL_CALLERS_ONLY; + +libxl_device_vdispl *libxl_device_vdispl_list(libxl_ctx *ctx, uint32_t domid, + int *num); +void libxl_device_vdispl_list_free(libxl_device_vdispl* list, int num); +int libxl_device_vdispl_getinfo(libxl_ctx *ctx, uint32_t domid, + libxl_device_vdispl *vdispl, + libxl_vdisplinfo *vdisplinfo); + /* Keyboard */ int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb, const libxl_asyncop_how *ao_how) diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index e741b9a..19e0683 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -1445,6 +1445,7 @@ const struct libxl_device_type *device_type_tbl[] = { &libxl__usbdev_devtype, &libxl__pcidev_devtype, &libxl__dtdev_devtype, + &libxl__vdispl_devtype, NULL }; diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 5bbede5..787bebb 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -3465,6 +3465,233 @@ _hidden void libxl__bootloader_run(libxl__egc*, libxl__bootloader_state *st); LIBXL_DEFINE_DEVICE_REMOVE_EXT(type, type, remove, 0) \ LIBXL_DEFINE_DEVICE_REMOVE_EXT(type, type, destroy, 1) +#define LIBXL_DEFINE_DEVICE_COMMIT(type) \ + static int libxl__device_##type##_commit(libxl__egc *egc, \ + uint32_t domid, libxl_device_##type *type, \ + flexarray_t *front, flexarray_t *back, \ + libxl__ao_device *aodev, int rc) \ + { \ + STATE_AO_GC(aodev->ao); \ + libxl__device *device; \ + xs_transaction_t t = XBT_NULL; \ + libxl_domain_config d_config; \ + libxl__domain_userdata_lock *lock = NULL; \ + \ + libxl_domain_config_init(&d_config); \ + \ + if (rc) goto out; \ + \ + if (aodev->update_json) { \ + lock = libxl__lock_domain_userdata(gc, domid); \ + if (!lock) { rc = ERROR_LOCK_FAIL; goto out; } \ + \ + rc = libxl__get_domain_configuration(gc, domid, &d_config); \ + if (rc) goto out; \ + \ + DEVICE_ADD(type, type##s, domid, type, \ + COMPARE_DEVID, &d_config); \ + \ + rc = libxl__dm_check_start(gc, &d_config, domid); \ + if (rc) goto out; \ + } \ + \ + GCNEW(device); \ + rc = libxl__device_from_##type(gc, domid, type, device); \ + if ( rc != 0 ) goto out; \ + \ + for (;;) { \ + rc = libxl__xs_transaction_start(gc, &t); \ + if (rc) goto out; \ + \ + rc = libxl__device_exists(gc, t, device); \ + if (rc < 0) goto out; \ + if (rc == 1) { \ + LOGD(ERROR, domid, "device already exists in xenstore");\ + aodev->action = LIBXL__DEVICE_ACTION_ADD; \ + rc = ERROR_DEVICE_EXISTS; \ + goto out; \ + } \ + \ + if (aodev->update_json) { \ + rc = libxl__set_domain_configuration(gc, domid, \ + &d_config); \ + if (rc) goto out; \ + } \ + \ + libxl__device_generic_add(gc, t, device, \ + libxl__xs_kvs_of_flexarray(gc, back), \ + libxl__xs_kvs_of_flexarray(gc, front), \ + NULL); \ + \ + rc = libxl__xs_transaction_commit(gc, &t); \ + if (rc < 0) { rc = ERROR_FAIL; goto out; } \ + if (!rc) break; \ + } \ + \ + aodev->dev = device; \ + aodev->action = LIBXL__DEVICE_ACTION_ADD; \ + libxl__wait_device_connection(egc, aodev); \ + rc = 0; \ + \ + out: \ + libxl__xs_transaction_abort(gc, &t); \ + if (lock) libxl__unlock_domain_userdata(lock); \ + libxl_domain_config_dispose(&d_config); \ + aodev->rc = rc; \ + if(rc) aodev->callback(egc, aodev); \ + return rc; \ + } + +#define LIBXL_DEFINE_DEVICE_LIST_GET(type) \ + libxl_device_##type *libxl_device_##type##_list(libxl_ctx *ctx, \ + uint32_t domid, \ + int *num) \ + { \ + GC_INIT(ctx); \ + \ + libxl_device_##type* types = NULL; \ + libxl_device_##type* r = NULL; \ + libxl_device_##type* type; \ + char *libxl_path; \ + char** dir = NULL; \ + unsigned int ndirs = 0; \ + int rc; \ + \ + *num = 0; \ + \ + libxl_path = GCSPRINTF("%s/device/%s", \ + libxl__xs_libxl_path(gc, domid), #type); \ + \ + dir = libxl__xs_directory(gc, XBT_NULL, libxl_path, &ndirs); \ + \ + if (dir && ndirs) { \ + types = malloc(sizeof(*types) * ndirs); \ + libxl_device_##type* end = types + ndirs; \ + \ + for(type = types; type < end; ++type, ++dir) { \ + const char* be_path = libxl__xs_read(gc, XBT_NULL, \ + GCSPRINTF("%s/%s/backend", libxl_path, *dir)); \ + \ + libxl_device_##type##_init(type); \ + \ + type->devid = atoi(*dir); \ + \ + rc = libxl__backendpath_parse_domid(gc, be_path, \ + &type->backend_domid); \ + if (rc) goto out; \ + } \ + } \ + \ + *num = ndirs; \ + r = types; \ + types = NULL; \ + \ + out: \ + if (types) { \ + for(; type >= types; --type) { \ + libxl_device_##type##_dispose(type); \ + } \ + free(types); \ + } \ + \ + GC_FREE; \ + \ + return r; \ + } + +#define LIBXL_DEFINE_DEVICE_LIST_FREE(type) \ + void libxl_device_##type##_list_free(libxl_device_##type* list, \ + int nr) \ + { \ + int i; \ + for (i = 0; i < nr; i++) { \ + libxl_device_##type##_dispose(&list[i]); \ + } \ + free(list); \ + } + +#define LIBXL_DEFINE_DEVID_TO_DEVICE(type) \ + int libxl_devid_to_device_##type(libxl_ctx *ctx, \ + uint32_t domid, \ + int devid, \ + libxl_device_##type *type) \ + { \ + libxl_device_##type *types = NULL; \ + int n, i; \ + int rc; \ + \ + libxl_device_##type##_init(type); \ + \ + types = libxl_device_##type##_list(ctx, domid, &n); \ + if (!types) { rc = ERROR_NOTFOUND; goto out; } \ + \ + for (i = 0; i < n; ++i) { \ + if (devid == types[i].devid) { \ + type->backend_domid = types[i].backend_domid; \ + type->devid = types[i].devid; \ + rc = 0; \ + goto out; \ + } \ + } \ + \ + rc = ERROR_NOTFOUND; \ + \ + out: \ + if (types) { \ + libxl_device_##type##_list_free(types, n); \ + } \ + return rc; \ + } + +#define LIBXL_DEFINE_DEVICE_GETINFO(type) \ + int libxl_device_##type##_getinfo(libxl_ctx *ctx, \ + uint32_t domid, \ + libxl_device_##type *type, \ + libxl_##type##info *info) \ + { \ + GC_INIT(ctx); \ + char *libxl_path, *dompath, *devpath; \ + char *val; \ + int rc; \ + \ + libxl_##type##info_init(info); \ + dompath = libxl__xs_get_dompath(gc, domid); \ + info->devid = type->devid; \ + \ + devpath = GCSPRINTF("%s/device/"#type"/%d", \ + dompath, info->devid); \ + libxl_path = GCSPRINTF("%s/device/"#type"/%d", \ + libxl__xs_libxl_path(gc, domid), \ + info->devid); \ + info->backend = xs_read(ctx->xsh, XBT_NULL, \ + GCSPRINTF("%s/backend", libxl_path), \ + NULL); \ + if (!info->backend) { rc = ERROR_FAIL; goto out; } \ + \ + rc = libxl__backendpath_parse_domid(gc, info->backend, \ + &info->backend_id); \ + if (rc) { goto out; } \ + \ + val = libxl__xs_read(gc, XBT_NULL, \ + GCSPRINTF("%s/state", devpath)); \ + info->state = val ? strtoul(val, NULL, 10) : -1; \ + \ + info->frontend = xs_read(ctx->xsh, XBT_NULL, \ + GCSPRINTF("%s/frontend", libxl_path), \ + NULL); \ + info->frontend_id = domid; \ + \ + rc = libxl__device_##type##_getinfo(gc, ctx->xsh, \ + libxl_path, info); \ + if (rc) { goto out; } \ + \ + rc = 0; \ + \ + out: \ + GC_FREE; \ + return rc; \ + } + struct libxl_device_type { char *type; int skip_attach; /* Skip entry in domcreate_attach_devices() if 1 */ @@ -3524,6 +3751,7 @@ extern const struct libxl_device_type libxl__vtpm_devtype; extern const struct libxl_device_type libxl__usbctrl_devtype; extern const struct libxl_device_type libxl__usbdev_devtype; extern const struct libxl_device_type libxl__pcidev_devtype; +extern const struct libxl_device_type libxl__vdispl_devtype; extern const struct libxl_device_type *device_type_tbl[]; diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index a612d1f..46845f0 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -690,7 +690,14 @@ libxl_device_vtpm = Struct("device_vtpm", [ ("backend_domname", string), ("devid", libxl_devid), ("uuid", libxl_uuid), -]) + ]) + +libxl_device_vdispl = Struct("device_vdispl", [ + ("backend_domid", libxl_domid), + ("backend_domname", string), + ("devid", libxl_devid), + ("be_alloc", bool), + ]) libxl_device_channel = Struct("device_channel", [ ("backend_domid", libxl_domid), @@ -702,7 +709,7 @@ libxl_device_channel = Struct("device_channel", [ ("pty", None), ("socket", Struct(None, [("path", string)])), ])), -]) + ]) libxl_domain_config = Struct("domain_config", [ ("c_info", libxl_domain_create_info), @@ -716,6 +723,7 @@ libxl_domain_config = Struct("domain_config", [ ("vfbs", Array(libxl_device_vfb, "num_vfbs")), ("vkbs", Array(libxl_device_vkb, "num_vkbs")), ("vtpms", Array(libxl_device_vtpm, "num_vtpms")), + ("vdispls", Array(libxl_device_vdispl, "num_vdispls")), # a channel manifests as a console with a name, # see docs/misc/channels.txt ("channels", Array(libxl_device_channel, "num_channels")), @@ -812,6 +820,16 @@ libxl_physinfo = Struct("physinfo", [ ("cap_hvm_directio", bool), ], dir=DIR_OUT) +libxl_vdisplinfo = Struct("vdisplinfo", [ + ("backend", string), + ("backend_id", uint32), + ("frontend", string), + ("frontend_id", uint32), + ("devid", libxl_devid), + ("state", integer), + ("be_alloc", bool), + ], dir=DIR_OUT) + # NUMA node characteristics: size and free are how much memory it has, and how # much of it is free, respectively. dists is an array of distances from this # node to each other node. diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl index 82e5c07..cdef4d3 100644 --- a/tools/libxl/libxl_types_internal.idl +++ b/tools/libxl/libxl_types_internal.idl @@ -25,6 +25,7 @@ libxl__device_kind = Enumeration("device_kind", [ (8, "VTPM"), (9, "VUSB"), (10, "QUSB"), + (11, "VDISPL") ]) libxl__console_backend = Enumeration("console_backend", [ diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h index 25773d8..9e743dc 100644 --- a/tools/libxl/libxl_utils.h +++ b/tools/libxl/libxl_utils.h @@ -78,6 +78,10 @@ int libxl_devid_to_device_vtpm(libxl_ctx *ctx, uint32_t domid, int devid, libxl_device_vtpm *vtpm); int libxl_devid_to_device_usbctrl(libxl_ctx *ctx, uint32_t domid, int devid, libxl_device_usbctrl *usbctrl); + +int libxl_devid_to_device_vdispl(libxl_ctx *ctx, uint32_t domid, + int devid, libxl_device_vdispl *vdispl); + int libxl_ctrlport_to_device_usbdev(libxl_ctx *ctx, uint32_t domid, int ctrl, int port, libxl_device_usbdev *usbdev); diff --git a/tools/libxl/libxl_vdispl.c b/tools/libxl/libxl_vdispl.c new file mode 100644 index 0000000..869aaa1 --- /dev/null +++ b/tools/libxl/libxl_vdispl.c @@ -0,0 +1,137 @@ +/* + * Copyright (C) 2016 EPAM Systems Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published + * by the Free Software Foundation; version 2.1 only. with the special + * exception on linking described in file LICENSE. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + */ + +#include "libxl_osdeps.h" + +#include "libxl_internal.h" + +static int libxl__device_vdispl_setdefault(libxl__gc *gc, domid_t domid, + libxl_device_vdispl *vdispl) +{ + int rc; + + rc = libxl__resolve_domid(gc, vdispl->backend_domname, + &vdispl->backend_domid); + + if (vdispl->devid == -1) { + vdispl->devid = libxl__device_nextid(gc, domid, + (char*)libxl__device_kind_to_string(LIBXL__DEVICE_KIND_VDISPL)); + } + + return rc; +} + +static int libxl__device_from_vdispl(libxl__gc *gc, uint32_t domid, + libxl_device_vdispl *vdispl, + libxl__device *device) +{ + device->backend_devid = vdispl->devid; + device->backend_domid = vdispl->backend_domid; + device->backend_kind = LIBXL__DEVICE_KIND_VDISPL; + device->devid = vdispl->devid; + device->domid = domid; + device->kind = LIBXL__DEVICE_KIND_VDISPL; + + return 0; +} + +LIBXL_DEFINE_DEVICE_COMMIT(vdispl) + +static void libxl__device_vdispl_add(libxl__egc *egc, uint32_t domid, + libxl_device_vdispl *vdispl, + libxl__ao_device *aodev) +{ + STATE_AO_GC(aodev->ao); + flexarray_t *front; + flexarray_t *back; + int rc; + + rc = libxl__device_vdispl_setdefault(gc, domid, vdispl); + if (rc) goto out; + + front = flexarray_make(gc, 16, 1); + back = flexarray_make(gc, 16, 1); + + flexarray_append(back, "frontend-id"); + flexarray_append(back, GCSPRINTF("%d", domid)); + flexarray_append(back, "online"); + flexarray_append(back, "1"); + flexarray_append(back, "state"); + flexarray_append(back, GCSPRINTF("%d", XenbusStateInitialising)); + flexarray_append(back, "handle"); + flexarray_append(back, GCSPRINTF("%d", vdispl->devid)); + + flexarray_append(front, "backend-id"); + flexarray_append(front, GCSPRINTF("%d", vdispl->backend_domid)); + flexarray_append(front, "state"); + flexarray_append(front, GCSPRINTF("%d", XenbusStateInitialising)); + flexarray_append(front, "handle"); + flexarray_append(front, GCSPRINTF("%d", vdispl->devid)); + flexarray_append(front, "be_alloc"); + flexarray_append(front, GCSPRINTF("%d", vdispl->be_alloc)); + +out: + libxl__device_vdispl_commit(egc, domid, vdispl, front, back, aodev, rc); +} + +static int libxl__device_vdispl_getinfo(libxl__gc *gc, struct xs_handle *xsh, + const char* libxl_path, + libxl_vdisplinfo *info) +{ + char *val; + + val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/be_alloc", libxl_path)); + info->be_alloc = val ? strtoul(val, NULL, 10) : 0; + + return 0; +} + +static void libxl__update_config_vdispl(libxl__gc *gc, void *dst, void *src) +{ + libxl_device_vdispl *d, *s; + + d = (libxl_device_vdispl*)dst; + s = (libxl_device_vdispl*)src; + + d->devid = s->devid; + d->be_alloc = s->be_alloc; +} + +static int libxl_device_vdispl_compare(libxl_device_vdispl *d1, + libxl_device_vdispl *d2) +{ + return COMPARE_DEVID(d1, d2); +} + +LIBXL_DEFINE_DEVICE_ADD(vdispl) +static LIBXL_DEFINE_DEVICES_ADD(vdispl) +LIBXL_DEFINE_DEVICE_REMOVE(vdispl) + +LIBXL_DEFINE_DEVICE_LIST_GET(vdispl) +LIBXL_DEFINE_DEVICE_LIST_FREE(vdispl) + +LIBXL_DEFINE_DEVID_TO_DEVICE(vdispl) +LIBXL_DEFINE_DEVICE_GETINFO(vdispl) + +DEFINE_DEVICE_TYPE_STRUCT(vdispl, + .update_config = libxl__update_config_vdispl +); + +/* + * Local variables: + * mode: C + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] xl: add PV display device commands 2017-03-23 10:10 [PATCH 0/2] libxl: add PV display device driver interface Oleksandr Grytsov 2017-03-23 10:10 ` [PATCH 1/2] " Oleksandr Grytsov @ 2017-03-23 10:10 ` Oleksandr Grytsov 2017-03-23 10:21 ` [PATCH 0/2] libxl: add PV display device driver interface Juergen Gross 2 siblings, 0 replies; 11+ messages in thread From: Oleksandr Grytsov @ 2017-03-23 10:10 UTC (permalink / raw) To: xen-devel; +Cc: Oleksandr Grytsov From: Oleksandr Grytsov <oleksandr_grytsov@epam.com> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> --- tools/xl/Makefile | 1 + tools/xl/xl.h | 3 + tools/xl/xl_cmdtable.c | 16 +++++ tools/xl/xl_parse.c | 44 +++++++++++++- tools/xl/xl_parse.h | 2 +- tools/xl/xl_vdispl.c | 162 +++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 226 insertions(+), 2 deletions(-) create mode 100644 tools/xl/xl_vdispl.c diff --git a/tools/xl/Makefile b/tools/xl/Makefile index e16f877..5c784b5 100644 --- a/tools/xl/Makefile +++ b/tools/xl/Makefile @@ -21,6 +21,7 @@ XL_OBJS += xl_vtpm.o xl_block.o xl_nic.o xl_usb.o XL_OBJS += xl_sched.o xl_pci.o xl_vcpu.o xl_cdrom.o xl_mem.o XL_OBJS += xl_psr.o xl_info.o xl_console.o xl_misc.o XL_OBJS += xl_vmcontrol.o xl_saverestore.o xl_migrate.o +XL_OBJS += xl_vdispl.o $(XL_OBJS): CFLAGS += $(CFLAGS_libxentoollog) $(XL_OBJS): CFLAGS += $(CFLAGS_XL) diff --git a/tools/xl/xl.h b/tools/xl/xl.h index 1ad0726..370a0d4 100644 --- a/tools/xl/xl.h +++ b/tools/xl/xl.h @@ -165,6 +165,9 @@ int main_blockdetach(int argc, char **argv); int main_vtpmattach(int argc, char **argv); int main_vtpmlist(int argc, char **argv); int main_vtpmdetach(int argc, char **argv); +int main_vdisplattach(int argc, char **argv); +int main_vdispllist(int argc, char **argv); +int main_vdispldetach(int argc, char **argv); int main_usbctrl_attach(int argc, char **argv); int main_usbctrl_detach(int argc, char **argv); int main_usbdev_attach(int argc, char **argv); diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c index 1219b33..1050dc1 100644 --- a/tools/xl/xl_cmdtable.c +++ b/tools/xl/xl_cmdtable.c @@ -372,6 +372,22 @@ struct cmd_spec cmd_table[] = { "Destroy a domain's virtual TPM device", "<Domain> <DevId|uuid>", }, + { "vdispl-attach", + &main_vdisplattach, 1, 1, + "Create a new virtual display device", + "<Domain> [devId=<Device>] [backend=<BackDomain>] [beAlloc=<BackAlloc>]", + " BackAlloc - set to 1 to allow backend allocated display buffers" + }, + { "vdispl-list", + &main_vdispllist, 0, 0, + "List virtual display devices for a domain", + "<Domain(s)>", + }, + { "vdispl-detach", + &main_vdispldetach, 0, 1, + "Destroy a domain's virtual display device", + "<Domain> <DevId>", + }, { "uptime", &main_uptime, 0, 0, "Print uptime for all/some domains", diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index 1ef0c27..6d378ed 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -707,6 +707,25 @@ int parse_usbdev_config(libxl_device_usbdev *usbdev, char *token) return 0; } +int parse_vdispl_config(libxl_device_vdispl *vdispl, char *token) +{ + char *oparg; + + if (MATCH_OPTION("backend", token, oparg)) { + vdispl->backend_domid = find_domain(oparg); + vdispl->backend_domname = strdup(oparg); + } else if (MATCH_OPTION("devId", token, oparg)) { + vdispl->devid = atoi(oparg); + } else if (MATCH_OPTION("beAlloc", token, oparg)) { + vdispl->be_alloc = strtoul(oparg, NULL, 0); + } else { + fprintf(stderr, "Unknown string `%s' in vdispl spec\n", token); + return 1; + } + + return 0; +} + void parse_config_data(const char *config_source, const char *config_data, int config_len, @@ -716,7 +735,7 @@ void parse_config_data(const char *config_source, long l, vcpus = 0; XLU_Config *config; XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms, - *usbctrls, *usbdevs; + *usbctrls, *usbdevs, *vdispls; XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs; int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian; int pci_power_mgmt = 0; @@ -1295,6 +1314,29 @@ void parse_config_data(const char *config_source, } } + if (!xlu_cfg_get_list(config, "vdispl", &vdispls, 0, 0)) { + d_config->num_vdispls = 0; + d_config->vdispls = NULL; + while ((buf = xlu_cfg_get_listitem(vdispls, d_config->num_vdispls)) != NULL) { + libxl_device_vdispl *vdispl; + char * buf2 = strdup(buf); + char *p; + vdispl = ARRAY_EXTEND_INIT(d_config->vdispls, + d_config->num_vdispls, + libxl_device_vdispl_init); + p = strtok (buf2, ","); + while (p != NULL) + { + while (*p == ' ') p++; + if (parse_vdispl_config(vdispl, p)) { + exit(1); + } + p = strtok (NULL, ","); + } + free(buf2); + } + } + if (!xlu_cfg_get_list (config, "channel", &channels, 0, 0)) { d_config->num_channels = 0; d_config->channels = NULL; diff --git a/tools/xl/xl_parse.h b/tools/xl/xl_parse.h index db8bc3f..cc459fb 100644 --- a/tools/xl/xl_parse.h +++ b/tools/xl/xl_parse.h @@ -33,7 +33,7 @@ int parse_usbctrl_config(libxl_device_usbctrl *usbctrl, char *token); int parse_usbdev_config(libxl_device_usbdev *usbdev, char *token); int parse_cpurange(const char *cpu, libxl_bitmap *cpumap); int parse_nic_config(libxl_device_nic *nic, XLU_Config **config, char *token); - +int parse_vdispl_config(libxl_device_vdispl *vdispl, char *token); int match_option_size(const char *prefix, size_t len, char *arg, char **argopt); diff --git a/tools/xl/xl_vdispl.c b/tools/xl/xl_vdispl.c new file mode 100644 index 0000000..4df90c9 --- /dev/null +++ b/tools/xl/xl_vdispl.c @@ -0,0 +1,162 @@ +/* + * Copyright (C) 2016 EPAM Systems Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published + * by the Free Software Foundation; version 2.1 only. with the special + * exception on linking described in file LICENSE. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + */ + +#include <stdlib.h> + +#include <libxl.h> +#include <libxl_utils.h> +#include <libxlutil.h> + +#include "xl.h" +#include "xl_utils.h" +#include "xl_parse.h" + +int main_vdisplattach(int argc, char **argv) +{ + int opt; + int rc; + char *oparg; + uint32_t domid; + libxl_device_vdispl vdispl; + + SWITCH_FOREACH_OPT(opt, "", NULL, "vdispl-attach", 1) { + /* No options */ + } + + libxl_device_vdispl_init(&vdispl); + domid = find_domain(argv[optind++]); + + for (argv += optind, argc -= optind; argc > 0; ++argv, --argc) { + if (MATCH_OPTION("devId", *argv, oparg)) { + vdispl.devid = atoi(oparg); + } else if (MATCH_OPTION("backend", *argv, oparg)) { + vdispl.backend_domid = find_domain(oparg); + replace_string(&vdispl.backend_domname, oparg); + } else if (MATCH_OPTION("beAlloc", *argv, oparg)) { + vdispl.be_alloc = strtoul(oparg, NULL, 0); + } else { + fprintf(stderr, "unrecognized argument `%s'\n", *argv); + rc = 1; + goto out; + } + } + + if (dryrun_only) { + char *json = libxl_device_vdispl_to_json(ctx, &vdispl); + printf("vdispl: %s\n", json); + free(json); + goto out; + } + + if (libxl_device_vdispl_add(ctx, domid, &vdispl, 0)) { + fprintf(stderr, "libxl_device_vdispl_add failed.\n"); + rc = ERROR_FAIL; + } + + rc = 0; + +out: + libxl_device_vdispl_dispose(&vdispl); + return rc; +} + +int main_vdispllist(int argc, char **argv) +{ + int opt; + int i, n; + libxl_device_vdispl *vdispls; + libxl_vdisplinfo vdisplinfo; + + SWITCH_FOREACH_OPT(opt, "", NULL, "vdispl-list", 1) { + /* No options */ + } + + /* vdisplinfo.uuid should be outputted too */ + printf("%-5s %-3s %-6s %-5s %-8s %-40s %-40s\n", + "Vdev", "BE", "handle", "state", "be-alloc", "BE-path", "FE-path"); + for (argv += optind, argc -= optind; argc > 0; --argc, ++argv) { + uint32_t domid; + + if (libxl_domain_qualifier_to_domid(ctx, *argv, &domid) < 0) { + fprintf(stderr, "%s is an invalid domain identifier\n", *argv); + continue; + } + + vdispls = libxl_device_vdispl_list(ctx, domid, &n); + + if (!vdispls) { + continue; + } + + for (i = 0; i < n; i++) { + libxl_vdisplinfo_init(&vdisplinfo); + if (libxl_device_vdispl_getinfo(ctx, domid, &vdispls[i], + &vdisplinfo) == 0) { + /* Vdev BE hdl st be-alloc BE-path FE-path*/ + printf("%-5d %-3d %-6d %-5d %-8d %-40s %-40s\n", + vdisplinfo.devid, vdisplinfo.backend_id, + vdisplinfo.frontend_id, + vdisplinfo.state, vdisplinfo.be_alloc, + vdisplinfo.backend, vdisplinfo.frontend); + } + libxl_vdisplinfo_dispose(&vdisplinfo); + } + + libxl_device_vdispl_list_free(vdispls, n); + } + return 0; +} + +int main_vdispldetach(int argc, char **argv) +{ + uint32_t domid, devid; + int opt, rc; + libxl_device_vdispl vdispl; + + SWITCH_FOREACH_OPT(opt, "", NULL, "vdispl-detach", 2) { + /* No options */ + } + + domid = find_domain(argv[optind++]); + devid = atoi(argv[optind++]); + + libxl_device_vdispl_init(&vdispl); + + if (libxl_devid_to_device_vdispl(ctx, domid, devid, &vdispl)) { + fprintf(stderr, "Error: Device %d not connected.\n", devid); + rc = ERROR_FAIL; + goto out; + } + + rc = libxl_device_vdispl_remove(ctx, domid, &vdispl, 0); + if (rc) { + fprintf(stderr, "libxl_device_vdispl_remove failed.\n"); + rc = ERROR_FAIL; + goto out; + } + + rc = 0; + +out: + libxl_device_vdispl_dispose(&vdispl); + return rc; +} + +/* + * Local variables: + * mode: C + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] libxl: add PV display device driver interface 2017-03-23 10:10 [PATCH 0/2] libxl: add PV display device driver interface Oleksandr Grytsov 2017-03-23 10:10 ` [PATCH 1/2] " Oleksandr Grytsov 2017-03-23 10:10 ` [PATCH 2/2] xl: add PV display device commands Oleksandr Grytsov @ 2017-03-23 10:21 ` Juergen Gross 2017-03-23 11:32 ` al1img . 2 siblings, 1 reply; 11+ messages in thread From: Juergen Gross @ 2017-03-23 10:21 UTC (permalink / raw) To: Oleksandr Grytsov, xen-devel; +Cc: Oleksandr Grytsov 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] libxl: add PV display device driver interface 2017-03-23 10:21 ` [PATCH 0/2] libxl: add PV display device driver interface Juergen Gross @ 2017-03-23 11:32 ` al1img . 2017-03-23 12:08 ` Juergen Gross 0 siblings, 1 reply; 11+ messages in thread From: al1img . @ 2017-03-23 11:32 UTC (permalink / raw) To: Juergen Gross; +Cc: xen-devel, Oleksandr Grytsov 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. 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 -- Best Regards, Oleksandr Grytsov. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] libxl: add PV display device driver interface 2017-03-23 11:32 ` al1img . @ 2017-03-23 12:08 ` Juergen Gross 2017-03-23 14:23 ` al1img . 0 siblings, 1 reply; 11+ messages in thread From: Juergen Gross @ 2017-03-23 12:08 UTC (permalink / raw) To: al1img .; +Cc: xen-devel, Oleksandr Grytsov 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 > > > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] libxl: add PV display device driver interface 2017-03-23 12:08 ` Juergen Gross @ 2017-03-23 14:23 ` al1img . 2017-03-23 14:58 ` Juergen Gross 0 siblings, 1 reply; 11+ messages in thread From: al1img . @ 2017-03-23 14:23 UTC (permalink / raw) To: Juergen Gross; +Cc: xen-devel, Oleksandr Grytsov 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); } Also where should this function (libxl__device_list_free) be located? libxl_device.c? 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? 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 >> >> >> > -- Best Regards, Oleksandr Grytsov. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] libxl: add PV display device driver interface 2017-03-23 14:23 ` al1img . @ 2017-03-23 14:58 ` Juergen Gross 2017-03-23 15:55 ` al1img . 0 siblings, 1 reply; 11+ messages in thread From: Juergen Gross @ 2017-03-23 14:58 UTC (permalink / raw) To: al1img .; +Cc: xen-devel, Oleksandr Grytsov 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 >>> >>> >>> >> > > > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] libxl: add PV display device driver interface 2017-03-23 14:58 ` Juergen Gross @ 2017-03-23 15:55 ` al1img . 2017-03-24 10:35 ` Oleksandr Grytsov 0 siblings, 1 reply; 11+ messages in thread From: al1img . @ 2017-03-23 15:55 UTC (permalink / raw) To: Juergen Gross; +Cc: xen-devel, Oleksandr Grytsov 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. -- Best Regards, Oleksandr Grytsov. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] libxl: add PV display device driver interface 2017-03-23 15:55 ` al1img . @ 2017-03-24 10:35 ` Oleksandr Grytsov 2017-03-27 15:10 ` Wei Liu 0 siblings, 1 reply; 11+ messages in thread From: Oleksandr Grytsov @ 2017-03-24 10:35 UTC (permalink / raw) To: Juergen Gross; +Cc: xen-devel, Oleksandr Grytsov, Ian Jackson, Wei Liu 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] libxl: add PV display device driver interface 2017-03-24 10:35 ` Oleksandr Grytsov @ 2017-03-27 15:10 ` Wei Liu 0 siblings, 0 replies; 11+ messages in thread From: Wei Liu @ 2017-03-27 15:10 UTC (permalink / raw) To: Oleksandr Grytsov Cc: Juergen Gross, xen-devel, Oleksandr Grytsov, Ian Jackson, Wei Liu On Fri, Mar 24, 2017 at 12:35:22PM +0200, Oleksandr Grytsov wrote: > 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. I'm leaning towards #1 because we get type-safety that way. Ian? Wei. > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-03-27 15:10 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-23 10:10 [PATCH 0/2] libxl: add PV display device driver interface Oleksandr Grytsov 2017-03-23 10:10 ` [PATCH 1/2] " Oleksandr Grytsov 2017-03-23 10:10 ` [PATCH 2/2] xl: add PV display device commands Oleksandr Grytsov 2017-03-23 10:21 ` [PATCH 0/2] libxl: add PV display device driver interface Juergen Gross 2017-03-23 11:32 ` al1img . 2017-03-23 12:08 ` Juergen Gross 2017-03-23 14:23 ` al1img . 2017-03-23 14:58 ` Juergen Gross 2017-03-23 15:55 ` al1img . 2017-03-24 10:35 ` Oleksandr Grytsov 2017-03-27 15:10 ` Wei Liu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.