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

[Ross Philipson <Ross.Philipson@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell
> Sent: Monday, February 04, 2013 5:58 AM
> To: Ross Philipson
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> 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@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?

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;
> >  }
> 

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