Re: [edk2-platforms][PATCH 1/5] Platform/RPi: Fix overlap of SoC registers and RAM

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 12/11/19 12:25 PM, Pete Batard wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> Having RAM and SoC register regions overlap is problematic for MMIO,
> since, at the very least, we don't want these regions to be declared
> as cacheable.
> 
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
>   Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c | 36 +++++++++++++-------
>   1 file changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
> index cc761bea1307..781cf78b83d3 100644
> --- a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
> +++ b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
> @@ -60,7 +60,7 @@ ArmPlatformGetVirtualMemoryMap (
>   {
>     UINTN                         Index = 0;
>     UINTN                         GpuIndex;
> -  INT64                         ExtendedMemorySize;
> +  INT64                         SystemMemorySize;
>     ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
>   
>     // Early output of the info we got from VideoCore can prove valuable.
> @@ -120,21 +120,21 @@ ArmPlatformGetVirtualMemoryMap (
>     VirtualMemoryInfo[Index].Type             = RPI_MEM_RESERVED_REGION;
>     VirtualMemoryInfo[Index++].Name           = L"GPU Reserved";
>   
> -  // Compute the amount of extended RAM available on this platform
> -  ExtendedMemorySize = SIZE_256MB;
> -  ExtendedMemorySize <<= (mBoardRevision >> 20) & 0x07;
> -  ExtendedMemorySize -= SIZE_1GB;
> -  if (ExtendedMemorySize > 0) {
> -    VirtualMemoryTable[Index].PhysicalBase  = FixedPcdGet64 (PcdExtendedMemoryBase);
> -    VirtualMemoryTable[Index].VirtualBase   = VirtualMemoryTable[Index].PhysicalBase;
> -    VirtualMemoryTable[Index].Length        = ExtendedMemorySize;
> -    VirtualMemoryTable[Index].Attributes    = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> -    VirtualMemoryInfo[Index].Type           = RPI_MEM_BASIC_REGION;
> -    VirtualMemoryInfo[Index++].Name         = L"Extended System RAM";
> -  }
> +  // Compute the total RAM size available on this platform
> +  SystemMemorySize = SIZE_256MB;
> +  SystemMemorySize <<= (mBoardRevision >> 20) & 0x07;

TIL I learn this field is 3 bits (I thought it was 4).

> +
> +  //
> +  // Ensure that what we declare as System Memory doesn't overlap with the
> +  // Bcm2836 SoC registers. This can be achieved through a MIN () with the
> +  // base address since SystemMemoryBase is 0 (we assert if it isn't).

Is the later comment "// On the Pi 3 the SoC registers may overlap 
VideoCore => fix this
" still accurate?

> +  //
> +  SystemMemorySize = MIN(SystemMemorySize, BCM2836_SOC_REGISTERS);
>   
>     // Extended SoC registers (PCIe, genet, ...)
>     if (BCM2711_SOC_REGISTERS > 0) {
> +    // Same overlap protection as above for the Bcm2711 SoC registers
> +    SystemMemorySize                        = MIN(SystemMemorySize, BCM2711_SOC_REGISTERS);
>       VirtualMemoryTable[Index].PhysicalBase  = BCM2711_SOC_REGISTERS;
>       VirtualMemoryTable[Index].VirtualBase   = VirtualMemoryTable[Index].PhysicalBase;
>       VirtualMemoryTable[Index].Length        = BCM2711_SOC_REGISTER_LENGTH;
> @@ -155,6 +155,16 @@ ArmPlatformGetVirtualMemoryMap (
>     VirtualMemoryInfo[Index].Type             = RPI_MEM_RESERVED_REGION;
>     VirtualMemoryInfo[Index++].Name           = L"SoC Reserved (283x)";

Sigh, the 2711/2838 naming is very confusing.

>   
> +  // If we have RAM above the 1 GB mark, declare it
> +  if (SystemMemorySize - SIZE_1GB > 0) {
> +    VirtualMemoryTable[Index].PhysicalBase  = FixedPcdGet64 (PcdExtendedMemoryBase);
> +    VirtualMemoryTable[Index].VirtualBase   = VirtualMemoryTable[Index].PhysicalBase;
> +    VirtualMemoryTable[Index].Length        = SystemMemorySize - SIZE_1GB;
> +    VirtualMemoryTable[Index].Attributes    = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +    VirtualMemoryInfo[Index].Type           = RPI_MEM_BASIC_REGION;
> +    VirtualMemoryInfo[Index++].Name         = L"Extended System RAM";
> +  }
> +
>     // End of Table
>     VirtualMemoryTable[Index].PhysicalBase    = 0;
>     VirtualMemoryTable[Index].VirtualBase     = 0;
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>