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