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

[Marcin Wojtas <mw@semihalf.com>]

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.

> 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)?

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

Best regards,
Marcin



>
>>    # Secure region reservation
>>    gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
>>    gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x0200000
>> @@ -364,7 +384,21 @@
>>  [Components.common]
>>
>>    # PEI Phase modules
>> -  ArmPlatformPkg/PrePi/PeiUniCore.inf
>> +  ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
>> +  MdeModulePkg/Core/Pei/PeiMain.inf
>> +  MdeModulePkg/Universal/PCD/Pei/Pcd.inf {
>> +    <LibraryClasses>
>> +      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>> +  }
>> +  ArmPlatformPkg/PlatformPei/PlatformPeim.inf
>> +  ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
>> +  ArmPkg/Drivers/CpuPei/CpuPei.inf
>> +  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
>> +  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf {
>> +    <LibraryClasses>
>> +      NULL|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
>> +  }
>> +
>>
>>    # DXE
>>    MdeModulePkg/Core/Dxe/DxeMain.inf {
>> --
>> 2.7.4
>>