Re: [RFC PATCH v2 05/16] libxl: Load guest BIOS from file

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:
> The path to the BIOS blob can be override by the xl's bios_override
> option,
> or provided by u.hvm.bios_filename in the domain_build_info struct by
> other
> libxl user.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_dom.c     | 66
> +++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_types.idl |  1 +
>  tools/libxl/xl_cmdimpl.c    | 11 +++++---
>  3 files changed, 75 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index b40d744..27a0021 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -858,6 +858,42 @@ err:
>      return ret;
>  }
>  
> +static const char *seabios_path(libxl__gc *gc)
> +{
> +    return SEABIOS_PATH;
> +}
> +
> +static const char *ovmf_path(libxl__gc *gc)
> +{
> +    return OVMF_PATH;
> +}

Can you put these in libxl_paths.c (for consistency) please.

> +
> +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;
> +
> +    LOG(DEBUG, "Loading %s: %s", what, filename);
> +    e = libxl_read_file_contents(CTX, filename, &data, &datalen);
> +    if (e) {
> +        LOGEV(ERROR, e, "failed to read %s file %s",

"e" here should be an errno val, not a libxl ERROR_*. So you need to print
manually via the format string using either LOG or LOGE (if
libxl_read_file_contents also sets errno, probably not).


> +              what,
> +              filename);
> +        return ERROR_FAIL;
> +    }
> +    libxl__ptr_add(gc, data);
> +    if (datalen) {
> +        /* Only accept non-empty files */

Error or logging otherwise? Silently ignoring seems likely to surprise
someone eventually.

> +        m->data = data;
> +        m->length = (uint32_t)datalen;


I hope that cast isn't really necessary. 

> +    }
> +    return 0;
> +}
> +
>  static int libxl__domain_firmware(libxl__gc *gc,
>                                    libxl_domain_build_info *info,
>                                    struct xc_dom_image *dom)
> @@ -910,6 +946,36 @@ static int libxl__domain_firmware(libxl__gc *gc,
>          goto out;
>      }
>  
> +    if (info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_NONE) {
> +        const char *bios_filename;
> +        // Look for BIOS and load it

Proper comment style please.

> +        if (info->u.hvm.bios_filename) {
> +            bios_filename = info->u.hvm.bios_filename;
> +        } else {
> +            switch (info->u.hvm.bios)
> +            {
> +            case LIBXL_BIOS_TYPE_ROMBIOS:
> +                bios_filename = libxl__abs_path(gc, "rombios.bin",
> +                                                libxl__xenfirmwaredir_path());

The cover letter implied that rombios was still built in, this suggests
not?

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 082fed8..a3fbcab 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -468,6 +468,7 @@ libxl_domain_build_info =
> Struct("domain_build_info",[
>      ("u", KeyedUnion(None, libxl_domain_type, "type",
>                  [("hvm", Struct(None, [("firmware",         string),
>                                         ("bios",             libxl_bios_type),
> +                                       ("bios_filename",    string),

Such new additions require a #define LIBXL_HAVE_* in libxl.h so
applications know they can use it.



> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 840d73f..27d7c25 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1500,12 +1500,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_filename, 0);
> +        if (!xlu_cfg_get_string(config, "bios", &buf, 0)) {
> +            if (libxl_bios_type_from_string(buf, &b_info->u.hvm.bios)) {

Please update the xl.cfg man page to reflect this.

>                  fprintf(stderr, "ERROR: invalid value \"%s\" for \"bios\"\n",
>                      buf);
>                  exit (1);
> -        }
> +            }
> +        } else if (b_info->u.hvm.bios_filename)
> +            fprintf(stderr, "WARNING: "
> +                    "bios_override given without specific bios name\n");

What I'm about to say is probably not a good idea, but:

Given that we might keep rombios in-hvmloader (hence bios=rombios
bios_override!=NULL could be considered an error) can we get away with
saying that any use of bios_filename implies some sort of common non-
specific BIOS version?

Eyeballing the diff between tools/firmware/hvmloader/{seabios,ovmf}.c I
think the answer is already no and in any case doing this may tie our hands
in the future with some fancy new thing.

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