[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v1 01/02] HVM firmware passthrough libxl support



> -----Original Message-----
> From: Ian Campbell
> Sent: Monday, January 21, 2013 4:57 AM
> To: Ross Philipson
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: 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

I will change that and look at using GCSPRINTF. I sort of just copied the
logging from elsewhere in that code.

> 
> > +
> > +        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.

Will look into it.

> 
> > +    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?

Yup.

> 
> > +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?

The previous code did not clean up the values returned from
libxl__abs_path() so I assumed it was like strings returned
from libxl__sprintf(), ie cleaned up by the GC.

> 
> > +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).

I think I understand what you are suggesting (having read the comment
in libxl.h). I guess the thing that really breaks backward compat is
the code in libxl_dom.c that calls libxc. Should I introduce a
LIBXL_HAVE_FIRMWARE_PASSTHROUGH and have an additional block in
Libxl_dom.c with the old code that calls xc_hvm_build_target_mem()?

I guess this would be the first instance of a LIBXL_HAVE_* from the
looks of it.

> 
> > 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®.