Re: [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&gt; > > --- > > 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.