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