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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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