[Linaro-uefi] Global variable write in PrePi

Message ID CAKv+Gu97UMMkjO27T0HDsgmy-18KFAV_HpUaU5iqVzsszkeWdw@mail.gmail.com
State New
Headers show

Commit Message

Ard Biesheuvel Oct. 22, 2016, 8:28 a.m.
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?


 _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

Comments

Ard Biesheuvel Oct. 22, 2016, 9:57 a.m. | #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
gary guo Nov. 15, 2016, 3:17 p.m. | #2
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

>
Ard Biesheuvel Nov. 15, 2016, 3:17 p.m. | #3
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.

Patch

--- 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]