[PATCH v2 0/4] libxl: fix CDROM issues

[PATCH v2 0/4] libxl: fix CDROM issues

[Roger Pau Monne]

Due to recent changes (and intended fixes) CDROM has become quite unstable
recently. This series aims to fix the issues reported with having empty
CDROM devices in the HVM guest configuration files. It also contains some
cleanups of more generic code in order to ease the implementation.

After some talk on IRC, the easiest solution at this point in the release is
to force PV CDROM backends of HVM guests with a device model to use Qdisk,
since the cd-{insert/eject} implementation is only able to deal with this
kind of PV backend.

Roger.


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

[PATCH v2 1/4] libxl: set the device model version earlier in xenstore

[Roger Pau Monne]

So libxl doesn't have to pass the build info around just to get the device
model used by the guest. This allows to simplify
libxl__device_nic_setdefault.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c          | 24 +++++++-----------------
 tools/libxl/libxl_create.c   |  9 +++++++--
 tools/libxl/libxl_dm.c       |  3 +--
 tools/libxl/libxl_internal.h |  3 +--
 4 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 5cdc09e..d35fc33 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3292,7 +3292,7 @@ out:
 /******************************************************************************/
 
 int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic,
-                                 uint32_t domid, libxl_domain_build_info *info)
+                                 uint32_t domid)
 {
     int rc;
 
@@ -3330,21 +3330,11 @@ int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic,
     switch (libxl__domain_type(gc, domid)) {
     case LIBXL_DOMAIN_TYPE_HVM:
         if (!nic->nictype) {
-            if (info != NULL) {
-                /* Path taken at creation time. */
-                if (info->device_model_version ==
-                    LIBXL_DEVICE_MODEL_VERSION_NONE)
-                    nic->nictype = LIBXL_NIC_TYPE_VIF;
-                else
-                    nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
-            } else {
-                /* Path taken when hot-adding a nic. */
-                if (libxl__device_model_version_running(gc, domid) ==
-                    LIBXL_DEVICE_MODEL_VERSION_NONE)
-                    nic->nictype = LIBXL_NIC_TYPE_VIF;
-                else
-                    nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
-            }
+            if (libxl__device_model_version_running(gc, domid) ==
+                LIBXL_DEVICE_MODEL_VERSION_NONE)
+                nic->nictype = LIBXL_NIC_TYPE_VIF;
+            else
+                nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
         }
         break;
     case LIBXL_DOMAIN_TYPE_PV:
@@ -3394,7 +3384,7 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
     libxl_device_nic_init(&nic_saved);
     libxl_device_nic_copy(CTX, &nic_saved, nic);
 
-    rc = libxl__device_nic_setdefault(gc, nic, domid, NULL);
+    rc = libxl__device_nic_setdefault(gc, nic, domid);
     if (rc) goto out;
 
     front = flexarray_make(gc, 16, 1);
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 4b02de9..24f168b 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -916,6 +916,12 @@ static void initiate_domain_create(libxl__egc *egc,
         goto error_out;
     }
 
+    /*
+     * Set the dm version quite early so that libxl doesn't have to pass the
+     * build info around just to know if the domain has a device model or not.
+     */
+    store_libxl_entry(gc, domid, &d_config->b_info);
+
     for (i = 0; i < d_config->num_disks; i++) {
         ret = libxl__device_disk_setdefault(gc, &d_config->disks[i]);
         if (ret) {
@@ -940,8 +946,7 @@ static void initiate_domain_create(libxl__egc *egc,
          * called libxl_device_nic_add when domcreate_launch_dm gets called,
          * but qemu needs the nic information to be complete.
          */
-        ret = libxl__device_nic_setdefault(gc, &d_config->nics[i], domid,
-                                           &d_config->b_info);
+        ret = libxl__device_nic_setdefault(gc, &d_config->nics[i], domid);
         if (ret) {
             LOG(ERROR, "Unable to set nic defaults for nic %d", i);
             goto error_out;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index eac5501..a12c90d 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1810,8 +1810,7 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
          * called libxl_device_nic_add at this point, but qemu needs
          * the nic information to be complete.
          */
-        ret = libxl__device_nic_setdefault(gc, &dm_config->nics[i], dm_domid,
-                                           &dm_config->b_info);
+        ret = libxl__device_nic_setdefault(gc, &dm_config->nics[i], dm_domid);
         if (ret)
             goto out;
     }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 653c152..55896f8 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1206,8 +1206,7 @@ _hidden int libxl__domain_build_info_setdefault(libxl__gc *gc,
 _hidden int libxl__device_disk_setdefault(libxl__gc *gc,
                                           libxl_device_disk *disk);
 _hidden int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic,
-                                         uint32_t domid,
-                                         libxl_domain_build_info *info);
+                                         uint32_t domid);
 _hidden int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm);
 _hidden int libxl__device_vfb_setdefault(libxl__gc *gc, libxl_device_vfb *vfb);
 _hidden int libxl__device_vkb_setdefault(libxl__gc *gc, libxl_device_vkb *vkb);
-- 
2.6.4 (Apple Git-63)


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

[PATCH v2 2/4] libxl: set the backend type to Qdisk for CDROM devices on DM HVM guests

[Roger Pau Monne]

This is needed because the cd-{insert/eject} functions are not prepared to
deal with blkback, which would be used by default if no backend was
specified.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c          | 24 +++++++++++++++++++-----
 tools/libxl/libxl_create.c   |  2 +-
 tools/libxl/libxl_internal.h |  3 ++-
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d35fc33..9d785a4 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2301,7 +2301,8 @@ int libxl_devid_to_device_vtpm(libxl_ctx *ctx,
 
 /******************************************************************************/
 
-int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk)
+int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk,
+                                  uint32_t domid)
 {
     int rc;
 
@@ -2312,6 +2313,19 @@ int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk)
     rc = libxl__resolve_domid(gc, disk->backend_domname, &disk->backend_domid);
     if (rc < 0) return rc;
 
+    /* Force Qdisk backend for CDROM devices of guests with a device model. */
+    if (disk->is_cdrom != 0 &&
+        libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM &&
+        libxl__device_model_version_running(gc, domid) !=
+        LIBXL_DEVICE_MODEL_VERSION_NONE) {
+        if (!(disk->backend == LIBXL_DISK_BACKEND_QDISK ||
+              disk->backend == LIBXL_DISK_BACKEND_UNKNOWN)) {
+            LOG(ERROR, "Backend for CD devices on HVM guests must be Qdisk");
+            return ERROR_FAIL;
+        }
+        disk->backend = LIBXL_DISK_BACKEND_QDISK;
+    }
+
     rc = libxl__device_disk_set_backend(gc, disk);
     return rc;
 }
@@ -2427,7 +2441,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
             }
         }
 
-        rc = libxl__device_disk_setdefault(gc, disk);
+        rc = libxl__device_disk_setdefault(gc, disk, domid);
         if (rc) goto out;
 
         front = flexarray_make(gc, 16, 1);
@@ -2869,7 +2883,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     disk_empty.vdev = libxl__strdup(NOGC, disk->vdev);
     disk_empty.pdev_path = libxl__strdup(NOGC, "");
     disk_empty.is_cdrom = 1;
-    libxl__device_disk_setdefault(gc, &disk_empty);
+    libxl__device_disk_setdefault(gc, &disk_empty, domid);
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
     if (type == LIBXL_DOMAIN_TYPE_INVALID) {
@@ -2910,7 +2924,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         goto out;
     }
 
-    rc = libxl__device_disk_setdefault(gc, disk);
+    rc = libxl__device_disk_setdefault(gc, disk, domid);
     if (rc) goto out;
 
     if (!disk->pdev_path) {
@@ -3173,7 +3187,7 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
             disk->script = libxl__strdup(gc, in_disk->script);
         disk->vdev = NULL;
         
-        rc = libxl__device_disk_setdefault(gc, disk);
+        rc = libxl__device_disk_setdefault(gc, disk, LIBXL_TOOLSTACK_DOMID);
         if (rc) goto out;
 
         libxl__prepare_ao_device(ao, &dls->aodev);
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 24f168b..1a57907 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -923,7 +923,7 @@ static void initiate_domain_create(libxl__egc *egc,
     store_libxl_entry(gc, domid, &d_config->b_info);
 
     for (i = 0; i < d_config->num_disks; i++) {
-        ret = libxl__device_disk_setdefault(gc, &d_config->disks[i]);
+        ret = libxl__device_disk_setdefault(gc, &d_config->disks[i], domid);
         if (ret) {
             LOG(ERROR, "Unable to set disk defaults for disk %d", i);
             goto error_out;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 55896f8..47aca76 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1204,7 +1204,8 @@ _hidden int libxl__domain_create_info_setdefault(libxl__gc *gc,
 _hidden int libxl__domain_build_info_setdefault(libxl__gc *gc,
                                         libxl_domain_build_info *b_info);
 _hidden int libxl__device_disk_setdefault(libxl__gc *gc,
-                                          libxl_device_disk *disk);
+                                          libxl_device_disk *disk,
+                                          uint32_t domid);
 _hidden int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic,
                                          uint32_t domid);
 _hidden int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm);
-- 
2.6.4 (Apple Git-63)


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

[PATCH v2 3/4] libxl: only allow guests with a device model to use cd-{eject/insert}

[Roger Pau Monne]

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes since v1:
 - Remove HVM from the error message.
---
 tools/libxl/libxl.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9d785a4..e6c3d11 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2909,6 +2909,12 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         goto out;
     }
 
+    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_NONE) {
+        LOG(ERROR, "Guests without a device model cannot use cd-insert");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
     disks = libxl_device_disk_list(ctx, domid, &num);
     for (i = 0; i < num; i++) {
         if (disks[i].is_cdrom && !strcmp(disk->vdev, disks[i].vdev))
-- 
2.6.4 (Apple Git-63)


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

[PATCH v2 4/4] libxl: remove code added to use the 'phy' backend with CDROM devices

[Roger Pau Monne]

This is a partial revert of 612f15, that allowed CDROM devices to use the
'phy' PV backend. Due to limitations in the current implementation of the
libxl_cdrom_insert function, the PV backend used in conjunction with an
emulated CDROM device must always be Qdisk at the moment. This is due to
libxl_cdrom_insert not running disk hotplug scripts on plug and unplug of PV
CDROM backends (and possibly other yet to be identified issues).

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes since v1:
 - Added a proper commit message.
---
 tools/libxl/libxl_device.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index ce53520..82405a7 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -191,20 +191,13 @@ static int disk_try_backend(disk_try_backend_args *a,
 
     switch (backend) {
     case LIBXL_DISK_BACKEND_PHY:
-        if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW ||
-              a->disk->format == LIBXL_DISK_FORMAT_EMPTY)) {
+        if (a->disk->format != LIBXL_DISK_FORMAT_RAW) {
             goto bad_format;
         }
 
         if (libxl_defbool_val(a->disk->colo_enable))
             goto bad_colo;
 
-        if (a->disk->format == LIBXL_DISK_FORMAT_EMPTY) {
-            LOG(DEBUG, "Disk vdev=%s is empty, skipping physical device check",
-                a->disk->vdev);
-            return backend;
-        }
-
         if (a->disk->backend_domid != LIBXL_TOOLSTACK_DOMID) {
             LOG(DEBUG, "Disk vdev=%s, is using a storage driver domain, "
                        "skipping physical device check", a->disk->vdev);
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH v2 0/4] libxl: fix CDROM issues

[George Dunlap]

BTW, I tested v1 of this series and everything seems sorted out.  On
the basis that the only changes are superficial:

Tested-by: George Dunlap <george.dunlap@citrix.com>

On Fri, Apr 8, 2016 at 5:13 PM, Roger Pau Monne <roger.pau@citrix.com> wrote:
> Due to recent changes (and intended fixes) CDROM has become quite unstable
> recently. This series aims to fix the issues reported with having empty
> CDROM devices in the HVM guest configuration files. It also contains some
> cleanups of more generic code in order to ease the implementation.
>
> After some talk on IRC, the easiest solution at this point in the release is
> to force PV CDROM backends of HVM guests with a device model to use Qdisk,
> since the cd-{insert/eject} implementation is only able to deal with this
> kind of PV backend.
>
> Roger.
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

Re: [PATCH v2 4/4] libxl: remove code added to use the 'phy' backend with CDROM devices

[Wei Liu]

On Fri, Apr 08, 2016 at 06:13:39PM +0200, Roger Pau Monne wrote:
> This is a partial revert of 612f15, that allowed CDROM devices to use the
> 'phy' PV backend. Due to limitations in the current implementation of the
> libxl_cdrom_insert function, the PV backend used in conjunction with an
> emulated CDROM device must always be Qdisk at the moment. This is due to
> libxl_cdrom_insert not running disk hotplug scripts on plug and unplug of PV
> CDROM backends (and possibly other yet to be identified issues).

LGTM

> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> Changes since v1:
>  - Added a proper commit message.
> ---
>  tools/libxl/libxl_device.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index ce53520..82405a7 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -191,20 +191,13 @@ static int disk_try_backend(disk_try_backend_args *a,
>  
>      switch (backend) {
>      case LIBXL_DISK_BACKEND_PHY:
> -        if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW ||
> -              a->disk->format == LIBXL_DISK_FORMAT_EMPTY)) {
> +        if (a->disk->format != LIBXL_DISK_FORMAT_RAW) {
>              goto bad_format;
>          }
>  
>          if (libxl_defbool_val(a->disk->colo_enable))
>              goto bad_colo;
>  
> -        if (a->disk->format == LIBXL_DISK_FORMAT_EMPTY) {
> -            LOG(DEBUG, "Disk vdev=%s is empty, skipping physical device check",
> -                a->disk->vdev);
> -            return backend;
> -        }
> -
>          if (a->disk->backend_domid != LIBXL_TOOLSTACK_DOMID) {
>              LOG(DEBUG, "Disk vdev=%s, is using a storage driver domain, "
>                         "skipping physical device check", a->disk->vdev);
> -- 
> 2.6.4 (Apple Git-63)
> 

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

Re: [PATCH v2 0/4] libxl: fix CDROM issues

[Roger Pau Monné]

On Fri, 8 Apr 2016, George Dunlap wrote:
> BTW, I tested v1 of this series and everything seems sorted out.  On
> the basis that the only changes are superficial:
> 
> Tested-by: George Dunlap <george.dunlap@citrix.com>

Thanks, FWIW I've also pushed the series to a git branch:

git://xenbits.xen.org/people/royger/xen.git fix_cdrom_v2

> 
> On Fri, Apr 8, 2016 at 5:13 PM, Roger Pau Monne <roger.pau@citrix.com> wrote:
> > Due to recent changes (and intended fixes) CDROM has become quite unstable
> > recently. This series aims to fix the issues reported with having empty
> > CDROM devices in the HVM guest configuration files. It also contains some
> > cleanups of more generic code in order to ease the implementation.
> >
> > After some talk on IRC, the easiest solution at this point in the release is
> > to force PV CDROM backends of HVM guests with a device model to use Qdisk,
> > since the cd-{insert/eject} implementation is only able to deal with this
> > kind of PV backend.
> >
> > Roger.
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 

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

Re: [PATCH v2 0/4] libxl: fix CDROM issues

[Roger Pau Monné]

[-- Attachment #1: Type: text/plain, Size: 1388 bytes --]

On Fri, 8 Apr 2016, Roger Pau Monné wrote:
> On Fri, 8 Apr 2016, George Dunlap wrote:
> > BTW, I tested v1 of this series and everything seems sorted out.  On
> > the basis that the only changes are superficial:
> > 
> > Tested-by: George Dunlap <george.dunlap@citrix.com>
> 
> Thanks, FWIW I've also pushed the series to a git branch:
> 
> git://xenbits.xen.org/people/royger/xen.git fix_cdrom_v2

Sorry, I've pushed a new one with George's Tested-by tag added:

git://xenbits.xen.org/people/royger/xen.git fix_cdrom_v2.1

> > 
> > On Fri, Apr 8, 2016 at 5:13 PM, Roger Pau Monne <roger.pau@citrix.com> wrote:
> > > Due to recent changes (and intended fixes) CDROM has become quite unstable
> > > recently. This series aims to fix the issues reported with having empty
> > > CDROM devices in the HVM guest configuration files. It also contains some
> > > cleanups of more generic code in order to ease the implementation.
> > >
> > > After some talk on IRC, the easiest solution at this point in the release is
> > > to force PV CDROM backends of HVM guests with a device model to use Qdisk,
> > > since the cd-{insert/eject} implementation is only able to deal with this
> > > kind of PV backend.
> > >
> > > Roger.
> > >
> > >
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > http://lists.xen.org/xen-devel
> > 
> 

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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