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

[Marcin Wojtas <mw@semihalf.com>]

2018-06-01 18:57 GMT+02:00 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?
>

Thanks for explanation. Not sure if I will be able to use the SRAM,
otherwise I will extend the hole in DRAM, which is reserved for the
PSCI and use it as the PEI stack and heap.

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