public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
@ 2017-10-20 11:23 Ard Biesheuvel
  2017-10-20 13:00 ` Leif Lindholm
  0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2017-10-20 11:23 UTC (permalink / raw)
  To: edk2-devel; +Cc: leif.lindholm, lersek, Ard Biesheuvel

DEBUG builds of PEI code will print a diagnostic message regarding
the utilization of temporary RAM before switching to permanent RAM.
For example,

  Total temporary memory:    16352 bytes.
    temporary memory stack ever used:       4820 bytes.
    temporary memory heap used for HobList: 4720 bytes.

Tracking stack utilization like this requires the stack to be seeded
with a known magic value, and this needs to occur before entering C
code, given that it uses the stack. Currently, only Nt32Pkg appears
to implement this feature, but it is useful nonetheless, so let's
wire it up for PrePeiCore.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S |  7 +++++++
 ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S     | 10 ++++++++++
 ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm   | 10 ++++++++++
 3 files changed, 27 insertions(+)

diff --git a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
index aab5edab0c42..7a33e2754869 100644
--- a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
+++ b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
@@ -13,6 +13,8 @@
 
 #include <AsmMacroIoLibV8.h>
 
+#define INIT_CAR_VALUE      0x5AA55AA55AA55AA5
+
 ASM_FUNC(_ModuleEntryPoint)
   // Do early platform specific actions
   bl    ASM_PFX(ArmPlatformPeiBootAction)
@@ -84,4 +86,9 @@ _PrepareArguments:
 
 _SetupPrimaryCoreStack:
   mov   sp, x1
+  MOV64 (x8, FixedPcdGet64(PcdCPUCoresStackBase))
+  MOV64 (x9, INIT_CAR_VALUE)
+0:stp   x9, x9, [x8], #16
+  cmp   x8, x1
+  b.lt  0b
   b     _PrepareArguments
diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
index 14344425ad4c..7342e49bea59 100644
--- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
+++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
@@ -13,6 +13,8 @@
 
 #include <AsmMacroIoLib.h>
 
+#define INIT_CAR_VALUE      0x5AA55AA5
+
 ASM_FUNC(_ModuleEntryPoint)
   // Do early platform specific actions
   bl    ASM_PFX(ArmPlatformPeiBootAction)
@@ -65,6 +67,14 @@ _PrepareArguments:
 
 _SetupPrimaryCoreStack:
   mov   sp, r1
+  MOV32 (r8, FixedPcdGet64(PcdCPUCoresStackBase))
+  MOV32 (r9, INIT_CAR_VALUE)
+  mov   r10, r9
+  mov   r11, r9
+  mov   r12, r9
+0:stm   r8!, {r9-r12}
+  cmp   r8, r1
+  blt   0b
   b     _PrepareArguments
 
 _NeverReturn:
diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
index abea675828df..7455de8aa66e 100644
--- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
+++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
@@ -13,6 +13,8 @@
 
 #include <AutoGen.h>
 
+#define INIT_CAR_VALUE      0x5AA55AA5
+
   INCLUDE AsmMacroIoLib.inc
 
   IMPORT  CEntryPoint
@@ -79,6 +81,14 @@ _PrepareArguments
 
 _SetupPrimaryCoreStack
   mov   sp, r1
+  mov32 r8, FixedPcdGet64(PcdCPUCoresStackBase)
+  mov32 r9, INIT_CAR_VALUE
+  mov   r10, r9
+  mov   r11, r9
+  mov   r12, r9
+0:stm   r8!, {r9-r12}
+  cmp   r8, r1
+  blt   0b
   b     _PrepareArguments
 
 _NeverReturn
-- 
2.11.0



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
  2017-10-20 11:23 [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core Ard Biesheuvel
@ 2017-10-20 13:00 ` Leif Lindholm
  2017-10-20 15:43   ` Laszlo Ersek
  0 siblings, 1 reply; 12+ messages in thread
From: Leif Lindholm @ 2017-10-20 13:00 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel, lersek

On Fri, Oct 20, 2017 at 12:23:25PM +0100, Ard Biesheuvel wrote:
> DEBUG builds of PEI code will print a diagnostic message regarding
> the utilization of temporary RAM before switching to permanent RAM.
> For example,
> 
>   Total temporary memory:    16352 bytes.
>     temporary memory stack ever used:       4820 bytes.
>     temporary memory heap used for HobList: 4720 bytes.
> 
> Tracking stack utilization like this requires the stack to be seeded
> with a known magic value, and this needs to occur before entering C
> code, given that it uses the stack. Currently, only Nt32Pkg appears
> to implement this feature, but it is useful nonetheless, so let's
> wire it up for PrePeiCore.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S |  7 +++++++
>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S     | 10 ++++++++++
>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm   | 10 ++++++++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
> index aab5edab0c42..7a33e2754869 100644
> --- a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
> +++ b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
> @@ -13,6 +13,8 @@
>  
>  #include <AsmMacroIoLibV8.h>
>  
> +#define INIT_CAR_VALUE      0x5AA55AA55AA55AA5
> +
>  ASM_FUNC(_ModuleEntryPoint)
>    // Do early platform specific actions
>    bl    ASM_PFX(ArmPlatformPeiBootAction)
> @@ -84,4 +86,9 @@ _PrepareArguments:
>  
>  _SetupPrimaryCoreStack:
>    mov   sp, x1
> +  MOV64 (x8, FixedPcdGet64(PcdCPUCoresStackBase))
> +  MOV64 (x9, INIT_CAR_VALUE)
> +0:stp   x9, x9, [x8], #16
> +  cmp   x8, x1
> +  b.lt  0b
>    b     _PrepareArguments
> diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
> index 14344425ad4c..7342e49bea59 100644
> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
> @@ -13,6 +13,8 @@
>  
>  #include <AsmMacroIoLib.h>
>  
> +#define INIT_CAR_VALUE      0x5AA55AA5
> +

Worth moving to a common header somewhere?

Also defined/used in MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c.

That file has an explicit comment saying "temporary memory is filled
with this initial value during SEC phase". Should this end have a
corresponding comment saying "checked for during PEI phase"?

/
    Leif

>  ASM_FUNC(_ModuleEntryPoint)
>    // Do early platform specific actions
>    bl    ASM_PFX(ArmPlatformPeiBootAction)
> @@ -65,6 +67,14 @@ _PrepareArguments:
>  
>  _SetupPrimaryCoreStack:
>    mov   sp, r1
> +  MOV32 (r8, FixedPcdGet64(PcdCPUCoresStackBase))
> +  MOV32 (r9, INIT_CAR_VALUE)
> +  mov   r10, r9
> +  mov   r11, r9
> +  mov   r12, r9
> +0:stm   r8!, {r9-r12}
> +  cmp   r8, r1
> +  blt   0b
>    b     _PrepareArguments
>  
>  _NeverReturn:
> diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
> index abea675828df..7455de8aa66e 100644
> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
> @@ -13,6 +13,8 @@
>  
>  #include <AutoGen.h>
>  
> +#define INIT_CAR_VALUE      0x5AA55AA5
> +
>    INCLUDE AsmMacroIoLib.inc
>  
>    IMPORT  CEntryPoint
> @@ -79,6 +81,14 @@ _PrepareArguments
>  
>  _SetupPrimaryCoreStack
>    mov   sp, r1
> +  mov32 r8, FixedPcdGet64(PcdCPUCoresStackBase)
> +  mov32 r9, INIT_CAR_VALUE
> +  mov   r10, r9
> +  mov   r11, r9
> +  mov   r12, r9
> +0:stm   r8!, {r9-r12}
> +  cmp   r8, r1
> +  blt   0b
>    b     _PrepareArguments
>  
>  _NeverReturn
> -- 
> 2.11.0
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
  2017-10-20 13:00 ` Leif Lindholm
@ 2017-10-20 15:43   ` Laszlo Ersek
  2017-10-20 16:10     ` Ard Biesheuvel
  0 siblings, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2017-10-20 15:43 UTC (permalink / raw)
  To: Leif Lindholm, Ard Biesheuvel; +Cc: edk2-devel

On 10/20/17 15:00, Leif Lindholm wrote:
> On Fri, Oct 20, 2017 at 12:23:25PM +0100, Ard Biesheuvel wrote:
>> DEBUG builds of PEI code will print a diagnostic message regarding
>> the utilization of temporary RAM before switching to permanent RAM.
>> For example,
>>
>>   Total temporary memory:    16352 bytes.
>>     temporary memory stack ever used:       4820 bytes.
>>     temporary memory heap used for HobList: 4720 bytes.
>>
>> Tracking stack utilization like this requires the stack to be seeded
>> with a known magic value, and this needs to occur before entering C
>> code, given that it uses the stack. Currently, only Nt32Pkg appears
>> to implement this feature, but it is useful nonetheless, so let's
>> wire it up for PrePeiCore.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S |  7 +++++++
>>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S     | 10 ++++++++++
>>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm   | 10 ++++++++++
>>  3 files changed, 27 insertions(+)
>>
>> diff --git a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>> index aab5edab0c42..7a33e2754869 100644
>> --- a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>> +++ b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>> @@ -13,6 +13,8 @@
>>  
>>  #include <AsmMacroIoLibV8.h>
>>  
>> +#define INIT_CAR_VALUE      0x5AA55AA55AA55AA5
>> +
>>  ASM_FUNC(_ModuleEntryPoint)
>>    // Do early platform specific actions
>>    bl    ASM_PFX(ArmPlatformPeiBootAction)
>> @@ -84,4 +86,9 @@ _PrepareArguments:
>>  
>>  _SetupPrimaryCoreStack:
>>    mov   sp, x1
>> +  MOV64 (x8, FixedPcdGet64(PcdCPUCoresStackBase))
>> +  MOV64 (x9, INIT_CAR_VALUE)
>> +0:stp   x9, x9, [x8], #16
>> +  cmp   x8, x1
>> +  b.lt  0b
>>    b     _PrepareArguments
>> diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
>> index 14344425ad4c..7342e49bea59 100644
>> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
>> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
>> @@ -13,6 +13,8 @@
>>  
>>  #include <AsmMacroIoLib.h>
>>  
>> +#define INIT_CAR_VALUE      0x5AA55AA5
>> +
> 
> Worth moving to a common header somewhere?
> 
> Also defined/used in MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c.

Furthermore, open-coded in:

EmulatorPkg/Unix/Host/Host.c:    *StackPointer = 0x5AA55AA5;
Nt32Pkg/Sec/SecMain.c:    *StackPointer = 0x5AA55AA5;

Honestly I think it should be a Fixed-At-Build PCD, in MdeModulePkg.dec,
similarly to

  gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue

in MdePkg.dec.

I'm unhappy that we have to annoy Ard with a request to "upstream" this
constant to MdeModulePkg in some form, but we'll need it yet again in
OVMF...

> 
> That file has an explicit comment saying "temporary memory is filled
> with this initial value during SEC phase". Should this end have a
> corresponding comment saying "checked for during PEI phase"?

Thanks
Laszlo

> 
> /
>     Leif
> 
>>  ASM_FUNC(_ModuleEntryPoint)
>>    // Do early platform specific actions
>>    bl    ASM_PFX(ArmPlatformPeiBootAction)
>> @@ -65,6 +67,14 @@ _PrepareArguments:
>>  
>>  _SetupPrimaryCoreStack:
>>    mov   sp, r1
>> +  MOV32 (r8, FixedPcdGet64(PcdCPUCoresStackBase))
>> +  MOV32 (r9, INIT_CAR_VALUE)
>> +  mov   r10, r9
>> +  mov   r11, r9
>> +  mov   r12, r9
>> +0:stm   r8!, {r9-r12}
>> +  cmp   r8, r1
>> +  blt   0b
>>    b     _PrepareArguments
>>  
>>  _NeverReturn:
>> diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
>> index abea675828df..7455de8aa66e 100644
>> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
>> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
>> @@ -13,6 +13,8 @@
>>  
>>  #include <AutoGen.h>
>>  
>> +#define INIT_CAR_VALUE      0x5AA55AA5
>> +
>>    INCLUDE AsmMacroIoLib.inc
>>  
>>    IMPORT  CEntryPoint
>> @@ -79,6 +81,14 @@ _PrepareArguments
>>  
>>  _SetupPrimaryCoreStack
>>    mov   sp, r1
>> +  mov32 r8, FixedPcdGet64(PcdCPUCoresStackBase)
>> +  mov32 r9, INIT_CAR_VALUE
>> +  mov   r10, r9
>> +  mov   r11, r9
>> +  mov   r12, r9
>> +0:stm   r8!, {r9-r12}
>> +  cmp   r8, r1
>> +  blt   0b
>>    b     _PrepareArguments
>>  
>>  _NeverReturn
>> -- 
>> 2.11.0
>>



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
  2017-10-20 15:43   ` Laszlo Ersek
@ 2017-10-20 16:10     ` Ard Biesheuvel
  2017-10-20 16:30       ` Laszlo Ersek
  2017-10-20 16:37       ` Gao, Liming
  0 siblings, 2 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2017-10-20 16:10 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: Leif Lindholm, edk2-devel@lists.01.org

On 20 October 2017 at 16:43, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/20/17 15:00, Leif Lindholm wrote:
>> On Fri, Oct 20, 2017 at 12:23:25PM +0100, Ard Biesheuvel wrote:
>>> DEBUG builds of PEI code will print a diagnostic message regarding
>>> the utilization of temporary RAM before switching to permanent RAM.
>>> For example,
>>>
>>>   Total temporary memory:    16352 bytes.
>>>     temporary memory stack ever used:       4820 bytes.
>>>     temporary memory heap used for HobList: 4720 bytes.
>>>
>>> Tracking stack utilization like this requires the stack to be seeded
>>> with a known magic value, and this needs to occur before entering C
>>> code, given that it uses the stack. Currently, only Nt32Pkg appears
>>> to implement this feature, but it is useful nonetheless, so let's
>>> wire it up for PrePeiCore.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S |  7 +++++++
>>>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S     | 10 ++++++++++
>>>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm   | 10 ++++++++++
>>>  3 files changed, 27 insertions(+)
>>>
>>> diff --git a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>>> index aab5edab0c42..7a33e2754869 100644
>>> --- a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>>> +++ b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>>> @@ -13,6 +13,8 @@
>>>
>>>  #include <AsmMacroIoLibV8.h>
>>>
>>> +#define INIT_CAR_VALUE      0x5AA55AA55AA55AA5
>>> +
>>>  ASM_FUNC(_ModuleEntryPoint)
>>>    // Do early platform specific actions
>>>    bl    ASM_PFX(ArmPlatformPeiBootAction)
>>> @@ -84,4 +86,9 @@ _PrepareArguments:
>>>
>>>  _SetupPrimaryCoreStack:
>>>    mov   sp, x1
>>> +  MOV64 (x8, FixedPcdGet64(PcdCPUCoresStackBase))
>>> +  MOV64 (x9, INIT_CAR_VALUE)
>>> +0:stp   x9, x9, [x8], #16
>>> +  cmp   x8, x1
>>> +  b.lt  0b
>>>    b     _PrepareArguments
>>> diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
>>> index 14344425ad4c..7342e49bea59 100644
>>> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
>>> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
>>> @@ -13,6 +13,8 @@
>>>
>>>  #include <AsmMacroIoLib.h>
>>>
>>> +#define INIT_CAR_VALUE      0x5AA55AA5
>>> +
>>
>> Worth moving to a common header somewhere?
>>
>> Also defined/used in MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c.
>
> Furthermore, open-coded in:
>
> EmulatorPkg/Unix/Host/Host.c:    *StackPointer = 0x5AA55AA5;
> Nt32Pkg/Sec/SecMain.c:    *StackPointer = 0x5AA55AA5;
>
> Honestly I think it should be a Fixed-At-Build PCD, in MdeModulePkg.dec,
> similarly to
>
>   gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue
>
> in MdePkg.dec.
>

Yes. And you both know how the MdeModulePkg maintainers are going to
respond if I propose adding another PCD.

> I'm unhappy that we have to annoy Ard with a request to "upstream" this
> constant to MdeModulePkg in some form, but we'll need it yet again in
> OVMF...
>


>>
>> That file has an explicit comment saying "temporary memory is filled
>> with this initial value during SEC phase". Should this end have a
>> corresponding comment saying "checked for during PEI phase"?
>
> Thanks
> Laszlo
>
>>
>> /
>>     Leif
>>
>>>  ASM_FUNC(_ModuleEntryPoint)
>>>    // Do early platform specific actions
>>>    bl    ASM_PFX(ArmPlatformPeiBootAction)
>>> @@ -65,6 +67,14 @@ _PrepareArguments:
>>>
>>>  _SetupPrimaryCoreStack:
>>>    mov   sp, r1
>>> +  MOV32 (r8, FixedPcdGet64(PcdCPUCoresStackBase))
>>> +  MOV32 (r9, INIT_CAR_VALUE)
>>> +  mov   r10, r9
>>> +  mov   r11, r9
>>> +  mov   r12, r9
>>> +0:stm   r8!, {r9-r12}
>>> +  cmp   r8, r1
>>> +  blt   0b
>>>    b     _PrepareArguments
>>>
>>>  _NeverReturn:
>>> diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
>>> index abea675828df..7455de8aa66e 100644
>>> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
>>> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
>>> @@ -13,6 +13,8 @@
>>>
>>>  #include <AutoGen.h>
>>>
>>> +#define INIT_CAR_VALUE      0x5AA55AA5
>>> +
>>>    INCLUDE AsmMacroIoLib.inc
>>>
>>>    IMPORT  CEntryPoint
>>> @@ -79,6 +81,14 @@ _PrepareArguments
>>>
>>>  _SetupPrimaryCoreStack
>>>    mov   sp, r1
>>> +  mov32 r8, FixedPcdGet64(PcdCPUCoresStackBase)
>>> +  mov32 r9, INIT_CAR_VALUE
>>> +  mov   r10, r9
>>> +  mov   r11, r9
>>> +  mov   r12, r9
>>> +0:stm   r8!, {r9-r12}
>>> +  cmp   r8, r1
>>> +  blt   0b
>>>    b     _PrepareArguments
>>>
>>>  _NeverReturn
>>> --
>>> 2.11.0
>>>
>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
  2017-10-20 16:10     ` Ard Biesheuvel
@ 2017-10-20 16:30       ` Laszlo Ersek
  2017-10-20 16:37       ` Gao, Liming
  1 sibling, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2017-10-20 16:30 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Leif Lindholm, edk2-devel@lists.01.org

On 10/20/17 18:10, Ard Biesheuvel wrote:
> On 20 October 2017 at 16:43, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 10/20/17 15:00, Leif Lindholm wrote:
>>> On Fri, Oct 20, 2017 at 12:23:25PM +0100, Ard Biesheuvel wrote:
>>>> DEBUG builds of PEI code will print a diagnostic message regarding
>>>> the utilization of temporary RAM before switching to permanent RAM.
>>>> For example,
>>>>
>>>>   Total temporary memory:    16352 bytes.
>>>>     temporary memory stack ever used:       4820 bytes.
>>>>     temporary memory heap used for HobList: 4720 bytes.
>>>>
>>>> Tracking stack utilization like this requires the stack to be seeded
>>>> with a known magic value, and this needs to occur before entering C
>>>> code, given that it uses the stack. Currently, only Nt32Pkg appears
>>>> to implement this feature, but it is useful nonetheless, so let's
>>>> wire it up for PrePeiCore.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>>>>  ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S |  7 +++++++
>>>>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S     | 10 ++++++++++
>>>>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm   | 10 ++++++++++
>>>>  3 files changed, 27 insertions(+)
>>>>
>>>> diff --git a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>>>> index aab5edab0c42..7a33e2754869 100644
>>>> --- a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>>>> +++ b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>>>> @@ -13,6 +13,8 @@
>>>>
>>>>  #include <AsmMacroIoLibV8.h>
>>>>
>>>> +#define INIT_CAR_VALUE      0x5AA55AA55AA55AA5
>>>> +
>>>>  ASM_FUNC(_ModuleEntryPoint)
>>>>    // Do early platform specific actions
>>>>    bl    ASM_PFX(ArmPlatformPeiBootAction)
>>>> @@ -84,4 +86,9 @@ _PrepareArguments:
>>>>
>>>>  _SetupPrimaryCoreStack:
>>>>    mov   sp, x1
>>>> +  MOV64 (x8, FixedPcdGet64(PcdCPUCoresStackBase))
>>>> +  MOV64 (x9, INIT_CAR_VALUE)
>>>> +0:stp   x9, x9, [x8], #16
>>>> +  cmp   x8, x1
>>>> +  b.lt  0b
>>>>    b     _PrepareArguments
>>>> diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
>>>> index 14344425ad4c..7342e49bea59 100644
>>>> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
>>>> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
>>>> @@ -13,6 +13,8 @@
>>>>
>>>>  #include <AsmMacroIoLib.h>
>>>>
>>>> +#define INIT_CAR_VALUE      0x5AA55AA5
>>>> +
>>>
>>> Worth moving to a common header somewhere?
>>>
>>> Also defined/used in MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c.
>>
>> Furthermore, open-coded in:
>>
>> EmulatorPkg/Unix/Host/Host.c:    *StackPointer = 0x5AA55AA5;
>> Nt32Pkg/Sec/SecMain.c:    *StackPointer = 0x5AA55AA5;
>>
>> Honestly I think it should be a Fixed-At-Build PCD, in MdeModulePkg.dec,
>> similarly to
>>
>>   gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue
>>
>> in MdePkg.dec.
>>
> 
> Yes. And you both know how the MdeModulePkg maintainers are going to
> respond if I propose adding another PCD.

Yes, I can certainly see myself on the "wrong end" of that patch. :(

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> 
>> I'm unhappy that we have to annoy Ard with a request to "upstream" this
>> constant to MdeModulePkg in some form, but we'll need it yet again in
>> OVMF...
>>
> 
> 
>>>
>>> That file has an explicit comment saying "temporary memory is filled
>>> with this initial value during SEC phase". Should this end have a
>>> corresponding comment saying "checked for during PEI phase"?
>>
>> Thanks
>> Laszlo
>>
>>>
>>> /
>>>     Leif
>>>
>>>>  ASM_FUNC(_ModuleEntryPoint)
>>>>    // Do early platform specific actions
>>>>    bl    ASM_PFX(ArmPlatformPeiBootAction)
>>>> @@ -65,6 +67,14 @@ _PrepareArguments:
>>>>
>>>>  _SetupPrimaryCoreStack:
>>>>    mov   sp, r1
>>>> +  MOV32 (r8, FixedPcdGet64(PcdCPUCoresStackBase))
>>>> +  MOV32 (r9, INIT_CAR_VALUE)
>>>> +  mov   r10, r9
>>>> +  mov   r11, r9
>>>> +  mov   r12, r9
>>>> +0:stm   r8!, {r9-r12}
>>>> +  cmp   r8, r1
>>>> +  blt   0b
>>>>    b     _PrepareArguments
>>>>
>>>>  _NeverReturn:
>>>> diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
>>>> index abea675828df..7455de8aa66e 100644
>>>> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
>>>> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
>>>> @@ -13,6 +13,8 @@
>>>>
>>>>  #include <AutoGen.h>
>>>>
>>>> +#define INIT_CAR_VALUE      0x5AA55AA5
>>>> +
>>>>    INCLUDE AsmMacroIoLib.inc
>>>>
>>>>    IMPORT  CEntryPoint
>>>> @@ -79,6 +81,14 @@ _PrepareArguments
>>>>
>>>>  _SetupPrimaryCoreStack
>>>>    mov   sp, r1
>>>> +  mov32 r8, FixedPcdGet64(PcdCPUCoresStackBase)
>>>> +  mov32 r9, INIT_CAR_VALUE
>>>> +  mov   r10, r9
>>>> +  mov   r11, r9
>>>> +  mov   r12, r9
>>>> +0:stm   r8!, {r9-r12}
>>>> +  cmp   r8, r1
>>>> +  blt   0b
>>>>    b     _PrepareArguments
>>>>
>>>>  _NeverReturn
>>>> --
>>>> 2.11.0
>>>>
>>



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
  2017-10-20 16:10     ` Ard Biesheuvel
  2017-10-20 16:30       ` Laszlo Ersek
@ 2017-10-20 16:37       ` Gao, Liming
  2017-10-20 16:39         ` Ard Biesheuvel
  1 sibling, 1 reply; 12+ messages in thread
From: Gao, Liming @ 2017-10-20 16:37 UTC (permalink / raw)
  To: Ard Biesheuvel, Laszlo Ersek; +Cc: edk2-devel@lists.01.org, Leif Lindholm

Ard:
  This case is to share the same value between PeiCore and SecCore. I also think it will be better to define one fixed PCD in MdeModulePkg.dec for this value. Could you submit bugzillar to catch this issue first?

Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
> Sent: Saturday, October 21, 2017 12:10 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>
> Subject: Re: [edk2] [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
> 
> On 20 October 2017 at 16:43, Laszlo Ersek <lersek@redhat.com> wrote:
> > On 10/20/17 15:00, Leif Lindholm wrote:
> >> On Fri, Oct 20, 2017 at 12:23:25PM +0100, Ard Biesheuvel wrote:
> >>> DEBUG builds of PEI code will print a diagnostic message regarding
> >>> the utilization of temporary RAM before switching to permanent RAM.
> >>> For example,
> >>>
> >>>   Total temporary memory:    16352 bytes.
> >>>     temporary memory stack ever used:       4820 bytes.
> >>>     temporary memory heap used for HobList: 4720 bytes.
> >>>
> >>> Tracking stack utilization like this requires the stack to be seeded
> >>> with a known magic value, and this needs to occur before entering C
> >>> code, given that it uses the stack. Currently, only Nt32Pkg appears
> >>> to implement this feature, but it is useful nonetheless, so let's
> >>> wire it up for PrePeiCore.
> >>>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> ---
> >>>  ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S |  7 +++++++
> >>>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S     | 10 ++++++++++
> >>>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm   | 10 ++++++++++
> >>>  3 files changed, 27 insertions(+)
> >>>
> >>> diff --git a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
> b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
> >>> index aab5edab0c42..7a33e2754869 100644
> >>> --- a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
> >>> +++ b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
> >>> @@ -13,6 +13,8 @@
> >>>
> >>>  #include <AsmMacroIoLibV8.h>
> >>>
> >>> +#define INIT_CAR_VALUE      0x5AA55AA55AA55AA5
> >>> +
> >>>  ASM_FUNC(_ModuleEntryPoint)
> >>>    // Do early platform specific actions
> >>>    bl    ASM_PFX(ArmPlatformPeiBootAction)
> >>> @@ -84,4 +86,9 @@ _PrepareArguments:
> >>>
> >>>  _SetupPrimaryCoreStack:
> >>>    mov   sp, x1
> >>> +  MOV64 (x8, FixedPcdGet64(PcdCPUCoresStackBase))
> >>> +  MOV64 (x9, INIT_CAR_VALUE)
> >>> +0:stp   x9, x9, [x8], #16
> >>> +  cmp   x8, x1
> >>> +  b.lt  0b
> >>>    b     _PrepareArguments
> >>> diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
> >>> index 14344425ad4c..7342e49bea59 100644
> >>> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
> >>> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
> >>> @@ -13,6 +13,8 @@
> >>>
> >>>  #include <AsmMacroIoLib.h>
> >>>
> >>> +#define INIT_CAR_VALUE      0x5AA55AA5
> >>> +
> >>
> >> Worth moving to a common header somewhere?
> >>
> >> Also defined/used in MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c.
> >
> > Furthermore, open-coded in:
> >
> > EmulatorPkg/Unix/Host/Host.c:    *StackPointer = 0x5AA55AA5;
> > Nt32Pkg/Sec/SecMain.c:    *StackPointer = 0x5AA55AA5;
> >
> > Honestly I think it should be a Fixed-At-Build PCD, in MdeModulePkg.dec,
> > similarly to
> >
> >   gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue
> >
> > in MdePkg.dec.
> >
> 
> Yes. And you both know how the MdeModulePkg maintainers are going to
> respond if I propose adding another PCD.
> 
> > I'm unhappy that we have to annoy Ard with a request to "upstream" this
> > constant to MdeModulePkg in some form, but we'll need it yet again in
> > OVMF...
> >
> 
> 
> >>
> >> That file has an explicit comment saying "temporary memory is filled
> >> with this initial value during SEC phase". Should this end have a
> >> corresponding comment saying "checked for during PEI phase"?
> >
> > Thanks
> > Laszlo
> >
> >>
> >> /
> >>     Leif
> >>
> >>>  ASM_FUNC(_ModuleEntryPoint)
> >>>    // Do early platform specific actions
> >>>    bl    ASM_PFX(ArmPlatformPeiBootAction)
> >>> @@ -65,6 +67,14 @@ _PrepareArguments:
> >>>
> >>>  _SetupPrimaryCoreStack:
> >>>    mov   sp, r1
> >>> +  MOV32 (r8, FixedPcdGet64(PcdCPUCoresStackBase))
> >>> +  MOV32 (r9, INIT_CAR_VALUE)
> >>> +  mov   r10, r9
> >>> +  mov   r11, r9
> >>> +  mov   r12, r9
> >>> +0:stm   r8!, {r9-r12}
> >>> +  cmp   r8, r1
> >>> +  blt   0b
> >>>    b     _PrepareArguments
> >>>
> >>>  _NeverReturn:
> >>> diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
> b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
> >>> index abea675828df..7455de8aa66e 100644
> >>> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
> >>> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
> >>> @@ -13,6 +13,8 @@
> >>>
> >>>  #include <AutoGen.h>
> >>>
> >>> +#define INIT_CAR_VALUE      0x5AA55AA5
> >>> +
> >>>    INCLUDE AsmMacroIoLib.inc
> >>>
> >>>    IMPORT  CEntryPoint
> >>> @@ -79,6 +81,14 @@ _PrepareArguments
> >>>
> >>>  _SetupPrimaryCoreStack
> >>>    mov   sp, r1
> >>> +  mov32 r8, FixedPcdGet64(PcdCPUCoresStackBase)
> >>> +  mov32 r9, INIT_CAR_VALUE
> >>> +  mov   r10, r9
> >>> +  mov   r11, r9
> >>> +  mov   r12, r9
> >>> +0:stm   r8!, {r9-r12}
> >>> +  cmp   r8, r1
> >>> +  blt   0b
> >>>    b     _PrepareArguments
> >>>
> >>>  _NeverReturn
> >>> --
> >>> 2.11.0
> >>>
> >
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
  2017-10-20 16:37       ` Gao, Liming
@ 2017-10-20 16:39         ` Ard Biesheuvel
  2017-10-20 16:51           ` Laszlo Ersek
  0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2017-10-20 16:39 UTC (permalink / raw)
  To: Gao, Liming; +Cc: Laszlo Ersek, edk2-devel@lists.01.org, Leif Lindholm

On 20 October 2017 at 17:37, Gao, Liming <liming.gao@intel.com> wrote:
> Ard:
>   This case is to share the same value between PeiCore and SecCore. I also think it will be better to define one fixed PCD in MdeModulePkg.dec for this value. Could you submit bugzillar to catch this issue first?
>

Certainly!


>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
>> Sent: Saturday, October 21, 2017 12:10 AM
>> To: Laszlo Ersek <lersek@redhat.com>
>> Cc: edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>
>> Subject: Re: [edk2] [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
>>
>> On 20 October 2017 at 16:43, Laszlo Ersek <lersek@redhat.com> wrote:
>> > On 10/20/17 15:00, Leif Lindholm wrote:
>> >> On Fri, Oct 20, 2017 at 12:23:25PM +0100, Ard Biesheuvel wrote:
>> >>> DEBUG builds of PEI code will print a diagnostic message regarding
>> >>> the utilization of temporary RAM before switching to permanent RAM.
>> >>> For example,
>> >>>
>> >>>   Total temporary memory:    16352 bytes.
>> >>>     temporary memory stack ever used:       4820 bytes.
>> >>>     temporary memory heap used for HobList: 4720 bytes.
>> >>>
>> >>> Tracking stack utilization like this requires the stack to be seeded
>> >>> with a known magic value, and this needs to occur before entering C
>> >>> code, given that it uses the stack. Currently, only Nt32Pkg appears
>> >>> to implement this feature, but it is useful nonetheless, so let's
>> >>> wire it up for PrePeiCore.
>> >>>
>> >>> Contributed-under: TianoCore Contribution Agreement 1.1
>> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >>> ---
>> >>>  ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S |  7 +++++++
>> >>>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S     | 10 ++++++++++
>> >>>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm   | 10 ++++++++++
>> >>>  3 files changed, 27 insertions(+)
>> >>>
>> >>> diff --git a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>> b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>> >>> index aab5edab0c42..7a33e2754869 100644
>> >>> --- a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>> >>> +++ b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>> >>> @@ -13,6 +13,8 @@
>> >>>
>> >>>  #include <AsmMacroIoLibV8.h>
>> >>>
>> >>> +#define INIT_CAR_VALUE      0x5AA55AA55AA55AA5
>> >>> +
>> >>>  ASM_FUNC(_ModuleEntryPoint)
>> >>>    // Do early platform specific actions
>> >>>    bl    ASM_PFX(ArmPlatformPeiBootAction)
>> >>> @@ -84,4 +86,9 @@ _PrepareArguments:
>> >>>
>> >>>  _SetupPrimaryCoreStack:
>> >>>    mov   sp, x1
>> >>> +  MOV64 (x8, FixedPcdGet64(PcdCPUCoresStackBase))
>> >>> +  MOV64 (x9, INIT_CAR_VALUE)
>> >>> +0:stp   x9, x9, [x8], #16
>> >>> +  cmp   x8, x1
>> >>> +  b.lt  0b
>> >>>    b     _PrepareArguments
>> >>> diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
>> >>> index 14344425ad4c..7342e49bea59 100644
>> >>> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
>> >>> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
>> >>> @@ -13,6 +13,8 @@
>> >>>
>> >>>  #include <AsmMacroIoLib.h>
>> >>>
>> >>> +#define INIT_CAR_VALUE      0x5AA55AA5
>> >>> +
>> >>
>> >> Worth moving to a common header somewhere?
>> >>
>> >> Also defined/used in MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c.
>> >
>> > Furthermore, open-coded in:
>> >
>> > EmulatorPkg/Unix/Host/Host.c:    *StackPointer = 0x5AA55AA5;
>> > Nt32Pkg/Sec/SecMain.c:    *StackPointer = 0x5AA55AA5;
>> >
>> > Honestly I think it should be a Fixed-At-Build PCD, in MdeModulePkg.dec,
>> > similarly to
>> >
>> >   gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue
>> >
>> > in MdePkg.dec.
>> >
>>
>> Yes. And you both know how the MdeModulePkg maintainers are going to
>> respond if I propose adding another PCD.
>>
>> > I'm unhappy that we have to annoy Ard with a request to "upstream" this
>> > constant to MdeModulePkg in some form, but we'll need it yet again in
>> > OVMF...
>> >
>>
>>
>> >>
>> >> That file has an explicit comment saying "temporary memory is filled
>> >> with this initial value during SEC phase". Should this end have a
>> >> corresponding comment saying "checked for during PEI phase"?
>> >
>> > Thanks
>> > Laszlo
>> >
>> >>
>> >> /
>> >>     Leif
>> >>
>> >>>  ASM_FUNC(_ModuleEntryPoint)
>> >>>    // Do early platform specific actions
>> >>>    bl    ASM_PFX(ArmPlatformPeiBootAction)
>> >>> @@ -65,6 +67,14 @@ _PrepareArguments:
>> >>>
>> >>>  _SetupPrimaryCoreStack:
>> >>>    mov   sp, r1
>> >>> +  MOV32 (r8, FixedPcdGet64(PcdCPUCoresStackBase))
>> >>> +  MOV32 (r9, INIT_CAR_VALUE)
>> >>> +  mov   r10, r9
>> >>> +  mov   r11, r9
>> >>> +  mov   r12, r9
>> >>> +0:stm   r8!, {r9-r12}
>> >>> +  cmp   r8, r1
>> >>> +  blt   0b
>> >>>    b     _PrepareArguments
>> >>>
>> >>>  _NeverReturn:
>> >>> diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
>> b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
>> >>> index abea675828df..7455de8aa66e 100644
>> >>> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
>> >>> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
>> >>> @@ -13,6 +13,8 @@
>> >>>
>> >>>  #include <AutoGen.h>
>> >>>
>> >>> +#define INIT_CAR_VALUE      0x5AA55AA5
>> >>> +
>> >>>    INCLUDE AsmMacroIoLib.inc
>> >>>
>> >>>    IMPORT  CEntryPoint
>> >>> @@ -79,6 +81,14 @@ _PrepareArguments
>> >>>
>> >>>  _SetupPrimaryCoreStack
>> >>>    mov   sp, r1
>> >>> +  mov32 r8, FixedPcdGet64(PcdCPUCoresStackBase)
>> >>> +  mov32 r9, INIT_CAR_VALUE
>> >>> +  mov   r10, r9
>> >>> +  mov   r11, r9
>> >>> +  mov   r12, r9
>> >>> +0:stm   r8!, {r9-r12}
>> >>> +  cmp   r8, r1
>> >>> +  blt   0b
>> >>>    b     _PrepareArguments
>> >>>
>> >>>  _NeverReturn
>> >>> --
>> >>> 2.11.0
>> >>>
>> >
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
  2017-10-20 16:39         ` Ard Biesheuvel
@ 2017-10-20 16:51           ` Laszlo Ersek
  2017-10-20 16:52             ` Ard Biesheuvel
  0 siblings, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2017-10-20 16:51 UTC (permalink / raw)
  To: Ard Biesheuvel, Gao, Liming; +Cc: edk2-devel@lists.01.org, Leif Lindholm

On 10/20/17 18:39, Ard Biesheuvel wrote:
> On 20 October 2017 at 17:37, Gao, Liming <liming.gao@intel.com> wrote:
>> Ard:
>>   This case is to share the same value between PeiCore and SecCore. I also think it will be better to define one fixed PCD in MdeModulePkg.dec for this value. Could you submit bugzillar to catch this issue first?
>>
> 
> Certainly!

Would it be possible to define the PCD as UINT32, and task 64-bit SEC
(and PEI_CORE) code to first construct the wider value manually (in a
register or otherwise)?

Just thinking out loud.

Thanks!
Laszlo


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
  2017-10-20 16:51           ` Laszlo Ersek
@ 2017-10-20 16:52             ` Ard Biesheuvel
  2017-10-20 17:18               ` Laszlo Ersek
  0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2017-10-20 16:52 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: Gao, Liming, edk2-devel@lists.01.org, Leif Lindholm

On 20 October 2017 at 17:51, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/20/17 18:39, Ard Biesheuvel wrote:
>> On 20 October 2017 at 17:37, Gao, Liming <liming.gao@intel.com> wrote:
>>> Ard:
>>>   This case is to share the same value between PeiCore and SecCore. I also think it will be better to define one fixed PCD in MdeModulePkg.dec for this value. Could you submit bugzillar to catch this issue first?
>>>
>>
>> Certainly!
>
> Would it be possible to define the PCD as UINT32, and task 64-bit SEC
> (and PEI_CORE) code to first construct the wider value manually (in a
> register or otherwise)?
>
> Just thinking out loud.
>

Could you think the reasoning behind that out loud as well?


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
  2017-10-20 16:52             ` Ard Biesheuvel
@ 2017-10-20 17:18               ` Laszlo Ersek
  2017-10-23 14:18                 ` Gao, Liming
  0 siblings, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2017-10-20 17:18 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Gao, Liming, edk2-devel@lists.01.org, Leif Lindholm

On 10/20/17 18:52, Ard Biesheuvel wrote:
> On 20 October 2017 at 17:51, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 10/20/17 18:39, Ard Biesheuvel wrote:
>>> On 20 October 2017 at 17:37, Gao, Liming <liming.gao@intel.com> wrote:
>>>> Ard:
>>>>   This case is to share the same value between PeiCore and SecCore. I also think it will be better to define one fixed PCD in MdeModulePkg.dec for this value. Could you submit bugzillar to catch this issue first?
>>>>
>>>
>>> Certainly!
>>
>> Would it be possible to define the PCD as UINT32, and task 64-bit SEC
>> (and PEI_CORE) code to first construct the wider value manually (in a
>> register or otherwise)?
>>
>> Just thinking out loud.
>>
> 
> Could you think the reasoning behind that out loud as well?

Haha, good stab :) Sure.

In your patch you have:

+#define INIT_CAR_VALUE      0x5AA55AA55AA55AA5

for 64-bit, and

+#define INIT_CAR_VALUE      0x5AA55AA5

for 32-bit.

Both 64-bit assembly code in SEC, and 64-bit C-code in the PEI_CORE, can
easily compose the large value from the small value, starting from
FixedPcdGet32(). The alternatives are:

- asking the 32-bit assembly code to truncate the 64-bit constant -- it
won't compile,

- defining *two* FixedAtBuild PCDs, one for 32-bit, another for 64-bit
SEC -- an idea probably not universally liked.

... When I originally started writing my previous email, I even thought
about introducing the PCD as UINT16 :) But then I realized, if any
platform lacks *some* 32-bit mode when it sets up the stack in assembly,
for C-language entry, then the platform won't be supported by edk2.

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
  2017-10-20 17:18               ` Laszlo Ersek
@ 2017-10-23 14:18                 ` Gao, Liming
  2017-10-23 14:39                   ` Ard Biesheuvel
  0 siblings, 1 reply; 12+ messages in thread
From: Gao, Liming @ 2017-10-23 14:18 UTC (permalink / raw)
  To: Laszlo Ersek, Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Leif Lindholm

This is for debug purpose. I think UINT32 is OK if AARCH64 can operate 32bit value. 

Thanks
Liming
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Saturday, October 21, 2017 1:19 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>
> Subject: Re: [edk2] [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
> 
> On 10/20/17 18:52, Ard Biesheuvel wrote:
> > On 20 October 2017 at 17:51, Laszlo Ersek <lersek@redhat.com> wrote:
> >> On 10/20/17 18:39, Ard Biesheuvel wrote:
> >>> On 20 October 2017 at 17:37, Gao, Liming <liming.gao@intel.com> wrote:
> >>>> Ard:
> >>>>   This case is to share the same value between PeiCore and SecCore. I also think it will be better to define one fixed PCD in
> MdeModulePkg.dec for this value. Could you submit bugzillar to catch this issue first?
> >>>>
> >>>
> >>> Certainly!
> >>
> >> Would it be possible to define the PCD as UINT32, and task 64-bit SEC
> >> (and PEI_CORE) code to first construct the wider value manually (in a
> >> register or otherwise)?
> >>
> >> Just thinking out loud.
> >>
> >
> > Could you think the reasoning behind that out loud as well?
> 
> Haha, good stab :) Sure.
> 
> In your patch you have:
> 
> +#define INIT_CAR_VALUE      0x5AA55AA55AA55AA5
> 
> for 64-bit, and
> 
> +#define INIT_CAR_VALUE      0x5AA55AA5
> 
> for 32-bit.
> 
> Both 64-bit assembly code in SEC, and 64-bit C-code in the PEI_CORE, can
> easily compose the large value from the small value, starting from
> FixedPcdGet32(). The alternatives are:
> 
> - asking the 32-bit assembly code to truncate the 64-bit constant -- it
> won't compile,
> 
> - defining *two* FixedAtBuild PCDs, one for 32-bit, another for 64-bit
> SEC -- an idea probably not universally liked.
> 
> ... When I originally started writing my previous email, I even thought
> about introducing the PCD as UINT16 :) But then I realized, if any
> platform lacks *some* 32-bit mode when it sets up the stack in assembly,
> for C-language entry, then the platform won't be supported by edk2.
> 
> Thanks
> Laszlo

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
  2017-10-23 14:18                 ` Gao, Liming
@ 2017-10-23 14:39                   ` Ard Biesheuvel
  0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2017-10-23 14:39 UTC (permalink / raw)
  To: Gao, Liming; +Cc: Laszlo Ersek, edk2-devel@lists.01.org, Leif Lindholm

On 23 October 2017 at 15:18, Gao, Liming <liming.gao@intel.com> wrote:
> This is for debug purpose. I think UINT32 is OK if AARCH64 can operate 32bit value.
>

Both my ARM and AARCH64 implementations write 16 bytes at a time, and
so whether the source value is UINT32 or UINT64 or UINT16 does not
make any difference at all.

>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Saturday, October 21, 2017 1:19 AM
>> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>
>> Subject: Re: [edk2] [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
>>
>> On 10/20/17 18:52, Ard Biesheuvel wrote:
>> > On 20 October 2017 at 17:51, Laszlo Ersek <lersek@redhat.com> wrote:
>> >> On 10/20/17 18:39, Ard Biesheuvel wrote:
>> >>> On 20 October 2017 at 17:37, Gao, Liming <liming.gao@intel.com> wrote:
>> >>>> Ard:
>> >>>>   This case is to share the same value between PeiCore and SecCore. I also think it will be better to define one fixed PCD in
>> MdeModulePkg.dec for this value. Could you submit bugzillar to catch this issue first?
>> >>>>
>> >>>
>> >>> Certainly!
>> >>
>> >> Would it be possible to define the PCD as UINT32, and task 64-bit SEC
>> >> (and PEI_CORE) code to first construct the wider value manually (in a
>> >> register or otherwise)?
>> >>
>> >> Just thinking out loud.
>> >>
>> >
>> > Could you think the reasoning behind that out loud as well?
>>
>> Haha, good stab :) Sure.
>>
>> In your patch you have:
>>
>> +#define INIT_CAR_VALUE      0x5AA55AA55AA55AA5
>>
>> for 64-bit, and
>>
>> +#define INIT_CAR_VALUE      0x5AA55AA5
>>
>> for 32-bit.
>>
>> Both 64-bit assembly code in SEC, and 64-bit C-code in the PEI_CORE, can
>> easily compose the large value from the small value, starting from
>> FixedPcdGet32(). The alternatives are:
>>
>> - asking the 32-bit assembly code to truncate the 64-bit constant -- it
>> won't compile,
>>
>> - defining *two* FixedAtBuild PCDs, one for 32-bit, another for 64-bit
>> SEC -- an idea probably not universally liked.
>>
>> ... When I originally started writing my previous email, I even thought
>> about introducing the PCD as UINT16 :) But then I realized, if any
>> platform lacks *some* 32-bit mode when it sets up the stack in assembly,
>> for C-language entry, then the platform won't be supported by edk2.
>>
>> Thanks
>> Laszlo


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-10-23 14:35 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-20 11:23 [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core Ard Biesheuvel
2017-10-20 13:00 ` Leif Lindholm
2017-10-20 15:43   ` Laszlo Ersek
2017-10-20 16:10     ` Ard Biesheuvel
2017-10-20 16:30       ` Laszlo Ersek
2017-10-20 16:37       ` Gao, Liming
2017-10-20 16:39         ` Ard Biesheuvel
2017-10-20 16:51           ` Laszlo Ersek
2017-10-20 16:52             ` Ard Biesheuvel
2017-10-20 17:18               ` Laszlo Ersek
2017-10-23 14:18                 ` Gao, Liming
2017-10-23 14:39                   ` Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox