Re: [Xen-devel] [PATCH v2 01/03] HVM firmware passthrough libxl support

[Ross Philipson <Ross.Philipson@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell
> Sent: Monday, February 04, 2013 5:43 AM
> To: Ross Philipson
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxx>
> >
> > 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;

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