Hi folks,
We are still using PrePi on some of our platforms and the code is still running on Flash at this stage. We found there is global data write in ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S. The change was introduced by this commit: 5dbacdb21b59748e885c2eccae370b81271ab795:
+*mSystemMemoryEnd: .8byte 0*^M
ASM_PFX(_ModuleEntryPoint): // Do early platform specific actions @@ -40,12 +42,23 @@ _SetSVCMode: // Check if we can install the stack at the top of the System Memory or if we need // to install the stacks at the bottom of the Firmware Device (case the FD is located // at the top of the DRAM) -_SetupStackPosition: - // Compute Top of System Memory - LoadConstantToReg (FixedPcdGet64 (PcdSystemMemoryBase), x1) - LoadConstantToReg (FixedPcdGet64 (PcdSystemMemorySize), x2) +_SystemMemoryEndInit:^M + ldr x1, mSystemMemoryEnd^M +^M + // Is mSystemMemoryEnd initialized?^M + cmp x1, #0^M + bne _SetupStackPosition^M +^M + LoadConstantToReg (FixedPcdGet32(PcdSystemMemoryBase), x1)^M + LoadConstantToReg (FixedPcdGet32(PcdSystemMemorySize), x2)^M sub x2, x2, #1 - add x1, x1, x2 // x1 = SystemMemoryTop = PcdSystemMemoryBase + PcdSystemMemorySize + add x1, x1, x2^M + // Update the global variable^M + *adr x2, mSystemMemoryEnd*^M + *str x1, [x2]*^M
I think direct write to flash should be forbidden. This change may work well for platforms which use ATF and load PrePi into memory, but not for other platforms.
Please let me know your comments about this.
Thanks and regards,
Heyi
On 22 October 2016 at 04:21, Heyi Guo heyi.guo@linaro.org wrote:
Hi folks,
We are still using PrePi on some of our platforms and the code is still running on Flash at this stage. We found there is global data write in ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S. The change was introduced by this commit: 5dbacdb21b59748e885c2eccae370b81271ab795:
+mSystemMemoryEnd: .8byte 0^M
ASM_PFX(_ModuleEntryPoint): // Do early platform specific actions @@ -40,12 +42,23 @@ _SetSVCMode: // Check if we can install the stack at the top of the System Memory or if we need // to install the stacks at the bottom of the Firmware Device (case the FD is located // at the top of the DRAM) -_SetupStackPosition:
- // Compute Top of System Memory
- LoadConstantToReg (FixedPcdGet64 (PcdSystemMemoryBase), x1)
- LoadConstantToReg (FixedPcdGet64 (PcdSystemMemorySize), x2)
+_SystemMemoryEndInit:^M
- ldr x1, mSystemMemoryEnd^M
+^M
- // Is mSystemMemoryEnd initialized?^M
- cmp x1, #0^M
- bne _SetupStackPosition^M
+^M
- LoadConstantToReg (FixedPcdGet32(PcdSystemMemoryBase), x1)^M
- LoadConstantToReg (FixedPcdGet32(PcdSystemMemorySize), x2)^M sub x2, x2, #1
- add x1, x1, x2 // x1 = SystemMemoryTop = PcdSystemMemoryBase +
PcdSystemMemorySize
- add x1, x1, x2^M
- // Update the global variable^M
- adr x2, mSystemMemoryEnd^M
- str x1, [x2]^M
I think direct write to flash should be forbidden. This change may work well for platforms which use ATF and load PrePi into memory, but not for other platforms.
Please let me know your comments about this.
You are right. This should be removed. I think it was added for ATF on Juno, but obviously, this is not the right way to do it, given that SEC and PEIM modules may run from NOR.
On 22 October 2016 at 09:09, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On 22 October 2016 at 04:21, Heyi Guo heyi.guo@linaro.org wrote:
Hi folks,
We are still using PrePi on some of our platforms and the code is still running on Flash at this stage. We found there is global data write in ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S. The change was introduced by this commit: 5dbacdb21b59748e885c2eccae370b81271ab795:
+mSystemMemoryEnd: .8byte 0^M
ASM_PFX(_ModuleEntryPoint): // Do early platform specific actions @@ -40,12 +42,23 @@ _SetSVCMode: // Check if we can install the stack at the top of the System Memory or if we need // to install the stacks at the bottom of the Firmware Device (case the FD is located // at the top of the DRAM) -_SetupStackPosition:
- // Compute Top of System Memory
- LoadConstantToReg (FixedPcdGet64 (PcdSystemMemoryBase), x1)
- LoadConstantToReg (FixedPcdGet64 (PcdSystemMemorySize), x2)
+_SystemMemoryEndInit:^M
- ldr x1, mSystemMemoryEnd^M
+^M
- // Is mSystemMemoryEnd initialized?^M
- cmp x1, #0^M
- bne _SetupStackPosition^M
+^M
- LoadConstantToReg (FixedPcdGet32(PcdSystemMemoryBase), x1)^M
- LoadConstantToReg (FixedPcdGet32(PcdSystemMemorySize), x2)^M sub x2, x2, #1
- add x1, x1, x2 // x1 = SystemMemoryTop = PcdSystemMemoryBase +
PcdSystemMemorySize
- add x1, x1, x2^M
- // Update the global variable^M
- adr x2, mSystemMemoryEnd^M
- str x1, [x2]^M
I think direct write to flash should be forbidden. This change may work well for platforms which use ATF and load PrePi into memory, but not for other platforms.
Please let me know your comments about this.
You are right. This should be removed. I think it was added for ATF on Juno, but obviously, this is not the right way to do it, given that SEC and PEIM modules may run from NOR.
Does this work for you?
--- a/ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S +++ b/ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S @@ -28,18 +28,6 @@ _SetSVCMode: // Check if we can install the stack at the top of the System Memory or if we need // to install the stacks at the bottom of the Firmware Device (case the FD is located // at the top of the DRAM) -_SystemMemoryEndInit: - ldr x1, mSystemMemoryEnd - - // Is mSystemMemoryEnd initialized? - cmp x1, #0 - bne _SetupStackPosition - - MOV64 (x1, FixedPcdGet64(PcdSystemMemoryBase) + FixedPcdGet64(PcdSystemMemorySize) - 1) - - // Update the global variable - adr x2, mSystemMemoryEnd - str x1, [x2]
_SetupStackPosition: // r1 = SystemMemoryTop @@ -130,4 +118,5 @@ _PrepareArguments: _NeverReturn: b _NeverReturn
-ASM_PFX(mSystemMemoryEnd): .8byte 0 +ASM_PFX(mSystemMemoryEnd): + .8byte FixedPcdGet64(PcdSystemMemoryBase) + FixedPcdGet64(PcdSystemMemorySize) - 1
On 22 October 2016 at 09:28, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On 22 October 2016 at 09:09, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On 22 October 2016 at 04:21, Heyi Guo heyi.guo@linaro.org wrote:
Hi folks,
We are still using PrePi on some of our platforms and the code is still running on Flash at this stage. We found there is global data write in ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S. The change was introduced by this commit: 5dbacdb21b59748e885c2eccae370b81271ab795:
+mSystemMemoryEnd: .8byte 0^M
ASM_PFX(_ModuleEntryPoint): // Do early platform specific actions @@ -40,12 +42,23 @@ _SetSVCMode: // Check if we can install the stack at the top of the System Memory or if we need // to install the stacks at the bottom of the Firmware Device (case the FD is located // at the top of the DRAM) -_SetupStackPosition:
- // Compute Top of System Memory
- LoadConstantToReg (FixedPcdGet64 (PcdSystemMemoryBase), x1)
- LoadConstantToReg (FixedPcdGet64 (PcdSystemMemorySize), x2)
+_SystemMemoryEndInit:^M
- ldr x1, mSystemMemoryEnd^M
+^M
- // Is mSystemMemoryEnd initialized?^M
- cmp x1, #0^M
- bne _SetupStackPosition^M
+^M
- LoadConstantToReg (FixedPcdGet32(PcdSystemMemoryBase), x1)^M
- LoadConstantToReg (FixedPcdGet32(PcdSystemMemorySize), x2)^M sub x2, x2, #1
- add x1, x1, x2 // x1 = SystemMemoryTop = PcdSystemMemoryBase +
PcdSystemMemorySize
- add x1, x1, x2^M
- // Update the global variable^M
- adr x2, mSystemMemoryEnd^M
- str x1, [x2]^M
I think direct write to flash should be forbidden. This change may work well for platforms which use ATF and load PrePi into memory, but not for other platforms.
Please let me know your comments about this.
You are right. This should be removed. I think it was added for ATF on Juno, but obviously, this is not the right way to do it, given that SEC and PEIM modules may run from NOR.
Does this work for you?
--- a/ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S +++ b/ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S @@ -28,18 +28,6 @@ _SetSVCMode: // Check if we can install the stack at the top of the System Memory or if we need // to install the stacks at the bottom of the Firmware Device (case the FD is located // at the top of the DRAM) -_SystemMemoryEndInit:
- ldr x1, mSystemMemoryEnd
We need to keep this load
- // Is mSystemMemoryEnd initialized?
- cmp x1, #0
- bne _SetupStackPosition
- MOV64 (x1, FixedPcdGet64(PcdSystemMemoryBase) +
FixedPcdGet64(PcdSystemMemorySize) - 1)
- // Update the global variable
- adr x2, mSystemMemoryEnd
- str x1, [x2]
_SetupStackPosition: // r1 = SystemMemoryTop @@ -130,4 +118,5 @@ _PrepareArguments: _NeverReturn: b _NeverReturn
-ASM_PFX(mSystemMemoryEnd): .8byte 0 +ASM_PFX(mSystemMemoryEnd):
- .8byte FixedPcdGet64(PcdSystemMemoryBase) +
FixedPcdGet64(PcdSystemMemorySize) - 1
Hi Ard,
Sorry for the late. We tested the patch and it worked. Would you help to post it to EDK2?
Thanks and regards,
Heyi
On 22 October 2016 at 17:57, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On 22 October 2016 at 09:28, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On 22 October 2016 at 09:09, Ard Biesheuvel ard.biesheuvel@linaro.org
wrote:
On 22 October 2016 at 04:21, Heyi Guo heyi.guo@linaro.org wrote:
Hi folks,
We are still using PrePi on some of our platforms and the code is still running on Flash at this stage. We found there is global data write in ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S. The change was
introduced
by this commit: 5dbacdb21b59748e885c2eccae370b81271ab795:
+mSystemMemoryEnd: .8byte 0^M
ASM_PFX(_ModuleEntryPoint): // Do early platform specific actions @@ -40,12 +42,23 @@ _SetSVCMode: // Check if we can install the stack at the top of the System Memory
or if
we need // to install the stacks at the bottom of the Firmware Device (case
the FD
is located // at the top of the DRAM) -_SetupStackPosition:
- // Compute Top of System Memory
- LoadConstantToReg (FixedPcdGet64 (PcdSystemMemoryBase), x1)
- LoadConstantToReg (FixedPcdGet64 (PcdSystemMemorySize), x2)
+_SystemMemoryEndInit:^M
- ldr x1, mSystemMemoryEnd^M
+^M
- // Is mSystemMemoryEnd initialized?^M
- cmp x1, #0^M
- bne _SetupStackPosition^M
+^M
- LoadConstantToReg (FixedPcdGet32(PcdSystemMemoryBase), x1)^M
- LoadConstantToReg (FixedPcdGet32(PcdSystemMemorySize), x2)^M sub x2, x2, #1
- add x1, x1, x2 // x1 = SystemMemoryTop = PcdSystemMemoryBase
PcdSystemMemorySize
- add x1, x1, x2^M
- // Update the global variable^M
- adr x2, mSystemMemoryEnd^M
- str x1, [x2]^M
I think direct write to flash should be forbidden. This change may
work well
for platforms which use ATF and load PrePi into memory, but not for
other
platforms.
Please let me know your comments about this.
You are right. This should be removed. I think it was added for ATF on Juno, but obviously, this is not the right way to do it, given that SEC and PEIM modules may run from NOR.
Does this work for you?
--- a/ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S +++ b/ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S @@ -28,18 +28,6 @@ _SetSVCMode: // Check if we can install the stack at the top of the System Memory or if we need // to install the stacks at the bottom of the Firmware Device (case the FD is located // at the top of the DRAM) -_SystemMemoryEndInit:
- ldr x1, mSystemMemoryEnd
We need to keep this load
- // Is mSystemMemoryEnd initialized?
- cmp x1, #0
- bne _SetupStackPosition
- MOV64 (x1, FixedPcdGet64(PcdSystemMemoryBase) +
FixedPcdGet64(PcdSystemMemorySize) - 1)
- // Update the global variable
- adr x2, mSystemMemoryEnd
- str x1, [x2]
_SetupStackPosition: // r1 = SystemMemoryTop @@ -130,4 +118,5 @@ _PrepareArguments: _NeverReturn: b _NeverReturn
-ASM_PFX(mSystemMemoryEnd): .8byte 0 +ASM_PFX(mSystemMemoryEnd):
- .8byte FixedPcdGet64(PcdSystemMemoryBase) +
FixedPcdGet64(PcdSystemMemorySize) - 1
On 15 November 2016 at 15:17, Heyi Guo heyi.guo@linaro.org wrote:
Hi Ard,
Sorry for the late. We tested the patch and it worked. Would you help to post it to EDK2?
That code is already merged,
Thanks, Ard.
-
Ard Biesheuvel -
Heyi Guo