On Thu, Apr 06, 2017 at 09:01:57PM +0100, Ard Biesheuvel wrote: > On 6 April 2017 at 20:06, Leif Lindholm <leif.lindh...@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.lindh...@linaro.org> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
