| Message ID | CAKv+Gu97UMMkjO27T0HDsgmy-18KFAV_HpUaU5iqVzsszkeWdw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
--- 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]