[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH] libxl: add basic spice support for pv domUs
Il 16/02/2016 18:44, Wei Liu ha scritto:
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?
yes, I'll do it
=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.
If I remember good some version ago I did it hvm specific but xl create
of pv domUs fails for this default not setted
+ 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.
So replace "b_info->spice = b_info->u.hvm.spice;" with
"libxl_spice_info_copy(ctx, &b_info->spice, b_info->u.hvm.spice);" is
correct?
+ }
+
+ 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?
usb redirection is based/needs an emulated usb controller (that require
emulated pci controller missed in pv), I saw that there is a xen pvusb
project without needing qemu emulated but I don't know if will be
possible use it (I suppose yes but doing some changes)
qxl is an emulated vga and require emulated pci controller missed in pv
vdagent require virtio-serial (that require emulated pci controller
missed in pv)
someone told that probably will be possible support them in pvh when
mmio support will be added but I don't know how to do it
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.
Done and sent
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
