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

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:

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

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

> > + - ``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.

> Acked-by: Kashyap Chamarthy <kchamart(a)redhat.com&gt; 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