Re: [Xen-devel] [PATCH v2 02/03] HVM firmware passthrough libxl support

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-02-01 at 20:16 +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.
> 
> LIBXL_HAVE_FIRMWARE_PASSTHROUGH is defined in libxl.h to allow users to
> determine if the feature is present.
>  
> This patch also updates the xl.cfg man page with descriptions of the two new
> parameters for firmware passthrough.
> 
> Signed-off-by: Ross Philipson <ross.philipson@xxxxxxxxxx>
> 
> diff -r 25b28b76fc68 docs/man/xl.cfg.pod.5
> --- a/docs/man/xl.cfg.pod.5     Fri Feb 01 10:20:25 2013 -0500
> +++ b/docs/man/xl.cfg.pod.5     Fri Feb 01 11:24:21 2013 -0500
> @@ -829,6 +829,25 @@ libxl: 'host,tm=0,sse3=0'
>  More info about the CPUID instruction can be found in the processor manuals, 
> and
>  in Wikipedia: L<http://en.wikipedia.org/wiki/CPUID>
>  
> +=item B<acpi_firmware="STRING">
> +
> +Specify a path to a file that contains extra ACPI firmware tables to pass in 
> to
> +a guest. The file can contain several tables in their binary AML form
> +concatenated together. Each table self describes its length so no additional
> +information is needed. These tables will be added to the ACPI table set in 
> the
> +guest. Note that existing tables cannot be overriden by this feature. For

                                             "overridden"

(or is this a en_US vs en_GB thing?)

> +example this cannot be used to override tables like DSDT, FADT, etc.
> +
> +=item B<smbios_firmware="STRING">
> +
> +Specify a path to a file that contains extra SMBIOS firmware structures to 
> pass
> +in to a guest. The file can contain a set DMTF predefined structures which 
> will
> +override the internal defaults. Not all predefined structures can be 
> overriden,

overridden again?

> +only the following types: 0, 1, 2, 3, 11, 22, 39. The file can also contain 
> any
> +number of vendor defined SMBIOS structures (type 128 - 255). Since SMBIOS
> +structures do not present their overall size, each entry in the file must be
> +preceeded by a 32b integer indicating the size of the next structure.

   preceded

> +
>  =back 
>  
>  =head3 Guest Virtual Time Controls
> diff -r 25b28b76fc68 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h       Fri Feb 01 10:20:25 2013 -0500
> +++ b/tools/libxl/libxl.h       Fri Feb 01 11:24:21 2013 -0500
> @@ -68,6 +68,13 @@
>   */
>  
>  /*
> + * LIBXL_HAVE_FIRMWARE_PASSTHROUGH indicates the new feature for

"new" now but not forever ;-)

(ok, maybe that's nit picking)

> + * passing in SMBIOS and ACPI firmware to HVM guests is present
> + * in the library.
> + */
> +#define LIBXL_HAVE_FIRMWARE_PASSTHROUGH 1
> +
> +/*
>   * libxl ABI compatibility
>   *
>   * The only guarantee which libxl makes regarding ABI compatibility
> diff -r 25b28b76fc68 tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c   Fri Feb 01 10:20:25 2013 -0500
> +++ b/tools/libxl/libxl_dom.c   Fri Feb 01 11:24:21 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)
>  {
> @@ -510,11 +511,61 @@ 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)
> +{
> +    char *path = NULL;
> +    int ret = 0;
> +
> +    if (args->smbios_module.guest_addr_out) {
> +        path = GCSPRINTF("/local/domain/%d/"HVM_XS_SMBIOS_PT_ADDRESS, domid);
> +
> +        ret = libxl__xs_write(gc, XBT_NULL, path, "0x%"PRIx64,
> +                              args->smbios_module.guest_addr_out);
> +        if (ret)
> +            goto err;
> +
> +        path = GCSPRINTF("/local/domain/%d/"HVM_XS_SMBIOS_PT_LENGTH, domid);
> +
> +        ret = libxl__xs_write(gc, XBT_NULL, path, "0x%x",
> +                              args->smbios_module.length);
> +        if (ret)
> +            goto err;
> +    }
> +
> +    if (args->acpi_module.guest_addr_out) {
> +        path = GCSPRINTF("/local/domain/%d/"HVM_XS_ACPI_PT_ADDRESS, domid);
> +
> +        ret = libxl__xs_write(gc, XBT_NULL, path, "0x%"PRIx64,
> +                              args->acpi_module.guest_addr_out);
> +        if (ret)
> +            goto err;
> +
> +        path = GCSPRINTF("/local/domain/%d/"HVM_XS_ACPI_PT_LENGTH, domid);
> +
> +        ret = libxl__xs_write(gc, XBT_NULL, path, "0x%x",
> +                              args->acpi_module.length);
> +        if (ret)
> +            goto err;
> +    }
> +
> +    return 0;
> +
> +err:
> +    LOG(ERROR, "failed to write firmware xenstore value, err: %d", ret);
> +    return -1;

Why not propagate ret?

> +}
> +
> +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;
> @@ -528,13 +579,58 @@ static const char *libxl__domain_firmwar
>              firmware = "hvmloader";
>              break;
>          default:
> -            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "invalid device model version 
> %d",
> -                       info->device_model_version);
> -            return NULL;
> +            LOG(ERROR, "invalid device model version %d",
> +                info->device_model_version);
> +            return -1;

I think this makes this function return a mixture of ERROR_* (via rc)
and -1? Likewise in a few other places. Using ERROR_* consistently would
be preferable I think.

>              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) {

I think the only way this can fail is memory allocation failure, which
you needn't worry about.

> +        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) {
> +            LOG(ERROR, "failed to read SMBIOS firmware file %s",
> +                info->u.hvm.smbios_firmware);

e here is an errno value (I think) so I think you could use LOGEV.

> +            goto out;
> +        }
> +        if (!datalen) {
> +            LOG(ERROR, "SMBIOS firmware file %s is empty",
> +                info->u.hvm.smbios_firmware);

I suppose passing an empty firmware file doesn't make much sense. It
does it mean that a caller who is generating the table needs to check if
it actually produced anything though, might be easier to silently accept
an empty table?

> +            goto out;
> +        }
> +        libxl__ptr_add(gc, data);
> +        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) {
> +            LOG(ERROR, "failed to read ACPI firmware file %s",
> +                info->u.hvm.acpi_firmware);

LOGEV again.

> +            goto out;
> +        }
> +        if (!datalen) {
> +            LOG(ERROR, "ACPI firmware file %s is empty",
> +                info->u.hvm.acpi_firmware);
> +            goto out;
> +        }
> +        libxl__ptr_add(gc, data);

I suppose adding libxl__read_file_contents(gc, ...) might be overkill?


> +        args->acpi_module.data = data;
> +        args->acpi_module.length = (uint32_t)datalen;
> +    }
> +
> +    return 0;
> +out:
> +    return rc;
>  }
>  
>  int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
> @@ -557,22 +649,34 @@ int libxl__build_hvm(libxl__gc *gc, uint
[...]
>      ret = hvm_build_set_params(ctx->xch, domid, info, state->store_port,
>                                 &state->store_mfn, state->console_port,
>                                 &state->console_mfn, state->store_domid,
>                                 state->console_domid);
>      if (ret) {
> -        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret, "hvm build set 
> params failed");
> +        LOGEV(ERROR, ret, "hvm build set params failed");
>          goto out;
>      }
> -    rc = 0;
> +
> +    ret = hvm_build_set_xs_values(gc, domid, &args);
> +    if (ret) {
> +        LOGEV(ERROR, ret, "hvm build set xenstore values failed");

I'm not sure this guy actually returns an errnoval.

> +        goto out;
> +    }
> +
> +    return 0;
>  out:
>      return rc;
>  }



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