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@xxxxxxxxxx>
---
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
