From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by mx.groups.io with SMTP id smtpd.web10.1821.1591120345620930430 for ; Tue, 02 Jun 2020 10:52:26 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=Jaswsgky; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.221.68, mailfrom: pete@akeo.ie) Received: by mail-wr1-f68.google.com with SMTP id t18so4297543wru.6 for ; Tue, 02 Jun 2020 10:52:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=S4IrluOegRozTuGhWin63G3ttD+Sx5CyDQ4NGJlUxVo=; b=Jaswsgky7pwFa+YB1KvCDJkEl4dae1Rw0gmVFcAjCAwPp867yzEHVZF2eWWRT9iqyF TpsjACessSJ8k06oycFol+D50mcPxsDS8vQTj8N1nL0Ke2Y+rkAiYPCe4MjP55uDzKp2 8XIc/IU4TVWLu7PduuSQgNe5AOhWAYN4vkjDj8QNSQMm14rbHKxcR5gn1C8R7IZP1t3h trMTJIbyRY4kOeNZw00g+El4aYuRvxzVVH3YEn1PzDneukPLyatCq3mv3AXgrdydB3Ls 8XE9x28IAB/waoQe2J/1gf449026Wv7NvVneR+ySwlnE80ET23a7ZmSrfCp+G1RNQSc+ 03lg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=S4IrluOegRozTuGhWin63G3ttD+Sx5CyDQ4NGJlUxVo=; b=KY4p20C9/m69sUv3ADiBp7XK8KfJJbHzx7wY5Auyk4ZTyAcNuTZzsHqXIM3v0fbzBb 8CW/JeeDBfzphhrasN3/6ki4kvJ9F0uiLDVyfea3el+bRF7TdbMmMAHFolpWoH/VWEga ofCvWJVXcJc1c8dS5xjVEK4ELQVeycFelZrbwkYF9Qoi78lRXWWw3oADOTglDU0hXzmT faqINBmo4aDZid79OytUOtdVFA1fuW/6DNEg/ydoIVSr1dfUXIY+qkBfLt2VeXP6Z/9F RlaUUlUIhVtSLfCyUzTaO8i/DWyvL2qLbmFgBxy1OrNEYayPAK7HD3NZKidudjIiUfpH HJig== X-Gm-Message-State: AOAM531BF7dtrxrkv1T8h9PJh4fBLyilEoCnf6S4PSkSTUqZw8zA23zI bzWO0rVd/AL7ZHwyoeGUee4zew== X-Google-Smtp-Source: ABdhPJwHXc+RkQoMhFMXxNJq9fIDW2Kvw7RAppwqUymDLoESK38J1T/hsJ09bXQXctb2fEKPeif9pw== X-Received: by 2002:a5d:5585:: with SMTP id i5mr26851154wrv.112.1591120343976; Tue, 02 Jun 2020 10:52:23 -0700 (PDT) Return-Path: Received: from [10.0.0.122] ([84.203.48.247]) by smtp.googlemail.com with ESMTPSA id z22sm644480wmf.9.2020.06.02.10.52.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 02 Jun 2020 10:52:23 -0700 (PDT) Subject: Re: [edk2-platforms][PATCH 1/1] RPi4: reserve/map memory above 4GB when present To: Andrei Warkentin , devel@edk2.groups.io Cc: ard.biesheuvel@arm.com, leif@nuviainc.com, philmd@redhat.com References: <20200601091020.108704-1-andrey.warkentin@gmail.com> From: "Pete Batard" Message-ID: <391575f7-f6c8-fd25-0c8b-539d1e86d262@akeo.ie> Date: Tue, 2 Jun 2020 18:52:20 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 MIME-Version: 1.0 In-Reply-To: <20200601091020.108704-1-andrey.warkentin@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit A few minor notes inline: On 2020.06.01 10:10, Andrei Warkentin wrote: > This makes all 8GB usable on the Pi 4 8GB variant. > > Like RAM in the 3-4GB range, this is still gated by the > option to limit RAM to 3GB. > > Tested on 4GB and 8GB boards, with and without 3GB limit. > > Signed-off-by: Andrei Warkentin > --- > Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 32 +++++-- > Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf | 1 - > Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c | 99 ++++++++++++-------- > Platform/RaspberryPi/RPi4/RPi4.dsc | 1 - > Platform/RaspberryPi/RaspberryPi.dec | 1 - > 5 files changed, 85 insertions(+), 49 deletions(-) > > diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c > index ad14eb3d..5f8e7109 100644 > --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c > +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c > @@ -339,7 +339,6 @@ ApplyVariables ( > UINT32 CpuClock = PcdGet32 (PcdCpuClock); > UINT32 CustomCpuClock = PcdGet32 (PcdCustomCpuClock); > UINT32 Rate = 0; > - UINT64 SystemMemorySize; > > switch (CpuClock) { > case CHIPSET_CPU_CLOCK_LOW: > @@ -382,27 +381,42 @@ ApplyVariables ( > > if (mModelFamily >= 4 && PcdGet32 (PcdRamMoreThan3GB) != 0 && > PcdGet32 (PcdRamLimitTo3GB) == 0) { > + UINT64 SystemMemorySize; > + UINT64 SystemMemorySizeBelow4GB; Might be worth explaining that we want to handle < 4 GB memory in a special way, and thus introduce this special variable, on account of performing the 32-bit SoC register overlap. > /* > * Similar to how we compute the > 3 GB RAM segment's size in PlatformLib/ > * RaspberryPiMem.c, with some overlap protection for the Bcm2xxx register > - * spaces. This computation should also work for models with more than 4 GB > - * RAM, if there ever exist ones. > + * spaces. > */ > SystemMemorySize = (UINT64)mModelInstalledMB * SIZE_1MB; > - ASSERT (SystemMemorySize > 3UL * SIZE_1GB); > - SystemMemorySize = MIN(SystemMemorySize, BCM2836_SOC_REGISTERS); > + > + SystemMemorySizeBelow4GB = MIN(SystemMemorySize, 4UL * SIZE_1GB); > + SystemMemorySizeBelow4GB = MIN(SystemMemorySizeBelow4GB, BCM2836_SOC_REGISTERS); > if (BCM2711_SOC_REGISTERS > 0) { > - SystemMemorySize = MIN(SystemMemorySize, BCM2711_SOC_REGISTERS); > + SystemMemorySizeBelow4GB = MIN(SystemMemorySizeBelow4GB, BCM2711_SOC_REGISTERS); > } > + ASSERT (SystemMemorySizeBelow4GB - 3UL * SIZE_1GB > 0); ASSERTs work better for me, and possibily others reading the code, when, if you are comparing A to B, you're no using "A - B > 0" but "A > B". In other words, I think (SystemMemorySizeBelow4GB > 3UL * SIZE_1GB) would be clearer, which is also how the ASSERT was originally. > Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, 3UL * BASE_1GB, > - SystemMemorySize - (3UL * SIZE_1GB), > + SystemMemorySizeBelow4GB - (3UL * SIZE_1GB), > EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB); > ASSERT_EFI_ERROR (Status); > Status = gDS->SetMemorySpaceAttributes (3UL * BASE_1GB, > - SystemMemorySize - (3UL * SIZE_1GB), > - EFI_MEMORY_WB); > + SystemMemorySizeBelow4GB - (3UL * SIZE_1GB), EFI_MEMORY_WB); > ASSERT_EFI_ERROR (Status); > + > + if (SystemMemorySize - 4UL * SIZE_1GB > 0) { > + // > + // Register any memory above 4GB. > + // > + Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, 4UL * BASE_1GB, > + SystemMemorySize - (4UL * SIZE_1GB), > + EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB); > + ASSERT_EFI_ERROR (Status); > + Status = gDS->SetMemorySpaceAttributes (4UL * BASE_1GB, > + SystemMemorySize - (4UL * SIZE_1GB), EFI_MEMORY_WB); > + ASSERT_EFI_ERROR (Status); > + } > } > > if (mModelFamily == 3 || mModelFamily == 2) { > diff --git a/Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf b/Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf > index f48016cc..54c3f303 100644 > --- a/Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf > +++ b/Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf > @@ -55,7 +55,6 @@ > gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset > gArmTokenSpaceGuid.PcdSystemMemoryBase > gArmTokenSpaceGuid.PcdSystemMemorySize > - gRaspberryPiTokenSpaceGuid.PcdExtendedMemoryBase > gRaspberryPiTokenSpaceGuid.PcdNvStorageEventLogSize > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize > diff --git a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c > index aae189ec..61c50157 100644 > --- a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c > +++ b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c > @@ -58,18 +58,55 @@ ArmPlatformGetVirtualMemoryMap ( > { > UINTN Index = 0; > UINTN GpuIndex; > - INT64 OrigMemorySize; > - INT64 SystemMemorySize; > + INT64 TotalMemorySize; > + INT64 MemorySizeBelow3GB; > + INT64 MemorySizeBelow4GB; > ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable; > > // Early output of the info we got from VideoCore can prove valuable. > DEBUG ((DEBUG_INFO, "Board Rev: 0x%lX\n", mBoardRevision)); > - DEBUG ((DEBUG_INFO, "Base RAM : 0x%ll08X (Size 0x%ll08X)\n", mSystemMemoryBase, mSystemMemoryEnd + 1)); > + DEBUG ((DEBUG_INFO, "RAM below 1GB: 0x%ll08X (Size 0x%ll08X)\n", mSystemMemoryBase, mSystemMemoryEnd + 1)); The whole point of using "Base RAM :" was to preserve alignment, as the expectation is that people reading this debug info output know what it's about. > DEBUG ((DEBUG_INFO, "VideoCore: 0x%ll08X (Size 0x%ll08X)\n", mVideoCoreBase, mVideoCoreSize)); > > ASSERT (mSystemMemoryBase == 0); > ASSERT (VirtualMemoryMap != NULL); > > + // Compute the total RAM size available on this platform > + TotalMemorySize = SIZE_256MB; > + TotalMemorySize <<= (mBoardRevision >> 20) & 0x07; > + DEBUG ((DEBUG_INFO, "Total RAM: 0x%ll08X\n", TotalMemorySize)); > + > + // > + // Ensure that what we declare as System Memory doesn't overlap with the > + // SoC MMIO registers. This can be achieved through a MIN () with the > + // base address since SystemMemoryBase is 0 (we assert if it isn't). > + // > + ASSERT (BCM2836_SOC_REGISTERS < 4UL * SIZE_1GB); > + MemorySizeBelow4GB = MIN(TotalMemorySize, 4UL * SIZE_1GB); > + MemorySizeBelow4GB = MIN(MemorySizeBelow4GB, BCM2836_SOC_REGISTERS); > + if (BCM2711_SOC_REGISTERS > 0) { > + ASSERT (BCM2836_SOC_REGISTERS < 4UL * SIZE_1GB); > + MemorySizeBelow4GB = MIN(MemorySizeBelow4GB, BCM2711_SOC_REGISTERS); > + } > + > + // > + // By default we limit the memory to 3 GB to work around OS support for > + // DMA bugs in the SoC, for OSes that don't support _DMA range descriptors. > + // On boards with more RAM (4GB, 8GB), the extra memory is added by ConfigDxe > + // provided the configuration setting for 3GB limit is off. > + // > + MemorySizeBelow3GB = MIN(MemorySizeBelow4GB, 3UL * SIZE_1GB); > + > + // > + // On Pi 3 we've seen SoC registers may overlap the reported VideoCore range, > + // so clamp that down as well. > + // > + if (mVideoCoreBase + mVideoCoreSize > BCM2836_SOC_REGISTERS) { > + mVideoCoreSize = BCM2836_SOC_REGISTERS - mVideoCoreBase; > + DEBUG ((DEBUG_WARN, "VideoCore range overlapped SoC MMIO, now 0x%ll08X (Size 0x%ll08X)\n", > + mVideoCoreBase, mVideoCoreSize)); > + } > + > VirtualMemoryTable = (ARM_MEMORY_REGION_DESCRIPTOR*)AllocatePages > (EFI_SIZE_TO_PAGES (sizeof (ARM_MEMORY_REGION_DESCRIPTOR) * > MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS)); > @@ -77,7 +114,6 @@ ArmPlatformGetVirtualMemoryMap ( > return; > } > > - > // Firmware Volume > VirtualMemoryTable[Index].PhysicalBase = FixedPcdGet64 (PcdFdBaseAddress); > VirtualMemoryTable[Index].VirtualBase = VirtualMemoryTable[Index].PhysicalBase; > @@ -134,21 +170,9 @@ ArmPlatformGetVirtualMemoryMap ( > VirtualMemoryInfo[Index].Type = RPI_MEM_UNMAPPED_REGION; > VirtualMemoryInfo[Index++].Name = L"GPU Reserved"; > > - // Compute the total RAM size available on this platform > - SystemMemorySize = SIZE_256MB; > - SystemMemorySize <<= (mBoardRevision >> 20) & 0x07; > - > - // > - // 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). > - // > - 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; > @@ -159,45 +183,46 @@ ArmPlatformGetVirtualMemoryMap ( > > // Base SoC registers > VirtualMemoryTable[Index].PhysicalBase = BCM2836_SOC_REGISTERS; > - // On the Pi 3 the SoC registers may overlap VideoCore => fix this > - if (VirtualMemoryTable[GpuIndex].PhysicalBase + VirtualMemoryTable[GpuIndex].Length > VirtualMemoryTable[Index].PhysicalBase) { > - VirtualMemoryTable[GpuIndex].Length = VirtualMemoryTable[Index].PhysicalBase - VirtualMemoryTable[GpuIndex].PhysicalBase; > - } > VirtualMemoryTable[Index].VirtualBase = VirtualMemoryTable[Index].PhysicalBase; > VirtualMemoryTable[Index].Length = BCM2836_SOC_REGISTER_LENGTH; > VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > VirtualMemoryInfo[Index].Type = RPI_MEM_UNMAPPED_REGION; > VirtualMemoryInfo[Index++].Name = L"SoC Reserved (283x)"; > > - // > - // By default we limit the memory to 3 GB to work around the DMA bugs in the SoC, > - // for OSes that don't support _DMA range descriptors. On 4GB boards, it's runtime > - // setting to boot with 4 GB, and the additional 1 GB is added by ConfigDxe. > - // > - OrigMemorySize = SystemMemorySize; > - SystemMemorySize = MIN(SystemMemorySize, 3UL * SIZE_1GB); > > - // If we have RAM above the 1 GB mark, declare it > - if (SystemMemorySize - SIZE_1GB > 0) { > - VirtualMemoryTable[Index].PhysicalBase = FixedPcdGet64 (PcdExtendedMemoryBase); > + // Memory in the 1GB - 3GB range is always available. > + if (MemorySizeBelow3GB - SIZE_1GB > 0) { > + VirtualMemoryTable[Index].PhysicalBase = SIZE_1GB; > VirtualMemoryTable[Index].VirtualBase = VirtualMemoryTable[Index].PhysicalBase; > - VirtualMemoryTable[Index].Length = SystemMemorySize - SIZE_1GB; > + VirtualMemoryTable[Index].Length = MemorySizeBelow3GB - VirtualMemoryTable[Index].PhysicalBase; > VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK; > VirtualMemoryInfo[Index].Type = RPI_MEM_BASIC_REGION; > - VirtualMemoryInfo[Index++].Name = L"Extended System RAM below 3 GB"; > + VirtualMemoryInfo[Index++].Name = L"Extended System RAM below 3GB"; > } > > // > - // If we have RAM above 3 GB mark, declare it so it's mapped, but > - // don't add it to the memory map. This is done later by ConfigDxe if necessary. > + // If we have RAM in the 3GB - 4GB range, declare it as mapped, but don't > + // add it to the memory map. This is done later by ConfigDxe if necessary. > // > - if (OrigMemorySize > (3UL * SIZE_1GB)) { > + if (MemorySizeBelow4GB - 3UL * SIZE_1GB > 0) { Same comment as for the assert above. The CPU is more than capable to peform the subtraction for the comparison. ;) > VirtualMemoryTable[Index].PhysicalBase = 3UL * SIZE_1GB; > VirtualMemoryTable[Index].VirtualBase = VirtualMemoryTable[Index].PhysicalBase; > - VirtualMemoryTable[Index].Length = OrigMemorySize - VirtualMemoryTable[Index].PhysicalBase; > + VirtualMemoryTable[Index].Length = MemorySizeBelow4GB - VirtualMemoryTable[Index].PhysicalBase; > + VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK; > + VirtualMemoryInfo[Index].Type = RPI_MEM_UNMAPPED_REGION; > + VirtualMemoryInfo[Index++].Name = L"Extended System RAM below 4GB"; > + } > + > + // > + // Same treatment for the 4GB - 16GB range. I'm not seeing anything related to an actual 16 GB limit, and if the Pi Foundation keep using the scheme of amount of RAM = 256 MB << 3-bit value, then we could theoretically see 32 GB Pi's. In other words, I see little point of talking about 16 GB unless there is an actual 16 GB in effect somewhere (which, if that is the case, should be documented in a comment). So I would prefer to see a "> 4 GB" > + // > + if (TotalMemorySize - 4UL * SIZE_1GB > 0) { > + VirtualMemoryTable[Index].PhysicalBase = 4UL * SIZE_1GB; > + VirtualMemoryTable[Index].VirtualBase = VirtualMemoryTable[Index].PhysicalBase; > + VirtualMemoryTable[Index].Length = TotalMemorySize - VirtualMemoryTable[Index].PhysicalBase; > VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK; > VirtualMemoryInfo[Index].Type = RPI_MEM_UNMAPPED_REGION; > - VirtualMemoryInfo[Index++].Name = L"Extended System RAM above 3 GB"; > + VirtualMemoryInfo[Index++].Name = L"Extended System RAM below 16GB"; Same thing. What was wrong with changing "above 3 GB" to "above 4GB"? > } > > // End of Table > diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc > index cbff9d99..e055f13c 100644 > --- a/Platform/RaspberryPi/RPi4/RPi4.dsc > +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc > @@ -403,7 +403,6 @@ > # > # Device specific addresses > # > - gRaspberryPiTokenSpaceGuid.PcdExtendedMemoryBase|0x40000000 > gBcm27xxTokenSpaceGuid.PcdBcm27xxRegistersAddress|0xfc000000 > gBcm27xxTokenSpaceGuid.PcdBcmGenetRegistersAddress|0xfd580000 > gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress|0xfe000000 > diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec > index 1a3c44e0..5b402123 100644 > --- a/Platform/RaspberryPi/RaspberryPi.dec > +++ b/Platform/RaspberryPi/RaspberryPi.dec > @@ -39,7 +39,6 @@ > gRaspberryPiTokenSpaceGuid.PcdNvStorageVariableBase|0x0|UINT32|0x00000005 > gRaspberryPiTokenSpaceGuid.PcdNvStorageFtwSpareBase|0x0|UINT32|0x00000006 > gRaspberryPiTokenSpaceGuid.PcdNvStorageFtwWorkingBase|0x0|UINT32|0x00000007 > - gRaspberryPiTokenSpaceGuid.PcdExtendedMemoryBase|0x0|UINT32|0x00000008 > gRaspberryPiTokenSpaceGuid.PcdFdtSize|0x10000|UINT32|0x00000009 > gRaspberryPiTokenSpaceGuid.PcdCpuLowSpeedMHz|600|UINT32|0x0000000a > gRaspberryPiTokenSpaceGuid.PcdCpuDefSpeedMHz|800|UINT32|0x0000000b > With the non-blocking remarks above: Reviewed-by: Pete Batard