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