Re: [Xen-devel] [PATCH] libxl: add basic spice support for pv domUs

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Jan 13, 2016 at 11:47:55AM +0100, Fabio Fantoni wrote:
[...]
> >>
> >>diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> >>index 3b695bd..04a96ba 100644
> >>--- a/docs/man/xl.cfg.pod.5
> >>+++ b/docs/man/xl.cfg.pod.5
> >>@@ -1607,82 +1607,6 @@ it.
> >>  =back
> >>-=head3 Spice Graphics Support
> >>-
> >>-The following options control the features of SPICE.
> >>-
> >>-=over 4
> >>-
> >>-=item B<spice=BOOLEAN>
> >>-
> >To be honest I'm a bit confused by this large hunk. Did you notice some
> >sort of misplacement of this section? Or is it your editor is using
> >different wrapping setting?
> 
> Now is inside hvm specific section but if basic support for the pv domUs
> will be added I think should be moved out of hvm section. Or I'm wrong?
> 

Yes, you're right. Could you say so in the commit message?

> >
> >>  =head2 Keymaps
> >>  The keymaps available are defined by the device-model which you are
> >>diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> >>index 6b73848..a5cbcfc 100644
> >>--- a/tools/libxl/libxl.h
> >>+++ b/tools/libxl/libxl.h
> >>@@ -510,6 +510,19 @@ typedef struct libxl__ctx libxl_ctx;
> >>  #define LIBXL_HAVE_BUILDINFO_USBVERSION 1
> >>  /*
> >>+ * LIBXL_HAVE_BUILDINFO_SPICE
> >>+ *
> >>+ * If this is defined, then the libxl_domain_build_info structure will
> >>+ * contain spice, a libxl_spice_info struct instead of older hvm.spice one
> >>+ * which is now deprecated.
> >>+ *
> >This reads like hvm.spice is removed but I think hvm.spice still exists.
> 
> Yes I keeped hvm.spice (as "duplicate") for compatibility as suggested by
> someone long time ago.
> 
> >
[...]
> >>   * If this is defined, libxl_device_* structures containing a 
> >> backend_domid
> >>diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> >>index 8770486..a1853dc 100644
> >>--- a/tools/libxl/libxl_create.c
> >>+++ b/tools/libxl/libxl_create.c
> >>@@ -195,6 +195,19 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> >>      if (!b_info->event_channels)
> >>          b_info->event_channels = 1023;
> >>+    /* If older u.hvm.spice is enabled then propagate it to the top level 
> >>*/
> >>+    libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
> >This is wrong -- don't set u.hvm without checking the guest is actually
> >hvm guest.
> 
> If I remember good without setting always setdefault I saw some cases of xl
> create segfault.
> I didn't remember exactly cases, was long time ago.
> 

I was not asking you to not set this. Presumably you still need to set
this, just that you can't set it unconditionally. You need to check if
this guest is really a HVM guest.

> >
> >>+    libxl_defbool_setdefault(&b_info->spice.enable, false);
> >>+    if (!libxl_defbool_val(b_info->spice.enable) &&
> >>+        libxl_defbool_val(b_info->u.hvm.spice.enable)) {
> >>+            b_info->spice = b_info->u.hvm.spice;
> >Please use libxl_spice_info_copy (an autogenerated function) to do the
> >copying.  Doing a shallow copy like this is prone to error -- consider
> >in the future your structure contains pointers to allocated memory,
> >doing shallow copy will cause double free.
> 
> Thank for spotted it.
> Can you explain exactly how to do it? I did a fast search but I'm probably
> not understanding correctly.
> >libxl_spice_info_copy(ctx, &b_info->spice, b_info->u.hvm.spice);
> Should the code be as the line above?
> I also not understand if I must also an _init before the _copy like this:
> >libxl_spice_info_init(&b_info->spice);
> 

You don't need to call _init because it should already be initialised
at this point.

And yes, you should just use libxl_spice_info_copy to replace the
direct assignment.

> 
> >
> >>+    }
> >>+
> >>+    libxl_defbool_setdefault(&b_info->spice.disable_ticketing, false);
> >>+    libxl_defbool_setdefault(&b_info->spice.agent_mouse, true);
> >>+    libxl_defbool_setdefault(&b_info->spice.vdagent, false);
> >>+    libxl_defbool_setdefault(&b_info->spice.clipboard_sharing, false);
> >>+
> >>      switch (b_info->type) {
> >>      case LIBXL_DOMAIN_TYPE_HVM:
> >>          if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
> >>@@ -291,18 +304,17 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> >>          libxl_defbool_setdefault(&b_info->u.hvm.usb,                
> >> false);
> >>          libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,   true);
> >>-        libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
> >>-        if (!libxl_defbool_val(b_info->u.hvm.spice.enable) &&
> >>-            (b_info->u.hvm.spice.usbredirection > 0) ){
> >>-            b_info->u.hvm.spice.usbredirection = 0;
> >>-            LOG(WARN, "spice disabled, disabling usbredirection");
> >>+        if (!libxl_defbool_val(b_info->spice.enable) &&
> >>+            (b_info->spice.usbredirection > 0) ){
> >>+            b_info->spice.usbredirection = 0;
> >>+            LOG(WARN,"spice disabled, disabling usbredirection");
> >>          }
> >>          if (!b_info->u.hvm.usbversion &&
> >>-            (b_info->u.hvm.spice.usbredirection > 0) )
> >>+            (b_info->spice.usbredirection > 0) )
> >>              b_info->u.hvm.usbversion = 2;
> >>-        if ((b_info->u.hvm.usbversion || 
> >>b_info->u.hvm.spice.usbredirection) &&
> >>+        if ((b_info->u.hvm.usbversion || b_info->spice.usbredirection) &&
> >>              ( libxl_defbool_val(b_info->u.hvm.usb)
> >>              || b_info->u.hvm.usbdevice_list
> >>              || b_info->u.hvm.usbdevice) ){
> >>@@ -330,15 +342,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> >>              libxl_defbool_setdefault(&b_info->u.hvm.sdl.opengl, false);
> >>          }
> >>-        if (libxl_defbool_val(b_info->u.hvm.spice.enable)) {
> >>-            
> >>libxl_defbool_setdefault(&b_info->u.hvm.spice.disable_ticketing,
> >>-                                     false);
> >>-            libxl_defbool_setdefault(&b_info->u.hvm.spice.agent_mouse, 
> >>true);
> >>-            libxl_defbool_setdefault(&b_info->u.hvm.spice.vdagent, false);
> >>-            
> >>libxl_defbool_setdefault(&b_info->u.hvm.spice.clipboard_sharing,
> >>-                                     false);
> >>-        }
> >>-
> >>          libxl_defbool_setdefault(&b_info->u.hvm.nographic, false);
> >>          libxl_defbool_setdefault(&b_info->u.hvm.gfx_passthru, false);
> >>@@ -372,6 +375,20 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> >>              b_info->cmdline = b_info->u.pv.cmdline;
> >>              b_info->u.pv.cmdline = NULL;
> >>          }
> >>+
> >>+        if (libxl_defbool_val(b_info->spice.vdagent)) {
> >>+            libxl_defbool_set(&b_info->spice.vdagent, false);
> >>+            LOG(WARN, "vdagent is not supported for PV guests");
> >>+        }
> >>+        if (libxl_defbool_val(b_info->spice.clipboard_sharing)) {
> >>+            libxl_defbool_set(&b_info->spice.clipboard_sharing, false);
> >>+            LOG(WARN, "clipboard sharing is not supported for PV guests");
> >>+        }
> >>+        if (b_info->spice.usbredirection > 0) {
> >>+            b_info->spice.usbredirection = 0;
> >>+            LOG(WARN, "usbredirection is not supported for PV guests");
> >>+        }
> >>+
> >Is there any support matrix in QEMU that can be used as reference?
> 
> No, FWIK. PV domUs don't have/support emulated pci, for example emulated
> qemu usb controller used by spice usbredirection. Probably a pvusb
> controller can be used for add it but I don't know how to do the needed
> changes.
> Same for vdagent (and other features provided by it) that require
> virtio-serial, someone wrote there will be probably possible in future with
> mmio on pvh (when will be implemented) but I don't remember the details.
>

If QEMU doesn't have one, can you provide one for Xen to reflect the
reality? There are several components in SPICE, can you point out
what each of them needs?

 
> >
> >>          break;
> >>      default:
> >>          LOG(ERROR, "invalid domain type %s in create info",
> >>diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> >>index a4934df..bf7cf1c 100644
> >>--- a/tools/libxl/libxl_dm.c
> >>+++ b/tools/libxl/libxl_dm.c
> >>@@ -897,6 +897,21 @@ static int 
> >>libxl__build_device_model_args_new(libxl__gc *gc,
> >>          flexarray_vappend(dm_args, "-k", keymap, NULL);
> >>      }
> >>+    if (libxl_defbool_val(b_info->spice.enable)) {
> >>+        const libxl_spice_info *spice = &b_info->spice;
> >>+        char *spiceoptions = dm_spice_options(gc, spice);
> >>+        if (!spiceoptions)
> >>+            return ERROR_INVAL;
> >>+
> >>+        flexarray_append(dm_args, "-spice");
> >>+        flexarray_append(dm_args, spiceoptions);
> >>+        if (libxl_defbool_val(b_info->spice.vdagent)) {
> >>+            flexarray_vappend(dm_args, "-device", "virtio-serial",
> >>+                "-chardev", "spicevmc,id=vdagent,name=vdagent", "-device",
> >>+                "virtserialport,chardev=vdagent,name=com.redhat.spice.0", 
> >>NULL);
> >There is hardcoded string here. Any reference why it is used?
> 
> Was already present is for add virtio-serial port used by vdagent.
> FWIK there isn't a better and simple way  to do it.
> http://www.spice-space.org/docs/manual/#_configuration_2
>

Fair enough.

> >
> >>+        }
> >>+    }
> >>+
> >>      if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
> >>          int ioemu_nics = 0;
> >>@@ -934,22 +949,6 @@ static int 
> >>libxl__build_device_model_args_new(libxl__gc *gc,
> >>              flexarray_append(dm_args, "-nographic");
> >>          }
> >>-        if (libxl_defbool_val(b_info->u.hvm.spice.enable)) {
> >>-            const libxl_spice_info *spice = &b_info->u.hvm.spice;
> >>-            char *spiceoptions = dm_spice_options(gc, spice);
> >>-            if (!spiceoptions)
> >>-                return ERROR_INVAL;
> >>-
> >>-            flexarray_append(dm_args, "-spice");
> >>-            flexarray_append(dm_args, spiceoptions);
> >>-            if (libxl_defbool_val(b_info->u.hvm.spice.vdagent)) {
> >>-                flexarray_vappend(dm_args, "-device", "virtio-serial",
> >>-                    "-chardev", "spicevmc,id=vdagent,name=vdagent", 
> >>"-device",
> >>-                    
> >>"virtserialport,chardev=vdagent,name=com.redhat.spice.0",
> >>-                    NULL);
> >>-            }
> >>-        }
> >>-
> >>          switch (b_info->u.hvm.vga.kind) {
> >>          case LIBXL_VGA_INTERFACE_TYPE_STD:
> >>              flexarray_append_pair(dm_args, "-device",
> >>@@ -1021,9 +1020,9 @@ static int 
> >>libxl__build_device_model_args_new(libxl__gc *gc,
> >>                      "must be between 1 and 3");
> >>                  return ERROR_INVAL;
> >>              }
> >>-            if (b_info->u.hvm.spice.usbredirection >= 0 &&
> >>-                b_info->u.hvm.spice.usbredirection < 5) {
> >>-                for (i = 1; i <= b_info->u.hvm.spice.usbredirection; i++)
> >>+            if (b_info->spice.usbredirection >= 0 &&
> >>+                b_info->spice.usbredirection < 5) {
> >>+                for (i = 1; i <= b_info->spice.usbredirection; i++)
> >>                      flexarray_vappend(dm_args, "-chardev",
> >>                          GCSPRINTF("spicevmc,name=usbredir,id=usbrc%d", i),
> >>                          "-device",
> >>@@ -1081,7 +1080,7 @@ static int 
> >>libxl__build_device_model_args_new(libxl__gc *gc,
> >>              flexarray_append(dm_args, "none");
> >>          }
> >>      } else {
> >>-        if (!sdl && !vnc) {
> >>+        if (!sdl && !vnc && !libxl_defbool_val(b_info->spice.enable)) {
> >>              flexarray_append(dm_args, "-nographic");
> >>          }
> >>      }
> >>diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> >>index cf3730f..354af0a 100644
> >>--- a/tools/libxl/libxl_types.idl
> >>+++ b/tools/libxl/libxl_types.idl
> >>@@ -455,7 +455,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >>      ("ioports",          Array(libxl_ioport_range, "num_ioports")),
> >>      ("irqs",             Array(uint32, "num_irqs")),
> >>      ("iomem",            Array(libxl_iomem_range, "num_iomem")),
> >>-    ("claim_mode",      libxl_defbool),
> >>+    ("claim_mode",       libxl_defbool),
> >Unrelated change.
> 
> Is only a small indentation fix, I found it long time ago and still not
> present.
> I must do separate patch only with it?

Yes, a separate patch is more appropriate.

Wei.

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