Re: [Xen-devel] [PATCH v2 1/3] libxl: add PV display device driver interface
On Tue, Jun 20, 2017 at 4:54 PM, Wei Liu wrote:
> On Thu, May 25, 2017 at 03:17:29PM +0300, Oleksandr Grytsov wrote:
>> From: Oleksandr Grytsov
>>
Hi Wei,
Thank you for your reply.
> I'm sorry, patch like this is impossible to review because: 1. there is
> no commit message 2. it is huge.
I will separate it to small ones and will add commit message.
> I can see it is adding a lot of hooks to the device handling framework.
> Please explain why they are needed. This sort of changes (refactoring
> and extending existing code) should also be in separate patches.
Hooks in the device handling framework is needed to avoid code duplication
on new PV device adding. There were two possibilities either use macro
or extend the device handling framework. See [1] and following conversation
for more details. The patches don't refactor existing code they extend
hooks to the device handling framework and add new functionality required
to add the display device driver.
Almost all libxl__device__add functions are the same except Xen store
parameters. So, I've moved setting Xen store parameters to
set_xenstore_config hook
and have created common function to add device (add hook).
Also I've created functions to libxl_device__list and
libxl_device__list_free.
which take device type as parameter.
>
>> Signed-off-by: Oleksandr Grytsov
>> ---
>> tools/libxl/Makefile | 2 +-
>> tools/libxl/libxl.h | 21 ++
>> tools/libxl/libxl_create.c | 3 +
>> tools/libxl/libxl_device.c | 178 -
>> tools/libxl/libxl_internal.h | 24 +++
>> tools/libxl/libxl_types.idl | 40 +++-
>> tools/libxl/libxl_types_internal.idl | 1 +
>> tools/libxl/libxl_usb.c | 2 +
>> tools/libxl/libxl_utils.h| 4 +
>> tools/libxl/libxl_vdispl.c | 372
>> +++
>> 10 files changed, 643 insertions(+), 4 deletions(-)
>> create mode 100644 tools/libxl/libxl_vdispl.c
>> };
>>
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index 5e96676..2954800 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -18,7 +18,7 @@
>>
>> #include "libxl_internal.h"
>>
>> -static char *libxl__device_frontend_path(libxl__gc *gc, libxl__device
>> *device)
>> +char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device)
>> {
>> char *dom_path = libxl__xs_get_dompath(gc, device->domid);
>>
>> @@ -1776,6 +1776,182 @@ out:
>> return AO_CREATE_FAIL(rc);
>> }
>>
>> +static int device_add_domain_config(libxl__gc *gc, uint32_t domid,
>> +const struct libxl_device_type *dt,
>> +void *type)
> [...]
>> +
>> +void libxl__device_add(libxl__egc *egc, uint32_t domid,
>> + const struct libxl_device_type *dt, void *type,
>> + libxl__ao_device *aodev)
> [...]
>> +
>> +void* libxl__device_list(const struct libxl_device_type *dt,
>> + libxl_ctx *ctx, uint32_t domid, int *num)
> [...]
>> +
>> +void libxl__device_list_free(const struct libxl_device_type *dt,
>> + void *list, int num)
>>
>
> I think existing code already provides these functionalities, right?
Right, but as I mentioned before there are almost same functions for
each device. These new functions are generic.
[1] http://marc.info/?l=xen-devel&m=149026463411873&w=2
--
Best Regards,
Oleksandr Grytsov.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 1/3] libxl: add PV display device driver interface
On Thu, May 25, 2017 at 03:17:29PM +0300, Oleksandr Grytsov wrote:
> From: Oleksandr Grytsov
>
I'm sorry, patch like this is impossible to review because: 1. there is
no commit message 2. it is huge.
I can see it is adding a lot of hooks to the device handling framework.
Please explain why they are needed. This sort of changes (refactoring
and extending existing code) should also be in separate patches.
> Signed-off-by: Oleksandr Grytsov
> ---
> tools/libxl/Makefile | 2 +-
> tools/libxl/libxl.h | 21 ++
> tools/libxl/libxl_create.c | 3 +
> tools/libxl/libxl_device.c | 178 -
> tools/libxl/libxl_internal.h | 24 +++
> tools/libxl/libxl_types.idl | 40 +++-
> tools/libxl/libxl_types_internal.idl | 1 +
> tools/libxl/libxl_usb.c | 2 +
> tools/libxl/libxl_utils.h| 4 +
> tools/libxl/libxl_vdispl.c | 372
> +++
> 10 files changed, 643 insertions(+), 4 deletions(-)
> create mode 100644 tools/libxl/libxl_vdispl.c
> };
>
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 5e96676..2954800 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -18,7 +18,7 @@
>
> #include "libxl_internal.h"
>
> -static char *libxl__device_frontend_path(libxl__gc *gc, libxl__device
> *device)
> +char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device)
> {
> char *dom_path = libxl__xs_get_dompath(gc, device->domid);
>
> @@ -1776,6 +1776,182 @@ out:
> return AO_CREATE_FAIL(rc);
> }
>
> +static int device_add_domain_config(libxl__gc *gc, uint32_t domid,
> +const struct libxl_device_type *dt,
> +void *type)
[...]
> +
> +void libxl__device_add(libxl__egc *egc, uint32_t domid,
> + const struct libxl_device_type *dt, void *type,
> + libxl__ao_device *aodev)
[...]
> +
> +void* libxl__device_list(const struct libxl_device_type *dt,
> + libxl_ctx *ctx, uint32_t domid, int *num)
[...]
> +
> +void libxl__device_list_free(const struct libxl_device_type *dt,
> + void *list, int num)
>
I think existing code already provides these functionalities, right?
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 1/3] libxl: add PV display device driver interface
From: Oleksandr Grytsov
Signed-off-by: Oleksandr Grytsov
---
tools/libxl/Makefile | 2 +-
tools/libxl/libxl.h | 21 ++
tools/libxl/libxl_create.c | 3 +
tools/libxl/libxl_device.c | 178 -
tools/libxl/libxl_internal.h | 24 +++
tools/libxl/libxl_types.idl | 40 +++-
tools/libxl/libxl_types_internal.idl | 1 +
tools/libxl/libxl_usb.c | 2 +
tools/libxl/libxl_utils.h| 4 +
tools/libxl/libxl_vdispl.c | 372 +++
10 files changed, 643 insertions(+), 4 deletions(-)
create mode 100644 tools/libxl/libxl_vdispl.c
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 566b706..3a932e1 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 72ec39d..4eff121 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 25389e1..b36383f 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1422,6 +1422,8 @@ out:
#define libxl_device_dtdev_list NULL
#define libxl_device_dtdev_compare NULL
+#define libxl__device_from_dtdev NULL
+#define libxl__device_dtdev_setdefault NULL
static DEFINE_DEVICE_TYPE_STRUCT(dtdev);
const struct libxl_device_type *device_type_tbl[] = {
@@ -1432,6 +1434,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_device.c b/tools/libxl/libxl_device.c
index 5e96676..2954800 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -18,7 +18,7 @@
#include "libxl_internal.h"
-static char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device)
+char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device)
{
char *dom_path = libxl__xs_get_dompath(gc, device->domid);
@@ -1776,6 +1776,182 @@ out:
return AO_CREATE_FAIL(rc);
}
+static int device_add_domain_config(libxl__gc *gc, uint32_t domid,
+const struct libxl_device_type *dt,
+void *type)
+{
+int rc;
+libxl_domain_config d_config;
+libxl__domain_userdata_lock *lock = NULL;
+int *num_dev;
+int i;
+void *item = NULL;
+
+libxl_domain_config_init(&d_config);
+
+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;
+
+num_dev = libxl__device_type_get_num(dt, &d_config);
+
+/* Check for existing device */
+for (i = 0; i < *num_d
