[libvirt PATCH 0/9] improve firmware auto-selection
Cleanup the parser code and implement firmware feature filtering support to allow users to modify firmware auto-selection behavior per VM. More details in PATCH 08. Pavel Hrdina (9): docs: improve description of secure attribute for loader element conf: introduce virDomainDefParseBootInitOptions conf: introduce virDomainDefParseBootKernelOptions conf: introduce virDomainDefParseBootFirmwareOptions conf: introduce virDomainDefParseBootLoaderOptions conf: introduce virDomainDefParseBootAcpiOptions conf: use switch in virDomainDefParseBootOptions conf: introduce support for firmware auto-selection feature filtering qemu: implement support for firmware auto-selection feature filtering docs/formatdomain.rst | 35 +- docs/schemas/domaincommon.rng | 23 + src/conf/domain_conf.c | 396 ++++++++++++------ src/conf/domain_conf.h | 10 + src/qemu/qemu_firmware.c | 40 ++ .../os-firmware-efi-invalid-type.xml | 28 ++ ...re-efi-no-enrolled-keys.x86_64-latest.args | 49 +++ .../os-firmware-efi-no-enrolled-keys.xml | 25 ++ ...os-firmware-invalid-type.x86_64-latest.err | 1 + .../os-firmware-invalid-type.xml | 28 ++ tests/qemuxml2argvtest.c | 2 + ...aarch64-os-firmware-efi.aarch64-latest.xml | 1 + .../os-firmware-bios.x86_64-latest.xml | 1 + ...are-efi-no-enrolled-keys.x86_64-latest.xml | 50 +++ .../os-firmware-efi-secboot.x86_64-latest.xml | 1 + .../os-firmware-efi.x86_64-latest.xml | 1 + tests/qemuxml2xmltest.c | 1 + tests/vmx2xmldata/vmx2xml-firmware-efi.xml | 1 + 18 files changed, 567 insertions(+), 126 deletions(-) create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-invalid-type.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.xml create mode 100644 tests/qemuxml2xmloutdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.xml -- 2.30.2
The original text was not explaining what this attribute actually controls and could have been interpreted as a control switch for the Secure boot feature in firmwares. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index a2ea2690a5..c101d5a1f1 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -167,7 +167,9 @@ harddisk, cdrom, network) determining where to obtain/find the boot image. in the guest memory the file should be mapped. For instance, if the loader path points to an UEFI image, ``type`` should be ``pflash``. Moreover, some firmwares may implement the Secure boot feature. Attribute ``secure`` can be - used then to control it. :since:`Since 2.1.0` + used to tell the hypervisor that the firmware implements Secure Boot Feature. + It cannot be used to enable or disable the feature itself in the firmware. + :since:`Since 2.1.0` ``nvram`` Some UEFI firmwares may want to use a non-volatile memory to store some variables. In the host, this is represented as a file and the absolute path -- 2.30.2
On Thu, Mar 18, 2021 at 01:26:38PM +0100, Pavel Hrdina wrote:
The original text was not explaining what this attribute actually controls and could have been interpreted as a control switch for the Secure boot feature in firmwares.
Yep, I've indeed seen people misread it as such.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index a2ea2690a5..c101d5a1f1 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -167,7 +167,9 @@ harddisk, cdrom, network) determining where to obtain/find the boot image. in the guest memory the file should be mapped. For instance, if the loader path points to an UEFI image, ``type`` should be ``pflash``. Moreover, some firmwares may implement the Secure boot feature. Attribute ``secure`` can be - used then to control it. :since:`Since 2.1.0` + used to tell the hypervisor that the firmware implements Secure Boot Feature.
s/Feature/feature/ Perhaps: "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`
This additional clarification is good. (Nit-pick: not this patch's fault: consistently use "Secure Boot"; I see both "Secure boot" and "Secure Boot".) Address the above only if you're respinning. FWIW: Reviewed-by: Kashyap Chamarthy <kchamart@redhat.com>
``nvram`` Some UEFI firmwares may want to use a non-volatile memory to store some variables. In the host, this is represented as a file and the absolute path -- 2.30.2
-- /kashyap
On Thu, Mar 18, 2021 at 05:42:47PM +0100, Kashyap Chamarthy wrote:
On Thu, Mar 18, 2021 at 01:26:38PM +0100, Pavel Hrdina wrote:
The original text was not explaining what this attribute actually controls and could have been interpreted as a control switch for the Secure boot feature in firmwares.
Yep, I've indeed seen people misread it as such.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index a2ea2690a5..c101d5a1f1 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -167,7 +167,9 @@ harddisk, cdrom, network) determining where to obtain/find the boot image. in the guest memory the file should be mapped. For instance, if the loader path points to an UEFI image, ``type`` should be ``pflash``. Moreover, some firmwares may implement the Secure boot feature. Attribute ``secure`` can be - used then to control it. :since:`Since 2.1.0` + used to tell the hypervisor that the firmware implements Secure Boot Feature.
s/Feature/feature/
Perhaps: "firmware is capable of Secure Boot feature"
Sounds reasonable, will change it.
+ It cannot be used to enable or disable the feature itself in the firmware. + :since:`Since 2.1.0`
This additional clarification is good.
(Nit-pick: not this patch's fault: consistently use "Secure Boot"; I see both "Secure boot" and "Secure Boot".)
If you check our documentation we lack consistency almost everywhere :) until this is enforced by some check it will happen all the time.
Address the above only if you're respinning. FWIW:
Reviewed-by: Kashyap Chamarthy <kchamart@redhat.com>
Thanks, I need to do some other changes to the series before pushing so I'll apply this suggestions as well before pushing. Pavel
Extract the code to it's own function. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 109 +++++++++++++++++++++++------------------ 1 file changed, 61 insertions(+), 48 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 47756ff0be..31b908d8fe 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19523,12 +19523,70 @@ virDomainVcpuParse(virDomainDefPtr def, } +static int +virDomainDefParseBootInitOptions(virDomainDefPtr def, + xmlXPathContextPtr ctxt) +{ + char *name = NULL; + size_t i; + int n; + g_autofree xmlNodePtr *nodes = NULL; + + def->os.init = virXPathString("string(./os/init[1])", ctxt); + def->os.cmdline = virXPathString("string(./os/cmdline[1])", ctxt); + def->os.initdir = virXPathString("string(./os/initdir[1])", ctxt); + def->os.inituser = virXPathString("string(./os/inituser[1])", ctxt); + def->os.initgroup = virXPathString("string(./os/initgroup[1])", ctxt); + + if ((n = virXPathNodeSet("./os/initarg", ctxt, &nodes)) < 0) + return -1; + + def->os.initargv = g_new0(char *, n+1); + for (i = 0; i < n; i++) { + if (!nodes[i]->children || + !nodes[i]->children->content) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("No data supplied for <initarg> element")); + return -1; + } + def->os.initargv[i] = g_strdup((const char *)nodes[i]->children->content); + } + def->os.initargv[n] = NULL; + VIR_FREE(nodes); + + if ((n = virXPathNodeSet("./os/initenv", ctxt, &nodes)) < 0) + return -1; + + def->os.initenv = g_new0(virDomainOSEnvPtr, n+1); + for (i = 0; i < n; i++) { + if (!(name = virXMLPropString(nodes[i], "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("No name supplied for <initenv> element")); + return -1; + } + + if (!nodes[i]->children || + !nodes[i]->children->content) { + virReportError(VIR_ERR_XML_ERROR, + _("No value supplied for <initenv name='%s'> element"), + name); + return -1; + } + + def->os.initenv[i] = g_new0(virDomainOSEnv, 1); + def->os.initenv[i]->name = name; + def->os.initenv[i]->value = g_strdup((const char *)nodes[i]->children->content); + } + def->os.initenv[n] = NULL; + + return 0; +} + + static int virDomainDefParseBootOptions(virDomainDefPtr def, xmlXPathContextPtr ctxt) { - char *name = NULL; - size_t i; int n; g_autofree xmlNodePtr *nodes = NULL; g_autofree char *tmp = NULL; @@ -19543,53 +19601,8 @@ virDomainDefParseBootOptions(virDomainDefPtr def, */ if (def->os.type == VIR_DOMAIN_OSTYPE_EXE) { - def->os.init = virXPathString("string(./os/init[1])", ctxt); - def->os.cmdline = virXPathString("string(./os/cmdline[1])", ctxt); - def->os.initdir = virXPathString("string(./os/initdir[1])", ctxt); - def->os.inituser = virXPathString("string(./os/inituser[1])", ctxt); - def->os.initgroup = virXPathString("string(./os/initgroup[1])", ctxt); - - if ((n = virXPathNodeSet("./os/initarg", ctxt, &nodes)) < 0) + if (virDomainDefParseBootInitOptions(def, ctxt) < 0) return -1; - - def->os.initargv = g_new0(char *, n+1); - for (i = 0; i < n; i++) { - if (!nodes[i]->children || - !nodes[i]->children->content) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("No data supplied for <initarg> element")); - return -1; - } - def->os.initargv[i] = g_strdup((const char *)nodes[i]->children->content); - } - def->os.initargv[n] = NULL; - VIR_FREE(nodes); - - if ((n = virXPathNodeSet("./os/initenv", ctxt, &nodes)) < 0) - return -1; - - def->os.initenv = g_new0(virDomainOSEnvPtr, n+1); - for (i = 0; i < n; i++) { - if (!(name = virXMLPropString(nodes[i], "name"))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("No name supplied for <initenv> element")); - return -1; - } - - if (!nodes[i]->children || - !nodes[i]->children->content) { - virReportError(VIR_ERR_XML_ERROR, - _("No value supplied for <initenv name='%s'> element"), - name); - return -1; - } - - def->os.initenv[i] = g_new0(virDomainOSEnv, 1); - def->os.initenv[i]->name = name; - def->os.initenv[i]->value = g_strdup((const char *)nodes[i]->children->content); - } - def->os.initenv[n] = NULL; - VIR_FREE(nodes); } if (def->os.type == VIR_DOMAIN_OSTYPE_XEN || -- 2.30.2
Extract the code to it's own function. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 31b908d8fe..4876fe61bb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19583,6 +19583,18 @@ virDomainDefParseBootInitOptions(virDomainDefPtr def, } +static void +virDomainDefParseBootKernelOptions(virDomainDefPtr def, + xmlXPathContextPtr ctxt) +{ + def->os.kernel = virXPathString("string(./os/kernel[1])", ctxt); + def->os.initrd = virXPathString("string(./os/initrd[1])", ctxt); + def->os.cmdline = virXPathString("string(./os/cmdline[1])", ctxt); + def->os.dtb = virXPathString("string(./os/dtb[1])", ctxt); + def->os.root = virXPathString("string(./os/root[1])", ctxt); +} + + static int virDomainDefParseBootOptions(virDomainDefPtr def, xmlXPathContextPtr ctxt) @@ -19612,11 +19624,7 @@ virDomainDefParseBootOptions(virDomainDefPtr def, g_autofree char *firmware = NULL; xmlNodePtr loader_node; - def->os.kernel = virXPathString("string(./os/kernel[1])", ctxt); - def->os.initrd = virXPathString("string(./os/initrd[1])", ctxt); - def->os.cmdline = virXPathString("string(./os/cmdline[1])", ctxt); - def->os.dtb = virXPathString("string(./os/dtb[1])", ctxt); - def->os.root = virXPathString("string(./os/root[1])", ctxt); + virDomainDefParseBootKernelOptions(def, ctxt); if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && (firmware = virXPathString("string(./os/@firmware)", ctxt))) { -- 2.30.2
Extract the code to it's own function. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4876fe61bb..03985b6687 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19595,6 +19595,31 @@ virDomainDefParseBootKernelOptions(virDomainDefPtr def, } +static int +virDomainDefParseBootFirmwareOptions(virDomainDefPtr def, + xmlXPathContextPtr ctxt) +{ + g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt); + int fw = 0; + + if (!firmware) + return 0; + + fw = virDomainOsDefFirmwareTypeFromString(firmware); + + if (fw <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown firmware value %s"), + firmware); + return -1; + } + + def->os.firmware = fw; + + return 0; +} + + static int virDomainDefParseBootOptions(virDomainDefPtr def, xmlXPathContextPtr ctxt) @@ -19621,23 +19646,13 @@ virDomainDefParseBootOptions(virDomainDefPtr def, def->os.type == VIR_DOMAIN_OSTYPE_XENPVH || def->os.type == VIR_DOMAIN_OSTYPE_HVM || def->os.type == VIR_DOMAIN_OSTYPE_UML) { - g_autofree char *firmware = NULL; xmlNodePtr loader_node; virDomainDefParseBootKernelOptions(def, ctxt); - if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && - (firmware = virXPathString("string(./os/@firmware)", ctxt))) { - int fw = virDomainOsDefFirmwareTypeFromString(firmware); - - if (fw <= 0) { - virReportError(VIR_ERR_XML_ERROR, - _("unknown firmware value %s"), - firmware); + if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { + if (virDomainDefParseBootFirmwareOptions(def, ctxt) < 0) return -1; - } - - def->os.firmware = fw; } if ((loader_node = virXPathNode("./os/loader[1]", ctxt))) { -- 2.30.2
Extract the code to it's own function. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 03985b6687..f6e8d5180a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19620,6 +19620,31 @@ virDomainDefParseBootFirmwareOptions(virDomainDefPtr def, } +static int +virDomainDefParseBootLoaderOptions(virDomainDefPtr def, + xmlXPathContextPtr ctxt) +{ + xmlNodePtr loader_node = virXPathNode("./os/loader[1]", ctxt); + const bool fwAutoSelect = def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE; + + if (!loader_node) + return 0; + + def->os.loader = g_new0(virDomainLoaderDef, 1); + + if (virDomainLoaderDefParseXML(loader_node, + def->os.loader, + fwAutoSelect) < 0) + return -1; + + def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt); + if (!fwAutoSelect) + def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt); + + return 0; +} + + static int virDomainDefParseBootOptions(virDomainDefPtr def, xmlXPathContextPtr ctxt) @@ -19646,7 +19671,6 @@ virDomainDefParseBootOptions(virDomainDefPtr def, def->os.type == VIR_DOMAIN_OSTYPE_XENPVH || def->os.type == VIR_DOMAIN_OSTYPE_HVM || def->os.type == VIR_DOMAIN_OSTYPE_UML) { - xmlNodePtr loader_node; virDomainDefParseBootKernelOptions(def, ctxt); @@ -19655,20 +19679,8 @@ virDomainDefParseBootOptions(virDomainDefPtr def, return -1; } - if ((loader_node = virXPathNode("./os/loader[1]", ctxt))) { - const bool fwAutoSelect = def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE; - - def->os.loader = g_new0(virDomainLoaderDef, 1); - - if (virDomainLoaderDefParseXML(loader_node, - def->os.loader, - fwAutoSelect) < 0) - return -1; - - def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt); - if (!fwAutoSelect) - def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt); - } + if (virDomainDefParseBootLoaderOptions(def, ctxt) < 0) + return -1; } if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { -- 2.30.2
Extract the code to it's own function. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 79 ++++++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 34 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f6e8d5180a..14a2c818d6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19645,14 +19645,54 @@ virDomainDefParseBootLoaderOptions(virDomainDefPtr def, } +static int +virDomainDefParseBootAcpiOptions(virDomainDefPtr def, + xmlXPathContextPtr ctxt) +{ + int n; + g_autofree xmlNodePtr *nodes = NULL; + g_autofree char *tmp = NULL; + + if ((n = virXPathNodeSet("./os/acpi/table", ctxt, &nodes)) < 0) + return -1; + + if (n > 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Only one acpi table is supported")); + return -1; + } + + if (n == 1) { + tmp = virXMLPropString(nodes[0], "type"); + + if (!tmp) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing acpi table type")); + return -1; + } + + if (STREQ_NULLABLE(tmp, "slic")) { + VIR_FREE(tmp); + if (!(tmp = virXMLNodeContentString(nodes[0]))) + return -1; + + def->os.slic_table = virFileSanitizePath(tmp); + } else { + virReportError(VIR_ERR_XML_ERROR, + _("Unknown acpi table type: %s"), + tmp); + return -1; + } + } + + return 0; +} + + static int virDomainDefParseBootOptions(virDomainDefPtr def, xmlXPathContextPtr ctxt) { - int n; - g_autofree xmlNodePtr *nodes = NULL; - g_autofree char *tmp = NULL; - /* * Booting options for different OS types.... * @@ -19684,38 +19724,9 @@ virDomainDefParseBootOptions(virDomainDefPtr def, } if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { - if ((n = virXPathNodeSet("./os/acpi/table", ctxt, &nodes)) < 0) + if (virDomainDefParseBootAcpiOptions(def, ctxt) < 0) return -1; - if (n > 1) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Only one acpi table is supported")); - return -1; - } - - if (n == 1) { - tmp = virXMLPropString(nodes[0], "type"); - - if (!tmp) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Missing acpi table type")); - return -1; - } - - if (STREQ_NULLABLE(tmp, "slic")) { - VIR_FREE(tmp); - if (!(tmp = virXMLNodeContentString(nodes[0]))) - return -1; - - def->os.slic_table = virFileSanitizePath(tmp); - } else { - virReportError(VIR_ERR_XML_ERROR, - _("Unknown acpi table type: %s"), - tmp); - return -1; - } - } - if (virDomainDefParseBootXML(ctxt, def) < 0) return -1; } -- 2.30.2
The original code used a lot of conditions and was not that obvious when each XML bits are parsed. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 14a2c818d6..7729333897 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19702,33 +19702,43 @@ virDomainDefParseBootOptions(virDomainDefPtr def, * - An init script (exe) */ - if (def->os.type == VIR_DOMAIN_OSTYPE_EXE) { - if (virDomainDefParseBootInitOptions(def, ctxt) < 0) - return -1; - } - - if (def->os.type == VIR_DOMAIN_OSTYPE_XEN || - def->os.type == VIR_DOMAIN_OSTYPE_XENPVH || - def->os.type == VIR_DOMAIN_OSTYPE_HVM || - def->os.type == VIR_DOMAIN_OSTYPE_UML) { - + switch ((virDomainOSType) def->os.type) { + case VIR_DOMAIN_OSTYPE_HVM: virDomainDefParseBootKernelOptions(def, ctxt); - if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { - if (virDomainDefParseBootFirmwareOptions(def, ctxt) < 0) - return -1; - } + if (virDomainDefParseBootFirmwareOptions(def, ctxt) < 0) + return -1; if (virDomainDefParseBootLoaderOptions(def, ctxt) < 0) return -1; - } - if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { if (virDomainDefParseBootAcpiOptions(def, ctxt) < 0) return -1; if (virDomainDefParseBootXML(ctxt, def) < 0) return -1; + + break; + + case VIR_DOMAIN_OSTYPE_XEN: + case VIR_DOMAIN_OSTYPE_XENPVH: + case VIR_DOMAIN_OSTYPE_UML: + virDomainDefParseBootKernelOptions(def, ctxt); + + if (virDomainDefParseBootLoaderOptions(def, ctxt) < 0) + return -1; + + break; + + case VIR_DOMAIN_OSTYPE_EXE: + if (virDomainDefParseBootInitOptions(def, ctxt) < 0) + return -1; + + break; + + case VIR_DOMAIN_OSTYPE_LINUX: + case VIR_DOMAIN_OSTYPE_LAST: + break; } return 0; -- 2.30.2
When the firmware auto-selection was introduced it always picked first usable firmware based on the JSON descriptions on the host. It is possible to add/remove/change the JSON files but it will always be for the whole host. This patch introduces support for configuring the auto-selection per VM by adding users an option to limit what features they would like to have available in the firmware. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.rst | 31 +++++++ docs/schemas/domaincommon.rng | 23 +++++ src/conf/domain_conf.c | 83 ++++++++++++++++++- src/conf/domain_conf.h | 10 +++ .../os-firmware-efi-invalid-type.xml | 28 +++++++ ...os-firmware-invalid-type.x86_64-latest.err | 1 + .../os-firmware-invalid-type.xml | 28 +++++++ tests/qemuxml2argvtest.c | 1 + ...aarch64-os-firmware-efi.aarch64-latest.xml | 1 + .../os-firmware-bios.x86_64-latest.xml | 1 + .../os-firmware-efi-secboot.x86_64-latest.xml | 1 + .../os-firmware-efi.x86_64-latest.xml | 1 + tests/vmx2xmldata/vmx2xml-firmware-efi.xml | 1 + 13 files changed, 207 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-invalid-type.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.xml diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index c101d5a1f1..dd063b0794 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -155,6 +155,37 @@ harddisk, cdrom, network) determining where to obtain/find the boot image. the host native arch will be chosen. For the ``test``, ``ESX`` and ``VMWare`` hypervisor drivers, however, the ``i686`` arch will always be chosen even on an ``x86_64`` host. :since:`Since 0.0.1` +``firmware`` + :since:`Since 7.2.0 QEMU/KVM only` + + When used together with ``firmware`` attribute of ``os`` element the ``type`` + attribute must have the same value. + + List of mandatory attributes: + + - ``type`` (accepted values are ``bios`` and ``efi``) same as the ``firmware`` + attribute of ``os`` element. + + When using firmware auto-selection there are different features enabled in + the firmwares. The list of features can be used to limit what firmware should + be automatically selected for the VM. The list of features can be specified + using zero or more ``feature`` elements. Libvirt will take into consideration + only the listed features and ignore the rest when selecting the firmware. + + ``feature`` + The list of mandatory attributes: + + - ``enabled`` (accepted values are ``yes`` and ``no``) is used to tell libvirt + if the feature must be enabled or not in the automatically selected firmware + + - ``name`` the name of the feature, the list of the features: + + - ``enrolled-keys`` whether the selected nvram template has default + certificate enrolled. Firmware with Secure Boot feature but without + enrolled keys will successfully boot non-signed binaries as well. + Valid only for firmwares with Secure Boot feature. + + - ``secure-boot`` whether the firmware implements UEFI Secure boot feature. ``loader`` The optional ``loader`` tag refers to a firmware blob, which is specified by absolute path, used to assist the domain creation process. It is used by Xen diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e6db2f5b74..1dbfc68f18 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -276,6 +276,29 @@ </attribute> </optional> <ref name="ostypehvm"/> + <optional> + <element name="firmware"> + <attribute name="type"> + <choice> + <value>bios</value> + <value>efi</value> + </choice> + </attribute> + <zeroOrMore> + <element name="feature"> + <attribute name="enabled"> + <ref name="virYesNo"/> + </attribute> + <attribute name="name"> + <choice> + <value>enrolled-keys</value> + <value>secure-boot</value> + </choice> + </attribute> + </element> + </zeroOrMore> + </element> + </optional> <optional> <element name="loader"> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7729333897..dcfe5c0d03 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1318,6 +1318,12 @@ VIR_ENUM_IMPL(virDomainOsDefFirmware, "efi", ); +VIR_ENUM_IMPL(virDomainOsDefFirmwareFeature, + VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_LAST, + "enrolled-keys", + "secure-boot", +); + VIR_ENUM_IMPL(virDomainCFPC, VIR_DOMAIN_CFPC_LAST, "none", @@ -19600,22 +19606,67 @@ virDomainDefParseBootFirmwareOptions(virDomainDefPtr def, xmlXPathContextPtr ctxt) { g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt); + g_autofree char *type = virXPathString("string(./os/firmware/@type)", ctxt); + g_autofree xmlNodePtr *nodes = NULL; + g_autofree int *features = NULL; int fw = 0; + int n = 0; + size_t i; - if (!firmware) + if (!firmware && !type) return 0; - fw = virDomainOsDefFirmwareTypeFromString(firmware); + if (firmware && type && STRNEQ(firmware, type)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("firmware attribute and firmware type has to be the same")); + return -1; + } + + if (!type) + type = g_steal_pointer(&firmware); + + fw = virDomainOsDefFirmwareTypeFromString(type); if (fw <= 0) { virReportError(VIR_ERR_XML_ERROR, _("unknown firmware value %s"), - firmware); + type); return -1; } def->os.firmware = fw; + if ((n = virXPathNodeSet("./os/firmware/feature", ctxt, &nodes)) < 0) + return -1; + + if (n > 0) + features = g_new0(int, VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_LAST); + + for (i = 0; i < n; i++) { + g_autofree char *name = virXMLPropString(nodes[i], "name"); + g_autofree char *enabled = virXMLPropString(nodes[i], "enabled"); + int feature = virDomainOsDefFirmwareFeatureTypeFromString(name); + int val = virTristateBoolTypeFromString(enabled); + + if (feature < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid firmware feature name '%s'"), + name); + return -1; + } + + if (val < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid firmware feature enabled value '%s'"), + enabled); + return -1; + } + + features[feature] = val; + } + + def->os.firmwareFeatures = g_steal_pointer(&features); + return 0; } @@ -29455,6 +29506,32 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, virBufferAsprintf(buf, ">%s</type>\n", virDomainOSTypeToString(def->os.type)); + if (def->os.firmware) { + virBufferAsprintf(buf, "<firmware type='%s'", + virDomainOsDefFirmwareTypeToString(def->os.firmware)); + + if (def->os.firmwareFeatures) { + virBufferAddLit(buf, ">\n"); + + virBufferAdjustIndent(buf, 2); + + for (i = 0; i < VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_LAST; i++) { + if (def->os.firmwareFeatures[i] == VIR_TRISTATE_BOOL_ABSENT) + continue; + + virBufferAsprintf(buf, "<feature enabled='%s' name='%s'/>\n", + virTristateBoolTypeToString(def->os.firmwareFeatures[i]), + virDomainOsDefFirmwareFeatureTypeToString(i)); + } + + virBufferAdjustIndent(buf, -2); + + virBufferAddLit(buf, "</firmware>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } + } + virBufferEscapeString(buf, "<init>%s</init>\n", def->os.init); for (i = 0; def->os.initargv && def->os.initargv[i]; i++) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 54a631853b..87bc7e8625 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2235,9 +2235,19 @@ G_STATIC_ASSERT((int)VIR_DOMAIN_OS_DEF_FIRMWARE_LAST == (int)VIR_DOMAIN_LOADER_T VIR_ENUM_DECL(virDomainOsDefFirmware); +typedef enum { + VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_ENROLLED_KEYS, + VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_SECURE_BOOT, + + VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_LAST +} virDomainOsDefFirmwareFeature; + +VIR_ENUM_DECL(virDomainOsDefFirmwareFeature); + struct _virDomainOSDef { int type; virDomainOsDefFirmware firmware; + int *firmwareFeatures; virArch arch; char *machine; size_t nBootDevs; diff --git a/tests/qemuxml2argvdata/os-firmware-efi-invalid-type.xml b/tests/qemuxml2argvdata/os-firmware-efi-invalid-type.xml new file mode 100644 index 0000000000..41360df0f7 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-invalid-type.xml @@ -0,0 +1,28 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <firmware type='bios'/> + <loader secure='no'/> + <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/os-firmware-invalid-type.x86_64-latest.err b/tests/qemuxml2argvdata/os-firmware-invalid-type.x86_64-latest.err new file mode 100644 index 0000000000..c8174b1c8b --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-invalid-type.x86_64-latest.err @@ -0,0 +1 @@ +unsupported configuration: firmware attribute and firmware type has to be the same diff --git a/tests/qemuxml2argvdata/os-firmware-invalid-type.xml b/tests/qemuxml2argvdata/os-firmware-invalid-type.xml new file mode 100644 index 0000000000..41360df0f7 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-invalid-type.xml @@ -0,0 +1,28 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <firmware type='bios'/> + <loader secure='no'/> + <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 29054ba168..2b32b7f303 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3549,6 +3549,7 @@ mymain(void) DO_TEST_CAPS_LATEST("os-firmware-bios"); DO_TEST_CAPS_LATEST("os-firmware-efi"); DO_TEST_CAPS_LATEST("os-firmware-efi-secboot"); + DO_TEST_CAPS_LATEST_PARSE_ERROR("os-firmware-invalid-type"); DO_TEST_CAPS_ARCH_LATEST("aarch64-os-firmware-efi", "aarch64"); DO_TEST_CAPS_LATEST("vhost-user-vga"); diff --git a/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml index 627e285ae1..cb4f3ccfce 100644 --- a/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml +++ b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml @@ -6,6 +6,7 @@ <vcpu placement='static'>1</vcpu> <os firmware='efi'> <type arch='aarch64' machine='virt-4.0'>hvm</type> + <firmware type='efi'/> <kernel>/aarch64.kernel</kernel> <initrd>/aarch64.initrd</initrd> <cmdline>earlyprintk console=ttyAMA0,115200n8 rw root=/dev/vda rootwait</cmdline> diff --git a/tests/qemuxml2xmloutdata/os-firmware-bios.x86_64-latest.xml b/tests/qemuxml2xmloutdata/os-firmware-bios.x86_64-latest.xml index df6f61421a..016c5b863f 100644 --- a/tests/qemuxml2xmloutdata/os-firmware-bios.x86_64-latest.xml +++ b/tests/qemuxml2xmloutdata/os-firmware-bios.x86_64-latest.xml @@ -6,6 +6,7 @@ <vcpu placement='static'>1</vcpu> <os firmware='bios'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <firmware type='bios'/> <loader secure='no'/> <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> <boot dev='hd'/> diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.x86_64-latest.xml b/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.x86_64-latest.xml index c383546cc6..fa5eaa3148 100644 --- a/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.x86_64-latest.xml +++ b/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.x86_64-latest.xml @@ -6,6 +6,7 @@ <vcpu placement='static'>1</vcpu> <os firmware='efi'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <firmware type='efi'/> <loader secure='yes'/> <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> <boot dev='hd'/> diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi.x86_64-latest.xml b/tests/qemuxml2xmloutdata/os-firmware-efi.x86_64-latest.xml index 04d57860e7..382146c23b 100644 --- a/tests/qemuxml2xmloutdata/os-firmware-efi.x86_64-latest.xml +++ b/tests/qemuxml2xmloutdata/os-firmware-efi.x86_64-latest.xml @@ -6,6 +6,7 @@ <vcpu placement='static'>1</vcpu> <os firmware='efi'> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <firmware type='efi'/> <loader secure='no'/> <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> <boot dev='hd'/> diff --git a/tests/vmx2xmldata/vmx2xml-firmware-efi.xml b/tests/vmx2xmldata/vmx2xml-firmware-efi.xml index fee707fe71..fa10daf3a6 100644 --- a/tests/vmx2xmldata/vmx2xml-firmware-efi.xml +++ b/tests/vmx2xmldata/vmx2xml-firmware-efi.xml @@ -6,6 +6,7 @@ <vcpu placement='static'>1</vcpu> <os firmware='efi'> <type arch='i686'>hvm</type> + <firmware type='efi'/> </os> <clock offset='utc'/> <on_poweroff>destroy</on_poweroff> -- 2.30.2
On 3/18/21 1:26 PM, Pavel Hrdina wrote:
When the firmware auto-selection was introduced it always picked first usable firmware based on the JSON descriptions on the host. It is possible to add/remove/change the JSON files but it will always be for the whole host.
This patch introduces support for configuring the auto-selection per VM by adding users an option to limit what features they would like to have available in the firmware.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.rst | 31 +++++++ docs/schemas/domaincommon.rng | 23 +++++ src/conf/domain_conf.c | 83 ++++++++++++++++++- src/conf/domain_conf.h | 10 +++ .../os-firmware-efi-invalid-type.xml | 28 +++++++ ...os-firmware-invalid-type.x86_64-latest.err | 1 + .../os-firmware-invalid-type.xml | 28 +++++++ tests/qemuxml2argvtest.c | 1 + ...aarch64-os-firmware-efi.aarch64-latest.xml | 1 + .../os-firmware-bios.x86_64-latest.xml | 1 + .../os-firmware-efi-secboot.x86_64-latest.xml | 1 + .../os-firmware-efi.x86_64-latest.xml | 1 + tests/vmx2xmldata/vmx2xml-firmware-efi.xml | 1 + 13 files changed, 207 insertions(+), 3 deletions(-)
create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-invalid-type.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.xml
These two are identical. Have you intended them to be different?
create mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.x86_64-latest.err
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index c101d5a1f1..dd063b0794 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -155,6 +155,37 @@ harddisk, cdrom, network) determining where to obtain/find the boot image. the host native arch will be chosen. For the ``test``, ``ESX`` and ``VMWare`` hypervisor drivers, however, the ``i686`` arch will always be chosen even on an ``x86_64`` host. :since:`Since 0.0.1` +``firmware`` + :since:`Since 7.2.0 QEMU/KVM only` + + When used together with ``firmware`` attribute of ``os`` element the ``type`` + attribute must have the same value. + + List of mandatory attributes: + + - ``type`` (accepted values are ``bios`` and ``efi``) same as the ``firmware`` + attribute of ``os`` element. + + When using firmware auto-selection there are different features enabled in + the firmwares. The list of features can be used to limit what firmware should + be automatically selected for the VM. The list of features can be specified + using zero or more ``feature`` elements. Libvirt will take into consideration + only the listed features and ignore the rest when selecting the firmware. + + ``feature`` + The list of mandatory attributes: + + - ``enabled`` (accepted values are ``yes`` and ``no``) is used to tell libvirt + if the feature must be enabled or not in the automatically selected firmware + + - ``name`` the name of the feature, the list of the features: + + - ``enrolled-keys`` whether the selected nvram template has default + certificate enrolled. Firmware with Secure Boot feature but without + enrolled keys will successfully boot non-signed binaries as well. + Valid only for firmwares with Secure Boot feature. + + - ``secure-boot`` whether the firmware implements UEFI Secure boot feature. ``loader`` The optional ``loader`` tag refers to a firmware blob, which is specified by absolute path, used to assist the domain creation process. It is used by Xen diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e6db2f5b74..1dbfc68f18 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -276,6 +276,29 @@ </attribute> </optional> <ref name="ostypehvm"/> + <optional> + <element name="firmware"> + <attribute name="type"> + <choice> + <value>bios</value> + <value>efi</value> + </choice> + </attribute> + <zeroOrMore> + <element name="feature"> + <attribute name="enabled"> + <ref name="virYesNo"/> + </attribute> + <attribute name="name"> + <choice> + <value>enrolled-keys</value> + <value>secure-boot</value> + </choice> + </attribute> + </element> + </zeroOrMore> + </element> + </optional> <optional> <element name="loader"> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7729333897..dcfe5c0d03 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1318,6 +1318,12 @@ VIR_ENUM_IMPL(virDomainOsDefFirmware, "efi", );
+VIR_ENUM_IMPL(virDomainOsDefFirmwareFeature, + VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_LAST, + "enrolled-keys", + "secure-boot", +); + VIR_ENUM_IMPL(virDomainCFPC, VIR_DOMAIN_CFPC_LAST, "none", @@ -19600,22 +19606,67 @@ virDomainDefParseBootFirmwareOptions(virDomainDefPtr def, xmlXPathContextPtr ctxt) { g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt); + g_autofree char *type = virXPathString("string(./os/firmware/@type)", ctxt); + g_autofree xmlNodePtr *nodes = NULL; + g_autofree int *features = NULL; int fw = 0; + int n = 0; + size_t i;
- if (!firmware) + if (!firmware && !type) return 0;
- fw = virDomainOsDefFirmwareTypeFromString(firmware); + if (firmware && type && STRNEQ(firmware, type)) {
Or STRNEQ_NULLABLE()
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("firmware attribute and firmware type has to be the same")); + return -1; + } + + if (!type) + type = g_steal_pointer(&firmware); + + fw = virDomainOsDefFirmwareTypeFromString(type);
if (fw <= 0) { virReportError(VIR_ERR_XML_ERROR, _("unknown firmware value %s"), - firmware); + type); return -1; }
def->os.firmware = fw;
+ if ((n = virXPathNodeSet("./os/firmware/feature", ctxt, &nodes)) < 0) + return -1; + + if (n > 0) + features = g_new0(int, VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_LAST); + + for (i = 0; i < n; i++) { + g_autofree char *name = virXMLPropString(nodes[i], "name"); + g_autofree char *enabled = virXMLPropString(nodes[i], "enabled"); + int feature = virDomainOsDefFirmwareFeatureTypeFromString(name); + int val = virTristateBoolTypeFromString(enabled); + + if (feature < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid firmware feature name '%s'"), + name); + return -1; + } + + if (val < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid firmware feature enabled value '%s'"), + enabled); + return -1; + } + + features[feature] = val; + } + + def->os.firmwareFeatures = g_steal_pointer(&features); + return 0; }
Michal
On Thu, Mar 18, 2021 at 05:18:36PM +0100, Michal Privoznik wrote:
On 3/18/21 1:26 PM, Pavel Hrdina wrote:
When the firmware auto-selection was introduced it always picked first usable firmware based on the JSON descriptions on the host. It is possible to add/remove/change the JSON files but it will always be for the whole host.
This patch introduces support for configuring the auto-selection per VM by adding users an option to limit what features they would like to have available in the firmware.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.rst | 31 +++++++ docs/schemas/domaincommon.rng | 23 +++++ src/conf/domain_conf.c | 83 ++++++++++++++++++- src/conf/domain_conf.h | 10 +++ .../os-firmware-efi-invalid-type.xml | 28 +++++++ ...os-firmware-invalid-type.x86_64-latest.err | 1 + .../os-firmware-invalid-type.xml | 28 +++++++ tests/qemuxml2argvtest.c | 1 + ...aarch64-os-firmware-efi.aarch64-latest.xml | 1 + .../os-firmware-bios.x86_64-latest.xml | 1 + .../os-firmware-efi-secboot.x86_64-latest.xml | 1 + .../os-firmware-efi.x86_64-latest.xml | 1 + tests/vmx2xmldata/vmx2xml-firmware-efi.xml | 1 + 13 files changed, 207 insertions(+), 3 deletions(-)
create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-invalid-type.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.xml
These two are identical. Have you intended them to be different?
Nice catch, the first one is leftover after rename, I'll drop it.
create mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.x86_64-latest.err
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index c101d5a1f1..dd063b0794 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -155,6 +155,37 @@ harddisk, cdrom, network) determining where to obtain/find the boot image. the host native arch will be chosen. For the ``test``, ``ESX`` and ``VMWare`` hypervisor drivers, however, the ``i686`` arch will always be chosen even on an ``x86_64`` host. :since:`Since 0.0.1` +``firmware`` + :since:`Since 7.2.0 QEMU/KVM only` + + When used together with ``firmware`` attribute of ``os`` element the ``type`` + attribute must have the same value. + + List of mandatory attributes: + + - ``type`` (accepted values are ``bios`` and ``efi``) same as the ``firmware`` + attribute of ``os`` element. + + When using firmware auto-selection there are different features enabled in + the firmwares. The list of features can be used to limit what firmware should + be automatically selected for the VM. The list of features can be specified + using zero or more ``feature`` elements. Libvirt will take into consideration + only the listed features and ignore the rest when selecting the firmware. + + ``feature`` + The list of mandatory attributes: + + - ``enabled`` (accepted values are ``yes`` and ``no``) is used to tell libvirt + if the feature must be enabled or not in the automatically selected firmware + + - ``name`` the name of the feature, the list of the features: + + - ``enrolled-keys`` whether the selected nvram template has default + certificate enrolled. Firmware with Secure Boot feature but without + enrolled keys will successfully boot non-signed binaries as well. + Valid only for firmwares with Secure Boot feature. + + - ``secure-boot`` whether the firmware implements UEFI Secure boot feature. ``loader`` The optional ``loader`` tag refers to a firmware blob, which is specified by absolute path, used to assist the domain creation process. It is used by Xen diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e6db2f5b74..1dbfc68f18 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -276,6 +276,29 @@ </attribute> </optional> <ref name="ostypehvm"/> + <optional> + <element name="firmware"> + <attribute name="type"> + <choice> + <value>bios</value> + <value>efi</value> + </choice> + </attribute> + <zeroOrMore> + <element name="feature"> + <attribute name="enabled"> + <ref name="virYesNo"/> + </attribute> + <attribute name="name"> + <choice> + <value>enrolled-keys</value> + <value>secure-boot</value> + </choice> + </attribute> + </element> + </zeroOrMore> + </element> + </optional> <optional> <element name="loader"> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7729333897..dcfe5c0d03 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1318,6 +1318,12 @@ VIR_ENUM_IMPL(virDomainOsDefFirmware, "efi", ); +VIR_ENUM_IMPL(virDomainOsDefFirmwareFeature, + VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_LAST, + "enrolled-keys", + "secure-boot", +); + VIR_ENUM_IMPL(virDomainCFPC, VIR_DOMAIN_CFPC_LAST, "none", @@ -19600,22 +19606,67 @@ virDomainDefParseBootFirmwareOptions(virDomainDefPtr def, xmlXPathContextPtr ctxt) { g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt); + g_autofree char *type = virXPathString("string(./os/firmware/@type)", ctxt); + g_autofree xmlNodePtr *nodes = NULL; + g_autofree int *features = NULL; int fw = 0; + int n = 0; + size_t i; - if (!firmware) + if (!firmware && !type) return 0; - fw = virDomainOsDefFirmwareTypeFromString(firmware); + if (firmware && type && STRNEQ(firmware, type)) {
Or STRNEQ_NULLABLE()
Right, I'll change it before pushing. Thanks, Pavel
On Thu, Mar 18, 2021 at 06:10:45PM +0100, Pavel Hrdina wrote:
On Thu, Mar 18, 2021 at 05:18:36PM +0100, Michal Privoznik wrote:
On 3/18/21 1:26 PM, Pavel Hrdina wrote:
When the firmware auto-selection was introduced it always picked first usable firmware based on the JSON descriptions on the host. It is possible to add/remove/change the JSON files but it will always be for the whole host.
This patch introduces support for configuring the auto-selection per VM by adding users an option to limit what features they would like to have available in the firmware.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.rst | 31 +++++++ docs/schemas/domaincommon.rng | 23 +++++ src/conf/domain_conf.c | 83 ++++++++++++++++++- src/conf/domain_conf.h | 10 +++ .../os-firmware-efi-invalid-type.xml | 28 +++++++ ...os-firmware-invalid-type.x86_64-latest.err | 1 + .../os-firmware-invalid-type.xml | 28 +++++++ tests/qemuxml2argvtest.c | 1 + ...aarch64-os-firmware-efi.aarch64-latest.xml | 1 + .../os-firmware-bios.x86_64-latest.xml | 1 + .../os-firmware-efi-secboot.x86_64-latest.xml | 1 + .../os-firmware-efi.x86_64-latest.xml | 1 + tests/vmx2xmldata/vmx2xml-firmware-efi.xml | 1 + 13 files changed, 207 insertions(+), 3 deletions(-)
create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-invalid-type.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.xml
These two are identical. Have you intended them to be different?
Nice catch, the first one is leftover after rename, I'll drop it.
create mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.x86_64-latest.err
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index c101d5a1f1..dd063b0794 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -155,6 +155,37 @@ harddisk, cdrom, network) determining where to obtain/find the boot image. the host native arch will be chosen. For the ``test``, ``ESX`` and ``VMWare`` hypervisor drivers, however, the ``i686`` arch will always be chosen even on an ``x86_64`` host. :since:`Since 0.0.1` +``firmware`` + :since:`Since 7.2.0 QEMU/KVM only` + + When used together with ``firmware`` attribute of ``os`` element the ``type`` + attribute must have the same value. + + List of mandatory attributes: + + - ``type`` (accepted values are ``bios`` and ``efi``) same as the ``firmware`` + attribute of ``os`` element. + + When using firmware auto-selection there are different features enabled in + the firmwares. The list of features can be used to limit what firmware should + be automatically selected for the VM. The list of features can be specified + using zero or more ``feature`` elements. Libvirt will take into consideration + only the listed features and ignore the rest when selecting the firmware. + + ``feature`` + The list of mandatory attributes: + + - ``enabled`` (accepted values are ``yes`` and ``no``) is used to tell libvirt + if the feature must be enabled or not in the automatically selected firmware + + - ``name`` the name of the feature, the list of the features: + + - ``enrolled-keys`` whether the selected nvram template has default + certificate enrolled. Firmware with Secure Boot feature but without + enrolled keys will successfully boot non-signed binaries as well. + Valid only for firmwares with Secure Boot feature. + + - ``secure-boot`` whether the firmware implements UEFI Secure boot feature. ``loader`` The optional ``loader`` tag refers to a firmware blob, which is specified by absolute path, used to assist the domain creation process. It is used by Xen diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e6db2f5b74..1dbfc68f18 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -276,6 +276,29 @@ </attribute> </optional> <ref name="ostypehvm"/> + <optional> + <element name="firmware"> + <attribute name="type"> + <choice> + <value>bios</value> + <value>efi</value> + </choice> + </attribute> + <zeroOrMore> + <element name="feature"> + <attribute name="enabled"> + <ref name="virYesNo"/> + </attribute> + <attribute name="name"> + <choice> + <value>enrolled-keys</value> + <value>secure-boot</value> + </choice> + </attribute> + </element> + </zeroOrMore> + </element> + </optional> <optional> <element name="loader"> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7729333897..dcfe5c0d03 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1318,6 +1318,12 @@ VIR_ENUM_IMPL(virDomainOsDefFirmware, "efi", ); +VIR_ENUM_IMPL(virDomainOsDefFirmwareFeature, + VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_LAST, + "enrolled-keys", + "secure-boot", +); + VIR_ENUM_IMPL(virDomainCFPC, VIR_DOMAIN_CFPC_LAST, "none", @@ -19600,22 +19606,67 @@ virDomainDefParseBootFirmwareOptions(virDomainDefPtr def, xmlXPathContextPtr ctxt) { g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt); + g_autofree char *type = virXPathString("string(./os/firmware/@type)", ctxt); + g_autofree xmlNodePtr *nodes = NULL; + g_autofree int *features = NULL; int fw = 0; + int n = 0; + size_t i; - if (!firmware) + if (!firmware && !type) return 0; - fw = virDomainOsDefFirmwareTypeFromString(firmware); + if (firmware && type && STRNEQ(firmware, type)) {
Or STRNEQ_NULLABLE()
Right, I'll change it before pushing.
Actually no, this will not work correctly. One of them can be NULL, in that case there is no issue and it will be parsed correctly. The point of this check is to complain only if both are specified and they are different. So I'll keep the original code. Pavel
On Thu, Mar 18, 2021 at 01:26:45PM +0100, Pavel Hrdina wrote:
When the firmware auto-selection was introduced it always picked first usable firmware based on the JSON descriptions on the host. It is possible to add/remove/change the JSON files but it will always be for the whole host.
This patch introduces support for configuring the auto-selection per VM by adding users an option to limit what features they would like to have available in the firmware.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> ---
[...]
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index c101d5a1f1..dd063b0794 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -155,6 +155,37 @@ harddisk, cdrom, network) determining where to obtain/find the boot image. the host native arch will be chosen. For the ``test``, ``ESX`` and ``VMWare`` hypervisor drivers, however, the ``i686`` arch will always be chosen even on an ``x86_64`` host. :since:`Since 0.0.1` +``firmware`` + :since:`Since 7.2.0 QEMU/KVM only` + + When used together with ``firmware`` attribute of ``os`` element the ``type`` + attribute must have the same value. + + List of mandatory attributes: + + - ``type`` (accepted values are ``bios`` and ``efi``) same as the ``firmware`` + attribute of ``os`` element. + + When using firmware auto-selection there are different features enabled in + the firmwares.
Nit: I'd recast it as: "When using firmware auto-selection, different features are enabled in any given firmware binary."
+ The list of features can be used to limit what firmware should + be automatically selected for the VM. The list of features can be specified + using zero or more ``feature`` elements. Libvirt will take into consideration + only the listed features and ignore the rest when selecting the firmware. + + ``feature``
Should we also list a couple of example features? E.g. "amd-sev" (on supported hardware), "acpi-s3", "secure-boot". I notice that "enrolled-keys" is the only feature advertized by one of the firmware descriptor files (40-edk2-ovmf-x64-sb-enrolled.json), which is used as a mandatory XML attribute for the ``feature`` element. I'm silently wondering there's a possibility for confusion: "Hmm, 'enrolled-keys' is a possible feature, but it is a mandatory attribute of ``feature`` element."
+ The list of mandatory attributes: + + - ``enabled`` (accepted values are ``yes`` and ``no``) is used to tell libvirt + if the feature must be enabled or not in the automatically selected firmware + + - ``name`` the name of the feature, the list of the features: + + - ``enrolled-keys`` whether the selected nvram template has default
Nit: s/nvram/NVRAM/ (Or ``nvram``, to use rST verbatim; as it's an XML element.)
+ certificate enrolled. Firmware with Secure Boot feature but without + enrolled keys will successfully boot non-signed binaries as well. + Valid only for firmwares with Secure Boot feature.
Nice; it spells out the granular possibilities.
+ - ``secure-boot`` whether the firmware implements UEFI Secure boot feature. ``loader``
Here too (to match with the other formatdomain.rst): "whether the firmware is _capable_ of UEFI Secure Boot feature." I'm not qualified to comment on the rest of the patch, as I don't dwell on this code; but, FWIW: Acked-by: Kashyap Chamarthy <kchamart@redhat.com> [...] -- /kashyap
On Fri, Mar 19, 2021 at 11:10:05AM +0100, Kashyap Chamarthy wrote:
On Thu, Mar 18, 2021 at 01:26:45PM +0100, Pavel Hrdina wrote:
When the firmware auto-selection was introduced it always picked first usable firmware based on the JSON descriptions on the host. It is possible to add/remove/change the JSON files but it will always be for the whole host.
This patch introduces support for configuring the auto-selection per VM by adding users an option to limit what features they would like to have available in the firmware.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> ---
[...]
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index c101d5a1f1..dd063b0794 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -155,6 +155,37 @@ harddisk, cdrom, network) determining where to obtain/find the boot image. the host native arch will be chosen. For the ``test``, ``ESX`` and ``VMWare`` hypervisor drivers, however, the ``i686`` arch will always be chosen even on an ``x86_64`` host. :since:`Since 0.0.1` +``firmware`` + :since:`Since 7.2.0 QEMU/KVM only` + + When used together with ``firmware`` attribute of ``os`` element the ``type`` + attribute must have the same value. + + List of mandatory attributes: + + - ``type`` (accepted values are ``bios`` and ``efi``) same as the ``firmware`` + attribute of ``os`` element. + + When using firmware auto-selection there are different features enabled in + the firmwares.
Nit: I'd recast it as: "When using firmware auto-selection, different features are enabled in any given firmware binary."
Sounds a bit better but I've already pushed the patches.
+ The list of features can be used to limit what firmware should + be automatically selected for the VM. The list of features can be specified + using zero or more ``feature`` elements. Libvirt will take into consideration + only the listed features and ignore the rest when selecting the firmware. + + ``feature``
Should we also list a couple of example features? E.g. "amd-sev" (on supported hardware), "acpi-s3", "secure-boot".
I was considering listing all features that the JSON files can have but most of the other features are already controlled by different XML elements. There is an explicit list of features later in the docs.
I notice that "enrolled-keys" is the only feature advertized by one of the firmware descriptor files (40-edk2-ovmf-x64-sb-enrolled.json), which is used as a mandatory XML attribute for the ``feature`` element.
I'm silently wondering there's a possibility for confusion: "Hmm, 'enrolled-keys' is a possible feature, but it is a mandatory attribute of ``feature`` element."
I don't understand what you mean here. If you are talking about our XML and the following text here in this patch then if you use the feature element both attributes ``enabled`` and ``name`` are mandatory. But few lines above there is a statement that the you can specify zero or more ``feature`` elements so you don't have to limit any features. Our documentation doesn't state that 'enrolled-keys' is possible feature and mandatory attribute at the same time.
+ The list of mandatory attributes: + + - ``enabled`` (accepted values are ``yes`` and ``no``) is used to tell libvirt + if the feature must be enabled or not in the automatically selected firmware + + - ``name`` the name of the feature, the list of the features: + + - ``enrolled-keys`` whether the selected nvram template has default
Nit: s/nvram/NVRAM/ (Or ``nvram``, to use rST verbatim; as it's an XML element.)
Using ``nvram`` would have been probably better.
+ certificate enrolled. Firmware with Secure Boot feature but without + enrolled keys will successfully boot non-signed binaries as well. + Valid only for firmwares with Secure Boot feature.
Nice; it spells out the granular possibilities.
+ - ``secure-boot`` whether the firmware implements UEFI Secure boot feature. ``loader``
Here too (to match with the other formatdomain.rst): "whether the firmware is _capable_ of UEFI Secure Boot feature."
I'm not qualified to comment on the rest of the patch, as I don't dwell on this code; but, FWIW:
Acked-by: Kashyap Chamarthy <kchamart@redhat.com>
As I stated the patch is already pushed so you can post a patch fixing the small nits listed here. Thanks for the review. Pavel
On Fri, Mar 19, 2021 at 11:59:11AM +0100, Pavel Hrdina wrote:
On Fri, Mar 19, 2021 at 11:10:05AM +0100, Kashyap Chamarthy wrote:
On Thu, Mar 18, 2021 at 01:26:45PM +0100, Pavel Hrdina wrote:
[...]
Nit: I'd recast it as: "When using firmware auto-selection, different features are enabled in any given firmware binary."
Sounds a bit better but I've already pushed the patches.
Np; can be a follow-up. [...]
Should we also list a couple of example features? E.g. "amd-sev" (on supported hardware), "acpi-s3", "secure-boot".
I was considering listing all features that the JSON files can have but most of the other features are already controlled by different XML elements. There is an explicit list of features later in the docs.
Ah, where's the explict list of features? I don't see them under the "BIOS bootloader" section: https://libvirt.org/formatdomain.html#bios-bootloader
I notice that "enrolled-keys" is the only feature advertized by one of the firmware descriptor files (40-edk2-ovmf-x64-sb-enrolled.json), which is used as a mandatory XML attribute for the ``feature`` element.
I'm silently wondering there's a possibility for confusion: "Hmm, 'enrolled-keys' is a possible feature, but it is a mandatory attribute of ``feature`` element."
I don't understand what you mean here. If you are talking about our XML and the following text here in this patch then if you use the feature element both attributes ``enabled`` and ``name`` are mandatory. But few lines above there is a statement that the you can specify zero or more ``feature`` elements so you don't have to limit any features.
I missed the "zero or more" bit; sorry.
Our documentation doesn't state that 'enrolled-keys' is possible feature and mandatory attribute at the same time.
Yep, fair enough. You can disregard my second paragraph above. [...]
+ - ``enrolled-keys`` whether the selected nvram template has default
Nit: s/nvram/NVRAM/ (Or ``nvram``, to use rST verbatim; as it's an XML element.)
Using ``nvram`` would have been probably better.
[...]
Acked-by: Kashyap Chamarthy <kchamart@redhat.com>
As I stated the patch is already pushed so you can post a patch fixing the small nits listed here.
Sure.
Thanks for the review.
Pavel
-- /kashyap
On Fri, Mar 19, 2021 at 04:11:39PM +0100, Kashyap Chamarthy wrote:
On Fri, Mar 19, 2021 at 11:59:11AM +0100, Pavel Hrdina wrote:
On Fri, Mar 19, 2021 at 11:10:05AM +0100, Kashyap Chamarthy wrote:
On Thu, Mar 18, 2021 at 01:26:45PM +0100, Pavel Hrdina wrote:
[...]
Nit: I'd recast it as: "When using firmware auto-selection, different features are enabled in any given firmware binary."
Sounds a bit better but I've already pushed the patches.
Np; can be a follow-up.
[...]
Should we also list a couple of example features? E.g. "amd-sev" (on supported hardware), "acpi-s3", "secure-boot".
I was considering listing all features that the JSON files can have but most of the other features are already controlled by different XML elements. There is an explicit list of features later in the docs.
Ah, where's the explict list of features? I don't see them under the "BIOS bootloader" section: https://libvirt.org/formatdomain.html#bios-bootloader
Under the 'firmware' element there is a description of 'feature' element that lists mandatory attributes and for attribute 'name' there is a list of possible features which includes 'enrolled-keys' and 'secure-boot'. This is the part from formatdomain.rst file: ``feature`` The list of mandatory attributes: - ``enabled`` (accepted values are ``yes`` and ``no``) is used to tell libvirt if the feature must be enabled or not in the automatically selected firmware - ``name`` the name of the feature, the list of the features: - ``enrolled-keys`` whether the selected nvram template has default certificate enrolled. Firmware with Secure Boot feature but without enrolled keys will successfully boot non-signed binaries as well. Valid only for firmwares with Secure Boot feature. - ``secure-boot`` whether the firmware implements UEFI Secure boot feature. Pavel
On Thu, Mar 18, 2021 at 01:26:45PM +0100, Pavel Hrdina wrote:
When the firmware auto-selection was introduced it always picked first usable firmware based on the JSON descriptions on the host. It is possible to add/remove/change the JSON files but it will always be for the whole host.
This patch introduces support for configuring the auto-selection per VM by adding users an option to limit what features they would like to have available in the firmware.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.rst | 31 +++++++ docs/schemas/domaincommon.rng | 23 +++++ src/conf/domain_conf.c | 83 ++++++++++++++++++- src/conf/domain_conf.h | 10 +++ .../os-firmware-efi-invalid-type.xml | 28 +++++++ ...os-firmware-invalid-type.x86_64-latest.err | 1 + .../os-firmware-invalid-type.xml | 28 +++++++ tests/qemuxml2argvtest.c | 1 + ...aarch64-os-firmware-efi.aarch64-latest.xml | 1 + .../os-firmware-bios.x86_64-latest.xml | 1 + .../os-firmware-efi-secboot.x86_64-latest.xml | 1 + .../os-firmware-efi.x86_64-latest.xml | 1 + tests/vmx2xmldata/vmx2xml-firmware-efi.xml | 1 + 13 files changed, 207 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-invalid-type.xml create mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.x86_64-latest.err create mode 100644 tests/qemuxml2argvdata/os-firmware-invalid-type.xml
@@ -19600,22 +19606,67 @@ virDomainDefParseBootFirmwareOptions(virDomainDefPtr def, xmlXPathContextPtr ctxt) { g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt); + g_autofree char *type = virXPathString("string(./os/firmware/@type)", ctxt);
- if (!firmware) + if (!firmware && !type) return 0;
- fw = virDomainOsDefFirmwareTypeFromString(firmware); + if (firmware && type && STRNEQ(firmware, type)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("firmware attribute and firmware type has to be the same")); + return -1; + }
Why do we need to introduce a second attribute that must be the same as the attribute we already have ? This feels like it introduces a error scenario that we don't really need to have and is redundant data for the user.
diff --git a/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml index 627e285ae1..cb4f3ccfce 100644 --- a/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml +++ b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.aarch64-latest.xml @@ -6,6 +6,7 @@ <vcpu placement='static'>1</vcpu> <os firmware='efi'> <type arch='aarch64' machine='virt-4.0'>hvm</type> + <firmware type='efi'/>
This just looks odd having to set 'efi' twice.
<kernel>/aarch64.kernel</kernel> <initrd>/aarch64.initrd</initrd> <cmdline>earlyprintk console=ttyAMA0,115200n8 rw root=/dev/vda rootwait</cmdline>
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_firmware.c | 40 +++++++++++++++ ...re-efi-no-enrolled-keys.x86_64-latest.args | 49 ++++++++++++++++++ .../os-firmware-efi-no-enrolled-keys.xml | 25 ++++++++++ tests/qemuxml2argvtest.c | 1 + ...are-efi-no-enrolled-keys.x86_64-latest.xml | 50 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 166 insertions(+) create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml create mode 100644 tests/qemuxml2xmloutdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.xml diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index d3198e2d45..f6f371f51f 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -930,6 +930,10 @@ qemuFirmwareMatchDomain(const virDomainDef *def, bool supportsS4 = false; bool requiresSMM = false; bool supportsSEV = false; + bool supportsSecureBoot = false; + bool hasEnrolledKeys = false; + int reqSecureBoot; + int reqEnrolledKeys; want = qemuFirmwareOSInterfaceTypeFromOsDefFirmware(def->os.firmware); @@ -979,7 +983,13 @@ qemuFirmwareMatchDomain(const virDomainDef *def, break; case QEMU_FIRMWARE_FEATURE_SECURE_BOOT: + supportsSecureBoot = true; + break; + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: + hasEnrolledKeys = true; + break; + case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC: case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC: case QEMU_FIRMWARE_FEATURE_NONE: @@ -1000,6 +1010,36 @@ qemuFirmwareMatchDomain(const virDomainDef *def, return false; } + if (def->os.firmwareFeatures) { + reqSecureBoot = def->os.firmwareFeatures[VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_SECURE_BOOT]; + if (reqSecureBoot != VIR_TRISTATE_BOOL_ABSENT) { + if (reqSecureBoot == VIR_TRISTATE_BOOL_YES && !supportsSecureBoot) { + VIR_DEBUG("User requested Secure Boot, firmware '%s' doesn't support it", + path); + return false; + } + + if (reqSecureBoot == VIR_TRISTATE_BOOL_NO && supportsSecureBoot) { + VIR_DEBUG("User refused Secure Boot, firmware '%s' supports it", path); + return false; + } + } + + reqEnrolledKeys = def->os.firmwareFeatures[VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_ENROLLED_KEYS]; + if (reqEnrolledKeys != VIR_TRISTATE_BOOL_ABSENT) { + if (reqEnrolledKeys == VIR_TRISTATE_BOOL_YES && !hasEnrolledKeys) { + VIR_DEBUG("User requested Enrolled keys, firmware '%s' doesn't support it", + path); + return false; + } + + if (reqEnrolledKeys == VIR_TRISTATE_BOOL_NO && hasEnrolledKeys) { + VIR_DEBUG("User refused Enrolled keys, firmware '%s' supports it", path); + return false; + } + } + } + if (def->os.loader && def->os.loader->secure == VIR_TRISTATE_BOOL_YES && !requiresSMM) { diff --git a/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.args b/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.args new file mode 100644 index 0000000000..561a905e78 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.args @@ -0,0 +1,49 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-fedora \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-fedora/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-fedora/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-fedora/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=fedora,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-fedora/master-key.aes \ +-blockdev '{"driver":"file","filename":"/usr/share/OVMF/OVMF_CODE.fd",\ +"node-name":"libvirt-pflash0-storage","auto-read-only":true,\ +"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,\ +"driver":"raw","file":"libvirt-pflash0-storage"}' \ +-blockdev '{"driver":"file",\ +"filename":"/var/lib/libvirt/qemu/nvram/fedora_VARS.fd",\ +"node-name":"libvirt-pflash1-storage","auto-read-only":true,\ +"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,\ +"driver":"raw","file":"libvirt-pflash1-storage"}' \ +-machine pc-q35-4.0,accel=kvm,usb=off,dump-guest-core=off,\ +pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format,\ +memory-backend=pc.ram \ +-cpu qemu64 \ +-m 8 \ +-object memory-backend-ram,id=pc.ram,size=8388608 \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot strict=on \ +-device pcie-root-port,port=0x8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,\ +addr=0x1 \ +-device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \ +-device qemu-xhci,id=usb,bus=pci.1,addr=0x0 \ +-audiodev id=audio1,driver=none \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml b/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml new file mode 100644 index 0000000000..6c0b323fd4 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml @@ -0,0 +1,25 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <firmware type='efi'> + <feature enabled='no' name='enrolled-keys'/> + </firmware> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 2b32b7f303..44c2a316b0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3549,6 +3549,7 @@ mymain(void) DO_TEST_CAPS_LATEST("os-firmware-bios"); DO_TEST_CAPS_LATEST("os-firmware-efi"); DO_TEST_CAPS_LATEST("os-firmware-efi-secboot"); + DO_TEST_CAPS_LATEST("os-firmware-efi-no-enrolled-keys"); DO_TEST_CAPS_LATEST_PARSE_ERROR("os-firmware-invalid-type"); DO_TEST_CAPS_ARCH_LATEST("aarch64-os-firmware-efi", "aarch64"); diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.xml b/tests/qemuxml2xmloutdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.xml new file mode 100644 index 0000000000..3dbfbf0082 --- /dev/null +++ b/tests/qemuxml2xmloutdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.xml @@ -0,0 +1,50 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <firmware type='efi'> + <feature enabled='no' name='enrolled-keys'/> + </firmware> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='usb' index='0' model='qemu-xhci'> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='1' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='1' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' multifunction='on'/> + </controller> + <controller type='pci' index='2' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='2' port='0x9'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index f25a0902c9..4e7cce21c6 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1123,6 +1123,7 @@ mymain(void) DO_TEST_CAPS_LATEST("os-firmware-bios"); DO_TEST_CAPS_LATEST("os-firmware-efi"); DO_TEST_CAPS_LATEST("os-firmware-efi-secboot"); + DO_TEST_CAPS_LATEST("os-firmware-efi-no-enrolled-keys"); DO_TEST("aarch64-aavmf-virtio-mmio", QEMU_CAPS_DEVICE_VIRTIO_MMIO, -- 2.30.2
On 3/18/21 1:26 PM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_firmware.c | 40 +++++++++++++++ ...re-efi-no-enrolled-keys.x86_64-latest.args | 49 ++++++++++++++++++ .../os-firmware-efi-no-enrolled-keys.xml | 25 ++++++++++ tests/qemuxml2argvtest.c | 1 + ...are-efi-no-enrolled-keys.x86_64-latest.xml | 50 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 166 insertions(+) create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml create mode 100644 tests/qemuxml2xmloutdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.xml
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index d3198e2d45..f6f371f51f 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -930,6 +930,10 @@ qemuFirmwareMatchDomain(const virDomainDef *def, bool supportsS4 = false; bool requiresSMM = false; bool supportsSEV = false; + bool supportsSecureBoot = false; + bool hasEnrolledKeys = false; + int reqSecureBoot; + int reqEnrolledKeys;
want = qemuFirmwareOSInterfaceTypeFromOsDefFirmware(def->os.firmware);
@@ -979,7 +983,13 @@ qemuFirmwareMatchDomain(const virDomainDef *def, break;
case QEMU_FIRMWARE_FEATURE_SECURE_BOOT: + supportsSecureBoot = true; + break; + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: + hasEnrolledKeys = true; + break; + case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC: case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC: case QEMU_FIRMWARE_FEATURE_NONE: @@ -1000,6 +1010,36 @@ qemuFirmwareMatchDomain(const virDomainDef *def, return false; }
+ if (def->os.firmwareFeatures) { + reqSecureBoot = def->os.firmwareFeatures[VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_SECURE_BOOT]; + if (reqSecureBoot != VIR_TRISTATE_BOOL_ABSENT) { + if (reqSecureBoot == VIR_TRISTATE_BOOL_YES && !supportsSecureBoot) { + VIR_DEBUG("User requested Secure Boot, firmware '%s' doesn't support it", + path); + return false; + } + + if (reqSecureBoot == VIR_TRISTATE_BOOL_NO && supportsSecureBoot) { + VIR_DEBUG("User refused Secure Boot, firmware '%s' supports it", path); + return false; + } + } + + reqEnrolledKeys = def->os.firmwareFeatures[VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_ENROLLED_KEYS]; + if (reqEnrolledKeys != VIR_TRISTATE_BOOL_ABSENT) { + if (reqEnrolledKeys == VIR_TRISTATE_BOOL_YES && !hasEnrolledKeys) { + VIR_DEBUG("User requested Enrolled keys, firmware '%s' doesn't support it",
"doesn't have them" perhaps?
+ path); + return false; + } + + if (reqEnrolledKeys == VIR_TRISTATE_BOOL_NO && hasEnrolledKeys) { + VIR_DEBUG("User refused Enrolled keys, firmware '%s' supports it", path);
"has them" perhaps?
+ return false; + } + } + } + if (def->os.loader && def->os.loader->secure == VIR_TRISTATE_BOOL_YES && !requiresSMM) { diff --git a/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.args b/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.args new file mode 100644 index 0000000000..561a905e78 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.args @@ -0,0 +1,49 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-fedora \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-fedora/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-fedora/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-fedora/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=fedora,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-fedora/master-key.aes \ +-blockdev '{"driver":"file","filename":"/usr/share/OVMF/OVMF_CODE.fd",\ +"node-name":"libvirt-pflash0-storage","auto-read-only":true,\ +"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,\ +"driver":"raw","file":"libvirt-pflash0-storage"}' \ +-blockdev '{"driver":"file",\ +"filename":"/var/lib/libvirt/qemu/nvram/fedora_VARS.fd",\ +"node-name":"libvirt-pflash1-storage","auto-read-only":true,\ +"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,\ +"driver":"raw","file":"libvirt-pflash1-storage"}' \ +-machine pc-q35-4.0,accel=kvm,usb=off,dump-guest-core=off,\ +pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format,\ +memory-backend=pc.ram \ +-cpu qemu64 \ +-m 8 \ +-object memory-backend-ram,id=pc.ram,size=8388608 \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot strict=on \ +-device pcie-root-port,port=0x8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,\ +addr=0x1 \ +-device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \ +-device qemu-xhci,id=usb,bus=pci.1,addr=0x0 \ +-audiodev id=audio1,driver=none \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml b/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml new file mode 100644 index 0000000000..6c0b323fd4 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml @@ -0,0 +1,25 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <firmware type='efi'> + <feature enabled='no' name='enrolled-keys'/> + </firmware> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 2b32b7f303..44c2a316b0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3549,6 +3549,7 @@ mymain(void) DO_TEST_CAPS_LATEST("os-firmware-bios"); DO_TEST_CAPS_LATEST("os-firmware-efi"); DO_TEST_CAPS_LATEST("os-firmware-efi-secboot"); + DO_TEST_CAPS_LATEST("os-firmware-efi-no-enrolled-keys"); DO_TEST_CAPS_LATEST_PARSE_ERROR("os-firmware-invalid-type"); DO_TEST_CAPS_ARCH_LATEST("aarch64-os-firmware-efi", "aarch64");
diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.xml b/tests/qemuxml2xmloutdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.xml
Alternatively, let this be link to the XML above, since the difference between them is not in the area of interest of this feature. Michal
On Thu, Mar 18, 2021 at 05:18:38PM +0100, Michal Privoznik wrote:
On 3/18/21 1:26 PM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_firmware.c | 40 +++++++++++++++ ...re-efi-no-enrolled-keys.x86_64-latest.args | 49 ++++++++++++++++++ .../os-firmware-efi-no-enrolled-keys.xml | 25 ++++++++++ tests/qemuxml2argvtest.c | 1 + ...are-efi-no-enrolled-keys.x86_64-latest.xml | 50 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 166 insertions(+) create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml create mode 100644 tests/qemuxml2xmloutdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.xml
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index d3198e2d45..f6f371f51f 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -930,6 +930,10 @@ qemuFirmwareMatchDomain(const virDomainDef *def, bool supportsS4 = false; bool requiresSMM = false; bool supportsSEV = false; + bool supportsSecureBoot = false; + bool hasEnrolledKeys = false; + int reqSecureBoot; + int reqEnrolledKeys; want = qemuFirmwareOSInterfaceTypeFromOsDefFirmware(def->os.firmware); @@ -979,7 +983,13 @@ qemuFirmwareMatchDomain(const virDomainDef *def, break; case QEMU_FIRMWARE_FEATURE_SECURE_BOOT: + supportsSecureBoot = true; + break; + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: + hasEnrolledKeys = true; + break; + case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC: case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC: case QEMU_FIRMWARE_FEATURE_NONE: @@ -1000,6 +1010,36 @@ qemuFirmwareMatchDomain(const virDomainDef *def, return false; } + if (def->os.firmwareFeatures) { + reqSecureBoot = def->os.firmwareFeatures[VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_SECURE_BOOT]; + if (reqSecureBoot != VIR_TRISTATE_BOOL_ABSENT) { + if (reqSecureBoot == VIR_TRISTATE_BOOL_YES && !supportsSecureBoot) { + VIR_DEBUG("User requested Secure Boot, firmware '%s' doesn't support it", + path); + return false; + } + + if (reqSecureBoot == VIR_TRISTATE_BOOL_NO && supportsSecureBoot) { + VIR_DEBUG("User refused Secure Boot, firmware '%s' supports it", path); + return false; + } + } + + reqEnrolledKeys = def->os.firmwareFeatures[VIR_DOMAIN_OS_DEF_FIRMWARE_FEATURE_ENROLLED_KEYS]; + if (reqEnrolledKeys != VIR_TRISTATE_BOOL_ABSENT) { + if (reqEnrolledKeys == VIR_TRISTATE_BOOL_YES && !hasEnrolledKeys) { + VIR_DEBUG("User requested Enrolled keys, firmware '%s' doesn't support it",
"doesn't have them" perhaps?
+ path); + return false; + } + + if (reqEnrolledKeys == VIR_TRISTATE_BOOL_NO && hasEnrolledKeys) { + VIR_DEBUG("User refused Enrolled keys, firmware '%s' supports it", path);
"has them" perhaps?
Sounds better, I wanted to change it after copy&paste of the secureBoot part, but as we can see it did not happen. :)
+ return false; + } + } + } + if (def->os.loader && def->os.loader->secure == VIR_TRISTATE_BOOL_YES && !requiresSMM) { diff --git a/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.args b/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.args new file mode 100644 index 0000000000..561a905e78 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.args @@ -0,0 +1,49 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-fedora \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-fedora/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-fedora/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-fedora/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=fedora,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-fedora/master-key.aes \ +-blockdev '{"driver":"file","filename":"/usr/share/OVMF/OVMF_CODE.fd",\ +"node-name":"libvirt-pflash0-storage","auto-read-only":true,\ +"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,\ +"driver":"raw","file":"libvirt-pflash0-storage"}' \ +-blockdev '{"driver":"file",\ +"filename":"/var/lib/libvirt/qemu/nvram/fedora_VARS.fd",\ +"node-name":"libvirt-pflash1-storage","auto-read-only":true,\ +"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,\ +"driver":"raw","file":"libvirt-pflash1-storage"}' \ +-machine pc-q35-4.0,accel=kvm,usb=off,dump-guest-core=off,\ +pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format,\ +memory-backend=pc.ram \ +-cpu qemu64 \ +-m 8 \ +-object memory-backend-ram,id=pc.ram,size=8388608 \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot strict=on \ +-device pcie-root-port,port=0x8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,\ +addr=0x1 \ +-device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \ +-device qemu-xhci,id=usb,bus=pci.1,addr=0x0 \ +-audiodev id=audio1,driver=none \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml b/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml new file mode 100644 index 0000000000..6c0b323fd4 --- /dev/null +++ b/tests/qemuxml2argvdata/os-firmware-efi-no-enrolled-keys.xml @@ -0,0 +1,25 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <firmware type='efi'> + <feature enabled='no' name='enrolled-keys'/> + </firmware> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 2b32b7f303..44c2a316b0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3549,6 +3549,7 @@ mymain(void) DO_TEST_CAPS_LATEST("os-firmware-bios"); DO_TEST_CAPS_LATEST("os-firmware-efi"); DO_TEST_CAPS_LATEST("os-firmware-efi-secboot"); + DO_TEST_CAPS_LATEST("os-firmware-efi-no-enrolled-keys"); DO_TEST_CAPS_LATEST_PARSE_ERROR("os-firmware-invalid-type"); DO_TEST_CAPS_ARCH_LATEST("aarch64-os-firmware-efi", "aarch64"); diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.xml b/tests/qemuxml2xmloutdata/os-firmware-efi-no-enrolled-keys.x86_64-latest.xml
Alternatively, let this be link to the XML above, since the difference between them is not in the area of interest of this feature.
Will do. I usually try to create the input XML as minimal as possible so it can be used as an example of the feature but I don't have a strong preference. Thanks, Pavel
participants (4)
-
Daniel P. Berrangé -
Kashyap Chamarthy -
Michal Privoznik -
Pavel Hrdina