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

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