[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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.