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

["Kubacki, Michael A" <michael.a.kubacki@intel.com>]

I completely understand the need for granular breakup of changes for code review and future maintenance. I would not send this as a single patch on the mailing list for formal code review. Due to the size of the change, the main point here was to initially focus feedback on the high-level approach and design sparing the review of implementation details for an actual code review. Though I understand for those interested, it is much easier to digest the code in a clean patch series so I will send that RFC series to the list once I incorporate the feedback already received.

I replied elsewhere inline.

Thanks,
Michael

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Monday, September 9, 2019 8:32 AM
> To: devel@edk2.groups.io; Kubacki, Michael A
> <michael.a.kubacki@intel.com>
> Subject: Re: [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction
> 
> 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/d812d43412a26e44581d283382
> 596
> > a863c1ae825
> >
> > 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?

Yes, that is correct.
> 
> > 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.

These points just summarize today's operation for the unaware. I agree the two buffers should be separate and there's no plan to change that.
> 
> Thanks!
> Laszlo