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

["Ni, Ray" <ray.ni@intel.com>]

> -----Original Message-----
> From: Justen, Jordan L <jordan.l.justen@intel.com>
> Sent: Thursday, February 21, 2019 1:44 AM
> To: Gao, Liming <liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; edk2-
> devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Anthony Perard
> <anthony.perard@citrix.com>; Laszlo Ersek <lersek@redhat.com>; Zeng,
> Star <star.zeng@intel.com>
> Subject: Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
> 
> On 2019-02-20 05:27:21, Ni, Ray wrote:
> > On 2/19/2019 9:25 PM, Gao, Liming wrote:
> > > Ray:
> > >
> > >      Now, real platform has no side effect. So, only TempRamDone
> > >      PPI is produced. For emulator platform, is there any side
> > >      effect when both Temporary RAM and Permanent RAM are enabled?
> > >
> >
> > No side effect when both of T-RAM and P-RAM are enabled.
> > Which means no side effect when neither of the PPIs is produced.
> > But for demo purpose, I think producing TemporaryRamDone PPI makes
> sense.
> 
> In addition to being a demo/sample, it also provides a check that no modules
> are accessing temp-ram after temp-ram should no longer be used.
> 
> > I will work out patches for EmulatorPkg to produce TemoraryRamDone.
> 
> I think we should first fix TemporaryRamSupport usage. Otherwise, we are
> just hiding the bug.
> 
> Have you tried these patches to verify that they fix the issue in your setup?
> Have you taken a look at the patches to see what problem is being fixed?

I feel the change to PeiCore is a bit complex and introduce additional deps (assembly).
Behavior of ARM and x86 becomes different. (The patch only fixes x86 issue.)
Given there is no real requirement on this, I prefer to not change PeiCore.

> 
> -Jordan
> 
> >
> > >> -----Original Message-----
> > >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > >> Of Ni, Ray
> > >> Sent: Tuesday, February 19, 2019 10:46 AM
> > >> To: Justen, Jordan L <jordan.l.justen@intel.com>;
> > >> edk2-devel@lists.01.org
> > >> Cc: Wu, Hao A <hao.a.wu@intel.com>; Anthony Perard
> > >> <anthony.perard@citrix.com>; Laszlo Ersek <lersek@redhat.com>;
> > >> Zeng, Star <star.zeng@intel.com>
> > >> Subject: Re: [edk2] [PATCH 00/10] Fix PEI Core issue during
> > >> TemporaryRamMigration
> > >>
> > >> 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).
> > >> 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?
> > >>
> > >> 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?
> > >>
> > >> Thanks,
> > >> Ray
> > >>
> > >>> -----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
> > >>
> > >> _______________________________________________
> > >> edk2-devel mailing list
> > >> edk2-devel@lists.01.org
> > >> https://lists.01.org/mailman/listinfo/edk2-devel
> >
> >
> > --
> > Thanks,
> > Ray