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

[Leif Lindholm <leif.lindholm@linaro.org>]

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>