[libvirt] [PATCH v2 0/4] qemu_firmware: Try to autofill for old style UEFI specification
v2 of: https://www.redhat.com/archives/libvir-list/2019-December/msg01076.html diff to v1: - split 2/2 in the original series into 3 patches as suggested by Cole Note, 1/4 is reviewed, but I haven't pushed it yet and sending it here for completeness. Michal Prívozník (4): qemu_firmware: Pass virDomainDef into qemuFirmwareFillDomain() qemu_firmware: Introduce @want variable to qemuFirmwareMatchDomain() src: Introduce and use virDomainDefHasOldStyleUEFI() and virDomainDefHasOldStyleROUEFI() qemu_firmware: Try to autofill for old style UEFI specification src/bhyve/bhyve_driver.c | 3 +- src/conf/domain_conf.c | 16 ++++++++ src/conf/domain_conf.h | 6 +++ src/libvirt_private.syms | 2 + src/libxl/libxl_conf.c | 3 +- src/libxl/xen_xl.c | 8 ++-- src/qemu/qemu_domain.c | 14 ++----- src/qemu/qemu_driver.c | 3 +- src/qemu/qemu_firmware.c | 80 +++++++++++++++++++++++++++++++++------- src/qemu/qemu_firmware.h | 2 +- src/qemu/qemu_process.c | 2 +- 11 files changed, 103 insertions(+), 36 deletions(-) -- 2.24.1
This function needs domain definition really, we don't need to pass the whole domain object. This saves couple of dereferences and characters esp. in more checks to come. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_firmware.c | 12 ++++++------ src/qemu/qemu_firmware.h | 2 +- src/qemu/qemu_process.c | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 6a76d355f5..b9bb1df179 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1202,7 +1202,7 @@ qemuFirmwareFetchParsedConfigs(bool privileged, int qemuFirmwareFillDomain(virQEMUDriverPtr driver, - virDomainObjPtr vm, + virDomainDefPtr def, unsigned int flags) { VIR_AUTOSTRINGLIST paths = NULL; @@ -1215,7 +1215,7 @@ qemuFirmwareFillDomain(virQEMUDriverPtr driver, if (!(flags & VIR_QEMU_PROCESS_START_NEW)) return 0; - if (vm->def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) + if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) return 0; if ((nfirmwares = qemuFirmwareFetchParsedConfigs(driver->privileged, @@ -1223,7 +1223,7 @@ qemuFirmwareFillDomain(virQEMUDriverPtr driver, return -1; for (i = 0; i < nfirmwares; i++) { - if (qemuFirmwareMatchDomain(vm->def, firmwares[i], paths[i])) { + if (qemuFirmwareMatchDomain(def, firmwares[i], paths[i])) { theone = firmwares[i]; VIR_DEBUG("Found matching firmware (description path '%s')", paths[i]); @@ -1234,7 +1234,7 @@ qemuFirmwareFillDomain(virQEMUDriverPtr driver, if (!theone) { virReportError(VIR_ERR_OPERATION_FAILED, _("Unable to find any firmware to satisfy '%s'"), - virDomainOsDefFirmwareTypeToString(vm->def->os.firmware)); + virDomainOsDefFirmwareTypeToString(def->os.firmware)); goto cleanup; } @@ -1243,10 +1243,10 @@ qemuFirmwareFillDomain(virQEMUDriverPtr driver, * likely that admin/FW manufacturer messed up. */ qemuFirmwareSanityCheck(theone, paths[i]); - if (qemuFirmwareEnableFeatures(driver, vm->def, theone) < 0) + if (qemuFirmwareEnableFeatures(driver, def, theone) < 0) goto cleanup; - vm->def->os.firmware = VIR_DOMAIN_OS_DEF_FIRMWARE_NONE; + def->os.firmware = VIR_DOMAIN_OS_DEF_FIRMWARE_NONE; ret = 0; cleanup: diff --git a/src/qemu/qemu_firmware.h b/src/qemu/qemu_firmware.h index 4be65bc664..37cbfae39d 100644 --- a/src/qemu/qemu_firmware.h +++ b/src/qemu/qemu_firmware.h @@ -45,7 +45,7 @@ qemuFirmwareFetchConfigs(char ***firmwares, int qemuFirmwareFillDomain(virQEMUDriverPtr driver, - virDomainObjPtr vm, + virDomainDefPtr def, unsigned int flags); int diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1e128d1d83..4195042194 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6267,7 +6267,7 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, return -1; VIR_DEBUG("Prepare bios/uefi paths"); - if (qemuFirmwareFillDomain(driver, vm, flags) < 0) + if (qemuFirmwareFillDomain(driver, vm->def, flags) < 0) return -1; if (qemuDomainInitializePflashStorageSource(vm) < 0) return -1; -- 2.24.1
On Tue, Jan 07, 2020 at 02:43:22PM +0100, Michal Privoznik wrote:
This function needs domain definition really, we don't need to pass the whole domain object. This saves couple of dereferences and characters esp. in more checks to come.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_firmware.c | 12 ++++++------ src/qemu/qemu_firmware.h | 2 +- src/qemu/qemu_process.c | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
This simplifies condition when matching FW interface by having a single line condition instead of multiline one. Also, it prepares the code for future expansion. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index b9bb1df179..a3305d5554 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -928,22 +928,39 @@ qemuFirmwareMatchesMachineArch(const qemuFirmware *fw, } +static qemuFirmwareOSInterface +qemuFirmwareOSInterfaceTypeFromOsDefFirmware(virDomainOsDefFirmware fw) +{ + switch (fw) { + case VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS: + return QEMU_FIRMWARE_OS_INTERFACE_BIOS; + case VIR_DOMAIN_OS_DEF_FIRMWARE_EFI: + return QEMU_FIRMWARE_OS_INTERFACE_UEFI; + case VIR_DOMAIN_OS_DEF_FIRMWARE_NONE: + case VIR_DOMAIN_OS_DEF_FIRMWARE_LAST: + break; + } + + return QEMU_FIRMWARE_OS_INTERFACE_NONE; +} + + static bool qemuFirmwareMatchDomain(const virDomainDef *def, const qemuFirmware *fw, const char *path) { size_t i; + qemuFirmwareOSInterface want; bool supportsS3 = false; bool supportsS4 = false; bool requiresSMM = false; bool supportsSEV = false; + want = qemuFirmwareOSInterfaceTypeFromOsDefFirmware(def->os.firmware); + for (i = 0; i < fw->ninterfaces; i++) { - if ((def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS && - fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_BIOS) || - (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI && - fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_UEFI)) + if (fw->interfaces[i] == want) break; } -- 2.24.1
On Tue, Jan 07, 2020 at 02:43:23PM +0100, Michal Privoznik wrote:
This simplifies condition when matching FW interface by having a single line condition instead of multiline one. Also, it prepares the code for future expansion.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
On Tue, Jan 07, 2020 at 02:43:23PM +0100, Michal Privoznik wrote:
This simplifies condition when matching FW interface by having a single line condition instead of multiline one. Also, it prepares the code for future expansion.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index b9bb1df179..a3305d5554 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -928,22 +928,39 @@ qemuFirmwareMatchesMachineArch(const qemuFirmware *fw, }
+static qemuFirmwareOSInterface +qemuFirmwareOSInterfaceTypeFromOsDefFirmware(virDomainOsDefFirmware fw) +{ + switch (fw) { + case VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS: + return QEMU_FIRMWARE_OS_INTERFACE_BIOS; + case VIR_DOMAIN_OS_DEF_FIRMWARE_EFI: + return QEMU_FIRMWARE_OS_INTERFACE_UEFI; + case VIR_DOMAIN_OS_DEF_FIRMWARE_NONE: + case VIR_DOMAIN_OS_DEF_FIRMWARE_LAST: + break; + } + + return QEMU_FIRMWARE_OS_INTERFACE_NONE; +} + + static bool qemuFirmwareMatchDomain(const virDomainDef *def, const qemuFirmware *fw, const char *path) { size_t i; + qemuFirmwareOSInterface want; bool supportsS3 = false; bool supportsS4 = false; bool requiresSMM = false; bool supportsSEV = false;
+ want = qemuFirmwareOSInterfaceTypeFromOsDefFirmware(def->os.firmware);
clang isn't happy with this as its passing the wrong enum ../../src/qemu/qemu_firmware.c:964:77: error: implicit conversion from enumeration type 'virDomainLoader' to different enumeration type 'virDomainOsDefFirmware' [-Werror,-Wenum-conversion] want = qemuFirmwareOSInterfaceTypeFromOsDefFirmware(def->os.loader->type); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~^~~~ 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 :|
On 1/7/20 8:08 PM, Daniel P. Berrangé wrote:
On Tue, Jan 07, 2020 at 02:43:23PM +0100, Michal Privoznik wrote:
This simplifies condition when matching FW interface by having a single line condition instead of multiline one. Also, it prepares the code for future expansion.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index b9bb1df179..a3305d5554 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -928,22 +928,39 @@ qemuFirmwareMatchesMachineArch(const qemuFirmware *fw, }
+static qemuFirmwareOSInterface +qemuFirmwareOSInterfaceTypeFromOsDefFirmware(virDomainOsDefFirmware fw) +{ + switch (fw) { + case VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS: + return QEMU_FIRMWARE_OS_INTERFACE_BIOS; + case VIR_DOMAIN_OS_DEF_FIRMWARE_EFI: + return QEMU_FIRMWARE_OS_INTERFACE_UEFI; + case VIR_DOMAIN_OS_DEF_FIRMWARE_NONE: + case VIR_DOMAIN_OS_DEF_FIRMWARE_LAST: + break; + } + + return QEMU_FIRMWARE_OS_INTERFACE_NONE; +} + + static bool qemuFirmwareMatchDomain(const virDomainDef *def, const qemuFirmware *fw, const char *path) { size_t i; + qemuFirmwareOSInterface want; bool supportsS3 = false; bool supportsS4 = false; bool requiresSMM = false; bool supportsSEV = false;
+ want = qemuFirmwareOSInterfaceTypeFromOsDefFirmware(def->os.firmware);
clang isn't happy with this as its passing the wrong enum
../../src/qemu/qemu_firmware.c:964:77: error: implicit conversion from enumeration type 'virDomainLoader' to different enumeration type 'virDomainOsDefFirmware' [-Werror,-Wenum-conversion] want = qemuFirmwareOSInterfaceTypeFromOsDefFirmware(def->os.loader->type); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~^~~~
Yeah, I've noticed this in our CI and I just pushed the fix: 8fcee47807 qemu_firmware: Accept int in qemuFirmwareOSInterfaceTypeFromOsDefFirmware() Michal
These functions are meant to replace verbose check for the old style of specifying UEFI with a simple function call. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/bhyve/bhyve_driver.c | 3 +-- src/conf/domain_conf.c | 16 ++++++++++++++++ src/conf/domain_conf.h | 6 ++++++ src/libvirt_private.syms | 2 ++ src/libxl/libxl_conf.c | 3 +-- src/libxl/xen_xl.c | 8 +++----- src/qemu/qemu_domain.c | 14 ++++---------- src/qemu/qemu_driver.c | 3 +-- 8 files changed, 34 insertions(+), 21 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index a764b4d4ed..5b8fba7467 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -704,8 +704,7 @@ bhyveConnectDomainXMLToNative(virConnectPtr conn, if (def->os.bootloader == NULL && def->os.loader) { - if ((def->os.loader->readonly != VIR_TRISTATE_BOOL_YES) || - (def->os.loader->type != VIR_DOMAIN_LOADER_TYPE_PFLASH)) { + if (!virDomainDefHasOldStyleROUEFI(def)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Only read-only pflash is supported.")); goto cleanup; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ee33b7caf0..64d7af9e3c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31510,6 +31510,22 @@ virDomainDefHasMdevHostdev(const virDomainDef *def) } +bool +virDomainDefHasOldStyleUEFI(const virDomainDef *def) +{ + return def->os.loader && + def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH; +} + + +bool +virDomainDefHasOldStyleROUEFI(const virDomainDef *def) +{ + return virDomainDefHasOldStyleUEFI(def) && + def->os.loader->readonly == VIR_TRISTATE_BOOL_YES; +} + + /** * virDomainGraphicsDefHasOpenGL: * @def: domain definition diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e012975fca..e6b06a8eba 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3673,6 +3673,12 @@ virDomainDefHasVFIOHostdev(const virDomainDef *def); bool virDomainDefHasMdevHostdev(const virDomainDef *def); +bool +virDomainDefHasOldStyleUEFI(const virDomainDef *def); + +bool +virDomainDefHasOldStyleROUEFI(const virDomainDef *def); + bool virDomainGraphicsDefHasOpenGL(const virDomainDef *def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9185e49fda..ec1054cdc3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -311,6 +311,8 @@ virDomainDefHasMdevHostdev; virDomainDefHasMemballoon; virDomainDefHasMemoryHotplug; virDomainDefHasNVMeDisk; +virDomainDefHasOldStyleROUEFI; +virDomainDefHasOldStyleUEFI; virDomainDefHasUSB; virDomainDefHasVcpusOffline; virDomainDefHasVFIOHostdev; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 1be2a789d5..2488bb9d32 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -545,8 +545,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, * future, Xen will support a user-specified firmware path. See * http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg01628.html */ - if (def->os.loader && - def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH) + if (virDomainDefHasOldStyleUEFI(def)) b_info->u.hvm.bios = LIBXL_BIOS_TYPE_OVMF; if (def->emulator) { diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index ebcea41d1c..91b1825399 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -1228,11 +1228,9 @@ xenFormatXLOS(virConfPtr conf, virDomainDefPtr def) if (xenConfigSetString(conf, "builder", "hvm") < 0) return -1; - if (def->os.loader && - def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH) { - if (xenConfigSetString(conf, "bios", "ovmf") < 0) - return -1; - } + if (virDomainDefHasOldStyleUEFI(def) && + xenConfigSetString(conf, "bios", "ovmf") < 0) + return -1; if (def->os.slic_table && xenConfigSetString(conf, "acpi_firmware", def->os.slic_table) < 0) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 065c9e97a9..788caf864c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5697,8 +5697,7 @@ qemuDomainDefValidate(const virDomainDef *def, /* On x86, UEFI requires ACPI */ if ((def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI || - (def->os.loader && - def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH)) && + virDomainDefHasOldStyleUEFI(def)) && ARCH_IS_X86(def->os.arch) && def->features[VIR_DOMAIN_FEATURE_ACPI] != VIR_TRISTATE_SWITCH_ON) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -5710,8 +5709,7 @@ qemuDomainDefValidate(const virDomainDef *def, if (def->features[VIR_DOMAIN_FEATURE_ACPI] == VIR_TRISTATE_SWITCH_ON && def->os.arch == VIR_ARCH_AARCH64 && (def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_EFI && - (!def->os.loader || - def->os.loader->type != VIR_DOMAIN_LOADER_TYPE_PFLASH))) { + !virDomainDefHasOldStyleUEFI(def))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("ACPI requires UEFI on this architecture")); goto cleanup; @@ -16608,12 +16606,9 @@ void qemuDomainNVRAMPathGenerate(virQEMUDriverConfigPtr cfg, virDomainDefPtr def) { - if (def->os.loader && - def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH && - def->os.loader->readonly == VIR_TRISTATE_BOOL_YES && + if (virDomainDefHasOldStyleROUEFI(def) && !def->os.loader->nvram) qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram); - } @@ -16740,8 +16735,7 @@ qemuDomainInitializePflashStorageSource(virDomainObjPtr vm) if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) return 0; - if (!def->os.loader || - def->os.loader->type != VIR_DOMAIN_LOADER_TYPE_PFLASH) + if (!virDomainDefHasOldStyleUEFI(def)) return 0; if (!(pflash0 = virStorageSourceNew())) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ae54c00239..34d1374d87 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15218,8 +15218,7 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, * Avoid the issues by forbidding internal snapshot with pflash completely. */ if (found_internal && - vm->def->os.loader && - vm->def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH) { + virDomainDefHasOldStyleUEFI(vm->def)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("internal snapshots of a VM with pflash based " "firmware are not supported")); -- 2.24.1
$SUBJECT could be cut down in length a bit "src: introduce helpers for checking UEFI config" On Tue, Jan 07, 2020 at 02:43:24PM +0100, Michal Privoznik wrote:
These functions are meant to replace verbose check for the old style of specifying UEFI with a simple function call.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/bhyve/bhyve_driver.c | 3 +-- src/conf/domain_conf.c | 16 ++++++++++++++++ src/conf/domain_conf.h | 6 ++++++ src/libvirt_private.syms | 2 ++ src/libxl/libxl_conf.c | 3 +-- src/libxl/xen_xl.c | 8 +++----- src/qemu/qemu_domain.c | 14 ++++---------- src/qemu/qemu_driver.c | 3 +-- 8 files changed, 34 insertions(+), 21 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
While we discourage people to use the old style of specifying UEFI for their domains (the old style is putting path to the FW image under /domain/os/loader/ whilst the new one is using /domain/os/@firmware), some applications might have not adopted yet. They still rely on libvirt autofilling NVRAM path and figuring out NVRAM template when using the old way (notably virt-install does this). And in a way they are right. However, since we really want distro maintainers to leave --with-loader-nvram configure option and rely on JSON descriptors, we need to implement autofilling of NVRAM template for the old way too. Fedora: https://bugzilla.redhat.com/show_bug.cgi?id=1782778 RHEL: https://bugzilla.redhat.com/show_bug.cgi?id=1776949 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 47 +++++++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index a3305d5554..a835fe6b4a 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -959,6 +959,21 @@ qemuFirmwareMatchDomain(const virDomainDef *def, want = qemuFirmwareOSInterfaceTypeFromOsDefFirmware(def->os.firmware); + if (want == QEMU_FIRMWARE_OS_INTERFACE_NONE && + def->os.loader) { + want = qemuFirmwareOSInterfaceTypeFromOsDefFirmware(def->os.loader->type); + + if (fw->mapping.device != QEMU_FIRMWARE_DEVICE_FLASH || + STRNEQ(def->os.loader->path, fw->mapping.data.flash.executable.filename)) { + VIR_DEBUG("Not matching FW interface %s or loader " + "path '%s' for user provided path '%s'", + qemuFirmwareDeviceTypeToString(fw->mapping.device), + fw->mapping.data.flash.executable.filename, + def->os.loader->path); + return false; + } + } + for (i = 0; i < fw->ninterfaces; i++) { if (fw->interfaces[i] == want) break; @@ -1226,14 +1241,29 @@ qemuFirmwareFillDomain(virQEMUDriverPtr driver, qemuFirmwarePtr *firmwares = NULL; ssize_t nfirmwares = 0; const qemuFirmware *theone = NULL; + bool needResult = true; size_t i; int ret = -1; if (!(flags & VIR_QEMU_PROCESS_START_NEW)) return 0; - if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) - return 0; + /* Fill in FW paths if either os.firmware is enabled, or + * loader path was provided with no nvram varstore. */ + if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) { + /* This is horrific check, but loosely said, if UEFI + * image was provided by the old method (by specifying + * 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 || + virFileExists(def->os.loader->nvram)) + return 0; + + /* ... then we want to consult JSON FW descriptors first, + * but we don't want to fail if we haven't found a match. */ + needResult = false; + } if ((nfirmwares = qemuFirmwareFetchParsedConfigs(driver->privileged, &firmwares, &paths)) < 0) @@ -1249,9 +1279,16 @@ qemuFirmwareFillDomain(virQEMUDriverPtr driver, } if (!theone) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("Unable to find any firmware to satisfy '%s'"), - virDomainOsDefFirmwareTypeToString(def->os.firmware)); + if (needResult) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Unable to find any firmware to satisfy '%s'"), + virDomainOsDefFirmwareTypeToString(def->os.firmware)); + } else { + VIR_DEBUG("Unable to find NVRAM template for '%s', " + "falling back to old style", + NULLSTR(def->os.loader ? def->os.loader->path : NULL)); + ret = 0; + } goto cleanup; } -- 2.24.1
On Tue, Jan 07, 2020 at 02:43:25PM +0100, Michal Privoznik wrote:
While we discourage people to use the old style of specifying UEFI for their domains (the old style is putting path to the FW image under /domain/os/loader/ whilst the new one is using /domain/os/@firmware), some applications might have not adopted
s/adopted/adapted/
yet. They still rely on libvirt autofilling NVRAM path and figuring out NVRAM template when using the old way (notably virt-install does this). And in a way they are right. However,
Not "in a way" - they are absolutely always right, because thats what our API guarantee means for apps. So s/And in a way they are right/We must preserve backcompat for this previously supported config approach/
since we really want distro maintainers to leave --with-loader-nvram configure option and rely on JSON descriptors, we need to implement autofilling of NVRAM template for the old way too.
Fedora: https://bugzilla.redhat.com/show_bug.cgi?id=1782778 RHEL: https://bugzilla.redhat.com/show_bug.cgi?id=1776949
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_firmware.c | 47 +++++++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 5 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 (2)
-
Daniel P. Berrangé -
Michal Privoznik