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@xxxxxxxxxx>
> > > ---
> > > 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
Attachment:
signature.asc
Description: PGP signature
