Re: [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction
On 09/09/19 20:03, Kubacki, Michael A wrote: > 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. Thank you! > I replied elsewhere inline. For those answers as well. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47121): https://edk2.groups.io/g/devel/message/47121 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] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction
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 > Sent: Monday, September 9, 2019 8:32 AM > To: devel@edk2.groups.io; Kubacki, Michael A > > 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] -=-=-=-=-=-=-=-=-=-=-=-
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/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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47043): https://edk2.groups.io/g/devel/message/47043 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] -=-=-=-=-=-=-=-=-=-=-=-
