[PATCH 0/2] Virtual Memory Map Fix for VE

[PATCH 0/2] Virtual Memory Map Fix for VE

[evan.lloyd]

From: Evan Lloyd <evan.lloyd@arm.com>

This patch set fixes an allocation error in the Versatile Express
RTSMMem.c.
An extra patch is included that merely tidies the code format before the
real fix is applied.

The resultant code is available for examination at:
https://github.com/EvanLloyd/tianocore/tree/649_VMM_v1

Evan Lloyd (1):
  ArmPlatformPkg: Reformat VE Memory Map code

Sami Mujawar (1):
  ArmPlatformPkg: Fix VE RTSM mem map descriptor count

 ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c | 104 +++++++++-----------
 1 file changed, 48 insertions(+), 56 deletions(-)

-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

[PATCH 1/2] ArmPlatformPkg: Reformat VE Memory Map code

[evan.lloyd]

From: Evan Lloyd <evan.lloyd@arm.com>

This change is purely cosmetic, with no functional impact, and only
exists to isolate cosmetic changes from a functional fix.
    Some indentation is adjusted.
    Overlength lines are re-flowed.
    alignment on = is adjusted as some lines exceeded 80 columns.
    if statement converted to conditional assignment.
    Redundant re-calculation of CacheAttributes removed.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
---
 ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c | 99 +++++++++-----------
 1 file changed, 45 insertions(+), 54 deletions(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
index 115df246796d23081dadcbda94f743bc63233f56..14541183d1eb6916690823d7e04507fc50639f5f 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
@@ -24,17 +24,19 @@
 #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          6
 
 // DDR attributes
-#define DDR_ATTRIBUTES_CACHED           ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
-#define DDR_ATTRIBUTES_UNCACHED         ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED
+#define DDR_ATTRIBUTES_CACHED   ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
+#define DDR_ATTRIBUTES_UNCACHED ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED
 
 /**
   Return the Virtual Memory Map of your platform
 
-  This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU on your platform.
+  This Virtual Memory Map is used by MemoryInitPei Module to initialize
+  the MMU on your platform.
 
-  @param[out]   VirtualMemoryMap    Array of ARM_MEMORY_REGION_DESCRIPTOR describing a Physical-to-
-                                    Virtual Memory mapping. This array must be ended by a zero-filled
-                                    entry
+  @param[out]   VirtualMemoryMap    Array of ARM_MEMORY_REGION_DESCRIPTOR
+                                    describing a Physical-to-Virtual Memory
+                                    mapping. This array must be ended by a
+                                    zero-filled entry.
 
 **/
 VOID
@@ -59,12 +61,12 @@ ArmPlatformGetVirtualMemoryMap (
     HasSparseMemory = TRUE;
 
     ResourceAttributes =
-        EFI_RESOURCE_ATTRIBUTE_PRESENT |
-        EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
-        EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
-        EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
-        EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
-        EFI_RESOURCE_ATTRIBUTE_TESTED;
+      EFI_RESOURCE_ATTRIBUTE_PRESENT |
+      EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
+      EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
+      EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
+      EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
+      EFI_RESOURCE_ATTRIBUTE_TESTED;
 
     // Declared the additional DRAM from 2GB to 4GB
     SparseMemoryBase = 0x0880000000;
@@ -81,77 +83,66 @@ ArmPlatformGetVirtualMemoryMap (
     SparseMemorySize = 0x0;
   }
 
-  VirtualMemoryTable = (ARM_MEMORY_REGION_DESCRIPTOR*)AllocatePages(EFI_SIZE_TO_PAGES (sizeof(ARM_MEMORY_REGION_DESCRIPTOR) * MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS));
+  VirtualMemoryTable = (ARM_MEMORY_REGION_DESCRIPTOR*)
+    AllocatePages (EFI_SIZE_TO_PAGES (sizeof(ARM_MEMORY_REGION_DESCRIPTOR)
+                                      * MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS));
   if (VirtualMemoryTable == NULL) {
-      return;
+    return;
   }
 
-  if (FeaturePcdGet(PcdCacheEnable) == TRUE) {
-      CacheAttributes = DDR_ATTRIBUTES_CACHED;
-  } else {
-      CacheAttributes = DDR_ATTRIBUTES_UNCACHED;
-  }
+  CacheAttributes = (FeaturePcdGet(PcdCacheEnable))
+                    ? DDR_ATTRIBUTES_CACHED
+                    : DDR_ATTRIBUTES_UNCACHED;
 
   // ReMap (Either NOR Flash or DRAM)
   VirtualMemoryTable[Index].PhysicalBase = ARM_VE_REMAP_BASE;
-  VirtualMemoryTable[Index].VirtualBase  = ARM_VE_REMAP_BASE;
-  VirtualMemoryTable[Index].Length       = ARM_VE_REMAP_SZ;
-
-  if (FeaturePcdGet(PcdNorFlashRemapping) == FALSE) {
-    // Map the NOR Flash as Secure Memory
-    if (FeaturePcdGet(PcdCacheEnable) == TRUE) {
-      VirtualMemoryTable[Index].Attributes   = DDR_ATTRIBUTES_CACHED;
-    } else {
-      VirtualMemoryTable[Index].Attributes   = DDR_ATTRIBUTES_UNCACHED;
-    }
-  } else {
-    // DRAM mapping
-    VirtualMemoryTable[Index].Attributes   = CacheAttributes;
-  }
+  VirtualMemoryTable[Index].VirtualBase = ARM_VE_REMAP_BASE;
+  VirtualMemoryTable[Index].Length = ARM_VE_REMAP_SZ;
+  VirtualMemoryTable[Index].Attributes = CacheAttributes;
 
   // DDR
   VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_DRAM_BASE;
-  VirtualMemoryTable[Index].VirtualBase  = ARM_VE_DRAM_BASE;
-  VirtualMemoryTable[Index].Length       = ARM_VE_DRAM_SZ;
-  VirtualMemoryTable[Index].Attributes   = CacheAttributes;
+  VirtualMemoryTable[Index].VirtualBase = ARM_VE_DRAM_BASE;
+  VirtualMemoryTable[Index].Length = ARM_VE_DRAM_SZ;
+  VirtualMemoryTable[Index].Attributes = CacheAttributes;
 
   // CPU peripherals. TRM. Manual says not all of them are implemented.
   VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_ON_CHIP_PERIPH_BASE;
-  VirtualMemoryTable[Index].VirtualBase  = ARM_VE_ON_CHIP_PERIPH_BASE;
-  VirtualMemoryTable[Index].Length       = ARM_VE_ON_CHIP_PERIPH_SZ;
-  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+  VirtualMemoryTable[Index].VirtualBase = ARM_VE_ON_CHIP_PERIPH_BASE;
+  VirtualMemoryTable[Index].Length = ARM_VE_ON_CHIP_PERIPH_SZ;
+  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
   // SMB CS0-CS1 - NOR Flash 1 & 2
   VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_SMB_NOR0_BASE;
-  VirtualMemoryTable[Index].VirtualBase  = ARM_VE_SMB_NOR0_BASE;
-  VirtualMemoryTable[Index].Length       = ARM_VE_SMB_NOR0_SZ + ARM_VE_SMB_NOR1_SZ;
-  VirtualMemoryTable[Index].Attributes   = CacheAttributes;
+  VirtualMemoryTable[Index].VirtualBase = ARM_VE_SMB_NOR0_BASE;
+  VirtualMemoryTable[Index].Length = ARM_VE_SMB_NOR0_SZ + ARM_VE_SMB_NOR1_SZ;
+  VirtualMemoryTable[Index].Attributes = CacheAttributes;
 
   // SMB CS2 - SRAM
   VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_SMB_SRAM_BASE;
-  VirtualMemoryTable[Index].VirtualBase  = ARM_VE_SMB_SRAM_BASE;
-  VirtualMemoryTable[Index].Length       = ARM_VE_SMB_SRAM_SZ;
-  VirtualMemoryTable[Index].Attributes   = CacheAttributes;
+  VirtualMemoryTable[Index].VirtualBase = ARM_VE_SMB_SRAM_BASE;
+  VirtualMemoryTable[Index].Length = ARM_VE_SMB_SRAM_SZ;
+  VirtualMemoryTable[Index].Attributes = CacheAttributes;
 
   // Peripheral CS2 and CS3
   VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_SMB_PERIPH_BASE;
-  VirtualMemoryTable[Index].VirtualBase  = ARM_VE_SMB_PERIPH_BASE;
-  VirtualMemoryTable[Index].Length       = 2 * ARM_VE_SMB_PERIPH_SZ;
-  VirtualMemoryTable[Index].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+  VirtualMemoryTable[Index].VirtualBase = ARM_VE_SMB_PERIPH_BASE;
+  VirtualMemoryTable[Index].Length = 2 * ARM_VE_SMB_PERIPH_SZ;
+  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
   // Map sparse memory region if present
   if (HasSparseMemory) {
     VirtualMemoryTable[++Index].PhysicalBase = SparseMemoryBase;
-    VirtualMemoryTable[Index].VirtualBase    = SparseMemoryBase;
-    VirtualMemoryTable[Index].Length         = SparseMemorySize;
-    VirtualMemoryTable[Index].Attributes     = CacheAttributes;
+    VirtualMemoryTable[Index].VirtualBase = SparseMemoryBase;
+    VirtualMemoryTable[Index].Length = SparseMemorySize;
+    VirtualMemoryTable[Index].Attributes = CacheAttributes;
   }
 
   // End of Table
   VirtualMemoryTable[++Index].PhysicalBase = 0;
-  VirtualMemoryTable[Index].VirtualBase  = 0;
-  VirtualMemoryTable[Index].Length       = 0;
-  VirtualMemoryTable[Index].Attributes   = (ARM_MEMORY_REGION_ATTRIBUTES)0;
+  VirtualMemoryTable[Index].VirtualBase = 0;
+  VirtualMemoryTable[Index].Length = 0;
+  VirtualMemoryTable[Index].Attributes = (ARM_MEMORY_REGION_ATTRIBUTES)0;
 
   *VirtualMemoryMap = VirtualMemoryTable;
 }
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

[PATCH 2/2] ArmPlatformPkg: Fix VE RTSM mem map descriptor count

[evan.lloyd]

From: Sami Mujawar <sami.mujawar@arm.com>

The number of memory map entries used exceeded the allocated count,
thereby causing memory corruption.

Fixed the number of Virtual Memory Map Descriptors allocated in
describing the RTSM Memory Map. Also added an assert to confirm
that the descriptor count has not been exceeded, in the hope that it may
help highlight the problem should a new entry be added.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
---
 ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
index 14541183d1eb6916690823d7e04507fc50639f5f..14c7e8e1d6729f4ca91ef42520b1e5765d22b318 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
+*  Copyright (c) 2011-2016, ARM Limited. All rights reserved.
 *
 *  This program and the accompanying materials
 *  are licensed and made available under the terms and conditions of the BSD License
@@ -21,7 +21,7 @@
 #include <ArmPlatform.h>
 
 // Number of Virtual Memory Map Descriptors
-#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          6
+#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          8
 
 // DDR attributes
 #define DDR_ATTRIBUTES_CACHED   ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
@@ -144,5 +144,6 @@ ArmPlatformGetVirtualMemoryMap (
   VirtualMemoryTable[Index].Length = 0;
   VirtualMemoryTable[Index].Attributes = (ARM_MEMORY_REGION_ATTRIBUTES)0;
 
+  ASSERT (Index < MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
   *VirtualMemoryMap = VirtualMemoryTable;
 }
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

Re: [PATCH 0/2] Virtual Memory Map Fix for VE

[Ard Biesheuvel]

On 24 November 2016 at 19:56,  <evan.lloyd@arm.com> wrote:
> From: Evan Lloyd <evan.lloyd@arm.com>
>
> This patch set fixes an allocation error in the Versatile Express
> RTSMMem.c.
> An extra patch is included that merely tidies the code format before the
> real fix is applied.
>
> The resultant code is available for examination at:
> https://github.com/EvanLloyd/tianocore/tree/649_VMM_v1
>

Pushed as

05153ff2219d ArmPlatformPkg: Reformat VE Memory Map code
4575a602ca60 ArmPlatformPkg: Fix VE RTSM mem map descriptor count

Thanks,
Ard.

> Evan Lloyd (1):
>   ArmPlatformPkg: Reformat VE Memory Map code
>
> Sami Mujawar (1):
>   ArmPlatformPkg: Fix VE RTSM mem map descriptor count
>
>  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c | 104 +++++++++-----------
>  1 file changed, 48 insertions(+), 56 deletions(-)
>
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>