-----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.)
