Re: [PATCH 0/6] implement standalone MM versions of the variable runtime drivers

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Wed, 9 Jan 2019 at 10:44, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 01/03/19 19:28, Ard Biesheuvel wrote:
> > This series proposed an alternative approach to the series sent out by
> > Jagadeesh [0]. In particular, it gets rid of the InMm() calls and the
> > special PCD, as well as some other if() conditionals.
> >
> > The primary difference is that this series defines and implements
> > MmServicesTableLib in such a way that the traditional SMM drivers can
> > use it as well. This is appropriate, considering that the PI spec has
> > rebranded traditional SMM as one implementation of the generic MM
> > framework.
> >
> > Patch #1 is based on Jagadeesh's patch, and introduces the
> > MmServicesTableLib library class, but for all SMM flavours, not only
> > for standalone MM.
> >
> > Patch #2 implements MmServicesTableLib for traditional SMM
> > implementations.
> >
> > Patch #3 refactors FaultTolerantWriteDxe so that the parts of the SMM
> > driver that invoke boot services are separated from the core SMM
> > pieces.
> >
> > Patch #4 implements FaultTolerantWriteSmm for the standalone MM
> > environment.
> >
> > Patches #5 and #6 do the same, respectively, for the variable runtime
> > driver.
> >
> > This approach minimizes the delta, and thus the maintenance burden,
> > between the traditional SMM and standalone MM drivers, while not
> > resorting to runtime checks or other conditionals in the code to
> > implement logic that should be decided at build time.
> >
> > Note that this series only covers part of the work contributed by
> > Jagadeesh. This series focuses on the MdePkg and MdeModulePkg changes
> > that affect shared code.
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao Wu <hao.a.wu@intel.com>
> > Cc: Jagadeesh Ujja <jagadeesh.ujja@arm.com>
> > Cc: Achin Gupta <Achin.Gupta@arm.com>
> > Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>
> > Cc: Sami Mujawar <Sami.Mujawar@arm.com>
>
> I tried building this, on top of commit a53a888de8f5:
>
> build \
>   -a IA32 \
>   -p OvmfPkg/OvmfPkgIa32.dsc \
>   -t GCC48 \
>   -b NOOPT \
>   -D HTTP_BOOT_ENABLE \
>   -D NETWORK_IP6_ENABLE \
>   -D SECURE_BOOT_ENABLE \
>   -D SMM_REQUIRE \
>   -D TLS_ENABLE \
>   --cmd-len=65536 \
>   --hash \
>   --genfds-multi-thread
>
> but it failed with:
>
> > OvmfPkg/OvmfPkgIa32.dsc(...): error 4000: Instance of library class [MmServicesTableLib] is not found
> >         in [MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf] [IA32]
> >         consumed by module [MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf]
>
> You did mention earlier that adding new lib class resolutions to several
> x86 DSC files would be necessary, so this is not unexpected. Can you
> please insert such a patch for OvmfPkg between patches #2 and #3?
>

Ah yes, of course.

> I've looked at the current OVMF DSC files, and SmmServicesTableLib is
> resolved for two module types,
>
> > [LibraryClasses.common.DXE_SMM_DRIVER]
> >   SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf
> >
> > [LibraryClasses.common.SMM_CORE]
> >   SmmServicesTableLib|MdeModulePkg/Library/PiSmmCoreSmmServicesTableLib/PiSmmCoreSmmServicesTableLib.inf
>
> I assume it should be enough, for this series, to update the
> DXE_SMM_DRIVER resolution only, and to leave SMM_CORE alone.
>
> (Because, my understanding is that the current, x86 specific
>
>   MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
>
> module, of type SMM_CORE, will not be refactored; instead, it is
> entirely supplanted -- in the affected platforms -- by the
>
>   StandaloneMmPkg/Core/StandaloneMmCore.inf
>
> module, which is of type MM_CORE_STANDALONE.)
>
> But, it's still not clear to me (without trying) whether I should
> resolve MmServicesTableLib  for DXE_SMM_DRIVER in addition to
> SmmServicesTableLib, or in its place. I'd prefer not experimenting with
> this myself; I'd just like to apply the series, and build & test it. :)
>

At the moment, you will need both resolutions for DXE_SMM_DRIVERS
globally. Based on the outcome of the review of this series, this is
something we will need to clean up going forward, but currently, even
the drivers that are updated to use MmServicesTableLib pull in
libraries that depend on SmmServicesTableLib.

This should be a rather straight-forward search/replace operation
[famous last words], and I can commit to dedicating some of my time to
getting this fixed throughout, at least to the point where modules
that consume MmServicesTableLib no longer have to supply a resolution
for SmmServicesTableLib as well.

So, I will include a patch in the next revision of the series. In the
mean time, the hunk below should suffice to complete your regression
testing.

--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -391,6 +391,7 @@
   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
   SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf
   SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf
+  MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf
 !ifdef $(DEBUG_ON_SERIAL_PORT)
   DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
 !else