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

["Pete Batard" <pete@akeo.ie>]

Hi Philippe, thanks for reviewing these.

On 2019.12.11 12:21, Philippe Mathieu-Daudé wrote:
> 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).

"32 GB should be enough for everybody" (which is the max RAM you'll be 
able to declare in 3 bits with the current scheme).

Unless RAM becomes very very cheap, I doubt the Raspberry Pi foundation 
considers the current limit as much of a problem...

> 
>> +
>> +  //
>> +  // 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?

It is. We read the size of the GPU region (mVideoCoreSize) and the size 
of the System RAM from different queries. The thing is actually that 
VideoCore on the Pi 3 considers that its region should encompass the SoC 
registers, so it reports a size that does so, whereas we want to report 
them as separate regions (like they are reported on the Pi 4).

Regards,

/Pete

>> +  //
>> +  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>
>