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.per...@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
