[libvirt PATCH 00/10] Cleanup and test more firmware handling scenarios
There are a mind bending number of possible ways to configure the firmware with/without NVRAM. Only a small portion are tested and many error scenarios are silently ignored. This series attempts to get coverage of every possible XML config scenario and report explicit errors in all invalid configs. There is an open question on patch 4. Essentially the use of NVRAM combined with writable executable feels like an accidental feature in libvirt that hasn't really been thought through. I'd like to better define expectations here but there are several possible strategies and I'm undecided which is best. Daniel P. Berrangé (10): qemu: fix bad indentation for qemuDomainNVRAMPathFormat tests: add explicit test case for pflash loader lacking path tests: add test case for NVRAM with template conf: validate NVRAM template usage with R/W loader binary tests: don't permit NVRAM path when using firmware auto-select qemu: inline code for filling in per-VM NVRAM path conf: rename struct field for NVRAM template conf: switch nvram parsing to use XML node / property helpers conf: move nvram parsing into virDomainLoaderDefParseXML conf: stop ignoring <loader>/<nvram> with firmware auto-select src/conf/domain_conf.c | 121 +++++++++++------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_domain.c | 18 +-- src/qemu/qemu_domain.h | 8 +- src/qemu/qemu_firmware.c | 13 +- src/qemu/qemu_process.c | 4 +- tests/qemuxml2argvdata/bios-nvram-no-path.err | 1 + tests/qemuxml2argvdata/bios-nvram-no-path.xml | 19 +++ ...-nvram-rw-template-vars.x86_64-latest.args | 41 ++++++ .../bios-nvram-rw-template-vars.xml | 36 ++++++ .../bios-nvram-rw-template.err | 1 + .../bios-nvram-rw-template.xml | 36 ++++++ .../bios-nvram-rw-vars.x86_64-latest.args | 41 ++++++ tests/qemuxml2argvdata/bios-nvram-rw-vars.xml | 36 ++++++ .../bios-nvram-template.x86_64-latest.args | 37 ++++++ .../qemuxml2argvdata/bios-nvram-template.xml | 21 +++ tests/qemuxml2argvdata/os-firmware-bios.xml | 1 - .../os-firmware-efi-bad-loader-path.err | 1 + .../os-firmware-efi-bad-loader-path.xml | 67 ++++++++++ .../os-firmware-efi-bad-loader-type.err | 1 + .../os-firmware-efi-bad-loader-type.xml | 67 ++++++++++ .../os-firmware-efi-bad-nvram-path.err | 1 + .../os-firmware-efi-bad-nvram-path.xml | 68 ++++++++++ .../os-firmware-efi-bad-nvram-template.err | 1 + .../os-firmware-efi-bad-nvram-template.xml | 68 ++++++++++ .../os-firmware-efi-secboot.xml | 1 - tests/qemuxml2argvdata/os-firmware-efi.xml | 1 - tests/qemuxml2argvtest.c | 9 ++ .../os-firmware-bios.x86_64-latest.xml | 1 - .../os-firmware-efi-secboot.x86_64-latest.xml | 1 - .../os-firmware-efi.x86_64-latest.xml | 1 - 31 files changed, 647 insertions(+), 77 deletions(-) create mode 100644 tests/qemuxml2argvdata/bios-nvram-no-path.err create mode 100644 tests/qemuxml2argvdata/bios-nvram-no-path.xml create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template-vars.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template-vars.xml create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template.err create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template.xml create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-vars.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-vars.xml create mode 100644 tests/qemuxml2argvdata/bios-nvram-template.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-template.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-loader-path.err create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-loader-path.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-loader-type.err create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-loader-type.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.err create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-template.err create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-template.xml -- 2.34.1
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_domain.c | 4 ++-- src/qemu/qemu_domain.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c764f6296c..4e9c845f68 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10956,8 +10956,8 @@ qemuDomainDiskIsMissingLocalOptional(virDomainDiskDef *disk) void qemuDomainNVRAMPathFormat(virQEMUDriverConfig *cfg, - virDomainDef *def, - char **path) + virDomainDef *def, + char **path) { *path = g_strdup_printf("%s/%s_VARS.fd", cfg->nvramDir, def->name); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 8bf1c91049..3ca6ef084e 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -980,8 +980,8 @@ qemuDomainDiskIsMissingLocalOptional(virDomainDiskDef *disk); void qemuDomainNVRAMPathFormat(virQEMUDriverConfig *cfg, - virDomainDef *def, - char **path); + virDomainDef *def, + char **path); void qemuDomainNVRAMPathGenerate(virQEMUDriverConfig *cfg, -- 2.34.1
The following is expected to raise an error: <os> <loader readonly='yes' type='pflash'/> </os> because no path to the pflash loader is given and there is no default built-in. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tests/qemuxml2argvdata/bios-nvram-no-path.err | 1 + tests/qemuxml2argvdata/bios-nvram-no-path.xml | 19 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 21 insertions(+) create mode 100644 tests/qemuxml2argvdata/bios-nvram-no-path.err create mode 100644 tests/qemuxml2argvdata/bios-nvram-no-path.xml diff --git a/tests/qemuxml2argvdata/bios-nvram-no-path.err b/tests/qemuxml2argvdata/bios-nvram-no-path.err new file mode 100644 index 0000000000..795386008c --- /dev/null +++ b/tests/qemuxml2argvdata/bios-nvram-no-path.err @@ -0,0 +1 @@ +no loader path specified and firmware auto selection disabled diff --git a/tests/qemuxml2argvdata/bios-nvram-no-path.xml b/tests/qemuxml2argvdata/bios-nvram-no-path.xml new file mode 100644 index 0000000000..bf97f0bdd6 --- /dev/null +++ b/tests/qemuxml2argvdata/bios-nvram-no-path.xml @@ -0,0 +1,19 @@ +<domain type='qemu'> + <name>test-bios</name> + <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <loader readonly='yes' type='pflash'/> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 6cf35a0ebf..fd5fe6054d 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1267,6 +1267,7 @@ mymain(void) DO_TEST("bios", QEMU_CAPS_DEVICE_ISA_SERIAL); DO_TEST_NOCAPS("bios-nvram"); + DO_TEST_PARSE_ERROR_NOCAPS("bios-nvram-no-path"); DO_TEST_CAPS_LATEST("bios-nvram-rw"); DO_TEST_CAPS_LATEST("bios-nvram-rw-implicit"); DO_TEST("bios-nvram-secure", -- 2.34.1
This demonstrates that <os> <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> <nvram template="/usr/share/OVMF/OVMF_VARS.fd"/> </os> gets expanded to give a per-VM NVRAM path. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .../bios-nvram-template.x86_64-latest.args | 37 +++++++++++++++++++ .../qemuxml2argvdata/bios-nvram-template.xml | 21 +++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 59 insertions(+) create mode 100644 tests/qemuxml2argvdata/bios-nvram-template.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-template.xml diff --git a/tests/qemuxml2argvdata/bios-nvram-template.x86_64-latest.args b/tests/qemuxml2argvdata/bios-nvram-template.x86_64-latest.args new file mode 100644 index 0000000000..7dc0d604a0 --- /dev/null +++ b/tests/qemuxml2argvdata/bios-nvram-template.x86_64-latest.args @@ -0,0 +1,37 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-test-bios \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-test-bios/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-test-bios/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-test-bios/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=test-bios,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-test-bios/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/test-bios_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,usb=off,dump-guest-core=off,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format,memory-backend=pc.ram \ +-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 362d1fc1-df7d-193e-5c18-49a71bd1da66 \ +-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 menu=on,strict=on \ +-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/bios-nvram-template.xml b/tests/qemuxml2argvdata/bios-nvram-template.xml new file mode 100644 index 0000000000..1bbe4314b5 --- /dev/null +++ b/tests/qemuxml2argvdata/bios-nvram-template.xml @@ -0,0 +1,21 @@ +<domain type='qemu'> + <name>test-bios</name> + <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> + <nvram template="/usr/share/OVMF/OVMF_VARS.fd"/> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index fd5fe6054d..a7e8246d38 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1276,6 +1276,7 @@ mymain(void) QEMU_CAPS_DEVICE_IOH3420, QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_VIRTIO_SCSI); + DO_TEST_CAPS_LATEST("bios-nvram-template"); /* Make sure all combinations of ACPI and UEFI behave as expected */ DO_TEST_NOCAPS("q35-acpi-uefi"); -- 2.34.1
On 2/15/22 19:54, Daniel P. Berrangé wrote:
This demonstrates that
<os> <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> <nvram template="/usr/share/OVMF/OVMF_VARS.fd"/> </os>
gets expanded to give a per-VM NVRAM path.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .../bios-nvram-template.x86_64-latest.args | 37 +++++++++++++++++++ .../qemuxml2argvdata/bios-nvram-template.xml | 21 +++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 59 insertions(+) create mode 100644 tests/qemuxml2argvdata/bios-nvram-template.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-template.xml
I wanted to suggest introducing the same test case for xml2xml test, but the nvram path generated looks weird after I did that: <nvram template='/usr/share/OVMF/OVMF_VARS.fd'>/bad-test-used-env-home/.config/libvirt/qemu/nvram/test-bios_VARS.fd</nvram> We're probably missing some mock somewhere, so I'll save that for future work. Michal
The QEMU driver will populate the template to the nvram file any time it sees both the template and nvram paths present. It will auto-generate a nvram path per-VM if not provided by the user, but only if the loader is marked R/O. So with a R/O loader we have these possible scenarios - No NVRAM path or template -> try to infer a template based on the loader path, if not possible, fatal error. Auto-generate NVRAM per per VM - NVRAM path only -> try to infer a template based on the loader path, if not possible, app must have pre-created NVRAM - NVRAM path + template -> QEMU driver will copy template to NVRAM - NVRAM template only -> auto-generate NVRAM path per VM and then copy template While with a R/W loader we have these possible scenarios - No NVRAM path or template -> do nothing - NVRAM path only -> app must have pre-created NVRAM - NVRAM path + template -> QEMU driver will copy template to NVRAM - NVRAM template only -> silently ignored This change improves the last scenario by reporting an error from the parser. Two alternative strategies though would be: - Auto-generate a NVRAM path per VM - Don't support templates at all with R/W loader Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_conf.c | 16 ++++++-- ...-nvram-rw-template-vars.x86_64-latest.args | 41 +++++++++++++++++++ .../bios-nvram-rw-template-vars.xml | 36 ++++++++++++++++ .../bios-nvram-rw-template.err | 1 + .../bios-nvram-rw-template.xml | 36 ++++++++++++++++ .../bios-nvram-rw-vars.x86_64-latest.args | 41 +++++++++++++++++++ tests/qemuxml2argvdata/bios-nvram-rw-vars.xml | 36 ++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ 8 files changed, 207 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template-vars.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template-vars.xml create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template.err create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template.xml create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-vars.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-vars.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ab8f2a52cc..31b49c4ec9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4808,17 +4808,26 @@ virDomainDefPostParseMemory(virDomainDef *def, } -static void +static int virDomainDefPostParseOs(virDomainDef *def) { if (!def->os.loader) - return; + return 0; if (def->os.loader->path && def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_NONE) { /* By default, loader is type of 'rom' */ def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM; } + + if (def->os.loader->readonly != VIR_TRISTATE_BOOL_YES && + def->os.loader->templt && !def->os.loader->nvram) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("NVRAM template without VARs path not permitted with writable loader")); + return -1; + } + + return 0; } @@ -6139,7 +6148,8 @@ virDomainDefPostParseCommon(virDomainDef *def, if (virDomainDefPostParseMemory(def, data->parseFlags) < 0) return -1; - virDomainDefPostParseOs(def); + if (virDomainDefPostParseOs(def) < 0) + return -1; virDomainDefPostParseMemtune(def); diff --git a/tests/qemuxml2argvdata/bios-nvram-rw-template-vars.x86_64-latest.args b/tests/qemuxml2argvdata/bios-nvram-rw-template-vars.x86_64-latest.args new file mode 100644 index 0000000000..8d971ec29b --- /dev/null +++ b/tests/qemuxml2argvdata/bios-nvram-rw-template-vars.x86_64-latest.args @@ -0,0 +1,41 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-test-bios \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-test-bios/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-test-bios/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-test-bios/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=test-bios,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-test-bios/master-key.aes"}' \ +-blockdev '{"driver":"file","filename":"/var/lib/libvirt/qemu/nvram/test-bios.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":"/some/vars/path.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,usb=off,dump-guest-core=off,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format,memory-backend=pc.ram \ +-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 362d1fc1-df7d-193e-5c18-49a71bd1da66 \ +-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 menu=on,strict=on \ +-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ +-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \ +-device '{"driver":"ide-hd","bus":"ide.0","unit":0,"drive":"libvirt-1-format","id":"ide0-0-0","bootindex":1}' \ +-device '{"driver":"usb-tablet","id":"input0","bus":"usb.0","port":"1"}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/bios-nvram-rw-template-vars.xml b/tests/qemuxml2argvdata/bios-nvram-rw-template-vars.xml new file mode 100644 index 0000000000..fe6a064c84 --- /dev/null +++ b/tests/qemuxml2argvdata/bios-nvram-rw-template-vars.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>test-bios</name> + <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <loader readonly='no' type='pflash'>/var/lib/libvirt/qemu/nvram/test-bios.fd</loader> + <nvram template="/some/vars/template/path.fd">/some/vars/path.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='tablet' bus='usb'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/bios-nvram-rw-template.err b/tests/qemuxml2argvdata/bios-nvram-rw-template.err new file mode 100644 index 0000000000..fdfa6c711e --- /dev/null +++ b/tests/qemuxml2argvdata/bios-nvram-rw-template.err @@ -0,0 +1 @@ +XML error: NVRAM template without VARs path not permitted with writable loader diff --git a/tests/qemuxml2argvdata/bios-nvram-rw-template.xml b/tests/qemuxml2argvdata/bios-nvram-rw-template.xml new file mode 100644 index 0000000000..334ed1425b --- /dev/null +++ b/tests/qemuxml2argvdata/bios-nvram-rw-template.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>test-bios</name> + <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <loader readonly='no' type='pflash'>/var/lib/libvirt/qemu/nvram/test-bios.fd</loader> + <nvram template="/some/vars/template/path.fd"/> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='tablet' bus='usb'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/bios-nvram-rw-vars.x86_64-latest.args b/tests/qemuxml2argvdata/bios-nvram-rw-vars.x86_64-latest.args new file mode 100644 index 0000000000..8d971ec29b --- /dev/null +++ b/tests/qemuxml2argvdata/bios-nvram-rw-vars.x86_64-latest.args @@ -0,0 +1,41 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-test-bios \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-test-bios/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-test-bios/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-test-bios/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=test-bios,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-test-bios/master-key.aes"}' \ +-blockdev '{"driver":"file","filename":"/var/lib/libvirt/qemu/nvram/test-bios.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":"/some/vars/path.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,usb=off,dump-guest-core=off,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format,memory-backend=pc.ram \ +-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 362d1fc1-df7d-193e-5c18-49a71bd1da66 \ +-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 menu=on,strict=on \ +-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ +-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \ +-device '{"driver":"ide-hd","bus":"ide.0","unit":0,"drive":"libvirt-1-format","id":"ide0-0-0","bootindex":1}' \ +-device '{"driver":"usb-tablet","id":"input0","bus":"usb.0","port":"1"}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/bios-nvram-rw-vars.xml b/tests/qemuxml2argvdata/bios-nvram-rw-vars.xml new file mode 100644 index 0000000000..12fdb251f3 --- /dev/null +++ b/tests/qemuxml2argvdata/bios-nvram-rw-vars.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>test-bios</name> + <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <loader readonly='no' type='pflash'>/var/lib/libvirt/qemu/nvram/test-bios.fd</loader> + <nvram>/some/vars/path.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='tablet' bus='usb'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a7e8246d38..a43f19b7a6 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1270,6 +1270,9 @@ mymain(void) DO_TEST_PARSE_ERROR_NOCAPS("bios-nvram-no-path"); DO_TEST_CAPS_LATEST("bios-nvram-rw"); DO_TEST_CAPS_LATEST("bios-nvram-rw-implicit"); + DO_TEST_PARSE_ERROR_NOCAPS("bios-nvram-rw-template"); + DO_TEST_CAPS_LATEST("bios-nvram-rw-template-vars"); + DO_TEST_CAPS_LATEST("bios-nvram-rw-vars"); DO_TEST("bios-nvram-secure", QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_DEVICE_PCI_BRIDGE, -- 2.34.1
On 2/15/22 19:54, Daniel P. Berrangé wrote:
The QEMU driver will populate the template to the nvram file any time it sees both the template and nvram paths present. It will auto-generate a nvram path per-VM if not provided by the user, but only if the loader is marked R/O.
So with a R/O loader we have these possible scenarios
- No NVRAM path or template -> try to infer a template based on the loader path, if not possible, fatal error. Auto-generate NVRAM per per VM - NVRAM path only -> try to infer a template based on the loader path, if not possible, app must have pre-created NVRAM - NVRAM path + template -> QEMU driver will copy template to NVRAM - NVRAM template only -> auto-generate NVRAM path per VM and then copy template
While with a R/W loader we have these possible scenarios
- No NVRAM path or template -> do nothing - NVRAM path only -> app must have pre-created NVRAM - NVRAM path + template -> QEMU driver will copy template to NVRAM - NVRAM template only -> silently ignored
This change improves the last scenario by reporting an error from the parser. Two alternative strategies though would be:
- Auto-generate a NVRAM path per VM - Don't support templates at all with R/W loader
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_conf.c | 16 ++++++-- ...-nvram-rw-template-vars.x86_64-latest.args | 41 +++++++++++++++++++ .../bios-nvram-rw-template-vars.xml | 36 ++++++++++++++++ .../bios-nvram-rw-template.err | 1 + .../bios-nvram-rw-template.xml | 36 ++++++++++++++++ .../bios-nvram-rw-vars.x86_64-latest.args | 41 +++++++++++++++++++ tests/qemuxml2argvdata/bios-nvram-rw-vars.xml | 36 ++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ 8 files changed, 207 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template-vars.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template-vars.xml create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template.err create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template.xml create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-vars.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-vars.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ab8f2a52cc..31b49c4ec9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4808,17 +4808,26 @@ virDomainDefPostParseMemory(virDomainDef *def, }
-static void +static int virDomainDefPostParseOs(virDomainDef *def) { if (!def->os.loader) - return; + return 0;
if (def->os.loader->path && def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_NONE) { /* By default, loader is type of 'rom' */ def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM; } + + if (def->os.loader->readonly != VIR_TRISTATE_BOOL_YES && + def->os.loader->templt && !def->os.loader->nvram) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("NVRAM template without VARs path not permitted with writable loader")); + return -1; + }
virDomainDefOSValidate() looks like a better fit for this check.
+ + return 0; }
Michal
When using <os firmware='...'/> we still parse the <nvram> path, but completely ignore it, replacing any user provided content with a custom generated path. This makes sense since when undefining the guest, the code to cleanup NVRAM also uses the same generated path. Instead of silently ignoring user config, we should report an explicit error message. This shows that some of our tests had the bogus config scenario present. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_conf.c | 8 +++ tests/qemuxml2argvdata/os-firmware-bios.xml | 1 - .../os-firmware-efi-bad-nvram-path.err | 1 + .../os-firmware-efi-bad-nvram-path.xml | 68 +++++++++++++++++++ .../os-firmware-efi-secboot.xml | 1 - tests/qemuxml2argvdata/os-firmware-efi.xml | 1 - tests/qemuxml2argvtest.c | 1 + .../os-firmware-bios.x86_64-latest.xml | 1 - .../os-firmware-efi-secboot.x86_64-latest.xml | 1 - .../os-firmware-efi.x86_64-latest.xml | 1 - 10 files changed, 78 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.err create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 31b49c4ec9..946a80c239 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4827,6 +4827,14 @@ virDomainDefPostParseOs(virDomainDef *def) return -1; } + if (def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) { + if (def->os.loader->nvram) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("NVRAM path is not permitted with firmware attribute")); + return -1; + } + } + return 0; } diff --git a/tests/qemuxml2argvdata/os-firmware-bios.xml b/tests/qemuxml2argvdata/os-firmware-bios.xml index 63886666dd..d89fcb6c58 100644 --- a/tests/qemuxml2argvdata/os-firmware-bios.xml +++ b/tests/qemuxml2argvdata/os-firmware-bios.xml @@ -7,7 +7,6 @@ <os firmware='bios'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> <loader secure='no'/> - <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> <boot dev='hd'/> <bootmenu enable='yes'/> </os> diff --git a/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.err b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.err new file mode 100644 index 0000000000..2ba8135ad4 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.err @@ -0,0 +1 @@ +XML error: NVRAM path is not permitted with firmware attribute diff --git a/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.xml b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.xml new file mode 100644 index 0000000000..a4afdb6d0b --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.xml @@ -0,0 +1,68 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <loader secure='no'/> + <nvram>/some/path</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x2'/> + </controller> + <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'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/os-firmware-efi-secboot.xml b/tests/qemuxml2argvdata/os-firmware-efi-secboot.xml index a285e06334..51faac54bf 100644 --- a/tests/qemuxml2argvdata/os-firmware-efi-secboot.xml +++ b/tests/qemuxml2argvdata/os-firmware-efi-secboot.xml @@ -7,7 +7,6 @@ <os firmware='efi'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> <loader secure='yes'/> - <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> <boot dev='hd'/> <bootmenu enable='yes'/> </os> diff --git a/tests/qemuxml2argvdata/os-firmware-efi.xml b/tests/qemuxml2argvdata/os-firmware-efi.xml index 46a7b1b780..cb21437ed8 100644 --- a/tests/qemuxml2argvdata/os-firmware-efi.xml +++ b/tests/qemuxml2argvdata/os-firmware-efi.xml @@ -7,7 +7,6 @@ <os firmware='efi'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> <loader secure='no'/> - <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> <boot dev='hd'/> <bootmenu enable='yes'/> </os> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a43f19b7a6..8909dcd064 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3478,6 +3478,7 @@ mymain(void) DO_TEST_CAPS_LATEST("os-firmware-bios"); DO_TEST_CAPS_LATEST("os-firmware-efi"); + DO_TEST_PARSE_ERROR_NOCAPS("os-firmware-efi-bad-nvram-path"); DO_TEST_CAPS_LATEST("os-firmware-efi-secboot"); DO_TEST_CAPS_LATEST("os-firmware-efi-no-enrolled-keys"); DO_TEST_CAPS_ARCH_LATEST("aarch64-os-firmware-efi", "aarch64"); diff --git a/tests/qemuxml2xmloutdata/os-firmware-bios.x86_64-latest.xml b/tests/qemuxml2xmloutdata/os-firmware-bios.x86_64-latest.xml index df6f61421a..a278ff059c 100644 --- a/tests/qemuxml2xmloutdata/os-firmware-bios.x86_64-latest.xml +++ b/tests/qemuxml2xmloutdata/os-firmware-bios.x86_64-latest.xml @@ -7,7 +7,6 @@ <os firmware='bios'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> <loader secure='no'/> - <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> <boot dev='hd'/> <bootmenu enable='yes'/> </os> diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.x86_64-latest.xml b/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.x86_64-latest.xml index c383546cc6..e7224896aa 100644 --- a/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.x86_64-latest.xml +++ b/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.x86_64-latest.xml @@ -7,7 +7,6 @@ <os firmware='efi'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> <loader secure='yes'/> - <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> <boot dev='hd'/> <bootmenu enable='yes'/> </os> diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi.x86_64-latest.xml b/tests/qemuxml2xmloutdata/os-firmware-efi.x86_64-latest.xml index 04d57860e7..73f4b9a033 100644 --- a/tests/qemuxml2xmloutdata/os-firmware-efi.x86_64-latest.xml +++ b/tests/qemuxml2xmloutdata/os-firmware-efi.x86_64-latest.xml @@ -7,7 +7,6 @@ <os firmware='efi'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> <loader secure='no'/> - <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> <boot dev='hd'/> <bootmenu enable='yes'/> </os> -- 2.34.1
On 2/15/22 19:54, Daniel P. Berrangé wrote:
When using <os firmware='...'/> we still parse the <nvram> path, but completely ignore it, replacing any user provided content with a custom generated path. This makes sense since when undefining the guest, the code to cleanup NVRAM also uses the same generated path.
Instead of silently ignoring user config, we should report an explicit error message. This shows that some of our tests had the bogus config scenario present.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_conf.c | 8 +++ tests/qemuxml2argvdata/os-firmware-bios.xml | 1 - .../os-firmware-efi-bad-nvram-path.err | 1 + .../os-firmware-efi-bad-nvram-path.xml | 68 +++++++++++++++++++ .../os-firmware-efi-secboot.xml | 1 - tests/qemuxml2argvdata/os-firmware-efi.xml | 1 - tests/qemuxml2argvtest.c | 1 + .../os-firmware-bios.x86_64-latest.xml | 1 - .../os-firmware-efi-secboot.x86_64-latest.xml | 1 - .../os-firmware-efi.x86_64-latest.xml | 1 - 10 files changed, 78 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.err create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 31b49c4ec9..946a80c239 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4827,6 +4827,14 @@ virDomainDefPostParseOs(virDomainDef *def) return -1; }
+ if (def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) { + if (def->os.loader->nvram) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("NVRAM path is not permitted with firmware attribute")); + return -1; + } + } + return 0;
Again, virDomainDefOSValidate(), ...
}
diff --git a/tests/qemuxml2argvdata/os-firmware-bios.xml b/tests/qemuxml2argvdata/os-firmware-bios.xml index 63886666dd..d89fcb6c58 100644 --- a/tests/qemuxml2argvdata/os-firmware-bios.xml +++ b/tests/qemuxml2argvdata/os-firmware-bios.xml @@ -7,7 +7,6 @@ <os firmware='bios'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> <loader secure='no'/> - <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> <boot dev='hd'/> <bootmenu enable='yes'/> </os> diff --git a/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.err b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.err new file mode 100644 index 0000000000..2ba8135ad4 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.err @@ -0,0 +1 @@ +XML error: NVRAM path is not permitted with firmware attribute
... which will have unfortunate result that this error message will look different, because one of earlier validators reports an error.
diff --git a/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.xml b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.xml new file mode 100644 index 0000000000..a4afdb6d0b --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.xml @@ -0,0 +1,68 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <loader secure='no'/> + <nvram>/some/path</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x2'/> + </controller> + <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'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon>
Nit pick, I've gotten so far that machine type is plain 'q35' and removed all these devices. They are not needed since this test is bound to fail anyway.
+ </devices> +</domain>
Michal
Before creating a NVRAM path, the qemuDomainNVRAMPathGenerate method checks whether the config is using the old style firmware approach. This check is redundant in one of the two callers. By inlining the check into the other caller, it makes it clearer to understand that the NVRAM path filling is done conditionally. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_domain.c | 14 +++----------- src/qemu/qemu_domain.h | 4 ---- src/qemu/qemu_firmware.c | 7 ++++--- 3 files changed, 7 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4e9c845f68..3c3dfe5984 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4402,7 +4402,9 @@ qemuDomainDefPostParse(virDomainDef *def, def->os.machine = g_strdup(machine); } - qemuDomainNVRAMPathGenerate(cfg, def); + if (virDomainDefHasOldStyleROUEFI(def) && + !def->os.loader->nvram) + qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram); if (qemuDomainDefAddDefaultDevices(driver, def, qemuCaps) < 0) return -1; @@ -10963,16 +10965,6 @@ qemuDomainNVRAMPathFormat(virQEMUDriverConfig *cfg, } -void -qemuDomainNVRAMPathGenerate(virQEMUDriverConfig *cfg, - virDomainDef *def) -{ - if (virDomainDefHasOldStyleROUEFI(def) && - !def->os.loader->nvram) - qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram); -} - - virDomainEventSuspendedDetailType qemuDomainPausedReasonToSuspendedEvent(virDomainPausedReason reason) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 3ca6ef084e..d2cb5775e8 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -983,10 +983,6 @@ qemuDomainNVRAMPathFormat(virQEMUDriverConfig *cfg, virDomainDef *def, char **path); -void -qemuDomainNVRAMPathGenerate(virQEMUDriverConfig *cfg, - virDomainDef *def); - virDomainEventSuspendedDetailType qemuDomainPausedReasonToSuspendedEvent(virDomainPausedReason reason); diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 84c80eaacb..a7373e3026 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1139,11 +1139,12 @@ qemuFirmwareEnableFeatures(virQEMUDriver *driver, VIR_FREE(def->os.loader->templt); def->os.loader->templt = g_strdup(flash->nvram_template.filename); - qemuDomainNVRAMPathGenerate(cfg, def); + qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram); - VIR_DEBUG("decided on firmware '%s' varstore template '%s'", + VIR_DEBUG("decided on firmware '%s' template '%s' NVRAM '%s'", def->os.loader->path, - def->os.loader->templt); + def->os.loader->templt, + def->os.loader->nvram); break; case QEMU_FIRMWARE_DEVICE_KERNEL: -- 2.34.1
This is to make it explicit that the template only applies to the NVRAM store, not the main loader binary, even if the loader is writable. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_conf.c | 10 +++++----- src/conf/domain_conf.h | 2 +- src/qemu/qemu_firmware.c | 8 ++++---- src/qemu/qemu_process.c | 4 ++-- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 946a80c239..13ec64c19f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3528,7 +3528,7 @@ virDomainLoaderDefFree(virDomainLoaderDef *loader) g_free(loader->path); g_free(loader->nvram); - g_free(loader->templt); + g_free(loader->nvramTemplate); g_free(loader); } @@ -4821,7 +4821,7 @@ virDomainDefPostParseOs(virDomainDef *def) } if (def->os.loader->readonly != VIR_TRISTATE_BOOL_YES && - def->os.loader->templt && !def->os.loader->nvram) { + def->os.loader->nvramTemplate && !def->os.loader->nvram) { virReportError(VIR_ERR_XML_ERROR, "%s", _("NVRAM template without VARs path not permitted with writable loader")); return -1; @@ -18243,7 +18243,7 @@ virDomainDefParseBootLoaderOptions(virDomainDef *def, def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt); if (!fwAutoSelect) - def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt); + def->os.loader->nvramTemplate = virXPathString("string(./os/nvram[1]/@template)", ctxt); return 0; } @@ -26930,9 +26930,9 @@ virDomainLoaderDefFormat(virBuffer *buf, else virBufferAddLit(buf, "/>\n"); - if (loader->nvram || loader->templt) { + if (loader->nvram || loader->nvramTemplate) { virBufferAddLit(buf, "<nvram"); - virBufferEscapeString(buf, " template='%s'", loader->templt); + virBufferEscapeString(buf, " template='%s'", loader->nvramTemplate); if (loader->nvram) virBufferEscapeString(buf, ">%s</nvram>\n", loader->nvram); else diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b2922e8cff..9fcf842ee7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2220,7 +2220,7 @@ struct _virDomainLoaderDef { virDomainLoader type; virTristateBool secure; char *nvram; /* path to non-volatile RAM */ - char *templt; /* user override of path to master nvram */ + char *nvramTemplate; /* user override of path to master nvram */ }; void virDomainLoaderDefFree(virDomainLoaderDef *loader); diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index a7373e3026..9ef4399838 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1136,14 +1136,14 @@ qemuFirmwareEnableFeatures(virQEMUDriver *driver, return -1; } - VIR_FREE(def->os.loader->templt); - def->os.loader->templt = g_strdup(flash->nvram_template.filename); + VIR_FREE(def->os.loader->nvramTemplate); + def->os.loader->nvramTemplate = g_strdup(flash->nvram_template.filename); qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram); VIR_DEBUG("decided on firmware '%s' template '%s' NVRAM '%s'", def->os.loader->path, - def->os.loader->templt, + def->os.loader->nvramTemplate, def->os.loader->nvram); break; @@ -1309,7 +1309,7 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, * its path in domain XML) but no template for NVRAM was * specified and the varstore doesn't exist ... */ if (!virDomainDefHasOldStyleROUEFI(def) || - def->os.loader->templt || + def->os.loader->nvramTemplate || virFileExists(def->os.loader->nvram)) return 0; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7066696f31..6057e25717 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4440,8 +4440,8 @@ qemuPrepareNVRAM(virQEMUDriver *driver, (virFileExists(loader->nvram) && !reset_nvram)) return 0; - master_nvram_path = loader->templt; - if (!loader->templt) { + master_nvram_path = loader->nvramTemplate; + if (!loader->nvramTemplate) { size_t i; for (i = 0; i < cfg->nfirmwares; i++) { if (STREQ(cfg->firmwares[i]->name, loader->path)) { -- 2.34.1
Instead of using XPath, parse the <nvram> using the XML node and property helpers so the code is consistent with the parsing of <loader>. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_conf.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 13ec64c19f..6163910c91 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18229,21 +18229,30 @@ virDomainDefParseBootLoaderOptions(virDomainDef *def, xmlXPathContextPtr ctxt) { xmlNodePtr loader_node = virXPathNode("./os/loader[1]", ctxt); + xmlNodePtr nvram_node = virXPathNode("./os/nvram[1]", ctxt); const bool fwAutoSelect = def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE; + virDomainLoaderDef *loader; if (!loader_node) return 0; - def->os.loader = g_new0(virDomainLoaderDef, 1); + def->os.loader = loader = g_new0(virDomainLoaderDef, 1); if (virDomainLoaderDefParseXML(loader_node, def->os.loader, fwAutoSelect) < 0) return -1; - def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt); - if (!fwAutoSelect) - def->os.loader->nvramTemplate = virXPathString("string(./os/nvram[1]/@template)", ctxt); + if (nvram_node) { + if (!(loader->nvram = virXMLNodeContentString(nvram_node))) + return -1; + + if (STREQ(loader->nvram, "")) + VIR_FREE(loader->nvram); + + if (!fwAutoSelect) + def->os.loader->nvramTemplate = virXMLPropString(nvram_node, "template"); + } return 0; } -- 2.34.1
The virDomainLoaderDef struct contains fields for both <loader> and <nvram> elements, so it makes sense to parse them in the same method, just like we'll format them in the same method. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_conf.c | 92 ++++++++++++++++++------------------------ 1 file changed, 40 insertions(+), 52 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6163910c91..ac2e068aea 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17813,29 +17813,51 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDef *def) } static int -virDomainLoaderDefParseXML(xmlNodePtr node, - virDomainLoaderDef *loader, - bool fwAutoSelect) +virDomainLoaderDefParseXML(virDomainDef *def, + xmlXPathContextPtr ctxt) { - if (!fwAutoSelect) { - if (virXMLPropTristateBool(node, "readonly", VIR_XML_PROP_NONE, - &loader->readonly) < 0) - return -1; + xmlNodePtr loader_node = virXPathNode("./os/loader[1]", ctxt); + xmlNodePtr nvram_node = virXPathNode("./os/nvram[1]", ctxt); + const bool fwAutoSelect = def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE; + virDomainLoaderDef *loader; + + if (!loader_node && !nvram_node) + return 0; + + def->os.loader = loader = g_new0(virDomainLoaderDef, 1); + + if (loader_node) { + if (!fwAutoSelect) { + if (virXMLPropTristateBool(loader_node, "readonly", VIR_XML_PROP_NONE, + &loader->readonly) < 0) + return -1; + + if (virXMLPropEnum(loader_node, "type", virDomainLoaderTypeFromString, + VIR_XML_PROP_NONZERO, &loader->type) < 0) + return -1; - if (virXMLPropEnum(node, "type", virDomainLoaderTypeFromString, - VIR_XML_PROP_NONZERO, &loader->type) < 0) + if (!(loader->path = virXMLNodeContentString(loader_node))) + return -1; + + if (STREQ(loader->path, "")) + VIR_FREE(loader->path); + } + + if (virXMLPropTristateBool(loader_node, "secure", VIR_XML_PROP_NONE, + &loader->secure) < 0) return -1; + } - if (!(loader->path = virXMLNodeContentString(node))) + if (nvram_node) { + if (!(loader->nvram = virXMLNodeContentString(nvram_node))) return -1; - if (STREQ(loader->path, "")) - VIR_FREE(loader->path); - } + if (STREQ(loader->nvram, "")) + VIR_FREE(loader->nvram); - if (virXMLPropTristateBool(node, "secure", VIR_XML_PROP_NONE, - &loader->secure) < 0) - return -1; + if (!fwAutoSelect) + loader->nvramTemplate = virXMLPropString(nvram_node, "template"); + } return 0; } @@ -18224,40 +18246,6 @@ virDomainDefParseBootFirmwareOptions(virDomainDef *def, } -static int -virDomainDefParseBootLoaderOptions(virDomainDef *def, - xmlXPathContextPtr ctxt) -{ - xmlNodePtr loader_node = virXPathNode("./os/loader[1]", ctxt); - xmlNodePtr nvram_node = virXPathNode("./os/nvram[1]", ctxt); - const bool fwAutoSelect = def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE; - virDomainLoaderDef *loader; - - if (!loader_node) - return 0; - - def->os.loader = loader = g_new0(virDomainLoaderDef, 1); - - if (virDomainLoaderDefParseXML(loader_node, - def->os.loader, - fwAutoSelect) < 0) - return -1; - - if (nvram_node) { - if (!(loader->nvram = virXMLNodeContentString(nvram_node))) - return -1; - - if (STREQ(loader->nvram, "")) - VIR_FREE(loader->nvram); - - if (!fwAutoSelect) - def->os.loader->nvramTemplate = virXMLPropString(nvram_node, "template"); - } - - return 0; -} - - static int virDomainDefParseBootAcpiOptions(virDomainDef *def, xmlXPathContextPtr ctxt) @@ -18322,7 +18310,7 @@ virDomainDefParseBootOptions(virDomainDef *def, if (virDomainDefParseBootFirmwareOptions(def, ctxt) < 0) return -1; - if (virDomainDefParseBootLoaderOptions(def, ctxt) < 0) + if (virDomainLoaderDefParseXML(def, ctxt) < 0) return -1; if (virDomainDefParseBootAcpiOptions(def, ctxt) < 0) @@ -18338,7 +18326,7 @@ virDomainDefParseBootOptions(virDomainDef *def, case VIR_DOMAIN_OSTYPE_UML: virDomainDefParseBootKernelOptions(def, ctxt); - if (virDomainDefParseBootLoaderOptions(def, ctxt) < 0) + if (virDomainLoaderDefParseXML(def, ctxt) < 0) return -1; break; -- 2.34.1
Currently if the <os> firmware attribute is set then we silently ignore most of the <loader> and <nvram> element configs. This changes the code so that we always fully parse the <loader> and <nvram> but then use a post-parse method to explicitly reject invalid combinations. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_conf.c | 50 ++++++++++---- .../os-firmware-efi-bad-loader-path.err | 1 + .../os-firmware-efi-bad-loader-path.xml | 67 ++++++++++++++++++ .../os-firmware-efi-bad-loader-type.err | 1 + .../os-firmware-efi-bad-loader-type.xml | 67 ++++++++++++++++++ .../os-firmware-efi-bad-nvram-template.err | 1 + .../os-firmware-efi-bad-nvram-template.xml | 68 +++++++++++++++++++ tests/qemuxml2argvtest.c | 3 + 8 files changed, 243 insertions(+), 15 deletions(-) create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-loader-path.err create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-loader-path.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-loader-type.err create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-loader-type.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-template.err create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-template.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ac2e068aea..542c9bda12 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4828,6 +4828,30 @@ virDomainDefPostParseOs(virDomainDef *def) } if (def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) { + if (def->os.loader->path) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Loader path is not permitted with firmware attribute")); + return -1; + } + + if (def->os.loader->type) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Loader type is not permitted with firmware attribute")); + return -1; + } + + if (def->os.loader->readonly) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Loader read-only attribute is not permitted with firmware attribute")); + return -1; + } + + if (def->os.loader->nvramTemplate) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("NVRAM template path is not permitted with firmware attribute")); + return -1; + } + if (def->os.loader->nvram) { virReportError(VIR_ERR_XML_ERROR, "%s", _("NVRAM path is not permitted with firmware attribute")); @@ -17818,7 +17842,6 @@ virDomainLoaderDefParseXML(virDomainDef *def, { xmlNodePtr loader_node = virXPathNode("./os/loader[1]", ctxt); xmlNodePtr nvram_node = virXPathNode("./os/nvram[1]", ctxt); - const bool fwAutoSelect = def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE; virDomainLoaderDef *loader; if (!loader_node && !nvram_node) @@ -17827,21 +17850,19 @@ virDomainLoaderDefParseXML(virDomainDef *def, def->os.loader = loader = g_new0(virDomainLoaderDef, 1); if (loader_node) { - if (!fwAutoSelect) { - if (virXMLPropTristateBool(loader_node, "readonly", VIR_XML_PROP_NONE, - &loader->readonly) < 0) - return -1; + if (virXMLPropTristateBool(loader_node, "readonly", VIR_XML_PROP_NONE, + &loader->readonly) < 0) + return -1; - if (virXMLPropEnum(loader_node, "type", virDomainLoaderTypeFromString, - VIR_XML_PROP_NONZERO, &loader->type) < 0) - return -1; + if (virXMLPropEnum(loader_node, "type", virDomainLoaderTypeFromString, + VIR_XML_PROP_NONZERO, &loader->type) < 0) + return -1; - if (!(loader->path = virXMLNodeContentString(loader_node))) - return -1; + if (!(loader->path = virXMLNodeContentString(loader_node))) + return -1; - if (STREQ(loader->path, "")) - VIR_FREE(loader->path); - } + if (STREQ(loader->path, "")) + VIR_FREE(loader->path); if (virXMLPropTristateBool(loader_node, "secure", VIR_XML_PROP_NONE, &loader->secure) < 0) @@ -17855,8 +17876,7 @@ virDomainLoaderDefParseXML(virDomainDef *def, if (STREQ(loader->nvram, "")) VIR_FREE(loader->nvram); - if (!fwAutoSelect) - loader->nvramTemplate = virXMLPropString(nvram_node, "template"); + loader->nvramTemplate = virXMLPropString(nvram_node, "template"); } return 0; diff --git a/tests/qemuxml2argvdata/os-firmware-efi-bad-loader-path.err b/tests/qemuxml2argvdata/os-firmware-efi-bad-loader-path.err new file mode 100644 index 0000000000..a8dbd0d6d8 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-bad-loader-path.err @@ -0,0 +1 @@ +XML error: Loader path is not permitted with firmware attribute diff --git a/tests/qemuxml2argvdata/os-firmware-efi-bad-loader-path.xml b/tests/qemuxml2argvdata/os-firmware-efi-bad-loader-path.xml new file mode 100644 index 0000000000..02eec67c35 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-bad-loader-path.xml @@ -0,0 +1,67 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <loader secure='no'>/some/path</loader> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x2'/> + </controller> + <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'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/os-firmware-efi-bad-loader-type.err b/tests/qemuxml2argvdata/os-firmware-efi-bad-loader-type.err new file mode 100644 index 0000000000..2824399628 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-bad-loader-type.err @@ -0,0 +1 @@ +XML error: Loader type is not permitted with firmware attribute diff --git a/tests/qemuxml2argvdata/os-firmware-efi-bad-loader-type.xml b/tests/qemuxml2argvdata/os-firmware-efi-bad-loader-type.xml new file mode 100644 index 0000000000..9091a2a8ce --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-bad-loader-type.xml @@ -0,0 +1,67 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <loader secure='no' type='pflash'/> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x2'/> + </controller> + <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'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-template.err b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-template.err new file mode 100644 index 0000000000..866ef34ec4 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-template.err @@ -0,0 +1 @@ +XML error: NVRAM template path is not permitted with firmware attribute diff --git a/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-template.xml b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-template.xml new file mode 100644 index 0000000000..cf77ca5433 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-template.xml @@ -0,0 +1,68 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <loader secure='no'/> + <nvram template="/some/path">/some/vars</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x2'/> + </controller> + <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'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 8909dcd064..82105892b0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3478,7 +3478,10 @@ mymain(void) DO_TEST_CAPS_LATEST("os-firmware-bios"); DO_TEST_CAPS_LATEST("os-firmware-efi"); + DO_TEST_PARSE_ERROR_NOCAPS("os-firmware-efi-bad-loader-path"); + DO_TEST_PARSE_ERROR_NOCAPS("os-firmware-efi-bad-loader-type"); DO_TEST_PARSE_ERROR_NOCAPS("os-firmware-efi-bad-nvram-path"); + DO_TEST_PARSE_ERROR_NOCAPS("os-firmware-efi-bad-nvram-template"); DO_TEST_CAPS_LATEST("os-firmware-efi-secboot"); DO_TEST_CAPS_LATEST("os-firmware-efi-no-enrolled-keys"); DO_TEST_CAPS_ARCH_LATEST("aarch64-os-firmware-efi", "aarch64"); -- 2.34.1
On 2/15/22 19:54, Daniel P. Berrangé wrote:
There are a mind bending number of possible ways to configure the firmware with/without NVRAM. Only a small portion are tested and many error scenarios are silently ignored.
This series attempts to get coverage of every possible XML config scenario and report explicit errors in all invalid configs.
There is an open question on patch 4. Essentially the use of NVRAM combined with writable executable feels like an accidental feature in libvirt that hasn't really been thought through. I'd like to better define expectations here but there are several possible strategies and I'm undecided which is best.
Daniel P. Berrangé (10): qemu: fix bad indentation for qemuDomainNVRAMPathFormat tests: add explicit test case for pflash loader lacking path tests: add test case for NVRAM with template conf: validate NVRAM template usage with R/W loader binary tests: don't permit NVRAM path when using firmware auto-select qemu: inline code for filling in per-VM NVRAM path conf: rename struct field for NVRAM template conf: switch nvram parsing to use XML node / property helpers conf: move nvram parsing into virDomainLoaderDefParseXML conf: stop ignoring <loader>/<nvram> with firmware auto-select
src/conf/domain_conf.c | 121 +++++++++++------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_domain.c | 18 +-- src/qemu/qemu_domain.h | 8 +- src/qemu/qemu_firmware.c | 13 +- src/qemu/qemu_process.c | 4 +- tests/qemuxml2argvdata/bios-nvram-no-path.err | 1 + tests/qemuxml2argvdata/bios-nvram-no-path.xml | 19 +++ ...-nvram-rw-template-vars.x86_64-latest.args | 41 ++++++ .../bios-nvram-rw-template-vars.xml | 36 ++++++ .../bios-nvram-rw-template.err | 1 + .../bios-nvram-rw-template.xml | 36 ++++++ .../bios-nvram-rw-vars.x86_64-latest.args | 41 ++++++ tests/qemuxml2argvdata/bios-nvram-rw-vars.xml | 36 ++++++ .../bios-nvram-template.x86_64-latest.args | 37 ++++++ .../qemuxml2argvdata/bios-nvram-template.xml | 21 +++ tests/qemuxml2argvdata/os-firmware-bios.xml | 1 - .../os-firmware-efi-bad-loader-path.err | 1 + .../os-firmware-efi-bad-loader-path.xml | 67 ++++++++++ .../os-firmware-efi-bad-loader-type.err | 1 + .../os-firmware-efi-bad-loader-type.xml | 67 ++++++++++ .../os-firmware-efi-bad-nvram-path.err | 1 + .../os-firmware-efi-bad-nvram-path.xml | 68 ++++++++++ .../os-firmware-efi-bad-nvram-template.err | 1 + .../os-firmware-efi-bad-nvram-template.xml | 68 ++++++++++ .../os-firmware-efi-secboot.xml | 1 - tests/qemuxml2argvdata/os-firmware-efi.xml | 1 - tests/qemuxml2argvtest.c | 9 ++ .../os-firmware-bios.x86_64-latest.xml | 1 - .../os-firmware-efi-secboot.x86_64-latest.xml | 1 - .../os-firmware-efi.x86_64-latest.xml | 1 - 31 files changed, 647 insertions(+), 77 deletions(-) create mode 100644 tests/qemuxml2argvdata/bios-nvram-no-path.err create mode 100644 tests/qemuxml2argvdata/bios-nvram-no-path.xml create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template-vars.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template-vars.xml create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template.err create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-template.xml create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-vars.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-rw-vars.xml create mode 100644 tests/qemuxml2argvdata/bios-nvram-template.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-template.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-loader-path.err create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-loader-path.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-loader-type.err create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-loader-type.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.err create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-path.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-template.err create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-bad-nvram-template.xml
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On 2/16/22 8:17 AM, Michal Prívozník wrote:
On 2/15/22 19:54, Daniel P. Berrangé wrote:
There are a mind bending number of possible ways to configure the firmware with/without NVRAM. Only a small portion are tested and many error scenarios are silently ignored.
This series attempts to get coverage of every possible XML config scenario and report explicit errors in all invalid configs.
There is an open question on patch 4. Essentially the use of NVRAM combined with writable executable feels like an accidental feature in libvirt that hasn't really been thought through. I'd like to better define expectations here but there are several possible strategies and I'm undecided which is best.
Daniel P. Berrangé (10): qemu: fix bad indentation for qemuDomainNVRAMPathFormat tests: add explicit test case for pflash loader lacking path tests: add test case for NVRAM with template conf: validate NVRAM template usage with R/W loader binary tests: don't permit NVRAM path when using firmware auto-select qemu: inline code for filling in per-VM NVRAM path conf: rename struct field for NVRAM template conf: switch nvram parsing to use XML node / property helpers conf: move nvram parsing into virDomainLoaderDefParseXML conf: stop ignoring <loader>/<nvram> with firmware auto-select
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Michal
I don't see the last 3 patches in git. Daniel was that intentional? Thanks, Cole
On Mon, Apr 04, 2022 at 02:45:41PM -0400, Cole Robinson wrote:
On 2/16/22 8:17 AM, Michal Prívozník wrote:
On 2/15/22 19:54, Daniel P. Berrangé wrote:
There are a mind bending number of possible ways to configure the firmware with/without NVRAM. Only a small portion are tested and many error scenarios are silently ignored.
This series attempts to get coverage of every possible XML config scenario and report explicit errors in all invalid configs.
There is an open question on patch 4. Essentially the use of NVRAM combined with writable executable feels like an accidental feature in libvirt that hasn't really been thought through. I'd like to better define expectations here but there are several possible strategies and I'm undecided which is best.
Daniel P. Berrangé (10): qemu: fix bad indentation for qemuDomainNVRAMPathFormat tests: add explicit test case for pflash loader lacking path tests: add test case for NVRAM with template conf: validate NVRAM template usage with R/W loader binary tests: don't permit NVRAM path when using firmware auto-select qemu: inline code for filling in per-VM NVRAM path conf: rename struct field for NVRAM template conf: switch nvram parsing to use XML node / property helpers conf: move nvram parsing into virDomainLoaderDefParseXML conf: stop ignoring <loader>/<nvram> with firmware auto-select
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Michal
I don't see the last 3 patches in git. Daniel was that intentional?
I didn't merge the ones with comments from Michal on, and that also invalidated the last few. I need to revisit the remaining ones again. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Cole Robinson -
Daniel P. Berrangé -
Michal Prívozník