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 <ler...@redhat.com> > Sent: Monday, September 9, 2019 8:32 AM > To: devel@edk2.groups.io; Kubacki, Michael A > <michael.a.kuba...@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47044): https://edk2.groups.io/g/devel/message/47044 Mute This Topic: https://groups.io/mt/33158252/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
