[libvirt PATCH 0/2] conf: Fix migration in some firmware autoselection scenarios
See commit 2/2 for details. Andrea Bolognani (2): tests: Tweak input file conf: Fix migration in some firmware autoselection scenarios src/conf/domain_conf.c | 39 ++++++++++++++++++- ...are-manual-efi-features.x86_64-latest.args | 35 +++++++++++++++++ .../firmware-manual-efi-features.xml | 2 +- tests/qemuxml2argvtest.c | 6 ++- ...ware-manual-efi-features.x86_64-latest.xml | 36 +++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 115 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/firmware-manual-efi-features.x86_64-latest.args create mode 100644 tests/qemuxml2xmloutdata/firmware-manual-efi-features.x86_64-latest.xml -- 2.39.2
The canonical order for <os> child elements is <firmware> then <loader>. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxml2argvdata/firmware-manual-efi-features.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemuxml2argvdata/firmware-manual-efi-features.xml b/tests/qemuxml2argvdata/firmware-manual-efi-features.xml index 8e5aff7559..092739af56 100644 --- a/tests/qemuxml2argvdata/firmware-manual-efi-features.xml +++ b/tests/qemuxml2argvdata/firmware-manual-efi-features.xml @@ -5,10 +5,10 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='x86_64' machine='pc-i440fx-4.0'>hvm</type> - <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> <firmware> <feature enabled='no' name='enrolled-keys'/> </firmware> + <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> </os> <features> <acpi/> -- 2.39.2
On a Tuesday in 2023, Andrea Bolognani wrote:
The canonical order for <os> child elements is <firmware> then <loader>.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxml2argvdata/firmware-manual-efi-features.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Introduce a small kludge in the parser to avoid unnecessarily blocking incoming migration from a range of recent libvirt releases. https://bugzilla.redhat.com/show_bug.cgi?id=2184966 Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 39 ++++++++++++++++++- ...are-manual-efi-features.x86_64-latest.args | 35 +++++++++++++++++ tests/qemuxml2argvtest.c | 6 ++- ...ware-manual-efi-features.x86_64-latest.xml | 36 +++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 5 files changed, 114 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/firmware-manual-efi-features.x86_64-latest.args create mode 100644 tests/qemuxml2xmloutdata/firmware-manual-efi-features.x86_64-latest.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 70e4d52ee6..bc20acf103 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17210,11 +17210,13 @@ virDomainDefParseBootKernelOptions(virDomainDef *def, static int virDomainDefParseBootFirmwareOptions(virDomainDef *def, - xmlXPathContextPtr ctxt) + xmlXPathContextPtr ctxt, + unsigned int flags) { g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt); g_autofree xmlNodePtr *nodes = NULL; g_autofree int *features = NULL; + bool abiUpdate = !!(flags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE); int fw = 0; int n = 0; size_t i; @@ -17222,6 +17224,39 @@ virDomainDefParseBootFirmwareOptions(virDomainDef *def, if ((n = virXPathNodeSet("./os/firmware/feature", ctxt, &nodes)) < 0) return -1; + /* Migration compatibility kludge. + * + * Between 8.6.0 and 9.1.0 (extremes included), the migratable + * XML produced when feature-based firmware autoselection was + * enabled looked like + * + * <os> + * <firmware> + * <feature name='foo' enabled='yes'/> + * + * Notice how there's no firmware='foo' attribute for the <os> + * element, meaning that firmware autoselection is disabled, and + * yet some <feature> elements, which are used to control the + * firmware autoselection process, are present. We don't consider + * this to be a valid combination, and want such a configuration + * to get rejected when submitted by users. + * + * In order to achieve that, while at the same time keeping + * migration coming from the libvirt versions listed above + * working, we can simply stop parsing early and ignore the + * <feature> tags when firmware autoselection is not enabled, + * *except* if we're defining a new domain. + * + * This is safe to do because the configuration will either come + * from another libvirt instance, in which case it will have a + * properly filled in <loader> element that contains enough + * information to successfully define and start the domain, or it + * will be a random configuration that lacks such information, in + * which case a different failure will be reported anyway. + */ + if (n > 0 && !firmware && !abiUpdate) + return 0; + if (n > 0) features = g_new0(int, VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_LAST); @@ -17350,7 +17385,7 @@ virDomainDefParseBootOptions(virDomainDef *def, case VIR_DOMAIN_OSTYPE_HVM: virDomainDefParseBootKernelOptions(def, ctxt); - if (virDomainDefParseBootFirmwareOptions(def, ctxt) < 0) + if (virDomainDefParseBootFirmwareOptions(def, ctxt, flags) < 0) return -1; if (virDomainDefParseBootLoaderOptions(def, ctxt, xmlopt, flags) < 0) diff --git a/tests/qemuxml2argvdata/firmware-manual-efi-features.x86_64-latest.args b/tests/qemuxml2argvdata/firmware-manual-efi-features.x86_64-latest.args new file mode 100644 index 0000000000..57c71542cd --- /dev/null +++ b/tests/qemuxml2argvdata/firmware-manual-efi-features.x86_64-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-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","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"raw","file":"libvirt-pflash1-storage"}' \ +-machine pc-i440fx-4.0,usb=off,dump-guest-core=off,memory-backend=pc.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format,acpi=on \ +-accel tcg \ +-cpu qemu64 \ +-m 1024 \ +-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/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index e055d372fa..1808d9fc02 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1051,7 +1051,11 @@ mymain(void) 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-efi"); - DO_TEST_CAPS_LATEST_PARSE_ERROR("firmware-manual-efi-features"); + DO_TEST_CAPS_LATEST("firmware-manual-efi-features"); + DO_TEST_CAPS_ARCH_LATEST_FULL("firmware-manual-efi-features", "x86_64", + ARG_FLAGS, FLAG_EXPECT_PARSE_ERROR, + ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, + ARG_END); DO_TEST_CAPS_LATEST("firmware-manual-efi-rw"); DO_TEST_CAPS_LATEST("firmware-manual-efi-rw-implicit"); DO_TEST_CAPS_LATEST("firmware-manual-efi-loader-secure"); diff --git a/tests/qemuxml2xmloutdata/firmware-manual-efi-features.x86_64-latest.xml b/tests/qemuxml2xmloutdata/firmware-manual-efi-features.x86_64-latest.xml new file mode 100644 index 0000000000..dc4b8bb97f --- /dev/null +++ b/tests/qemuxml2xmloutdata/firmware-manual-efi-features.x86_64-latest.xml @@ -0,0 +1,36 @@ +<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='x86_64' machine='pc-i440fx-4.0'>hvm</type> + <firmware> + <feature enabled='no' name='enrolled-keys'/> + <feature enabled='no' name='secure-boot'/> + </firmware> + <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> + <nvram template='/usr/share/OVMF/OVMF_VARS.fd'>/var/lib/libvirt/qemu/nvram/guest_VARS.fd</nvram> + <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='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/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index e19aba0878..b1bc6e9216 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -866,6 +866,7 @@ mymain(void) DO_TEST_CAPS_LATEST("firmware-manual-bios"); DO_TEST_CAPS_LATEST("firmware-manual-bios-stateless"); DO_TEST_CAPS_LATEST("firmware-manual-efi"); + DO_TEST_CAPS_LATEST("firmware-manual-efi-features"); DO_TEST_CAPS_LATEST("firmware-manual-efi-rw"); DO_TEST_CAPS_LATEST("firmware-manual-efi-rw-implicit"); DO_TEST_CAPS_LATEST("firmware-manual-efi-loader-secure"); -- 2.39.2
On a Tuesday in 2023, Andrea Bolognani wrote:
Introduce a small kludge in the parser to avoid unnecessarily blocking incoming migration from a range of recent libvirt releases.
https://bugzilla.redhat.com/show_bug.cgi?id=2184966
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 39 ++++++++++++++++++- ...are-manual-efi-features.x86_64-latest.args | 35 +++++++++++++++++ tests/qemuxml2argvtest.c | 6 ++- ...ware-manual-efi-features.x86_64-latest.xml | 36 +++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 5 files changed, 114 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/firmware-manual-efi-features.x86_64-latest.args create mode 100644 tests/qemuxml2xmloutdata/firmware-manual-efi-features.x86_64-latest.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 70e4d52ee6..bc20acf103 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17210,11 +17210,13 @@ virDomainDefParseBootKernelOptions(virDomainDef *def,
static int virDomainDefParseBootFirmwareOptions(virDomainDef *def, - xmlXPathContextPtr ctxt) + xmlXPathContextPtr ctxt, + unsigned int flags) { g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt); g_autofree xmlNodePtr *nodes = NULL; g_autofree int *features = NULL; + bool abiUpdate = !!(flags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);
The flag is documented as: /* allow updates in post parse callback that would break ABI otherwise */ VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 7, and I also think that this is something that better belongs in post-parse. Jano
int fw = 0; int n = 0; size_t i;
On Wed, Apr 12, 2023 at 12:55:51PM +0200, Ján Tomko wrote:
On a Tuesday in 2023, Andrea Bolognani wrote:
static int virDomainDefParseBootFirmwareOptions(virDomainDef *def, - xmlXPathContextPtr ctxt) + xmlXPathContextPtr ctxt, + unsigned int flags) { g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt); g_autofree xmlNodePtr *nodes = NULL; g_autofree int *features = NULL; + bool abiUpdate = !!(flags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);
The flag is documented as: /* allow updates in post parse callback that would break ABI otherwise */ VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 7,
and I also think that this is something that better belongs in post-parse.
Okay, so the idea would be to keep picking up the firmware features here and possibly drop them from the DomainDef during the PostParse phase? I think that could work too. Let me give it a try. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Apr 12, 2023 at 04:52:16AM -0700, Andrea Bolognani wrote:
On Wed, Apr 12, 2023 at 12:55:51PM +0200, Ján Tomko wrote:
On a Tuesday in 2023, Andrea Bolognani wrote:
static int virDomainDefParseBootFirmwareOptions(virDomainDef *def, - xmlXPathContextPtr ctxt) + xmlXPathContextPtr ctxt, + unsigned int flags) { g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt); g_autofree xmlNodePtr *nodes = NULL; g_autofree int *features = NULL; + bool abiUpdate = !!(flags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);
The flag is documented as: /* allow updates in post parse callback that would break ABI otherwise */ VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 7,
and I also think that this is something that better belongs in post-parse.
Okay, so the idea would be to keep picking up the firmware features here and possibly drop them from the DomainDef during the PostParse phase? I think that could work too. Let me give it a try.
This doesn't seem to work as smoothly as I hoped it would :( The problem is that, if the kludge is performed in PostParse, the call tree ends up looking like virDomainDefPostParse() qemuDomainDefPostParse() qemuFirmwareFillDomain() <- firmware selection virDomainDefOSValidate() virDomainDefPostParseCommon() virDomainDefPostParseOs() <- migration kludge and firmware selection fails, because the configuration it sees is the original one, which doesn't pass validation. I could take virDomainDefPostParseOs() out of virDomainDefPostParseCommon() and ensure it gets called before the driver-specific PostParse callback, but to be honest I'm not convinced making the kludge even more invasive would represent an improvement. Other ideas? -- Andrea Bolognani / Red Hat / Virtualization
On a Wednesday in 2023, Andrea Bolognani wrote:
On Wed, Apr 12, 2023 at 04:52:16AM -0700, Andrea Bolognani wrote:
On Wed, Apr 12, 2023 at 12:55:51PM +0200, Ján Tomko wrote:
On a Tuesday in 2023, Andrea Bolognani wrote:
static int virDomainDefParseBootFirmwareOptions(virDomainDef *def, - xmlXPathContextPtr ctxt) + xmlXPathContextPtr ctxt, + unsigned int flags) { g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt); g_autofree xmlNodePtr *nodes = NULL; g_autofree int *features = NULL; + bool abiUpdate = !!(flags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE);
The flag is documented as: /* allow updates in post parse callback that would break ABI otherwise */ VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 7,
and I also think that this is something that better belongs in post-parse.
Okay, so the idea would be to keep picking up the firmware features here and possibly drop them from the DomainDef during the PostParse phase? I think that could work too. Let me give it a try.
This doesn't seem to work as smoothly as I hoped it would :(
The problem is that, if the kludge is performed in PostParse, the call tree ends up looking like
virDomainDefPostParse() qemuDomainDefPostParse() qemuFirmwareFillDomain() <- firmware selection virDomainDefOSValidate() virDomainDefPostParseCommon() virDomainDefPostParseOs() <- migration kludge
and firmware selection fails, because the configuration it sees is the original one, which doesn't pass validation.
I could take virDomainDefPostParseOs() out of virDomainDefPostParseCommon() and ensure it gets called before the driver-specific PostParse callback, but to be honest I'm not convinced making the kludge even more invasive would represent an improvement.
You could call it virDomainDefPrePostParse.
Other ideas?
Make the complete separation of XML parsing and post-parsing a problem of future ourselves. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Andrea Bolognani -
Ján Tomko