[libvirt PATCH 8/9] conf: introduce support for firmware auto-selection feature filtering

[Kashyap Chamarthy]

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 at 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 at redhat.com>   

[...]

-- 
/kashyap