Laszlo, Regards, Jian
> -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Tuesday, January 08, 2019 11:38 PM > To: Ard Biesheuvel <[email protected]>; [email protected] > Cc: Leif Lindholm <[email protected]>; Kinney, Michael D > <[email protected]>; Gao, Liming <[email protected]>; Wang, > Jian J <[email protected]>; Wu, Hao A <[email protected]>; Jagadeesh > Ujja <[email protected]>; Achin Gupta <[email protected]>; > Thomas Panakamattam Abraham <[email protected]>; Sami Mujawar > <[email protected]> > Subject: Re: [PATCH 5/6] MdeModulePkg/VariableRuntimeDxe: factor out boot > service accesses > > On 01/03/19 19:28, Ard Biesheuvel wrote: > > In preparation of providing a standalone MM based variable runtime > > driver, move the existing SMM driver to the new MM services table, > > and factor out some pieces that are specific to the traditional > > driver, mainly related to the use of UEFI boot services, which are > > not accessible to standalone MM drivers. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel <[email protected]> > > --- > > MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 18 > +--- > > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 50 > +++++++++ > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 59 > ++++------ > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 5 +- > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c | > 114 ++++++++++++++++++++ > > 5 files changed, 187 insertions(+), 59 deletions(-) > > I *vaguely* feel like we should extract the new functions to > "PrivilegePolymorphic.h", rather than to "Variable.h". > > Please see initial commit 00663d047fc9 > ("MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new > header", 2017-10-10), and other commits that touched that file. > > I realize this is not a 100% "constructive" suggestion, and I feel > appropriately bad about that. It's just that "Variable.h" has so many > internals that I feel it's not a good dumping ground for these new > functions. And the other header we have, looks closer in purpose. > > For example, MorLockInitAtEndOfDxe() is already declared in > "PrivilegePolymorphic.h" (see commit f1304280435f, > "MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe() > hook", 2017-10-10). > > Admittedly, now that we're going to have three separate builds of this > driver, dedicating a separate header file to each "shared between A and > B" relationship is getting a bit too complex. In retrospect, introducing > "PrivilegePolymorphic.h" may not have been a "scalable" idea, after all, > and I should have just dumped those functions all in "Variable.h". > > IOW, I think > - targeting "Variable.h" now is inconsistent with earlier code, > - extending "PrivilegePolymorphic.h" is also suboptimal (although still > better than the previous option), > - adding yet another header might be technically correct, but it would > be over-engineering, > - asking you to merge "PrivilegePolymorphic.h" back into "Variable.h" > feels awkward, especially after I argued *for* "PrivilegePolymorphic.h" > at length, when I originally introduced it. :/ > > Sigh. Can the variable driver maintainers comment please? > > (I still plan to regression-test this series, but I feel like I should > force myself to at least skim the variable driver patches, beyond > testing them. Because, next time I can't avoid working with this very > complex driver, I wouldn't like to be *completely* lost.) > I agree "PrivilegePolymorphic.h" is more appropriate place for them. Maybe Star have different opinion. > Thanks, > Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
