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

[Pavel Hrdina]

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 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."

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210319/56e758dd/attachment-0001.sig>