Re: [platforms PATCH 2/4] Marvell/Aramda7k8k: Enable PEI booting stage

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 1 June 2018 at 18:43, Marcin Wojtas <mw@semihalf.com> wrote:
> 2018-06-01 17:30 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
>> On 1 June 2018 at 16:32, Marcin Wojtas <mw@semihalf.com> wrote:
>>> PEI phase will allow to use more robust platform initialization,
>>> with new features like the capsule support. Wire up all
>>> dependencies for that purpose.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>>> ---
>>>  Platform/Marvell/Armada70x0Db/Armada70x0Db.fdf | 15 ++++++--
>>>  Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc  | 40 ++++++++++++++++++--
>>>  2 files changed, 48 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Platform/Marvell/Armada70x0Db/Armada70x0Db.fdf b/Platform/Marvell/Armada70x0Db/Armada70x0Db.fdf
>>> index 69cb4cd..bf04f4d 100644
>>> --- a/Platform/Marvell/Armada70x0Db/Armada70x0Db.fdf
>>> +++ b/Platform/Marvell/Armada70x0Db/Armada70x0Db.fdf
>>> @@ -63,7 +63,7 @@ DATA = {
>>>  !endif
>>>  }
>>>
>>> -0x00001000|0x000ff000
>>> +0x00001000|0x001ff000
>>>  gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize
>>>  FV = FVMAIN_COMPACT
>>>
>>> @@ -221,7 +221,14 @@ READ_STATUS        = TRUE
>>>  READ_LOCK_CAP      = TRUE
>>>  READ_LOCK_STATUS   = TRUE
>>>
>>> -  INF ArmPlatformPkg/PrePi/PeiUniCore.inf
>>> +  INF ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
>>> +  INF MdeModulePkg/Core/Pei/PeiMain.inf
>>> +  INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
>>> +  INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf
>>> +  INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
>>> +  INF ArmPkg/Drivers/CpuPei/CpuPei.inf
>>> +  INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
>>> +  INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>>>
>>>    FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
>>>      SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED = TRUE {
>>> @@ -264,14 +271,14 @@ READ_LOCK_STATUS   = TRUE
>>>
>>>  [Rule.Common.PEI_CORE]
>>>    FILE PEI_CORE = $(NAMED_GUID) {
>>> -    TE     TE                           $(INF_OUTPUT)/$(MODULE_NAME).efi
>>> +    TE     TE Align = Auto              $(INF_OUTPUT)/$(MODULE_NAME).efi
>>>      UI     STRING ="$(MODULE_NAME)" Optional
>>>    }
>>>
>>>  [Rule.Common.PEIM]
>>>    FILE PEIM = $(NAMED_GUID) {
>>>       PEI_DEPEX PEI_DEPEX Optional       $(INF_OUTPUT)/$(MODULE_NAME).depex
>>> -     TE       TE                        $(INF_OUTPUT)/$(MODULE_NAME).efi
>>> +     TE       TE Align = Auto           $(INF_OUTPUT)/$(MODULE_NAME).efi
>>>       UI       STRING="$(MODULE_NAME)" Optional
>>>    }
>>>
>>> diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
>>> index 4129742..195b6b7 100644
>>> --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
>>> +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
>>> @@ -145,13 +145,28 @@
>>>    MemoryAllocationLib|EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiMemoryAllocationLib.inf
>>>    HobLib|EmbeddedPkg/Library/PrePiHobLib/PrePiHobLib.inf
>>>    PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
>>> -  PlatformPeiLib|ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
>>>    ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
>>>    PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>>>
>>>  [LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
>>> -  MemoryInitPeiLib|Silicon/Marvell/Armada7k8k/Library/Armada7k8kMemoryInitPeiLib/Armada7k8kMemoryInitPeiLib.inf
>>>    BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>>> +  PlatformPeiLib|ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
>>> +
>>> +[LibraryClasses.common.PEI_CORE, LibraryClasses.common.PEIM]
>>> +  PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>>> +  BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>>> +  HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
>>> +  PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
>>> +  MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
>>> +  ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedSectionLib/PeiExtractGuidedSectionLib.inf
>>> +  PeiServicesTablePointerLib|ArmPkg/Library/PeiServicesTablePointerLib/PeiServicesTablePointerLib.inf
>>> +
>>> +[LibraryClasses.common.PEI_CORE]
>>> +  PeiCoreEntryPoint|MdePkg/Library/PeiCoreEntryPoint/PeiCoreEntryPoint.inf
>>> +
>>> +[LibraryClasses.common.PEIM]
>>> +  PeimEntryPoint|MdePkg/Library/PeimEntryPoint/PeimEntryPoint.inf
>>> +  MemoryInitPeiLib|Silicon/Marvell/Armada7k8k/Library/Armada7k8kMemoryInitPeiLib/Armada7k8kMemoryInitPeiLib.inf
>>>
>>>  [LibraryClasses.common.DXE_CORE]
>>>    HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
>>> @@ -336,8 +351,13 @@
>>>    # ARM Pcds
>>>    gArmTokenSpaceGuid.PcdSystemMemoryBase|0
>>>    gArmTokenSpaceGuid.PcdSystemMemorySize|0x40000000
>>> +
>>> +  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|36
>>>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|36
>>>
>>> +  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x2E000000
>>> +  gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0xFFC0
>>> +
>>
>> These were copy/pasted from SynQuacer, I suppose?
>>
>
> SynQuacer and ARM VExpress use this value, I also checked ones used in
> HiSilicon Platforms - I didn't notice any issue with neither of them.
>

SynQuacer and ARM VExpress are both platforms designed by ARM Ltd, and
both have non-secure SRAM at this offset.

>> Ideally, these should point to some memory that is not exposed to the
>> OS, so that the PEI phase cannot corrupt a capsule image that has been
>> left in DRAM by the OS.
>>
>> This only becomes relevant once we implement support for PSCI warm
>> reboot, otherwise DRAM will be cleared anyway. However, pointing these
>> into a random slice of main memory feels a little risky.
>
>
> Currently we have also a hole between 0xc0000000 - 4GB for the config
> space, so how about squeezing those 64kB there?
>
> Alternatively would you recommend to use value outside the DRAM space
> (e.g. above max DRAM size)?
>

No, this region should point to usable memory, and ideally memory that
the OS does not use. So the options are (in order of preference)
- put the PEI stack and heap in non-secure SRAM
- put the PEI stack and heap in a slice of DRAM that is marked as
'reserved' in the EFI memory map.
- put the PEI stack and heap in a slice of DRAM that is otherwise
protected from allocation until DxeCore launches.

When the OS calls UpdateCapsule(), it will put the capsule at an
arbitrary location in memory, and if this happens to be the same
region PEI uses for its stack and heap, you will corrupt the capsule
before the capsule PEIM gets a chance to preserve it.

At the moment, this does not make any difference given that we don't
implement warm reboot yet (but PSCI defines it now). Also, fwupdate in
Linux does not rely on UpdateCapsule() at OS runtime. However, M$
Windows does, and I assume you don't want to make your UEFI
implementation Linux-only?


>>
>> Do you have non-secure SRAM on this SoC?
>
> There are a couple of SRAMs (for offload CM3 CPUs), one configured as
> secure. Not sure about others. Why do you ask?
>

Because that is the preferred place for the PEI stack and heap.