[Linaro-uefi] Platforms/Marvell/Armada70x0: use dynamically discovered DRAM size

Message ID 20170530134802.20658-1-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel May 30, 2017, 1:48 p.m.
Find the structure in memory that describes the DRAM discovered by the
secure firmware, and use it to initialize the UEFI memory descriptors.

At the same time, drop the hardcoded DRAM size to 2 GB, which should be
a reasonable lower bound for the amount of DDR4 DRAM on platforms built
around this SoC family.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Platforms/Marvell/Armada/Armada.dsc.inc            |  2 +-
 .../Armada70x0Lib/AArch64/ArmPlatformHelper.S      | 84 +++++++++++++++------
 .../Library/Armada70x0Lib/ARM/ArmPlatformHelper.S  | 88 ++++++++++++++++------
 .../Library/Armada70x0Lib/Armada70x0LibMem.c       | 15 ++--
 4 files changed, 140 insertions(+), 49 deletions(-)

Comments

Leif Lindholm May 30, 2017, 4:24 p.m. | #1
On Tue, May 30, 2017 at 01:48:02PM +0000, Ard Biesheuvel wrote:
> Find the structure in memory that describes the DRAM discovered by the
> secure firmware, and use it to initialize the UEFI memory descriptors.
> 
> At the same time, drop the hardcoded DRAM size to 2 GB, which should be
> a reasonable lower bound for the amount of DDR4 DRAM on platforms built
> around this SoC family.

Well, if that turns out a lower value is needed somewhere, we can
always break it out to the .dsc later.

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---

So, to clarify, this is not a patch against current upstream, but
sharing of work-in-progress?

>  Platforms/Marvell/Armada/Armada.dsc.inc            |  2 +-
>  .../Armada70x0Lib/AArch64/ArmPlatformHelper.S      | 84 +++++++++++++++------
>  .../Library/Armada70x0Lib/ARM/ArmPlatformHelper.S  | 88 ++++++++++++++++------
>  .../Library/Armada70x0Lib/Armada70x0LibMem.c       | 15 ++--
>  4 files changed, 140 insertions(+), 49 deletions(-)
> 
> diff --git a/Platforms/Marvell/Armada/Armada.dsc.inc b/Platforms/Marvell/Armada/Armada.dsc.inc
> index 222ab89cb45e..002c3b3ec961 100644
> --- a/Platforms/Marvell/Armada/Armada.dsc.inc
> +++ b/Platforms/Marvell/Armada/Armada.dsc.inc
> @@ -371,7 +371,7 @@
>  
>    # ARM Pcds
>    gArmTokenSpaceGuid.PcdSystemMemoryBase|0
> -  gArmTokenSpaceGuid.PcdSystemMemorySize|0x100000000
> +  gArmTokenSpaceGuid.PcdSystemMemorySize|0x80000000
>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|36
>  
>    gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
> diff --git a/Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S b/Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
> index ba9685f1d36e..8246892f2585 100644
> --- a/Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
> +++ b/Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
> @@ -26,37 +26,47 @@
>  #define CCU_MC_RTBR_OFFSET              0x8
>  #define CCU_MC_RTBR_TARGET_BASE(Base)   (((Base) >> 20) << 10)
>  
> +#define SYSTEM_INFO_ADDRESS             0x4000000
> +#define SYSINFO_ARRAY_SIZE              0x0
> +#define SYSINFO_DRAM_CS0_SIZE           0x1
> +
>  ASM_FUNC(ArmPlatformPeiBootAction)
>    mov   x29, xzr
> +  mov   x28, x30
>  
>    .if   FixedPcdGet64 (PcdSystemMemoryBase) != 0
>    .err  PcdSystemMemoryBase should be 0x0 on this platform!
>    .endif
>  
>    .if   FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget)
> -    //
> -    // Remap the DRAM region [DramRemapTarget, DramRemapTarget + DramRemapSize)
> -    // to the top of the DRAM space. This is mainly intended to free up 32-bit
> -    // addressable memory for MMIO and PCI windows.
> -    //
> -    MOV32 (x0, CCU_MC_BASE)
> -    MOV32 (w1, CCU_MC_RSBR_SOURCE_BASE (FixedPcdGet64 (PcdSystemMemorySize)))
> -    MOV32 (w2, CCU_MC_RTBR_TARGET_BASE (FixedPcdGet32 (PcdDramRemapTarget)))
> -    MOV32 (w3, CCU_MC_RCR_REMAP_SIZE (FixedPcdGet32 (PcdDramRemapSize)) | CCU_MC_RCR_REMAP_EN)
> -    str   w1, [x0, #CCU_MC_RSBR_OFFSET]
> -    str   w2, [x0, #CCU_MC_RTBR_OFFSET]
> -    str   w3, [x0, #CCU_MC_RCR_OFFSET]
> -
> -    //
> -    // Use the low range for UEFI itself. The remaining memory will be mapped
> -    // and added to the GCD map later.
> -    //
> -    adr   x0, mSystemMemoryEnd
> -    MOV64 (x1, FixedPcdGet32 (PcdDramRemapTarget) - 1)
> -    str   x1, [x0]
> +  .err  Hard coded PcdSystemMemorySize should not exceed PcdDramRemapTarget
>    .endif
>  
> -  MOV32 (x0, FixedPcdGet64 (PcdFvBaseAddress))
> +  bl    DiscoverDramSize
> +  cbz   w0, 0f

This confused me until I got to the end of the patch - the amount of
DRAM is initialized by default to PcdSystemMemorySize. Could you add a
comment to that effect? i.e.

  cbz  w0, 0f    // Use default PcdSystemMemorySize at address 0

?

> +
> +  //
> +  // Remap the DRAM region [DramRemapTarget, DramRemapTarget + DramRemapSize)
> +  // to the top of the DRAM space. This is mainly intended to free up 32-bit
> +  // addressable memory for MMIO and PCI windows.
> +  //
> +  lsl   w1, w0, #10          // set CCU_MC_RSBR_SOURCE_BASE to DRAM size in KiB
> +  MOV32 (x0, CCU_MC_BASE)
> +  MOV32 (w2, CCU_MC_RTBR_TARGET_BASE (FixedPcdGet32 (PcdDramRemapTarget)))
> +  MOV32 (w3, CCU_MC_RCR_REMAP_SIZE (FixedPcdGet32 (PcdDramRemapSize)) | CCU_MC_RCR_REMAP_EN)
> +  str   w1, [x0, #CCU_MC_RSBR_OFFSET]
> +  str   w2, [x0, #CCU_MC_RTBR_OFFSET]
> +  str   w3, [x0, #CCU_MC_RCR_OFFSET]
> +
> +  //
> +  // Use the low range for UEFI itself. The remaining memory will be mapped
> +  // and added to the GCD map later.
> +  //
> +  adr   x0, mSystemMemoryEnd
> +  MOV64 (x1, FixedPcdGet32 (PcdDramRemapTarget) - 1)
> +  str   x1, [x0]
> +
> +0:MOV32 (x0, FixedPcdGet64 (PcdFvBaseAddress))
>    MOV32 (x3, FixedPcdGet32 (PcdFvSize))
>    add   x3, x3, x0
>  
> @@ -70,6 +80,38 @@ ASM_FUNC(ArmPlatformPeiBootAction)
>    cmp   x0, x3
>    b.lt  0b
>  
> +  ret   x28
> +
> +DiscoverDramSize:
> +  //
> +  // ARM Trusted Firmware leaves us a structure in memory that describes the
> +  // nature of the available DRAM. It consists of an array of <u32, u32>
> +  // key/value pairs, where the only keys we are [currently] interested in are:
> +  //
> +  // 0x0 (ARRAY_SIZE)     the overall number of entries (should be the first)
> +  // 0x1 (DRAM_CS0_SIZE)  the size in MiB of DRAM bank 0
> +  //
> +  MOV32 (x4, SYSTEM_INFO_ADDRESS)
> +  ldp   w0, w1, [x4], #8
> +
> +  cmp   w0, #SYSINFO_ARRAY_SIZE   // first entry contains array size?
> +  b.ne  1f
> +
> +0:subs  w1, w1, #1                // more entries to check?
> +  b.eq  1f
> +
> +  ldp   w2, w3, [x4], #8          // next entry contains DRAM CS0 size?
> +  cmp   w2, #SYSINFO_DRAM_CS0_SIZE
> +  b.ne  0b
> +
> +  mov   w0, w3                    // return value in MiB
> +
> +  adr   x1, DiscoveredDramSize    // store value in bytes
> +  lsl   x3, x3, #20
> +  str   x3, [x1]
> +  ret
> +
> +1:mov   w0, wzr
>    ret
>  
>  //UINTN
> diff --git a/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S b/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
> index 4cd8b3154e82..fdb688ca489a 100644
> --- a/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
> +++ b/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
> @@ -26,36 +26,80 @@
>  #define CCU_MC_RTBR_OFFSET              0x8
>  #define CCU_MC_RTBR_TARGET_BASE(Base)   (((Base) >> 20) << 10)
>  
> +#define SYSTEM_INFO_ADDRESS             0x4000000
> +#define SYSINFO_ARRAY_SIZE              0x0
> +#define SYSINFO_DRAM_CS0_SIZE           0x1
> +

So, this is not the end of the world, but I'd prefer it if these magic
values weren't duplicated across the two versions.

>  ASM_FUNC(ArmPlatformPeiBootAction)
> +  mov   r13, lr
> +
>    .if   FixedPcdGet64 (PcdSystemMemoryBase) != 0
>    .err  PcdSystemMemoryBase should be 0x0 on this platform!
>    .endif
>  
>    .if   FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget)
> -    //
> -    // Remap the DRAM region [DramRemapTarget, DramRemapTarget + DramRemapSize)
> -    // to the top of the DRAM space. This is mainly intended to free up 32-bit
> -    // addressable memory for MMIO and PCI windows.
> -    //
> -    MOV32 (r0, CCU_MC_BASE)
> -    MOV32 (r1, CCU_MC_RSBR_SOURCE_BASE (FixedPcdGet64 (PcdSystemMemorySize)))
> -    MOV32 (r2, CCU_MC_RTBR_TARGET_BASE (FixedPcdGet32 (PcdDramRemapTarget)))
> -    MOV32 (r3, CCU_MC_RCR_REMAP_SIZE (FixedPcdGet32 (PcdDramRemapSize)) | CCU_MC_RCR_REMAP_EN)
> -    str   r1, [r0, #CCU_MC_RSBR_OFFSET]
> -    str   r2, [r0, #CCU_MC_RTBR_OFFSET]
> -    str   r3, [r0, #CCU_MC_RCR_OFFSET]
> -
> -    //
> -    // Use the low range for UEFI itself. The remaining memory will be mapped
> -    // and added to the GCD map later.
> -    //
> -    ADRL  (r0, mSystemMemoryEnd)
> -    MOV32 (r2, FixedPcdGet32 (PcdDramRemapTarget) - 1)
> -    mov   r3, #0
> -    strd  r2, r3, [r0]
> -
> +  .err  Hard coded PcdSystemMemorySize should not exceed PcdDramRemapTarget
>    .endif
>  
> +  bl    DiscoverDramSize
> +  cmp   r0, #0
> +  beq   0f

Same comment as 64-bit, // Use default PcdSystemMemorySize at address 0
?

> +
> +  //
> +  // Remap the DRAM region [DramRemapTarget, DramRemapTarget + DramRemapSize)
> +  // to the top of the DRAM space. This is mainly intended to free up 32-bit
> +  // addressable memory for MMIO and PCI windows.
> +  //
> +  lsl   r1, r0, #10          // set CCU_MC_RSBR_SOURCE_BASE to DRAM size in KiB
> +  MOV32 (r0, CCU_MC_BASE)
> +  MOV32 (r2, CCU_MC_RTBR_TARGET_BASE (FixedPcdGet32 (PcdDramRemapTarget)))
> +  MOV32 (r3, CCU_MC_RCR_REMAP_SIZE (FixedPcdGet32 (PcdDramRemapSize)) | CCU_MC_RCR_REMAP_EN)
> +  str   r1, [r0, #CCU_MC_RSBR_OFFSET]
> +  str   r2, [r0, #CCU_MC_RTBR_OFFSET]
> +  str   r3, [r0, #CCU_MC_RCR_OFFSET]
> +
> +  //
> +  // Use the low range for UEFI itself. The remaining memory will be mapped
> +  // and added to the GCD map later.
> +  //
> +  ADRL  (r0, mSystemMemoryEnd)
> +  MOV32 (r2, FixedPcdGet32 (PcdDramRemapTarget) - 1)
> +  mov   r3, #0
> +  strd  r2, r3, [r0]
> +
> +0:bx    r13
> +
> +DiscoverDramSize:
> +  //
> +  // ARM Trusted Firmware leaves us a structure in memory that describes the
> +  // nature of the available DRAM. It consists of an array of <u32, u32>
> +  // key/value pairs, where the only keys we are [currently] interested in are:
> +  //
> +  // 0x0 (ARRAY_SIZE)     the overall number of entries (should be the first)
> +  // 0x1 (DRAM_CS0_SIZE)  the size in MiB of DRAM bank 0
> +  //
> +  MOV32 (r4, SYSTEM_INFO_ADDRESS)
> +  ldrd  r0, r1, [r4], #8
> +
> +  cmp   r0, #SYSINFO_ARRAY_SIZE   // first entry contains array size?
> +  bne   1f
> +
> +0:subs  r1, r1, #1                // more entries to check?
> +  beq   1f
> +
> +  ldrd  r2, r3, [r4], #8          // next entry contains DRAM CS0 size?
> +  cmp   r2, #SYSINFO_DRAM_CS0_SIZE
> +  bne   0b
> +
> +  mov   r0, r3                    // return value in MiB
> +
> +  ADRL  (r1, DiscoveredDramSize)
> +  lsl   r2, r3, #20               // store value in bytes
> +  lsr   r3, r3, #12
> +  strd  r2, r3, [r1]
> +  bx    lr
> +
> +1:mov   r0, #0
>    bx    lr
>  
>  //UINTN
> diff --git a/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> index 4d80426c177c..85df57164b7f 100644
> --- a/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> +++ b/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> @@ -47,6 +47,13 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  
>  STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
>  
> +//
> +// This will be overwritten by ArmPlatformPeiBootAction() based on information
> +// provided by the secure firmware. PcdSystemMemorySize should be a reasonable
> +// default otherwise.
> +//
> +UINT64 DiscoveredDramSize = FixedPcdGet64 (PcdSystemMemorySize);

Should this be mDiscoveredDramSize?
(Would also make it harder to misparse in DiscoverDramSize :)

> +
>  /**
>    Return the Virtual Memory Map of your platform
>  
> @@ -63,7 +70,6 @@ ArmPlatformGetVirtualMemoryMap (
>    )
>  {
>    UINTN                         Index = 0;
> -  UINT64                        MemSize;
>    UINT64                        MemLowSize;
>    UINT64                        MemHighStart;
>    UINT64                        MemHighSize;
> @@ -71,11 +77,10 @@ ArmPlatformGetVirtualMemoryMap (
>  
>    ASSERT (VirtualMemoryMap != NULL);
>  
> -  MemSize = FixedPcdGet64 (PcdSystemMemorySize);
> -  MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), MemSize);
> +  MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), DiscoveredDramSize);
>    MemHighStart = (UINT64)FixedPcdGet64 (PcdDramRemapTarget) +
>                   FixedPcdGet32 (PcdDramRemapSize);
>
> -  MemHighSize = MemSize - MemLowSize;
> +  MemHighSize = DiscoveredDramSize - MemLowSize;

Is the above operation still correct if DiscoverDramSize returned 0?

>  
>    ResourceAttributes = (
>        EFI_RESOURCE_ATTRIBUTE_PRESENT |
> @@ -105,7 +110,7 @@ ArmPlatformGetVirtualMemoryMap (
>    VirtualMemoryTable[Index].Length          = 0x20000000;
>    VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>  
> -  if (MemSize > MemLowSize) {
> +  if (DiscoveredDramSize > MemLowSize) {
>      //
>      // If we have more than MemLowSize worth of DRAM, the remainder will be
>      // mapped at the top of the remapped window.
> -- 
> 2.9.3
>
Ard Biesheuvel May 30, 2017, 4:40 p.m. | #2
On 30 May 2017 at 16:24, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, May 30, 2017 at 01:48:02PM +0000, Ard Biesheuvel wrote:
>> Find the structure in memory that describes the DRAM discovered by the
>> secure firmware, and use it to initialize the UEFI memory descriptors.
>>
>> At the same time, drop the hardcoded DRAM size to 2 GB, which should be
>> a reasonable lower bound for the amount of DDR4 DRAM on platforms built
>> around this SoC family.
>
> Well, if that turns out a lower value is needed somewhere, we can
> always break it out to the .dsc later.
>

Indeed.

>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>
> So, to clarify, this is not a patch against current upstream, but
> sharing of work-in-progress?
>

This applies to the top of my OPP tree. At the request of Marcin, I
shared this bit of code. The rest of the tree hasn't changed much over
the past couple of weeks.

>>  Platforms/Marvell/Armada/Armada.dsc.inc            |  2 +-
>>  .../Armada70x0Lib/AArch64/ArmPlatformHelper.S      | 84 +++++++++++++++------
>>  .../Library/Armada70x0Lib/ARM/ArmPlatformHelper.S  | 88 ++++++++++++++++------
>>  .../Library/Armada70x0Lib/Armada70x0LibMem.c       | 15 ++--
>>  4 files changed, 140 insertions(+), 49 deletions(-)
>>
>> diff --git a/Platforms/Marvell/Armada/Armada.dsc.inc b/Platforms/Marvell/Armada/Armada.dsc.inc
>> index 222ab89cb45e..002c3b3ec961 100644
>> --- a/Platforms/Marvell/Armada/Armada.dsc.inc
>> +++ b/Platforms/Marvell/Armada/Armada.dsc.inc
>> @@ -371,7 +371,7 @@
>>
>>    # ARM Pcds
>>    gArmTokenSpaceGuid.PcdSystemMemoryBase|0
>> -  gArmTokenSpaceGuid.PcdSystemMemorySize|0x100000000
>> +  gArmTokenSpaceGuid.PcdSystemMemorySize|0x80000000
>>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|36
>>
>>    gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
>> diff --git a/Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S b/Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
>> index ba9685f1d36e..8246892f2585 100644
>> --- a/Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
>> +++ b/Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
>> @@ -26,37 +26,47 @@
>>  #define CCU_MC_RTBR_OFFSET              0x8
>>  #define CCU_MC_RTBR_TARGET_BASE(Base)   (((Base) >> 20) << 10)
>>
>> +#define SYSTEM_INFO_ADDRESS             0x4000000
>> +#define SYSINFO_ARRAY_SIZE              0x0
>> +#define SYSINFO_DRAM_CS0_SIZE           0x1
>> +
>>  ASM_FUNC(ArmPlatformPeiBootAction)
>>    mov   x29, xzr
>> +  mov   x28, x30
>>
>>    .if   FixedPcdGet64 (PcdSystemMemoryBase) != 0
>>    .err  PcdSystemMemoryBase should be 0x0 on this platform!
>>    .endif
>>
>>    .if   FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget)
>> -    //
>> -    // Remap the DRAM region [DramRemapTarget, DramRemapTarget + DramRemapSize)
>> -    // to the top of the DRAM space. This is mainly intended to free up 32-bit
>> -    // addressable memory for MMIO and PCI windows.
>> -    //
>> -    MOV32 (x0, CCU_MC_BASE)
>> -    MOV32 (w1, CCU_MC_RSBR_SOURCE_BASE (FixedPcdGet64 (PcdSystemMemorySize)))
>> -    MOV32 (w2, CCU_MC_RTBR_TARGET_BASE (FixedPcdGet32 (PcdDramRemapTarget)))
>> -    MOV32 (w3, CCU_MC_RCR_REMAP_SIZE (FixedPcdGet32 (PcdDramRemapSize)) | CCU_MC_RCR_REMAP_EN)
>> -    str   w1, [x0, #CCU_MC_RSBR_OFFSET]
>> -    str   w2, [x0, #CCU_MC_RTBR_OFFSET]
>> -    str   w3, [x0, #CCU_MC_RCR_OFFSET]
>> -
>> -    //
>> -    // Use the low range for UEFI itself. The remaining memory will be mapped
>> -    // and added to the GCD map later.
>> -    //
>> -    adr   x0, mSystemMemoryEnd
>> -    MOV64 (x1, FixedPcdGet32 (PcdDramRemapTarget) - 1)
>> -    str   x1, [x0]
>> +  .err  Hard coded PcdSystemMemorySize should not exceed PcdDramRemapTarget
>>    .endif
>>
>> -  MOV32 (x0, FixedPcdGet64 (PcdFvBaseAddress))
>> +  bl    DiscoverDramSize
>> +  cbz   w0, 0f
>
> This confused me until I got to the end of the patch - the amount of
> DRAM is initialized by default to PcdSystemMemorySize. Could you add a
> comment to that effect? i.e.
>
>   cbz  w0, 0f    // Use default PcdSystemMemorySize at address 0
>

OK

>> +
>> +  //
>> +  // Remap the DRAM region [DramRemapTarget, DramRemapTarget + DramRemapSize)
>> +  // to the top of the DRAM space. This is mainly intended to free up 32-bit
>> +  // addressable memory for MMIO and PCI windows.
>> +  //
>> +  lsl   w1, w0, #10          // set CCU_MC_RSBR_SOURCE_BASE to DRAM size in KiB
>> +  MOV32 (x0, CCU_MC_BASE)
>> +  MOV32 (w2, CCU_MC_RTBR_TARGET_BASE (FixedPcdGet32 (PcdDramRemapTarget)))
>> +  MOV32 (w3, CCU_MC_RCR_REMAP_SIZE (FixedPcdGet32 (PcdDramRemapSize)) | CCU_MC_RCR_REMAP_EN)
>> +  str   w1, [x0, #CCU_MC_RSBR_OFFSET]
>> +  str   w2, [x0, #CCU_MC_RTBR_OFFSET]
>> +  str   w3, [x0, #CCU_MC_RCR_OFFSET]
>> +
>> +  //
>> +  // Use the low range for UEFI itself. The remaining memory will be mapped
>> +  // and added to the GCD map later.
>> +  //
>> +  adr   x0, mSystemMemoryEnd
>> +  MOV64 (x1, FixedPcdGet32 (PcdDramRemapTarget) - 1)
>> +  str   x1, [x0]
>> +
>> +0:MOV32 (x0, FixedPcdGet64 (PcdFvBaseAddress))
>>    MOV32 (x3, FixedPcdGet32 (PcdFvSize))
>>    add   x3, x3, x0
>>
>> @@ -70,6 +80,38 @@ ASM_FUNC(ArmPlatformPeiBootAction)
>>    cmp   x0, x3
>>    b.lt  0b
>>
>> +  ret   x28
>> +
>> +DiscoverDramSize:
>> +  //
>> +  // ARM Trusted Firmware leaves us a structure in memory that describes the
>> +  // nature of the available DRAM. It consists of an array of <u32, u32>
>> +  // key/value pairs, where the only keys we are [currently] interested in are:
>> +  //
>> +  // 0x0 (ARRAY_SIZE)     the overall number of entries (should be the first)
>> +  // 0x1 (DRAM_CS0_SIZE)  the size in MiB of DRAM bank 0
>> +  //
>> +  MOV32 (x4, SYSTEM_INFO_ADDRESS)
>> +  ldp   w0, w1, [x4], #8
>> +
>> +  cmp   w0, #SYSINFO_ARRAY_SIZE   // first entry contains array size?
>> +  b.ne  1f
>> +
>> +0:subs  w1, w1, #1                // more entries to check?
>> +  b.eq  1f
>> +
>> +  ldp   w2, w3, [x4], #8          // next entry contains DRAM CS0 size?
>> +  cmp   w2, #SYSINFO_DRAM_CS0_SIZE
>> +  b.ne  0b
>> +
>> +  mov   w0, w3                    // return value in MiB
>> +
>> +  adr   x1, DiscoveredDramSize    // store value in bytes
>> +  lsl   x3, x3, #20
>> +  str   x3, [x1]
>> +  ret
>> +
>> +1:mov   w0, wzr
>>    ret
>>
>>  //UINTN
>> diff --git a/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S b/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
>> index 4cd8b3154e82..fdb688ca489a 100644
>> --- a/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
>> +++ b/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
>> @@ -26,36 +26,80 @@
>>  #define CCU_MC_RTBR_OFFSET              0x8
>>  #define CCU_MC_RTBR_TARGET_BASE(Base)   (((Base) >> 20) << 10)
>>
>> +#define SYSTEM_INFO_ADDRESS             0x4000000
>> +#define SYSINFO_ARRAY_SIZE              0x0
>> +#define SYSINFO_DRAM_CS0_SIZE           0x1
>> +
>
> So, this is not the end of the world, but I'd prefer it if these magic
> values weren't duplicated across the two versions.
>

Yeah. I gues that applies equally to all the defines in this file.
Perhaps I should simply .include a shared .S file here?

>>  ASM_FUNC(ArmPlatformPeiBootAction)
>> +  mov   r13, lr
>> +
>>    .if   FixedPcdGet64 (PcdSystemMemoryBase) != 0
>>    .err  PcdSystemMemoryBase should be 0x0 on this platform!
>>    .endif
>>
>>    .if   FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget)
>> -    //
>> -    // Remap the DRAM region [DramRemapTarget, DramRemapTarget + DramRemapSize)
>> -    // to the top of the DRAM space. This is mainly intended to free up 32-bit
>> -    // addressable memory for MMIO and PCI windows.
>> -    //
>> -    MOV32 (r0, CCU_MC_BASE)
>> -    MOV32 (r1, CCU_MC_RSBR_SOURCE_BASE (FixedPcdGet64 (PcdSystemMemorySize)))
>> -    MOV32 (r2, CCU_MC_RTBR_TARGET_BASE (FixedPcdGet32 (PcdDramRemapTarget)))
>> -    MOV32 (r3, CCU_MC_RCR_REMAP_SIZE (FixedPcdGet32 (PcdDramRemapSize)) | CCU_MC_RCR_REMAP_EN)
>> -    str   r1, [r0, #CCU_MC_RSBR_OFFSET]
>> -    str   r2, [r0, #CCU_MC_RTBR_OFFSET]
>> -    str   r3, [r0, #CCU_MC_RCR_OFFSET]
>> -
>> -    //
>> -    // Use the low range for UEFI itself. The remaining memory will be mapped
>> -    // and added to the GCD map later.
>> -    //
>> -    ADRL  (r0, mSystemMemoryEnd)
>> -    MOV32 (r2, FixedPcdGet32 (PcdDramRemapTarget) - 1)
>> -    mov   r3, #0
>> -    strd  r2, r3, [r0]
>> -
>> +  .err  Hard coded PcdSystemMemorySize should not exceed PcdDramRemapTarget
>>    .endif
>>
>> +  bl    DiscoverDramSize
>> +  cmp   r0, #0
>> +  beq   0f
>
> Same comment as 64-bit, // Use default PcdSystemMemorySize at address 0
> ?
>
>> +
>> +  //
>> +  // Remap the DRAM region [DramRemapTarget, DramRemapTarget + DramRemapSize)
>> +  // to the top of the DRAM space. This is mainly intended to free up 32-bit
>> +  // addressable memory for MMIO and PCI windows.
>> +  //
>> +  lsl   r1, r0, #10          // set CCU_MC_RSBR_SOURCE_BASE to DRAM size in KiB
>> +  MOV32 (r0, CCU_MC_BASE)
>> +  MOV32 (r2, CCU_MC_RTBR_TARGET_BASE (FixedPcdGet32 (PcdDramRemapTarget)))
>> +  MOV32 (r3, CCU_MC_RCR_REMAP_SIZE (FixedPcdGet32 (PcdDramRemapSize)) | CCU_MC_RCR_REMAP_EN)
>> +  str   r1, [r0, #CCU_MC_RSBR_OFFSET]
>> +  str   r2, [r0, #CCU_MC_RTBR_OFFSET]
>> +  str   r3, [r0, #CCU_MC_RCR_OFFSET]
>> +
>> +  //
>> +  // Use the low range for UEFI itself. The remaining memory will be mapped
>> +  // and added to the GCD map later.
>> +  //
>> +  ADRL  (r0, mSystemMemoryEnd)
>> +  MOV32 (r2, FixedPcdGet32 (PcdDramRemapTarget) - 1)
>> +  mov   r3, #0
>> +  strd  r2, r3, [r0]
>> +
>> +0:bx    r13
>> +
>> +DiscoverDramSize:
>> +  //
>> +  // ARM Trusted Firmware leaves us a structure in memory that describes the
>> +  // nature of the available DRAM. It consists of an array of <u32, u32>
>> +  // key/value pairs, where the only keys we are [currently] interested in are:
>> +  //
>> +  // 0x0 (ARRAY_SIZE)     the overall number of entries (should be the first)
>> +  // 0x1 (DRAM_CS0_SIZE)  the size in MiB of DRAM bank 0
>> +  //
>> +  MOV32 (r4, SYSTEM_INFO_ADDRESS)
>> +  ldrd  r0, r1, [r4], #8
>> +
>> +  cmp   r0, #SYSINFO_ARRAY_SIZE   // first entry contains array size?
>> +  bne   1f
>> +
>> +0:subs  r1, r1, #1                // more entries to check?
>> +  beq   1f
>> +
>> +  ldrd  r2, r3, [r4], #8          // next entry contains DRAM CS0 size?
>> +  cmp   r2, #SYSINFO_DRAM_CS0_SIZE
>> +  bne   0b
>> +
>> +  mov   r0, r3                    // return value in MiB
>> +
>> +  ADRL  (r1, DiscoveredDramSize)
>> +  lsl   r2, r3, #20               // store value in bytes
>> +  lsr   r3, r3, #12
>> +  strd  r2, r3, [r1]
>> +  bx    lr
>> +
>> +1:mov   r0, #0
>>    bx    lr
>>
>>  //UINTN
>> diff --git a/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>> index 4d80426c177c..85df57164b7f 100644
>> --- a/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>> +++ b/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>> @@ -47,6 +47,13 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>
>>  STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
>>
>> +//
>> +// This will be overwritten by ArmPlatformPeiBootAction() based on information
>> +// provided by the secure firmware. PcdSystemMemorySize should be a reasonable
>> +// default otherwise.
>> +//
>> +UINT64 DiscoveredDramSize = FixedPcdGet64 (PcdSystemMemorySize);
>
> Should this be mDiscoveredDramSize?
> (Would also make it harder to misparse in DiscoverDramSize :)
>

Yes.

>> +
>>  /**
>>    Return the Virtual Memory Map of your platform
>>
>> @@ -63,7 +70,6 @@ ArmPlatformGetVirtualMemoryMap (
>>    )
>>  {
>>    UINTN                         Index = 0;
>> -  UINT64                        MemSize;
>>    UINT64                        MemLowSize;
>>    UINT64                        MemHighStart;
>>    UINT64                        MemHighSize;
>> @@ -71,11 +77,10 @@ ArmPlatformGetVirtualMemoryMap (
>>
>>    ASSERT (VirtualMemoryMap != NULL);
>>
>> -  MemSize = FixedPcdGet64 (PcdSystemMemorySize);
>> -  MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), MemSize);
>> +  MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), DiscoveredDramSize);
>>    MemHighStart = (UINT64)FixedPcdGet64 (PcdDramRemapTarget) +
>>                   FixedPcdGet32 (PcdDramRemapSize);
>>
>> -  MemHighSize = MemSize - MemLowSize;
>> +  MemHighSize = DiscoveredDramSize - MemLowSize;
>
> Is the above operation still correct if DiscoverDramSize returned 0?
>

Yes. The same applies to the original MemSize: the value is only used
if it is positive, given the conditional in the hunk below. C allows
wrapping arithmetic of unsigned quantities, but I could just as easily
but this assignment inside the if ()

>>
>>    ResourceAttributes = (
>>        EFI_RESOURCE_ATTRIBUTE_PRESENT |
>> @@ -105,7 +110,7 @@ ArmPlatformGetVirtualMemoryMap (
>>    VirtualMemoryTable[Index].Length          = 0x20000000;
>>    VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>
>> -  if (MemSize > MemLowSize) {
>> +  if (DiscoveredDramSize > MemLowSize) {
>>      //
>>      // If we have more than MemLowSize worth of DRAM, the remainder will be
>>      // mapped at the top of the remapped window.
>> --
>> 2.9.3
>>
Leif Lindholm May 31, 2017, 10:05 a.m. | #3
On Tue, May 30, 2017 at 04:40:02PM +0000, Ard Biesheuvel wrote:
> >> diff --git a/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S b/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
> >> index 4cd8b3154e82..fdb688ca489a 100644
> >> --- a/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
> >> +++ b/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
> >> @@ -26,36 +26,80 @@
> >>  #define CCU_MC_RTBR_OFFSET              0x8
> >>  #define CCU_MC_RTBR_TARGET_BASE(Base)   (((Base) >> 20) << 10)
> >>
> >> +#define SYSTEM_INFO_ADDRESS             0x4000000
> >> +#define SYSINFO_ARRAY_SIZE              0x0
> >> +#define SYSINFO_DRAM_CS0_SIZE           0x1
> >> +
> >
> > So, this is not the end of the world, but I'd prefer it if these magic
> > values weren't duplicated across the two versions.
> 
> Yeah. I gues that applies equally to all the defines in this file.

That's probably true.

> Perhaps I should simply .include a shared .S file here?

Well, it's a .S, so it could be a .h with an __ASSEMBLER__ check?
But, yeah, a .include of a shared .S is fine too.

> >> diff --git a/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> >> index 4d80426c177c..85df57164b7f 100644
> >> --- a/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> >> +++ b/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c

> >> @@ -71,11 +77,10 @@ ArmPlatformGetVirtualMemoryMap (
> >>
> >>    ASSERT (VirtualMemoryMap != NULL);
> >>
> >> -  MemSize = FixedPcdGet64 (PcdSystemMemorySize);
> >> -  MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), MemSize);
> >> +  MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), DiscoveredDramSize);
> >>    MemHighStart = (UINT64)FixedPcdGet64 (PcdDramRemapTarget) +
> >>                   FixedPcdGet32 (PcdDramRemapSize);
> >>
> >> -  MemHighSize = MemSize - MemLowSize;
> >> +  MemHighSize = DiscoveredDramSize - MemLowSize;
> >
> > Is the above operation still correct if DiscoverDramSize returned 0?
> 
> Yes. The same applies to the original MemSize: the value is only used
> if it is positive, given the conditional in the hunk below. C allows
> wrapping arithmetic of unsigned quantities, but I could just as easily
> but this assignment inside the if ()

Yes please, that wold be clearer.

/
    Leif

> >>
> >>    ResourceAttributes = (
> >>        EFI_RESOURCE_ATTRIBUTE_PRESENT |
> >> @@ -105,7 +110,7 @@ ArmPlatformGetVirtualMemoryMap (
> >>    VirtualMemoryTable[Index].Length          = 0x20000000;
> >>    VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >>
> >> -  if (MemSize > MemLowSize) {
> >> +  if (DiscoveredDramSize > MemLowSize) {
> >>      //
> >>      // If we have more than MemLowSize worth of DRAM, the remainder will be
> >>      // mapped at the top of the remapped window.
> >> --
> >> 2.9.3
> >>

Patch hide | download patch | download mbox

diff --git a/Platforms/Marvell/Armada/Armada.dsc.inc b/Platforms/Marvell/Armada/Armada.dsc.inc
index 222ab89cb45e..002c3b3ec961 100644
--- a/Platforms/Marvell/Armada/Armada.dsc.inc
+++ b/Platforms/Marvell/Armada/Armada.dsc.inc
@@ -371,7 +371,7 @@ 
 
   # ARM Pcds
   gArmTokenSpaceGuid.PcdSystemMemoryBase|0
-  gArmTokenSpaceGuid.PcdSystemMemorySize|0x100000000
+  gArmTokenSpaceGuid.PcdSystemMemorySize|0x80000000
   gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|36
 
   gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
diff --git a/Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S b/Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
index ba9685f1d36e..8246892f2585 100644
--- a/Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
+++ b/Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
@@ -26,37 +26,47 @@ 
 #define CCU_MC_RTBR_OFFSET              0x8
 #define CCU_MC_RTBR_TARGET_BASE(Base)   (((Base) >> 20) << 10)
 
+#define SYSTEM_INFO_ADDRESS             0x4000000
+#define SYSINFO_ARRAY_SIZE              0x0
+#define SYSINFO_DRAM_CS0_SIZE           0x1
+
 ASM_FUNC(ArmPlatformPeiBootAction)
   mov   x29, xzr
+  mov   x28, x30
 
   .if   FixedPcdGet64 (PcdSystemMemoryBase) != 0
   .err  PcdSystemMemoryBase should be 0x0 on this platform!
   .endif
 
   .if   FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget)
-    //
-    // Remap the DRAM region [DramRemapTarget, DramRemapTarget + DramRemapSize)
-    // to the top of the DRAM space. This is mainly intended to free up 32-bit
-    // addressable memory for MMIO and PCI windows.
-    //
-    MOV32 (x0, CCU_MC_BASE)
-    MOV32 (w1, CCU_MC_RSBR_SOURCE_BASE (FixedPcdGet64 (PcdSystemMemorySize)))
-    MOV32 (w2, CCU_MC_RTBR_TARGET_BASE (FixedPcdGet32 (PcdDramRemapTarget)))
-    MOV32 (w3, CCU_MC_RCR_REMAP_SIZE (FixedPcdGet32 (PcdDramRemapSize)) | CCU_MC_RCR_REMAP_EN)
-    str   w1, [x0, #CCU_MC_RSBR_OFFSET]
-    str   w2, [x0, #CCU_MC_RTBR_OFFSET]
-    str   w3, [x0, #CCU_MC_RCR_OFFSET]
-
-    //
-    // Use the low range for UEFI itself. The remaining memory will be mapped
-    // and added to the GCD map later.
-    //
-    adr   x0, mSystemMemoryEnd
-    MOV64 (x1, FixedPcdGet32 (PcdDramRemapTarget) - 1)
-    str   x1, [x0]
+  .err  Hard coded PcdSystemMemorySize should not exceed PcdDramRemapTarget
   .endif
 
-  MOV32 (x0, FixedPcdGet64 (PcdFvBaseAddress))
+  bl    DiscoverDramSize
+  cbz   w0, 0f
+
+  //
+  // Remap the DRAM region [DramRemapTarget, DramRemapTarget + DramRemapSize)
+  // to the top of the DRAM space. This is mainly intended to free up 32-bit
+  // addressable memory for MMIO and PCI windows.
+  //
+  lsl   w1, w0, #10          // set CCU_MC_RSBR_SOURCE_BASE to DRAM size in KiB
+  MOV32 (x0, CCU_MC_BASE)
+  MOV32 (w2, CCU_MC_RTBR_TARGET_BASE (FixedPcdGet32 (PcdDramRemapTarget)))
+  MOV32 (w3, CCU_MC_RCR_REMAP_SIZE (FixedPcdGet32 (PcdDramRemapSize)) | CCU_MC_RCR_REMAP_EN)
+  str   w1, [x0, #CCU_MC_RSBR_OFFSET]
+  str   w2, [x0, #CCU_MC_RTBR_OFFSET]
+  str   w3, [x0, #CCU_MC_RCR_OFFSET]
+
+  //
+  // Use the low range for UEFI itself. The remaining memory will be mapped
+  // and added to the GCD map later.
+  //
+  adr   x0, mSystemMemoryEnd
+  MOV64 (x1, FixedPcdGet32 (PcdDramRemapTarget) - 1)
+  str   x1, [x0]
+
+0:MOV32 (x0, FixedPcdGet64 (PcdFvBaseAddress))
   MOV32 (x3, FixedPcdGet32 (PcdFvSize))
   add   x3, x3, x0
 
@@ -70,6 +80,38 @@  ASM_FUNC(ArmPlatformPeiBootAction)
   cmp   x0, x3
   b.lt  0b
 
+  ret   x28
+
+DiscoverDramSize:
+  //
+  // ARM Trusted Firmware leaves us a structure in memory that describes the
+  // nature of the available DRAM. It consists of an array of <u32, u32>
+  // key/value pairs, where the only keys we are [currently] interested in are:
+  //
+  // 0x0 (ARRAY_SIZE)     the overall number of entries (should be the first)
+  // 0x1 (DRAM_CS0_SIZE)  the size in MiB of DRAM bank 0
+  //
+  MOV32 (x4, SYSTEM_INFO_ADDRESS)
+  ldp   w0, w1, [x4], #8
+
+  cmp   w0, #SYSINFO_ARRAY_SIZE   // first entry contains array size?
+  b.ne  1f
+
+0:subs  w1, w1, #1                // more entries to check?
+  b.eq  1f
+
+  ldp   w2, w3, [x4], #8          // next entry contains DRAM CS0 size?
+  cmp   w2, #SYSINFO_DRAM_CS0_SIZE
+  b.ne  0b
+
+  mov   w0, w3                    // return value in MiB
+
+  adr   x1, DiscoveredDramSize    // store value in bytes
+  lsl   x3, x3, #20
+  str   x3, [x1]
+  ret
+
+1:mov   w0, wzr
   ret
 
 //UINTN
diff --git a/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S b/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
index 4cd8b3154e82..fdb688ca489a 100644
--- a/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
+++ b/Platforms/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
@@ -26,36 +26,80 @@ 
 #define CCU_MC_RTBR_OFFSET              0x8
 #define CCU_MC_RTBR_TARGET_BASE(Base)   (((Base) >> 20) << 10)
 
+#define SYSTEM_INFO_ADDRESS             0x4000000
+#define SYSINFO_ARRAY_SIZE              0x0
+#define SYSINFO_DRAM_CS0_SIZE           0x1
+
 ASM_FUNC(ArmPlatformPeiBootAction)
+  mov   r13, lr
+
   .if   FixedPcdGet64 (PcdSystemMemoryBase) != 0
   .err  PcdSystemMemoryBase should be 0x0 on this platform!
   .endif
 
   .if   FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget)
-    //
-    // Remap the DRAM region [DramRemapTarget, DramRemapTarget + DramRemapSize)
-    // to the top of the DRAM space. This is mainly intended to free up 32-bit
-    // addressable memory for MMIO and PCI windows.
-    //
-    MOV32 (r0, CCU_MC_BASE)
-    MOV32 (r1, CCU_MC_RSBR_SOURCE_BASE (FixedPcdGet64 (PcdSystemMemorySize)))
-    MOV32 (r2, CCU_MC_RTBR_TARGET_BASE (FixedPcdGet32 (PcdDramRemapTarget)))
-    MOV32 (r3, CCU_MC_RCR_REMAP_SIZE (FixedPcdGet32 (PcdDramRemapSize)) | CCU_MC_RCR_REMAP_EN)
-    str   r1, [r0, #CCU_MC_RSBR_OFFSET]
-    str   r2, [r0, #CCU_MC_RTBR_OFFSET]
-    str   r3, [r0, #CCU_MC_RCR_OFFSET]
-
-    //
-    // Use the low range for UEFI itself. The remaining memory will be mapped
-    // and added to the GCD map later.
-    //
-    ADRL  (r0, mSystemMemoryEnd)
-    MOV32 (r2, FixedPcdGet32 (PcdDramRemapTarget) - 1)
-    mov   r3, #0
-    strd  r2, r3, [r0]
-
+  .err  Hard coded PcdSystemMemorySize should not exceed PcdDramRemapTarget
   .endif
 
+  bl    DiscoverDramSize
+  cmp   r0, #0
+  beq   0f
+
+  //
+  // Remap the DRAM region [DramRemapTarget, DramRemapTarget + DramRemapSize)
+  // to the top of the DRAM space. This is mainly intended to free up 32-bit
+  // addressable memory for MMIO and PCI windows.
+  //
+  lsl   r1, r0, #10          // set CCU_MC_RSBR_SOURCE_BASE to DRAM size in KiB
+  MOV32 (r0, CCU_MC_BASE)
+  MOV32 (r2, CCU_MC_RTBR_TARGET_BASE (FixedPcdGet32 (PcdDramRemapTarget)))
+  MOV32 (r3, CCU_MC_RCR_REMAP_SIZE (FixedPcdGet32 (PcdDramRemapSize)) | CCU_MC_RCR_REMAP_EN)
+  str   r1, [r0, #CCU_MC_RSBR_OFFSET]
+  str   r2, [r0, #CCU_MC_RTBR_OFFSET]
+  str   r3, [r0, #CCU_MC_RCR_OFFSET]
+
+  //
+  // Use the low range for UEFI itself. The remaining memory will be mapped
+  // and added to the GCD map later.
+  //
+  ADRL  (r0, mSystemMemoryEnd)
+  MOV32 (r2, FixedPcdGet32 (PcdDramRemapTarget) - 1)
+  mov   r3, #0
+  strd  r2, r3, [r0]
+
+0:bx    r13
+
+DiscoverDramSize:
+  //
+  // ARM Trusted Firmware leaves us a structure in memory that describes the
+  // nature of the available DRAM. It consists of an array of <u32, u32>
+  // key/value pairs, where the only keys we are [currently] interested in are:
+  //
+  // 0x0 (ARRAY_SIZE)     the overall number of entries (should be the first)
+  // 0x1 (DRAM_CS0_SIZE)  the size in MiB of DRAM bank 0
+  //
+  MOV32 (r4, SYSTEM_INFO_ADDRESS)
+  ldrd  r0, r1, [r4], #8
+
+  cmp   r0, #SYSINFO_ARRAY_SIZE   // first entry contains array size?
+  bne   1f
+
+0:subs  r1, r1, #1                // more entries to check?
+  beq   1f
+
+  ldrd  r2, r3, [r4], #8          // next entry contains DRAM CS0 size?
+  cmp   r2, #SYSINFO_DRAM_CS0_SIZE
+  bne   0b
+
+  mov   r0, r3                    // return value in MiB
+
+  ADRL  (r1, DiscoveredDramSize)
+  lsl   r2, r3, #20               // store value in bytes
+  lsr   r3, r3, #12
+  strd  r2, r3, [r1]
+  bx    lr
+
+1:mov   r0, #0
   bx    lr
 
 //UINTN
diff --git a/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
index 4d80426c177c..85df57164b7f 100644
--- a/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
+++ b/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
@@ -47,6 +47,13 @@  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
 
+//
+// This will be overwritten by ArmPlatformPeiBootAction() based on information
+// provided by the secure firmware. PcdSystemMemorySize should be a reasonable
+// default otherwise.
+//
+UINT64 DiscoveredDramSize = FixedPcdGet64 (PcdSystemMemorySize);
+
 /**
   Return the Virtual Memory Map of your platform
 
@@ -63,7 +70,6 @@  ArmPlatformGetVirtualMemoryMap (
   )
 {
   UINTN                         Index = 0;
-  UINT64                        MemSize;
   UINT64                        MemLowSize;
   UINT64                        MemHighStart;
   UINT64                        MemHighSize;
@@ -71,11 +77,10 @@  ArmPlatformGetVirtualMemoryMap (
 
   ASSERT (VirtualMemoryMap != NULL);
 
-  MemSize = FixedPcdGet64 (PcdSystemMemorySize);
-  MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), MemSize);
+  MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), DiscoveredDramSize);
   MemHighStart = (UINT64)FixedPcdGet64 (PcdDramRemapTarget) +
                  FixedPcdGet32 (PcdDramRemapSize);
-  MemHighSize = MemSize - MemLowSize;
+  MemHighSize = DiscoveredDramSize - MemLowSize;
 
   ResourceAttributes = (
       EFI_RESOURCE_ATTRIBUTE_PRESENT |
@@ -105,7 +110,7 @@  ArmPlatformGetVirtualMemoryMap (
   VirtualMemoryTable[Index].Length          = 0x20000000;
   VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
-  if (MemSize > MemLowSize) {
+  if (DiscoveredDramSize > MemLowSize) {
     //
     // If we have more than MemLowSize worth of DRAM, the remainder will be
     // mapped at the top of the remapped window.