Re: [PATCH 09/15] ArmVirtPkg/ArmVirtPlatformLib: remove support for uncached mappings

[Laszlo Ersek <lersek@redhat.com>]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> QEMU/KVM has very little tolerance for using anything except writeback
> cacheable mappings of DRAM, so let's remove the 'feature' that allows
> us to select uncached mappings at build time.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
> index d10548f86dfc..4368d05f76ef 100644
> --- a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
> +++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
> @@ -22,10 +22,6 @@
>  // Number of Virtual Memory Map Descriptors
>  #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          5
>  
> -// DDR attributes
> -#define DDR_ATTRIBUTES_CACHED    ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
> -#define DDR_ATTRIBUTES_UNCACHED  ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED
> -
>  EFI_PHYSICAL_ADDRESS
>  ArmGetPhysAddrTop (
>    VOID
> @@ -48,7 +44,6 @@ ArmPlatformGetVirtualMemoryMap (
>    IN ARM_MEMORY_REGION_DESCRIPTOR** VirtualMemoryMap
>    )
>  {
> -  ARM_MEMORY_REGION_ATTRIBUTES  CacheAttributes;
>    ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
>  
>    ASSERT (VirtualMemoryMap != NULL);
> @@ -65,17 +60,11 @@ ArmPlatformGetVirtualMemoryMap (
>      return;
>    }
>  
> -  if (FeaturePcdGet (PcdCacheEnable) == TRUE) {
> -    CacheAttributes = DDR_ATTRIBUTES_CACHED;
> -  } else {
> -    CacheAttributes = DDR_ATTRIBUTES_UNCACHED;
> -  }
> -
>    // System DRAM
>    VirtualMemoryTable[0].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
>    VirtualMemoryTable[0].VirtualBase  = VirtualMemoryTable[0].PhysicalBase;
>    VirtualMemoryTable[0].Length       = PcdGet64 (PcdSystemMemorySize);
> -  VirtualMemoryTable[0].Attributes   = CacheAttributes;
> +  VirtualMemoryTable[0].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>  
>    DEBUG ((EFI_D_INFO, "%a: Dumping System DRAM Memory Map:\n"
>        "\tPhysicalBase: 0x%lX\n"
> @@ -104,7 +93,7 @@ ArmPlatformGetVirtualMemoryMap (
>    VirtualMemoryTable[3].PhysicalBase = FixedPcdGet64 (PcdFdBaseAddress);
>    VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;
>    VirtualMemoryTable[3].Length       = FixedPcdGet32 (PcdFdSize);
> -  VirtualMemoryTable[3].Attributes   = CacheAttributes;
> +  VirtualMemoryTable[3].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>  
>    // End of Table
>    ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
> 

Looks OK.

I see that PcdCacheEnable is removed from -- or, well, "with" -- all the
affected INF files in the last patch. I also see that the EmbeddedPkg
default for the PCD is FALSE. So it likely makes sense to keep our TRUE
default in "ArmVirt.dsc.inc", for any other (external) modules that
depend on the PCD.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>