diff mbox series

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

Message ID 20170406131551.3322-2-ard.biesheuvel@linaro.org
State New
Headers show
Series ArmPlatformPkg: map VRAM using memory semantics | expand

Commit Message

Ard Biesheuvel April 6, 2017, 1:15 p.m. UTC 
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(-)

-- 
2.9.3

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm April 6, 2017, 6:26 p.m. UTC | #1 
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

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 6, 2017, 6:31 p.m. UTC | #2 
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

>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm April 6, 2017, 6:45 p.m. UTC | #3 
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

> >>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 6, 2017, 6:46 p.m. UTC | #4 
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?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm April 6, 2017, 7:06 p.m. UTC | #5 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 6, 2017, 8:01 p.m. UTC | #6 
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

   // Map sparse memory region if present

perhaps?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel--- 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;
Leif Lindholm April 6, 2017, 8:09 p.m. UTC | #7 
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>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

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;