On Fri, Nov 20, 2015 at 12:20:56PM +0100, Ard Biesheuvel wrote:
> On 20 November 2015 at 10:29, Heyi Guo <[email protected]> wrote:
> > Hi Ard,
> >
> > Do we have special reason of not cleaning the allocated pages? If not
> > having, I think it is safer to use "clean and invalidate".
>
> Actually, I disagree. If your firmware relies on data making it to
> main memory only due to a subsequent clean performed by the next owner
> of the memory range, I'd much rather make it explicit.
> And clean + invalidate is obviously more costly.
>
> Leif?
Completely agree (with Ard).
Such a contract would have to be codified into the UEFI
specification to begin with (and I would argue against it if anyone
proposed to do that).
/
Leif
> > On 11/20/2015 02:25 PM, Ard Biesheuvel wrote:
> >>
> >> On 20 November 2015 at 06:32, Heyi Guo <[email protected]> wrote:
> >>>
> >>> It is implied that the memory returned from UncachedMemoryAllocationLib
> >>> should have cache cleaned. So we clean and invalidate memory range after
> >>> changing memory attribute to uncached.
> >>>
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Heyi Guo <[email protected]>
> >>> Cc: Leif Lindholm <[email protected]>
> >>> Cc: Ard Biesheuvel <[email protected]>
> >>> ---
> >>> .../Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> >>> | 3 +++
> >>> .../UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf
> >>> | 1 +
> >>> 2 files changed, 4 insertions(+)
> >>>
> >>> diff --git
> >>> a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> >>> b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> >>> index b859f63..3242579 100644
> >>> ---
> >>> a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> >>> +++
> >>> b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> >>> @@ -25,6 +25,7 @@
> >>> #include <Library/PcdLib.h>
> >>> #include <Library/ArmLib.h>
> >>> #include <Library/DxeServicesTableLib.h>
> >>> +#include <Library/CacheMaintenanceLib.h>
> >>>
> >>> VOID *
> >>> UncachedInternalAllocatePages (
> >>> @@ -165,6 +166,8 @@ AllocatePagesFromList (
> >>> return Status;
> >>> }
> >>>
> >>> + WriteBackInvalidateDataCacheRange ((VOID *)(UINTN)Memory,
> >>> EFI_PAGES_TO_SIZE (Pages));
> >>> +
> >>
> >> I think invalidate only should be fine here, since it is a new
> >> allocation, and we are allocating full pages. Since the architectural
> >> max value of CWG is 2 KB, we could never end up discarding data that
> >> is not covered by the allocation itself.
> >>
> >>> NewNode = AllocatePool (sizeof (FREE_PAGE_NODE));
> >>> if (NewNode == NULL) {
> >>> ASSERT (FALSE);
> >>> diff --git
> >>> a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf
> >>> b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf
> >>> index 0a0b6cb..d7a0f2f 100644
> >>> ---
> >>> a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf
> >>> +++
> >>> b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf
> >>> @@ -38,6 +38,7 @@
> >>> MemoryAllocationLib
> >>> PcdLib
> >>> DxeServicesTableLib
> >>> + CacheMaintenanceLib
> >>>
> >>> [Pcd]
> >>> gArmTokenSpaceGuid.PcdArmFreeUncachedMemorySizeThreshold
> >>> --
> >>> 2.6.2
> >>>
> >
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
