[PATCH 0/2] OvmfPkg/PlatformPei: prefer etc/e820 for memory detection

[PATCH 0/2] OvmfPkg/PlatformPei: prefer etc/e820 for memory detection

[Gerd Hoffmann]

Don't use cmos for memory detection if possible.
qemu provides the etc/e820 firmware config file
as alternative since 2013.

Gerd Hoffmann (2):
  OvmfPkg/PlatformPei: ScanOrAdd64BitE820Ram improvements
  OvmfPkg/PlatformPei: prefer etc/e820 for memory detection

 OvmfPkg/PlatformPei/MemDetect.c | 43 +++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 10 deletions(-)

-- 
2.31.1

[PATCH 1/2] OvmfPkg/PlatformPei: ScanOrAdd64BitE820Ram improvements

[Gerd Hoffmann]

Add a bool parameter to ScanOrAdd64BitE820Ram to explicitly specify
whenever ScanOrAdd64BitE820Ram should add HOBs for high memory (above
4G) or scan only.

Also add a lowmem parameter so ScanOrAdd64BitE820Ram
can report the memory size below 4G.

This allows a more flexible usage of ScanOrAdd64BitE820Ram,
a followup patch will use it for all memory detection.

No functional change.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/PlatformPei/MemDetect.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 2deec128f464..d7fb3e742be3 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -200,6 +200,8 @@ QemuUc32BaseInitialization (
 STATIC
 EFI_STATUS
 ScanOrAdd64BitE820Ram (
+  IN BOOLEAN AddHighHob,
+  OUT UINT64 *LowMemory OPTIONAL,
   OUT UINT64 *MaxAddress OPTIONAL
   )
 {
@@ -217,6 +219,9 @@ ScanOrAdd64BitE820Ram (
     return EFI_PROTOCOL_ERROR;
   }
 
+  if (LowMemory != NULL) {
+    *LowMemory = 0;
+  }
   if (MaxAddress != NULL) {
     *MaxAddress = BASE_4GB;
   }
@@ -232,9 +237,8 @@ ScanOrAdd64BitE820Ram (
       E820Entry.Length,
       E820Entry.Type
       ));
-    if (E820Entry.Type == EfiAcpiAddressRangeMemory &&
-        E820Entry.BaseAddr >= BASE_4GB) {
-      if (MaxAddress == NULL) {
+    if (E820Entry.Type == EfiAcpiAddressRangeMemory) {
+      if (AddHighHob && E820Entry.BaseAddr >= BASE_4GB) {
         UINT64 Base;
         UINT64 End;
 
@@ -254,11 +258,12 @@ ScanOrAdd64BitE820Ram (
             End
             ));
         }
-      } else {
+      }
+      if (MaxAddress || LowMemory) {
         UINT64 Candidate;
 
         Candidate = E820Entry.BaseAddr + E820Entry.Length;
-        if (Candidate > *MaxAddress) {
+        if (MaxAddress && Candidate > *MaxAddress) {
           *MaxAddress = Candidate;
           DEBUG ((
             DEBUG_VERBOSE,
@@ -267,6 +272,15 @@ ScanOrAdd64BitE820Ram (
             *MaxAddress
             ));
         }
+        if (LowMemory && Candidate > *LowMemory && Candidate < BASE_4GB) {
+          *LowMemory = Candidate;
+          DEBUG ((
+            DEBUG_VERBOSE,
+            "%a: LowMemory=0x%Lx\n",
+            __FUNCTION__,
+            *LowMemory
+            ));
+        }
       }
     }
   }
@@ -353,7 +367,7 @@ GetFirstNonAddress (
   // Otherwise, get the flat size of the memory above 4GB from the CMOS (which
   // can only express a size smaller than 1TB), and add it to 4GB.
   //
-  Status = ScanOrAdd64BitE820Ram (&FirstNonAddress);
+  Status = ScanOrAdd64BitE820Ram (FALSE, NULL, &FirstNonAddress);
   if (EFI_ERROR (Status)) {
     FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb ();
   }
@@ -754,7 +768,7 @@ QemuInitializeRam (
     // entries. Otherwise, create a single memory HOB with the flat >=4GB
     // memory size read from the CMOS.
     //
-    Status = ScanOrAdd64BitE820Ram (NULL);
+    Status = ScanOrAdd64BitE820Ram (TRUE, NULL, NULL);
     if (EFI_ERROR (Status) && UpperMemorySize != 0) {
       AddMemoryBaseSizeHob (BASE_4GB, UpperMemorySize);
     }
-- 
2.31.1

Re: [edk2-devel] [PATCH 1/2] OvmfPkg/PlatformPei: ScanOrAdd64BitE820Ram improvements

[Philippe Mathieu-Daudé]

On 8/19/21 10:11 AM, Gerd Hoffmann wrote:
> Add a bool parameter to ScanOrAdd64BitE820Ram to explicitly specify
> whenever ScanOrAdd64BitE820Ram should add HOBs for high memory (above
> 4G) or scan only.
> 
> Also add a lowmem parameter so ScanOrAdd64BitE820Ram
> can report the memory size below 4G.
> 
> This allows a more flexible usage of ScanOrAdd64BitE820Ram,
> a followup patch will use it for all memory detection.
> 
> No functional change.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/PlatformPei/MemDetect.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)

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

[PATCH 2/2] OvmfPkg/PlatformPei: prefer etc/e820 for memory detection

[Gerd Hoffmann]

Prefer the e820 map provided via qemu firmware config interface
for memory detection.  Use rtc cmos only as fallback, which should
be rarely needed these days as qemu supports etc/e820 since 2013.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/PlatformPei/MemDetect.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index d7fb3e742be3..71bd28bd1d2b 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -293,9 +293,16 @@ GetSystemMemorySizeBelow4gb (
   VOID
   )
 {
+  EFI_STATUS Status;
+  UINT64 LowerMemorySize;
   UINT8 Cmos0x34;
   UINT8 Cmos0x35;
 
+  Status = ScanOrAdd64BitE820Ram (FALSE, &LowerMemorySize, NULL);
+  if (Status == EFI_SUCCESS || LowerMemorySize > 0) {
+    return LowerMemorySize;
+  }
+
   //
   // CMOS 0x34/0x35 specifies the system memory above 16 MB.
   // * CMOS(0x35) is the high byte
@@ -722,7 +729,6 @@ QemuInitializeRam (
   // Determine total memory size available
   //
   LowerMemorySize = GetSystemMemorySizeBelow4gb ();
-  UpperMemorySize = GetSystemMemorySizeAbove4gb ();
 
   if (mBootMode == BOOT_ON_S3_RESUME) {
     //
@@ -769,8 +775,11 @@ QemuInitializeRam (
     // memory size read from the CMOS.
     //
     Status = ScanOrAdd64BitE820Ram (TRUE, NULL, NULL);
-    if (EFI_ERROR (Status) && UpperMemorySize != 0) {
-      AddMemoryBaseSizeHob (BASE_4GB, UpperMemorySize);
+    if (EFI_ERROR (Status)) {
+      UpperMemorySize = GetSystemMemorySizeAbove4gb ();
+      if (UpperMemorySize != 0) {
+        AddMemoryBaseSizeHob (BASE_4GB, UpperMemorySize);
+      }
     }
   }
 
-- 
2.31.1

Re: [edk2-devel] [PATCH 2/2] OvmfPkg/PlatformPei: prefer etc/e820 for memory detection

[Philippe Mathieu-Daudé]

On 8/19/21 10:11 AM, Gerd Hoffmann wrote:
> Prefer the e820 map provided via qemu firmware config interface
> for memory detection.  Use rtc cmos only as fallback, which should
> be rarely needed these days as qemu supports etc/e820 since 2013.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/PlatformPei/MemDetect.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)

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

Re: [edk2-devel] [PATCH 2/2] OvmfPkg/PlatformPei: prefer etc/e820 for memory detection

[Yao, Jiewen]

Hi
Would you please follow EDKII process?
1) File an EDKII Bugzilla.
2) CC all reviewers in OVMF package.


Please also describe what the reason of change, what is the benefit we can get from the change?

I just feel it is confusing. Previously, the data is consistent from CMOS. Now, we have two ways to get one data from different sources.

Please help me understand:

A) What if the data are different from different source?

B) Why we choose to trust E820 at first, the CMOS? Not verse versa.

C) If we trust E820 (in B), then why we need go back to CMOS, if LowMemorySize is 0?

Thank you
Yao Jiewen


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Thursday, August 19, 2021 4:11 PM
> To: devel@edk2.groups.io
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Subject: [edk2-devel] [PATCH 2/2] OvmfPkg/PlatformPei: prefer etc/e820 for
> memory detection
> 
> Prefer the e820 map provided via qemu firmware config interface
> for memory detection.  Use rtc cmos only as fallback, which should
> be rarely needed these days as qemu supports etc/e820 since 2013.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/PlatformPei/MemDetect.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c
> b/OvmfPkg/PlatformPei/MemDetect.c
> index d7fb3e742be3..71bd28bd1d2b 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -293,9 +293,16 @@ GetSystemMemorySizeBelow4gb (
>    VOID
>    )
>  {
> +  EFI_STATUS Status;
> +  UINT64 LowerMemorySize;
>    UINT8 Cmos0x34;
>    UINT8 Cmos0x35;
> 
> +  Status = ScanOrAdd64BitE820Ram (FALSE, &LowerMemorySize, NULL);
> +  if (Status == EFI_SUCCESS || LowerMemorySize > 0) {
> +    return LowerMemorySize;
> +  }
> +
>    //
>    // CMOS 0x34/0x35 specifies the system memory above 16 MB.
>    // * CMOS(0x35) is the high byte
> @@ -722,7 +729,6 @@ QemuInitializeRam (
>    // Determine total memory size available
>    //
>    LowerMemorySize = GetSystemMemorySizeBelow4gb ();
> -  UpperMemorySize = GetSystemMemorySizeAbove4gb ();
> 
>    if (mBootMode == BOOT_ON_S3_RESUME) {
>      //
> @@ -769,8 +775,11 @@ QemuInitializeRam (
>      // memory size read from the CMOS.
>      //
>      Status = ScanOrAdd64BitE820Ram (TRUE, NULL, NULL);
> -    if (EFI_ERROR (Status) && UpperMemorySize != 0) {
> -      AddMemoryBaseSizeHob (BASE_4GB, UpperMemorySize);
> +    if (EFI_ERROR (Status)) {
> +      UpperMemorySize = GetSystemMemorySizeAbove4gb ();
> +      if (UpperMemorySize != 0) {
> +        AddMemoryBaseSizeHob (BASE_4GB, UpperMemorySize);
> +      }
>      }
>    }
> 
> --
> 2.31.1
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 2/2] OvmfPkg/PlatformPei: prefer etc/e820 for memory detection

[Gerd Hoffmann]

On Wed, Aug 25, 2021 at 09:24:43AM +0000, Yao, Jiewen wrote:
> Hi
> Would you please follow EDKII process?
> 1) File an EDKII Bugzilla.

Ok, will do.

> 2) CC all reviewers in OVMF package.

Is there some way to automate this?

I see there is BaseTools/Scripts/GetMaintainer.py, but the script wants
a commit hash not a patch file as argument, so I can't hook it into 'git
send-email'.

> Please also describe what the reason of change, what is the benefit we
> can get from the change?
> 
> I just feel it is confusing. Previously, the data is consistent from
> CMOS. Now, we have two ways to get one data from different sources.

It is *not* consistent from CMOS.  CMOS is only used for memory below 4G
whereas the etc/e820 fw_cfg file is used for memory above 4G.  So this
patch actually makes things more consistent.

> Please help me understand:
> 
> A) What if the data are different from different source?
>
> B) Why we choose to trust E820 at first, the CMOS? Not verse versa.

e820 is the newer and more capable interface, specifically cmos can
handle at most 1TB of memory (which is the reason why e820 is already
used to detect high memory).

> C) If we trust E820 (in B), then why we need go back to CMOS, if LowMemorySize is 0?

Backward compatibility with old qemu versions.  etc/e820 is available
in qemu version 1.7 (released Nov 2013) and newer.

Does OVMF have any policy for backward compatibility?  If breaking
compatibility with qemu versions that old is acceptable I happily
delete any CMOS access from qemu PlatformPei and throw an assert()
instead.

take care,
  Gerd

Re: [edk2-devel] [PATCH 2/2] OvmfPkg/PlatformPei: prefer etc/e820 for memory detection

[Yao, Jiewen]

Thanks for the detail info.
We do have process for handle compatibility.

My recommendation is:
1) Please send out RFC email about removing CMOS support in QEMU.
To see if someone still need support old version before qemu version 1.7 (released Nov 2013).
2) Let's wait for 1 WW.
3) If no people need this, we can file a bugzilla to remove CMOS. Then we can follow up to submit patch to remove.


Please help me understand another thing: Is that any plan in QEMU to *remove* CMOS interface ?
That could be a good indicator to remove the problem - we have two ways to get one data.


I am worried the logic below to add "LowerMemorySize > 0".

=====================
GetSystemMemorySizeBelow4gb()
{
  EFI_STATUS Status;
  UINT64 LowerMemorySize;
  UINT8 Cmos0x34;
  UINT8 Cmos0x35;

  Status = ScanOrAdd64BitE820Ram (FALSE, &LowerMemorySize, NULL);
  if (Status == EFI_SUCCESS || LowerMemorySize > 0) {
    return LowerMemorySize;
  }
...
}

EFI_STATUS
ScanOrAdd64BitE820Ram (
  IN BOOLEAN AddHighHob,
  OUT UINT64 *LowMemory OPTIONAL,
  OUT UINT64 *MaxAddress OPTIONAL
  )
{
  EFI_STATUS           Status;
  FIRMWARE_CONFIG_ITEM FwCfgItem;
  UINTN                FwCfgSize;
  EFI_E820_ENTRY64     E820Entry;
  UINTN                Processed;

  Status = QemuFwCfgFindFile ("etc/e820", &FwCfgItem, &FwCfgSize);
  if (EFI_ERROR (Status)) {
    return Status;
  }
  if (FwCfgSize % sizeof E820Entry != 0) {
    return EFI_PROTOCOL_ERROR;
  }

  if (LowMemory != NULL) {
    *LowMemory = 0;
  }
...
}
=====================

In GetSystemMemorySizeBelow4gb (), we return LowerMemorySize, if Status is SUCCESS, or (Status == ERROR && LowerMemorySize > 0)

However, the LowerMemorySize is unitialized data in stack. It may be a garbage.
In ScanOrAdd64BitE820Ram(), the LowerMemorySize is initialized after the ERROR is returned.

That means, if the Status is ERROR, we have still a big chance to return LowerMemorySize > 0 case, if the LowerMemorySize is garbage.



Would you please also share the info, what kind of test you have run for that code? Have you tried the E820 fail case?

Thank you
Yao Jiewen






> -----Original Message-----
> From: kraxel@redhat.com <kraxel@redhat.com>
> Sent: Wednesday, August 25, 2021 9:14 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: devel@edk2.groups.io
> Subject: Re: [edk2-devel] [PATCH 2/2] OvmfPkg/PlatformPei: prefer etc/e820
> for memory detection
> 
> On Wed, Aug 25, 2021 at 09:24:43AM +0000, Yao, Jiewen wrote:
> > Hi
> > Would you please follow EDKII process?
> > 1) File an EDKII Bugzilla.
> 
> Ok, will do.
> 
> > 2) CC all reviewers in OVMF package.
> 
> Is there some way to automate this?
> 
> I see there is BaseTools/Scripts/GetMaintainer.py, but the script wants
> a commit hash not a patch file as argument, so I can't hook it into 'git
> send-email'.
> 
> > Please also describe what the reason of change, what is the benefit we
> > can get from the change?
> >
> > I just feel it is confusing. Previously, the data is consistent from
> > CMOS. Now, we have two ways to get one data from different sources.
> 
> It is *not* consistent from CMOS.  CMOS is only used for memory below 4G
> whereas the etc/e820 fw_cfg file is used for memory above 4G.  So this
> patch actually makes things more consistent.
> 
> > Please help me understand:
> >
> > A) What if the data are different from different source?
> >
> > B) Why we choose to trust E820 at first, the CMOS? Not verse versa.
> 
> e820 is the newer and more capable interface, specifically cmos can
> handle at most 1TB of memory (which is the reason why e820 is already
> used to detect high memory).
> 
> > C) If we trust E820 (in B), then why we need go back to CMOS, if
> LowMemorySize is 0?
> 
> Backward compatibility with old qemu versions.  etc/e820 is available
> in qemu version 1.7 (released Nov 2013) and newer.
> 
> Does OVMF have any policy for backward compatibility?  If breaking
> compatibility with qemu versions that old is acceptable I happily
> delete any CMOS access from qemu PlatformPei and throw an assert()
> instead.
> 
> take care,
>   Gerd

Re: [edk2-devel] [PATCH 2/2] OvmfPkg/PlatformPei: prefer etc/e820 for memory detection

[Gerd Hoffmann]

On Wed, Aug 25, 2021 at 03:15:04PM +0000, Yao, Jiewen wrote:
> Thanks for the detail info.
> We do have process for handle compatibility.
> 
> My recommendation is:
> 1) Please send out RFC email about removing CMOS support in QEMU.
> To see if someone still need support old version before qemu version 1.7 (released Nov 2013).
> 2) Let's wait for 1 WW.
> 3) If no people need this, we can file a bugzilla to remove CMOS. Then we can follow up to submit patch to remove.
> 
> 
> Please help me understand another thing: Is that any plan in QEMU to *remove* CMOS interface ?

For the microvm machine type several legacy devices are optional (pic,
pit, rtc).  cmos is provided by the rtc, so turning off rtc will also
remove the cmos.

There is no microvm support in ovmf yet, I have some experimental
patches but its incomplete + low priority right now.

There are no plans to drop cmos for 'pc' and 'q35' machine types.

> That could be a good indicator to remove the problem - we have two ways to get one data.

Can hardly be avoided long-term.  It happens now and then that some
interface turns out to be insufficient and can't be easily extended.
So adding a new, better one while keeping the old working for
compatibility is the only way out ...

> I am worried the logic below to add "LowerMemorySize > 0".
> 
> =====================
> GetSystemMemorySizeBelow4gb()
> {
>   EFI_STATUS Status;
>   UINT64 LowerMemorySize;
>   UINT8 Cmos0x34;
>   UINT8 Cmos0x35;
> 
>   Status = ScanOrAdd64BitE820Ram (FALSE, &LowerMemorySize, NULL);
>   if (Status == EFI_SUCCESS || LowerMemorySize > 0) {
>     return LowerMemorySize;
>   }

Oops, yes, that logic is wrong.  LowerMemorySize must be initialized and
it should be "&&" not "||".  I'll fix it for v2.

take care,
  Gerd