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