[libvirt PATCH 0/3] qemu: Fix firmware selection backwards compatibility

Andrea Bolognani (3): tests: Introduce DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE() tests: Add firmware-auto-efi-abi-update-aarch64 test case qemu: Default to raw firmware for existing domains src/qemu/qemu_domain.c | 17 +++++++++++++++++ ...irmware-auto-efi-aarch64.aarch64-latest.args | 8 ++++---- ...-efi-abi-update-aarch64.aarch64-latest.args} | 0 .../firmware-auto-efi-abi-update-aarch64.xml | 17 +++++++++++++++++ .../pvpanic-pci-aarch64.aarch64-latest.args | 8 ++++---- ...c-pci-no-address-aarch64.aarch64-latest.args | 8 ++++---- .../virtio-iommu-aarch64.aarch64-latest.args | 8 ++++---- tests/qemuxml2argvtest.c | 6 ++++++ ...firmware-auto-efi-aarch64.aarch64-latest.xml | 4 ++-- ...o-efi-abi-update-aarch64.aarch64-latest.xml} | 0 .../pvpanic-pci-aarch64.aarch64-latest.xml | 4 ++-- ...ic-pci-no-address-aarch64.aarch64-latest.xml | 4 ++-- .../virtio-iommu-aarch64.aarch64-latest.xml | 4 ++-- tests/qemuxml2xmltest.c | 6 ++++++ 14 files changed, 70 insertions(+), 24 deletions(-) copy tests/qemuxml2argvdata/{firmware-auto-efi-aarch64.aarch64-latest.args => firmware-auto-efi-abi-update-aarch64.aarch64-latest.args} (100%) create mode 100644 tests/qemuxml2argvdata/firmware-auto-efi-abi-update-aarch64.xml copy tests/qemuxml2xmloutdata/{firmware-auto-efi-aarch64.aarch64-latest.xml => firmware-auto-efi-abi-update-aarch64.aarch64-latest.xml} (100%) -- 2.39.2

Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxml2argvtest.c | 5 +++++ tests/qemuxml2xmltest.c | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index bd3f46fbe0..cc85790017 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -890,6 +890,11 @@ mymain(void) # define DO_TEST_CAPS_ARCH_LATEST(name, arch) \ DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ARG_END) +# define DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE(name, arch) \ + DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, \ + ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, \ + ARG_END) + # define DO_TEST_CAPS_ARCH_VER(name, arch, ver) \ DO_TEST_CAPS_ARCH_VER_FULL(name, arch, ver, ARG_END) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 6cdead532d..fc8fdaad07 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -193,6 +193,11 @@ mymain(void) #define DO_TEST_CAPS_ARCH_LATEST(name, arch) \ DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ARG_END) +#define DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE(name, arch) \ + DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, \ + ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, \ + ARG_END) + #define DO_TEST_CAPS_ARCH_VER(name, arch, ver) \ DO_TEST_CAPS_ARCH_VER_FULL(name, arch, ver, ARG_END) -- 2.39.2

On Mon, Mar 27, 2023 at 10:12:33PM +0200, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
--- tests/qemuxml2argvtest.c | 5 +++++ tests/qemuxml2xmltest.c | 5 +++++ 2 files changed, 10 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index bd3f46fbe0..cc85790017 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -890,6 +890,11 @@ mymain(void) # define DO_TEST_CAPS_ARCH_LATEST(name, arch) \ DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ARG_END)
+# define DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE(name, arch) \ + DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, \ + ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, \ + ARG_END) + # define DO_TEST_CAPS_ARCH_VER(name, arch, ver) \ DO_TEST_CAPS_ARCH_VER_FULL(name, arch, ver, ARG_END)
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 6cdead532d..fc8fdaad07 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -193,6 +193,11 @@ mymain(void) #define DO_TEST_CAPS_ARCH_LATEST(name, arch) \ DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ARG_END)
+#define DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE(name, arch) \ + DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, \ + ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, \ + ARG_END) + #define DO_TEST_CAPS_ARCH_VER(name, arch, ver) \ DO_TEST_CAPS_ARCH_VER_FULL(name, arch, ver, ARG_END)
-- 2.39.2

The input is identical to that of the existing firmware-auto-efi-aarch64 test, but in this case we want to cover the scenario in which that input is used to define a new domain rather than loading the definition of an existing domain from disk. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ...efi-abi-update-aarch64.aarch64-latest.args | 35 +++++++++++++++++++ .../firmware-auto-efi-abi-update-aarch64.xml | 17 +++++++++ tests/qemuxml2argvtest.c | 1 + ...-efi-abi-update-aarch64.aarch64-latest.xml | 35 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 5 files changed, 89 insertions(+) create mode 100644 tests/qemuxml2argvdata/firmware-auto-efi-abi-update-aarch64.aarch64-latest.args create mode 100644 tests/qemuxml2argvdata/firmware-auto-efi-abi-update-aarch64.xml create mode 100644 tests/qemuxml2xmloutdata/firmware-auto-efi-abi-update-aarch64.aarch64-latest.xml diff --git a/tests/qemuxml2argvdata/firmware-auto-efi-abi-update-aarch64.aarch64-latest.args b/tests/qemuxml2argvdata/firmware-auto-efi-abi-update-aarch64.aarch64-latest.args new file mode 100644 index 0000000000..81dbf9cf1b --- /dev/null +++ b/tests/qemuxml2argvdata/firmware-auto-efi-abi-update-aarch64.aarch64-latest.args @@ -0,0 +1,35 @@ +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-aarch64 \ +-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/AAVMF/AAVMF_CODE.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 virt-4.0,usb=off,gic-version=2,dump-guest-core=off,memory-backend=mach-virt.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format \ +-accel tcg \ +-cpu cortex-a15 \ +-m 1024 \ +-object '{"qom-type":"memory-backend-ram","id":"mach-virt.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/qemuxml2argvdata/firmware-auto-efi-abi-update-aarch64.xml b/tests/qemuxml2argvdata/firmware-auto-efi-abi-update-aarch64.xml new file mode 100644 index 0000000000..5c6c5192ba --- /dev/null +++ b/tests/qemuxml2argvdata/firmware-auto-efi-abi-update-aarch64.xml @@ -0,0 +1,17 @@ +<domain type='qemu'> + <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='aarch64' machine='virt-4.0'>hvm</type> + </os> + <features> + <acpi/> + </features> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index cc85790017..72903911bf 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1133,6 +1133,7 @@ mymain(void) DO_TEST_CAPS_LATEST_PARSE_ERROR("firmware-auto-efi-enrolled-keys-no-secboot"); DO_TEST_CAPS_LATEST("firmware-auto-efi-smm-off"); DO_TEST_CAPS_ARCH_LATEST("firmware-auto-efi-aarch64", "aarch64"); + DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("firmware-auto-efi-abi-update-aarch64", "aarch64"); DO_TEST_CAPS_LATEST("firmware-auto-efi-nvram-file"); DO_TEST_CAPS_LATEST("firmware-auto-efi-nvram-network-nbd"); DO_TEST_CAPS_LATEST("firmware-auto-efi-nvram-network-iscsi"); diff --git a/tests/qemuxml2xmloutdata/firmware-auto-efi-abi-update-aarch64.aarch64-latest.xml b/tests/qemuxml2xmloutdata/firmware-auto-efi-abi-update-aarch64.aarch64-latest.xml new file mode 100644 index 0000000000..5779eca7a0 --- /dev/null +++ b/tests/qemuxml2xmloutdata/firmware-auto-efi-abi-update-aarch64.aarch64-latest.xml @@ -0,0 +1,35 @@ +<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 firmware='efi'> + <type arch='aarch64' machine='virt-4.0'>hvm</type> + <firmware> + <feature enabled='no' name='enrolled-keys'/> + <feature enabled='no' name='secure-boot'/> + </firmware> + <loader readonly='yes' type='pflash' format='qcow2'>/usr/share/AAVMF/AAVMF_CODE.qcow2</loader> + <nvram template='/usr/share/AAVMF/AAVMF_VARS.qcow2' format='qcow2'>/var/lib/libvirt/qemu/nvram/guest_VARS.qcow2</nvram> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <gic version='2'/> + </features> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>cortex-a15</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-aarch64</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='pci' index='0' model='pcie-root'/> + <audio id='1' type='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index fc8fdaad07..005b231c1a 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -961,6 +961,7 @@ mymain(void) DO_TEST_CAPS_LATEST("firmware-auto-efi-no-enrolled-keys"); DO_TEST_CAPS_LATEST("firmware-auto-efi-smm-off"); DO_TEST_CAPS_ARCH_LATEST("firmware-auto-efi-aarch64", "aarch64"); + DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("firmware-auto-efi-abi-update-aarch64", "aarch64"); DO_TEST_CAPS_LATEST("firmware-auto-efi-nvram-file"); DO_TEST_CAPS_LATEST("firmware-auto-efi-nvram-network-nbd"); DO_TEST_CAPS_LATEST("firmware-auto-efi-nvram-network-iscsi"); -- 2.39.2

On Mon, Mar 27, 2023 at 10:12:34PM +0200, Andrea Bolognani wrote:
The input is identical to that of the existing firmware-auto-efi-aarch64 test, but in this case we want to cover the scenario in which that input is used to define a new domain rather than loading the definition of an existing domain from disk.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

The changes to the output files are the exact opposite of those from commit 22207713cf8e: this is proof that the fix is working as intended, and that existing domains will keep using raw firmware images regardless of whether or not qcow2 images are available on the system and have higher priority. New domains will keep picking whatever firmware is considered the preferred one according to the order of descriptors, as evidenced by the fact that the recently introduced firmware-auto-efi-abi-update-aarch64 test case is unaffected. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 17 +++++++++++++++++ ...irmware-auto-efi-aarch64.aarch64-latest.args | 8 ++++---- .../pvpanic-pci-aarch64.aarch64-latest.args | 8 ++++---- ...c-pci-no-address-aarch64.aarch64-latest.args | 8 ++++---- .../virtio-iommu-aarch64.aarch64-latest.args | 8 ++++---- ...firmware-auto-efi-aarch64.aarch64-latest.xml | 4 ++-- .../pvpanic-pci-aarch64.aarch64-latest.xml | 4 ++-- ...ic-pci-no-address-aarch64.aarch64-latest.xml | 4 ++-- .../virtio-iommu-aarch64.aarch64-latest.xml | 4 ++-- 9 files changed, 41 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6c29c8f09f..d3e74d5c71 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4474,6 +4474,23 @@ qemuDomainDefBootPostParse(virDomainDef *def, { bool abiUpdate = !!(parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE); + /* 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 + * could never have resulted, before libvirt 9.2.0, in anything + * but a raw firmware image being selected. + * + * In order to ensure that existing domains keep working even if + * a firmware descriptor for a build with a different format is + * given higher priority, explicitly add this requirement to the + * definition before performing firmware selection */ + if (!abiUpdate && def->os.firmware) { + if (!def->os.loader) + def->os.loader = virDomainLoaderDefNew(); + if (!def->os.loader->format) + def->os.loader->format = VIR_STORAGE_FILE_RAW; + } + /* Firmware selection can fail for a number of reasons, but the * most likely one is that the requested configuration contains * mistakes or includes constraints that are impossible to diff --git a/tests/qemuxml2argvdata/firmware-auto-efi-aarch64.aarch64-latest.args b/tests/qemuxml2argvdata/firmware-auto-efi-aarch64.aarch64-latest.args index 81dbf9cf1b..a8b9cae35f 100644 --- a/tests/qemuxml2argvdata/firmware-auto-efi-aarch64.aarch64-latest.args +++ b/tests/qemuxml2argvdata/firmware-auto-efi-aarch64.aarch64-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/AAVMF/AAVMF_CODE.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"}' \ +-blockdev '{"driver":"file","filename":"/usr/share/AAVMF/AAVMF_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","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"raw","file":"libvirt-pflash1-storage"}' \ -machine virt-4.0,usb=off,gic-version=2,dump-guest-core=off,memory-backend=mach-virt.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format \ -accel tcg \ -cpu cortex-a15 \ diff --git a/tests/qemuxml2argvdata/pvpanic-pci-aarch64.aarch64-latest.args b/tests/qemuxml2argvdata/pvpanic-pci-aarch64.aarch64-latest.args index a5bf0d68be..32cfa998c2 100644 --- a/tests/qemuxml2argvdata/pvpanic-pci-aarch64.aarch64-latest.args +++ b/tests/qemuxml2argvdata/pvpanic-pci-aarch64.aarch64-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/AAVMF/AAVMF_CODE.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"}' \ +-blockdev '{"driver":"file","filename":"/usr/share/AAVMF/AAVMF_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","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"raw","file":"libvirt-pflash1-storage"}' \ -machine virt-6.0,usb=off,gic-version=2,dump-guest-core=off,memory-backend=mach-virt.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format \ -accel tcg \ -cpu cortex-a15 \ diff --git a/tests/qemuxml2argvdata/pvpanic-pci-no-address-aarch64.aarch64-latest.args b/tests/qemuxml2argvdata/pvpanic-pci-no-address-aarch64.aarch64-latest.args index ed38ba1b81..4e78e03ec4 100644 --- a/tests/qemuxml2argvdata/pvpanic-pci-no-address-aarch64.aarch64-latest.args +++ b/tests/qemuxml2argvdata/pvpanic-pci-no-address-aarch64.aarch64-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/AAVMF/AAVMF_CODE.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"}' \ +-blockdev '{"driver":"file","filename":"/usr/share/AAVMF/AAVMF_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","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"raw","file":"libvirt-pflash1-storage"}' \ -machine virt-6.0,usb=off,gic-version=2,dump-guest-core=off,memory-backend=mach-virt.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format \ -accel tcg \ -cpu cortex-a15 \ diff --git a/tests/qemuxml2argvdata/virtio-iommu-aarch64.aarch64-latest.args b/tests/qemuxml2argvdata/virtio-iommu-aarch64.aarch64-latest.args index 7f1612454a..a58798acf1 100644 --- a/tests/qemuxml2argvdata/virtio-iommu-aarch64.aarch64-latest.args +++ b/tests/qemuxml2argvdata/virtio-iommu-aarch64.aarch64-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/AAVMF/AAVMF_CODE.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"}' \ +-blockdev '{"driver":"file","filename":"/usr/share/AAVMF/AAVMF_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","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"raw","file":"libvirt-pflash1-storage"}' \ -machine virt-6.0,usb=off,gic-version=2,dump-guest-core=off,memory-backend=mach-virt.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format \ -accel tcg \ -cpu cortex-a15 \ diff --git a/tests/qemuxml2xmloutdata/firmware-auto-efi-aarch64.aarch64-latest.xml b/tests/qemuxml2xmloutdata/firmware-auto-efi-aarch64.aarch64-latest.xml index 5779eca7a0..8eb6086e40 100644 --- a/tests/qemuxml2xmloutdata/firmware-auto-efi-aarch64.aarch64-latest.xml +++ b/tests/qemuxml2xmloutdata/firmware-auto-efi-aarch64.aarch64-latest.xml @@ -10,8 +10,8 @@ <feature enabled='no' name='enrolled-keys'/> <feature enabled='no' name='secure-boot'/> </firmware> - <loader readonly='yes' type='pflash' format='qcow2'>/usr/share/AAVMF/AAVMF_CODE.qcow2</loader> - <nvram template='/usr/share/AAVMF/AAVMF_VARS.qcow2' format='qcow2'>/var/lib/libvirt/qemu/nvram/guest_VARS.qcow2</nvram> + <loader readonly='yes' type='pflash'>/usr/share/AAVMF/AAVMF_CODE.fd</loader> + <nvram template='/usr/share/AAVMF/AAVMF_VARS.fd'>/var/lib/libvirt/qemu/nvram/guest_VARS.fd</nvram> <boot dev='hd'/> </os> <features> diff --git a/tests/qemuxml2xmloutdata/pvpanic-pci-aarch64.aarch64-latest.xml b/tests/qemuxml2xmloutdata/pvpanic-pci-aarch64.aarch64-latest.xml index 2a83ace748..92dcd92d19 100644 --- a/tests/qemuxml2xmloutdata/pvpanic-pci-aarch64.aarch64-latest.xml +++ b/tests/qemuxml2xmloutdata/pvpanic-pci-aarch64.aarch64-latest.xml @@ -10,8 +10,8 @@ <feature enabled='no' name='enrolled-keys'/> <feature enabled='no' name='secure-boot'/> </firmware> - <loader readonly='yes' type='pflash' format='qcow2'>/usr/share/AAVMF/AAVMF_CODE.qcow2</loader> - <nvram template='/usr/share/AAVMF/AAVMF_VARS.qcow2' format='qcow2'>/var/lib/libvirt/qemu/nvram/guest_VARS.qcow2</nvram> + <loader readonly='yes' type='pflash'>/usr/share/AAVMF/AAVMF_CODE.fd</loader> + <nvram template='/usr/share/AAVMF/AAVMF_VARS.fd'>/var/lib/libvirt/qemu/nvram/guest_VARS.fd</nvram> <boot dev='hd'/> </os> <features> diff --git a/tests/qemuxml2xmloutdata/pvpanic-pci-no-address-aarch64.aarch64-latest.xml b/tests/qemuxml2xmloutdata/pvpanic-pci-no-address-aarch64.aarch64-latest.xml index d5ed9b23fe..f3e7e2c911 100644 --- a/tests/qemuxml2xmloutdata/pvpanic-pci-no-address-aarch64.aarch64-latest.xml +++ b/tests/qemuxml2xmloutdata/pvpanic-pci-no-address-aarch64.aarch64-latest.xml @@ -10,8 +10,8 @@ <feature enabled='no' name='enrolled-keys'/> <feature enabled='no' name='secure-boot'/> </firmware> - <loader readonly='yes' type='pflash' format='qcow2'>/usr/share/AAVMF/AAVMF_CODE.qcow2</loader> - <nvram template='/usr/share/AAVMF/AAVMF_VARS.qcow2' format='qcow2'>/var/lib/libvirt/qemu/nvram/guest_VARS.qcow2</nvram> + <loader readonly='yes' type='pflash'>/usr/share/AAVMF/AAVMF_CODE.fd</loader> + <nvram template='/usr/share/AAVMF/AAVMF_VARS.fd'>/var/lib/libvirt/qemu/nvram/guest_VARS.fd</nvram> <boot dev='hd'/> </os> <features> diff --git a/tests/qemuxml2xmloutdata/virtio-iommu-aarch64.aarch64-latest.xml b/tests/qemuxml2xmloutdata/virtio-iommu-aarch64.aarch64-latest.xml index 589295e602..ee747e7496 100644 --- a/tests/qemuxml2xmloutdata/virtio-iommu-aarch64.aarch64-latest.xml +++ b/tests/qemuxml2xmloutdata/virtio-iommu-aarch64.aarch64-latest.xml @@ -10,8 +10,8 @@ <feature enabled='no' name='enrolled-keys'/> <feature enabled='no' name='secure-boot'/> </firmware> - <loader readonly='yes' type='pflash' format='qcow2'>/usr/share/AAVMF/AAVMF_CODE.qcow2</loader> - <nvram template='/usr/share/AAVMF/AAVMF_VARS.qcow2' format='qcow2'>/var/lib/libvirt/qemu/nvram/guest_VARS.qcow2</nvram> + <loader readonly='yes' type='pflash'>/usr/share/AAVMF/AAVMF_CODE.fd</loader> + <nvram template='/usr/share/AAVMF/AAVMF_VARS.fd'>/var/lib/libvirt/qemu/nvram/guest_VARS.fd</nvram> <boot dev='hd'/> </os> <features> -- 2.39.2

On Mon, Mar 27, 2023 at 10:12:35PM +0200, Andrea Bolognani wrote:
The changes to the output files are the exact opposite of those from commit 22207713cf8e: this is proof that the fix is working as intended, and that existing domains will keep using raw firmware images regardless of whether or not qcow2 images are available on the system and have higher priority.
New domains will keep picking whatever firmware is considered the preferred one according to the order of descriptors, as evidenced by the fact that the recently introduced firmware-auto-efi-abi-update-aarch64 test case is unaffected.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com> and safe for freeze (I presume we _really_ do not want to release without this patch). Should we add "Fixes: 22207713cf8e82ab792acb3412720702938bfc81" ? I'll leave that up to you.

On Tue, Mar 28, 2023 at 09:13:01AM +0200, Martin Kletzander wrote:
On Mon, Mar 27, 2023 at 10:12:35PM +0200, Andrea Bolognani wrote:
The changes to the output files are the exact opposite of those from commit 22207713cf8e: this is proof that the fix is working as intended, and that existing domains will keep using raw firmware images regardless of whether or not qcow2 images are available on the system and have higher priority.
New domains will keep picking whatever firmware is considered the preferred one according to the order of descriptors, as evidenced by the fact that the recently introduced firmware-auto-efi-abi-update-aarch64 test case is unaffected.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
and safe for freeze (I presume we _really_ do not want to release without this patch).
Yeah, that would be far from ideal :)
Should we add "Fixes: 22207713cf8e82ab792acb3412720702938bfc81" ? I'll leave that up to you.
Even though that's the one where the effects of the issue can be seen reflected in the test suite, there is absolutely nothing wrong with the changes made by that commit, so claiming that this patch is a fix for it feels wrong. The actual culprit would be one of the preparatory commits, perhaps 1a6469e81fd0? But I'm not 100% convinced, so I think I'll just leave the tag out. It's significantly less useful when the issue is both introduced and resolved within the same release anyway. Thanks a lot for the review! -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani
-
Martin Kletzander