1:26 p.m.
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
1:26 p.m.
New subject: [libvirt PATCH 1/9] docs: improve description of secure attribute for loader element
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(a)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
5:42 p.m.
New subject: [libvirt PATCH 1/9] docs: improve description of secure attribute for loader element
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(a)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(a)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
6:21 p.m.
New subject: [libvirt PATCH 1/9] docs: improve description of secure attribute for loader element
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(a)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(a)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
1:26 p.m.
New subject: [libvirt PATCH 2/9] conf: introduce virDomainDefParseBootInitOptions
Extract the code to it's own function.
Signed-off-by: Pavel Hrdina <phrdina(a)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
1:26 p.m.
New subject: [libvirt PATCH 3/9] conf: introduce virDomainDefParseBootKernelOptions
Extract the code to it's own function.
Signed-off-by: Pavel Hrdina <phrdina(a)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
1:26 p.m.
New subject: [libvirt PATCH 4/9] conf: introduce virDomainDefParseBootFirmwareOptions
Extract the code to it's own function.
Signed-off-by: Pavel Hrdina <phrdina(a)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
1:26 p.m.
New subject: [libvirt PATCH 5/9] conf: introduce virDomainDefParseBootLoaderOptions
Extract the code to it's own function.
Signed-off-by: Pavel Hrdina <phrdina(a)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
1:26 p.m.
New subject: [libvirt PATCH 6/9] conf: introduce virDomainDefParseBootAcpiOptions
Extract the code to it's own function.
Signed-off-by: Pavel Hrdina <phrdina(a)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
1:26 p.m.
New subject: [libvirt PATCH 7/9] conf: use switch in virDomainDefParseBootOptions
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(a)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
1:26 p.m.
New subject: [libvirt PATCH 8/9] conf: introduce support for firmware auto-selection feature filtering
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(a)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
5:18 p.m.
New subject: [libvirt PATCH 8/9] conf: introduce support for firmware auto-selection feature filtering
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(a)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
6:10 p.m.
New subject: [libvirt PATCH 8/9] conf: introduce support for firmware auto-selection feature filtering
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(a)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
6:31 p.m.
New subject: [libvirt PATCH 8/9] conf: introduce support for firmware auto-selection feature filtering
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(a)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
11:10 a.m.
New subject: [libvirt PATCH 8/9] conf: introduce support for firmware auto-selection feature filtering
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(a)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(a)redhat.com>
[...]
--
/kashyap
11:59 a.m.
New subject: [libvirt PATCH 8/9] conf: introduce support for firmware auto-selection feature filtering
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(a)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(a)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
4:11 p.m.
New subject: [libvirt PATCH 8/9] conf: introduce support for firmware auto-selection feature filtering
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(a)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
5:10 p.m.
New subject: [libvirt PATCH 8/9] conf: introduce support for firmware auto-selection feature filtering
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
7:33 p.m.
New subject: [libvirt PATCH 8/9] conf: introduce support for firmware auto-selection feature filtering
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(a)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 :|
1:26 p.m.
New subject: [libvirt PATCH 9/9] qemu: implement support for firmware auto-selection feature filtering
Signed-off-by: Pavel Hrdina <phrdina(a)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
5:18 p.m.
New subject: [libvirt PATCH 9/9] qemu: implement support for firmware auto-selection feature filtering
On 3/18/21 1:26 PM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)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
6:16 p.m.
New subject: [libvirt PATCH 9/9] qemu: implement support for firmware auto-selection feature filtering
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(a)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
1278
days inactive
1289
days old
21 comments
4 participants
participants (4)
-
Daniel P. Berrangé -
Kashyap Chamarthy -
Michal Privoznik -
Pavel Hrdina