|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [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@xxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxx>
> >
> >
> > 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
I 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:
> > {
> >
> >
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |