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