Re: [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

["Laszlo Ersek" <lersek@redhat.com>]

On 09/05/19 21:54, Kubacki, Michael A wrote:

> Proof-of-Concept Implementation
> ----------------------------------------------
> The implementation is available in the following commit - check the commit message for some more details. 
> https://github.com/makubacki/edk2/commit/d812d43412a26e44581d283382596a863c1ae825
> 
> Please note this is "POC" level code quality and there will be cleanup of lock interfaces used and some other minor changes. Please feel free to leave any comments on the changes.

First some meta thoughts:

- If this code is meant for edk2 ultimately, please keep the discussion
on the mailing list, and/or in a TianoCore bugzilla.

- The size of this feature is significant. According to the github
webui, "19 changed files with 2,083 additions and 1,072 deletions".

A feature of this size must absolutely be broken up into a fine-grained
patch series (assuming the feature targets the edk2 master branch). It's
not only that such a huge patch is basically unreviewable: if someone
runs into an issue after the feature is committed, they will need the
ability to bisect the regression to a well isolated, self contained
modification. Then the experts around the feature have a much better
chance at root causing the issue from the patch that the bug reporter
has identified. An all-or-nothing patch is not bisectable.

- Combining the above two points into one, I'd suggest splitting the
feature into small patches, and posting it as an RFC series to the list.
(Assuming the initial reaction to the feature is interest -- I think it
is.) Admittedly, this is a lot of work.

Some more on-topic comments:

> Why Keep SMM on Variable Writes
> ------------------------------------------------
>  * SMM provides a fairly ubiquitous isolated execution environment in x86 for authenticated UEFI variables.
>  * BIOS region SPI flash write restrictions to SMM in platforms today can be retained.

Can you confirm that the runtime DXE code would not read flash, only the
cache in EfiRuntimeServicesData memory?

> Today's UEFI Variable Cache
> --------------------------------------
>  * Maintained in SMRAM via VariableSmm.
>  * A "write-through" cache of variable data in the form of a UEFI variable store.
>  * Non-volatile and volatile variables are maintained in separate buffers (variable stores).

I'm unclear on how the items on this list should be interpreted. Are
these items from today that we keep, or items that we change?

For example, it's quite beneficial that NV and V variables are
maintained in separate buffers -- the sizing can be different, and
that's good. I believe we shouldn't change that.

Thanks!
Laszlo