Xen devel - Feb 2013 - [PATCH v2 02/03] 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.

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@citrix.com>




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

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@citrix.com>
> 
> 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;
>  }

> -----Original Message-----
> From: Ian Campbell
> Sent: Monday, February 04, 2013 5:58 AM
> To: Ross Philipson
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH v2 02/03] HVM firmware passthrough libxl
> support
> 
> 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@citrix.com>
> >
> > 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?
I can do that though I think it would be better to silently drop
the table (which would have the same outcome ultimately). 
> 
> > +            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?
Yea I thought about this but the file read routine was used in a
number of places and it might not have been all that easy to ensure
that I tested all of them so I decided to leave libxl__read_file_contents
alone as the safer route.

I will fix the other issues noted in there also.
> 
> 
> > +        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;
> >  }
>