Re: [Xen-devel] [PATCH v1 01/02] HVM firmware passthrough libxl support

[Ross Philipson <Ross.Philipson@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell
> Sent: Monday, January 21, 2013 12:12 PM
> To: Ross Philipson
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v1 01/02] HVM firmware passthrough libxl
> support
> 
> On Mon, 2013-01-21 at 17:05 +0000, Ross Philipson wrote:
> 
> > I will change that and look at using GCSPRINTF. I sort of just copied
> the
> > logging from elsewhere in that code.
> 
> The GC* things are pretty new, likewise the shortened LOG macros.
> 
> 
> > >
> > > > +err:
> > > > +    if (args->smbios_module.data) {
> > > > +        free(args->smbios_module.data);
> > > > +        args->smbios_module.data = NULL;
> > > > +    }
> > > > +    if (args->acpi_module.data) {
> > > > +        free(args->acpi_module.data);
> > > > +        args->acpi_module.data = NULL;
> > > > +    }
> > >
> > > DO you leak args->image_file_name here?
> >
> > The previous code did not clean up the values returned from
> > libxl__abs_path() so I assumed it was like strings returned
> > from libxl__sprintf(), ie cleaned up by the GC.
> 
> You are right, I missed that this was a gc allocation.
> 
> You could add the module data to the gc too btw and have it take care of
> everything in that struct, arguably it is less confusing of each struct
> only contains one "kind" of pointer. (the other option is to pass NOGC
> to libxl__abs_path and manage that by hand too).

Ok I can switch all the items in that struct to ones that are gc'ed. I
noticed there was not GC* macro for a straight allocation. Would adding
a GCZALLOC for consistency be ok?

> 
> >
> > >
> > > > +out:
> > > > +    return rc;
> > > >  }
> > > >
> > > >  int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
> > >
> > > > diff -r 35a0556a7f76 tools/libxl/libxl_types.idl
> > > > --- a/tools/libxl/libxl_types.idl       Thu Jan 10 17:32:10 2013
> +0000
> > > > +++ b/tools/libxl/libxl_types.idl       Fri Jan 18 15:21:50 2013 -
> 0500
> > > > @@ -308,6 +308,8 @@ libxl_domain_build_info = Struct("domain
> > > >                                         ("vpt_align",
> > > libxl_defbool),
> > > >                                         ("timer_mode",
> > > libxl_timer_mode),
> > > >                                         ("nested_hvm",
> > > libxl_defbool),
> > > > +                                       ("smbios_firmware",
> string),
> > > > +                                       ("acpi_firmware",
> string),
> > > >                                         ("nographic",
> > > libxl_defbool),
> > > >                                         ("vga",
> > > libxl_vga_interface_info),
> > > >                                         ("vnc",
> > > libxl_vnc_info),
> > >
> > > I think we ought to have a LIBXL_HAVE_FOO define for these two, in
> > > libxl.h I suppose since the IDL doesn't support that sort of thing
> (not
> > > sure if it should or not).
> >
> > I think I understand what you are suggesting (having read the comment
> > in libxl.h). I guess the thing that really breaks backward compat is
> > the code in libxl_dom.c that calls libxc. Should I introduce a
> > LIBXL_HAVE_FIRMWARE_PASSTHROUGH and have an additional block in
> > Libxl_dom.c with the old code that calls xc_hvm_build_target_mem()?
> 
> I don't think that is necessary, all which is required is some way for
> users of libxl (like libvirt, or xl if it weren't in tree) to tell if
> they can try and use the new *_firmware fields or not. There's no need
> to vary the libxl implementation based on it.
> 
> > I guess this would be the first instance of a LIBXL_HAVE_* from the
> > looks of it.
> 
> Right.

I guess I misunderstood the use of the LIBXL_HAVE_* defines. So all I
really need is something like this in libxl.h with a comment about what
it is:
#define LIBXL_HAVE_FIRMWARE_PASSTHROUGH 1

Or am I still missing something?

> 
> >
> > >
> > > > diff -r 35a0556a7f76 tools/libxl/xl_cmdimpl.c
> > > > --- a/tools/libxl/xl_cmdimpl.c  Thu Jan 10 17:32:10 2013 +0000
> > > > +++ b/tools/libxl/xl_cmdimpl.c  Fri Jan 18 15:21:50 2013 -0500
> > > > @@ -882,6 +882,11 @@ static void parse_config_data(const char
> > > >          }
> > > >
> > > >          xlu_cfg_get_defbool(config, "nestedhvm", &b_info-
> > > >u.hvm.nested_hvm, 0);
> > > > +
> > > > +        xlu_cfg_replace_string (config, "smbios_firmware",
> > > > +                                &b_info->u.hvm.smbios_firmware,
> 0);
> > > > +        xlu_cfg_replace_string (config, "acpi_firmware",
> > > > +                                &b_info->u.hvm.acpi_firmware, 0);
> > > >          break;
> > > >      case LIBXL_DOMAIN_TYPE_PV:
> > > >      {
> > > >
> > > >
> >
> 

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