[PATCH v2 0/5] ArmPlatformPkg: map VRAM using memory semantics

[PATCH v2 0/5] ArmPlatformPkg: map VRAM using memory semantics

[Ard Biesheuvel]

As reported by Jeremy, the recently introduced accelerated memcpy/memset
routines are causing problems when used on framebuffer memory. While
framebuffers are arguably right on the blurry line between MMIO (with
side effects that are sensitive to ordering) and memory, mapping VRAM
as device memory is unnecessary to begin with, and so we can improve
our situation by using memory semantics for the VRAM mappings. (Whether
we end up reverting to the unaccelerated memcpy/memset routines is a
separate matter)

So fix both the FVP case, which has a separate VRAM region, and TC2, which
allocates VRAM from normal memory and changes the mapping attributes.

Note to Ryan: this fixes FVP Base model for me, but not the Foundation model
(whose PL111 support I never saw working to begin with, so I am not sure what
is going on there)

Ard Biesheuvel (5):
  ArmPlatformPkg/FVP: map motherboard VRAM as uncached memory
  ArmPlatformPkg/HdLcdArmVExpressLib: fix incorrect FreePool () call
  ArmPlatformPkg/PL111LcdArmVExpressLib: fix incorrect FreePool () call
  ArmPlatformPkg/HdLcdArmVExpressLib: use write-combine mapping for VRAM
  ArmPlatformPkg/PL111LcdArmVExpressLib: use write-combine mapping for
    VRAM

 ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h                       | 10 ++++++----
 ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c                      |  8 +++++++-
 ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c            | 14 +++++---------
 ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf       |  3 ++-
 ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c      | 14 +++++---------
 ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf |  3 ++-
 6 files changed, 27 insertions(+), 25 deletions(-)

-- 
2.9.3

[PATCH v2 1/5] ArmPlatformPkg/FVP: map motherboard VRAM as uncached memory

[Ard Biesheuvel]

The VRAM of the PL111 on the FVP Base/Foundation models is described as
device memory rather than uncached memory, which is not an accurate
description of the nature of the region (i.e., a framebuffer), and may
result in problems when using accelerated string routines to access the
region, since this may legally involve unaligned accesses or DC ZVA
instructions, which are not allowed on device mappings.

So split of the 8 MB VRAM region into a separate region, and map it using
memory attributes.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h  | 10 ++++++----
 ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c |  8 +++++++-
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h b/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h
index 06414e6e7208..4e17c800a34f 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h
+++ b/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h
@@ -40,9 +40,11 @@
 #define ARM_VE_SMB_SRAM_BASE                    0x2E000000
 #define ARM_VE_SMB_SRAM_SZ                      SIZE_64KB
 // USB, Ethernet, VRAM
-#define ARM_VE_SMB_PERIPH_BASE                  0x18000000
-#define PL111_CLCD_VRAM_MOTHERBOARD_BASE        ARM_VE_SMB_PERIPH_BASE
-#define ARM_VE_SMB_PERIPH_SZ                    SIZE_64MB
+#define ARM_VE_SMB_PERIPH_BASE                  0x18800000
+#define ARM_VE_SMB_PERIPH_SZ                    (SIZE_64MB - SIZE_8MB)
+
+#define PL111_CLCD_VRAM_MOTHERBOARD_BASE        0x18000000
+#define PL111_CLCD_VRAM_MOTHERBOARD_SIZE        SIZE_8MB
 
 // DRAM
 #define ARM_VE_DRAM_BASE                        PcdGet64 (PcdSystemMemoryBase)
@@ -75,6 +77,6 @@
 #define PL111_CLCD_CORE_TILE_VIDEO_MODE_OSC_ID  1
 
 // VRAM offset for the PL111 Colour LCD Controller on the motherboard
-#define VRAM_MOTHERBOARD_BASE                     (ARM_VE_SMB_PERIPH_BASE   + 0x00000)
+#define VRAM_MOTHERBOARD_BASE                     (PL111_CLCD_VRAM_MOTHERBOARD_BASE)
 
 #endif
diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
index 14c7e8e1d672..70c17ae70478 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
@@ -21,7 +21,7 @@
 #include <ArmPlatform.h>
 
 // Number of Virtual Memory Map Descriptors
-#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          8
+#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          9
 
 // DDR attributes
 #define DDR_ATTRIBUTES_CACHED   ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
@@ -130,6 +130,12 @@ ArmPlatformGetVirtualMemoryMap (
   VirtualMemoryTable[Index].Length = 2 * ARM_VE_SMB_PERIPH_SZ;
   VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
+  // VRAM
+  VirtualMemoryTable[++Index].PhysicalBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
+  VirtualMemoryTable[Index].VirtualBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
+  VirtualMemoryTable[Index].Length = PL111_CLCD_VRAM_MOTHERBOARD_SIZE;
+  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
+
   // Map sparse memory region if present
   if (HasSparseMemory) {
     VirtualMemoryTable[++Index].PhysicalBase = SparseMemoryBase;
-- 
2.9.3

[PATCH v2 2/5] ArmPlatformPkg/HdLcdArmVExpressLib: fix incorrect FreePool () call

[Ard Biesheuvel]

When we fail to modify the memory attributes for the VRAM allocation,
the allocation - which was made using AllocatePages() - is freed using
FreePool(). This is incorrect by itself, but it masks a second bug, i.e.,
that the address of the allocation is not in VramBaseAddress but in
*VramBaseAddress. So fix both issues.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
index a57846715ed7..67b2f14beee3 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
@@ -146,7 +146,7 @@ LcdPlatformGetVram (
   Status = Cpu->SetMemoryAttributes (Cpu, *VramBaseAddress, *VramSize, EFI_MEMORY_UC);
   ASSERT_EFI_ERROR(Status);
   if (EFI_ERROR(Status)) {
-    gBS->FreePool (VramBaseAddress);
+    gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
     return Status;
   }
 
-- 
2.9.3

[PATCH v2 3/5] ArmPlatformPkg/PL111LcdArmVExpressLib: fix incorrect FreePool () call

[Ard Biesheuvel]

When we fail to modify the memory attributes for the VRAM allocation,
the allocation - which was made using AllocatePages() - is freed using
FreePool(). This is incorrect by itself, but it masks a second bug, i.e.,
that the address of the allocation is not in VramBaseAddress but in
*VramBaseAddress. So fix both issues.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
index 2000c9bdf436..a8125e81daac 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
@@ -195,7 +195,7 @@ LcdPlatformGetVram (
     Status = Cpu->SetMemoryAttributes(Cpu, *VramBaseAddress, *VramSize, EFI_MEMORY_UC);
     ASSERT_EFI_ERROR(Status);
     if (EFI_ERROR(Status)) {
-      gBS->FreePool(VramBaseAddress);
+      gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES(*VramSize));
       return Status;
     }
     break;
-- 
2.9.3

[PATCH v2 4/5] ArmPlatformPkg/HdLcdArmVExpressLib: use write-combine mapping for VRAM

[Ard Biesheuvel]

Replace the uncached memory mapping of the framebuffer with a write-
combining one. This improves performance, and avoids issues with
unaligned accesses and DC ZVA instructions performed by the accelerated
memcpy/memset routines.

Instead of manipulating the memory attributes directly, use the
SetMemorySpaceAttributes() DXE services, which validates the attributes
against the capabilities of the region before making the actual change.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c      | 12 ++++--------
 ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf |  3 ++-
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
index 67b2f14beee3..b1106ee19b98 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
@@ -18,11 +18,11 @@
 #include <Library/IoLib.h>
 #include <Library/PcdLib.h>
 #include <Library/DebugLib.h>
+#include <Library/DxeServicesTableLib.h>
 #include <Library/LcdPlatformLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 
-#include <Protocol/Cpu.h>
 #include <Protocol/EdidDiscovered.h>
 #include <Protocol/EdidActive.h>
 
@@ -119,7 +119,6 @@ LcdPlatformGetVram (
   )
 {
   EFI_STATUS              Status;
-  EFI_CPU_ARCH_PROTOCOL  *Cpu;
   EFI_ALLOCATE_TYPE       AllocationType;
 
   // Set the vram size
@@ -138,12 +137,9 @@ LcdPlatformGetVram (
     return Status;
   }
 
-  // Ensure the Cpu architectural protocol is already installed
-  Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&Cpu);
-  ASSERT_EFI_ERROR(Status);
-
-  // Mark the VRAM as un-cacheable. The VRAM is inside the DRAM, which is cacheable.
-  Status = Cpu->SetMemoryAttributes (Cpu, *VramBaseAddress, *VramSize, EFI_MEMORY_UC);
+  // Mark the VRAM as write-combining. The VRAM is inside the DRAM, which is cacheable.
+  Status = gDS->SetMemorySpaceAttributes (*VramBaseAddress, *VramSize,
+                  EFI_MEMORY_WC);
   ASSERT_EFI_ERROR(Status);
   if (EFI_ERROR(Status)) {
     gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf
index 780724737929..dff17e86fd3e 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf
@@ -32,8 +32,9 @@ [Packages]
   ArmPlatformPkg/ArmVExpressPkg/ArmVExpressPkg.dec
 
 [LibraryClasses]
-  BaseLib
   ArmPlatformSysConfigLib
+  BaseLib
+  DxeServicesTableLib
 
 [Protocols]
   gEfiEdidDiscoveredProtocolGuid                # Produced
-- 
2.9.3

[PATCH v2 5/5] ArmPlatformPkg/PL111LcdArmVExpressLib: use write-combine mapping for VRAM

[Ard Biesheuvel]

Replace the uncached memory mapping of the framebuffer with a write-
combining one. This improves performance, and avoids issues with
unaligned accesses and DC ZVA instructions performed by the accelerated
memcpy/memset routines.

Instead of manipulating the memory attributes directly, use the
SetMemorySpaceAttributes() DXE services, which validates the attributes
against the capabilities of the region before making the actual change.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c      | 12 ++++--------
 ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf |  3 ++-
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
index a8125e81daac..3f3ceb3d2fa8 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
@@ -17,10 +17,10 @@
 #include <Library/IoLib.h>
 #include <Library/PcdLib.h>
 #include <Library/DebugLib.h>
+#include <Library/DxeServicesTableLib.h>
 #include <Library/LcdPlatformLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 
-#include <Protocol/Cpu.h>
 #include <Protocol/EdidDiscovered.h>
 #include <Protocol/EdidActive.h>
 
@@ -165,7 +165,6 @@ LcdPlatformGetVram (
   )
 {
   EFI_STATUS              Status;
-  EFI_CPU_ARCH_PROTOCOL  *Cpu;
 
   Status = EFI_SUCCESS;
 
@@ -187,12 +186,9 @@ LcdPlatformGetVram (
       return Status;
     }
 
-    // Ensure the Cpu architectural protocol is already installed
-    Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&Cpu);
-    ASSERT_EFI_ERROR(Status);
-
-    // Mark the VRAM as un-cachable. The VRAM is inside the DRAM, which is cachable.
-    Status = Cpu->SetMemoryAttributes(Cpu, *VramBaseAddress, *VramSize, EFI_MEMORY_UC);
+    // Mark the VRAM as write-combining. The VRAM is inside the DRAM, which is cacheable.
+    Status = gDS->SetMemorySpaceAttributes (*VramBaseAddress, *VramSize,
+                    EFI_MEMORY_WC);
     ASSERT_EFI_ERROR(Status);
     if (EFI_ERROR(Status)) {
       gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES(*VramSize));
diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf
index d1978e7110d5..658558ab1523 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf
@@ -31,8 +31,9 @@ [Packages]
   ArmPlatformPkg/ArmPlatformPkg.dec
 
 [LibraryClasses]
-  BaseLib
   ArmPlatformSysConfigLib
+  BaseLib
+  DxeServicesTableLib
 
 [Protocols]
   gEfiEdidDiscoveredProtocolGuid                # Produced
-- 
2.9.3

Re: [PATCH v2 0/5] ArmPlatformPkg: map VRAM using memory semantics

[Jeremy Linton]

Hi,

On 04/06/2017 08:15 AM, Ard Biesheuvel wrote:
> As reported by Jeremy, the recently introduced accelerated memcpy/memset
> routines are causing problems when used on framebuffer memory. While
> framebuffers are arguably right on the blurry line between MMIO (with
> side effects that are sensitive to ordering) and memory, mapping VRAM
> as device memory is unnecessary to begin with, and so we can improve
> our situation by using memory semantics for the VRAM mappings. (Whether
> we end up reverting to the unaccelerated memcpy/memset routines is a
> separate matter)
>
> So fix both the FVP case, which has a separate VRAM region, and TC2, which
> allocates VRAM from normal memory and changes the mapping attributes.
>
> Note to Ryan: this fixes FVP Base model for me, but not the Foundation model
> (whose PL111 support I never saw working to begin with, so I am not sure what
> is going on there)

So, I applied a similar set of patches against the juno, and everything 
continues to work. The change also looks good.

So,

Reviewed-by: Jeremy Linton <jeremy.linton@arm.com>


But I'm still concerned in general, since the HDLCD frame buffer is odd 
in that its just system memory rather than being attached to an IO bus 
of some form. AKA its not actually a "VRAM" (cue discussion about GDDR 
not having multiple ports/etc)...


>
> Ard Biesheuvel (5):
>   ArmPlatformPkg/FVP: map motherboard VRAM as uncached memory
>   ArmPlatformPkg/HdLcdArmVExpressLib: fix incorrect FreePool () call
>   ArmPlatformPkg/PL111LcdArmVExpressLib: fix incorrect FreePool () call
>   ArmPlatformPkg/HdLcdArmVExpressLib: use write-combine mapping for VRAM
>   ArmPlatformPkg/PL111LcdArmVExpressLib: use write-combine mapping for
>     VRAM
>
>  ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h                       | 10 ++++++----
>  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c                      |  8 +++++++-
>  ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c            | 14 +++++---------
>  ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf       |  3 ++-
>  ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c      | 14 +++++---------
>  ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf |  3 ++-
>  6 files changed, 27 insertions(+), 25 deletions(-)
>

Re: [PATCH v2 0/5] ArmPlatformPkg: map VRAM using memory semantics

[Ard Biesheuvel]

On 6 April 2017 at 17:29, Jeremy Linton <jeremy.linton@arm.com> wrote:
> Hi,
>
> On 04/06/2017 08:15 AM, Ard Biesheuvel wrote:
>>
>> As reported by Jeremy, the recently introduced accelerated memcpy/memset
>> routines are causing problems when used on framebuffer memory. While
>> framebuffers are arguably right on the blurry line between MMIO (with
>> side effects that are sensitive to ordering) and memory, mapping VRAM
>> as device memory is unnecessary to begin with, and so we can improve
>> our situation by using memory semantics for the VRAM mappings. (Whether
>> we end up reverting to the unaccelerated memcpy/memset routines is a
>> separate matter)
>>
>> So fix both the FVP case, which has a separate VRAM region, and TC2, which
>> allocates VRAM from normal memory and changes the mapping attributes.
>>
>> Note to Ryan: this fixes FVP Base model for me, but not the Foundation
>> model
>> (whose PL111 support I never saw working to begin with, so I am not sure
>> what
>> is going on there)
>
>
> So, I applied a similar set of patches against the juno, and everything
> continues to work. The change also looks good.
>
> So,
>
> Reviewed-by: Jeremy Linton <jeremy.linton@arm.com>
>

Thanks!

>
> But I'm still concerned in general, since the HDLCD frame buffer is odd in
> that its just system memory rather than being attached to an IO bus of some
> form. AKA its not actually a "VRAM" (cue discussion about GDDR not having
> multiple ports/etc)...
>

Re: [PATCH v2 0/5] ArmPlatformPkg: map VRAM using memory semantics

[Ryan Harkin]

On 6 April 2017 at 14:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> As reported by Jeremy, the recently introduced accelerated memcpy/memset
> routines are causing problems when used on framebuffer memory. While
> framebuffers are arguably right on the blurry line between MMIO (with
> side effects that are sensitive to ordering) and memory, mapping VRAM
> as device memory is unnecessary to begin with, and so we can improve
> our situation by using memory semantics for the VRAM mappings. (Whether
> we end up reverting to the unaccelerated memcpy/memset routines is a
> separate matter)
>
> So fix both the FVP case, which has a separate VRAM region, and TC2, which
> allocates VRAM from normal memory and changes the mapping attributes.
>
> Note to Ryan: this fixes FVP Base model for me, but not the Foundation model
> (whose PL111 support I never saw working to begin with, so I am not sure what
> is going on there)
>

With the fix to remove the duplicate VRAM_MOTHERBOARD_BASE, this
series is tested on Juno R0/1/2 to make sure they didn't break for
inexplicable reasons, on TC2 and FVP Base AEMv8 models.

FVP Foundation has never been verified with CLCD and it doesn't work
with or without this change, so I'm happy with that.

Tested-by: Ryan Harkin <ryan.harkin@linaro.org>


> Ard Biesheuvel (5):
>   ArmPlatformPkg/FVP: map motherboard VRAM as uncached memory
>   ArmPlatformPkg/HdLcdArmVExpressLib: fix incorrect FreePool () call
>   ArmPlatformPkg/PL111LcdArmVExpressLib: fix incorrect FreePool () call
>   ArmPlatformPkg/HdLcdArmVExpressLib: use write-combine mapping for VRAM
>   ArmPlatformPkg/PL111LcdArmVExpressLib: use write-combine mapping for
>     VRAM
>
>  ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h                       | 10 ++++++----
>  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c                      |  8 +++++++-
>  ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c            | 14 +++++---------
>  ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf       |  3 ++-
>  ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c      | 14 +++++---------
>  ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf |  3 ++-
>  6 files changed, 27 insertions(+), 25 deletions(-)
>
> --
> 2.9.3
>

Re: [PATCH v2 1/5] ArmPlatformPkg/FVP: map motherboard VRAM as uncached memory

[Leif Lindholm]

On Thu, Apr 06, 2017 at 02:15:47PM +0100, Ard Biesheuvel wrote:
> The VRAM of the PL111 on the FVP Base/Foundation models is described as
> device memory rather than uncached memory, which is not an accurate
> description of the nature of the region (i.e., a framebuffer), and may
> result in problems when using accelerated string routines to access the
> region, since this may legally involve unaligned accesses or DC ZVA
> instructions, which are not allowed on device mappings.
> 
> So split of the 8 MB VRAM region into a separate region, and map it using
> memory attributes.

"Normal memory attributes"?

> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h  | 10 ++++++----
>  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c |  8 +++++++-
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h b/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h
> index 06414e6e7208..4e17c800a34f 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h
> +++ b/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h
> @@ -40,9 +40,11 @@
>  #define ARM_VE_SMB_SRAM_BASE                    0x2E000000
>  #define ARM_VE_SMB_SRAM_SZ                      SIZE_64KB
>  // USB, Ethernet, VRAM
> -#define ARM_VE_SMB_PERIPH_BASE                  0x18000000
> -#define PL111_CLCD_VRAM_MOTHERBOARD_BASE        ARM_VE_SMB_PERIPH_BASE
> -#define ARM_VE_SMB_PERIPH_SZ                    SIZE_64MB
> +#define ARM_VE_SMB_PERIPH_BASE                  0x18800000
> +#define ARM_VE_SMB_PERIPH_SZ                    (SIZE_64MB - SIZE_8MB)
> +
> +#define PL111_CLCD_VRAM_MOTHERBOARD_BASE        0x18000000
> +#define PL111_CLCD_VRAM_MOTHERBOARD_SIZE        SIZE_8MB
>  
>  // DRAM
>  #define ARM_VE_DRAM_BASE                        PcdGet64 (PcdSystemMemoryBase)
> @@ -75,6 +77,6 @@
>  #define PL111_CLCD_CORE_TILE_VIDEO_MODE_OSC_ID  1
>  
>  // VRAM offset for the PL111 Colour LCD Controller on the motherboard
> -#define VRAM_MOTHERBOARD_BASE                     (ARM_VE_SMB_PERIPH_BASE   + 0x00000)
> +#define VRAM_MOTHERBOARD_BASE                     (PL111_CLCD_VRAM_MOTHERBOARD_BASE)
>  
>  #endif
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> index 14c7e8e1d672..70c17ae70478 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> @@ -21,7 +21,7 @@
>  #include <ArmPlatform.h>
>  
>  // Number of Virtual Memory Map Descriptors
> -#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          8
> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          9
>  
>  // DDR attributes
>  #define DDR_ATTRIBUTES_CACHED   ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
> @@ -130,6 +130,12 @@ ArmPlatformGetVirtualMemoryMap (
>    VirtualMemoryTable[Index].Length = 2 * ARM_VE_SMB_PERIPH_SZ;
>    VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>  
> +  // VRAM
> +  VirtualMemoryTable[++Index].PhysicalBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
> +  VirtualMemoryTable[Index].VirtualBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
> +  VirtualMemoryTable[Index].Length = PL111_CLCD_VRAM_MOTHERBOARD_SIZE;
> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;

Hmm, looking at this made me a bit confused though. Normal uncached
memory is certainly bufferable (that's basically what write-combining
means).

This looks like a naming hangover from ARMv5 translation table format.
Is it about time we clean this up?
Or have I sprained my brain?

/
    Leif

> +
>    // Map sparse memory region if present
>    if (HasSparseMemory) {
>      VirtualMemoryTable[++Index].PhysicalBase = SparseMemoryBase;
> -- 
> 2.9.3
> 

Re: [PATCH v2 0/5] ArmPlatformPkg: map VRAM using memory semantics

[Leif Lindholm]

On Thu, Apr 06, 2017 at 02:15:46PM +0100, Ard Biesheuvel wrote:
> As reported by Jeremy, the recently introduced accelerated memcpy/memset
> routines are causing problems when used on framebuffer memory. While
> framebuffers are arguably right on the blurry line between MMIO (with
> side effects that are sensitive to ordering) and memory, mapping VRAM
> as device memory is unnecessary to begin with, and so we can improve
> our situation by using memory semantics for the VRAM mappings. (Whether
> we end up reverting to the unaccelerated memcpy/memset routines is a
> separate matter)
> 
> So fix both the FVP case, which has a separate VRAM region, and TC2, which
> allocates VRAM from normal memory and changes the mapping attributes.

For 2-5/5:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> 
> Note to Ryan: this fixes FVP Base model for me, but not the Foundation model
> (whose PL111 support I never saw working to begin with, so I am not sure what
> is going on there)
> 
> Ard Biesheuvel (5):
>   ArmPlatformPkg/FVP: map motherboard VRAM as uncached memory
>   ArmPlatformPkg/HdLcdArmVExpressLib: fix incorrect FreePool () call
>   ArmPlatformPkg/PL111LcdArmVExpressLib: fix incorrect FreePool () call
>   ArmPlatformPkg/HdLcdArmVExpressLib: use write-combine mapping for VRAM
>   ArmPlatformPkg/PL111LcdArmVExpressLib: use write-combine mapping for
>     VRAM
> 
>  ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h                       | 10 ++++++----
>  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c                      |  8 +++++++-
>  ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c            | 14 +++++---------
>  ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf       |  3 ++-
>  ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c      | 14 +++++---------
>  ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf |  3 ++-
>  6 files changed, 27 insertions(+), 25 deletions(-)
> 
> -- 
> 2.9.3
> 

Re: [PATCH v2 1/5] ArmPlatformPkg/FVP: map motherboard VRAM as uncached memory

[Ard Biesheuvel]

On 6 April 2017 at 19:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Apr 06, 2017 at 02:15:47PM +0100, Ard Biesheuvel wrote:
>> The VRAM of the PL111 on the FVP Base/Foundation models is described as
>> device memory rather than uncached memory, which is not an accurate
>> description of the nature of the region (i.e., a framebuffer), and may
>> result in problems when using accelerated string routines to access the
>> region, since this may legally involve unaligned accesses or DC ZVA
>> instructions, which are not allowed on device mappings.
>>
>> So split of the 8 MB VRAM region into a separate region, and map it using
>> memory attributes.
>
> "Normal memory attributes"?
>

OK

>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h  | 10 ++++++----
>>  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c |  8 +++++++-
>>  2 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h b/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h
>> index 06414e6e7208..4e17c800a34f 100644
>> --- a/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h
>> +++ b/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h
>> @@ -40,9 +40,11 @@
>>  #define ARM_VE_SMB_SRAM_BASE                    0x2E000000
>>  #define ARM_VE_SMB_SRAM_SZ                      SIZE_64KB
>>  // USB, Ethernet, VRAM
>> -#define ARM_VE_SMB_PERIPH_BASE                  0x18000000
>> -#define PL111_CLCD_VRAM_MOTHERBOARD_BASE        ARM_VE_SMB_PERIPH_BASE
>> -#define ARM_VE_SMB_PERIPH_SZ                    SIZE_64MB
>> +#define ARM_VE_SMB_PERIPH_BASE                  0x18800000
>> +#define ARM_VE_SMB_PERIPH_SZ                    (SIZE_64MB - SIZE_8MB)
>> +
>> +#define PL111_CLCD_VRAM_MOTHERBOARD_BASE        0x18000000
>> +#define PL111_CLCD_VRAM_MOTHERBOARD_SIZE        SIZE_8MB
>>
>>  // DRAM
>>  #define ARM_VE_DRAM_BASE                        PcdGet64 (PcdSystemMemoryBase)
>> @@ -75,6 +77,6 @@
>>  #define PL111_CLCD_CORE_TILE_VIDEO_MODE_OSC_ID  1
>>
>>  // VRAM offset for the PL111 Colour LCD Controller on the motherboard
>> -#define VRAM_MOTHERBOARD_BASE                     (ARM_VE_SMB_PERIPH_BASE   + 0x00000)
>> +#define VRAM_MOTHERBOARD_BASE                     (PL111_CLCD_VRAM_MOTHERBOARD_BASE)
>>
>>  #endif
>> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
>> index 14c7e8e1d672..70c17ae70478 100644
>> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
>> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
>> @@ -21,7 +21,7 @@
>>  #include <ArmPlatform.h>
>>
>>  // Number of Virtual Memory Map Descriptors
>> -#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          8
>> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          9
>>
>>  // DDR attributes
>>  #define DDR_ATTRIBUTES_CACHED   ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
>> @@ -130,6 +130,12 @@ ArmPlatformGetVirtualMemoryMap (
>>    VirtualMemoryTable[Index].Length = 2 * ARM_VE_SMB_PERIPH_SZ;
>>    VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>
>> +  // VRAM
>> +  VirtualMemoryTable[++Index].PhysicalBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
>> +  VirtualMemoryTable[Index].VirtualBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
>> +  VirtualMemoryTable[Index].Length = PL111_CLCD_VRAM_MOTHERBOARD_SIZE;
>> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
>
> Hmm, looking at this made me a bit confused though. Normal uncached
> memory is certainly bufferable (that's basically what write-combining
> means).
>

It maps to MAIR attribute encoding 0x44, which translates as

Normal Memory, Outer Non-Cacheable, Inner Non-Cacheable

> This looks like a naming hangover from ARMv5 translation table format.
> Is it about time we clean this up?

The whole 'ARM_MEMORY_REGION_xxxx' intermediate namespace should be
removed, I think.

> Or have I sprained my brain?
>
> /
>     Leif
>
>> +
>>    // Map sparse memory region if present
>>    if (HasSparseMemory) {
>>      VirtualMemoryTable[++Index].PhysicalBase = SparseMemoryBase;
>> --
>> 2.9.3
>>

Re: [PATCH v2 1/5] ArmPlatformPkg/FVP: map motherboard VRAM as uncached memory

[Leif Lindholm]

On Thu, Apr 06, 2017 at 07:31:13PM +0100, Ard Biesheuvel wrote:
> On 6 April 2017 at 19:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Apr 06, 2017 at 02:15:47PM +0100, Ard Biesheuvel wrote:
> >> The VRAM of the PL111 on the FVP Base/Foundation models is described as
> >> device memory rather than uncached memory, which is not an accurate
> >> description of the nature of the region (i.e., a framebuffer), and may
> >> result in problems when using accelerated string routines to access the
> >> region, since this may legally involve unaligned accesses or DC ZVA
> >> instructions, which are not allowed on device mappings.
> >>
> >> So split of the 8 MB VRAM region into a separate region, and map it using
> >> memory attributes.
> >
> > "Normal memory attributes"?
> >
> 
> OK
> 
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h  | 10 ++++++----
> >>  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c |  8 +++++++-
> >>  2 files changed, 13 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h b/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h
> >> index 06414e6e7208..4e17c800a34f 100644
> >> --- a/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h
> >> +++ b/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h
> >> @@ -40,9 +40,11 @@
> >>  #define ARM_VE_SMB_SRAM_BASE                    0x2E000000
> >>  #define ARM_VE_SMB_SRAM_SZ                      SIZE_64KB
> >>  // USB, Ethernet, VRAM
> >> -#define ARM_VE_SMB_PERIPH_BASE                  0x18000000
> >> -#define PL111_CLCD_VRAM_MOTHERBOARD_BASE        ARM_VE_SMB_PERIPH_BASE
> >> -#define ARM_VE_SMB_PERIPH_SZ                    SIZE_64MB
> >> +#define ARM_VE_SMB_PERIPH_BASE                  0x18800000
> >> +#define ARM_VE_SMB_PERIPH_SZ                    (SIZE_64MB - SIZE_8MB)
> >> +
> >> +#define PL111_CLCD_VRAM_MOTHERBOARD_BASE        0x18000000
> >> +#define PL111_CLCD_VRAM_MOTHERBOARD_SIZE        SIZE_8MB
> >>
> >>  // DRAM
> >>  #define ARM_VE_DRAM_BASE                        PcdGet64 (PcdSystemMemoryBase)
> >> @@ -75,6 +77,6 @@
> >>  #define PL111_CLCD_CORE_TILE_VIDEO_MODE_OSC_ID  1
> >>
> >>  // VRAM offset for the PL111 Colour LCD Controller on the motherboard
> >> -#define VRAM_MOTHERBOARD_BASE                     (ARM_VE_SMB_PERIPH_BASE   + 0x00000)
> >> +#define VRAM_MOTHERBOARD_BASE                     (PL111_CLCD_VRAM_MOTHERBOARD_BASE)
> >>
> >>  #endif
> >> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> >> index 14c7e8e1d672..70c17ae70478 100644
> >> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> >> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> >> @@ -21,7 +21,7 @@
> >>  #include <ArmPlatform.h>
> >>
> >>  // Number of Virtual Memory Map Descriptors
> >> -#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          8
> >> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          9
> >>
> >>  // DDR attributes
> >>  #define DDR_ATTRIBUTES_CACHED   ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
> >> @@ -130,6 +130,12 @@ ArmPlatformGetVirtualMemoryMap (
> >>    VirtualMemoryTable[Index].Length = 2 * ARM_VE_SMB_PERIPH_SZ;
> >>    VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >>
> >> +  // VRAM
> >> +  VirtualMemoryTable[++Index].PhysicalBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
> >> +  VirtualMemoryTable[Index].VirtualBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
> >> +  VirtualMemoryTable[Index].Length = PL111_CLCD_VRAM_MOTHERBOARD_SIZE;
> >> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> >
> > Hmm, looking at this made me a bit confused though. Normal uncached
> > memory is certainly bufferable (that's basically what write-combining
> > means).
> >
> 
> It maps to MAIR attribute encoding 0x44, which translates as
> 
> Normal Memory, Outer Non-Cacheable, Inner Non-Cacheable

Exactly - which is definitely "buffered".

> > This looks like a naming hangover from ARMv5 translation table format.
> > Is it about time we clean this up?
> 
> The whole 'ARM_MEMORY_REGION_xxxx' intermediate namespace should be
> removed, I think.

That sounds like a good idea to me.
There's also _NONSECURE crud in there.

/
    Leif

> > Or have I sprained my brain?
> >
> > /
> >     Leif
> >
> >> +
> >>    // Map sparse memory region if present
> >>    if (HasSparseMemory) {
> >>      VirtualMemoryTable[++Index].PhysicalBase = SparseMemoryBase;
> >> --
> >> 2.9.3
> >>

Re: [PATCH v2 1/5] ArmPlatformPkg/FVP: map motherboard VRAM as uncached memory

[Ard Biesheuvel]

On 6 April 2017 at 19:45, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Apr 06, 2017 at 07:31:13PM +0100, Ard Biesheuvel wrote:
>> On 6 April 2017 at 19:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > On Thu, Apr 06, 2017 at 02:15:47PM +0100, Ard Biesheuvel wrote:
>> >> The VRAM of the PL111 on the FVP Base/Foundation models is described as
>> >> device memory rather than uncached memory, which is not an accurate
>> >> description of the nature of the region (i.e., a framebuffer), and may
>> >> result in problems when using accelerated string routines to access the
>> >> region, since this may legally involve unaligned accesses or DC ZVA
>> >> instructions, which are not allowed on device mappings.
>> >>
>> >> So split of the 8 MB VRAM region into a separate region, and map it using
>> >> memory attributes.
>> >
>> > "Normal memory attributes"?
>> >
>>
>> OK
>>
>> >>
>> >> Contributed-under: TianoCore Contribution Agreement 1.0
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> ---
>> >>  ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h  | 10 ++++++----
>> >>  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c |  8 +++++++-
>> >>  2 files changed, 13 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h b/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h
>> >> index 06414e6e7208..4e17c800a34f 100644
>> >> --- a/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h
>> >> +++ b/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM/ArmPlatform.h
>> >> @@ -40,9 +40,11 @@
>> >>  #define ARM_VE_SMB_SRAM_BASE                    0x2E000000
>> >>  #define ARM_VE_SMB_SRAM_SZ                      SIZE_64KB
>> >>  // USB, Ethernet, VRAM
>> >> -#define ARM_VE_SMB_PERIPH_BASE                  0x18000000
>> >> -#define PL111_CLCD_VRAM_MOTHERBOARD_BASE        ARM_VE_SMB_PERIPH_BASE
>> >> -#define ARM_VE_SMB_PERIPH_SZ                    SIZE_64MB
>> >> +#define ARM_VE_SMB_PERIPH_BASE                  0x18800000
>> >> +#define ARM_VE_SMB_PERIPH_SZ                    (SIZE_64MB - SIZE_8MB)
>> >> +
>> >> +#define PL111_CLCD_VRAM_MOTHERBOARD_BASE        0x18000000
>> >> +#define PL111_CLCD_VRAM_MOTHERBOARD_SIZE        SIZE_8MB
>> >>
>> >>  // DRAM
>> >>  #define ARM_VE_DRAM_BASE                        PcdGet64 (PcdSystemMemoryBase)
>> >> @@ -75,6 +77,6 @@
>> >>  #define PL111_CLCD_CORE_TILE_VIDEO_MODE_OSC_ID  1
>> >>
>> >>  // VRAM offset for the PL111 Colour LCD Controller on the motherboard
>> >> -#define VRAM_MOTHERBOARD_BASE                     (ARM_VE_SMB_PERIPH_BASE   + 0x00000)
>> >> +#define VRAM_MOTHERBOARD_BASE                     (PL111_CLCD_VRAM_MOTHERBOARD_BASE)
>> >>
>> >>  #endif
>> >> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
>> >> index 14c7e8e1d672..70c17ae70478 100644
>> >> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
>> >> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
>> >> @@ -21,7 +21,7 @@
>> >>  #include <ArmPlatform.h>
>> >>
>> >>  // Number of Virtual Memory Map Descriptors
>> >> -#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          8
>> >> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          9
>> >>
>> >>  // DDR attributes
>> >>  #define DDR_ATTRIBUTES_CACHED   ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
>> >> @@ -130,6 +130,12 @@ ArmPlatformGetVirtualMemoryMap (
>> >>    VirtualMemoryTable[Index].Length = 2 * ARM_VE_SMB_PERIPH_SZ;
>> >>    VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>> >>
>> >> +  // VRAM
>> >> +  VirtualMemoryTable[++Index].PhysicalBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
>> >> +  VirtualMemoryTable[Index].VirtualBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
>> >> +  VirtualMemoryTable[Index].Length = PL111_CLCD_VRAM_MOTHERBOARD_SIZE;
>> >> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
>> >
>> > Hmm, looking at this made me a bit confused though. Normal uncached
>> > memory is certainly bufferable (that's basically what write-combining
>> > means).
>> >
>>
>> It maps to MAIR attribute encoding 0x44, which translates as
>>
>> Normal Memory, Outer Non-Cacheable, Inner Non-Cacheable
>
> Exactly - which is definitely "buffered".
>
>> > This looks like a naming hangover from ARMv5 translation table format.
>> > Is it about time we clean this up?
>>
>> The whole 'ARM_MEMORY_REGION_xxxx' intermediate namespace should be
>> removed, I think.
>
> That sounds like a good idea to me.
> There's also _NONSECURE crud in there.
>

Yes. But I hope you're not saying you want that to be done first
before this patch can go in?

Re: [PATCH v2 1/5] ArmPlatformPkg/FVP: map motherboard VRAM as uncached memory

[Leif Lindholm]

On Thu, Apr 06, 2017 at 07:46:50PM +0100, Ard Biesheuvel wrote:
> >> >> +  // VRAM
> >> >> +  VirtualMemoryTable[++Index].PhysicalBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
> >> >> +  VirtualMemoryTable[Index].VirtualBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
> >> >> +  VirtualMemoryTable[Index].Length = PL111_CLCD_VRAM_MOTHERBOARD_SIZE;
> >> >> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> >> >
> >> > Hmm, looking at this made me a bit confused though. Normal uncached
> >> > memory is certainly bufferable (that's basically what write-combining
> >> > means).
> >> >
> >>
> >> It maps to MAIR attribute encoding 0x44, which translates as
> >>
> >> Normal Memory, Outer Non-Cacheable, Inner Non-Cacheable
> >
> > Exactly - which is definitely "buffered".
> >
> >> > This looks like a naming hangover from ARMv5 translation table format.
> >> > Is it about time we clean this up?
> >>
> >> The whole 'ARM_MEMORY_REGION_xxxx' intermediate namespace should be
> >> removed, I think.
> >
> > That sounds like a good idea to me.
> > There's also _NONSECURE crud in there.
> >
> 
> Yes. But I hope you're not saying you want that to be done first
> before this patch can go in?

No, but it may mean it makes sense to add a comment regarding the
Attributes line, since it looks like it's doing the opposite of what
is actually being done.

/
    Leif

Re: [PATCH v2 1/5] ArmPlatformPkg/FVP: map motherboard VRAM as uncached memory

[Ard Biesheuvel]

On 6 April 2017 at 20:06, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Apr 06, 2017 at 07:46:50PM +0100, Ard Biesheuvel wrote:
>> >> >> +  // VRAM
>> >> >> +  VirtualMemoryTable[++Index].PhysicalBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
>> >> >> +  VirtualMemoryTable[Index].VirtualBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
>> >> >> +  VirtualMemoryTable[Index].Length = PL111_CLCD_VRAM_MOTHERBOARD_SIZE;
>> >> >> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
>> >> >
>> >> > Hmm, looking at this made me a bit confused though. Normal uncached
>> >> > memory is certainly bufferable (that's basically what write-combining
>> >> > means).
>> >> >
>> >>
>> >> It maps to MAIR attribute encoding 0x44, which translates as
>> >>
>> >> Normal Memory, Outer Non-Cacheable, Inner Non-Cacheable
>> >
>> > Exactly - which is definitely "buffered".
>> >
>> >> > This looks like a naming hangover from ARMv5 translation table format.
>> >> > Is it about time we clean this up?
>> >>
>> >> The whole 'ARM_MEMORY_REGION_xxxx' intermediate namespace should be
>> >> removed, I think.
>> >
>> > That sounds like a good idea to me.
>> > There's also _NONSECURE crud in there.
>> >
>>
>> Yes. But I hope you're not saying you want that to be done first
>> before this patch can go in?
>
> No, but it may mean it makes sense to add a comment regarding the
> Attributes line, since it looks like it's doing the opposite of what
> is actually being done.
>

Something like

--- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
@@ -134,6 +134,12 @@ ArmPlatformGetVirtualMemoryMap (
   VirtualMemoryTable[++Index].PhysicalBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
   VirtualMemoryTable[Index].VirtualBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
   VirtualMemoryTable[Index].Length = PL111_CLCD_VRAM_MOTHERBOARD_SIZE;
+  //
+  // Map the VRAM region as Normal Non-Cacheable memory and not device memory,
+  // so that we can use the accelerated string routines that may use unaligned
+  // accesses or DC ZVA instructions. The enum identifier is slightly awkward
+  // here, but it maps to a memory type that allows buffering and reordering.
+  //
   VirtualMemoryTable[Index].Attributes =
ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;

   // Map sparse memory region if present

perhaps?

Re: [PATCH v2 1/5] ArmPlatformPkg/FVP: map motherboard VRAM as uncached memory

[Leif Lindholm]

On Thu, Apr 06, 2017 at 09:01:57PM +0100, Ard Biesheuvel wrote:
> On 6 April 2017 at 20:06, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Apr 06, 2017 at 07:46:50PM +0100, Ard Biesheuvel wrote:
> >> >> >> +  // VRAM
> >> >> >> +  VirtualMemoryTable[++Index].PhysicalBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
> >> >> >> +  VirtualMemoryTable[Index].VirtualBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
> >> >> >> +  VirtualMemoryTable[Index].Length = PL111_CLCD_VRAM_MOTHERBOARD_SIZE;
> >> >> >> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> >> >> >
> >> >> > Hmm, looking at this made me a bit confused though. Normal uncached
> >> >> > memory is certainly bufferable (that's basically what write-combining
> >> >> > means).
> >> >> >
> >> >>
> >> >> It maps to MAIR attribute encoding 0x44, which translates as
> >> >>
> >> >> Normal Memory, Outer Non-Cacheable, Inner Non-Cacheable
> >> >
> >> > Exactly - which is definitely "buffered".
> >> >
> >> >> > This looks like a naming hangover from ARMv5 translation table format.
> >> >> > Is it about time we clean this up?
> >> >>
> >> >> The whole 'ARM_MEMORY_REGION_xxxx' intermediate namespace should be
> >> >> removed, I think.
> >> >
> >> > That sounds like a good idea to me.
> >> > There's also _NONSECURE crud in there.
> >> >
> >>
> >> Yes. But I hope you're not saying you want that to be done first
> >> before this patch can go in?
> >
> > No, but it may mean it makes sense to add a comment regarding the
> > Attributes line, since it looks like it's doing the opposite of what
> > is actually being done.
> >
> 
> Something like
> 
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> @@ -134,6 +134,12 @@ ArmPlatformGetVirtualMemoryMap (
>    VirtualMemoryTable[++Index].PhysicalBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
>    VirtualMemoryTable[Index].VirtualBase = PL111_CLCD_VRAM_MOTHERBOARD_BASE;
>    VirtualMemoryTable[Index].Length = PL111_CLCD_VRAM_MOTHERBOARD_SIZE;
> +  //
> +  // Map the VRAM region as Normal Non-Cacheable memory and not device memory,
> +  // so that we can use the accelerated string routines that may use unaligned
> +  // accesses or DC ZVA instructions. The enum identifier is slightly awkward
> +  // here, but it maps to a memory type that allows buffering and reordering.
> +  //
>    VirtualMemoryTable[Index].Attributes =
> ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> 
>    // Map sparse memory region if present
> 
> perhaps?

Yeah, that's spot on.
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Re: [PATCH v2 0/5] ArmPlatformPkg: map VRAM using memory semantics

[Ard Biesheuvel]

On 6 April 2017 at 19:29, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Apr 06, 2017 at 02:15:46PM +0100, Ard Biesheuvel wrote:
>> As reported by Jeremy, the recently introduced accelerated memcpy/memset
>> routines are causing problems when used on framebuffer memory. While
>> framebuffers are arguably right on the blurry line between MMIO (with
>> side effects that are sensitive to ordering) and memory, mapping VRAM
>> as device memory is unnecessary to begin with, and so we can improve
>> our situation by using memory semantics for the VRAM mappings. (Whether
>> we end up reverting to the unaccelerated memcpy/memset routines is a
>> separate matter)
>>
>> So fix both the FVP case, which has a separate VRAM region, and TC2, which
>> allocates VRAM from normal memory and changes the mapping attributes.
>
> For 2-5/5:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Thanks all. Pushed.