Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration

[Jordan Justen <jordan.l.justen@intel.com>]

On 2019-02-18 18:46:24, Ni, Ray wrote:
> Jordan,
> I find many real platforms do not implement the temporary ram migration
> PPI and rely on the PeiCore migration  logic.
> So perhaps TemporaryRamMigration PPI was added to help platform to
> destroy the temporary RAM (CAR in x86 platform).

I guess since it is in the PI spec, we can't be sure it is only used
for this case, or that it might not be used in the future.

> But with the introduction of TemporaryRamDone PPI, maybe the
> TemporaryRamMigration PPI can be retired.
> The logic in PeiCore to call TemporaryRamMigration is just for backward
> compatibility.
> If that's true, do you still need to enhance PeiCore?

I checked the PI 1.4 spec, and I didn't see anything indicating that
TemporaryRamSupport PPI is deprecated.

Since it is not deprecated, should we ignore the known issue?

> For the Emulator case, I already found without TemporaryRamMigration
> the platform can still boot.
> 
> Does OVMF hard-depend on TemporaryRamMigration? Or it can reply on
> the PeiCore migration logic + TemporaryDone PPI?

This is a good question. If it is true that TemporaryRamSupport is
rarely used, then maybe it is better to have the sample platforms use
the more commonly used path.

Personally, I think we should still address the issue with
TemporaryRamSupport, and leave the question of whether to test
TemporaryRamSupport code paths in the sample platforms as a separate
task.

At the least, I think we should still continue to use TemporaryRamDone
to reset the temp-ram contents to help make sure nothing accidentally
depends on a temp-ram pointer. Unfortunately, this would mean that the
TemporaryRamSupport path is not really being tested, but it might be
the better choise if TemporaryRamSupport is never used in real
platforms.

-Jordan

> 
> > -----Original Message-----
> > From: Justen, Jordan L <jordan.l.justen@intel.com>
> > Sent: Monday, February 18, 2019 12:12 PM
> > To: edk2-devel@lists.01.org
> > Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Liu Yu
> > <pedroa.liu@outlook.com>; Andrew Fish <afish@apple.com>; Anthony
> > Perard <anthony.perard@citrix.com>; Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>; Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian
> > J <jian.j.wang@intel.com>; Julien Grall <julien.grall@linaro.org>; Laszlo Ersek
> > <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> > <star.zeng@intel.com>
> > Subject: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
> > 
> > https://github.com/jljusten/edk2.git temp-ram-support
> > 
> > This series fixes an issue that, while rare, is possible based on the way the
> > TemporaryRamSupport PPI is defined along with how it is used by the PEI
> > Core.
> > 
> > Liu Yu reported a boot issue with EmulatorPkg, which I believe was caused by
> > this issue.
> > 
> > The details of the issue are described in the commit message of the
> > "MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram
> > migration" patch.
> > 
> > Along with this, I added a few Temporary RAM patches for EmulatorPkg and
> > OvmfPkg.
> > 
> > Cc: Liu Yu <pedroa.liu@outlook.com>
> > Cc: Andrew Fish <afish@apple.com>
> > Cc: Anthony Perard <anthony.perard@citrix.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Hao Wu <hao.a.wu@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Julien Grall <julien.grall@linaro.org>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > 
> > Jordan Justen (10):
> >   EmulatorPkg/build.sh: Fix missing usage of -b BUILDTARGET parameter
> >   EmulatorPkg/Unix/Host: Use PcdInitValueInTempStack to init temp-ram
> >   EmulatorPkg/Sec: Replace assembly temp-ram support with C code
> >   EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration
> >     function
> >   OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations
> >   OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
> >   MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram
> >     migration
> >   MdeModulePkg/Core/Pei: Use assembly for X64 TemporaryRamMigration
> >   MdeModulePkg/Core/Pei: Use assembly for IA32 TemporaryRamMigration
> >   OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration
> > 
> >  EmulatorPkg/Sec/Ia32/SwitchRam.S              | 95 -------------------
> >  EmulatorPkg/Sec/Ia32/SwitchRam.asm            | 94 ------------------
> >  EmulatorPkg/Sec/Ia32/TempRam.c                | 65 -------------
> >  EmulatorPkg/Sec/Sec.c                         | 76 ++++++++++++++-
> >  EmulatorPkg/Sec/Sec.inf                       | 13 +--
> >  EmulatorPkg/Sec/X64/SwitchRam.S               | 72 --------------
> >  EmulatorPkg/Sec/X64/SwitchRam.asm             | 76 ---------------
> >  EmulatorPkg/Unix/Host/Host.c                  |  2 +-
> >  EmulatorPkg/Unix/Host/Host.inf                |  1 +
> >  EmulatorPkg/build.sh                          | 10 +-
> >  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 59 ++++++++----
> >  .../Dispatcher/Ia32/TemporaryRamMigration.S   | 72 ++++++++++++++
> >  .../Ia32/TemporaryRamMigration.nasm           | 78 +++++++++++++++
> >  .../Pei/Dispatcher/TemporaryRamMigration.c    | 52 ++++++++++
> >  .../Dispatcher/X64/TemporaryRamMigration.S    | 69 ++++++++++++++
> >  .../Dispatcher/X64/TemporaryRamMigration.nasm | 75 +++++++++++++++
> >  MdeModulePkg/Core/Pei/PeiMain.h               | 52 ++++++++++
> >  MdeModulePkg/Core/Pei/PeiMain.inf             | 14 +++
> >  OvmfPkg/Sec/Ia32/SecEntry.nasm                |  2 +-
> >  OvmfPkg/Sec/SecMain.c                         | 59 ++++++++----
> >  OvmfPkg/Sec/X64/SecEntry.nasm                 |  2 +-
> >  21 files changed, 577 insertions(+), 461 deletions(-)  delete mode 100644
> > EmulatorPkg/Sec/Ia32/SwitchRam.S  delete mode 100644
> > EmulatorPkg/Sec/Ia32/SwitchRam.asm
> >  delete mode 100644 EmulatorPkg/Sec/Ia32/TempRam.c  delete mode
> > 100644 EmulatorPkg/Sec/X64/SwitchRam.S  delete mode 100644
> > EmulatorPkg/Sec/X64/SwitchRam.asm  create mode 100644
> > MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.S
> >  create mode 100644
> > MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
> >  create mode 100644
> > MdeModulePkg/Core/Pei/Dispatcher/TemporaryRamMigration.c
> >  create mode 100644
> > MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.S
> >  create mode 100644
> > MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
> > 
> > --
> > 2.20.0.rc1
>