On 1/17/23 13:16, Gerd Hoffmann wrote:
> Add PlatformAddHobCB() callback function for use with
> PlatformScanE820(). It adds HOBs for high memory and reservations (low
> memory is handled elsewhere because there are some special cases to
> consider). This replaces calls to PlatformScanOrAdd64BitE820Ram() with
> AddHighHobs = TRUE.
>
> Write any actions done (adding HOBs, skip unknown types) to the firmware
> log with INFO loglevel.
>
> Also remove PlatformScanOrAdd64BitE820Ram() which is not used any more.
>
> Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
> ---
> OvmfPkg/Library/PlatformInitLib/MemDetect.c | 185 +++++---------------
> 1 file changed, 47 insertions(+), 138 deletions(-)
>
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index dfaa05a1c24f..c626fc49cf6c 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -112,143 +112,6 @@ PlatformQemuUc32BaseInitialization (
> }
> }
>
> -/**
> - Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map that start
> outside
> - of the 32-bit address range.
> -
> - Find the highest exclusive >=4GB RAM address, or produce memory resource
> - descriptor HOBs for RAM entries that start at or above 4GB.
> -
> - @param[out] MaxAddress If MaxAddress is NULL, then
> PlatformScanOrAdd64BitE820Ram()
> - produces memory resource descriptor HOBs for RAM
> - entries that start at or above 4GB.
> -
> - Otherwise, MaxAddress holds the highest exclusive
> - >=4GB RAM address on output. If QEMU's fw_cfg E820
> - RAM map contains no RAM entry that starts outside
> of
> - the 32-bit address range, then MaxAddress is
> exactly
> - 4GB on output.
> -
> - @retval EFI_SUCCESS The fw_cfg E820 RAM map was found and
> processed.
> -
> - @retval EFI_PROTOCOL_ERROR The RAM map was found, but its size wasn't a
> - whole multiple of sizeof(EFI_E820_ENTRY64). No
> - RAM entry was processed.
> -
> - @return Error codes from QemuFwCfgFindFile(). No RAM
> - entry was processed.
> -**/
> -STATIC
> -EFI_STATUS
> -PlatformScanOrAdd64BitE820Ram (
> - 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;
> - }
> -
> - if (MaxAddress != NULL) {
> - *MaxAddress = BASE_4GB;
> - }
> -
> - QemuFwCfgSelectItem (FwCfgItem);
> - for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) {
> - QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry);
> - DEBUG ((
> - DEBUG_VERBOSE,
> - "%a: Base=0x%Lx Length=0x%Lx Type=%u\n",
> - __FUNCTION__,
> - E820Entry.BaseAddr,
> - E820Entry.Length,
> - E820Entry.Type
> - ));
> - if (E820Entry.Type == EfiAcpiAddressRangeMemory) {
> - if (AddHighHob && (E820Entry.BaseAddr >= BASE_4GB)) {
> - UINT64 Base;
> - UINT64 End;
> -
> - //
> - // Round up the start address, and round down the end address.
> - //
> - Base = ALIGN_VALUE (E820Entry.BaseAddr, (UINT64)EFI_PAGE_SIZE);
> - End = (E820Entry.BaseAddr + E820Entry.Length) &
> - ~(UINT64)EFI_PAGE_MASK;
> - if (Base < End) {
> - PlatformAddMemoryRangeHob (Base, End);
> - DEBUG ((
> - DEBUG_VERBOSE,
> - "%a: PlatformAddMemoryRangeHob [0x%Lx, 0x%Lx)\n",
> - __FUNCTION__,
> - Base,
> - End
> - ));
> - }
> - }
> -
> - if (MaxAddress || LowMemory) {
> - UINT64 Candidate;
> -
> - Candidate = E820Entry.BaseAddr + E820Entry.Length;
> - if (MaxAddress && (Candidate > *MaxAddress)) {
> - *MaxAddress = Candidate;
> - DEBUG ((
> - DEBUG_VERBOSE,
> - "%a: MaxAddress=0x%Lx\n",
> - __FUNCTION__,
> - *MaxAddress
> - ));
> - }
> -
> - if (LowMemory && (Candidate > *LowMemory) && (Candidate < BASE_4GB))
> {
> - *LowMemory = Candidate;
> - DEBUG ((
> - DEBUG_VERBOSE,
> - "%a: LowMemory=0x%Lx\n",
> - __FUNCTION__,
> - *LowMemory
> - ));
> - }
> - }
> - } else if (E820Entry.Type == EfiAcpiAddressRangeReserved) {
> - if (AddHighHob) {
> - DEBUG ((
> - DEBUG_INFO,
> - "%a: Reserved: Base=0x%Lx Length=0x%Lx\n",
> - __FUNCTION__,
> - E820Entry.BaseAddr,
> - E820Entry.Length
> - ));
> - BuildResourceDescriptorHob (
> - EFI_RESOURCE_MEMORY_RESERVED,
> - 0,
> - E820Entry.BaseAddr,
> - E820Entry.Length
> - );
> - }
> - }
> - }
> -
> - return EFI_SUCCESS;
> -}
> -
> typedef VOID (*E820_SCAN_CALLBACK) (
> EFI_E820_ENTRY64 *E820Entry,
> EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> @@ -304,6 +167,52 @@ PlatformGetLowMemoryCB (
> }
> }
>
> +/**
> + Create HOBs for reservations and RAM (except low memory).
> +**/
> +STATIC VOID
> +PlatformAddHobCB (
> + IN EFI_E820_ENTRY64 *E820Entry,
> + IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> + )
> +{
> + UINT64 Base, End;
> +
> + Base = E820Entry->BaseAddr;
> + End = E820Entry->BaseAddr + E820Entry->Length;
> +
> + switch (E820Entry->Type) {
> + case EfiAcpiAddressRangeMemory:
> + if (Base >= BASE_4GB) {
> + //
> + // Round up the start address, and round down the end address.
> + //
> + Base = ALIGN_VALUE (Base, (UINT64)EFI_PAGE_SIZE);
> + End = End & ~(UINT64)EFI_PAGE_MASK;
> + if (Base < End) {
> + DEBUG ((DEBUG_INFO, "%a: HighMemory [0x%Lx, 0x%Lx)\n",
> __FUNCTION__, Base, End-1));
(1) Hehe, this is a bit funny: we now have *both* the round closing
parenthesies ")" in the debug message, for the interval, *and* (End-1)
as the argument! :)
We need exactly one of those. Either-or. :) If the argument is exclusive
("End"), then the interval should be closed with ")". If the arg is
inclusive ("End-1"), then we need a bracket "]".
> + PlatformAddMemoryRangeHob (Base, End);
> + }
> + }
> +
(2) Still not sure if this empty line is intentional (it may be, you
didn't answer it under the v2 review AFAICT).
> + break;
> + case EfiAcpiAddressRangeReserved:
> + BuildResourceDescriptorHob (EFI_RESOURCE_MEMORY_RESERVED, 0, Base,
> End);
> + DEBUG ((DEBUG_INFO, "%a: Reserved [0x%Lx, 0x%Lx)\n", __FUNCTION__,
> Base, End-1));
(3) Same comment as (1) -- please pick either End-1 as arg, with "]", or
")" as interval closing character, with End as arg.
> + break;
> + default:
> + DEBUG ((
> + DEBUG_WARN,
> + "%a: Type %u [0x%Lx, 0x%Lx) (NOT HANDLED)\n",
> + __FUNCTION__,
> + E820Entry->Type,
> + Base,
> + End-1
> + ));
> + break;
> + }
(4) Same as (1).
> +}
> +
> /**
> Iterate over the entries in QEMU's fw_cfg E820 RAM map, call the
> passed callback for each entry.
> @@ -1049,7 +958,7 @@ PlatformQemuInitializeRam (
> // entries. Otherwise, create a single memory HOB with the flat >=4GB
> // memory size read from the CMOS.
> //
> - Status = PlatformScanOrAdd64BitE820Ram (TRUE, NULL, NULL);
> + Status = PlatformScanE820 (PlatformAddHobCB, PlatformInfoHob);
> if (EFI_ERROR (Status)) {
> UpperMemorySize = PlatformGetSystemMemorySizeAbove4gb ();
> if (UpperMemorySize != 0) {
Update as you see fit, either way:
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98706): https://edk2.groups.io/g/devel/message/98706
Mute This Topic: https://groups.io/mt/96328402/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
