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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-04-26 at 10:44 +0100, Wei Liu wrote:
> On Fri, Apr 26, 2013 at 10:18:51AM +0100, Ian Campbell wrote:
> > On Thu, 2013-04-25 at 22:36 +0100, Wei Liu wrote:
> > > @@ -2156,6 +2166,24 @@ 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 kernel disk frontends 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) {
> > > +            protocol = xc_domain_get_native_protocol(ctx->xch, domid);
> > 
> > You can declare protocol here instead of globally in the function. The =
> > NULL is also redundant.
> > 
> > > +            if (protocol) {
> > > +                strncpy(p, protocol, strlen(protocol));
> > > +                p[strlen(protocol)] = 0;
> > > +                flexarray_append(front, "protocol");
> > > +                flexarray_append(front, p);
> > 
> > GCSPRINTF would be the right way to do this, but actually you can pass
> > protocol directly to flexarray_append, that is fine since the flexarray
> > never frees the values which it contains so they either need to be gc'd,
> > string constants or managed manually.
> > 
> 
> Oh thanks for the hint. This saves serveral lines of code. The (void *)
> argument of flexarray_append misled me. ;-)
> 
> Below is updated version of the patch. Casting "protocol" to (void *) is
> necessary to have it compiled. Otherwise gcc complains we discard the
> const quilifier.

Oh, sorry, if this is the case the libxl__strdup(gc,protocol) is better
than the cast. Sorry for misleading :-(

Now you mention it I do seem to recall trying to correctly constify
flexarray's at one point and not quite being able to make it work.

Ian.

> 
> 
> Wei.
> 
> -----------8<--------
> commit 7961cfe75b1282a2d27163a04ff268cedf4919b5
> Author: Wei Liu <wei.liu2@xxxxxxxxxx>
> Date:   Wed Apr 24 19:00:01 2013 +0100
> 
>     libxl: write IO ABI for disk frontends
>     
>     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>
> 
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index 041832e..aaf35ca 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -29,6 +29,13 @@
>  #define CONSOLE_PFN_OFFSET 0
>  #define XENSTORE_PFN_OFFSET 1
>  
> +/* get guest IO ABI protocol */
> +const char *xc_domain_get_native_protocol(xc_interface *xch,
> +                                          uint32_t domid)
> +{
> +    return XEN_IO_PROTO_ABI_ARM;
> +}
> +
>  /* ------------------------------------------------------------------------ 
> */
>  /*
>   * arm guests are hybrid and start off with paging disabled, therefore no
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index eb9ac07..84a8de6 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -47,6 +47,40 @@
>  #define round_down(addr, mask)   ((addr) & ~(mask))
>  #define round_up(addr, mask)     ((addr) | (mask))
>  
> +/* get guest IO ABI protocol */
> +const char *xc_domain_get_native_protocol(xc_interface *xch,
> +                                          uint32_t domid)
> +{
> +    int ret;
> +    uint32_t guest_width;
> +    const char *protocol;
> +    DECLARE_DOMCTL;
> +
> +    memset(&domctl, 0, sizeof(domctl));
> +    domctl.domain = domid;
> +    domctl.cmd = XEN_DOMCTL_get_address_size;
> +
> +    ret = do_domctl(xch, &domctl);
> +
> +    if ( ret )
> +        return NULL;
> +
> +    guest_width = domctl.u.address_size.size;
> +
> +    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:
> +        protocol = NULL;
> +    }
> +
> +    return protocol;
> +}
> +
>  static unsigned long
>  nr_page_tables(struct xc_dom_image *dom,
>                 xen_vaddr_t start, xen_vaddr_t end, unsigned long bits)
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 50853af..2f32151 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -637,6 +637,16 @@ int xc_domain_hvm_setcontext(xc_interface *xch,
>                               uint32_t size);
>  
>  /**
> + * This function will return guest IO ABI protocol
> + *
> + * @parm xch a handle to an open hypervisor interface
> + * @parm domid the domain to get IO ABI protocol for
> + * @return guest protocol on success, NULL on failure
> + */
> +const char *xc_domain_get_native_protocol(xc_interface *xch,
> +                                          uint32_t domid);
> +
> +/**
>   * 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..9013932 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2038,6 +2038,12 @@ static void device_disk_add(libxl__egc *egc, uint32_t 
> domid,
>      libxl_ctx *ctx = gc->owner;
>      xs_transaction_t t = XBT_NULL;
>  
> +    libxl_domain_type type = libxl__domain_type(gc, domid);
> +    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
>      for (;;) {
>          rc = libxl__xs_transaction_start(gc, &t);
>          if (rc) goto out;
> @@ -2156,6 +2162,23 @@ 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 kernel disk frontends 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) {
> +            const char *protocol =
> +                xc_domain_get_native_protocol(ctx->xch, domid);
> +            if (protocol) {
> +                flexarray_append(front, "protocol");
> +                flexarray_append(front, (void *)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));



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