[PATCH 0/4] Use libvirt firmware autoselection
Starting with 5.2.0, libvirt has a way to select the firmware by specifying its type, provided configuration files for the firmware are shipped. Currently we start the appliance as UEFI if any of the firmware are found, so instead we can try to just set the firmware type iff: - the libvirt autoselection works - the 'efi' firmware is available The only behaviour change is that the default firmware may be a non-debug version, so we get no debug messages even when running in verbose mode. This most probably will need an addition in libvirt to select this feature among the available firmwares. Pino Toscano (4): launch: libvirt: parse firmware autoselection lib: uefi: reset out parameters earlier lib: allow to use libvirt firmware autoselection lib: uefi: use the efi libvirt firmware if available lib/appliance-uefi.c | 32 ++++++++++++++++++++++++++------ lib/guestfs-internal.h | 2 +- lib/launch-direct.c | 3 ++- lib/launch-libvirt.c | 40 +++++++++++++++++++++++++++++++++++++++- 4 files changed, 68 insertions(+), 9 deletions(-) -- 2.24.1
Parse from the domain capabilities whether libvirt supports the autoselection of firmware, and which values are supported. --- lib/launch-libvirt.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c index 95aed8054..fa7b6987c 100644 --- a/lib/launch-libvirt.c +++ b/lib/launch-libvirt.c @@ -135,6 +135,11 @@ struct backend_libvirt_data { char *uefi_code; /* UEFI (firmware) code and variables. */ char *uefi_vars; char *default_qemu; /* default qemu (from domcapabilities) */ + char **firmware_autoselect; /* supported firmwares (from domcapabilities); + * NULL means "not supported", otherwise it + * contains a list with supported values for + * <os firmware='...'> + */ char guestfsd_path[UNIX_PATH_MAX]; /* paths to sockets */ char console_path[UNIX_PATH_MAX]; }; @@ -843,6 +848,8 @@ parse_domcapabilities (guestfs_h *g, const char *domcapabilities_xml, CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL; CLEANUP_XMLXPATHFREECONTEXT xmlXPathContextPtr xpathCtx = NULL; CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpathObj = NULL; + xmlNodeSetPtr nodes; + size_t i; doc = xmlReadMemory (domcapabilities_xml, strlen (domcapabilities_xml), NULL, NULL, XML_PARSE_NONET); @@ -869,6 +876,26 @@ parse_domcapabilities (guestfs_h *g, const char *domcapabilities_xml, assert (xpathObj->type == XPATH_STRING); data->default_qemu = safe_strdup (g, (char *) xpathObj->stringval); + /* + * This gives us whether the firmware autoselection is supported, + * and which values are allowed. + */ +#define XPATH_EXPR "/domainCapabilities/os/enum[@name='firmware']/value" + xpathObj = xmlXPathEvalExpression (BAD_CAST XPATH_EXPR, xpathCtx); + if (xpathObj == NULL) { + error (g, _("unable to evaluate XPath expression: %s"), XPATH_EXPR); + return -1; + } +#undef XPATH_EXPR + + assert (xpathObj->type == XPATH_NODESET); + nodes = xpathObj->nodesetval; + if (nodes != NULL) { + data->firmware_autoselect = safe_calloc (g, nodes->nodeNr + 1, sizeof (char *)); + for (i = 0; i < (size_t) nodes->nodeNr; ++i) + data->firmware_autoselect[i] = (char *) xmlNodeGetContent(nodes->nodeTab[i]); + } + return 0; } @@ -2072,6 +2099,9 @@ shutdown_libvirt (guestfs_h *g, void *datav, int check_for_errors) free (data->default_qemu); data->default_qemu = NULL; + guestfs_int_free_string_list (data->firmware_autoselect); + data->firmware_autoselect = NULL; + return ret; } -- 2.24.1
Make sure they are always reset, no matter the code branches later on. This is mostly code motion. --- lib/appliance-uefi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/appliance-uefi.c b/lib/appliance-uefi.c index 5fca8cd37..138a8d2f9 100644 --- a/lib/appliance-uefi.c +++ b/lib/appliance-uefi.c @@ -56,6 +56,9 @@ int guestfs_int_get_uefi (guestfs_h *g, char **code, char **vars, int *flags) { + *code = *vars = NULL; + *flags = 0; + #ifdef __aarch64__ size_t i; @@ -102,7 +105,5 @@ guestfs_int_get_uefi (guestfs_h *g, char **code, char **vars, int *flags) #endif /* Not found. */ - *code = *vars = NULL; - *flags = 0; return 0; } -- 2.24.1
Enhance the UEFi firmware lookup function with the information on the libvirt firmware autoselection, allowing it to return a value to use for the appliance. At the moment no firmware is selected this way, so there is no behaviour change. --- lib/appliance-uefi.c | 18 ++++++++++++++---- lib/guestfs-internal.h | 2 +- lib/launch-direct.c | 3 ++- lib/launch-libvirt.c | 10 +++++++++- 4 files changed, 26 insertions(+), 7 deletions(-) diff --git a/lib/appliance-uefi.c b/lib/appliance-uefi.c index 138a8d2f9..d2908867c 100644 --- a/lib/appliance-uefi.c +++ b/lib/appliance-uefi.c @@ -38,14 +38,20 @@ * is aarch64 only currently, since that's the only architecture where * UEFI is mandatory (and that only for RHEL). * + * C<firmwares> is an optional list of allowed values for the firmware + * autoselection of libvirt. It is C<NULL> to indicate it is not + * supported. C<*firmware> is set to one of the strings in + * C<firmwares> in case one can be used. + * * C<*code> is initialized with the path to the read-only UEFI code * file. C<*vars> is initialized with the path to a copy of the UEFI * vars file (which is cleaned up automatically on exit). * - * If C<*code> == C<*vars> == C<NULL> then no UEFI firmware is - * available. + * In case a UEFI firmare is available, either C<*firmware> is set to + * a non-C<NULL> value, or C<*code> and C<*vars> are. * - * C<*code> and C<*vars> should be freed by the caller. + * C<*code> and C<*vars> should be freed by the caller, and + * C<*firmware> B<must> not. * * If the function returns C<-1> then there was a real error which * should cause appliance building to fail (no UEFI firmware is not an @@ -54,10 +60,14 @@ * See also F<virt-v2v.git/v2v/utils.ml>:find_uefi_firmware */ int -guestfs_int_get_uefi (guestfs_h *g, char **code, char **vars, int *flags) +guestfs_int_get_uefi (guestfs_h *g, char *const *firmwares, + const char **firmware, char **code, char **vars, + int *flags) { *code = *vars = NULL; *flags = 0; + if (firmware) + *firmware = NULL; #ifdef __aarch64__ size_t i; diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h index 75b8a5c8e..6799c265d 100644 --- a/lib/guestfs-internal.h +++ b/lib/guestfs-internal.h @@ -691,7 +691,7 @@ extern char *guestfs_int_appliance_command_line (guestfs_h *g, const char *appli #define APPLIANCE_COMMAND_LINE_IS_TCG 1 /* appliance-uefi.c */ -extern int guestfs_int_get_uefi (guestfs_h *g, char **code, char **vars, int *flags); +extern int guestfs_int_get_uefi (guestfs_h *g, char *const *firmwares, const char **firmware, char **code, char **vars, int *flags); /* launch.c */ extern int64_t guestfs_int_timeval_diff (const struct timeval *x, const struct timeval *y); diff --git a/lib/launch-direct.c b/lib/launch-direct.c index ee2dcb884..ae6ca093b 100644 --- a/lib/launch-direct.c +++ b/lib/launch-direct.c @@ -533,7 +533,8 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) #endif /* UEFI (firmware) if required. */ - if (guestfs_int_get_uefi (g, &uefi_code, &uefi_vars, &uefi_flags) == -1) + if (guestfs_int_get_uefi (g, NULL, NULL, &uefi_code, &uefi_vars, + &uefi_flags) == -1) goto cleanup0; if (uefi_flags & UEFI_FLAG_SECURE_BOOT_REQUIRED) { /* Implementing this requires changes to the qemu command line. diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c index fa7b6987c..864eae314 100644 --- a/lib/launch-libvirt.c +++ b/lib/launch-libvirt.c @@ -140,6 +140,10 @@ struct backend_libvirt_data { * contains a list with supported values for * <os firmware='...'> */ + const char *firmware; /* firmware to set in autoselection mode; + * points to one of the elements in + * "firmware_autoselect" + */ char guestfsd_path[UNIX_PATH_MAX]; /* paths to sockets */ char console_path[UNIX_PATH_MAX]; }; @@ -442,7 +446,8 @@ launch_libvirt (guestfs_h *g, void *datav, const char *libvirt_uri) goto cleanup; /* UEFI code and variables, on architectures where that is required. */ - if (guestfs_int_get_uefi (g, &data->uefi_code, &data->uefi_vars, + if (guestfs_int_get_uefi (g, data->firmware_autoselect, &data->firmware, + &data->uefi_code, &data->uefi_vars, &uefi_flags) == -1) goto cleanup; if (uefi_flags & UEFI_FLAG_SECURE_BOOT_REQUIRED) { @@ -1207,6 +1212,9 @@ construct_libvirt_xml_boot (guestfs_h *g, cmdline = guestfs_int_appliance_command_line (g, params->appliance_dev, flags); start_element ("os") { + if (params->data->firmware) + attribute ("firmware", params->data->firmware); + start_element ("type") { #ifdef MACHINE_TYPE attribute ("machine", MACHINE_TYPE); -- 2.24.1
In case libvirt supports the firmware autoselection and there is an EFI firmware available, use it directly instead of handling the firmware manually. --- lib/appliance-uefi.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/appliance-uefi.c b/lib/appliance-uefi.c index d2908867c..0f07ed5a9 100644 --- a/lib/appliance-uefi.c +++ b/lib/appliance-uefi.c @@ -72,6 +72,15 @@ guestfs_int_get_uefi (guestfs_h *g, char *const *firmwares, #ifdef __aarch64__ size_t i; + if (firmwares && firmware) { + for (i = 0; firmwares[i] != NULL; ++i) { + if (STREQ(firmwares[i], "efi")) { + *firmware = firmwares[i]; + return 0; + } + } + } + for (i = 0; guestfs_int_uefi_aarch64_firmware[i].code != NULL; ++i) { const char *codefile = guestfs_int_uefi_aarch64_firmware[i].code; const char *code_debug_file = -- 2.24.1
On 1/16/20 1:27 PM, Pino Toscano wrote:
Starting with 5.2.0, libvirt has a way to select the firmware by specifying its type, provided configuration files for the firmware are shipped. Currently we start the appliance as UEFI if any of the firmware are found, so instead we can try to just set the firmware type iff: - the libvirt autoselection works - the 'efi' firmware is available
The only behaviour change is that the default firmware may be a non-debug version, so we get no debug messages even when running in verbose mode. This most probably will need an addition in libvirt to select this feature among the available firmwares.
Firmware autoselection parses JSON files at well defined locations giving each file precedence over others based on the path it's found under and it's name. The file name precedence is based on alphabetical ordering. That is why file names usually consists of two leading numbers, e.g. 40-bios.json, 50-ovmf-sb.json, 60-ovmf.json and so on. Now, the default, distro wide path where these files live is /usr/share/qemu/firmware/ and has the least precedence. If a sysadmin wants to alter config of some FW blob, instead of fighting with package manager and overwriting files under /usr/.. they can put changed versions under /etc/qemu/firmware which takes precedence over the former path. For instance, if 40-bios.json is found under both paths, only the one from /etc is considered and the other is not even parsed. If you would apply this logic one more time, you will get $HOME/.config/qemu/firmware which allows non-root users to override their sysadmin's config and have their private FW blobs with the highest priority. So if libguestfs wants libvirt to select certain FW image it can install its own JSON descriptor somewhere into a suitable path and if the file has the correct content it will direct libvirt into using FW blob with debug messages enabled.
Pino Toscano (4): launch: libvirt: parse firmware autoselection lib: uefi: reset out parameters earlier lib: allow to use libvirt firmware autoselection lib: uefi: use the efi libvirt firmware if available
lib/appliance-uefi.c | 32 ++++++++++++++++++++++++++------ lib/guestfs-internal.h | 2 +- lib/launch-direct.c | 3 ++- lib/launch-libvirt.c | 40 +++++++++++++++++++++++++++++++++++++++- 4 files changed, 68 insertions(+), 9 deletions(-)
I'm no libguestfs devel, but this looks sane to me. Michal
On Thu, Jan 16, 2020 at 01:49:09PM +0100, Michal Privoznik wrote:
On 1/16/20 1:27 PM, Pino Toscano wrote:
Starting with 5.2.0, libvirt has a way to select the firmware by specifying its type, provided configuration files for the firmware are shipped. Currently we start the appliance as UEFI if any of the firmware are found, so instead we can try to just set the firmware type iff: - the libvirt autoselection works - the 'efi' firmware is available
The only behaviour change is that the default firmware may be a non-debug version, so we get no debug messages even when running in verbose mode. This most probably will need an addition in libvirt to select this feature among the available firmwares.
Firmware autoselection parses JSON files at well defined locations giving each file precedence over others based on the path it's found under and it's name.
The file name precedence is based on alphabetical ordering. That is why file names usually consists of two leading numbers, e.g. 40-bios.json, 50-ovmf-sb.json, 60-ovmf.json and so on. Now, the default, distro wide path where these files live is /usr/share/qemu/firmware/ and has the least precedence. If a sysadmin wants to alter config of some FW blob, instead of fighting with package manager and overwriting files under /usr/.. they can put changed versions under /etc/qemu/firmware which takes precedence over the former path. For instance, if 40-bios.json is found under both paths, only the one from /etc is considered and the other is not even parsed. If you would apply this logic one more time, you will get $HOME/.config/qemu/firmware which allows non-root users to override their sysadmin's config and have their private FW blobs with the highest priority.
So if libguestfs wants libvirt to select certain FW image it can install its own JSON descriptor somewhere into a suitable path and if the file has the correct content it will direct libvirt into using FW blob with debug messages enabled.
I not sure we should encourage applications / libraries to do this. Installing a JSON descriptor into any of the directories mentioned above will affect *all* applications on the host using libvirt, which is not appropriate IMHO. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The patch series looks fine to me. TBH I'm not really bothered about verbose using a non-debug firmware. It's rare that we would need to actually debug a UEFI issue, plus the debug output of UEFI is kind of crazy. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
participants (4)
-
Daniel P. Berrangé -
Michal Privoznik -
Pino Toscano -
Richard W.M. Jones