[edk2] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core

Message ID 20171020112325.10814-1-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • [edk2] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core
Related show

Commit Message

Ard Biesheuvel Oct. 20, 2017, 11:23 a.m.
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(+)

-- 
2.11.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm Oct. 20, 2017, 1 p.m. | #1
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

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Oct. 20, 2017, 3:43 p.m. | #2
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

>>


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Oct. 20, 2017, 4:10 p.m. | #3
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
Laszlo Ersek Oct. 20, 2017, 4:30 p.m. | #4
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

>>>>

>>


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Gao, Liming Oct. 20, 2017, 4:37 p.m. | #5
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Oct. 20, 2017, 4:39 p.m. | #6
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Oct. 20, 2017, 4:51 p.m. | #7
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Oct. 20, 2017, 4:52 p.m. | #8
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?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Oct. 20, 2017, 5:18 p.m. | #9
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Gao, Liming Oct. 23, 2017, 2:18 p.m. | #10
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Oct. 23, 2017, 2:39 p.m. | #11
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

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