[libvirt PATCH 1/2] conf: support stateless UEFI firmware

[Andrea Bolognani]

Apologies for this feedback coming very late - not just post-merge
but also extremely close to release.

On Fri, Jul 22, 2022 at 05:23:16PM +0100, Daniel P. Berrangé wrote:
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 3ea094e64c..4199abfd1a 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -242,7 +242,11 @@ harddisk, cdrom, network) determining where to obtain/find the boot image.
>     firmwares may implement the Secure boot feature. Attribute ``secure`` can be
>     used to tell the hypervisor that the firmware is capable of Secure Boot feature.
>     It cannot be used to enable or disable the feature itself in the firmware.
> -   :since:`Since 2.1.0`
> +   :since:`Since 2.1.0`. If the loader is marked as read-only, then with UEFI it
> +   is assumed that there will be a writable NVRAM available. In some cases,
> +   however, it may be desirable for the loader to run without any NVRAM, discarding
> +   any config changes on shutdown. The ``stateless`` flag can be used to control
> +   this behaviour, when set to ``no`` NVRAM will never be created.

Isn't the actual behavior the opposite of what you're describing
here? That is, stateless=yes is what causes the NVRAM file to not be
created.

> +++ b/src/conf/domain_validate.c
> @@ -1672,6 +1672,32 @@ virDomainDefOSValidate(const virDomainDef *def,
>          }
>      }
>
> +    if (loader->stateless == VIR_TRISTATE_BOOL_YES) {
> +        if (loader->nvramTemplate) {
> +            virReportError(VIR_ERR_XML_DETAIL, "%s",
> +                           _("NVRAM template is not permitted when loader is stateless"));
> +            return -1;
> +        }
> +
> +        if (loader->nvram) {
> +            virReportError(VIR_ERR_XML_DETAIL, "%s",
> +                           _("NVRAM is not permitted when loader is stateless"));
> +            return -1;
> +        }
> +    } else if (loader->stateless == VIR_TRISTATE_BOOL_NO) {
> +        if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) {
> +            if (def->os.loader->type != VIR_DOMAIN_LOADER_TYPE_PFLASH) {
> +                virReportError(VIR_ERR_XML_DETAIL, "%s",
> +                               _("Only pflash loader type permits NVRAM"));
> +                return -1;
> +            }
> +        } else if (def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_EFI) {
> +            virReportError(VIR_ERR_XML_DETAIL, "%s",
> +                           _("Only EFI firmware permits NVRAM"));
> +            return -1;
> +        }

These last two error messages could be improved IMO.

Consider the firmware-auto-bios-not-stateless test case, where the
input configuration looks like

  <os firmware='bios'>
    <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
    <loader stateless='no'/>
  </os>

In this case, printing out

  Only EFI firmware permits NVRAM

is a bit confusing, since the user has not directly mentioned NVRAM
anywhere. Something along the lines of

  virReportError(VIR_ERR_XML_DETAIL,
                 _("Firmware type '%s' only supports stateless operations"),
                 virDomainOsDefFirmwareTypeToString(def->os.firmware));

would be more understandable and actionable, I think.

-- 
Andrea Bolognani / Red Hat / Virtualization