* [PATCH 1/3] libxl: add framework for device types
2016-06-21 14:24 [PATCH 0/3] libxl: add framework for device types Juergen Gross
@ 2016-06-21 14:24 ` Juergen Gross
2016-06-21 14:24 ` [PATCH 2/3] libxl: refactor domcreate_attach_pci() to use device type framework Juergen Gross
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2016-06-21 14:24 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, wei.liu2, ian.jackson
Instead of duplicate coding for each device type (vtpms, usbctrls, ...)
especially on domain creation introduce a framework for that purpose.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
tools/libxl/libxl.c | 12 ++++
tools/libxl/libxl_create.c | 163 +++++++++++++------------------------------
tools/libxl/libxl_internal.h | 13 ++++
tools/libxl/libxl_pvusb.c | 13 ++++
4 files changed, 87 insertions(+), 114 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 1c81239..df94f2e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -7434,6 +7434,18 @@ out:
return rc;
}
+struct libxl_device_type libxl__nic_devtype = {
+ .type = "nic",
+ .num_offset = offsetof(libxl_domain_config, num_nics),
+ .add = libxl__add_nics,
+};
+
+struct libxl_device_type libxl__vtpm_devtype = {
+ .type = "vtpm",
+ .num_offset = offsetof(libxl_domain_config, num_vtpms),
+ .add = libxl__add_vtpms,
+};
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1b99472..bb0f535 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -742,12 +742,6 @@ static void domcreate_bootloader_done(libxl__egc *egc,
static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *aodevs,
int ret);
-static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev *multidev,
- int ret);
-static void domcreate_attach_usbctrls(libxl__egc *egc,
- libxl__multidev *multidev, int ret);
-static void domcreate_attach_usbdevs(libxl__egc *egc, libxl__multidev *multidev,
- int ret);
static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *aodevs,
int ret);
static void domcreate_attach_dtdev(libxl__egc *egc,
@@ -1407,6 +1401,53 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
domcreate_complete(egc, dcs, ret);
}
+static struct libxl_device_type *device_type_tbl[] = {
+ &libxl__nic_devtype,
+ &libxl__vtpm_devtype,
+ &libxl__usbctrl_devtype,
+ &libxl__usbdev_devtype,
+};
+
+static void domcreate_attach_devices(libxl__egc *egc,
+ libxl__multidev *multidev,
+ int ret)
+{
+ libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
+ STATE_AO_GC(dcs->ao);
+ int domid = dcs->guest_domid;
+ libxl_domain_config *const d_config = dcs->guest_config;
+ struct libxl_device_type *dt;
+
+ if (ret) {
+ LOG(ERROR, "unable to add %s devices",
+ device_type_tbl[dcs->device_type_idx]->type);
+ goto error_out;
+ }
+
+ dcs->device_type_idx++;
+ if (dcs->device_type_idx < ARRAY_SIZE(device_type_tbl)) {
+ dt = device_type_tbl[dcs->device_type_idx];
+ if (*(int *)((void *)d_config + dt->num_offset) > 0) {
+ /* Attach devices */
+ libxl__multidev_begin(ao, &dcs->multidev);
+ dcs->multidev.callback = domcreate_attach_devices;
+ dt->add(egc, ao, domid, d_config, &dcs->multidev);
+ libxl__multidev_prepared(egc, &dcs->multidev, 0);
+ return;
+ }
+
+ domcreate_attach_devices(egc, &dcs->multidev, 0);
+ return;
+ }
+
+ domcreate_attach_pci(egc, multidev, 0);
+ return;
+
+error_out:
+ assert(ret);
+ domcreate_complete(egc, dcs, ret);
+}
+
static void domcreate_devmodel_started(libxl__egc *egc,
libxl__dm_spawn_state *dmss,
int ret)
@@ -1430,113 +1471,8 @@ static void domcreate_devmodel_started(libxl__egc *egc,
}
}
- /* Plug nic interfaces */
- if (d_config->num_nics > 0) {
- /* Attach nics */
- libxl__multidev_begin(ao, &dcs->multidev);
- dcs->multidev.callback = domcreate_attach_vtpms;
- libxl__add_nics(egc, ao, domid, d_config, &dcs->multidev);
- libxl__multidev_prepared(egc, &dcs->multidev, 0);
- return;
- }
-
- domcreate_attach_vtpms(egc, &dcs->multidev, 0);
- return;
-
-error_out:
- assert(ret);
- domcreate_complete(egc, dcs, ret);
-}
-
-static void domcreate_attach_vtpms(libxl__egc *egc,
- libxl__multidev *multidev,
- int ret)
-{
- libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
- STATE_AO_GC(dcs->ao);
- int domid = dcs->guest_domid;
-
- libxl_domain_config* const d_config = dcs->guest_config;
-
- if(ret) {
- LOG(ERROR, "unable to add nic devices");
- goto error_out;
- }
-
- /* Plug vtpm devices */
- if (d_config->num_vtpms > 0) {
- /* Attach vtpms */
- libxl__multidev_begin(ao, &dcs->multidev);
- dcs->multidev.callback = domcreate_attach_usbctrls;
- libxl__add_vtpms(egc, ao, domid, d_config, &dcs->multidev);
- libxl__multidev_prepared(egc, &dcs->multidev, 0);
- return;
- }
-
- domcreate_attach_usbctrls(egc, multidev, 0);
- return;
-
-error_out:
- assert(ret);
- domcreate_complete(egc, dcs, ret);
-}
-
-static void domcreate_attach_usbctrls(libxl__egc *egc,
- libxl__multidev *multidev, int ret)
-{
- libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
- STATE_AO_GC(dcs->ao);
- int domid = dcs->guest_domid;
-
- libxl_domain_config *const d_config = dcs->guest_config;
-
- if (ret) {
- LOG(ERROR, "unable to add vtpm devices");
- goto error_out;
- }
-
- if (d_config->num_usbctrls > 0) {
- /* Attach usbctrls */
- libxl__multidev_begin(ao, &dcs->multidev);
- dcs->multidev.callback = domcreate_attach_usbdevs;
- libxl__add_usbctrls(egc, ao, domid, d_config, &dcs->multidev);
- libxl__multidev_prepared(egc, &dcs->multidev, 0);
- return;
- }
-
- domcreate_attach_usbdevs(egc, multidev, 0);
- return;
-
-error_out:
- assert(ret);
- domcreate_complete(egc, dcs, ret);
-}
-
-
-static void domcreate_attach_usbdevs(libxl__egc *egc, libxl__multidev *multidev,
- int ret)
-{
- libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
- STATE_AO_GC(dcs->ao);
- int domid = dcs->guest_domid;
-
- libxl_domain_config *const d_config = dcs->guest_config;
-
- if (ret) {
- LOG(ERROR, "unable to add usbctrl devices");
- goto error_out;
- }
-
- if (d_config->num_usbdevs > 0) {
- /* Attach usbctrls */
- libxl__multidev_begin(ao, &dcs->multidev);
- dcs->multidev.callback = domcreate_attach_pci;
- libxl__add_usbdevs(egc, ao, domid, d_config, &dcs->multidev);
- libxl__multidev_prepared(egc, &dcs->multidev, 0);
- return;
- }
-
- domcreate_attach_pci(egc, multidev, 0);
+ dcs->device_type_idx = -1;
+ domcreate_attach_devices(egc, &dcs->multidev, 0);
return;
error_out:
@@ -1556,7 +1492,6 @@ static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *multidev,
libxl_domain_config *const d_config = dcs->guest_config;
if (ret) {
- LOG(ERROR, "unable to add usb devices");
goto error_out;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e7ab85d..2603b33 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3389,6 +3389,18 @@ _hidden void libxl__bootloader_init(libxl__bootloader_state *bl);
* If callback is passed rc==0, will have updated st->info appropriately */
_hidden void libxl__bootloader_run(libxl__egc*, libxl__bootloader_state *st);
+/*----- Generic Device Handling -----*/
+struct libxl_device_type {
+ char *type;
+ int num_offset; /* Offset of # of devices in libxl_domain_config */
+ void (*add)(libxl__egc *, libxl__ao *, uint32_t, libxl_domain_config *,
+ libxl__multidev *);
+};
+
+extern struct libxl_device_type libxl__nic_devtype;
+extern struct libxl_device_type libxl__vtpm_devtype;
+extern struct libxl_device_type libxl__usbctrl_devtype;
+extern struct libxl_device_type libxl__usbdev_devtype;
/*----- Domain destruction -----*/
/* Domain destruction has been split into two functions:
@@ -3565,6 +3577,7 @@ struct libxl__domain_create_state {
libxl_asyncprogress_how aop_console_how;
/* private to domain_create */
int guest_domid;
+ int device_type_idx;
const char *colo_proxy_script;
libxl__domain_build_state build_state;
libxl__colo_restore_state crs;
diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index 885f0d4..31c38b9 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -1667,6 +1667,19 @@ out:
GC_FREE;
return rc;
}
+
+struct libxl_device_type libxl__usbctrl_devtype = {
+ .type = "usbctrl",
+ .num_offset = offsetof(libxl_domain_config, num_usbctrls),
+ .add = libxl__add_usbctrls,
+};
+
+struct libxl_device_type libxl__usbdev_devtype = {
+ .type = "usbdev",
+ .num_offset = offsetof(libxl_domain_config, num_usbdevs),
+ .add = libxl__add_usbdevs,
+};
+
/*
* Local variables:
* mode: C
--
2.6.6
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] libxl: refactor domcreate_attach_pci() to use device type framework
2016-06-21 14:24 [PATCH 0/3] libxl: add framework for device types Juergen Gross
2016-06-21 14:24 ` [PATCH 1/3] " Juergen Gross
@ 2016-06-21 14:24 ` Juergen Gross
2016-06-21 14:24 ` [PATCH 3/3] libxl: refactor domcreate_attach_dtdev() " Juergen Gross
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2016-06-21 14:24 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, wei.liu2, ian.jackson
Signed-off-by: Juergen Gross <jgross@suse.com>
---
tools/libxl/libxl_create.c | 54 ++++++--------------------------------------
tools/libxl/libxl_internal.h | 1 +
tools/libxl/libxl_pci.c | 36 +++++++++++++++++++++++++++++
3 files changed, 44 insertions(+), 47 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index bb0f535..c4e85f0 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -742,10 +742,8 @@ static void domcreate_bootloader_done(libxl__egc *egc,
static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *aodevs,
int ret);
-static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *aodevs,
- int ret);
-static void domcreate_attach_dtdev(libxl__egc *egc,
- libxl__domain_create_state *dcs);
+static void domcreate_attach_dtdev(libxl__egc *egc, libxl__multidev *multidev,
+ int ret);
static void domcreate_console_available(libxl__egc *egc,
libxl__domain_create_state *dcs);
@@ -1406,6 +1404,7 @@ static struct libxl_device_type *device_type_tbl[] = {
&libxl__vtpm_devtype,
&libxl__usbctrl_devtype,
&libxl__usbdev_devtype,
+ &libxl__pci_devtype,
};
static void domcreate_attach_devices(libxl__egc *egc,
@@ -1440,7 +1439,7 @@ static void domcreate_attach_devices(libxl__egc *egc,
return;
}
- domcreate_attach_pci(egc, multidev, 0);
+ domcreate_attach_dtdev(egc, multidev, 0);
return;
error_out:
@@ -1480,52 +1479,13 @@ error_out:
domcreate_complete(egc, dcs, ret);
}
-static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *multidev,
- int ret)
-{
- libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
- STATE_AO_GC(dcs->ao);
- int i;
- int domid = dcs->guest_domid;
-
- /* convenience aliases */
- libxl_domain_config *const d_config = dcs->guest_config;
-
- if (ret) {
- goto error_out;
- }
-
- for (i = 0; i < d_config->num_pcidevs; i++) {
- ret = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
- if (ret < 0) {
- LOG(ERROR, "libxl_device_pci_add failed: %d", ret);
- goto error_out;
- }
- }
-
- if (d_config->num_pcidevs > 0) {
- ret = libxl__create_pci_backend(gc, domid, d_config->pcidevs,
- d_config->num_pcidevs);
- if (ret < 0) {
- LOG(ERROR, "libxl_create_pci_backend failed: %d", ret);
- goto error_out;
- }
- }
-
- domcreate_attach_dtdev(egc, dcs);
- return;
-
-error_out:
- assert(ret);
- domcreate_complete(egc, dcs, ret);
-}
-
static void domcreate_attach_dtdev(libxl__egc *egc,
- libxl__domain_create_state *dcs)
+ libxl__multidev *multidev,
+ int ret)
{
+ libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
STATE_AO_GC(dcs->ao);
int i;
- int ret;
int domid = dcs->guest_domid;
/* convenience aliases */
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2603b33..547a78b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3401,6 +3401,7 @@ extern struct libxl_device_type libxl__nic_devtype;
extern struct libxl_device_type libxl__vtpm_devtype;
extern struct libxl_device_type libxl__usbctrl_devtype;
extern struct libxl_device_type libxl__usbdev_devtype;
+extern struct libxl_device_type libxl__pci_devtype;
/*----- Domain destruction -----*/
/* Domain destruction has been split into two functions:
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 236bdd0..fd245ea 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1291,6 +1291,36 @@ out:
return rc;
}
+static void libxl__add_pcis(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
+ libxl_domain_config *d_config,
+ libxl__multidev *multidev)
+{
+ AO_GC;
+ libxl__ao_device *aodev = libxl__multidev_prepare(multidev);
+ int i, rc = 0;
+
+ for (i = 0; i < d_config->num_pcidevs; i++) {
+ rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
+ if (rc < 0) {
+ LOG(ERROR, "libxl_device_pci_add failed: %d", rc);
+ goto out;
+ }
+ }
+
+ if (d_config->num_pcidevs > 0) {
+ rc = libxl__create_pci_backend(gc, domid, d_config->pcidevs,
+ d_config->num_pcidevs);
+ if (rc < 0) {
+ LOG(ERROR, "libxl_create_pci_backend failed: %d", rc);
+ goto out;
+ }
+ }
+
+out:
+ aodev->rc = rc;
+ aodev->callback(egc, aodev);
+}
+
static int qemu_pci_remove_xenstore(libxl__gc *gc, uint32_t domid,
libxl_device_pci *pcidev, int force)
{
@@ -1668,6 +1698,12 @@ int libxl__grant_vga_iomem_permission(libxl__gc *gc, const uint32_t domid,
return 0;
}
+struct libxl_device_type libxl__pci_devtype = {
+ .type = "pci",
+ .num_offset = offsetof(libxl_domain_config, num_pcidevs),
+ .add = libxl__add_pcis,
+};
+
/*
* Local variables:
* mode: C
--
2.6.6
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] libxl: refactor domcreate_attach_dtdev() to use device type framework
2016-06-21 14:24 [PATCH 0/3] libxl: add framework for device types Juergen Gross
2016-06-21 14:24 ` [PATCH 1/3] " Juergen Gross
2016-06-21 14:24 ` [PATCH 2/3] libxl: refactor domcreate_attach_pci() to use device type framework Juergen Gross
@ 2016-06-21 14:24 ` Juergen Gross
2016-07-04 10:26 ` [PATCH 0/3] libxl: add framework for device types Juergen Gross
2016-07-06 11:04 ` Ian Jackson
4 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2016-06-21 14:24 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, wei.liu2, ian.jackson
Signed-off-by: Juergen Gross <jgross@suse.com>
---
tools/libxl/libxl_create.c | 72 ++++++++++++++++++++++------------------------
1 file changed, 35 insertions(+), 37 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index c4e85f0..e93b880 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -742,9 +742,6 @@ static void domcreate_bootloader_done(libxl__egc *egc,
static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *aodevs,
int ret);
-static void domcreate_attach_dtdev(libxl__egc *egc, libxl__multidev *multidev,
- int ret);
-
static void domcreate_console_available(libxl__egc *egc,
libxl__domain_create_state *dcs);
@@ -1399,12 +1396,43 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
domcreate_complete(egc, dcs, ret);
}
+static void libxl__add_dtdevs(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
+ libxl_domain_config *d_config,
+ libxl__multidev *multidev)
+{
+ AO_GC;
+ libxl__ao_device *aodev = libxl__multidev_prepare(multidev);
+ int i, rc = 0;
+
+ for (i = 0; i < d_config->num_dtdevs; i++) {
+ const libxl_device_dtdev *dtdev = &d_config->dtdevs[i];
+
+ LOG(DEBUG, "Assign device \"%s\" to dom%u", dtdev->path, domid);
+ rc = xc_assign_dt_device(CTX->xch, domid, dtdev->path);
+ if (rc < 0) {
+ LOG(ERROR, "xc_assign_dtdevice failed: %d", rc);
+ goto out;
+ }
+ }
+
+out:
+ aodev->rc = rc;
+ aodev->callback(egc, aodev);
+}
+
+static struct libxl_device_type libxl__dtdev_devtype = {
+ .type = "dtdev",
+ .num_offset = offsetof(libxl_domain_config, num_dtdevs),
+ .add = libxl__add_dtdevs,
+};
+
static struct libxl_device_type *device_type_tbl[] = {
&libxl__nic_devtype,
&libxl__vtpm_devtype,
&libxl__usbctrl_devtype,
&libxl__usbdev_devtype,
&libxl__pci_devtype,
+ &libxl__dtdev_devtype,
};
static void domcreate_attach_devices(libxl__egc *egc,
@@ -1439,7 +1467,10 @@ static void domcreate_attach_devices(libxl__egc *egc,
return;
}
- domcreate_attach_dtdev(egc, multidev, 0);
+ domcreate_console_available(egc, dcs);
+
+ domcreate_complete(egc, dcs, 0);
+
return;
error_out:
@@ -1479,39 +1510,6 @@ error_out:
domcreate_complete(egc, dcs, ret);
}
-static void domcreate_attach_dtdev(libxl__egc *egc,
- libxl__multidev *multidev,
- int ret)
-{
- libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
- STATE_AO_GC(dcs->ao);
- int i;
- int domid = dcs->guest_domid;
-
- /* convenience aliases */
- libxl_domain_config *const d_config = dcs->guest_config;
-
- for (i = 0; i < d_config->num_dtdevs; i++) {
- const libxl_device_dtdev *dtdev = &d_config->dtdevs[i];
-
- LOG(DEBUG, "Assign device \"%s\" to dom%u", dtdev->path, domid);
- ret = xc_assign_dt_device(CTX->xch, domid, dtdev->path);
- if (ret < 0) {
- LOG(ERROR, "xc_assign_dtdevice failed: %d", ret);
- goto error_out;
- }
- }
-
- domcreate_console_available(egc, dcs);
-
- domcreate_complete(egc, dcs, 0);
- return;
-
-error_out:
- assert(ret);
- domcreate_complete(egc, dcs, ret);
-}
-
static void domcreate_complete(libxl__egc *egc,
libxl__domain_create_state *dcs,
int rc)
--
2.6.6
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] libxl: add framework for device types
2016-06-21 14:24 [PATCH 0/3] libxl: add framework for device types Juergen Gross
` (2 preceding siblings ...)
2016-06-21 14:24 ` [PATCH 3/3] libxl: refactor domcreate_attach_dtdev() " Juergen Gross
@ 2016-07-04 10:26 ` Juergen Gross
2016-07-06 11:04 ` Ian Jackson
4 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2016-07-04 10:26 UTC (permalink / raw)
To: xen-devel; +Cc: ian.jackson, wei.liu2
On 21/06/16 16:24, Juergen Gross wrote:
> Instead of duplicate coding for each device type (vtpms, usbctrls, ...)
> especially on domain creation introduce a framework for that purpose.
>
> I especially found it annoying that e.g. the vtpm callback issued the
> error message for a failed attach of nic devices.
>
> Juergen Gross (3):
> libxl: add framework for device types
> libxl: refactor domcreate_attach_pci() to use device type framework
> libxl: refactor domcreate_attach_dtdev() to use device type framework
>
> tools/libxl/libxl.c | 12 ++
> tools/libxl/libxl_create.c | 275 +++++++++++++------------------------------
> tools/libxl/libxl_internal.h | 14 +++
> tools/libxl/libxl_pci.c | 36 ++++++
> tools/libxl/libxl_pvusb.c | 13 ++
> 5 files changed, 159 insertions(+), 191 deletions(-)
>
Ping?
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] libxl: add framework for device types
2016-06-21 14:24 [PATCH 0/3] libxl: add framework for device types Juergen Gross
` (3 preceding siblings ...)
2016-07-04 10:26 ` [PATCH 0/3] libxl: add framework for device types Juergen Gross
@ 2016-07-06 11:04 ` Ian Jackson
2016-07-06 12:40 ` Juergen Gross
4 siblings, 1 reply; 10+ messages in thread
From: Ian Jackson @ 2016-07-06 11:04 UTC (permalink / raw)
To: Juergen Gross; +Cc: wei.liu2, xen-devel
Juergen Gross writes ("[PATCH 0/3] libxl: add framework for device types"):
> Instead of duplicate coding for each device type (vtpms, usbctrls, ...)
> especially on domain creation introduce a framework for that purpose.
I think something like this is a jolly good idea. Thanks a lot!
The rough shape - a set of structs with what amount to method calls -
seems a good direction to be going in.
I have some comments/questions.
I saw this in 2/3:
> + for (i = 0; i < d_config->num_pcidevs; i++) {
> + rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
> + if (rc < 0) {
> + LOG(ERROR, "libxl_device_pci_add failed: %d", rc);
> + goto out;
> + }
> + }
> +
And there is similar code in 3/3 for dtdevs. Could that be lifted
away somehow ? (You'd have to take some care about the types, sadly;
ie, I think libxl__device_pci_add might have to start to take a
void*; maybe some macros could make things typesafe?)
In 1/3:
> +struct libxl_device_type libxl__usbctrl_devtype = {
> + .type = "usbctrl",
> + .num_offset = offsetof(libxl_domain_config, num_usbctrls),
> + .add = libxl__add_usbctrls,
> +};
And then num_offset is used like this:
> + if (*(int *)((void *)d_config + dt->num_offset) > 0) {
This is a fine approach but I would prefer it if the there were a bit
more type safety, particularly in the parts that have to occur once
for each device type.
For example, there is nothing stopping one using this pattern with a
num_* field which is not an int.
Perhaps there should be a macro for generating the libxl_device_type
contents ? It could perhaps take `usbctrls' and make `num_usbctrls'
out of it using token pasting.
Also these structs should be static const, I think.
Thanks again!
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] libxl: add framework for device types
2016-07-06 11:04 ` Ian Jackson
@ 2016-07-06 12:40 ` Juergen Gross
2016-07-06 12:47 ` Ian Jackson
0 siblings, 1 reply; 10+ messages in thread
From: Juergen Gross @ 2016-07-06 12:40 UTC (permalink / raw)
To: Ian Jackson; +Cc: wei.liu2, xen-devel
On 06/07/16 13:04, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH 0/3] libxl: add framework for device types"):
>> Instead of duplicate coding for each device type (vtpms, usbctrls, ...)
>> especially on domain creation introduce a framework for that purpose.
>
> I think something like this is a jolly good idea. Thanks a lot!
>
> The rough shape - a set of structs with what amount to method calls -
> seems a good direction to be going in.
>
> I have some comments/questions.
>
> I saw this in 2/3:
>
>> + for (i = 0; i < d_config->num_pcidevs; i++) {
>> + rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
>> + if (rc < 0) {
>> + LOG(ERROR, "libxl_device_pci_add failed: %d", rc);
>> + goto out;
>> + }
>> + }
>> +
>
> And there is similar code in 3/3 for dtdevs. Could that be lifted
> away somehow ? (You'd have to take some care about the types, sadly;
> ie, I think libxl__device_pci_add might have to start to take a
> void*; maybe some macros could make things typesafe?)
I thought about this idea already. I think we would end up with more
code which would be rather unpleasant to read. Main reason is the
need for a dtdev wrapper function and the pci backend creation.
> In 1/3:
>
>> +struct libxl_device_type libxl__usbctrl_devtype = {
>> + .type = "usbctrl",
>> + .num_offset = offsetof(libxl_domain_config, num_usbctrls),
>> + .add = libxl__add_usbctrls,
>> +};
>
> And then num_offset is used like this:
>
>> + if (*(int *)((void *)d_config + dt->num_offset) > 0) {
>
> This is a fine approach but I would prefer it if the there were a bit
> more type safety, particularly in the parts that have to occur once
> for each device type.
>
> For example, there is nothing stopping one using this pattern with a
> num_* field which is not an int.
>
> Perhaps there should be a macro for generating the libxl_device_type
> contents ? It could perhaps take `usbctrls' and make `num_usbctrls'
> out of it using token pasting.
I think 'usbctrl' as the macro parameter would be just fine: it would
allow to generate the complete struct:
#define DEFINE_DEVICE_STRUCT(name) \
const libxl_device_type libxl__ ## name ## _devtype = { \
.type = # name , \
.num_offset = offsetof(libxl_domain_config, num_ ## name ## s), \
.add = libxl__add_ ## name ## s, \
}
DEFINE_DEVICE_STRUCT(usbctrl);
> Also these structs should be static const, I think.
static: sometimes, const: yes.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] libxl: add framework for device types
2016-07-06 12:40 ` Juergen Gross
@ 2016-07-06 12:47 ` Ian Jackson
2016-07-06 12:55 ` Juergen Gross
0 siblings, 1 reply; 10+ messages in thread
From: Ian Jackson @ 2016-07-06 12:47 UTC (permalink / raw)
To: Juergen Gross; +Cc: wei.liu2, xen-devel
Juergen Gross writes ("Re: [PATCH 0/3] libxl: add framework for device types"):
> On 06/07/16 13:04, Ian Jackson wrote:
> >> + for (i = 0; i < d_config->num_pcidevs; i++) {
> >> + rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
> >> + if (rc < 0) {
> >> + LOG(ERROR, "libxl_device_pci_add failed: %d", rc);
> >> + goto out;
> >> + }
> >> + }
> >> +
> >
> > And there is similar code in 3/3 for dtdevs. Could that be lifted
> > away somehow ? (You'd have to take some care about the types, sadly;
> > ie, I think libxl__device_pci_add might have to start to take a
> > void*; maybe some macros could make things typesafe?)
>
> I thought about this idea already. I think we would end up with more
> code which would be rather unpleasant to read. Main reason is the
> need for a dtdev wrapper function and the pci backend creation.
I'm not sure what you mean by dtdev wrapper function.
As for pci backend, there could be a separate hook for "after adding
all devices of this type".
But if you don't think this is feasible I won't insist on it. The
approach you have is already a big improvement.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] libxl: add framework for device types
2016-07-06 12:47 ` Ian Jackson
@ 2016-07-06 12:55 ` Juergen Gross
2016-07-06 16:09 ` Ian Jackson
0 siblings, 1 reply; 10+ messages in thread
From: Juergen Gross @ 2016-07-06 12:55 UTC (permalink / raw)
To: Ian Jackson; +Cc: wei.liu2, xen-devel
On 06/07/16 14:47, Ian Jackson wrote:
> Juergen Gross writes ("Re: [PATCH 0/3] libxl: add framework for device types"):
>> On 06/07/16 13:04, Ian Jackson wrote:
>>>> + for (i = 0; i < d_config->num_pcidevs; i++) {
>>>> + rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
>>>> + if (rc < 0) {
>>>> + LOG(ERROR, "libxl_device_pci_add failed: %d", rc);
>>>> + goto out;
>>>> + }
>>>> + }
>>>> +
>>>
>>> And there is similar code in 3/3 for dtdevs. Could that be lifted
>>> away somehow ? (You'd have to take some care about the types, sadly;
>>> ie, I think libxl__device_pci_add might have to start to take a
>>> void*; maybe some macros could make things typesafe?)
>>
>> I thought about this idea already. I think we would end up with more
>> code which would be rather unpleasant to read. Main reason is the
>> need for a dtdev wrapper function and the pci backend creation.
>
> I'm not sure what you mean by dtdev wrapper function.
The loop for dtdev calls a xc_ function with different parameters than
the one for pci. We'd need a libxl__device_dtdev_add() wrapper function
to do the xc_ call.
> As for pci backend, there could be a separate hook for "after adding
> all devices of this type".
Right. And summing up the additional hooks and queries whether they
are specified sums up to more overhead than the current version.
> But if you don't think this is feasible I won't insist on it. The
> approach you have is already a big improvement.
Thanks.
I'm planning to add more to it (e.g. I'd like to get rid of the
MERGE() macro in libxl_retrieve_domain_configuration() in favor of a
device type hook in order to be able to have _all_ stuff for one type
in one source file.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] libxl: add framework for device types
2016-07-06 12:55 ` Juergen Gross
@ 2016-07-06 16:09 ` Ian Jackson
0 siblings, 0 replies; 10+ messages in thread
From: Ian Jackson @ 2016-07-06 16:09 UTC (permalink / raw)
To: Juergen Gross; +Cc: wei.liu2, xen-devel
Juergen Gross writes ("Re: [PATCH 0/3] libxl: add framework for device types"):
> On 06/07/16 14:47, Ian Jackson wrote:
> > Juergen Gross writes ("Re: [PATCH 0/3] libxl: add framework for device types"):
> >> On 06/07/16 13:04, Ian Jackson wrote:
> >>>> + for (i = 0; i < d_config->num_pcidevs; i++) {
> >>>> + rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
> >>>> + if (rc < 0) {
> >>>> + LOG(ERROR, "libxl_device_pci_add failed: %d", rc);
> >>>> + goto out;
> >>>> + }
> >>>> + }
> >>>> +
> >>>
> >>> And there is similar code in 3/3 for dtdevs. Could that be lifted
> >>> away somehow ? (You'd have to take some care about the types, sadly;
> >>> ie, I think libxl__device_pci_add might have to start to take a
> >>> void*; maybe some macros could make things typesafe?)
> >>
> >> I thought about this idea already. I think we would end up with more
> >> code which would be rather unpleasant to read. Main reason is the
> >> need for a dtdev wrapper function and the pci backend creation.
> >
> > I'm not sure what you mean by dtdev wrapper function.
>
> The loop for dtdev calls a xc_ function with different parameters than
> the one for pci. We'd need a libxl__device_dtdev_add() wrapper function
> to do the xc_ call.
Oh, I see.
Sorry. I was just suggesting that the iteration over the devices, and
the error handling, could be lifted out. Not the contents of
libxl__device_FOO_add.
And yes, we'd need libxl__device_dtdev_add.
>
> > But if you don't think this is feasible I won't insist on it. The
> > approach you have is already a big improvement.
>
> Thanks.
>
> I'm planning to add more to it (e.g. I'd like to get rid of the
> MERGE() macro in libxl_retrieve_domain_configuration() in favor of a
> device type hook in order to be able to have _all_ stuff for one type
> in one source file.
Right. That would be brilliant.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread