ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S | 7 +++++++ ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S | 10 ++++++++++ ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm | 10 ++++++++++ 3 files changed, 27 insertions(+)
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
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
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
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
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
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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.