[libvirt] [PATCH v1 4/4] domain capabilities: Expose firmware auto selection feature

Michal Privoznik posted 4 patches 6 years, 10 months ago
There is a newer version of this series
[libvirt] [PATCH v1 4/4] domain capabilities: Expose firmware auto selection feature
Posted by Michal Privoznik 6 years, 10 months ago
If a management application wants to use firmware auto selection
feature it can't currently know if the libvirtd it's talking to
support is or not. Moreover, it doesn't know which values that
are accepted for the @firmware attribute of <os/> when parsing
will allow successful start of the domain later, i.e. if the mgmt
application wants to use 'bios' whether there exists a FW
descriptor in the system that describes bios.

This commit then adds 'firmware' enum to <os/> element in
<domainCapabilities/> XML like this:

  <enum name='firmware'>
    <value>bios</value>
    <value>efi</value>
  </enum>

We can see both 'bios' and 'efi' listed which means that there
are descriptors for both found in the system (matched with the
machine type and architecture reported in the domain capabilities
earlier and not shown here).

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 docs/formatdomaincaps.html.in                 | 14 +++++++++++
 docs/schemas/domaincaps.rng                   |  3 +++
 src/conf/domain_capabilities.c                |  2 ++
 src/conf/domain_capabilities.h                |  1 +
 src/qemu/qemu_capabilities.c                  | 23 ++++++++++++++++++-
 src/qemu/qemu_capabilities.h                  |  1 +
 src/qemu/qemu_driver.c                        |  1 +
 tests/Makefile.am                             |  4 +++-
 .../qemu_1.7.0.x86_64.xml                     |  4 ++++
 .../qemu_2.12.0-virt.aarch64.xml              |  3 +++
 .../qemu_2.12.0.ppc64.xml                     |  1 +
 .../qemu_2.12.0.s390x.xml                     |  1 +
 .../qemu_2.12.0.x86_64.xml                    |  4 ++++
 .../qemu_2.6.0-virt.aarch64.xml               |  3 +++
 .../qemu_2.6.0.aarch64.xml                    |  1 +
 .../domaincapsschemadata/qemu_2.6.0.ppc64.xml |  1 +
 .../qemu_2.6.0.x86_64.xml                     |  4 ++++
 .../domaincapsschemadata/qemu_2.7.0.s390x.xml |  1 +
 .../qemu_2.8.0-tcg.x86_64.xml                 |  4 ++++
 .../domaincapsschemadata/qemu_2.8.0.s390x.xml |  1 +
 .../qemu_2.8.0.x86_64.xml                     |  4 ++++
 .../qemu_2.9.0-q35.x86_64.xml                 |  4 ++++
 .../qemu_2.9.0-tcg.x86_64.xml                 |  4 ++++
 .../qemu_2.9.0.x86_64.xml                     |  4 ++++
 .../domaincapsschemadata/qemu_3.0.0.s390x.xml |  1 +
 .../qemu_4.0.0.x86_64.xml                     |  4 ++++
 tests/domaincapstest.c                        | 16 +++++++++++++
 27 files changed, 112 insertions(+), 2 deletions(-)

diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in
index 2583f9bead..4b7d9630a4 100644
--- a/docs/formatdomaincaps.html.in
+++ b/docs/formatdomaincaps.html.in
@@ -119,6 +119,10 @@
 &lt;domainCapabilities&gt;
   ...
   &lt;os supported='yes'&gt;
+    &lt;enum name='firmware'&gt;
+      &lt;value&gt;bios&lt;/value&gt;
+      &lt;value&gt;efi&lt;/value&gt;
+    &lt;/enum&gt;
     &lt;loader supported='yes'&gt;
       &lt;value&gt;/usr/share/OVMF/OVMF_CODE.fd&lt;/value&gt;
       &lt;enum name='type'&gt;
@@ -135,6 +139,16 @@
 &lt;domainCapabilities&gt;
 </pre>
 
+    <p>The <code>firmware</code> enum corresponds to
+      <code>firmware</code> attribute of the <code>os</code> element.
+      Plain presence of this enum means that libvirt is capable of so
+      called firmware auto selection. The listed values then represent
+      accepted values for the domain attribute. Only values for which
+      there exists a firmware descriptor that matches machine type and
+      architecture are listed, i.e. those which won't cause a failure
+      on domain startup.
+    </p>
+
     <p>For the <code>loader</code> element, the following can occur:</p>
 
     <dl>
diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng
index 3c42cb8075..230697f824 100644
--- a/docs/schemas/domaincaps.rng
+++ b/docs/schemas/domaincaps.rng
@@ -77,6 +77,9 @@
     <element name='os'>
       <interleave>
         <ref name='supported'/>
+        <optional>
+          <ref name='enum'/>
+        </optional>
         <optional>
           <ref name='loader'/>
         </optional>
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 5a8f48da61..29d9f1f640 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -427,6 +427,8 @@ virDomainCapsOSFormat(virBufferPtr buf,
 
     FORMAT_PROLOGUE(os);
 
+    ENUM_PROCESS(os, firmware, virDomainOsDefFirmwareTypeToString);
+
     virDomainCapsLoaderFormat(buf, loader);
 
     FORMAT_EPILOGUE(os);
diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
index 26f4b8c394..4d4a3f0cf1 100644
--- a/src/conf/domain_capabilities.h
+++ b/src/conf/domain_capabilities.h
@@ -57,6 +57,7 @@ typedef struct _virDomainCapsOS virDomainCapsOS;
 typedef virDomainCapsOS *virDomainCapsOSPtr;
 struct _virDomainCapsOS {
     virTristateBool supported;
+    virDomainCapsEnum firmware;     /* Info about virDomainOsDefFirmware */
     virDomainCapsLoader loader;     /* Info about virDomainLoaderDef */
 };
 
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 71d4c01296..452d092069 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -46,6 +46,7 @@
 #include "qemu_capspriv.h"
 #include "qemu_qapi.h"
 #include "qemu_process.h"
+#include "qemu_firmware.h"
 
 #include <fcntl.h>
 #include <sys/stat.h>
@@ -4962,12 +4963,27 @@ virQEMUCapsFillDomainLoaderCaps(virDomainCapsLoaderPtr capsLoader,
 
 static int
 virQEMUCapsFillDomainOSCaps(virDomainCapsOSPtr os,
+                            const char *machine,
+                            virArch arch,
+                            bool privileged,
                             virFirmwarePtr *firmwares,
                             size_t nfirmwares)
 {
     virDomainCapsLoaderPtr capsLoader = &os->loader;
+    unsigned int autoFirmwares = 0;
 
     os->supported = VIR_TRISTATE_BOOL_YES;
+
+    if (qemuFirmwareGetSupported(machine, arch, privileged, &autoFirmwares) < 0)
+        return -1;
+
+    os->firmware.report = true;
+
+    if (autoFirmwares & (1 << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS))
+        VIR_DOMAIN_CAPS_ENUM_SET(os->firmware, VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS);
+    if (autoFirmwares & (1 << VIR_DOMAIN_OS_DEF_FIRMWARE_EFI))
+        VIR_DOMAIN_CAPS_ENUM_SET(os->firmware, VIR_DOMAIN_OS_DEF_FIRMWARE_EFI);
+
     if (virQEMUCapsFillDomainLoaderCaps(capsLoader, firmwares, nfirmwares) < 0)
         return -1;
     return 0;
@@ -5298,6 +5314,7 @@ int
 virQEMUCapsFillDomainCaps(virCapsPtr caps,
                           virDomainCapsPtr domCaps,
                           virQEMUCapsPtr qemuCaps,
+                          bool privileged,
                           virFirmwarePtr *firmwares,
                           size_t nfirmwares)
 {
@@ -5324,7 +5341,11 @@ virQEMUCapsFillDomainCaps(virCapsPtr caps,
     domCaps->genid = virTristateBoolFromBool(
             virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMGENID));
 
-    if (virQEMUCapsFillDomainOSCaps(os, firmwares, nfirmwares) < 0 ||
+    if (virQEMUCapsFillDomainOSCaps(os,
+                                    domCaps->machine,
+                                    domCaps->arch,
+                                    privileged,
+                                    firmwares, nfirmwares) < 0 ||
         virQEMUCapsFillDomainCPUCaps(caps, qemuCaps, domCaps) < 0 ||
         virQEMUCapsFillDomainIOThreadCaps(qemuCaps, domCaps) < 0 ||
         virQEMUCapsFillDomainDeviceDiskCaps(qemuCaps,
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index c6f6980684..2b099c15c9 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -631,6 +631,7 @@ int virQEMUCapsInitGuestFromBinary(virCapsPtr caps,
 int virQEMUCapsFillDomainCaps(virCapsPtr caps,
                               virDomainCapsPtr domCaps,
                               virQEMUCapsPtr qemuCaps,
+                              bool privileged,
                               virFirmwarePtr *firmwares,
                               size_t nfirmwares);
 
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7e5bbc3cc9..7fa5c985ec 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19856,6 +19856,7 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn,
         goto cleanup;
 
     if (virQEMUCapsFillDomainCaps(caps, domCaps, qemuCaps,
+                                  driver->privileged,
                                   cfg->firmwares, cfg->nfirmwares) < 0)
         goto cleanup;
 
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 1319c3b12c..46d94d2236 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1033,7 +1033,9 @@ domaincapsmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
 domaincapsmock_la_LIBADD = $(MOCKLIBS_LIBS)
 
 domaincapstest_SOURCES = \
-	domaincapstest.c testutils.h testutils.c
+	domaincapstest.c testutils.h testutils.c \
+	virfilewrapper.c virfilewrapper.h \
+	$(NULL)
 domaincapstest_LDADD = $(LDADDS)
 
 if WITH_QEMU
diff --git a/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml b/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml
index 497363bbe9..13ff70695c 100644
--- a/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml
@@ -6,6 +6,10 @@
   <vcpu max='255'/>
   <iothreads supported='no'/>
   <os supported='yes'>
+    <enum name='firmware'>
+      <value>bios</value>
+      <value>efi</value>
+    </enum>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_2.12.0-virt.aarch64.xml b/tests/domaincapsschemadata/qemu_2.12.0-virt.aarch64.xml
index 7639df44c6..8c225e3c89 100644
--- a/tests/domaincapsschemadata/qemu_2.12.0-virt.aarch64.xml
+++ b/tests/domaincapsschemadata/qemu_2.12.0-virt.aarch64.xml
@@ -6,6 +6,9 @@
   <vcpu max='255'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'>
+      <value>efi</value>
+    </enum>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml b/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml
index f10d361359..24fbbf505a 100644
--- a/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml
+++ b/tests/domaincapsschemadata/qemu_2.12.0.ppc64.xml
@@ -6,6 +6,7 @@
   <vcpu max='1024'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'/>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_2.12.0.s390x.xml b/tests/domaincapsschemadata/qemu_2.12.0.s390x.xml
index 41a81ff02f..35ff91c53a 100644
--- a/tests/domaincapsschemadata/qemu_2.12.0.s390x.xml
+++ b/tests/domaincapsschemadata/qemu_2.12.0.s390x.xml
@@ -6,6 +6,7 @@
   <vcpu max='248'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'/>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml b/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml
index 5913e7fc63..e71d6e7bba 100644
--- a/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml
@@ -6,6 +6,10 @@
   <vcpu max='255'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'>
+      <value>bios</value>
+      <value>efi</value>
+    </enum>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_2.6.0-virt.aarch64.xml b/tests/domaincapsschemadata/qemu_2.6.0-virt.aarch64.xml
index 9ee801092e..3bd46b566b 100644
--- a/tests/domaincapsschemadata/qemu_2.6.0-virt.aarch64.xml
+++ b/tests/domaincapsschemadata/qemu_2.6.0-virt.aarch64.xml
@@ -6,6 +6,9 @@
   <vcpu max='255'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'>
+      <value>efi</value>
+    </enum>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml b/tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml
index 4dd0b52ed3..148ffc8539 100644
--- a/tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml
+++ b/tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml
@@ -6,6 +6,7 @@
   <vcpu max='1'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'/>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_2.6.0.ppc64.xml b/tests/domaincapsschemadata/qemu_2.6.0.ppc64.xml
index aa982d237e..a0ff809775 100644
--- a/tests/domaincapsschemadata/qemu_2.6.0.ppc64.xml
+++ b/tests/domaincapsschemadata/qemu_2.6.0.ppc64.xml
@@ -6,6 +6,7 @@
   <vcpu max='255'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'/>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml b/tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml
index 6aa3f52ee4..993c4bb396 100644
--- a/tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml
@@ -6,6 +6,10 @@
   <vcpu max='255'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'>
+      <value>bios</value>
+      <value>efi</value>
+    </enum>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_2.7.0.s390x.xml b/tests/domaincapsschemadata/qemu_2.7.0.s390x.xml
index 8daa15ab9d..e7b72912e7 100644
--- a/tests/domaincapsschemadata/qemu_2.7.0.s390x.xml
+++ b/tests/domaincapsschemadata/qemu_2.7.0.s390x.xml
@@ -6,6 +6,7 @@
   <vcpu max='248'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'/>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml b/tests/domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml
index 081805aa4a..b80745864e 100644
--- a/tests/domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_2.8.0-tcg.x86_64.xml
@@ -6,6 +6,10 @@
   <vcpu max='255'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'>
+      <value>bios</value>
+      <value>efi</value>
+    </enum>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_2.8.0.s390x.xml b/tests/domaincapsschemadata/qemu_2.8.0.s390x.xml
index 62c51e4087..80810cd876 100644
--- a/tests/domaincapsschemadata/qemu_2.8.0.s390x.xml
+++ b/tests/domaincapsschemadata/qemu_2.8.0.s390x.xml
@@ -6,6 +6,7 @@
   <vcpu max='248'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'/>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml b/tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml
index 1bb034aa4f..ef4972fbc3 100644
--- a/tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_2.8.0.x86_64.xml
@@ -6,6 +6,10 @@
   <vcpu max='255'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'>
+      <value>bios</value>
+      <value>efi</value>
+    </enum>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml b/tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml
index 67c6d5e77e..8de185e5cc 100644
--- a/tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_2.9.0-q35.x86_64.xml
@@ -6,6 +6,10 @@
   <vcpu max='288'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'>
+      <value>bios</value>
+      <value>efi</value>
+    </enum>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml b/tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml
index 588ef08199..1a7fdd5402 100644
--- a/tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml
@@ -6,6 +6,10 @@
   <vcpu max='255'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'>
+      <value>bios</value>
+      <value>efi</value>
+    </enum>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml b/tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml
index 598937a971..1f59f1a2f4 100644
--- a/tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_2.9.0.x86_64.xml
@@ -6,6 +6,10 @@
   <vcpu max='255'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'>
+      <value>bios</value>
+      <value>efi</value>
+    </enum>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_3.0.0.s390x.xml b/tests/domaincapsschemadata/qemu_3.0.0.s390x.xml
index 1d97f1f344..64acf4727d 100644
--- a/tests/domaincapsschemadata/qemu_3.0.0.s390x.xml
+++ b/tests/domaincapsschemadata/qemu_3.0.0.s390x.xml
@@ -6,6 +6,7 @@
   <vcpu max='248'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'/>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml b/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml
index df66be9e29..319407be0a 100644
--- a/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml
@@ -6,6 +6,10 @@
   <vcpu max='255'/>
   <iothreads supported='yes'/>
   <os supported='yes'>
+    <enum name='firmware'>
+      <value>bios</value>
+      <value>efi</value>
+    </enum>
     <loader supported='yes'>
       <value>/usr/share/AAVMF/AAVMF_CODE.fd</value>
       <value>/usr/share/AAVMF/AAVMF32_CODE.fd</value>
diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
index 2cccfbc8e2..7637435e1f 100644
--- a/tests/domaincapstest.c
+++ b/tests/domaincapstest.c
@@ -20,6 +20,8 @@
 
 #include "testutils.h"
 #include "domain_capabilities.h"
+#include "virfilewrapper.h"
+#include "configmake.h"
 
 
 #define VIR_FROM_THIS VIR_FROM_NONE
@@ -104,6 +106,7 @@ fillQemuCaps(virDomainCapsPtr domCaps,
         goto cleanup;
 
     if (virQEMUCapsFillDomainCaps(caps, domCaps, qemuCaps,
+                                  false,
                                   cfg->firmwares,
                                   cfg->nfirmwares) < 0)
         goto cleanup;
@@ -364,6 +367,13 @@ mymain(void)
 
 #if WITH_QEMU
 
+    virFileWrapperAddPrefix(SYSCONFDIR "/qemu/firmware",
+                            abs_srcdir "/qemufirmwaredata/etc/qemu/firmware");
+    virFileWrapperAddPrefix(PREFIX "/share/qemu/firmware",
+                            abs_srcdir "/qemufirmwaredata/usr/share/qemu/firmware");
+    virFileWrapperAddPrefix("/home/user/.config/qemu/firmware",
+                            abs_srcdir "/qemufirmwaredata/home/user/.config/qemu/firmware");
+
     DO_TEST_QEMU("1.7.0", "caps_1.7.0",
                  "/usr/bin/qemu-system-x86_64", NULL,
                  "x86_64", VIR_DOMAIN_VIRT_KVM);
@@ -437,6 +447,10 @@ mymain(void)
                  "x86_64", VIR_DOMAIN_VIRT_KVM);
     virObjectUnref(cfg);
 
+    virFileWrapperRemovePrefix(SYSCONFDIR "/qemu/firmware");
+    virFileWrapperRemovePrefix(PREFIX "/share/qemu/firmware");
+    virFileWrapperRemovePrefix("/home/user/.config/qemu/firmware");
+
 #endif /* WITH_QEMU */
 
 #if WITH_LIBXL
@@ -458,6 +472,8 @@ mymain(void)
     DO_TEST_BHYVE("fbuf", "/usr/sbin/bhyve", &bhyve_caps, VIR_DOMAIN_VIRT_BHYVE);
 #endif /* WITH_BHYVE */
 
+    virFileWrapperClearPrefixes();
+
     return ret;
 }
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 4/4] domain capabilities: Expose firmware auto selection feature
Posted by Daniel P. Berrangé 6 years, 10 months ago
On Fri, Apr 05, 2019 at 09:19:48AM +0200, Michal Privoznik wrote:
> If a management application wants to use firmware auto selection
> feature it can't currently know if the libvirtd it's talking to
> support is or not. Moreover, it doesn't know which values that
> are accepted for the @firmware attribute of <os/> when parsing
> will allow successful start of the domain later, i.e. if the mgmt
> application wants to use 'bios' whether there exists a FW
> descriptor in the system that describes bios.
> 
> This commit then adds 'firmware' enum to <os/> element in
> <domainCapabilities/> XML like this:
> 
>   <enum name='firmware'>
>     <value>bios</value>
>     <value>efi</value>
>   </enum>
> 
> We can see both 'bios' and 'efi' listed which means that there
> are descriptors for both found in the system (matched with the
> machine type and architecture reported in the domain capabilities
> earlier and not shown here).

I wonder if we should also have a enum for the "secure" attribute
of <loader> to deal with SecureBoot

   <enum name='secure'>
     <value>yes</value>
     <value>no</value>
   </enum>

Always report "no", only report "yes" if there is at least one
firmware file we see that can do SecureBoot.

Yes, in theory that is a matrix against the firmware attribute
value, but we have many such dependancies between attributes
and it is not practical to fully express all valid combinations
in the capabilities, so taking the simple approach is valid
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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 4/4] domain capabilities: Expose firmware auto selection feature
Posted by Michal Privoznik 6 years, 10 months ago
On 4/5/19 10:31 AM, Daniel P. Berrangé wrote:
> On Fri, Apr 05, 2019 at 09:19:48AM +0200, Michal Privoznik wrote:
>> If a management application wants to use firmware auto selection
>> feature it can't currently know if the libvirtd it's talking to
>> support is or not. Moreover, it doesn't know which values that
>> are accepted for the @firmware attribute of <os/> when parsing
>> will allow successful start of the domain later, i.e. if the mgmt
>> application wants to use 'bios' whether there exists a FW
>> descriptor in the system that describes bios.
>>
>> This commit then adds 'firmware' enum to <os/> element in
>> <domainCapabilities/> XML like this:
>>
>>    <enum name='firmware'>
>>      <value>bios</value>
>>      <value>efi</value>
>>    </enum>
>>
>> We can see both 'bios' and 'efi' listed which means that there
>> are descriptors for both found in the system (matched with the
>> machine type and architecture reported in the domain capabilities
>> earlier and not shown here).
> 
> I wonder if we should also have a enum for the "secure" attribute
> of <loader> to deal with SecureBoot
> 
>     <enum name='secure'>
>       <value>yes</value>
>       <value>no</value>
>     </enum>
> 
> Always report "no", only report "yes" if there is at least one
> firmware file we see that can do SecureBoot.
> 
> Yes, in theory that is a matrix against the firmware attribute
> value, but we have many such dependancies between attributes
> and it is not practical to fully express all valid combinations
> in the capabilities, so taking the simple approach is valid
> IMHO.

Makes sense, I'll put it on my TODO list for v2.

Thanks,
Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 4/4] domain capabilities: Expose firmware auto selection feature
Posted by Laszlo Ersek 6 years, 10 months ago
On 04/05/19 10:44, Michal Privoznik wrote:
> On 4/5/19 10:31 AM, Daniel P. Berrangé wrote:
>> On Fri, Apr 05, 2019 at 09:19:48AM +0200, Michal Privoznik wrote:
>>> If a management application wants to use firmware auto selection
>>> feature it can't currently know if the libvirtd it's talking to
>>> support is or not. Moreover, it doesn't know which values that
>>> are accepted for the @firmware attribute of <os/> when parsing
>>> will allow successful start of the domain later, i.e. if the mgmt
>>> application wants to use 'bios' whether there exists a FW
>>> descriptor in the system that describes bios.
>>>
>>> This commit then adds 'firmware' enum to <os/> element in
>>> <domainCapabilities/> XML like this:
>>>
>>>    <enum name='firmware'>
>>>      <value>bios</value>
>>>      <value>efi</value>
>>>    </enum>
>>>
>>> We can see both 'bios' and 'efi' listed which means that there
>>> are descriptors for both found in the system (matched with the
>>> machine type and architecture reported in the domain capabilities
>>> earlier and not shown here).
>>
>> I wonder if we should also have a enum for the "secure" attribute
>> of <loader> to deal with SecureBoot
>>
>>     <enum name='secure'>
>>       <value>yes</value>
>>       <value>no</value>
>>     </enum>
>>
>> Always report "no", only report "yes" if there is at least one
>> firmware file we see that can do SecureBoot.
>>
>> Yes, in theory that is a matrix against the firmware attribute
>> value, but we have many such dependancies between attributes
>> and it is not practical to fully express all valid combinations
>> in the capabilities, so taking the simple approach is valid
>> IMHO.
>
> Makes sense, I'll put it on my TODO list for v2.

With the above, the series looks good to me as well (mostly ready to
ACK).

I have a low-level question though. In patch #3:

  verify(sizeof(unsigned int) >= ((1ULL << VIR_DOMAIN_OS_DEF_FIRMWARE_LAST) >> 2));

are you trying to express that all non-LAST enum values are <= 31?

I'm probably missing something, but on the LHS, you have a number of
bytes, while on the RHS, you have a bitmask with the "LAST" bit set,
divided by 4 (not 8).


A related question for patch #4:

  1 << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS

Apparently, we'd like to store such bitmasks in "unsigned int" objects
however (not in "int"s), so all similar expressions should be written
like

  1u << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS

Because otherwise, if (1 << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS) is
unrepresentable as a signed int (e.g. int is int32_t and
VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS has value 31), then the behavior is
undefined.


... Honestly I'd just go with "uint64_t" -- it's an optional type, per
standard C, but libvirt already uses that type elsewhere,
unconditionally. Then you could use:

  verify(VIR_DOMAIN_OS_DEF_FIRMWARE_LAST <= 64);

(assuming you never want to set the "LAST" bit in the mask)

and

  UINT64_C(1) << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS

for creating the single-bit masks.

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 4/4] domain capabilities: Expose firmware auto selection feature
Posted by Michal Privoznik 6 years, 10 months ago
On 4/8/19 1:33 PM, Laszlo Ersek wrote:
> On 04/05/19 10:44, Michal Privoznik wrote:
>> On 4/5/19 10:31 AM, Daniel P. Berrangé wrote:
>>> On Fri, Apr 05, 2019 at 09:19:48AM +0200, Michal Privoznik wrote:
>>>> If a management application wants to use firmware auto selection
>>>> feature it can't currently know if the libvirtd it's talking to
>>>> support is or not. Moreover, it doesn't know which values that
>>>> are accepted for the @firmware attribute of <os/> when parsing
>>>> will allow successful start of the domain later, i.e. if the mgmt
>>>> application wants to use 'bios' whether there exists a FW
>>>> descriptor in the system that describes bios.
>>>>
>>>> This commit then adds 'firmware' enum to <os/> element in
>>>> <domainCapabilities/> XML like this:
>>>>
>>>>     <enum name='firmware'>
>>>>       <value>bios</value>
>>>>       <value>efi</value>
>>>>     </enum>
>>>>
>>>> We can see both 'bios' and 'efi' listed which means that there
>>>> are descriptors for both found in the system (matched with the
>>>> machine type and architecture reported in the domain capabilities
>>>> earlier and not shown here).
>>>
>>> I wonder if we should also have a enum for the "secure" attribute
>>> of <loader> to deal with SecureBoot
>>>
>>>      <enum name='secure'>
>>>        <value>yes</value>
>>>        <value>no</value>
>>>      </enum>
>>>
>>> Always report "no", only report "yes" if there is at least one
>>> firmware file we see that can do SecureBoot.
>>>
>>> Yes, in theory that is a matrix against the firmware attribute
>>> value, but we have many such dependancies between attributes
>>> and it is not practical to fully express all valid combinations
>>> in the capabilities, so taking the simple approach is valid
>>> IMHO.
>>
>> Makes sense, I'll put it on my TODO list for v2.
> 
> With the above, the series looks good to me as well (mostly ready to
> ACK).
> 
> I have a low-level question though. In patch #3:
> 
>    verify(sizeof(unsigned int) >= ((1ULL << VIR_DOMAIN_OS_DEF_FIRMWARE_LAST) >> 2));
> 
> are you trying to express that all non-LAST enum values are <= 31? >
> I'm probably missing something, but on the LHS, you have a number of
> bytes, while on the RHS, you have a bitmask with the "LAST" bit set,
> divided by 4 (not 8).

Ah, right. Yeah, I want to ensure that all VIR_DOMAIN_SO_DEF_* values 
can be shifted left and still fit into uint. This is because of the way 
these are returned from qemuFirmwareGetSupported(): 1 << 
VIR_DOMAIN_OS_DEF_FIRMWARE_*. Migt as well check if 
(VIR_DOMAIN_OS_DEF_FIRMWARE_LAST * 8) <= sizeof(int).

> 
> 
> A related question for patch #4:
> 
>    1 << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS
> 
> Apparently, we'd like to store such bitmasks in "unsigned int" objects
> however (not in "int"s), so all similar expressions should be written
> like
> 
>    1u << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS
> 
> Because otherwise, if (1 << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS) is
> unrepresentable as a signed int (e.g. int is int32_t and
> VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS has value 31), then the behavior is
> undefined.
> 
> 
> ... Honestly I'd just go with "uint64_t" -- it's an optional type, per
> standard C, but libvirt already uses that type elsewhere,
> unconditionally. Then you could use:
> 
>    verify(VIR_DOMAIN_OS_DEF_FIRMWARE_LAST <= 64);
> 
> (assuming you never want to set the "LAST" bit in the mask)
> 
> and
> 
>    UINT64_C(1) << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS
> 
> for creating the single-bit masks.

Okay, let me fix this and repost.

Thanks,
Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list