conf: Don't default to raw format for loader/NVRAM

Due to the way the information is stored by the XML parser, we've
had this quirk where specifying any information about the loader
or NVRAM would implicitly set its format to raw. That is,

  <nvram>/path/to/guest_VARS.fd</nvram>

would effectively be interpreted as

  <nvram format='raw'>/path/to/guest_VARS.fd</nvram>

forcing the use of raw format firmware even when qcow2 format
would normally be preferred based on the ordering of firmware
descriptors. This behavior can be worked around in a number of
ways, but it's fairly unintuitive.

In order to remove this quirk, move the selection of the default
firmware format from the parser down to the individual drivers.

Most drivers only support raw firmware images, so they can
unconditionally set the format early and be done with it; the
QEMU driver, however, supports multiple formats and so in that
case we want this default to be applied as late as possible,
when we have already ruled out the possibility of using qcow2
formatted firmware images.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

diff --git a/src/bhyve/bhyve_firmware.c b/src/bhyve/bhyve_firmware.c
index 57ab9c7a82fdeedbd457809e0e4bcab2f2eb8875..8aaf05dc62a61c35c13bced353af902533542c7e 100644 (file)
--- a/src/bhyve/bhyve_firmware.c
+++ b/src/bhyve/bhyve_firmware.c
@@ -80,6 +80,9 @@ bhyveFirmwareFillDomain(bhyveConn *driver,
     if (!def->os.loader)
         def->os.loader = virDomainLoaderDefNew();
 
+    if (!def->os.loader->format)
+        def->os.loader->format = VIR_STORAGE_FILE_RAW;
+
     if (def->os.loader->format != VIR_STORAGE_FILE_RAW) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                        _("Unsupported loader format '%1$s'"),

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 47693a49bf5f0c77cd023ff561a329a9a17ae84b..2e60927799af04ccc8d3ec257cd5967c9b3bf3fc 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3728,7 +3728,6 @@ virDomainLoaderDefNew(void)
     virDomainLoaderDef *def = NULL;
 
     def = g_new0(virDomainLoaderDef, 1);
-    def->format = VIR_STORAGE_FILE_RAW;
 
     return def;
 }
@@ -16771,10 +16770,11 @@ virDomainLoaderDefParseXMLNvram(virDomainLoaderDef *loader,
 
     if (virXMLPropEnumDefault(nvramNode, "format",
                               virStorageFileFormatTypeFromString, VIR_XML_PROP_NONE,
-                              &format, VIR_STORAGE_FILE_RAW) < 0) {
+                              &format, VIR_STORAGE_FILE_NONE) < 0) {
         return -1;
     }
-    if (format != VIR_STORAGE_FILE_RAW &&
+    if (format &&
+        format != VIR_STORAGE_FILE_RAW &&
         format != VIR_STORAGE_FILE_QCOW2) {
         virReportError(VIR_ERR_XML_ERROR,
                        _("Unsupported nvram format '%1$s'"),
@@ -16860,10 +16860,11 @@ virDomainLoaderDefParseXMLLoader(virDomainLoaderDef *loader,
 
     if (virXMLPropEnumDefault(loaderNode, "format",
                               virStorageFileFormatTypeFromString, VIR_XML_PROP_NONE,
-                              &format, VIR_STORAGE_FILE_RAW) < 0) {
+                              &format, VIR_STORAGE_FILE_NONE) < 0) {
         return -1;
     }
-    if (format != VIR_STORAGE_FILE_RAW &&
+    if (format &&
+        format != VIR_STORAGE_FILE_RAW &&
         format != VIR_STORAGE_FILE_QCOW2) {
         virReportError(VIR_ERR_XML_ERROR,
                        _("Unsupported loader format '%1$s'"),
@@ -16894,7 +16895,9 @@ virDomainLoaderDefParseXML(virDomainLoaderDef *loader,
                                          loaderNode) < 0)
         return -1;
 
-    if (loader->nvram && loader->format != loader->nvram->format) {
+    if (loader->nvram &&
+        loader->format && loader->nvram->format &&
+        loader->format != loader->nvram->format) {
         virReportError(VIR_ERR_XML_ERROR,
                        _("Format mismatch: loader.format='%1$s' nvram.format='%2$s'"),
                        virStorageFileFormatTypeToString(loader->format),
@@ -26224,7 +26227,8 @@ virDomainLoaderDefFormatNvram(virBuffer *buf,
                 return -1;
         }
 
-        if (src->format != VIR_STORAGE_FILE_RAW) {
+        if (src->format &&
+            src->format != VIR_STORAGE_FILE_RAW) {
             virBufferEscapeString(&attrBuf, " format='%s'",
                                   virStorageFileFormatTypeToString(src->format));
         }
@@ -26262,7 +26266,8 @@ virDomainLoaderDefFormat(virBuffer *buf,
                           virTristateBoolTypeToString(loader->stateless));
     }
 
-    if (loader->format != VIR_STORAGE_FILE_RAW) {
+    if (loader->format &&
+        loader->format != VIR_STORAGE_FILE_RAW) {
         virBufferEscapeString(&loaderAttrBuf, " format='%s'",
                               virStorageFileFormatTypeToString(loader->format));
     }

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index a1c76935b642367de31e095325759b2a3914f66f..14ad32002354d0fb2aa8d7b7ed369c63a2f6bfa1 100644 (file)
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -654,11 +654,16 @@ libxlMakeDomBuildInfo(virDomainDef *def,
             b_info->u.hvm.system_firmware = g_strdup(def->os.loader->path);
         }
 
-        if (def->os.loader && def->os.loader->format != VIR_STORAGE_FILE_RAW) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("Unsupported loader format '%1$s'"),
-                           virStorageFileFormatTypeToString(def->os.loader->format));
-            return -1;
+        if (def->os.loader) {
+            if (!def->os.loader->format)
+                def->os.loader->format = VIR_STORAGE_FILE_RAW;
+
+            if (def->os.loader->format != VIR_STORAGE_FILE_RAW) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("Unsupported loader format '%1$s'"),
+                               virStorageFileFormatTypeToString(def->os.loader->format));
+                return -1;
+            }
         }
 
         if (def->emulator) {

diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index 1cc42fa59f3ec3c25edd30084605699a5868e3b9..ab1941454ddb4aca39c9f2a789bbf9f7a96ba0b0 100644 (file)
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -115,6 +115,7 @@ xenParseXLOS(virConf *conf, virDomainDef *def, virCaps *caps)
 
         if (bios && STREQ(bios, "ovmf")) {
             def->os.loader = virDomainLoaderDefNew();
+            def->os.loader->format = VIR_STORAGE_FILE_RAW;
             def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH;
             def->os.loader->readonly = VIR_TRISTATE_BOOL_YES;
             if (bios_path)
@@ -126,6 +127,7 @@ xenParseXLOS(virConf *conf, virDomainDef *def, virCaps *caps)
                 if (caps->guests[i]->ostype == VIR_DOMAIN_OSTYPE_HVM &&
                     caps->guests[i]->arch.id == def->os.arch) {
                     def->os.loader = virDomainLoaderDefNew();
+                    def->os.loader->format = VIR_STORAGE_FILE_RAW;
                     def->os.loader->path = g_strdup(caps->guests[i]->arch.defaultInfo.loader);
                 }
             }

diff --git a/src/libxl/xen_xm.c b/src/libxl/xen_xm.c
index 0031d6cbc6eea95feb8cb191a00b2b13dc9bfe2b..5705a5ec0c8bde9d72fec76fdbab160269a5f468 100644 (file)
--- a/src/libxl/xen_xm.c
+++ b/src/libxl/xen_xm.c
@@ -43,6 +43,7 @@ xenParseXMOS(virConf *conf, virDomainDef *def)
         g_autofree char *boot = NULL;
 
         def->os.loader = virDomainLoaderDefNew();
+        def->os.loader->format = VIR_STORAGE_FILE_RAW;
 
         if (xenConfigCopyString(conf, "kernel", &def->os.loader->path) < 0)
             return -1;

diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
index ebaf32cf71d728114c0c6ecbf693096e281640a7..3dcd139a478163ebeef512c16ce377b2bf2c5760 100644 (file)
--- a/src/qemu/qemu_firmware.c
+++ b/src/qemu/qemu_firmware.c
@@ -1082,6 +1082,11 @@ qemuFirmwareEnsureNVRAM(virDomainDef *def,
     if (loader->stateless == VIR_TRISTATE_BOOL_YES)
         return;
 
+    /* If the NVRAM format hasn't been set yet, inherit the same as
+     * the loader */
+    if (loader->nvram && !loader->nvram->format)
+        loader->nvram->format = loader->format;
+
     /* If the source already exists and is fully specified, including
      * the path, leave it alone */
     if (loader->nvram && loader->nvram->path)
@@ -1328,7 +1333,7 @@ qemuFirmwareMatchDomain(const virDomainDef *def,
                       flash->executable.format);
             return false;
         }
-        if (loader &&
+        if (loader && loader->format &&
             STRNEQ(flash->executable.format, virStorageFileFormatTypeToString(loader->format))) {
             VIR_DEBUG("Discarding loader with mismatching flash format '%s' != '%s'",
                       flash->executable.format,
@@ -1342,7 +1347,7 @@ qemuFirmwareMatchDomain(const virDomainDef *def,
                           flash->nvram_template.format);
                 return false;
             }
-            if (loader && loader->nvram &&
+            if (loader && loader->nvram && loader->nvram->format &&
                 STRNEQ(flash->nvram_template.format, virStorageFileFormatTypeToString(loader->nvram->format))) {
                 VIR_DEBUG("Discarding loader with mismatching nvram template format '%s' != '%s'",
                           flash->nvram_template.format,
@@ -1630,7 +1635,8 @@ qemuFirmwareFillDomainLegacy(virQEMUDriver *driver,
         return 1;
     }
 
-    if (loader->format != VIR_STORAGE_FILE_RAW) {
+    if (loader->format &&
+        loader->format != VIR_STORAGE_FILE_RAW) {
         VIR_DEBUG("Ignoring legacy entries for loader with flash format '%s'",
                   virStorageFileFormatTypeToString(loader->format));
         return 1;
@@ -1793,6 +1799,7 @@ qemuFirmwareFillDomain(virQEMUDriver *driver,
         return -1;
 
     if (loader &&
+        loader->format &&
         loader->format != VIR_STORAGE_FILE_RAW &&
         loader->format != VIR_STORAGE_FILE_QCOW2) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -1801,6 +1808,7 @@ qemuFirmwareFillDomain(virQEMUDriver *driver,
         return -1;
     }
     if (nvram &&
+        nvram->format &&
         nvram->format != VIR_STORAGE_FILE_RAW &&
         nvram->format != VIR_STORAGE_FILE_QCOW2) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -1831,8 +1839,19 @@ qemuFirmwareFillDomain(virQEMUDriver *driver,
          * CODE:NVRAM pairs that might have been provided at build
          * time */
         if (!autoSelection) {
-            if (qemuFirmwareFillDomainLegacy(driver, def) < 0)
+            if ((ret = qemuFirmwareFillDomainLegacy(driver, def)) < 0)
                 return -1;
+
+            /* If we've gotten this far without finding a match, it
+             * means that we're dealing with a set of completely
+             * custom paths. In that case, unless the user has
+             * specified otherwise, we have to assume that they're in
+             * raw format */
+            if (ret == 1) {
+                if (loader && !loader->format) {
+                    loader->format = VIR_STORAGE_FILE_RAW;
+                }
+            }
         } else {
             virReportError(VIR_ERR_OPERATION_FAILED,
                            _("Unable to find any firmware to satisfy '%1$s'"),

diff --git a/tests/qemuxml2argvdata/firmware-auto-efi-format-mismatch.x86_64-latest.args b/tests/qemuxml2argvdata/firmware-auto-efi-format-mismatch.x86_64-latest.args
new file mode 100644 (file)
index 0000000..e8d7d58
--- /dev/null
+++ b/tests/qemuxml2argvdata/firmware-auto-efi-format-mismatch.x86_64-latest.args
@@ -0,0 +1,38 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/var/lib/libvirt/qemu/domain--1-guest \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-guest/.local/share \
+XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-guest/.cache \
+XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=guest,debug-threads=on \
+-S \
+-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-guest/master-key.aes"}' \
+-blockdev '{"driver":"file","filename":"/usr/share/edk2/ovmf/OVMF_CODE_4M.secboot.qcow2","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}' \
+-blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"qcow2","file":"libvirt-pflash0-storage"}' \
+-blockdev '{"driver":"file","filename":"/path/to/guest_VARS.qcow2","node-name":"libvirt-pflash1-storage","auto-read-only":true,"discard":"unmap"}' \
+-blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"qcow2","file":"libvirt-pflash1-storage"}' \
+-machine pc-q35-4.0,usb=off,smm=on,dump-guest-core=off,memory-backend=pc.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format,acpi=on \
+-accel kvm \
+-cpu qemu64 \
+-global driver=cfi.pflash01,property=secure,value=on \
+-m size=1048576k \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot strict=on \
+-audiodev '{"id":"audio1","driver":"none"}' \
+-global ICH9-LPC.noreboot=off \
+-watchdog-action reset \
+-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on

diff --git a/tests/qemuxml2argvdata/firmware-auto-efi-format-mismatch.x86_64-latest.err b/tests/qemuxml2argvdata/firmware-auto-efi-format-mismatch.x86_64-latest.err
deleted file mode 100644 (file)
index abfdfc4..0000000
--- a/tests/qemuxml2argvdata/firmware-auto-efi-format-mismatch.x86_64-latest.err
+++ /dev/null
@@ -1 +0,0 @@
-XML error: Format mismatch: loader.format='qcow2' nvram.format='raw'

diff --git a/tests/qemuxml2argvdata/firmware-auto-efi-format-mismatch.xml b/tests/qemuxml2argvdata/firmware-auto-efi-format-mismatch.xml
index 6613d9e9a91938295919010f136a1da9df7779bb..75fa44fd20b3483932eda8ce3283aa870406b516 100644 (file)
--- a/tests/qemuxml2argvdata/firmware-auto-efi-format-mismatch.xml
+++ b/tests/qemuxml2argvdata/firmware-auto-efi-format-mismatch.xml
@@ -6,7 +6,7 @@
   <os firmware='efi'>
     <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
     <loader format='qcow2'/>
-    <nvram>/path/to/guest_VARS.fd</nvram>
+    <nvram>/path/to/guest_VARS.qcow2</nvram>
   </os>
   <features>
     <acpi/>

diff --git a/tests/qemuxml2argvdata/firmware-auto-efi-loader-secure-abi-update.x86_64-latest.args b/tests/qemuxml2argvdata/firmware-auto-efi-loader-secure-abi-update.x86_64-latest.args
index 48f357cbf93ef7884e3465875e74519af994855d..790fb619e8df0a94707b3534a596ff73c298a290 100644 (file)
--- a/tests/qemuxml2argvdata/firmware-auto-efi-loader-secure-abi-update.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/firmware-auto-efi-loader-secure-abi-update.x86_64-latest.args
@@ -10,10 +10,10 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
 -name guest=guest,debug-threads=on \
 -S \
 -object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-guest/master-key.aes"}' \
--blockdev '{"driver":"file","filename":"/usr/share/edk2/ovmf/OVMF_CODE.secboot.fd","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}' \
--blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}' \
--blockdev '{"driver":"file","filename":"/var/lib/libvirt/qemu/nvram/guest_VARS.fd","node-name":"libvirt-pflash1-storage","auto-read-only":true,"discard":"unmap"}' \
--blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"raw","file":"libvirt-pflash1-storage"}' \
+-blockdev '{"driver":"file","filename":"/usr/share/edk2/ovmf/OVMF_CODE_4M.secboot.qcow2","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}' \
+-blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"qcow2","file":"libvirt-pflash0-storage"}' \
+-blockdev '{"driver":"file","filename":"/var/lib/libvirt/qemu/nvram/guest_VARS.qcow2","node-name":"libvirt-pflash1-storage","auto-read-only":true,"discard":"unmap"}' \
+-blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"qcow2","file":"libvirt-pflash1-storage"}' \
 -machine pc-q35-4.0,usb=off,smm=on,dump-guest-core=off,memory-backend=pc.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format,acpi=on \
 -accel kvm \
 -cpu qemu64 \

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 1436e3724c217b91044bd072a784b5abe68adef6..7df044e03b89a407175210f1243baf5318e529b9 100644 (file)
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1115,7 +1115,7 @@ mymain(void)
     DO_TEST_CAPS_LATEST("firmware-auto-efi-format-nvram-qcow2-network-nbd");
     DO_TEST_CAPS_ARCH_LATEST("firmware-auto-efi-format-loader-raw", "aarch64");
     DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("firmware-auto-efi-format-loader-raw-abi-update", "aarch64");
-    DO_TEST_CAPS_LATEST_PARSE_ERROR("firmware-auto-efi-format-mismatch");
+    DO_TEST_CAPS_LATEST("firmware-auto-efi-format-mismatch");
 
     DO_TEST_NOCAPS("clock-utc");
     DO_TEST_NOCAPS("clock-localtime");

diff --git a/tests/qemuxml2xmloutdata/firmware-auto-efi-loader-secure-abi-update.x86_64-latest.xml b/tests/qemuxml2xmloutdata/firmware-auto-efi-loader-secure-abi-update.x86_64-latest.xml
index 332d931ba190c519e8a0d0cdb624bcedbf98d7af..f4ff7a0fc2786ff0796b2b7ef455f5b17bc83d28 100644 (file)
--- a/tests/qemuxml2xmloutdata/firmware-auto-efi-loader-secure-abi-update.x86_64-latest.xml
+++ b/tests/qemuxml2xmloutdata/firmware-auto-efi-loader-secure-abi-update.x86_64-latest.xml
@@ -10,8 +10,8 @@
       <feature enabled='yes' name='enrolled-keys'/>
       <feature enabled='yes' name='secure-boot'/>
     </firmware>
-    <loader readonly='yes' secure='yes' type='pflash'>/usr/share/edk2/ovmf/OVMF_CODE.secboot.fd</loader>
-    <nvram template='/usr/share/edk2/ovmf/OVMF_VARS.secboot.fd'>/var/lib/libvirt/qemu/nvram/guest_VARS.fd</nvram>
+    <loader readonly='yes' secure='yes' type='pflash' format='qcow2'>/usr/share/edk2/ovmf/OVMF_CODE_4M.secboot.qcow2</loader>
+    <nvram template='/usr/share/edk2/ovmf/OVMF_VARS_4M.secboot.qcow2' format='qcow2'>/var/lib/libvirt/qemu/nvram/guest_VARS.qcow2</nvram>
     <boot dev='hd'/>
   </os>
   <features>