[libvirt] [PATCH 2/2] qemu_firmware: Try to autofill for old style UEFI specification

[Cole Robinson]

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 at 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