Xen devel - Feb 2013 - [PATCH v2 01/03] HVM firmware passthrough libxl support

Switch libxl to use the new xc_hvm_build() libxc API.

Signed-off-by: Ross Philipson <ross.philipson@citrix.com>





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

On Fri, 2013-02-01 at 20:15 +0000, Ross Philipson wrote:
> Switch libxl to use the new xc_hvm_build() libxc API.
> 
> Signed-off-by: Ross Philipson <ross.philipson@citrix.com>
> 
> diff -r 27778b4099ba tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c   Fri Jan 25 15:04:11 2013 +0000
> +++ b/tools/libxl/libxl_dom.c   Fri Jan 25 13:37:55 2013 -0500
> @@ -542,17 +542,24 @@ int libxl__build_hvm(libxl__gc *gc, uint
>                libxl__domain_build_state *state)
>  {
>      libxl_ctx *ctx = libxl__gc_owner(gc);
> +    struct xc_hvm_build_args args = {};
>      int ret, rc = ERROR_FAIL;
>      const char *firmware = libxl__domain_firmware(gc, info);
>  
>      if (!firmware)
>          goto out;
> -    ret = xc_hvm_build_target_mem(
> -        ctx->xch,
> -        domid,
> -        (info->max_memkb - info->video_memkb) / 1024,
> -        (info->target_memkb - info->video_memkb) / 1024,
> -        firmware);
> +
> +    memset(&args, 0, sizeof(struct xc_hvm_build_args));
> +    /* The memory size names are misleading. The params are in Mb then
> +     * multiplied by 1 Kb.
What''s misleading about max_memkb? It contains kb doesn''t it?
Likewise
the others.

The actual code in this patch looks good to me. Assuming I''ve
remembered
the precedence of cast vs << correctly.
>  This was then divided off when calling
> +     * the old xc_hvm_build_target_mem() which then turned them to bytes.
> +     * Do all this in one step here...
> +     */
> +    args.mem_size = (uint64_t)(info->max_memkb - info->video_memkb)
<< 10;
> +    args.mem_target = (uint64_t)(info->target_memkb -
info->video_memkb) << 10;
> +    args.image_file_name = firmware;
> +
> +    ret = xc_hvm_build(ctx->xch, domid, &args);
>      if (ret) {
>          LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret, "hvm building
failed");
>          goto out;

I don''t suppose you could arrange for your replies to be threaded with
the 0/N mail could you? It helps keep them together in my queue, which I
useful when I come to apply a series which has had a few revisions.

hg email and git send-email can both do this for you.
http://wiki.xen.org/wiki/Submitting_Xen_Patches has some tips for hg.
http://wiki.xen.org/wiki/Submitting_Xen_Patches_with_Git some for git.

Ian.

> -----Original Message-----
> From: Ian Campbell
> Sent: Monday, February 04, 2013 5:43 AM
> To: Ross Philipson
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH v2 01/03] HVM firmware passthrough libxl
> support
> 
> On Fri, 2013-02-01 at 20:15 +0000, Ross Philipson wrote:
> > Switch libxl to use the new xc_hvm_build() libxc API.
> >
> > Signed-off-by: Ross Philipson <ross.philipson@citrix.com>
> >
> > diff -r 27778b4099ba tools/libxl/libxl_dom.c
> > --- a/tools/libxl/libxl_dom.c   Fri Jan 25 15:04:11 2013 +0000
> > +++ b/tools/libxl/libxl_dom.c   Fri Jan 25 13:37:55 2013 -0500
> > @@ -542,17 +542,24 @@ int libxl__build_hvm(libxl__gc *gc, uint
> >                libxl__domain_build_state *state)
> >  {
> >      libxl_ctx *ctx = libxl__gc_owner(gc);
> > +    struct xc_hvm_build_args args = {};
> >      int ret, rc = ERROR_FAIL;
> >      const char *firmware = libxl__domain_firmware(gc, info);
> >
> >      if (!firmware)
> >          goto out;
> > -    ret = xc_hvm_build_target_mem(
> > -        ctx->xch,
> > -        domid,
> > -        (info->max_memkb - info->video_memkb) / 1024,
> > -        (info->target_memkb - info->video_memkb) / 1024,
> > -        firmware);
> > +
> > +    memset(&args, 0, sizeof(struct xc_hvm_build_args));
> > +    /* The memory size names are misleading. The params are in Mb
> then
> > +     * multiplied by 1 Kb.
> 
> What''s misleading about max_memkb? It contains kb doesn''t
it? Likewise
> the others.
I guess it was just confusing when I first looked at it because it got
changed in 3 spots by the time it was passed to libxc. It is not
confusing now - we could just ditch the comment.
> 
> The actual code in this patch looks good to me. Assuming I''ve
remembered
> the precedence of cast vs << correctly.
> 
> >  This was then divided off when calling
> > +     * the old xc_hvm_build_target_mem() which then turned them to
> bytes.
> > +     * Do all this in one step here...
> > +     */
> > +    args.mem_size = (uint64_t)(info->max_memkb -
info->video_memkb)
> << 10;
> > +    args.mem_target = (uint64_t)(info->target_memkb - info-
> >video_memkb) << 10;
> > +    args.image_file_name = firmware;
> > +
> > +    ret = xc_hvm_build(ctx->xch, domid, &args);
> >      if (ret) {
> >          LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret, "hvm
building
> failed");
> >          goto out;

> -----Original Message-----
> From: Ian Campbell
> Sent: Monday, February 04, 2013 6:25 AM
> To: Ross Philipson
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH v2 01/03] HVM firmware passthrough libxl
> support
> 
> I don''t suppose you could arrange for your replies to be threaded
with
> the 0/N mail could you? It helps keep them together in my queue, which I
> useful when I come to apply a series which has had a few revisions.
> 
> hg email and git send-email can both do this for you.
> http://wiki.xen.org/wiki/Submitting_Xen_Patches has some tips for hg.
> http://wiki.xen.org/wiki/Submitting_Xen_Patches_with_Git some for git.
> 
> Ian.
Will do.