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@xxxxxxxxxx> > > 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
