This is a patch to forward-port a Xend behaviour. Xend writes IO ABI used for
all frontends. Linux kernel before 2.6.26 relies on this behaviour otherwise
it cannot boot. Blkfront after 2.6.26 writes that node itself, in which case
it''s just an overwrite of the existing node which should be OK.
In fact Xend writes the ABI for all frontends including console and vif. But
nowadays it seems only old disk frontends rely on that ABI node so that we
only write the ABI for disk frontends in libxl, minimizing the impact. Also
ARM guests should not have this problem because they have new disk frontend so
the snippets of this workaround are compiled for X86 target only.
Also cleaned up trailing whitespaces while I was there.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxc/xc_domain.c | 20 ++++++++++++++++++
tools/libxc/xenctrl.h | 14 +++++++++++++
tools/libxl/libxl.c | 12 +++++++++++
tools/libxl/libxl_create.c | 49 +++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_types.idl | 7 ++++---
5 files changed, 99 insertions(+), 3 deletions(-)
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 480ce91..c3d8e28 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -379,6 +379,26 @@ int xc_domain_hvm_setcontext(xc_interface *xch,
return ret;
}
+#if defined (__i386__) || (__x86_64__)
+/* get guest word width */
+int xc_domain_getwidth(xc_interface *xch,
+ uint32_t domid,
+ uint32_t *guest_width)
+{
+ int ret;
+ DECLARE_DOMCTL;
+
+ domctl.domain = domid;
+ domctl.cmd = XEN_DOMCTL_get_address_size;
+
+ ret = do_domctl(xch, &domctl);
+
+ *guest_width = domctl.u.address_size.size;
+
+ return ret;
+}
+#endif
+
int xc_vcpu_getcontext(xc_interface *xch,
uint32_t domid,
uint32_t vcpu,
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 50853af..c02dc1f 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -636,6 +636,20 @@ int xc_domain_hvm_setcontext(xc_interface *xch,
uint8_t *hvm_ctxt,
uint32_t size);
+#if defined (__i386__) || (__x86_64__)
+/**
+ * This function will return the guest word width
+ *
+ * @parm xch a handle to an open hypervisor interface
+ * @parm domid the domain to get address width for
+ * @parm guest_width word width for guest
+ * @return 0 on success, -ev on failure
+ */
+int xc_domain_getwidth(xc_interface *xch,
+ uint32_t domid,
+ uint32_t *guest_width);
+#endif
+
/**
* This function returns information about the execution context of a
* particular vcpu of a domain.
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 572c2c6..5b8c9dc 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2156,6 +2156,18 @@ static void device_disk_add(libxl__egc *egc, uint32_t
domid,
flexarray_append(front, "device-type");
flexarray_append(front, disk->is_cdrom ? "cdrom" :
"disk");
+ /*
+ * Old PV kernels before 2.6.26 rely on tool stack to write disk native
+ * protocol to frontend node.
+ *
+ * New kernels write this node themselves. In that case it just
+ * overwrites an existing node which is OK.
+ */
+ if (disk->native_protocol) {
+ flexarray_append(front, "protocol");
+ flexarray_append(front, disk->native_protocol);
+ }
+
libxl__device_generic_add(gc, t, device,
libxl__xs_kvs_of_flexarray(gc, back,
back->count),
libxl__xs_kvs_of_flexarray(gc, front,
front->count));
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 30a4507..7abcaf9 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -23,6 +23,7 @@
#include <xc_dom.h>
#include <xenguest.h>
#include <xen/hvm/hvm_info_table.h>
+#include <xen/io/protocols.h>
int libxl__domain_create_info_setdefault(libxl__gc *gc,
libxl_domain_create_info *c_info)
@@ -910,6 +911,54 @@ static void domcreate_rebuild_done(libxl__egc *egc,
goto error_out;
}
+#if defined (__i386__) || (__x86_64__)
+ /*
+ * Pass along the disk navtive protocol to disks so that the protocol can
+ * be written to frontend Xenstore node. This is a workaround for old PV
+ * kernels before 2.6.26.
+ *
+ * Newer blkfront will write that node itself. In that case frontend just
+ * rewrites the node as it sees fit.
+ *
+ * Note that Xend actually propagate this vaule to all frontends including
+ * console and vifs. As now this is only needed for disk frontend so here
+ * we minimize the impact.
+ *
+ * Presumably ARM guests don''t have this problem, so this snippet
is only
+ * compiled for X86 target.
+ */
+
+ if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) {
+ int i;
+ uint32_t guest_width;
+ const char *protocol = NULL;
+
+ ret = xc_domain_getwidth(CTX->xch, domid, &guest_width);
+ if (ret) {
+ ret = ERROR_FAIL;
+ goto error_out;
+ }
+
+ switch (guest_width) {
+ case 32: /* 32 bit guest */
+ protocol = XEN_IO_PROTO_ABI_X86_32;
+ break;
+ case 64: /* 64 bit guest */
+ protocol = XEN_IO_PROTO_ABI_X86_64;
+ break;
+ default:
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "invalid address size for domain: %u",
+ guest_width);
+ ret = ERROR_FAIL;
+ goto error_out;
+ }
+
+ for (i = 0; i < d_config->num_disks; i++)
+ d_config->disks[i].native_protocol = strdup(protocol);
+ }
+#endif
+
store_libxl_entry(gc, domid, &d_config->b_info);
libxl__multidev_begin(ao, &dcs->multidev);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 6cb6de6..22622a9 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -274,7 +274,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
("disable_migrate", libxl_defbool),
("cpuid", libxl_cpuid_policy_list),
("blkdev_start", string),
-
+
("device_model_version", libxl_device_model_version),
("device_model_stubdomain", libxl_defbool),
# if you set device_model you must set device_model_version too
@@ -318,9 +318,9 @@ libxl_domain_build_info =
Struct("domain_build_info",[
("keymap", string),
("sdl",
libxl_sdl_info),
("spice",
libxl_spice_info),
-
+
("gfx_passthru",
libxl_defbool),
-
+
("serial", string),
("boot", string),
("usb",
libxl_defbool),
@@ -371,6 +371,7 @@ libxl_device_disk = Struct("device_disk", [
("removable", integer),
("readwrite", integer),
("is_cdrom", integer),
+ ("native_protocol", string),
])
libxl_device_nic = Struct("device_nic", [
--
1.7.10.4
Hi, adding Valtteri to CC because he originally reported the problem.. -- Pasi On Tue, Apr 23, 2013 at 09:25:54PM +0100, Wei Liu wrote:> This is a patch to forward-port a Xend behaviour. Xend writes IO ABI used for > all frontends. Linux kernel before 2.6.26 relies on this behaviour otherwise > it cannot boot. Blkfront after 2.6.26 writes that node itself, in which case > it''s just an overwrite of the existing node which should be OK. > > In fact Xend writes the ABI for all frontends including console and vif. But > nowadays it seems only old disk frontends rely on that ABI node so that we > only write the ABI for disk frontends in libxl, minimizing the impact. Also > ARM guests should not have this problem because they have new disk frontend so > the snippets of this workaround are compiled for X86 target only. > > Also cleaned up trailing whitespaces while I was there. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > tools/libxc/xc_domain.c | 20 ++++++++++++++++++ > tools/libxc/xenctrl.h | 14 +++++++++++++ > tools/libxl/libxl.c | 12 +++++++++++ > tools/libxl/libxl_create.c | 49 +++++++++++++++++++++++++++++++++++++++++++ > tools/libxl/libxl_types.idl | 7 ++++--- > 5 files changed, 99 insertions(+), 3 deletions(-) > > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c > index 480ce91..c3d8e28 100644 > --- a/tools/libxc/xc_domain.c > +++ b/tools/libxc/xc_domain.c > @@ -379,6 +379,26 @@ int xc_domain_hvm_setcontext(xc_interface *xch, > return ret; > } > > +#if defined (__i386__) || (__x86_64__) > +/* get guest word width */ > +int xc_domain_getwidth(xc_interface *xch, > + uint32_t domid, > + uint32_t *guest_width) > +{ > + int ret; > + DECLARE_DOMCTL; > + > + domctl.domain = domid; > + domctl.cmd = XEN_DOMCTL_get_address_size; > + > + ret = do_domctl(xch, &domctl); > + > + *guest_width = domctl.u.address_size.size; > + > + return ret; > +} > +#endif > + > int xc_vcpu_getcontext(xc_interface *xch, > uint32_t domid, > uint32_t vcpu, > diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h > index 50853af..c02dc1f 100644 > --- a/tools/libxc/xenctrl.h > +++ b/tools/libxc/xenctrl.h > @@ -636,6 +636,20 @@ int xc_domain_hvm_setcontext(xc_interface *xch, > uint8_t *hvm_ctxt, > uint32_t size); > > +#if defined (__i386__) || (__x86_64__) > +/** > + * This function will return the guest word width > + * > + * @parm xch a handle to an open hypervisor interface > + * @parm domid the domain to get address width for > + * @parm guest_width word width for guest > + * @return 0 on success, -ev on failure > + */ > +int xc_domain_getwidth(xc_interface *xch, > + uint32_t domid, > + uint32_t *guest_width); > +#endif > + > /** > * This function returns information about the execution context of a > * particular vcpu of a domain. > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 572c2c6..5b8c9dc 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -2156,6 +2156,18 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid, > flexarray_append(front, "device-type"); > flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk"); > > + /* > + * Old PV kernels before 2.6.26 rely on tool stack to write disk native > + * protocol to frontend node. > + * > + * New kernels write this node themselves. In that case it just > + * overwrites an existing node which is OK. > + */ > + if (disk->native_protocol) { > + flexarray_append(front, "protocol"); > + flexarray_append(front, disk->native_protocol); > + } > + > libxl__device_generic_add(gc, t, device, > libxl__xs_kvs_of_flexarray(gc, back, back->count), > libxl__xs_kvs_of_flexarray(gc, front, front->count)); > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index 30a4507..7abcaf9 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -23,6 +23,7 @@ > #include <xc_dom.h> > #include <xenguest.h> > #include <xen/hvm/hvm_info_table.h> > +#include <xen/io/protocols.h> > > int libxl__domain_create_info_setdefault(libxl__gc *gc, > libxl_domain_create_info *c_info) > @@ -910,6 +911,54 @@ static void domcreate_rebuild_done(libxl__egc *egc, > goto error_out; > } > > +#if defined (__i386__) || (__x86_64__) > + /* > + * Pass along the disk navtive protocol to disks so that the protocol can > + * be written to frontend Xenstore node. This is a workaround for old PV > + * kernels before 2.6.26. > + * > + * Newer blkfront will write that node itself. In that case frontend just > + * rewrites the node as it sees fit. > + * > + * Note that Xend actually propagate this vaule to all frontends including > + * console and vifs. As now this is only needed for disk frontend so here > + * we minimize the impact. > + * > + * Presumably ARM guests don''t have this problem, so this snippet is only > + * compiled for X86 target. > + */ > + > + if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) { > + int i; > + uint32_t guest_width; > + const char *protocol = NULL; > + > + ret = xc_domain_getwidth(CTX->xch, domid, &guest_width); > + if (ret) { > + ret = ERROR_FAIL; > + goto error_out; > + } > + > + switch (guest_width) { > + case 32: /* 32 bit guest */ > + protocol = XEN_IO_PROTO_ABI_X86_32; > + break; > + case 64: /* 64 bit guest */ > + protocol = XEN_IO_PROTO_ABI_X86_64; > + break; > + default: > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > + "invalid address size for domain: %u", > + guest_width); > + ret = ERROR_FAIL; > + goto error_out; > + } > + > + for (i = 0; i < d_config->num_disks; i++) > + d_config->disks[i].native_protocol = strdup(protocol); > + } > +#endif > + > store_libxl_entry(gc, domid, &d_config->b_info); > > libxl__multidev_begin(ao, &dcs->multidev); > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 6cb6de6..22622a9 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -274,7 +274,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ > ("disable_migrate", libxl_defbool), > ("cpuid", libxl_cpuid_policy_list), > ("blkdev_start", string), > - > + > ("device_model_version", libxl_device_model_version), > ("device_model_stubdomain", libxl_defbool), > # if you set device_model you must set device_model_version too > @@ -318,9 +318,9 @@ libxl_domain_build_info = Struct("domain_build_info",[ > ("keymap", string), > ("sdl", libxl_sdl_info), > ("spice", libxl_spice_info), > - > + > ("gfx_passthru", libxl_defbool), > - > + > ("serial", string), > ("boot", string), > ("usb", libxl_defbool), > @@ -371,6 +371,7 @@ libxl_device_disk = Struct("device_disk", [ > ("removable", integer), > ("readwrite", integer), > ("is_cdrom", integer), > + ("native_protocol", string), > ]) > > libxl_device_nic = Struct("device_nic", [ > -- > 1.7.10.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On Wed, Apr 24, 2013 at 08:39:50AM +0100, Pasi Kärkkäinen wrote:> Hi, > > adding Valtteri to CC because he originally reported the problem.. >Thanks. I tested this with my own hack -- disabling the code used to write the node in frontend. Having the original reporter verifies this fix would be helpful. Valtteri could you please help test this if possible. Wei.
> @@ -910,6 +911,54 @@ static void domcreate_rebuild_done(libxl__egc *egc, > goto error_out; > } > > +#if defined (__i386__) || (__x86_64__) > + /* > + * Pass along the disk navtive protocol to disks so that the protocol cannative> + * be written to frontend Xenstore node. This is a workaround for old PV > + * kernels before 2.6.26. > + * > + * Newer blkfront will write that node itself. In that case frontend just > + * rewrites the node as it sees fit. > + * > + * Note that Xend actually propagate this vaule to all frontends includingvalue> + * console and vifs. As now this is only needed for disk frontend so here > + * we minimize the impact. > + * > + * Presumably ARM guests don''t have this problem, so this snippet is only > + * compiled for X86 target.Yes, this isn''t required on ARM. Is there a reason why this is only done for PV guests? I''d expect at least some older versions of the PV on HVM drivers to also need this node. What did Xend do in this regard? I think it would be preferable to have the logic for determining the protocol in libxc, rather than exposing the guest width and putting the logic in libxl. Mostly because on domain build it is also libxc which makes this determination and keeping it in the same library makes sense to me. What I''d actually prefer is to use the value of dom->arch_hooks.native_protocol in libxl__build_pv when building a new domain and have xc_domain_restore include the protocol as an output when restoring so that libxl can use that in the restore case. This might be more plumbing than we are willing to do at this stage of the release though, so if you remove the bits which expose the protocol in the libxl API (see below) and make this purely internal functionality of libxl+ libxc we could live with an explicit query for the protocol.> + */ > + > + if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) { > + int i; > + uint32_t guest_width; > + const char *protocol = NULL; > + > + ret = xc_domain_getwidth(CTX->xch, domid, &guest_width); > + if (ret) { > + ret = ERROR_FAIL; > + goto error_out; > + } > + > + switch (guest_width) { > + case 32: /* 32 bit guest */ > + protocol = XEN_IO_PROTO_ABI_X86_32; > + break; > + case 64: /* 64 bit guest */ > + protocol = XEN_IO_PROTO_ABI_X86_64; > + break; > + default: > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > + "invalid address size for domain: %u", > + guest_width); > + ret = ERROR_FAIL; > + goto error_out; > + } > + > + for (i = 0; i < d_config->num_disks; i++) > + d_config->disks[i].native_protocol = strdup(protocol);If you are going to expose native_protocol to the libxl user then you need to allow for them to have set it. However I don''t think we want/need to expose this to the user, device_disk_add() can just query and write the string in one go. Or it could be queried once for the domain early on and remembered in some handy place (either a suitable datastructure or in xenstore) Did Xend only do this for disks, or did it do it for all devices?> + } > +#endif > + > store_libxl_entry(gc, domid, &d_config->b_info); > > libxl__multidev_begin(ao, &dcs->multidev); > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 6cb6de6..22622a9 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -274,7 +274,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ > ("disable_migrate", libxl_defbool), > ("cpuid", libxl_cpuid_policy_list), > ("blkdev_start", string), > - > + > ("device_model_version", libxl_device_model_version), > ("device_model_stubdomain", libxl_defbool), > # if you set device_model you must set device_model_version too > @@ -318,9 +318,9 @@ libxl_domain_build_info = Struct("domain_build_info",[ > ("keymap", string), > ("sdl", libxl_sdl_info), > ("spice", libxl_spice_info), > - > + > ("gfx_passthru", libxl_defbool), > - > + > ("serial", string), > ("boot", string), > ("usb", libxl_defbool), > @@ -371,6 +371,7 @@ libxl_device_disk = Struct("device_disk", [ > ("removable", integer), > ("readwrite", integer), > ("is_cdrom", integer), > + ("native_protocol", string), > ]) > > libxl_device_nic = Struct("device_nic", [
On 23/04/13 22:25, Wei Liu wrote:> This is a patch to forward-port a Xend behaviour. Xend writes IO ABI used for > all frontends. Linux kernel before 2.6.26 relies on this behaviour otherwise > it cannot boot. Blkfront after 2.6.26 writes that node itself, in which case > it''s just an overwrite of the existing node which should be OK. > > In fact Xend writes the ABI for all frontends including console and vif. But > nowadays it seems only old disk frontends rely on that ABI node so that we > only write the ABI for disk frontends in libxl, minimizing the impact. Also > ARM guests should not have this problem because they have new disk frontend so > the snippets of this workaround are compiled for X86 target only. > > Also cleaned up trailing whitespaces while I was there. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > tools/libxc/xc_domain.c | 20 ++++++++++++++++++ > tools/libxc/xenctrl.h | 14 +++++++++++++ > tools/libxl/libxl.c | 12 +++++++++++ > tools/libxl/libxl_create.c | 49 +++++++++++++++++++++++++++++++++++++++++++ > tools/libxl/libxl_types.idl | 7 ++++--- > 5 files changed, 99 insertions(+), 3 deletions(-) > > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c > index 480ce91..c3d8e28 100644 > --- a/tools/libxc/xc_domain.c > +++ b/tools/libxc/xc_domain.c > @@ -379,6 +379,26 @@ int xc_domain_hvm_setcontext(xc_interface *xch, > return ret; > } > > +#if defined (__i386__) || (__x86_64__)Could we define this for all guests even if we only use it on x86 right now?> +/* get guest word width */ > +int xc_domain_getwidth(xc_interface *xch, > + uint32_t domid, > + uint32_t *guest_width) > +{ > + int ret; > + DECLARE_DOMCTL; > + > + domctl.domain = domid; > + domctl.cmd = XEN_DOMCTL_get_address_size; > + > + ret = do_domctl(xch, &domctl); > + > + *guest_width = domctl.u.address_size.size; > + > + return ret; > +} > +#endif > + > int xc_vcpu_getcontext(xc_interface *xch, > uint32_t domid, > uint32_t vcpu, > diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h > index 50853af..c02dc1f 100644 > --- a/tools/libxc/xenctrl.h > +++ b/tools/libxc/xenctrl.h > @@ -636,6 +636,20 @@ int xc_domain_hvm_setcontext(xc_interface *xch, > uint8_t *hvm_ctxt, > uint32_t size); > > +#if defined (__i386__) || (__x86_64__) > +/** > + * This function will return the guest word width > + * > + * @parm xch a handle to an open hypervisor interface > + * @parm domid the domain to get address width for > + * @parm guest_width word width for guest > + * @return 0 on success, -ev on failure > + */ > +int xc_domain_getwidth(xc_interface *xch, > + uint32_t domid, > + uint32_t *guest_width); > +#endif > + > /** > * This function returns information about the execution context of a > * particular vcpu of a domain. > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 572c2c6..5b8c9dc 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -2156,6 +2156,18 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid, > flexarray_append(front, "device-type"); > flexarray_append(front, disk->is_cdrom ? "cdrom" : "disk"); > > + /* > + * Old PV kernels before 2.6.26 rely on tool stack to write disk native > + * protocol to frontend node. > + * > + * New kernels write this node themselves. In that case it just > + * overwrites an existing node which is OK. > + */ > + if (disk->native_protocol) { > + flexarray_append(front, "protocol"); > + flexarray_append(front, disk->native_protocol); > + } > + > libxl__device_generic_add(gc, t, device, > libxl__xs_kvs_of_flexarray(gc, back, back->count), > libxl__xs_kvs_of_flexarray(gc, front, front->count)); > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index 30a4507..7abcaf9 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -23,6 +23,7 @@ > #include <xc_dom.h> > #include <xenguest.h> > #include <xen/hvm/hvm_info_table.h> > +#include <xen/io/protocols.h> > > int libxl__domain_create_info_setdefault(libxl__gc *gc, > libxl_domain_create_info *c_info) > @@ -910,6 +911,54 @@ static void domcreate_rebuild_done(libxl__egc *egc, > goto error_out; > } > > +#if defined (__i386__) || (__x86_64__)I would prefer to get the guest type (x86 or arm) from libxc if possible and use that instead of a define, different compilers tend to have different names for this macros.> + /* > + * Pass along the disk navtive protocol to disks so that the protocol can > + * be written to frontend Xenstore node. This is a workaround for old PV > + * kernels before 2.6.26. > + * > + * Newer blkfront will write that node itself. In that case frontend just > + * rewrites the node as it sees fit. > + * > + * Note that Xend actually propagate this vaule to all frontends including > + * console and vifs. As now this is only needed for disk frontend so here > + * we minimize the impact. > + * > + * Presumably ARM guests don''t have this problem, so this snippet is only > + * compiled for X86 target. > + */ > + > + if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) {There is no PVHVM (or HVM with PV drivers) support on Linux version 2.6.26 and earlier?> + int i; > + uint32_t guest_width; > + const char *protocol = NULL; > + > + ret = xc_domain_getwidth(CTX->xch, domid, &guest_width); > + if (ret) { > + ret = ERROR_FAIL; > + goto error_out; > + } > + > + switch (guest_width) { > + case 32: /* 32 bit guest */ > + protocol = XEN_IO_PROTO_ABI_X86_32; > + break; > + case 64: /* 64 bit guest */ > + protocol = XEN_IO_PROTO_ABI_X86_64; > + break; > + default: > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > + "invalid address size for domain: %u", > + guest_width); > + ret = ERROR_FAIL; > + goto error_out; > + } > + > + for (i = 0; i < d_config->num_disks; i++) > + d_config->disks[i].native_protocol = strdup(protocol); > + } > +#endif > + > store_libxl_entry(gc, domid, &d_config->b_info); > > libxl__multidev_begin(ao, &dcs->multidev); > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 6cb6de6..22622a9 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -274,7 +274,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ > ("disable_migrate", libxl_defbool), > ("cpuid", libxl_cpuid_policy_list), > ("blkdev_start", string), > - > + > ("device_model_version", libxl_device_model_version), > ("device_model_stubdomain", libxl_defbool), > # if you set device_model you must set device_model_version too > @@ -318,9 +318,9 @@ libxl_domain_build_info = Struct("domain_build_info",[ > ("keymap", string), > ("sdl", libxl_sdl_info), > ("spice", libxl_spice_info), > - > + > ("gfx_passthru", libxl_defbool), > - > + > ("serial", string), > ("boot", string), > ("usb", libxl_defbool), > @@ -371,6 +371,7 @@ libxl_device_disk = Struct("device_disk", [ > ("removable", integer), > ("readwrite", integer), > ("is_cdrom", integer), > + ("native_protocol", string), > ]) > > libxl_device_nic = Struct("device_nic", [ >
On Wed, Apr 24, 2013 at 09:57:28AM +0100, Ian Campbell wrote: [...]> > + * > > + * Presumably ARM guests don''t have this problem, so this snippet is only > > + * compiled for X86 target. > > Yes, this isn''t required on ARM. > > Is there a reason why this is only done for PV guests? I''d expect at > least some older versions of the PV on HVM drivers to also need this > node. What did Xend do in this regard?My question, does Linux prior to 2.6.26 support PV on HVM?> > I think it would be preferable to have the logic for determining the > protocol in libxc, rather than exposing the guest width and putting the > logic in libxl. Mostly because on domain build it is also libxc which > makes this determination and keeping it in the same library makes sense > to me. > > What I''d actually prefer is to use the value of > dom->arch_hooks.native_protocol in libxl__build_pv when building a new > domain and have xc_domain_restore include the protocol as an output when > restoring so that libxl can use that in the restore case. This might be > more plumbing than we are willing to do at this stage of the release > though, so if you remove the bits which expose the protocol in the libxl > API (see below) and make this purely internal functionality of libxl+ > libxc we could live with an explicit query for the protocol. >I did use arch_hooks.native_protocol in my first implementation but later found out that it is impossible to get this value when doing restore. I also tried to avoid altering existing restore API as you pointed out we are now close to release. So I just came up with exposing guest width to libxl. :-(> > + */ > > + > > + if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) { > > + int i; > > + uint32_t guest_width; > > + const char *protocol = NULL; > > + > > + ret = xc_domain_getwidth(CTX->xch, domid, &guest_width); > > + if (ret) { > > + ret = ERROR_FAIL; > > + goto error_out; > > + } > > + > > + switch (guest_width) { > > + case 32: /* 32 bit guest */ > > + protocol = XEN_IO_PROTO_ABI_X86_32; > > + break; > > + case 64: /* 64 bit guest */ > > + protocol = XEN_IO_PROTO_ABI_X86_64; > > + break; > > + default: > > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > > + "invalid address size for domain: %u", > > + guest_width); > > + ret = ERROR_FAIL; > > + goto error_out; > > + } > > + > > + for (i = 0; i < d_config->num_disks; i++) > > + d_config->disks[i].native_protocol = strdup(protocol); > > If you are going to expose native_protocol to the libxl user then you > need to allow for them to have set it. > > However I don''t think we want/need to expose this to the user, > device_disk_add() can just query and write the string in one go. Or it > could be queried once for the domain early on and remembered in some > handy place (either a suitable datastructure or in xenstore) >I think querying when adding disk is a good idea.> Did Xend only do this for disks, or did it do it for all devices? >It does this for all devices, but only very old disk frontend relies on this AFAICT. Wei.
On Wed, 2013-04-24 at 10:13 +0100, Wei Liu wrote:> On Wed, Apr 24, 2013 at 09:57:28AM +0100, Ian Campbell wrote: > [...] > > > + * > > > + * Presumably ARM guests don''t have this problem, so this snippet is only > > > + * compiled for X86 target. > > > > Yes, this isn''t required on ARM. > > > > Is there a reason why this is only done for PV guests? I''d expect at > > least some older versions of the PV on HVM drivers to also need this > > node. What did Xend do in this regard? > > My question, does Linux prior to 2.6.26 support PV on HVM?Not in tree, but you can get out-of-tree modules for various old versions, see the unmodified_drivers tree in the Xen source. They support from something ancient (like 2.6.9) up to 2.6.2x. They effectively stop supporting versions when pvops arrived I think, so there is a gap between these drivers and when PVHVM was added to mainline Linux. However I''d be more inclined to follow what Xend did here rather than try for wider coverage "just because".> > I think it would be preferable to have the logic for determining the > > protocol in libxc, rather than exposing the guest width and putting the > > logic in libxl. Mostly because on domain build it is also libxc which > > makes this determination and keeping it in the same library makes sense > > to me. > > > > What I''d actually prefer is to use the value of > > dom->arch_hooks.native_protocol in libxl__build_pv when building a new > > domain and have xc_domain_restore include the protocol as an output when > > restoring so that libxl can use that in the restore case. This might be > > more plumbing than we are willing to do at this stage of the release > > though, so if you remove the bits which expose the protocol in the libxl > > API (see below) and make this purely internal functionality of libxl+ > > libxc we could live with an explicit query for the protocol. > > > > I did use arch_hooks.native_protocol in my first implementation but > later found out that it is impossible to get this value when doing > restore. I also tried to avoid altering existing restore APIIn general changing the libxc APIs where necessary is absolutely fine -- we don''t make any guarantees about that interface. But ...> as you pointed out we are now close to release.... yes. If it were just a case of adding an option to xc_domain_restore I''d probably still push for it but since it will also involve plumbing things through the libxl-save-helper process and libxl callbacks that makes it a bit more involved.> So I just came up with exposing guest width to libxl. :-(I think just exposing a new function xc_domain_get_native_protocol would be OK wrt the freeze and preferable to exposing the guest width. [...]> > Did Xend only do this for disks, or did it do it for all devices? > > > > It does this for all devices, but only very old disk frontend relies on > this AFAICT.Disk is the only one I can think of where the ring structures are different for 32 vs 64.
On Wed, 2013-04-24 at 10:08 +0100, Roger Pau Monné wrote:> On 23/04/13 22:25, Wei Liu wrote: > > +#if defined (__i386__) || (__x86_64__) > > I would prefer to get the guest type (x86 or arm) from libxc if possible > and use that instead of a define, different compilers tend to have > different names for this macros.By pushing the logic down into libxc we can put the implementation in xc_dom_x86.c and a stub in xc_dom_arm.c and get this behaviour without the ifdefs. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Wed, Apr 24, 2013 at 10:24:57AM +0100, Ian Campbell wrote:> On Wed, 2013-04-24 at 10:08 +0100, Roger Pau Monné wrote: > > On 23/04/13 22:25, Wei Liu wrote: > > > +#if defined (__i386__) || (__x86_64__) > > > > I would prefer to get the guest type (x86 or arm) from libxc if possible > > and use that instead of a define, different compilers tend to have > > different names for this macros. > > By pushing the logic down into libxc we can put the implementation in > xc_dom_x86.c and a stub in xc_dom_arm.c and get this behaviour without > the ifdefs. >xc_dom_*.c is for domain builder, is it really the right place to put generic domain control / information query functions? I would prefer creating new files called xc_domain_x86.c and xc_domain_arm.c. Wei.> Ian. >
On Wed, 2013-04-24 at 12:33 +0100, Wei Liu wrote:> On Wed, Apr 24, 2013 at 10:24:57AM +0100, Ian Campbell wrote: > > On Wed, 2013-04-24 at 10:08 +0100, Roger Pau Monné wrote: > > > On 23/04/13 22:25, Wei Liu wrote: > > > > +#if defined (__i386__) || (__x86_64__) > > > > > > I would prefer to get the guest type (x86 or arm) from libxc if possible > > > and use that instead of a define, different compilers tend to have > > > different names for this macros. > > > > By pushing the logic down into libxc we can put the implementation in > > xc_dom_x86.c and a stub in xc_dom_arm.c and get this behaviour without > > the ifdefs. > > > > xc_dom_*.c is for domain builder, is it really the right place to put > generic domain control / information query functions?It's sort of consistent because its the domain builder which currently knows about the native protocol and returns it when building. e.g. those files contain the xc_dom_arch datastructures which map from guest type to native protocol.> I would prefer creating new files called xc_domain_x86.c and > xc_domain_arm.c. > > > Wei. > > > Ian. > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel