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

["Wang, Jian J" <jian.j.wang@intel.com>]

Laszlo,

Regards,
Jian

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, January 08, 2019 11:38 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Wang,
> Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Jagadeesh
> Ujja <jagadeesh.ujja@arm.com>; Achin Gupta <Achin.Gupta@arm.com>;
> Thomas Panakamattam Abraham <thomas.abraham@arm.com>; Sami Mujawar
> <Sami.Mujawar@arm.com>
> 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 <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.)
> 

I agree "PrivilegePolymorphic.h" is more appropriate place for them.
Maybe Star have different opinion.

> Thanks,
> Laszlo