thr3ads.net - Libguestfs - Re: [Libguestfs] [PATCH 0/4] Use libvirt firmware autoselection [Jan 2020]

If this information is useful, please help other people find it:
Share via:

Pino Toscano

2020-Jan-16 12:27 UTC

[Libguestfs] [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

Pino Toscano

2020-Jan-16 12:27 UTC

head link

[Libguestfs] [PATCH 1/4] launch: libvirt: parse firmware autoselection

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

Pino Toscano

2020-Jan-16 12:27 UTC

head link

[Libguestfs] [PATCH 2/4] lib: uefi: reset out parameters earlier

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

Pino Toscano

2020-Jan-16 12:27 UTC

head link

[Libguestfs] [PATCH 3/4] lib: allow to use libvirt firmware autoselection

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

Pino Toscano

2020-Jan-16 12:27 UTC

head link

[Libguestfs] [PATCH 4/4] lib: uefi: use the efi libvirt firmware if available

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

Michal Privoznik

2020-Jan-16 12:49 UTC

head link

Re: [Libguestfs] [PATCH 0/4] Use libvirt firmware autoselection

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

Daniel P. Berrangé

2020-Jan-16 12:53 UTC

head link

Re: [Libguestfs] [PATCH 0/4] Use libvirt firmware autoselection

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 :|

Richard W.M. Jones

2020-Jan-16 15:12 UTC

head link

Re: [Libguestfs] [PATCH 0/4] Use libvirt firmware autoselection

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

Apparently Analagous Threads

Search for more reasonably related threads

Libguestfs - Jan 2020 - Re: [PATCH 0/4] Use libvirt firmware autoselection

[Libguestfs] [PATCH 0/4] Use libvirt firmware autoselection

[Libguestfs] [PATCH 1/4] launch: libvirt: parse firmware autoselection

[Libguestfs] [PATCH 2/4] lib: uefi: reset out parameters earlier

[Libguestfs] [PATCH 3/4] lib: allow to use libvirt firmware autoselection

[Libguestfs] [PATCH 4/4] lib: uefi: use the efi libvirt firmware if available

Re: [Libguestfs] [PATCH 0/4] Use libvirt firmware autoselection

Re: [Libguestfs] [PATCH 0/4] Use libvirt firmware autoselection

Re: [Libguestfs] [PATCH 0/4] Use libvirt firmware autoselection

Apparently Analagous Threads