[edk2-platforms][PATCH 0/2] RPi4 fixes to 3GB RAM limit logic

[edk2-platforms][PATCH 0/2] RPi4 fixes to 3GB RAM limit logic

[Andrei Warkentin]

Dear all,

Here are two minor fixes to the recently checked-in runtime logic
for enabling/disabling 3GB limit.

It's been tested on 2GB and 4GB boards.

Andrei Warkentin (2):
  Platform/RaspberryPi/Drivers/ConfigDxe: fix bug in 3GB RAM logic
  Platform/RaspberryPi/Drivers/PlatformSmbiosDxe: improve UX with 3GB
    limit

 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c         | 3 ++-
 .../Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c          | 7 ++++++-
 .../Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf        | 2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)

-- 
2.17.1

[edk2-platforms][PATCH 1/2] Platform/RaspberryPi/Drivers/ConfigDxe: fix bug in 3GB RAM logic

[Andrei Warkentin]

The original change*** used positive logic (PcdPi4GBEnabled), while the
upstreamed change uses negative logic (PcdRamLimitTo3GB), which
requires an additional condition, or it blows up on 1GiB and 2GiB boards.

Tested on 2GB and 4GB boards (with limiting and without)

*** https://github.com/pftf/edk2-platforms/\
    commit/968451beb7c9302517098abf72f7e42b57a0e024

Signed-off-by: Andrei Warkentin <awarkentin@vmware.com>
---
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
index ccac8daa..5fca3c7a 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
@@ -288,7 +288,8 @@ ApplyVariables (
     DEBUG ((DEBUG_INFO, "Current CPU speed is %uHz\n", Rate));
   }
 
-  if (mModelFamily >= 4 && PcdGet32 (PcdRamLimitTo3GB) == 0) {
+  if (mModelFamily >= 4 && PcdGet32 (PcdRamMoreThan3GB) &&
+      PcdGet32 (PcdRamLimitTo3GB) == 0) {
     /*
      * Similar to how we compute the > 3 GB RAM segment's size in PlatformLib/
      * RaspberryPiMem.c, with some overlap protection for the Bcm2xxx register
-- 
2.17.1

Re: [edk2-platforms][PATCH 1/2] Platform/RaspberryPi/Drivers/ConfigDxe: fix bug in 3GB RAM logic

[Pete Batard]

On 2020.03.04 22:31, Andrei Warkentin wrote:
> The original change*** used positive logic (PcdPi4GBEnabled), while the
> upstreamed change uses negative logic (PcdRamLimitTo3GB), which
> requires an additional condition, or it blows up on 1GiB and 2GiB boards.

Good catch!

> Tested on 2GB and 4GB boards (with limiting and without)
> 
> *** https://github.com/pftf/edk2-platforms/\
>      commit/968451beb7c9302517098abf72f7e42b57a0e024
> 
> Signed-off-by: Andrei Warkentin <awarkentin@vmware.com>
> ---
>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> index ccac8daa..5fca3c7a 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> @@ -288,7 +288,8 @@ ApplyVariables (
>       DEBUG ((DEBUG_INFO, "Current CPU speed is %uHz\n", Rate));
>     }
>   
> -  if (mModelFamily >= 4 && PcdGet32 (PcdRamLimitTo3GB) == 0) {
> +  if (mModelFamily >= 4 && PcdGet32 (PcdRamMoreThan3GB) &&

Just a super minor nitpick, but if we want to be consistent with the 
next line, where we use the comparison to the integer value as the 
boolean, instead of using the result of the PcdGet operation itself, 
then the condition above should be:

      if (mModelFamily >= 4 && PcdGet32 (PcdRamMoreThan3GB) != 0 &&

Since it doesn't matter either way, I'm 100% fine with the patch as it 
stands. It's just that I remember trying to go for an explicit integer 
comparison for code clarity, on account that we went for integer PCDs 
rather than boolean ones...

> +      PcdGet32 (PcdRamLimitTo3GB) == 0) {
>       /*
>        * Similar to how we compute the > 3 GB RAM segment's size in PlatformLib/
>        * RaspberryPiMem.c, with some overlap protection for the Bcm2xxx register
> 

Reviewed-by: Pete Batard <pete@akeo.ie>

Re: [edk2-devel] [edk2-platforms][PATCH 1/2] Platform/RaspberryPi/Drivers/ConfigDxe: fix bug in 3GB RAM logic

[Philippe Mathieu-Daudé]

On 3/5/20 12:38 AM, Pete Batard wrote:
> On 2020.03.04 22:31, Andrei Warkentin wrote:
>> The original change*** used positive logic (PcdPi4GBEnabled), while the
>> upstreamed change uses negative logic (PcdRamLimitTo3GB), which
>> requires an additional condition, or it blows up on 1GiB and 2GiB boards.
> 
> Good catch!
> 
>> Tested on 2GB and 4GB boards (with limiting and without)
>>
>> *** https://github.com/pftf/edk2-platforms/\
>>      commit/968451beb7c9302517098abf72f7e42b57a0e024
>>
>> Signed-off-by: Andrei Warkentin <awarkentin@vmware.com>
>> ---
>>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c 
>> b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> index ccac8daa..5fca3c7a 100644
>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> @@ -288,7 +288,8 @@ ApplyVariables (
>>       DEBUG ((DEBUG_INFO, "Current CPU speed is %uHz\n", Rate));
>>     }
>> -  if (mModelFamily >= 4 && PcdGet32 (PcdRamLimitTo3GB) == 0) {
>> +  if (mModelFamily >= 4 && PcdGet32 (PcdRamMoreThan3GB) &&
> 
> Just a super minor nitpick, but if we want to be consistent with the 
> next line, where we use the comparison to the integer value as the 
> boolean, instead of using the result of the PcdGet operation itself, 
> then the condition above should be:
> 
>       if (mModelFamily >= 4 && PcdGet32 (PcdRamMoreThan3GB) != 0 &&

Yes please. Can the maintainer amend that change? It can be quicker to 
simply repost. With the change:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> 
> Since it doesn't matter either way, I'm 100% fine with the patch as it 
> stands. It's just that I remember trying to go for an explicit integer 
> comparison for code clarity, on account that we went for integer PCDs 
> rather than boolean ones...
> 
>> +      PcdGet32 (PcdRamLimitTo3GB) == 0) {
>>       /*
>>        * Similar to how we compute the > 3 GB RAM segment's size in 
>> PlatformLib/
>>        * RaspberryPiMem.c, with some overlap protection for the 
>> Bcm2xxx register
>>
> 
> Reviewed-by: Pete Batard <pete@akeo.ie>
> 
> 
> 
> 

[edk2-platforms][PATCH 2/2] Platform/RaspberryPi/Drivers/PlatformSmbiosDxe: improve UX with 3GB limit

[Andrei Warkentin]

Right now there was no way to tell you're booting with RAM limited
to 3GB, since the setup front page still listed 4096 MB.

Fix this by honoring PcdRamLimitTo3GB in PlatformSmbiosDxe.

Tested on 2GB and 4GB boards (with limiting and without)

Signed-off-by: Andrei Warkentin <awarkentin@vmware.com>
---
 Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c   | 7 ++++++-
 Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
index 5585cb84..3351fea2 100644
--- a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
+++ b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
@@ -882,7 +882,12 @@ MemArrMapInfoUpdateSmbiosType19 (
   if (Status != EFI_SUCCESS) {
     DEBUG ((DEBUG_WARN, "Couldn't get the board memory size - defaulting to 256 MB: %r\n", Status));
   } else {
-    mMemArrMapInfoType19.EndingAddress = InstalledMB * 1024;
+    if (PcdGet32 (PcdRamMoreThan3GB) && PcdGet32 (PcdRamLimitTo3GB)) {
+      ASSERT (InstalledMB > 3 * 1024);
+      mMemArrMapInfoType19.EndingAddress = 3 * 1024 * 1024;
+    } else {
+      mMemArrMapInfoType19.EndingAddress = InstalledMB * 1024;
+    }
   }
   mMemArrMapInfoType19.EndingAddress -= 1;
 
diff --git a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
index 9554c2e9..1ed6338c 100644
--- a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
+++ b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
@@ -52,3 +52,5 @@
   gArmTokenSpaceGuid.PcdSystemMemorySize
   gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor
   gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
+  gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB
+  gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB
-- 
2.17.1

Re: [edk2-platforms][PATCH 2/2] Platform/RaspberryPi/Drivers/PlatformSmbiosDxe: improve UX with 3GB limit

[Pete Batard]

On 2020.03.04 22:31, Andrei Warkentin wrote:
> Right now there was no way to tell you're booting with RAM limited
> to 3GB, since the setup front page still listed 4096 MB.
> 
> Fix this by honoring PcdRamLimitTo3GB in PlatformSmbiosDxe.
> 
> Tested on 2GB and 4GB boards (with limiting and without)
> 
> Signed-off-by: Andrei Warkentin <awarkentin@vmware.com>
> ---
>   Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c   | 7 ++++++-
>   Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf | 2 ++
>   2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
> index 5585cb84..3351fea2 100644
> --- a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
> +++ b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
> @@ -882,7 +882,12 @@ MemArrMapInfoUpdateSmbiosType19 (
>     if (Status != EFI_SUCCESS) {
>       DEBUG ((DEBUG_WARN, "Couldn't get the board memory size - defaulting to 256 MB: %r\n", Status));
>     } else {
> -    mMemArrMapInfoType19.EndingAddress = InstalledMB * 1024;
> +    if (PcdGet32 (PcdRamMoreThan3GB) && PcdGet32 (PcdRamLimitTo3GB)) {
> +      ASSERT (InstalledMB > 3 * 1024);
> +      mMemArrMapInfoType19.EndingAddress = 3 * 1024 * 1024;
> +    } else {
> +      mMemArrMapInfoType19.EndingAddress = InstalledMB * 1024;
> +    }
>     }
>     mMemArrMapInfoType19.EndingAddress -= 1;
>   
> diff --git a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
> index 9554c2e9..1ed6338c 100644
> --- a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
> +++ b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
> @@ -52,3 +52,5 @@
>     gArmTokenSpaceGuid.PcdSystemMemorySize
>     gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor
>     gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
> +  gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB
> +  gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB
> 

Reviewed-by: Pete Batard <pete@akeo.ie>

Re: [edk2-platforms][PATCH 0/2] RPi4 fixes to 3GB RAM limit logic

[Ard Biesheuvel]

On Wed, 4 Mar 2020 at 23:31, Andrei Warkentin <awarkentin@vmware.com> wrote:
>
> Dear all,
>
> Here are two minor fixes to the recently checked-in runtime logic
> for enabling/disabling 3GB limit.
>
> It's been tested on 2GB and 4GB boards.
>

Thanks Andrei,

For future postings, could you please use git send-email or some other
tool that doesn't mangle patches and require me to go through each one
and fix it up by hand? Diff surgery is not the best use of my time.

Thanks,

> Andrei Warkentin (2):
>   Platform/RaspberryPi/Drivers/ConfigDxe: fix bug in 3GB RAM logic
>   Platform/RaspberryPi/Drivers/PlatformSmbiosDxe: improve UX with 3GB
>     limit
>
>  Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c         | 3 ++-
>  .../Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c          | 7 ++++++-
>  .../Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf        | 2 ++
>  3 files changed, 10 insertions(+), 2 deletions(-)
>
> --
> 2.17.1
>

Re: [edk2-platforms][PATCH 0/2] RPi4 fixes to 3GB RAM limit logic

[Ard Biesheuvel]

On Thu, 5 Mar 2020 at 14:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Wed, 4 Mar 2020 at 23:31, Andrei Warkentin <awarkentin@vmware.com> wrote:
> >
> > Dear all,
> >
> > Here are two minor fixes to the recently checked-in runtime logic
> > for enabling/disabling 3GB limit.
> >
> > It's been tested on 2GB and 4GB boards.
> >
>
> Thanks Andrei,
>
> For future postings, could you please use git send-email or some other
> tool that doesn't mangle patches and require me to go through each one
> and fix it up by hand? Diff surgery is not the best use of my time.
>
> Thanks,
>
> > Andrei Warkentin (2):
> >   Platform/RaspberryPi/Drivers/ConfigDxe: fix bug in 3GB RAM logic
> >   Platform/RaspberryPi/Drivers/PlatformSmbiosDxe: improve UX with 3GB
> >     limit
> >

Pushed as dac18ca51781..28d2f4ecd87d

(with the suggested modification, and acks applied)