Re: [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration

2019-04-10 Thread Jordan Justen 
On 2019-04-10 09:41:43, Ard Biesheuvel wrote:
> On Wed, 10 Apr 2019 at 01:41, Jordan Justen  wrote:
> >
> > https://github.com/jljusten/edk2.git temp-ram-support-v2
> >
> > https://github.com/jljusten/edk2/commits/temp-ram-support-v2
> >
> > v2:
> >  * Add AARCH64 and ARM assembly
> 
> Hi Jordan,
> 
> I'm not sure I'm following the reasoning behind this.

Did you see the explanation in patch 1 commit message? Could you reply
there with questions, or maybe I should try to expand on that?

> Does this fix an issue we currently have on ARM systems?

Yes, but I don't know of a case where it has been observed on
AARCH64/ARM. Nevertheless, as far as I can tell, a similar issue could
happen because the current implementation relies on non-spec'd
behavior of code gen within a C function. (Not the C calling
convention, but what code does with inside the function between
calls.)

> And how did you build and/or test OVMF for ARM?

I built ArmVirtPkg for AARCH64 and ARM on x86_64 Linux with a
cross-compiler, and ran it with qemu.

-Jordan

> 
> >  * Drop IA32 and X64 .S source files
> >  * Adjust PEI_CORE_TEMPORARY_RAM_TRANSITION pointer in the assembly
> >code based on the stack pointer change before & after
> >TemporaryRamSupport->TemporaryRamMigration
> >  * Drop extra cleanup patches for OvmfPkg & EmulatorPkg. These were
> >just complicating the series.
> >
> > 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
> > "MdeModulePkg/Core/Pei: Add interface for assembly based
> > TemporaryRamSupport" patch.
> >
> > Testing:
> >
> > I tested building and booting in several scenarios:
> >
> > * OVMF IA32 & X64 on Linux
> > * ArmVirtPkg AARCH64 & ARM on x86_64 Linux
> > * EmulatorPkg IA32 & X64 on Linux
> >
> > Untested:
> >
> > * My system does not reproduce the issue that Liu Yu reported with
> >   EmulatorPkg, so I can't say that I have verified that issue.
> > * Building on windows
> > * AARCH64/ARM TemporaryRamMigration.asm sources

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#38834): https://edk2.groups.io/g/devel/message/38834
Mute This Topic: https://groups.io/mt/31016921/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration

2019-04-10 Thread Ard Biesheuvel 
On Wed, 10 Apr 2019 at 11:28, Laszlo Ersek  wrote:
>
> On 04/10/19 18:41, Ard Biesheuvel wrote:
> > On Wed, 10 Apr 2019 at 01:41, Jordan Justen  
> > wrote:
> >>
> >> https://github.com/jljusten/edk2.git temp-ram-support-v2
> >>
> >> https://github.com/jljusten/edk2/commits/temp-ram-support-v2
> >>
> >> v2:
> >>  * Add AARCH64 and ARM assembly
> >
> > Hi Jordan,
> >
> > I'm not sure I'm following the reasoning behind this. Does this fix an
> > issue we currently have on ARM systems?
>
> IIUC, the PEI Core can only be updated to use the "safe path" (= the
> assembly path) on IA32/X64 if that path (= the assembly path) *exists*
> regardless of architecture.
>
> This is anyway my understanding of the last commit message in the series.
>
> I can't evaluate whether the problem statement, in the first commit
> message in the series, would ever turn into an actual issue on
> ARM/AARCH64, dependent on toolchain. On IA32/X64, we've seen examples
> (although none of those have bit me personally), and AIUI the issue
> could theoretically apply to all arches (again, dependent on toolchain).
>
> (Apologies if I've only increased the confusion with this -- but at
> least it could help me improve my own understanding.)
>
> > And how did you build and/or test OVMF for ARM?
>
> Hmmm, I'm unsure where this question / implication comes from. AIUI, the
> new ARM/AARCH64 assembly is automatically put to use if you run the PEI
> Core -- as part of a firmware platform that uses temp RAM migration --
> on ARM/AARCH64.
>

OK, seems like I'm the one creating confusion here. These are
MdeModulePkg patches not OvmfPkg patches, and I noticed some other
patches by Jordan adding ARM/AARCH64 build support to OVMF.

I will put the assembly patches on my to-review list, but it may take
me a couple of days to get to them.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#38832): https://edk2.groups.io/g/devel/message/38832
Mute This Topic: https://groups.io/mt/31016921/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration

2019-04-10 Thread Laszlo Ersek 
On 04/10/19 18:41, Ard Biesheuvel wrote:
> On Wed, 10 Apr 2019 at 01:41, Jordan Justen  wrote:
>>
>> https://github.com/jljusten/edk2.git temp-ram-support-v2
>>
>> https://github.com/jljusten/edk2/commits/temp-ram-support-v2
>>
>> v2:
>>  * Add AARCH64 and ARM assembly
> 
> Hi Jordan,
> 
> I'm not sure I'm following the reasoning behind this. Does this fix an
> issue we currently have on ARM systems?

IIUC, the PEI Core can only be updated to use the "safe path" (= the
assembly path) on IA32/X64 if that path (= the assembly path) *exists*
regardless of architecture.

This is anyway my understanding of the last commit message in the series.

I can't evaluate whether the problem statement, in the first commit
message in the series, would ever turn into an actual issue on
ARM/AARCH64, dependent on toolchain. On IA32/X64, we've seen examples
(although none of those have bit me personally), and AIUI the issue
could theoretically apply to all arches (again, dependent on toolchain).

(Apologies if I've only increased the confusion with this -- but at
least it could help me improve my own understanding.)

> And how did you build and/or test OVMF for ARM?

Hmmm, I'm unsure where this question / implication comes from. AIUI, the
new ARM/AARCH64 assembly is automatically put to use if you run the PEI
Core -- as part of a firmware platform that uses temp RAM migration --
on ARM/AARCH64.

Thanks,
Laszlo

> 
> 
>>  * Drop IA32 and X64 .S source files
>>  * Adjust PEI_CORE_TEMPORARY_RAM_TRANSITION pointer in the assembly
>>code based on the stack pointer change before & after
>>TemporaryRamSupport->TemporaryRamMigration
>>  * Drop extra cleanup patches for OvmfPkg & EmulatorPkg. These were
>>just complicating the series.
>>
>> 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
>> "MdeModulePkg/Core/Pei: Add interface for assembly based
>> TemporaryRamSupport" patch.
>>
>> Testing:
>>
>> I tested building and booting in several scenarios:
>>
>> * OVMF IA32 & X64 on Linux
>> * ArmVirtPkg AARCH64 & ARM on x86_64 Linux
>> * EmulatorPkg IA32 & X64 on Linux
>>
>> Untested:
>>
>> * My system does not reproduce the issue that Liu Yu reported with
>>   EmulatorPkg, so I can't say that I have verified that issue.
>> * Building on windows
>> * AARCH64/ARM TemporaryRamMigration.asm sources
>>
>> Cc: Liu Yu 
>> Cc: Ray Ni 
>> Cc: Andrew Fish 
>> Cc: Laszlo Ersek 
>> Cc: Leif Lindholm 
>> Cc: Michael D Kinney 
>>
>> Jordan Justen (6):
>>   MdeModulePkg/Core/Pei: Add interface for assembly based
>> TemporaryRamSupport
>>   MdeModulePkg/Core/Pei: Add AARCH64 assembly for TemporaryRamMigration
>>   MdeModulePkg/Core/Pei: Add ARM assembly for TemporaryRamMigration
>>   MdeModulePkg/Core/Pei: Add IA32 assembly for TemporaryRamMigration
>>   MdeModulePkg/Core/Pei: Add X64 assembly for TemporaryRamMigration
>>   MdeModulePkg/Core/Pei: Use code path for assembly based
>> TemporaryRamSupport
>>
>>  .../AArch64/TemporaryRamMigration.S   | 63 +++
>>  .../AArch64/TemporaryRamMigration.asm | 63 +++
>>  .../Dispatcher/Arm/TemporaryRamMigration.S| 68 
>>  .../Dispatcher/Arm/TemporaryRamMigration.asm  | 68 
>>  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 59 +-
>>  .../Ia32/TemporaryRamMigration.nasm   | 77 +++
>>  .../Dispatcher/X64/TemporaryRamMigration.nasm | 74 ++
>>  MdeModulePkg/Core/Pei/PeiMain.h   | 52 +
>>  MdeModulePkg/Core/Pei/PeiMain.inf | 15 
>>  9 files changed, 518 insertions(+), 21 deletions(-)
>>  create mode 100644 
>> MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.S
>>  create mode 100644 
>> MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.asm
>>  create mode 100644 
>> MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.S
>>  create mode 100644 
>> MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.asm
>>  create mode 100644 
>> MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
>>  create mode 100644 
>> MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
>>
>> --
>> 2.20.1
>>
>>
>> 
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#38831): https://edk2.groups.io/g/devel/message/38831
Mute This Topic: https://groups.io/mt/31016921/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration

2019-04-10 Thread Laszlo Ersek 
On 04/10/19 10:39, Jordan Justen wrote:
> https://github.com/jljusten/edk2.git temp-ram-support-v2
> 
> https://github.com/jljusten/edk2/commits/temp-ram-support-v2
> 
> v2:
>  * Add AARCH64 and ARM assembly
>  * Drop IA32 and X64 .S source files
>  * Adjust PEI_CORE_TEMPORARY_RAM_TRANSITION pointer in the assembly
>code based on the stack pointer change before & after
>TemporaryRamSupport->TemporaryRamMigration
>  * Drop extra cleanup patches for OvmfPkg & EmulatorPkg. These were
>just complicating the series.
> 
> 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
> "MdeModulePkg/Core/Pei: Add interface for assembly based
> TemporaryRamSupport" patch.
> 
> Testing:
> 
> I tested building and booting in several scenarios:
> 
> * OVMF IA32 & X64 on Linux

Performed my usual Linux checks on these, as described in
.

For building, I used the GCC48 toolchain.

> * ArmVirtPkg AARCH64 & ARM on x86_64 Linux

Performed a normal boot test, on AARCH64 KVM.

For building, I used the GCC5 toolchain (using an oldie 6.1.1
cross-compiler from x86_64).

For the series,

Regression-tested-by: Laszlo Ersek 


If a nontrivial update is needed for the series, I don't mind retesting,
but only as the last action before pushing. Testing is time consuming
and now I'm asking myself if I should have waited for some feedback to
arrive first. :)

Thanks
Laszlo

> * EmulatorPkg IA32 & X64 on Linux
> 
> Untested:
> 
> * My system does not reproduce the issue that Liu Yu reported with
>   EmulatorPkg, so I can't say that I have verified that issue.
> * Building on windows
> * AARCH64/ARM TemporaryRamMigration.asm sources
> 
> Cc: Liu Yu 
> Cc: Ray Ni 
> Cc: Andrew Fish 
> Cc: Laszlo Ersek 
> Cc: Leif Lindholm 
> Cc: Michael D Kinney 
> 
> Jordan Justen (6):
>   MdeModulePkg/Core/Pei: Add interface for assembly based
> TemporaryRamSupport
>   MdeModulePkg/Core/Pei: Add AARCH64 assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Add ARM assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Add IA32 assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Add X64 assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Use code path for assembly based
> TemporaryRamSupport
> 
>  .../AArch64/TemporaryRamMigration.S   | 63 +++
>  .../AArch64/TemporaryRamMigration.asm | 63 +++
>  .../Dispatcher/Arm/TemporaryRamMigration.S| 68 
>  .../Dispatcher/Arm/TemporaryRamMigration.asm  | 68 
>  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 59 +-
>  .../Ia32/TemporaryRamMigration.nasm   | 77 +++
>  .../Dispatcher/X64/TemporaryRamMigration.nasm | 74 ++
>  MdeModulePkg/Core/Pei/PeiMain.h   | 52 +
>  MdeModulePkg/Core/Pei/PeiMain.inf | 15 
>  9 files changed, 518 insertions(+), 21 deletions(-)
>  create mode 100644 
> MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.S
>  create mode 100644 
> MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.asm
>  create mode 100644 
> MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.S
>  create mode 100644 
> MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.asm
>  create mode 100644 
> MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
>  create mode 100644 
> MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#38828): https://edk2.groups.io/g/devel/message/38828
Mute This Topic: https://groups.io/mt/31016921/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration

2019-04-10 Thread Ard Biesheuvel 
On Wed, 10 Apr 2019 at 01:41, Jordan Justen  wrote:
>
> https://github.com/jljusten/edk2.git temp-ram-support-v2
>
> https://github.com/jljusten/edk2/commits/temp-ram-support-v2
>
> v2:
>  * Add AARCH64 and ARM assembly

Hi Jordan,

I'm not sure I'm following the reasoning behind this. Does this fix an
issue we currently have on ARM systems? And how did you build and/or
test OVMF for ARM?


>  * Drop IA32 and X64 .S source files
>  * Adjust PEI_CORE_TEMPORARY_RAM_TRANSITION pointer in the assembly
>code based on the stack pointer change before & after
>TemporaryRamSupport->TemporaryRamMigration
>  * Drop extra cleanup patches for OvmfPkg & EmulatorPkg. These were
>just complicating the series.
>
> 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
> "MdeModulePkg/Core/Pei: Add interface for assembly based
> TemporaryRamSupport" patch.
>
> Testing:
>
> I tested building and booting in several scenarios:
>
> * OVMF IA32 & X64 on Linux
> * ArmVirtPkg AARCH64 & ARM on x86_64 Linux
> * EmulatorPkg IA32 & X64 on Linux
>
> Untested:
>
> * My system does not reproduce the issue that Liu Yu reported with
>   EmulatorPkg, so I can't say that I have verified that issue.
> * Building on windows
> * AARCH64/ARM TemporaryRamMigration.asm sources
>
> Cc: Liu Yu 
> Cc: Ray Ni 
> Cc: Andrew Fish 
> Cc: Laszlo Ersek 
> Cc: Leif Lindholm 
> Cc: Michael D Kinney 
>
> Jordan Justen (6):
>   MdeModulePkg/Core/Pei: Add interface for assembly based
> TemporaryRamSupport
>   MdeModulePkg/Core/Pei: Add AARCH64 assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Add ARM assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Add IA32 assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Add X64 assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Use code path for assembly based
> TemporaryRamSupport
>
>  .../AArch64/TemporaryRamMigration.S   | 63 +++
>  .../AArch64/TemporaryRamMigration.asm | 63 +++
>  .../Dispatcher/Arm/TemporaryRamMigration.S| 68 
>  .../Dispatcher/Arm/TemporaryRamMigration.asm  | 68 
>  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 59 +-
>  .../Ia32/TemporaryRamMigration.nasm   | 77 +++
>  .../Dispatcher/X64/TemporaryRamMigration.nasm | 74 ++
>  MdeModulePkg/Core/Pei/PeiMain.h   | 52 +
>  MdeModulePkg/Core/Pei/PeiMain.inf | 15 
>  9 files changed, 518 insertions(+), 21 deletions(-)
>  create mode 100644 
> MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.S
>  create mode 100644 
> MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.asm
>  create mode 100644 
> MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.S
>  create mode 100644 
> MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.asm
>  create mode 100644 
> MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
>  create mode 100644 
> MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
>
> --
> 2.20.1
>
>
> 
>

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#38826): https://edk2.groups.io/g/devel/message/38826
Mute This Topic: https://groups.io/mt/31016921/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-