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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> Â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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel