Re: [PATCH v4 05/14] libxl: Load guest BIOS from file

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Mar 14, 2016 at 05:55:40PM +0000, Anthony PERARD wrote:
> The path to the BIOS blob can be override by the xl's bios_override option,

s/override/overriden/

> or provided by u.hvm.bios_firmware in the domain_build_info struct by other
> libxl user.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> ---
> Change in V4:
> - updating man page to have bios_override described.
> - return ERROR_INVAL in libxl__load_hvm_firmware_module when the file is
>   empty.
> 
> Change in V3:
> - move seabios_path and ovmf_path to libxl_path.c (with renaming)
> - fix some coding style
> - warn for empty file
> - remove rombios stuff (will still be built-in hvmloader)
> - rename field bios_filename in domain_build_info to bios_firmware to
>   follow naming of acpi and smbios.
> - log an error after libxl_read_file_contents() only when it return ENOENT
> - return an error on empty file.
> - added #define LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
> ---
>  docs/man/xl.cfg.pod.5        |  9 +++++++
>  tools/libxl/libxl.h          |  8 +++++++
>  tools/libxl/libxl_dom.c      | 57 ++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_internal.h |  2 ++
>  tools/libxl/libxl_paths.c    | 10 ++++++++
>  tools/libxl/libxl_types.idl  |  1 +
>  tools/libxl/xl_cmdimpl.c     | 11 ++++++---
>  7 files changed, 95 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 56b1117..165915b 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1121,6 +1121,15 @@ Requires device_model_version=qemu-xen.
>  
>  =back
>  
> +=item B<bios_override="PATH">

Perhaps 'bios_override_path' ?

> +
> +Override the path to the blob to be used as BIOS. The blob provided here MUST

Perhaps:

Override the path to search for the B<bios=>?

Or is this the full path including the name?? In which case should it
mention that B<bios=> is overriden?


> +be consistent with the `bios` which you have specified. You should not normally
> +need to specify this option.
> +
> +This options does not have any effect if using bios="rombios" or

B<bios="rombios"> ?
> +device_model_version="qemu-xen-traditional".

And here too?

> +
>  =item B<pae=BOOLEAN>
>  
>  Hide or expose the IA32 Physical Address Extensions. These extensions
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index f9e3ef5..d06c6c5 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -884,6 +884,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
>  #define LIBXL_HAVE_CHECKPOINTED_STREAM 1
>  
>  /*
> + * LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
> + *
> + * libxl_domain_build_info has u.hvm.bios_firmware field which can be use
> + * to provide a different bios blob (like SeaBIOS or OVMF).
> + */
> +#define LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
> +
> +/*
>   * ERROR_REMUS_XXX error code only exists from Xen 4.5, Xen 4.6 and it
>   * is changed to ERROR_CHECKPOINT_XXX in Xen 4.7
>   */
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index b825b98..c112cc5 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -862,6 +862,38 @@ err:
>      return ret;
>  }
>  
> +static int libxl__load_hvm_firmware_module(libxl__gc *gc,
> +                                           const char *filename,
> +                                           const char *what,
> +                                           struct xc_hvm_firmware_module *m)
> +{
> +    int datalen = 0;
> +    void *data = NULL;
> +    int e;

That is interesting.

The CODING_STYLE in tools/libxl says:

 36   int rc;    /* a libxl error code - and not anything else */                   
 37   int r;     /* the return value from a system call (or libxc call) */          
 38   bool ok;   /* the success return value from a boolean function */             

And libxl_read_file_contents is quite weird. It does return an normal
errno value, so .. one could say it should be 'r'? But the existing users
of this are either 'e','ret' or 'rc'. 'rc' is not good, and 'ret'
is 'rc' is clearly wrong.

How confusing.

Ian, Wei, maybe you could clarify please?


> +
> +    LOG(DEBUG, "Loading %s: %s", what, filename);
> +    e = libxl_read_file_contents(CTX, filename, &data, &datalen);
> +    if (e) {
> +        /*
> +         * Print a message only on ENOENT, other error are logged by the

s/error/errors/
> +         * function libxl_read_file_contents().
> +         */
> +        if (e == ENOENT)
> +            LOGEV(ERROR, e, "failed to read %s file", what);
> +        return ERROR_FAIL;
> +    }
> +    libxl__ptr_add(gc, data);
> +    if (datalen) {
> +        /* Only accept non-empty files */
> +        m->data = data;
> +        m->length = datalen;
> +    } else {
> +        LOG(ERROR, "file %s for %s is empty", filename, what);
> +        return ERROR_INVAL;
> +    }
> +    return 0;
> +}
> +
>  static int libxl__domain_firmware(libxl__gc *gc,
>                                    libxl_domain_build_info *info,
>                                    struct xc_dom_image *dom)
> @@ -871,6 +903,7 @@ static int libxl__domain_firmware(libxl__gc *gc,
>      int e, rc;
>      int datalen = 0;
>      void *data;
> +    const char *bios_filename = NULL;
>  
>      if (info->u.hvm.firmware)
>          firmware = info->u.hvm.firmware;
> @@ -914,6 +947,30 @@ static int libxl__domain_firmware(libxl__gc *gc,
>          goto out;
>      }
>  
> +    if (info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
> +        if (info->u.hvm.bios_firmware) {
> +            bios_filename = info->u.hvm.bios_firmware;
> +        } else {
> +            switch (info->u.hvm.bios) {
> +            case LIBXL_BIOS_TYPE_SEABIOS:
> +                bios_filename = libxl__seabios_path();
> +                break;
> +            case LIBXL_BIOS_TYPE_OVMF:
> +                bios_filename = libxl__ovmf_path();
> +                break;
> +            case LIBXL_BIOS_TYPE_ROMBIOS:
> +            default:
> +                abort();
> +            }
> +        }
> +    }
> +
> +    if (bios_filename) {
> +        rc = libxl__load_hvm_firmware_module(gc, bios_filename, "BIOS",
> +                                             &dom->bios_module);
> +        if (rc) goto out;
> +    }
> +
>      if (info->u.hvm.smbios_firmware) {
>          data = NULL;
>          e = libxl_read_file_contents(ctx, info->u.hvm.smbios_firmware,
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 005fe53..af3ba9a 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2265,6 +2265,8 @@ _hidden const char *libxl__xen_config_dir_path(void);
>  _hidden const char *libxl__xen_script_dir_path(void);
>  _hidden const char *libxl__lock_dir_path(void);
>  _hidden const char *libxl__run_dir_path(void);
> +_hidden const char *libxl__seabios_path(void);
> +_hidden const char *libxl__ovmf_path(void);
>  
>  /*----- subprocess execution with timeout -----*/
>  
> diff --git a/tools/libxl/libxl_paths.c b/tools/libxl/libxl_paths.c
> index 9b7b0d5..6972b90 100644
> --- a/tools/libxl/libxl_paths.c
> +++ b/tools/libxl/libxl_paths.c
> @@ -35,6 +35,16 @@ const char *libxl__run_dir_path(void)
>      return XEN_RUN_DIR;
>  }
>  
> +const char *libxl__seabios_path(void)
> +{
> +    return SEABIOS_PATH;
> +}
> +
> +const char *libxl__ovmf_path(void)
> +{
> +    return OVMF_PATH;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 632c009..3978fd9 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -493,6 +493,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                         ("timer_mode",       libxl_timer_mode),
>                                         ("nested_hvm",       libxl_defbool),
>                                         ("altp2m",           libxl_defbool),
> +                                       ("bios_firmware",    string),
>                                         ("smbios_firmware",  string),
>                                         ("acpi_firmware",    string),
>                                         ("hdtype",           libxl_hdtype),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 990d3c9..08ceede 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1505,12 +1505,17 @@ static void parse_config_data(const char *config_source,
>  
>          xlu_cfg_replace_string (config, "firmware_override",
>                                  &b_info->u.hvm.firmware, 0);
> -        if (!xlu_cfg_get_string(config, "bios", &buf, 0) &&
> -            libxl_bios_type_from_string(buf, &b_info->u.hvm.bios)) {
> +        xlu_cfg_replace_string (config, "bios_override",
> +                                &b_info->u.hvm.bios_firmware, 0);
> +        if (!xlu_cfg_get_string(config, "bios", &buf, 0)) {
> +            if (libxl_bios_type_from_string(buf, &b_info->u.hvm.bios)) {
>                  fprintf(stderr, "ERROR: invalid value \"%s\" for \"bios\"\n",
>                      buf);
>                  exit (1);
> -        }
> +            }
> +        } else if (b_info->u.hvm.bios_firmware)
> +            fprintf(stderr, "WARNING: "
> +                    "bios_override given without specific bios name\n");
>  
>          xlu_cfg_get_defbool(config, "pae", &b_info->u.hvm.pae, 0);
>          xlu_cfg_get_defbool(config, "apic", &b_info->u.hvm.apic, 0);
> -- 
> Anthony PERARD
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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