[libvirt] [PATCH 0/2] qemu_firmware: Try to autofill for old style UEFI specification
See 2/2 for info. Michal Prívozník (2): qemu_firmware: Pass virDomainDef into qemuFirmwareFillDomain() qemu_firmware: Try to autofill for old style UEFI specification src/qemu/qemu_firmware.c | 90 +++++++++++++++++++++++++++++++++------- src/qemu/qemu_firmware.h | 2 +- src/qemu/qemu_process.c | 2 +- 3 files changed, 78 insertions(+), 16 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> --- 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 f62ce90ac9..96058c9b45 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1204,7 +1204,7 @@ qemuFirmwareFetchParsedConfigs(bool privileged, int qemuFirmwareFillDomain(virQEMUDriverPtr driver, - virDomainObjPtr vm, + virDomainDefPtr def, unsigned int flags) { VIR_AUTOSTRINGLIST paths = NULL; @@ -1217,7 +1217,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, @@ -1225,7 +1225,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]); @@ -1236,7 +1236,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; } @@ -1245,10 +1245,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 7e1db50e8f..6d4511a49f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6344,7 +6344,7 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, goto cleanup; VIR_DEBUG("Prepare bios/uefi paths"); - if (qemuFirmwareFillDomain(driver, vm, flags) < 0) + if (qemuFirmwareFillDomain(driver, vm->def, flags) < 0) goto cleanup; if (qemuDomainInitializePflashStorageSource(vm) < 0) goto cleanup; -- 2.24.1
On 12/17/19 12:25 PM, 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> --- 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: Cole Robinson <crobinso@redhat.com> - Cole
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 | 82 +++++++++++++++++++++++++++++++++++----- 1 file changed, 72 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 96058c9b45..a31bde5d04 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -935,17 +935,53 @@ qemuFirmwareMatchDomain(const virDomainDef *def, const qemuFirmware *fw, const char *path) { - size_t i; + qemuFirmwareOSInterface want = QEMU_FIRMWARE_OS_INTERFACE_NONE; bool supportsS3 = false; bool supportsS4 = false; bool requiresSMM = false; bool supportsSEV = false; + size_t i; + + switch (def->os.firmware) { + case VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS: + want = QEMU_FIRMWARE_OS_INTERFACE_BIOS; + break; + case VIR_DOMAIN_OS_DEF_FIRMWARE_EFI: + want = QEMU_FIRMWARE_OS_INTERFACE_UEFI; + break; + case VIR_DOMAIN_OS_DEF_FIRMWARE_NONE: + case VIR_DOMAIN_OS_DEF_FIRMWARE_LAST: + break; + } + + if (want == QEMU_FIRMWARE_OS_INTERFACE_NONE && + def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE && + def->os.loader) { + switch (def->os.loader->type) { + case VIR_DOMAIN_LOADER_TYPE_ROM: + want = QEMU_FIRMWARE_OS_INTERFACE_BIOS; + break; + case VIR_DOMAIN_LOADER_TYPE_PFLASH: + want = QEMU_FIRMWARE_OS_INTERFACE_UEFI; + break; + case VIR_DOMAIN_LOADER_TYPE_NONE: + case VIR_DOMAIN_LOADER_TYPE_LAST: + break; + } + + 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 ((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; } @@ -1211,14 +1247,33 @@ 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 ... */ + if (!(def->os.loader && + def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH && + def->os.loader->path && + def->os.loader->readonly == VIR_TRISTATE_BOOL_YES && + !def->os.loader->templt && + def->os.loader->nvram && + !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) @@ -1234,9 +1289,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 12/17/19 12:25 PM, 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 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> ---
So this uses firmware.json to info to turn this input XML <loader readonly="yes" type="pflash">/CODE.fd</loader> into this output XML <loader readonly="yes" type="pflash">/CODE.fd</loader> <nvram template="/VARS.fd"/> independent of --with-loader-nvram, which was previously used to handle that case. And virt-install still uses this method by default. Is that correct? I'm surprised we don't have an an XML test case to cover this. How hard is it to add?
src/qemu/qemu_firmware.c | 82 +++++++++++++++++++++++++++++++++++----- 1 file changed, 72 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 96058c9b45..a31bde5d04 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -935,17 +935,53 @@ qemuFirmwareMatchDomain(const virDomainDef *def, const qemuFirmware *fw, const char *path) { - size_t i; + qemuFirmwareOSInterface want = QEMU_FIRMWARE_OS_INTERFACE_NONE; bool supportsS3 = false; bool supportsS4 = false; bool requiresSMM = false; bool supportsSEV = false; + size_t i; + + switch (def->os.firmware) { + case VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS: + want = QEMU_FIRMWARE_OS_INTERFACE_BIOS; + break; + case VIR_DOMAIN_OS_DEF_FIRMWARE_EFI: + want = QEMU_FIRMWARE_OS_INTERFACE_UEFI; + break; + case VIR_DOMAIN_OS_DEF_FIRMWARE_NONE: + case VIR_DOMAIN_OS_DEF_FIRMWARE_LAST: + break; + } +
This is a refactoring that could have been done first. It's hard to digest all the details in this patch.
+ if (want == QEMU_FIRMWARE_OS_INTERFACE_NONE && + def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE && + def->os.loader) { + switch (def->os.loader->type) { + case VIR_DOMAIN_LOADER_TYPE_ROM: + want = QEMU_FIRMWARE_OS_INTERFACE_BIOS; + break; + case VIR_DOMAIN_LOADER_TYPE_PFLASH: + want = QEMU_FIRMWARE_OS_INTERFACE_UEFI; + break; + case VIR_DOMAIN_LOADER_TYPE_NONE: + case VIR_DOMAIN_LOADER_TYPE_LAST: + break; + } +
And it might be overkill but it would help readability if these switches were moved to their own functions, like qemuFirmwareTypeFromInterfaceType or similar
+ 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 ((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; }
@@ -1211,14 +1247,33 @@ 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 ... */ + if (!(def->os.loader && + def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH && + def->os.loader->path && + def->os.loader->readonly == VIR_TRISTATE_BOOL_YES && + !def->os.loader->templt && + def->os.loader->nvram && + !virFileExists(def->os.loader->nvram))) + return 0; +
This loader checking logic is duplicated in a few places already, see all 4 of 5 PFLASH uses in qemu_domain.c and similar checks in xen code. IMO it should be factored out first - Cole
On 12/18/19 11:50 PM, Cole Robinson wrote:
On 12/17/19 12:25 PM, 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 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> ---
So this uses firmware.json to info to turn this input XML
<loader readonly="yes" type="pflash">/CODE.fd</loader>
into this output XML
<loader readonly="yes" type="pflash">/CODE.fd</loader> <nvram template="/VARS.fd"/>
independent of --with-loader-nvram, which was previously used to handle that case. And virt-install still uses this method by default. Is that correct?
Use of --with-loader-nvram will trigger a warning at compile time and use of nvram=[] in qemu.conf will trigger a warning at runtime. And we need to fill nvram template (in the code it's loader->templt) to avoid looking it up qemuPrepareNVRAM(). This is what <os firmware='uefi'/> does too. So the way this is supposed to work is: - FW descriptors are preferred, if a NVRAM template is found in a FW descriptor file it's used, - to give distro maintainers time to switch, --with-loader-nvram is not going away just yet, and will be consulted if the previous point failed, - nvram=[] from qemu.conf is ignored loudly. I think this is reasonable order. Also, note that the template is generated into live XML only, and will be generated only once - when NVRAM template is needed, i.e. at the first boot. The second time the domain is started the horrific check I'm introducing in qemuFirmwareFillDomain() will trigger 'return 0' and thus no template will be filled in.
I'm surprised we don't have an an XML test case to cover this. How hard is it to add?
The thing is, we don't hit the problem when generating the command line but later in the process in qemuPrepareNVRAM() which is called from qemuProcessPrepareHost() which is not called from tests. And I don't think we have a test that checks if live XML generated from an inactive one matches expected one.
src/qemu/qemu_firmware.c | 82 +++++++++++++++++++++++++++++++++++----- 1 file changed, 72 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 96058c9b45..a31bde5d04 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -935,17 +935,53 @@ qemuFirmwareMatchDomain(const virDomainDef *def, const qemuFirmware *fw, const char *path) { - size_t i; + qemuFirmwareOSInterface want = QEMU_FIRMWARE_OS_INTERFACE_NONE; bool supportsS3 = false; bool supportsS4 = false; bool requiresSMM = false; bool supportsSEV = false; + size_t i; + + switch (def->os.firmware) { + case VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS: + want = QEMU_FIRMWARE_OS_INTERFACE_BIOS; + break; + case VIR_DOMAIN_OS_DEF_FIRMWARE_EFI: + want = QEMU_FIRMWARE_OS_INTERFACE_UEFI; + break; + case VIR_DOMAIN_OS_DEF_FIRMWARE_NONE: + case VIR_DOMAIN_OS_DEF_FIRMWARE_LAST: + break; + } +
This is a refactoring that could have been done first. It's hard to digest all the details in this patch.
+ if (want == QEMU_FIRMWARE_OS_INTERFACE_NONE && + def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE && + def->os.loader) { + switch (def->os.loader->type) { + case VIR_DOMAIN_LOADER_TYPE_ROM: + want = QEMU_FIRMWARE_OS_INTERFACE_BIOS; + break; + case VIR_DOMAIN_LOADER_TYPE_PFLASH: + want = QEMU_FIRMWARE_OS_INTERFACE_UEFI; + break; + case VIR_DOMAIN_LOADER_TYPE_NONE: + case VIR_DOMAIN_LOADER_TYPE_LAST: + break; + } +
And it might be overkill but it would help readability if these switches were moved to their own functions, like qemuFirmwareTypeFromInterfaceType or similar
+ 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 ((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; }
@@ -1211,14 +1247,33 @@ 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 ... */ + if (!(def->os.loader && + def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH && + def->os.loader->path && + def->os.loader->readonly == VIR_TRISTATE_BOOL_YES && + !def->os.loader->templt && + def->os.loader->nvram && + !virFileExists(def->os.loader->nvram))) + return 0; +
This loader checking logic is duplicated in a few places already, see all 4 of 5 PFLASH uses in qemu_domain.c and similar checks in xen code. IMO it should be factored out first
Okay, I will try to work these into v2. Thanks, Michal
participants (2)
-
Cole Robinson -
Michal Privoznik