* [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