I have been able to find exactly zero evidence of this type of firmware descriptor actually existing in the wild, so this is essentialy dead code. Dropping it simplifies the task of further tweaking the firmware selection code. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_firmware.c | 83 ++-------------------------------------- 1 file changed, 3 insertions(+), 80 deletions(-) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 5bd34ea87f..b168ec7cf7 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -92,12 +92,6 @@ struct _qemuFirmwareMappingFlash { }; -typedef struct _qemuFirmwareMappingKernel qemuFirmwareMappingKernel; -struct _qemuFirmwareMappingKernel { - char *filename; -}; - - typedef struct _qemuFirmwareMappingMemory qemuFirmwareMappingMemory; struct _qemuFirmwareMappingMemory { char *filename; @@ -107,7 +101,6 @@ struct _qemuFirmwareMappingMemory { typedef enum { QEMU_FIRMWARE_DEVICE_NONE = 0, QEMU_FIRMWARE_DEVICE_FLASH, - QEMU_FIRMWARE_DEVICE_KERNEL, QEMU_FIRMWARE_DEVICE_MEMORY, QEMU_FIRMWARE_DEVICE_LAST @@ -118,7 +111,6 @@ VIR_ENUM_IMPL(qemuFirmwareDevice, QEMU_FIRMWARE_DEVICE_LAST, "", "flash", - "kernel", "memory", ); @@ -129,7 +121,6 @@ struct _qemuFirmwareMapping { union { qemuFirmwareMappingFlash flash; - qemuFirmwareMappingKernel kernel; qemuFirmwareMappingMemory memory; } data; }; @@ -222,13 +213,6 @@ qemuFirmwareMappingFlashFreeContent(qemuFirmwareMappingFlash *flash) } -static void -qemuFirmwareMappingKernelFreeContent(qemuFirmwareMappingKernel *kernel) -{ - g_free(kernel->filename); -} - - static void qemuFirmwareMappingMemoryFreeContent(qemuFirmwareMappingMemory *memory) { @@ -243,9 +227,6 @@ qemuFirmwareMappingFreeContent(qemuFirmwareMapping *mapping) case QEMU_FIRMWARE_DEVICE_FLASH: qemuFirmwareMappingFlashFreeContent(&mapping->data.flash); break; - case QEMU_FIRMWARE_DEVICE_KERNEL: - qemuFirmwareMappingKernelFreeContent(&mapping->data.kernel); - break; case QEMU_FIRMWARE_DEVICE_MEMORY: qemuFirmwareMappingMemoryFreeContent(&mapping->data.memory); break; @@ -418,24 +399,6 @@ qemuFirmwareMappingFlashParse(const char *path, } -static int -qemuFirmwareMappingKernelParse(const char *path, - virJSONValue *doc, - qemuFirmwareMappingKernel *kernel) -{ - const char *filename; - - if (!(filename = virJSONValueObjectGetString(doc, "filename"))) { - VIR_DEBUG("missing 'filename' in '%s'", path); - return -1; - } - - kernel->filename = g_strdup(filename); - - return 0; -} - - static int qemuFirmwareMappingMemoryParse(const char *path, virJSONValue *doc, @@ -485,10 +448,6 @@ qemuFirmwareMappingParse(const char *path, if (qemuFirmwareMappingFlashParse(path, mapping, &fw->mapping.data.flash) < 0) return -1; break; - case QEMU_FIRMWARE_DEVICE_KERNEL: - if (qemuFirmwareMappingKernelParse(path, mapping, &fw->mapping.data.kernel) < 0) - return -1; - break; case QEMU_FIRMWARE_DEVICE_MEMORY: if (qemuFirmwareMappingMemoryParse(path, mapping, &fw->mapping.data.memory) < 0) return -1; @@ -732,19 +691,6 @@ qemuFirmwareMappingFlashFormat(virJSONValue *mapping, } -static int -qemuFirmwareMappingKernelFormat(virJSONValue *mapping, - qemuFirmwareMappingKernel *kernel) -{ - if (virJSONValueObjectAppendString(mapping, - "filename", - kernel->filename) < 0) - return -1; - - return 0; -} - - static int qemuFirmwareMappingMemoryFormat(virJSONValue *mapping, qemuFirmwareMappingMemory *memory) @@ -774,10 +720,6 @@ qemuFirmwareMappingFormat(virJSONValue *doc, if (qemuFirmwareMappingFlashFormat(mapping, &fw->mapping.data.flash) < 0) return -1; break; - case QEMU_FIRMWARE_DEVICE_KERNEL: - if (qemuFirmwareMappingKernelFormat(mapping, &fw->mapping.data.kernel) < 0) - return -1; - break; case QEMU_FIRMWARE_DEVICE_MEMORY: if (qemuFirmwareMappingMemoryFormat(mapping, &fw->mapping.data.memory) < 0) return -1; @@ -920,21 +862,17 @@ qemuFirmwareMatchesMachineArch(const qemuFirmware *fw, * qemuFirmwareMatchesPaths: * @fw: firmware definition * @loader: loader definition - * @kernelPath: path to kernel image * * Checks whether @fw is compatible with the information provided as * part of the domain definition. * - * Returns: true if @fw is compatible with @loader and @kernelPath, - * false otherwise + * Returns: true if @fw is compatible with @loader, false otherwise */ static bool qemuFirmwareMatchesPaths(const qemuFirmware *fw, - const virDomainLoaderDef *loader, - const char *kernelPath) + const virDomainLoaderDef *loader) { const qemuFirmwareMappingFlash *flash = &fw->mapping.data.flash; - const qemuFirmwareMappingKernel *kernel = &fw->mapping.data.kernel; const qemuFirmwareMappingMemory *memory = &fw->mapping.data.memory; switch (fw->mapping.device) { @@ -954,11 +892,6 @@ qemuFirmwareMatchesPaths(const qemuFirmware *fw, !virFileComparePaths(loader->path, memory->filename)) return false; break; - case QEMU_FIRMWARE_DEVICE_KERNEL: - if (kernelPath && - !virFileComparePaths(kernelPath, kernel->filename)) - return false; - break; case QEMU_FIRMWARE_DEVICE_NONE: case QEMU_FIRMWARE_DEVICE_LAST: return false; @@ -1183,7 +1116,7 @@ qemuFirmwareMatchDomain(const virDomainDef *def, return false; } - if (!qemuFirmwareMatchesPaths(fw, def->os.loader, def->os.kernel)) { + if (!qemuFirmwareMatchesPaths(fw, def->os.loader)) { VIR_DEBUG("No matching path in '%s'", path); return false; } @@ -1424,7 +1357,6 @@ qemuFirmwareEnableFeaturesModern(virDomainDef *def, const qemuFirmware *fw) { const qemuFirmwareMappingFlash *flash = &fw->mapping.data.flash; - const qemuFirmwareMappingKernel *kernel = &fw->mapping.data.kernel; const qemuFirmwareMappingMemory *memory = &fw->mapping.data.memory; virDomainLoaderDef *loader = NULL; virStorageFileFormat format; @@ -1482,14 +1414,6 @@ qemuFirmwareEnableFeaturesModern(virDomainDef *def, loader->path, NULLSTR(loader->nvramTemplate)); break; - case QEMU_FIRMWARE_DEVICE_KERNEL: - VIR_FREE(def->os.kernel); - def->os.kernel = g_strdup(kernel->filename); - - VIR_DEBUG("decided on kernel '%s'", - def->os.kernel); - break; - case QEMU_FIRMWARE_DEVICE_MEMORY: if (!def->os.loader) def->os.loader = virDomainLoaderDefNew(); @@ -2056,7 +1980,6 @@ qemuFirmwareGetSupported(const char *machine, fwpath = memory->filename; break; - case QEMU_FIRMWARE_DEVICE_KERNEL: case QEMU_FIRMWARE_DEVICE_NONE: case QEMU_FIRMWARE_DEVICE_LAST: break; -- 2.52.0

We access the NVRAM information via the 'loader' local variable throughout the file, and this is the only spot where the 'nvram' local variable exists. It makes things inconsistent and opens up the possibility of the values for 'loader' and 'nvram' going out of sync, especially after a future commit will introduce the need to set the former. Just get rid of the additional variable. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_firmware.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index b168ec7cf7..903b0a984d 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1780,7 +1780,6 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, bool abiUpdate) { virDomainLoaderDef *loader = def->os.loader; - virStorageSource *nvram = loader ? loader->nvram : NULL; bool autoSelection = (def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE); int ret; @@ -1804,13 +1803,14 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, virStorageFileFormatTypeToString(loader->format)); return -1; } - if (nvram && - nvram->format && - nvram->format != VIR_STORAGE_FILE_RAW && - nvram->format != VIR_STORAGE_FILE_QCOW2) { + if (loader && + loader->nvram && + loader->nvram->format && + loader->nvram->format != VIR_STORAGE_FILE_RAW && + loader->nvram->format != VIR_STORAGE_FILE_QCOW2) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported nvram format '%1$s'"), - virStorageFileFormatTypeToString(nvram->format)); + virStorageFileFormatTypeToString(loader->nvram->format)); return -1; } -- 2.52.0

As explained in the comment that comes along with it, this code ensures that the user's preference is taken into account when nvram.format is the only information that's provided. Currently it lives in the parser, but it makes more sense for it to be together with the rest of the firmware selection code instead. Note that this move is not completely seamless: once the code is moved outside of the parser, it can no longer reliably know whether the <loader> element actually existed in the domain XML. The difference is subtle enough that the test suite is completely unaffected, and we are going to rework the handling of this scenario in a way that restores the original behavior later anyway, so it ultimately doesn't matter. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 18 +----------------- src/qemu/qemu_firmware.c | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 541dad5bdc..25494cb01a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17823,24 +17823,8 @@ virDomainLoaderDefParseXMLLoader(virDomainLoaderDef *loader, { unsigned int format = 0; - if (!loaderNode) { - /* If there is no <loader> element but the <nvram> element - * was present, copy the format from the latter to the - * former. - * - * This ensures that a configuration such as - * - * <os> - * <nvram format='foo'/> - * </os> - * - * behaves as expected, that is, results in a firmware build - * with format 'foo' being selected */ - if (loader->nvram) - loader->format = loader->nvram->format; - + if (!loaderNode) return 0; - } if (virXMLPropTristateBool(loaderNode, "readonly", VIR_XML_PROP_NONE, &loader->readonly) < 0) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 6c609ece6a..a22853361b 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1783,6 +1783,21 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, bool autoSelection = (def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE); int ret; + /* If there is no <loader> element but the <nvram> element + * was present, copy the format from the latter to the + * former. + * + * This ensures that a configuration such as + * + * <os> + * <nvram format='foo'/> + * </os> + * + * behaves as expected, that is, results in a firmware build + * with format 'foo' being selected */ + if (loader && loader->nvram && !loader->format) + loader->format = loader->nvram->format; + /* If we're loading an existing configuration from disk, we * should try as hard as possible to preserve historical * behavior. In particular, firmware autoselection being enabled -- 2.52.0

The combination doesn't make sense. After this change the firmware-manual-bios-rw test cases starts failing, as it should have in the first place. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_validate.c | 14 +++++++ ...are-manual-efi-rw-nvram.x86_64-latest.args | 37 ----------------- ...ware-manual-efi-rw-nvram.x86_64-latest.err | 1 + ...ware-manual-efi-rw-nvram.x86_64-latest.xml | 40 ------------------- tests/qemuxmlconftest.c | 2 +- 5 files changed, 16 insertions(+), 78 deletions(-) delete mode 100644 tests/qemuxmlconfdata/firmware-manual-efi-rw-nvram.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/firmware-manual-efi-rw-nvram.x86_64-latest.err delete mode 100644 tests/qemuxmlconfdata/firmware-manual-efi-rw-nvram.x86_64-latest.xml diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 4558e7b210..09c1b3f13f 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -1765,6 +1765,20 @@ virDomainDefOSValidate(const virDomainDef *def, } } + if (loader->readonly == VIR_TRISTATE_BOOL_NO) { + if (loader->nvramTemplate) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("NVRAM template is not permitted when loader is read/write")); + return -1; + } + + if (loader->nvram) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("NVRAM is not permitted when loader is read/write")); + return -1; + } + } + if (loader->stateless == VIR_TRISTATE_BOOL_YES) { if (loader->nvramTemplate) { virReportError(VIR_ERR_XML_DETAIL, "%s", diff --git a/tests/qemuxmlconfdata/firmware-manual-efi-rw-nvram.x86_64-latest.args b/tests/qemuxmlconfdata/firmware-manual-efi-rw-nvram.x86_64-latest.args deleted file mode 100644 index 6b3eec0a27..0000000000 --- a/tests/qemuxmlconfdata/firmware-manual-efi-rw-nvram.x86_64-latest.args +++ /dev/null @@ -1,37 +0,0 @@ -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.combined.fd","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}' \ --blockdev '{"node-name":"libvirt-pflash0-format","read-only":false,"driver":"raw","file":"libvirt-pflash0-storage"}' \ --blockdev '{"driver":"file","filename":"/path/to/guest_VARS.fd","node-name":"libvirt-pflash1-storage","read-only":false}' \ --machine pc-q35-10.0,usb=off,smm=on,dump-guest-core=off,memory-backend=pc.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-storage,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/qemuxmlconfdata/firmware-manual-efi-rw-nvram.x86_64-latest.err b/tests/qemuxmlconfdata/firmware-manual-efi-rw-nvram.x86_64-latest.err new file mode 100644 index 0000000000..d0cf62061a --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-manual-efi-rw-nvram.x86_64-latest.err @@ -0,0 +1 @@ +NVRAM is not permitted when loader is read/write diff --git a/tests/qemuxmlconfdata/firmware-manual-efi-rw-nvram.x86_64-latest.xml b/tests/qemuxmlconfdata/firmware-manual-efi-rw-nvram.x86_64-latest.xml deleted file mode 100644 index f6436df80f..0000000000 --- a/tests/qemuxmlconfdata/firmware-manual-efi-rw-nvram.x86_64-latest.xml +++ /dev/null @@ -1,40 +0,0 @@ -<domain type='kvm'> - <name>guest</name> - <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> - <memory unit='KiB'>1048576</memory> - <currentMemory unit='KiB'>1048576</currentMemory> - <vcpu placement='static'>1</vcpu> - <os firmware='efi'> - <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> - <firmware> - <feature enabled='yes' name='secure-boot'/> - </firmware> - <loader readonly='no' secure='yes' type='pflash' format='raw'>/usr/share/edk2/ovmf/OVMF.combined.fd</loader> - <nvram format='raw'>/path/to/guest_VARS.fd</nvram> - <boot dev='hd'/> - </os> - <features> - <acpi/> - <smm state='on'/> - </features> - <cpu mode='custom' match='exact' check='none'> - <model fallback='forbid'>qemu64</model> - </cpu> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - <controller type='usb' index='0' model='none'/> - <controller type='sata' index='0'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> - </controller> - <controller type='pci' index='0' model='pcie-root'/> - <input type='mouse' bus='ps2'/> - <input type='keyboard' bus='ps2'/> - <audio id='1' type='none'/> - <watchdog model='itco' action='reset'/> - <memballoon model='none'/> - </devices> -</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index 932d7410d2..1a9dd3b153 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1534,7 +1534,7 @@ mymain(void) DO_TEST_CAPS_LATEST("firmware-manual-efi-rw-legacy-paths"); DO_TEST_CAPS_LATEST("firmware-manual-efi-rw-modern-paths"); DO_TEST_CAPS_LATEST("firmware-manual-efi-rw-implicit"); - DO_TEST_CAPS_LATEST("firmware-manual-efi-rw-nvram"); + DO_TEST_CAPS_LATEST_PARSE_ERROR("firmware-manual-efi-rw-nvram"); DO_TEST_CAPS_LATEST("firmware-manual-efi-loader-secure"); DO_TEST_CAPS_LATEST_PARSE_ERROR("firmware-manual-efi-loader-no-path"); DO_TEST_CAPS_LATEST("firmware-manual-efi-loader-path-nonstandard"); -- 2.52.0

This test case demonstrates a flaw in the XML validation process. ROM images are by definition read-only, so attempting to use one as read/write should have resulted in the domain XML being rejected. The issue will be addressed in an upcoming commit. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ...firmware-manual-bios-rw.x86_64-latest.args | 32 +++++++++++++++++++ .../firmware-manual-bios-rw.x86_64-latest.xml | 28 ++++++++++++++++ .../firmware-manual-bios-rw.xml | 15 +++++++++ tests/qemuxmlconftest.c | 1 + 4 files changed, 76 insertions(+) create mode 100644 tests/qemuxmlconfdata/firmware-manual-bios-rw.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/firmware-manual-bios-rw.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/firmware-manual-bios-rw.xml diff --git a/tests/qemuxmlconfdata/firmware-manual-bios-rw.x86_64-latest.args b/tests/qemuxmlconfdata/firmware-manual-bios-rw.x86_64-latest.args new file mode 100644 index 0000000000..969c7ad68c --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-manual-bios-rw.x86_64-latest.args @@ -0,0 +1,32 @@ +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"}' \ +-machine pc-i440fx-10.0,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=off \ +-accel tcg \ +-cpu qemu64 \ +-bios /usr/share/seabios/bios.bin \ +-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"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxmlconfdata/firmware-manual-bios-rw.x86_64-latest.xml b/tests/qemuxmlconfdata/firmware-manual-bios-rw.x86_64-latest.xml new file mode 100644 index 0000000000..65bb8493c9 --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-manual-bios-rw.x86_64-latest.xml @@ -0,0 +1,28 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-10.0'>hvm</type> + <loader readonly='no' type='rom' format='raw'>/usr/share/seabios/bios.bin</loader> + <boot dev='hd'/> + </os> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/firmware-manual-bios-rw.xml b/tests/qemuxmlconfdata/firmware-manual-bios-rw.xml new file mode 100644 index 0000000000..b12aa67d1a --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-manual-bios-rw.xml @@ -0,0 +1,15 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-10.0'>hvm</type> + <loader readonly='no'>/usr/share/seabios/bios.bin</loader> + </os> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index 37ddda56f6..f7f4aae79b 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1527,6 +1527,7 @@ mymain(void) DO_TEST_CAPS_LATEST("firmware-manual-bios"); DO_TEST_CAPS_LATEST("firmware-manual-bios-stateless"); DO_TEST_CAPS_LATEST_PARSE_ERROR("firmware-manual-bios-not-stateless"); + DO_TEST_CAPS_LATEST("firmware-manual-bios-rw"); DO_TEST_CAPS_LATEST("firmware-manual-efi"); DO_TEST_CAPS_LATEST("firmware-manual-efi-features"); DO_TEST_CAPS_LATEST_ABI_UPDATE_PARSE_ERROR("firmware-manual-efi-features"); -- 2.52.0

This test case demonstrates a flaw in the XML validation process. ROM images are by definition in raw format, so attempting to use any other format should have resulted in the domain XML being rejected. The issue will be addressed in an upcoming commit. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ...format-loader-qcow2-rom.x86_64-latest.args | 34 ++++++++++++++++ ...-format-loader-qcow2-rom.x86_64-latest.xml | 39 +++++++++++++++++++ ...mware-auto-efi-format-loader-qcow2-rom.xml | 18 +++++++++ tests/qemuxmlconftest.c | 1 + 4 files changed, 92 insertions(+) create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-loader-qcow2-rom.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-loader-qcow2-rom.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-loader-qcow2-rom.xml diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-loader-qcow2-rom.x86_64-latest.args b/tests/qemuxmlconfdata/firmware-auto-efi-format-loader-qcow2-rom.x86_64-latest.args new file mode 100644 index 0000000000..417084d45e --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-loader-qcow2-rom.x86_64-latest.args @@ -0,0 +1,34 @@ +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"}' \ +-machine pc-q35-10.0,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=on \ +-accel kvm \ +-cpu qemu64 \ +-bios /usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd \ +-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/qemuxmlconfdata/firmware-auto-efi-format-loader-qcow2-rom.x86_64-latest.xml b/tests/qemuxmlconfdata/firmware-auto-efi-format-loader-qcow2-rom.x86_64-latest.xml new file mode 100644 index 0000000000..862a50ddb4 --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-loader-qcow2-rom.x86_64-latest.xml @@ -0,0 +1,39 @@ +<domain type='kvm'> + <name>guest</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> + <firmware> + <feature enabled='yes' name='enrolled-keys'/> + <feature enabled='yes' name='secure-boot'/> + </firmware> + <loader type='rom' format='qcow2'>/usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd</loader> + <boot dev='hd'/> + </os> + <features> + <acpi/> + </features> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <watchdog model='itco' action='reset'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-loader-qcow2-rom.xml b/tests/qemuxmlconfdata/firmware-auto-efi-format-loader-qcow2-rom.xml new file mode 100644 index 0000000000..abc2dc6d31 --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-loader-qcow2-rom.xml @@ -0,0 +1,18 @@ +<domain type='kvm'> + <name>guest</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> + <loader type='rom' format='qcow2'/> + </os> + <features> + <acpi/> + </features> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index 5f3cfe23fd..c3bb768ecb 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1601,6 +1601,7 @@ mymain(void) DO_TEST_CAPS_LATEST("firmware-auto-efi-nvram-network-iscsi"); DO_TEST_CAPS_LATEST("firmware-auto-efi-format-loader-qcow2"); + DO_TEST_CAPS_LATEST("firmware-auto-efi-format-loader-qcow2-rom"); DO_TEST_CAPS_LATEST("firmware-auto-efi-format-loader-qcow2-nvram-path"); DO_TEST_CAPS_LATEST("firmware-auto-efi-format-nvram-qcow2"); DO_TEST_CAPS_LATEST("firmware-auto-efi-format-nvram-qcow2-path"); -- 2.52.0

For combined firmware builds, the variable storage is part of the same image as the executable code, whereas stateless builds don't support variable storage at all. In both cases, the use of a separate NVRAM storage area is not supported, so if attributes connected to one are present in the domain XML, firmware descriptors for stateless/combined builds should be ignored. ROM firmware builds are stateless by definition, so the same handling applies to them as well. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_firmware.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index a22853361b..47a3987b64 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1285,6 +1285,17 @@ qemuFirmwareMatchDomain(const virDomainDef *def, flash->nvram_template.format); return false; } + } else { + if (loader && loader->nvram && + (loader->nvram->path || loader->nvram->format)) { + VIR_DEBUG("Discarding non split loader (nvram configured)"); + return false; + } + if (loader && + (loader->nvramTemplate || loader->nvramTemplateFormat)) { + VIR_DEBUG("Discarding non split loader (nvram template configured)"); + return false; + } } } else if (fw->mapping.device == QEMU_FIRMWARE_DEVICE_MEMORY) { if (loader && loader->type && @@ -1302,6 +1313,17 @@ qemuFirmwareMatchDomain(const virDomainDef *def, VIR_DEBUG("Discarding readonly loader"); return false; } + + if (loader && loader->nvram && + (loader->nvram->path || loader->nvram->format)) { + VIR_DEBUG("Discarding rom loader (nvram configured)"); + return false; + } + if (loader && + (loader->nvramTemplate || loader->nvramTemplateFormat)) { + VIR_DEBUG("Discarding rom loader (nvram template configured)"); + return false; + } } if (def->sec) { -- 2.52.0

Similarly to how we allow the format for the loader and the NVRAM file to be specified without the corresponding path being present, we should allow that to happen for the NVRAM template too. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/schemas/domaincommon.rng | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index 1f9ac102a0..c7346526ef 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -354,11 +354,11 @@ <attribute name="template"> <ref name="absFilePath"/> </attribute> - <optional> - <attribute name="templateFormat"> - <ref name="pflashFormatTypes"/> - </attribute> - </optional> + </optional> + <optional> + <attribute name="templateFormat"> + <ref name="pflashFormatTypes"/> + </attribute> </optional> <optional> <ref name="pflashFormat"/> -- 2.52.0

This test cases demonstrates that it's possible to use a custom NVRAM template together with a standard firmware binary even when referring to the latter by its legacy path rather than its modern, canonical one. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ...onstandard-legacy-paths.x86_64-latest.args | 37 +++++++++++++++++++ ...nonstandard-legacy-paths.x86_64-latest.xml | 37 +++++++++++++++++++ ...vram-template-nonstandard-legacy-paths.xml | 20 ++++++++++ tests/qemuxmlconftest.c | 1 + 4 files changed, 95 insertions(+) create mode 100644 tests/qemuxmlconfdata/firmware-manual-efi-nvram-template-nonstandard-legacy-paths.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/firmware-manual-efi-nvram-template-nonstandard-legacy-paths.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/firmware-manual-efi-nvram-template-nonstandard-legacy-paths.xml diff --git a/tests/qemuxmlconfdata/firmware-manual-efi-nvram-template-nonstandard-legacy-paths.x86_64-latest.args b/tests/qemuxmlconfdata/firmware-manual-efi-nvram-template-nonstandard-legacy-paths.x86_64-latest.args new file mode 100644 index 0000000000..18ca736065 --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-manual-efi-nvram-template-nonstandard-legacy-paths.x86_64-latest.args @@ -0,0 +1,37 @@ +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/OVMF/OVMF_CODE.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","read-only":false}' \ +-machine pc-q35-10.0,usb=off,smm=on,dump-guest-core=off,memory-backend=pc.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-storage,acpi=on \ +-accel tcg \ +-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/qemuxmlconfdata/firmware-manual-efi-nvram-template-nonstandard-legacy-paths.x86_64-latest.xml b/tests/qemuxmlconfdata/firmware-manual-efi-nvram-template-nonstandard-legacy-paths.x86_64-latest.xml new file mode 100644 index 0000000000..8073a042f9 --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-manual-efi-nvram-template-nonstandard-legacy-paths.x86_64-latest.xml @@ -0,0 +1,37 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> + <loader readonly='yes' secure='yes' type='pflash' format='raw'>/usr/share/OVMF/OVMF_CODE.fd</loader> + <nvram template='/path/to/OVMF_VARS.fd' templateFormat='raw' format='raw'>/var/lib/libvirt/qemu/nvram/guest_VARS.fd</nvram> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <smm state='on'/> + </features> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <watchdog model='itco' action='reset'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/firmware-manual-efi-nvram-template-nonstandard-legacy-paths.xml b/tests/qemuxmlconfdata/firmware-manual-efi-nvram-template-nonstandard-legacy-paths.xml new file mode 100644 index 0000000000..f32d29c6f5 --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-manual-efi-nvram-template-nonstandard-legacy-paths.xml @@ -0,0 +1,20 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> + <loader readonly='yes' secure='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> + <nvram template='/path/to/OVMF_VARS.fd'/> + </os> + <features> + <acpi/> + <smm state='on'/> + </features> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index a2eb689a64..a3bf82dae3 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1546,6 +1546,7 @@ mymain(void) DO_TEST_CAPS_LATEST("firmware-manual-efi-nvram-template"); DO_TEST_CAPS_LATEST("firmware-manual-efi-nvram-template-nonstandard"); DO_TEST_CAPS_LATEST("firmware-manual-efi-nvram-template-nonstandard-format"); + DO_TEST_CAPS_LATEST("firmware-manual-efi-nvram-template-nonstandard-legacy-paths"); DO_TEST_CAPS_LATEST_PARSE_ERROR("firmware-manual-efi-nvram-template-stateless"); DO_TEST_CAPS_LATEST("firmware-manual-efi-nvram-network-iscsi"); DO_TEST_CAPS_LATEST("firmware-manual-efi-nvram-network-nbd"); -- 2.52.0

This test case demonstrates an issue with the current implementation of firmware autoselection. libvirt would normally be able to find the firmware descriptor for the binary mentioned in the domain XML, but the fact that at the same time we're asking for the NVRAM file to be of a different format throws a spanner in the works. Of course there is no requirement for the format of the NVRAM file to match that of the NVRAM template, so the fact that libvirt is unable to produce a working configuration out of this input is an issues that will be addressed in an upcoming commit. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ...at-nvram-raw-loader-path.x86_64-latest.err | 1 + ...at-nvram-raw-loader-path.x86_64-latest.xml | 36 +++++++++++++++++++ ...-auto-efi-format-nvram-raw-loader-path.xml | 19 ++++++++++ tests/qemuxmlconftest.c | 1 + 4 files changed, 57 insertions(+) create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.err create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.xml diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.err b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.err new file mode 100644 index 0000000000..3edb2b3451 --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.err @@ -0,0 +1 @@ +operation failed: Unable to find 'efi' firmware that is compatible with the current configuration diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.xml b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.xml new file mode 100644 index 0000000000..6bb1ad1507 --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.xml @@ -0,0 +1,36 @@ +<domain type='kvm'> + <name>guest</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> + <loader type='pflash' format='raw'>/usr/share/edk2/ovmf/OVMF_CODE_4M.secboot.qcow2</loader> + <nvram format='raw'/> + <boot dev='hd'/> + </os> + <features> + <acpi/> + </features> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <watchdog model='itco' action='reset'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.xml b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.xml new file mode 100644 index 0000000000..66e6910fc2 --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.xml @@ -0,0 +1,19 @@ +<domain type='kvm'> + <name>guest</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> + <loader type='pflash'>/usr/share/edk2/ovmf/OVMF_CODE_4M.secboot.qcow2</loader> + <nvram format='raw'/> + </os> + <features> + <acpi/> + </features> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index 513064a0c7..4c08a77f12 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1610,6 +1610,7 @@ mymain(void) DO_TEST_CAPS_LATEST("firmware-auto-efi-format-nvram-qcow2-network-nbd"); DO_TEST_CAPS_LATEST("firmware-auto-efi-format-nvram-raw"); DO_TEST_CAPS_LATEST_ABI_UPDATE("firmware-auto-efi-format-nvram-raw"); + DO_TEST_CAPS_LATEST_FAILURE("firmware-auto-efi-format-nvram-raw-loader-path"); 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", "aarch64"); DO_TEST_CAPS_LATEST("firmware-auto-efi-format-mismatch"); -- 2.52.0

This test case demonstrates an issue with the current implementation of firmware autoselection. While the test case passes, the outcome is not the desired one. The domain XML explicitly requests that the NVRAM template is in qcow2 format, and yet the selected firmware build uses the raw format for the NVRAM template instead. The issue will be addressed in an upcoming commit. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ...mat-nvramtemplate-qcow2.x86_64-latest.args | 37 +++++++++++++++++ ...rmat-nvramtemplate-qcow2.x86_64-latest.xml | 41 +++++++++++++++++++ ...re-auto-efi-format-nvramtemplate-qcow2.xml | 18 ++++++++ tests/qemuxmlconftest.c | 1 + 4 files changed, 97 insertions(+) create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.xml diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.args b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.args new file mode 100644 index 0000000000..e7c9110c95 --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.args @@ -0,0 +1,37 @@ +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.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","read-only":false}' \ +-machine pc-q35-10.0,usb=off,smm=on,dump-guest-core=off,memory-backend=pc.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-storage,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/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.xml b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.xml new file mode 100644 index 0000000000..f4df8c07ed --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.xml @@ -0,0 +1,41 @@ +<domain type='kvm'> + <name>guest</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> + <firmware> + <feature enabled='yes' name='enrolled-keys'/> + <feature enabled='yes' name='secure-boot'/> + </firmware> + <loader readonly='yes' secure='yes' type='pflash' format='raw'>/usr/share/edk2/ovmf/OVMF_CODE.secboot.fd</loader> + <nvram template='/usr/share/edk2/ovmf/OVMF_VARS.secboot.fd' templateFormat='raw' format='raw'>/var/lib/libvirt/qemu/nvram/guest_VARS.fd</nvram> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <smm state='on'/> + </features> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <watchdog model='itco' action='reset'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.xml b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.xml new file mode 100644 index 0000000000..582b2636e4 --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.xml @@ -0,0 +1,18 @@ +<domain type='kvm'> + <name>guest</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> + <nvram templateFormat='qcow2'/> + </os> + <features> + <acpi/> + </features> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index d061444226..ea61e2025d 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1614,6 +1614,7 @@ mymain(void) DO_TEST_CAPS_LATEST_FAILURE("firmware-auto-efi-format-nvram-raw-nvramtemplate-path"); 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", "aarch64"); + DO_TEST_CAPS_LATEST("firmware-auto-efi-format-nvramtemplate-qcow2"); DO_TEST_CAPS_LATEST("firmware-auto-efi-format-mismatch"); /* This test passes, but the outcome is not the desired one: the -- 2.52.0

Simple helper for the case where completely custom firmware paths are in use. It's quite trivial right now, but it will be expanded slightly in an upcoming commit. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_firmware.c | 38 ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 9dff3828a2..9b6c14701f 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1601,6 +1601,32 @@ qemuFirmwareFetchParsedConfigs(bool privileged, } +/** + * qemuFirmwareFillDomainCustom: + * @def: domain definition + * + * Fill in whatever information we can when totally custom firmware + * paths are in use. + * + * Should only be used as a fallback in case looking at the firmware + * descriptors yielded no results, and neither did going through the + * legacy list of CODE:VARS pairs. + */ +static void +qemuFirmwareFillDomainCustom(virDomainDef *def) +{ + virDomainLoaderDef *loader = def->os.loader; + + if (!loader) + return; + + if (!loader->format) + loader->format = VIR_STORAGE_FILE_RAW; + + return; +} + + /** * qemuFirmwareFillDomainLegacy: * @driver: QEMU driver @@ -1890,15 +1916,11 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, 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; - } + /* If we've gotten this far without finding a match, + * it means that we're dealing with a set of completely + * custom paths. We can still fill in some information */ + qemuFirmwareFillDomainCustom(def); } } else { virReportError(VIR_ERR_OPERATION_FAILED, -- 2.52.0

Currently we're doing a weird dance to avoid overwriting the user-provided path to the NVRAM template, which might potentially be one we actually know about but just so happens not to be listed first. Explaining why we're doing things this way requires a fairly long comment. We can make things simpler: if the NVRAM template path is present in the domain XML, include it into the matching criteria. This is consistent with how we match firmware descriptors. Handling of format, both for the firmware executable and the NVRAM template, is improved too. Legacy paths were used before non-raw firmware builds existed, so we can set the format to raw unconditionally. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_firmware.c | 65 +++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 35 deletions(-) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 2b16d66818..a7bb8f7e45 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1653,6 +1653,7 @@ qemuFirmwareFillDomainLegacy(virQEMUDriver *driver, { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virDomainLoaderDef *loader = def->os.loader; + virFirmware *theone = NULL; size_t i; if (!loader) @@ -1681,6 +1682,13 @@ qemuFirmwareFillDomainLegacy(virQEMUDriver *driver, return 1; } + if (loader->nvramTemplateFormat && + loader->nvramTemplateFormat != VIR_STORAGE_FILE_RAW) { + VIR_DEBUG("Ignoring legacy entries for loader with nvram template format '%s'", + virStorageFileFormatTypeToString(loader->nvramTemplateFormat)); + return 1; + } + for (i = 0; i < cfg->nfirmwares; i++) { virFirmware *fw = cfg->firmwares[i]; @@ -1690,47 +1698,34 @@ qemuFirmwareFillDomainLegacy(virQEMUDriver *driver, continue; } - loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH; - loader->readonly = VIR_TRISTATE_BOOL_YES; - loader->format = VIR_STORAGE_FILE_RAW; - - /* Only use the default template path if one hasn't been - * provided by the user. Assume that the template is in 'raw' format. - * - * In addition to fully-custom templates, which are a valid - * use case, we could simply be in a situation where - * qemu.conf contains - * - * nvram = [ - * "/path/to/OVMF_CODE.secboot.fd:/path/to/OVMF_VARS.fd", - * "/path/to/OVMF_CODE.secboot.fd:/path/to/OVMF_VARS.secboot.fd" - * ] - * - * and the domain has been configured as - * - * <os> - * <loader readonly='yes' type='pflash'>/path/to/OVMF_CODE.secboot.fd</loader> - * <nvram template='/path/to/OVMF/OVMF_VARS.secboot.fd'> - * </os> - * - * In this case, the global default is to have Secure Boot - * disabled, but the domain configuration explicitly enables - * it, and we shouldn't overrule this choice */ - if (!loader->nvramTemplate) { - loader->nvramTemplate = g_strdup(cfg->firmwares[i]->nvram); - loader->nvramTemplateFormat = VIR_STORAGE_FILE_RAW; + if (loader->nvramTemplate && + !virFileComparePaths(fw->nvram, loader->nvramTemplate)) { + VIR_DEBUG("Not matching nvram template path '%s' for user provided path '%s'", + fw->nvram, loader->nvramTemplate); + continue; } - if (loader->nvramTemplateFormat == VIR_STORAGE_FILE_NONE) - loader->nvramTemplateFormat = VIR_STORAGE_FILE_RAW; + theone = fw; + break; + } - VIR_DEBUG("decided on firmware '%s' template '%s'", - loader->path, NULLSTR(loader->nvramTemplate)); + if (!theone) + return 1; - return 0; + loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH; + loader->readonly = VIR_TRISTATE_BOOL_YES; + + loader->format = VIR_STORAGE_FILE_RAW; + loader->nvramTemplateFormat = VIR_STORAGE_FILE_RAW; + + if (!loader->nvramTemplate) { + loader->nvramTemplate = g_strdup(theone->nvram); } - return 1; + VIR_DEBUG("decided on firmware '%s' template '%s'", + loader->path, loader->nvramTemplate); + + return 0; } -- 2.52.0

In the vast majority of cases they will match, but it just makes more logical sense to copy the format from the NVRAM template to the NVRAM file itself. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_firmware.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index f32e46cc8c..b08fb95585 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -991,7 +991,10 @@ qemuFirmwareEnsureNVRAM(virDomainDef *def, * NVRAM format if that's missing though */ if (!virStorageSourceIsEmpty(loader->nvram)) { if (!loader->nvram->format) { - loader->nvram->format = loader->format; + if (loader->nvramTemplateFormat) + loader->nvram->format = loader->nvramTemplateFormat; + else + loader->nvram->format = loader->format; } return; } -- 2.52.0

If the user has specified a desired format for the NVRAM template, we should take that information into account when looking for a suitable firmware build instead of ignoring it. Two test cases start failing as a result of this change. For firmware-auto-efi-format-nvramtemplate-qcow2, the failure is temporary and the test case will pass once again with an upcoming commit. It should be noted that, until now, the selected firmware used raw, not qcow2, as the NVRAM template format, meaning that though the test case passed the outcome was not the desired one. For firmware-auto-efi-format-mismatch-nvramtemplate, the failure is desired and the test case should not have succeeded in the first place, as there are no firmware descriptors for a build that uses raw format for the executable and qcow2 format for the NVRAM template. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_firmware.c | 7 ++++ ...-mismatch-nvramtemplate.x86_64-latest.args | 37 ------------------- ...t-mismatch-nvramtemplate.x86_64-latest.err | 1 + ...t-mismatch-nvramtemplate.x86_64-latest.xml | 9 +---- ...mat-nvramtemplate-qcow2.x86_64-latest.args | 37 ------------------- ...rmat-nvramtemplate-qcow2.x86_64-latest.err | 1 + ...rmat-nvramtemplate-qcow2.x86_64-latest.xml | 9 +---- tests/qemuxmlconftest.c | 4 +- 8 files changed, 15 insertions(+), 90 deletions(-) delete mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-mismatch-nvramtemplate.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-mismatch-nvramtemplate.x86_64-latest.err delete mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.err diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index dca0a79868..e13cce0887 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1297,6 +1297,13 @@ qemuFirmwareMatchDomain(const virDomainDef *def, flash->nvram_template.format); return false; } + if (loader && loader->nvramTemplateFormat && + STRNEQ(flash->nvram_template.format, virStorageFileFormatTypeToString(loader->nvramTemplateFormat))) { + VIR_DEBUG("Discarding loader with mismatching nvram template format '%s' != '%s'", + flash->nvram_template.format, + virStorageFileFormatTypeToString(loader->nvramTemplateFormat)); + return false; + } } else { if (loader && loader->nvram && (loader->nvram->path || loader->nvram->format)) { diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-mismatch-nvramtemplate.x86_64-latest.args b/tests/qemuxmlconfdata/firmware-auto-efi-format-mismatch-nvramtemplate.x86_64-latest.args deleted file mode 100644 index e7c9110c95..0000000000 --- a/tests/qemuxmlconfdata/firmware-auto-efi-format-mismatch-nvramtemplate.x86_64-latest.args +++ /dev/null @@ -1,37 +0,0 @@ -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.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","read-only":false}' \ --machine pc-q35-10.0,usb=off,smm=on,dump-guest-core=off,memory-backend=pc.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-storage,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/qemuxmlconfdata/firmware-auto-efi-format-mismatch-nvramtemplate.x86_64-latest.err b/tests/qemuxmlconfdata/firmware-auto-efi-format-mismatch-nvramtemplate.x86_64-latest.err new file mode 100644 index 0000000000..3edb2b3451 --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-mismatch-nvramtemplate.x86_64-latest.err @@ -0,0 +1 @@ +operation failed: Unable to find 'efi' firmware that is compatible with the current configuration diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-mismatch-nvramtemplate.x86_64-latest.xml b/tests/qemuxmlconfdata/firmware-auto-efi-format-mismatch-nvramtemplate.x86_64-latest.xml index f4df8c07ed..1f039061ba 100644 --- a/tests/qemuxmlconfdata/firmware-auto-efi-format-mismatch-nvramtemplate.x86_64-latest.xml +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-mismatch-nvramtemplate.x86_64-latest.xml @@ -6,17 +6,12 @@ <vcpu placement='static'>1</vcpu> <os firmware='efi'> <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> - <firmware> - <feature enabled='yes' name='enrolled-keys'/> - <feature enabled='yes' name='secure-boot'/> - </firmware> - <loader readonly='yes' secure='yes' type='pflash' format='raw'>/usr/share/edk2/ovmf/OVMF_CODE.secboot.fd</loader> - <nvram template='/usr/share/edk2/ovmf/OVMF_VARS.secboot.fd' templateFormat='raw' format='raw'>/var/lib/libvirt/qemu/nvram/guest_VARS.fd</nvram> + <loader format='raw'/> + <nvram templateFormat='qcow2'/> <boot dev='hd'/> </os> <features> <acpi/> - <smm state='on'/> </features> <cpu mode='custom' match='exact' check='none'> <model fallback='forbid'>qemu64</model> diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.args b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.args deleted file mode 100644 index e7c9110c95..0000000000 --- a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.args +++ /dev/null @@ -1,37 +0,0 @@ -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.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","read-only":false}' \ --machine pc-q35-10.0,usb=off,smm=on,dump-guest-core=off,memory-backend=pc.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-storage,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/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.err b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.err new file mode 100644 index 0000000000..3edb2b3451 --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.err @@ -0,0 +1 @@ +operation failed: Unable to find 'efi' firmware that is compatible with the current configuration diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.xml b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.xml index f4df8c07ed..1f039061ba 100644 --- a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.xml +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.xml @@ -6,17 +6,12 @@ <vcpu placement='static'>1</vcpu> <os firmware='efi'> <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> - <firmware> - <feature enabled='yes' name='enrolled-keys'/> - <feature enabled='yes' name='secure-boot'/> - </firmware> - <loader readonly='yes' secure='yes' type='pflash' format='raw'>/usr/share/edk2/ovmf/OVMF_CODE.secboot.fd</loader> - <nvram template='/usr/share/edk2/ovmf/OVMF_VARS.secboot.fd' templateFormat='raw' format='raw'>/var/lib/libvirt/qemu/nvram/guest_VARS.fd</nvram> + <loader format='raw'/> + <nvram templateFormat='qcow2'/> <boot dev='hd'/> </os> <features> <acpi/> - <smm state='on'/> </features> <cpu mode='custom' match='exact' check='none'> <model fallback='forbid'>qemu64</model> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index 223310e8a2..75db051e32 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1614,9 +1614,9 @@ mymain(void) DO_TEST_CAPS_LATEST_FAILURE("firmware-auto-efi-format-nvram-raw-nvramtemplate-path"); 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", "aarch64"); - DO_TEST_CAPS_LATEST("firmware-auto-efi-format-nvramtemplate-qcow2"); + DO_TEST_CAPS_LATEST_FAILURE("firmware-auto-efi-format-nvramtemplate-qcow2"); DO_TEST_CAPS_LATEST("firmware-auto-efi-format-mismatch"); - DO_TEST_CAPS_LATEST("firmware-auto-efi-format-mismatch-nvramtemplate"); + DO_TEST_CAPS_LATEST_FAILURE("firmware-auto-efi-format-mismatch-nvramtemplate"); /* This test passes, but the outcome is not the desired one: the * generic edk2 build gets selected instead of the AMD SEV one */ -- 2.52.0

Now that the hack is gone, a few test cases that were failing before start succeeding instead. The firmware-auto-efi-format-nvramtemplate-qcow2 test case originally passed but produced wrong results, then started failing once we began taking templateFormat into account, and now passes once again, finally producing the correct results. The firmware-auto-efi-format-nvram-raw-loader-path and firmware-auto-efi-format-nvram-raw-nvramtemplate-path test cases, on the other hand, never passed before now, because the hack resulted in information contradicting those provided by the user being injected into the configuration, which in turn made it impossible to successfully pick a firmware build. With the hack gone they can finally succeed, as they should have in the first place. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_firmware.c | 19 ++-------- ...t-nvram-raw-loader-path.x86_64-latest.args | 37 ++++++++++++++++++ ...at-nvram-raw-loader-path.x86_64-latest.err | 1 - ...at-nvram-raw-loader-path.x86_64-latest.xml | 9 ++++- ...-raw-nvramtemplate-path.x86_64-latest.args | 37 ++++++++++++++++++ ...m-raw-nvramtemplate-path.x86_64-latest.err | 1 - ...m-raw-nvramtemplate-path.x86_64-latest.xml | 9 ++++- ...mat-nvramtemplate-qcow2.x86_64-latest.args | 38 +++++++++++++++++++ ...rmat-nvramtemplate-qcow2.x86_64-latest.err | 1 - ...rmat-nvramtemplate-qcow2.x86_64-latest.xml | 9 ++++- tests/qemuxmlconftest.c | 6 +-- 11 files changed, 139 insertions(+), 28 deletions(-) create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.args delete mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.err create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-nvramtemplate-path.x86_64-latest.args delete mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-nvramtemplate-path.x86_64-latest.err create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.args delete mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.err diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 8714538ba3..70ac88c373 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1859,21 +1859,6 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, bool autoSelection = (def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE); int ret; - /* If there is no <loader> element but the <nvram> element - * was present, copy the format from the latter to the - * former. - * - * This ensures that a configuration such as - * - * <os> - * <nvram format='foo'/> - * </os> - * - * behaves as expected, that is, results in a firmware build - * with format 'foo' being selected */ - if (loader && loader->nvram && !loader->format) - loader->format = loader->nvram->format; - /* If we're loading an existing configuration from disk, we * should try as hard as possible to preserve historical * behavior. In particular, firmware autoselection being enabled @@ -1889,7 +1874,9 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, def->os.loader = virDomainLoaderDefNew(); loader = def->os.loader; } - if (!loader->format) { + if (!loader->format && + !loader->nvramTemplateFormat && + (!loader->nvram || !loader->nvram->format)) { loader->format = VIR_STORAGE_FILE_RAW; } } diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.args b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.args new file mode 100644 index 0000000000..14027c21db --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.args @@ -0,0 +1,37 @@ +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","backing":null}' \ +-blockdev '{"driver":"file","filename":"/var/lib/libvirt/qemu/nvram/guest_VARS.fd","node-name":"libvirt-pflash1-storage","read-only":false}' \ +-machine pc-q35-10.0,usb=off,smm=on,dump-guest-core=off,memory-backend=pc.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-storage,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/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.err b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.err deleted file mode 100644 index 3edb2b3451..0000000000 --- a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.err +++ /dev/null @@ -1 +0,0 @@ -operation failed: Unable to find 'efi' firmware that is compatible with the current configuration diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.xml b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.xml index 6bb1ad1507..a02714d7b9 100644 --- a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.xml +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-loader-path.x86_64-latest.xml @@ -6,12 +6,17 @@ <vcpu placement='static'>1</vcpu> <os firmware='efi'> <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> - <loader type='pflash' format='raw'>/usr/share/edk2/ovmf/OVMF_CODE_4M.secboot.qcow2</loader> - <nvram format='raw'/> + <firmware> + <feature enabled='yes' name='enrolled-keys'/> + <feature enabled='yes' name='secure-boot'/> + </firmware> + <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' templateFormat='qcow2' format='raw'>/var/lib/libvirt/qemu/nvram/guest_VARS.fd</nvram> <boot dev='hd'/> </os> <features> <acpi/> + <smm state='on'/> </features> <cpu mode='custom' match='exact' check='none'> <model fallback='forbid'>qemu64</model> diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-nvramtemplate-path.x86_64-latest.args b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-nvramtemplate-path.x86_64-latest.args new file mode 100644 index 0000000000..14027c21db --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-nvramtemplate-path.x86_64-latest.args @@ -0,0 +1,37 @@ +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","backing":null}' \ +-blockdev '{"driver":"file","filename":"/var/lib/libvirt/qemu/nvram/guest_VARS.fd","node-name":"libvirt-pflash1-storage","read-only":false}' \ +-machine pc-q35-10.0,usb=off,smm=on,dump-guest-core=off,memory-backend=pc.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-storage,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/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-nvramtemplate-path.x86_64-latest.err b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-nvramtemplate-path.x86_64-latest.err deleted file mode 100644 index 3edb2b3451..0000000000 --- a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-nvramtemplate-path.x86_64-latest.err +++ /dev/null @@ -1 +0,0 @@ -operation failed: Unable to find 'efi' firmware that is compatible with the current configuration diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-nvramtemplate-path.x86_64-latest.xml b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-nvramtemplate-path.x86_64-latest.xml index 8bb8f1b26c..a02714d7b9 100644 --- a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-nvramtemplate-path.x86_64-latest.xml +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvram-raw-nvramtemplate-path.x86_64-latest.xml @@ -6,12 +6,17 @@ <vcpu placement='static'>1</vcpu> <os firmware='efi'> <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> - <loader format='raw'/> - <nvram template='/usr/share/edk2/ovmf/OVMF_VARS_4M.secboot.qcow2' format='raw'/> + <firmware> + <feature enabled='yes' name='enrolled-keys'/> + <feature enabled='yes' name='secure-boot'/> + </firmware> + <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' templateFormat='qcow2' format='raw'>/var/lib/libvirt/qemu/nvram/guest_VARS.fd</nvram> <boot dev='hd'/> </os> <features> <acpi/> + <smm state='on'/> </features> <cpu mode='custom' match='exact' check='none'> <model fallback='forbid'>qemu64</model> diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.args b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.args new file mode 100644 index 0000000000..468ad4326c --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.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","backing":null}' \ +-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","backing":null}' \ +-machine pc-q35-10.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/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.err b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.err deleted file mode 100644 index 3edb2b3451..0000000000 --- a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.err +++ /dev/null @@ -1 +0,0 @@ -operation failed: Unable to find 'efi' firmware that is compatible with the current configuration diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.xml b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.xml index 1f039061ba..4061a0ae35 100644 --- a/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.xml +++ b/tests/qemuxmlconfdata/firmware-auto-efi-format-nvramtemplate-qcow2.x86_64-latest.xml @@ -6,12 +6,17 @@ <vcpu placement='static'>1</vcpu> <os firmware='efi'> <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> - <loader format='raw'/> - <nvram templateFormat='qcow2'/> + <firmware> + <feature enabled='yes' name='enrolled-keys'/> + <feature enabled='yes' name='secure-boot'/> + </firmware> + <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' templateFormat='qcow2' format='qcow2'>/var/lib/libvirt/qemu/nvram/guest_VARS.qcow2</nvram> <boot dev='hd'/> </os> <features> <acpi/> + <smm state='on'/> </features> <cpu mode='custom' match='exact' check='none'> <model fallback='forbid'>qemu64</model> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index 75db051e32..a87863410f 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1610,11 +1610,11 @@ mymain(void) DO_TEST_CAPS_LATEST("firmware-auto-efi-format-nvram-qcow2-network-nbd"); DO_TEST_CAPS_LATEST("firmware-auto-efi-format-nvram-raw"); DO_TEST_CAPS_LATEST_ABI_UPDATE("firmware-auto-efi-format-nvram-raw"); - DO_TEST_CAPS_LATEST_FAILURE("firmware-auto-efi-format-nvram-raw-loader-path"); - DO_TEST_CAPS_LATEST_FAILURE("firmware-auto-efi-format-nvram-raw-nvramtemplate-path"); + DO_TEST_CAPS_LATEST("firmware-auto-efi-format-nvram-raw-loader-path"); + DO_TEST_CAPS_LATEST("firmware-auto-efi-format-nvram-raw-nvramtemplate-path"); 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", "aarch64"); - DO_TEST_CAPS_LATEST_FAILURE("firmware-auto-efi-format-nvramtemplate-qcow2"); + DO_TEST_CAPS_LATEST("firmware-auto-efi-format-nvramtemplate-qcow2"); DO_TEST_CAPS_LATEST("firmware-auto-efi-format-mismatch"); DO_TEST_CAPS_LATEST_FAILURE("firmware-auto-efi-format-mismatch-nvramtemplate"); -- 2.52.0

This test case demonstrates that firmware selection does not run for domains manually configured to use the Intel TDX build of edk2, and as a result some expected information is missing; in particular, the fact that the firmware type is EFI is not reflected in the domain XML. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ...manual-efi-tdx.x86_64-latest+inteltdx.args | 34 +++++++++++++++ ...-manual-efi-tdx.x86_64-latest+inteltdx.xml | 42 +++++++++++++++++++ .../firmware-manual-efi-tdx.xml | 25 +++++++++++ tests/qemuxmlconftest.c | 3 ++ 4 files changed, 104 insertions(+) create mode 100644 tests/qemuxmlconfdata/firmware-manual-efi-tdx.x86_64-latest+inteltdx.args create mode 100644 tests/qemuxmlconfdata/firmware-manual-efi-tdx.x86_64-latest+inteltdx.xml create mode 100644 tests/qemuxmlconfdata/firmware-manual-efi-tdx.xml diff --git a/tests/qemuxmlconfdata/firmware-manual-efi-tdx.x86_64-latest+inteltdx.args b/tests/qemuxmlconfdata/firmware-manual-efi-tdx.x86_64-latest+inteltdx.args new file mode 100644 index 0000000000..33a73bfc10 --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-manual-efi-tdx.x86_64-latest+inteltdx.args @@ -0,0 +1,34 @@ +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"}' \ +-machine pc-q35-10.0,usb=off,dump-guest-core=off,memory-backend=pc.ram,confidential-guest-support=lsec0,acpi=on \ +-accel kvm \ +-cpu qemu64 \ +-bios /usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd \ +-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 \ +-object '{"qom-type":"tdx-guest","id":"lsec0","mrconfigid":"ASNFZ4mrze8BI0VniavN7wEjRWeJq83vASNFZ4mrze8BI0VniavN7wEjRWeJq83v","mrowner":"ASNFZ4mrze8BI0VniavN7wEjRWeJq83vASNFZ4mrze8BI0VniavN7wEjRWeJq83v","mrownerconfig":"ASNFZ4mrze8BI0VniavN7wEjRWeJq83vASNFZ4mrze8BI0VniavN7wEjRWeJq83v","quote-generation-socket":{"type":"unix","path":"/var/run/tdx-qgs/qgs.socket"},"attributes":268435456}' \ +-msg timestamp=on diff --git a/tests/qemuxmlconfdata/firmware-manual-efi-tdx.x86_64-latest+inteltdx.xml b/tests/qemuxmlconfdata/firmware-manual-efi-tdx.x86_64-latest+inteltdx.xml new file mode 100644 index 0000000000..7428a3dfef --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-manual-efi-tdx.x86_64-latest+inteltdx.xml @@ -0,0 +1,42 @@ +<domain type='kvm'> + <name>guest</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> + <loader readonly='yes' type='rom'>/usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd</loader> + <boot dev='hd'/> + </os> + <features> + <acpi/> + </features> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <watchdog model='itco' action='reset'/> + <memballoon model='none'/> + </devices> + <launchSecurity type='tdx'> + <policy>0x10000000</policy> + <mrConfigId>ASNFZ4mrze8BI0VniavN7wEjRWeJq83vASNFZ4mrze8BI0VniavN7wEjRWeJq83v</mrConfigId> + <mrOwner>ASNFZ4mrze8BI0VniavN7wEjRWeJq83vASNFZ4mrze8BI0VniavN7wEjRWeJq83v</mrOwner> + <mrOwnerConfig>ASNFZ4mrze8BI0VniavN7wEjRWeJq83vASNFZ4mrze8BI0VniavN7wEjRWeJq83v</mrOwnerConfig> + <quoteGenerationService path='/var/run/tdx-qgs/qgs.socket'/> + </launchSecurity> +</domain> diff --git a/tests/qemuxmlconfdata/firmware-manual-efi-tdx.xml b/tests/qemuxmlconfdata/firmware-manual-efi-tdx.xml new file mode 100644 index 0000000000..ee9d63c5fe --- /dev/null +++ b/tests/qemuxmlconfdata/firmware-manual-efi-tdx.xml @@ -0,0 +1,25 @@ +<domain type='kvm'> + <name>guest</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> + <loader readonly='yes' type='rom'>/usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd</loader> + </os> + <features> + <acpi/> + </features> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='none'/> + <memballoon model='none'/> + </devices> + <launchSecurity type='tdx'> + <policy>0x10000000</policy> + <mrConfigId>ASNFZ4mrze8BI0VniavN7wEjRWeJq83vASNFZ4mrze8BI0VniavN7wEjRWeJq83v</mrConfigId> + <mrOwner>ASNFZ4mrze8BI0VniavN7wEjRWeJq83vASNFZ4mrze8BI0VniavN7wEjRWeJq83v</mrOwner> + <mrOwnerConfig>ASNFZ4mrze8BI0VniavN7wEjRWeJq83vASNFZ4mrze8BI0VniavN7wEjRWeJq83v</mrOwnerConfig> + <quoteGenerationService path='/var/run/tdx-qgs/qgs.socket'/> + </launchSecurity> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index 61fd4b5c3e..89b8ad1a35 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1556,6 +1556,9 @@ mymain(void) DO_TEST_CAPS_ARCH_LATEST_FULL("firmware-manual-efi-sev-snp", "x86_64", ARG_CAPS_VARIANT, "+amdsev", ARG_END); + DO_TEST_CAPS_ARCH_LATEST_FULL("firmware-manual-efi-tdx", "x86_64", + ARG_CAPS_VARIANT, "+inteltdx", + ARG_END); /* Make sure all combinations of ACPI and UEFI behave as expected */ DO_TEST_CAPS_ARCH_LATEST("firmware-manual-efi-acpi-aarch64", "aarch64"); -- 2.52.0

It's possible to have firmware descriptors for builds intended to be loaded as ROM, as is the case for those loaded as pflash. There is no reason to skip firmware autoselection in those cases, and doing so prevents useful information from being filled in. After this change, the firmware-manual-efi-tdx test case is augmented with some additional information. Even more information will be filled in later, when we improve the matching logic. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_firmware.c | 10 ++++------ .../firmware-manual-efi-tdx.x86_64-latest+inteltdx.xml | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 9ba5d899fa..7953b297bc 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1913,13 +1913,11 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, return -1; } - /* If firmware autoselection is disabled and the loader is a ROM - * instead of a PFLASH device, then we're using BIOS and we don't - * need any information at all */ - if (!autoSelection && - (!loader || (loader && loader->type == VIR_DOMAIN_LOADER_TYPE_ROM))) { + /* If firmware autoselection is disabled and no information + * related to the loader was provided, then we're using the + * default built-in firmware and we can stop here */ + if (!autoSelection && !loader) return 0; - } /* Look for the information we need in firmware descriptors */ if ((ret = qemuFirmwareFillDomainModern(driver, def)) < 0) diff --git a/tests/qemuxmlconfdata/firmware-manual-efi-tdx.x86_64-latest+inteltdx.xml b/tests/qemuxmlconfdata/firmware-manual-efi-tdx.x86_64-latest+inteltdx.xml index 7428a3dfef..cdb92dcf1d 100644 --- a/tests/qemuxmlconfdata/firmware-manual-efi-tdx.x86_64-latest+inteltdx.xml +++ b/tests/qemuxmlconfdata/firmware-manual-efi-tdx.x86_64-latest+inteltdx.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='x86_64' machine='pc-q35-10.0'>hvm</type> - <loader readonly='yes' type='rom'>/usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd</loader> + <loader readonly='yes' type='rom' format='raw'>/usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd</loader> <boot dev='hd'/> </os> <features> -- 2.52.0

Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- NEWS.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index b749ffff79..6ca49a4c1b 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -19,6 +19,14 @@ v12.0.0 (unreleased) * **Improvements** + * qemu: Improvements and fixes to firmware selection + + Firmware selection now works more reliably and predictably in many + scenarios. + + Notably, issues that were preventing the use of firmware designed for + confidential VMs on aarch64 have been addressed. + * **Bug fixes** -- 2.52.0