| Message ID | 1457978150-27201-6-git-send-email-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Tue, 2016-03-15 at 20:53 -0400, Konrad Rzeszutek Wilk wrote: > On Mon, Mar 14, 2016 at 05:55:40PM +0000, Anthony PERARD wrote: > > > > --- 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' ? > Or 'bios_path_override' ? Wow, a French, an Italian and a Polish arguing about best looking English-ish words... This must be a (bad) joke! :-P :-P > > + > > +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=>? > AFAIUI, B<bios=> does not contain an exact file name, but the indication of what kind of BIOS we want. Therefore, referencing it directly like this may be confusing (IMHO). So, maybe something like: "Override the path at which to look for the BIOS binary. Such binary MUST be consistent with what has been specified in B<bios=>." > Or is this the full path including the name?? In which case should it > mention that B<bios=> is overriden? > If that is the case, indeed. > > --- 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? > Hehe... I'd also go for 'r', and call the (or at least some of the) existing users not codying style compliant, but it's indeed a funny case, and we should hear from maintainers what they prefer. :-) Regards, Dario
On Wed, Mar 16, 2016 at 10:27:25AM +0100, Dario Faggioli wrote: > On Tue, 2016-03-15 at 20:53 -0400, Konrad Rzeszutek Wilk wrote: > > On Mon, Mar 14, 2016 at 05:55:40PM +0000, Anthony PERARD wrote: > > > > > > --- 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' ? > > > Or 'bios_path_override' ? > > Wow, a French, an Italian and a Polish arguing about best looking > English-ish words... This must be a (bad) joke! :-P :-P :) To be honest, I just try to do the same as what is done for "device_model_override". I even copied over the description, replacing "binary" by "blob". Maybe bios_path_override would be a good name, would this name can be describe as "override bios_path"? At least that would not make someone beleave that the bios= config option itself is overridden. > > > + > > > +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=>? > > > AFAIUI, B<bios=> does not contain an exact file name, but the > indication of what kind of BIOS we want. Therefore, referencing it > directly like this may be confusing (IMHO). That's right, value can only be "rombios" or "seabios" or "ovmf". > So, maybe something like: "Override the path at which to look for the > BIOS binary. Such binary MUST be consistent with what has been > specified in B<bios=>." > > > Or is this the full path including the name?? In which case should it > > mention that B<bios=> is overriden? > > > If that is the case, indeed. Yes this new config option is the full path. But it does not override bios=. If someone start a guest with this: bios="seabios" bios_override="/usr/share/ovmf/ovmf_x64.bin" it's going to fail.
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"> + +Override the path to the blob to be used as BIOS. The blob provided here MUST +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 +device_model_version="qemu-xen-traditional". + =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; + + 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 + * 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);
The path to the BIOS blob can be override by the xl's bios_override option, 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(-)