Re: [Xen-devel] [PATCH V2] libxl: write IO ABI for disk frontends

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-04-24 at 19:20 +0100, Wei Liu wrote:
> This is a patch to forward-port a Xend behaviour. Xend writes IO ABI used for
> all frontends. Blkfront before 2.6.26 relies on this behaviour otherwise guest
> cannot boot when running in 32-on-64 mode. Blkfront after 2.6.26 writes that
> node itself, in which case it's just an overwrite to an existing node which
> should be OK.
> 
> In fact Xend writes the ABI for all frontends including console and vif. But
> nowadays only old disk frontends rely on that behaviour so that we only write
> the ABI for disk frontends in libxl, minimizing the impact.
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Cc: Valtteri Kiviniemi <kiviniemi.valtteri@xxxxxxxxx>
> ---
>  tools/libxc/xc_dom_arm.c |   13 +++++++++++++
>  tools/libxc/xc_dom_x86.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxc/xenctrl.h    |   12 ++++++++++++
>  tools/libxl/libxl.c      |   25 +++++++++++++++++++++++++
>  4 files changed, 94 insertions(+)
> 
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index 041832e..acdac9e 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -29,6 +29,19 @@
>  #define CONSOLE_PFN_OFFSET 0
>  #define XENSTORE_PFN_OFFSET 1
>  
> +/* get guest IO ABI */
> +int xc_domain_get_native_protocol(xc_interface *xch,
> +                                  uint32_t domid,
> +                                  char **guest_abi);

With the correct placement of const in here I think you can avoid the
potential for memory allocation failure as well as the need for the
caller to free by just:
        *guest_abi = XEN_IO_PROTO_ABI_xxx

Perhaps even:
        const char *xc_domain_get_native_protocol(xch, domid)
        ...
                return XEN_IO_PROTO...

In the X86 case where the domctl can fail we can PERROR and return NULL,
since the caller doesn't really care why the ABI isn't available. Or
instead of NULL we can return the protocol of whichever bit width we are
compiler for. Probably NULL is safer though.

> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 50853af..83cca1f 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -637,6 +637,18 @@ int xc_domain_hvm_setcontext(xc_interface *xch,
>                               uint32_t size);
>  
>  /**
> + * 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_abi guest's navtive IO ABI string
> + * @return 0 on success, -ev on failure
> + */
> +int xc_domain_get_native_protocol(xc_interface *xch,
> +                                  uint32_t domid,
> +                                  char **guest_abi);
> +
> +/**
>   * 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..3f6afad 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2037,6 +2037,13 @@ static void device_disk_add(libxl__egc *egc, uint32_t 
> domid,
>      int rc;
>      libxl_ctx *ctx = gc->owner;
>      xs_transaction_t t = XBT_NULL;
> +    char *protocol = NULL;
> +
> +    libxl_domain_type type = libxl__domain_type(gc, domid);

Did we decide that Xend only does this for PV guests? Would there be any
harm in doing this for all PV disks, whether attached to PV or HVM
domains? Would remove some code ;-)

> +    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
>  
>      for (;;) {
>          rc = libxl__xs_transaction_start(gc, &t);
> @@ -2156,10 +2163,28 @@ 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. Xend does this, port
> +         * this behaviour to xl.
> +         *
> +         * New kernels write this node themselves. In that case it just
> +         * overwrites an existing node which is OK.
> +         */
> +        if (type == LIBXL_DOMAIN_TYPE_PV) {
> +            if (!xc_domain_get_native_protocol(ctx->xch, domid, &protocol)) {
> +                flexarray_append(front, "protocol");
> +                flexarray_append(front, 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));
>  
> +        if (protocol)
> +            free(protocol);
> +
>          rc = libxl__xs_transaction_commit(gc, &t);
>          if (!rc) break;
>          if (rc < 0) goto out;



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