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.)
Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
