Re: [PATCH 5/6] MdeModulePkg/VariableRuntimeDxe: factor out boot service accesses

[Laszlo Ersek <lersek@redhat.com>]

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 <ard.biesheuvel@linaro.org>
> ---
>  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