[PATCH] libxl: write IO ABI for disk frontends

[PATCH] libxl: write IO ABI for disk frontends

[Wei Liu]

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

Re: [PATCH] libxl: write IO ABI for disk frontends

[Pasi Kärkkäinen]

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

Re: [PATCH] libxl: write IO ABI for disk frontends

[Wei Liu]

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.

Re: [PATCH] libxl: write IO ABI for disk frontends

[Ian Campbell]

> @@ -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

                              native

> +     * 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

                                                 value

> +     * 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", [

Re: [PATCH] libxl: write IO ABI for disk frontends

[Roger Pau Monné]

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", [
> 

Re: [PATCH] libxl: write IO ABI for disk frontends

[Wei Liu]

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.

Re: [PATCH] libxl: write IO ABI for disk frontends

[Ian Campbell]

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 API

In 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.

Re: [PATCH] libxl: write IO ABI for disk frontends

[Ian Campbell]

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

Re: [PATCH] libxl: write IO ABI for disk frontends

[Wei Liu]

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.
> 

Re: [PATCH] libxl: write IO ABI for disk frontends

[Ian Campbell]

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