On Fri, Mar 19, 2021 at 11:59:11AM +0100, Pavel Hrdina wrote:
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.
Np; can be a follow-up. [...]
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.
Ah, where's the explict list of features? I don't see them under the "BIOS bootloader" section: https://libvirt.org/formatdomain.html#bios-bootloader
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.
I missed the "zero or more" bit; sorry.
Our documentation doesn't state that 'enrolled-keys' is possible feature and mandatory attribute at the same time.
Yep, fair enough. You can disregard my second paragraph above. [...]
+ - ``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@redhat.com>
As I stated the patch is already pushed so you can post a patch fixing the small nits listed here.
Sure.
Thanks for the review.
Pavel
-- /kashyap