Ross Philipson
2013-Jan-18 21:40 UTC
[PATCH v1 01/02] HVM firmware passthrough libxl support
This patch introduces support for two new parameters in libxl: smbios_firmware=<path_to_smbios_structures_file> acpi_firmware=<path_to_acpi_tables_file> The changes are primarily in the domain building code where the firmware files are read and passed to libxc for loading into the new guest. After the domain building call to libxc, the addresses for the loaded blobs are returned and written to xenstore. Additionally, this patch switches to using the new xc_hvm_build() routine. Signed-off-by: Ross Philipson <ross.philipson@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2013-Jan-21 09:57 UTC
Re: [PATCH v1 01/02] HVM firmware passthrough libxl support
On Fri, 2013-01-18 at 21:40 +0000, Ross Philipson wrote:> This patch introduces support for two new parameters in libxl: > > smbios_firmware=<path_to_smbios_structures_file> > acpi_firmware=<path_to_acpi_tables_file> > > The changes are primarily in the domain building code where the firmware files > are read and passed to libxc for loading into the new guest. After the domain > building call to libxc, the addresses for the loaded blobs are returned and > written to xenstore. > > Additionally, this patch switches to using the new xc_hvm_build() routine. > > Signed-off-by: Ross Philipson <ross.philipson@citrix.com> > > > diff -r 35a0556a7f76 tools/libxl/libxl_dom.c > --- a/tools/libxl/libxl_dom.c Thu Jan 10 17:32:10 2013 +0000 > +++ b/tools/libxl/libxl_dom.c Fri Jan 18 15:21:50 2013 -0500 > @@ -21,6 +21,7 @@ > > #include <xc_dom.h> > #include <xen/hvm/hvm_info_table.h> > +#include <xen/hvm/hvm_xs_strings.h> > > libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid) > { > @@ -462,6 +463,7 @@ static unsigned long timer_mode(const li > mode <= LIBXL_TIMER_MODE_ONE_MISSED_TICK_PENDING); > return ((unsigned long)mode); > } > + > static int hvm_build_set_params(xc_interface *handle, uint32_t domid, > libxl_domain_build_info *info, > int store_evtchn, unsigned long *store_mfn, > @@ -510,11 +512,79 @@ static int hvm_build_set_params(xc_inter > return 0; > } > > -static const char *libxl__domain_firmware(libxl__gc *gc, > - libxl_domain_build_info *info) > +static int hvm_build_set_xs_values(libxl__gc *gc, > + uint32_t domid, > + struct xc_hvm_build_args *args) > +{ > + libxl_ctx *ctx = libxl__gc_owner(gc); > + char *path = NULL; > + int ret = 0; > + > + if (args->smbios_module.guest_addr_out) { > + path = libxl__sprintf(gc, "/local/domain/%d/"HVM_XS_SMBIOS_PT_ADDRESS, > + domid); > + if (!path) > + goto str_err;xl will panic on memory allocation failure so you don''t need to handle it yourself, which removes a lot of error handling. You could also use GCSPRINTF to shorten the line if you wanted> + > + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%"PRIx64, > + args->smbios_module.guest_addr_out); > + if (ret) > + goto xs_err; > + > + path = libxl__sprintf(gc, "/local/domain/%d/"HVM_XS_SMBIOS_PT_LENGTH, > + domid); > + if (!path) > + goto str_err; > + > + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%x", > + args->smbios_module.length); > + if (ret) > + goto xs_err; > + } > + > + if (args->acpi_module.guest_addr_out) { > + path = libxl__sprintf(gc, "/local/domain/%d/"HVM_XS_ACPI_PT_ADDRESS, > + domid); > + if (!path) > + goto str_err; > + > + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%"PRIx64, > + args->acpi_module.guest_addr_out); > + if (ret) > + goto xs_err; > + > + path = libxl__sprintf(gc, "/local/domain/%d/"HVM_XS_ACPI_PT_LENGTH, > + domid); > + if (!path) > + goto str_err; > + > + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%x", > + args->acpi_module.length); > + if (ret) > + goto xs_err; > + } > + > + return 0; > + > +xs_err: > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > + "failed to write firmware xenstore value, err: %d", ret);You can use the LOG macro to shorten these long lines too.> + return -1; > +str_err: > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > + "failed to get firmware xenstore path"); > + return -1; > +} > + > +static int libxl__domain_firmware(libxl__gc *gc, > + libxl_domain_build_info *info, > + struct xc_hvm_build_args *args) > { > libxl_ctx *ctx = libxl__gc_owner(gc); > const char *firmware; > + int e, rc = ERROR_FAIL; > + int datalen = 0; > + void *data = 0; > > if (info->u.hvm.firmware) > firmware = info->u.hvm.firmware; > @@ -530,11 +600,68 @@ static const char *libxl__domain_firmwar > default: > LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "invalid device model version %d", > info->device_model_version); > - return NULL; > + return -1; > break; > } > } > - return libxl__abs_path(gc, firmware, libxl__xenfirmwaredir_path()); > + args->image_file_name = libxl__abs_path(gc, firmware, > + libxl__xenfirmwaredir_path()); > + if (!args->image_file_name) { > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "invalid firmware path"); > + return -1; > + } > + > + if (info->u.hvm.smbios_firmware) { > + e = libxl_read_file_contents(ctx, info->u.hvm.smbios_firmware, > + &data, &datalen); > + if (e) { > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > + "failed to read SMBIOS firmware file %s", > + info->u.hvm.smbios_firmware); > + goto err; > + } > + if (!datalen) { > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > + "SMBIOS firmware file %s is empty", > + info->u.hvm.smbios_firmware); > + goto err; > + } > + args->smbios_module.data = data; > + args->smbios_module.length = (uint32_t)datalen; > + } > + > + if (info->u.hvm.acpi_firmware) { > + e = libxl_read_file_contents(ctx, info->u.hvm.acpi_firmware, > + &data, &datalen); > + if (e) { > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > + "failed to read ACPI firmware file %s", > + info->u.hvm.acpi_firmware); > + goto err; > + } > + if (!datalen) { > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > + "ACPI firmware file %s is empty", > + info->u.hvm.acpi_firmware); > + goto err; > + } > + args->acpi_module.data = data; > + args->acpi_module.length = (uint32_t)datalen; > + } > + > + rc = 0; > + goto out;Might as well return 0?> +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?> +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).> 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: > { > >
Ross Philipson
2013-Jan-21 17:05 UTC
Re: [PATCH v1 01/02] HVM firmware passthrough libxl support
> -----Original Message----- > From: Ian Campbell > Sent: Monday, January 21, 2013 4:57 AM > To: Ross Philipson > Cc: xen-devel@lists.xensource.com > Subject: Re: [Xen-devel] [PATCH v1 01/02] HVM firmware passthrough libxl > support > > On Fri, 2013-01-18 at 21:40 +0000, Ross Philipson wrote: > > This patch introduces support for two new parameters in libxl: > > > > smbios_firmware=<path_to_smbios_structures_file> > > acpi_firmware=<path_to_acpi_tables_file> > > > > The changes are primarily in the domain building code where the > firmware files > > are read and passed to libxc for loading into the new guest. After the > domain > > building call to libxc, the addresses for the loaded blobs are > returned and > > written to xenstore. > > > > Additionally, this patch switches to using the new xc_hvm_build() > routine. > > > > Signed-off-by: Ross Philipson <ross.philipson@citrix.com> > > > > > > diff -r 35a0556a7f76 tools/libxl/libxl_dom.c > > --- a/tools/libxl/libxl_dom.c Thu Jan 10 17:32:10 2013 +0000 > > +++ b/tools/libxl/libxl_dom.c Fri Jan 18 15:21:50 2013 -0500 > > @@ -21,6 +21,7 @@ > > > > #include <xc_dom.h> > > #include <xen/hvm/hvm_info_table.h> > > +#include <xen/hvm/hvm_xs_strings.h> > > > > libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid) > > { > > @@ -462,6 +463,7 @@ static unsigned long timer_mode(const li > > mode <= LIBXL_TIMER_MODE_ONE_MISSED_TICK_PENDING); > > return ((unsigned long)mode); > > } > > + > > static int hvm_build_set_params(xc_interface *handle, uint32_t domid, > > libxl_domain_build_info *info, > > int store_evtchn, unsigned long > *store_mfn, > > @@ -510,11 +512,79 @@ static int hvm_build_set_params(xc_inter > > return 0; > > } > > > > -static const char *libxl__domain_firmware(libxl__gc *gc, > > - libxl_domain_build_info > *info) > > +static int hvm_build_set_xs_values(libxl__gc *gc, > > + uint32_t domid, > > + struct xc_hvm_build_args *args) > > +{ > > + libxl_ctx *ctx = libxl__gc_owner(gc); > > + char *path = NULL; > > + int ret = 0; > > + > > + if (args->smbios_module.guest_addr_out) { > > + path = libxl__sprintf(gc, > "/local/domain/%d/"HVM_XS_SMBIOS_PT_ADDRESS, > > + domid); > > + if (!path) > > + goto str_err; > > xl will panic on memory allocation failure so you don''t need to handle > it yourself, which removes a lot of error handling. > > You could also use GCSPRINTF to shorten the line if you wantedI will change that and look at using GCSPRINTF. I sort of just copied the logging from elsewhere in that code.> > > + > > + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%"PRIx64, > > + args->smbios_module.guest_addr_out); > > + if (ret) > > + goto xs_err; > > + > > + path = libxl__sprintf(gc, > "/local/domain/%d/"HVM_XS_SMBIOS_PT_LENGTH, > > + domid); > > + if (!path) > > + goto str_err; > > + > > + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%x", > > + args->smbios_module.length); > > + if (ret) > > + goto xs_err; > > + } > > + > > + if (args->acpi_module.guest_addr_out) { > > + path = libxl__sprintf(gc, > "/local/domain/%d/"HVM_XS_ACPI_PT_ADDRESS, > > + domid); > > + if (!path) > > + goto str_err; > > + > > + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%"PRIx64, > > + args->acpi_module.guest_addr_out); > > + if (ret) > > + goto xs_err; > > + > > + path = libxl__sprintf(gc, > "/local/domain/%d/"HVM_XS_ACPI_PT_LENGTH, > > + domid); > > + if (!path) > > + goto str_err; > > + > > + ret = libxl__xs_write(gc, XBT_NULL, path, "0x%x", > > + args->acpi_module.length); > > + if (ret) > > + goto xs_err; > > + } > > + > > + return 0; > > + > > +xs_err: > > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > > + "failed to write firmware xenstore value, err: %d", > ret); > > You can use the LOG macro to shorten these long lines too.Will look into it.> > > + return -1; > > +str_err: > > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > > + "failed to get firmware xenstore path"); > > + return -1; > > +} > > + > > +static int libxl__domain_firmware(libxl__gc *gc, > > + libxl_domain_build_info *info, > > + struct xc_hvm_build_args *args) > > { > > libxl_ctx *ctx = libxl__gc_owner(gc); > > const char *firmware; > > + int e, rc = ERROR_FAIL; > > + int datalen = 0; > > + void *data = 0; > > > > if (info->u.hvm.firmware) > > firmware = info->u.hvm.firmware; > > @@ -530,11 +600,68 @@ static const char *libxl__domain_firmwar > > default: > > LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "invalid device model > version %d", > > info->device_model_version); > > - return NULL; > > + return -1; > > break; > > } > > } > > - return libxl__abs_path(gc, firmware, > libxl__xenfirmwaredir_path()); > > + args->image_file_name = libxl__abs_path(gc, firmware, > > + > libxl__xenfirmwaredir_path()); > > + if (!args->image_file_name) { > > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "invalid firmware path"); > > + return -1; > > + } > > + > > + if (info->u.hvm.smbios_firmware) { > > + e = libxl_read_file_contents(ctx, info- > >u.hvm.smbios_firmware, > > + &data, &datalen); > > + if (e) { > > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > > + "failed to read SMBIOS firmware file %s", > > + info->u.hvm.smbios_firmware); > > + goto err; > > + } > > + if (!datalen) { > > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > > + "SMBIOS firmware file %s is empty", > > + info->u.hvm.smbios_firmware); > > + goto err; > > + } > > + args->smbios_module.data = data; > > + args->smbios_module.length = (uint32_t)datalen; > > + } > > + > > + if (info->u.hvm.acpi_firmware) { > > + e = libxl_read_file_contents(ctx, info->u.hvm.acpi_firmware, > > + &data, &datalen); > > + if (e) { > > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > > + "failed to read ACPI firmware file %s", > > + info->u.hvm.acpi_firmware); > > + goto err; > > + } > > + if (!datalen) { > > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, > > + "ACPI firmware file %s is empty", > > + info->u.hvm.acpi_firmware); > > + goto err; > > + } > > + args->acpi_module.data = data; > > + args->acpi_module.length = (uint32_t)datalen; > > + } > > + > > + rc = 0; > > + goto out; > > Might as well return 0?Yup.> > > +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.> > > +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 guess this would be the first instance of a LIBXL_HAVE_* from the looks of it.> > > 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: > > { > > > >
Ian Campbell
2013-Jan-21 17:12 UTC
Re: [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).> > > > > > +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.> > > > > > 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: > > > { > > > > > > >
Ross Philipson
2013-Jan-21 18:35 UTC
Re: [PATCH v1 01/02] HVM firmware passthrough libxl support
> -----Original Message----- > From: Ian Campbell > Sent: Monday, January 21, 2013 12:12 PM > To: Ross Philipson > Cc: xen-devel@lists.xensource.com > 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: > > > > { > > > > > > > > > > >
Ian Campbell
2013-Jan-22 09:32 UTC
Re: [PATCH v1 01/02] HVM firmware passthrough libxl support
On Mon, 2013-01-21 at 18:35 +0000, Ross Philipson wrote:> > 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?I guest neither GCNEW nor GCNEW_ARRAY do what you need do they, so yes I think that would be fine.> 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?Nope, that it. Just enough so that users can do: if (opt = get_opt("firmware")) { #ifdef LIBXL_HAVE_FIRMWARE_PASSTHROUGH d_config->....firmware = opt; #else fprintf(stderr, "Firmware not supported\n"); #endif } Ian.
Ross Philipson
2013-Jan-22 15:47 UTC
Re: [PATCH v1 01/02] HVM firmware passthrough libxl support
-----Original Message-----> From: Ian Campbell > Sent: Tuesday, January 22, 2013 4:32 AM > To: Ross Philipson > Cc: xen-devel@lists.xensource.com > Subject: Re: [Xen-devel] [PATCH v1 01/02] HVM firmware passthrough libxl > support > > On Mon, 2013-01-21 at 18:35 +0000, Ross Philipson wrote: > > > 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? > > I guest neither GCNEW nor GCNEW_ARRAY do what you need do they, so yes I > think that would be fine. > > > 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? > > Nope, that it. Just enough so that users can do: > > if (opt = get_opt("firmware")) > { > #ifdef LIBXL_HAVE_FIRMWARE_PASSTHROUGH > d_config->....firmware = opt; > #else > fprintf(stderr, "Firmware not supported\n"); > #endif > } > > Ian.Great, thanks. I should be able to resubmit this week.
Seemingly Similar Threads
- [PATCH v2 02/03] HVM firmware passthrough libxl support
- [PATCH V2 0/3] Set VNC password to QEMU upstream
- [PATCH v10 0/7] build upstream qemu and seabios by default
- [PATCH] libxl: do not overwrite user supplied config when running bootloader
- XEN/arm XENFB support