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 <kraxel@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));
+          PlatformAddMemoryRangeHob (Base, End);
+        }
+      }
+
+      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));
+      break;
+    default:
+      DEBUG ((
+        DEBUG_WARN,
+        "%a: Type %u [0x%Lx, 0x%Lx) (NOT HANDLED)\n",
+        __FUNCTION__,
+        E820Entry->Type,
+        Base,
+        End-1
+        ));
+      break;
+  }
+}
+
 /**
   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) {
-- 
2.39.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98674): https://edk2.groups.io/g/devel/message/98674
Mute This Topic: https://groups.io/mt/96328402/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 1/17/23 06:16, Gerd Hoffmann via groups.io 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.

Hi Gerd,

A problem was reported to me for an SEV-ES guest that I bisected to this 
patch. It only occurs when using the OVMF_CODE.fd file without specifying 
the OVMF_VARS.fd file (i.e. only the one pflash device on the qemu command 
line, but not using the OVMF.fd file). I don't ever boot without an 
OVMF_VARS.fd file, so I didn't catch this.

With this patch, SEV-ES terminates now because it detects doing MMIO to 
encrypted memory area at 0xFFC00000 (where the OVMF_VARS.fd file would 
normally be mapped). Prior to this commit, an SEV-ES guest booted without 
issue in this configuration.

First, is not specifying an OVMF_VARS.fd a valid configuration for booting 
given the CODE/VARS split build?

If it is valid, is the lack of the OVMF_VARS.fd resulting in the 
0xFFC00000 address range getting marked reserved now (and thus mapped 
encrypted)?

Let me know if you need me to provide any output or testing if you can't 
boot an SEV-ES guest.

Thanks,
Tom

> 
> Signed-off-by: Gerd Hoffmann <kraxel@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));
> +          PlatformAddMemoryRangeHob (Base, End);
> +        }
> +      }
> +
> +      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));
> +      break;
> +    default:
> +      DEBUG ((
> +        DEBUG_WARN,
> +        "%a: Type %u [0x%Lx, 0x%Lx) (NOT HANDLED)\n",
> +        __FUNCTION__,
> +        E820Entry->Type,
> +        Base,
> +        End-1
> +        ));
> +      break;
> +  }
> +}
> +
>   /**
>     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) {


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98986): https://edk2.groups.io/g/devel/message/98986
Mute This Topic: https://groups.io/mt/96328402/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, Jan 24, 2023 at 04:33:48PM -0600, Tom Lendacky wrote:
> On 1/17/23 06:16, Gerd Hoffmann via groups.io 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.
> 
> Hi Gerd,
> 
> A problem was reported to me for an SEV-ES guest that I bisected to this
> patch. It only occurs when using the OVMF_CODE.fd file without specifying
> the OVMF_VARS.fd file (i.e. only the one pflash device on the qemu command
> line, but not using the OVMF.fd file). I don't ever boot without an
> OVMF_VARS.fd file, so I didn't catch this.
> 
> With this patch, SEV-ES terminates now because it detects doing MMIO to
> encrypted memory area at 0xFFC00000 (where the OVMF_VARS.fd file would
> normally be mapped). Prior to this commit, an SEV-ES guest booted without
> issue in this configuration.
> 
> First, is not specifying an OVMF_VARS.fd a valid configuration for booting
> given the CODE/VARS split build?

No.

> If it is valid, is the lack of the OVMF_VARS.fd resulting in the 0xFFC00000
> address range getting marked reserved now (and thus mapped encrypted)?

I have no clue offhand.  The patch is not supposed to change OVMF
behavior.  Adding the HOBs was done by the (increasingly messy)
PlatformScanOrAdd64BitE820Ram() function before, with this patch in
place PlatformScanE820() + PlatformAddHobCB() handle it instead.  The
end result should be identical though.

OVMF does MMIO access @ 0xFFC00000, to check whenever it finds flash
there or not (to handle the -bios OVMF.fd case).  That happens at a
completely different place though (see
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c).

> Let me know if you need me to provide any output or testing if you can't
> boot an SEV-ES guest.

Yes, the firmware log hopefully gives clues what is going on here.

thanks,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99004): https://edk2.groups.io/g/devel/message/99004
Mute This Topic: https://groups.io/mt/96328402/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 1/25/23 03:11, Gerd Hoffmann wrote:
> On Tue, Jan 24, 2023 at 04:33:48PM -0600, Tom Lendacky wrote:
>> On 1/17/23 06:16, Gerd Hoffmann via groups.io 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.
>>
>> Hi Gerd,
>>
>> A problem was reported to me for an SEV-ES guest that I bisected to this
>> patch. It only occurs when using the OVMF_CODE.fd file without specifying
>> the OVMF_VARS.fd file (i.e. only the one pflash device on the qemu command
>> line, but not using the OVMF.fd file). I don't ever boot without an
>> OVMF_VARS.fd file, so I didn't catch this.
>>
>> With this patch, SEV-ES terminates now because it detects doing MMIO to
>> encrypted memory area at 0xFFC00000 (where the OVMF_VARS.fd file would
>> normally be mapped). Prior to this commit, an SEV-ES guest booted without
>> issue in this configuration.
>>
>> First, is not specifying an OVMF_VARS.fd a valid configuration for booting
>> given the CODE/VARS split build?
> 
> No.

Ok, good to know.

> 
>> If it is valid, is the lack of the OVMF_VARS.fd resulting in the 0xFFC00000
>> address range getting marked reserved now (and thus mapped encrypted)?
> 
> I have no clue offhand.  The patch is not supposed to change OVMF
> behavior.  Adding the HOBs was done by the (increasingly messy)
> PlatformScanOrAdd64BitE820Ram() function before, with this patch in
> place PlatformScanE820() + PlatformAddHobCB() handle it instead.  The
> end result should be identical though.
> 
> OVMF does MMIO access @ 0xFFC00000, to check whenever it finds flash
> there or not (to handle the -bios OVMF.fd case).  That happens at a
> completely different place though (see
> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c).
> 
>> Let me know if you need me to provide any output or testing if you can't
>> boot an SEV-ES guest.
> 
> Yes, the firmware log hopefully gives clues what is going on here.

So here are the differences (with some debug message that I added) between
booting at:

124b76505133 ("OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB")

   PlatformScanOrAdd64BitE820Ram: Reserved: Base=0xFEFFC000 Length=0x4000
   ...
   *** DEBUG: AmdSevDxeEntryPoint:120 - Clearing encryption bit for FF000000 to FFFFFFFF - MMIO=0
   *** DEBUG: AmdSevDxeEntryPoint:120 - Clearing encryption bit for 180000000 to 7FFFFFFFFFFF - MMIO=0
   ...
   QEMU Flash: Failed to find probe location
   QEMU flash was not detected. Writable FVB is not being installed.

and

328076cfdf45 ("OvmfPkg/PlatformInitLib: Add PlatformAddHobCB")

   PlatformAddHobCB: Reserved [0xFEFFC000, 0xFF000000)
   PlatformAddHobCB: HighMemory [0x100000000, 0x180000000)
   ...
   *** DEBUG: AmdSevDxeEntryPoint:120 - Clearing encryption bit for 1FDFFC000 to 7FFFFFFFFFFF - MMIO=0
   ...
   MMIO using encrypted memory: FFC00000
   !!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID - 00000000 !!!!


So before the patch in question, we see that AmdSevDxeEntryPoint() in
OvmfPkg/AmdSevDxe/AmdSevDxe.c found an entry in the GCD map for 0xFF000000
to 0xFFFFFFFF that was marked as EfiGcdMemoryTypeNonExistent and so the
mapping was changed to unencrypted. But after that patch, that entry is
not present and so the 0xFFC00000 address is mapped encrypted and results
in the failure.

Thanks,
Tom

> 
> thanks,
>    Gerd
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99027): https://edk2.groups.io/g/devel/message/99027
Mute This Topic: https://groups.io/mt/96328402/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Tom,

On 1/25/23 16:35, Tom Lendacky wrote:
> On 1/25/23 03:11, Gerd Hoffmann wrote:
>> On Tue, Jan 24, 2023 at 04:33:48PM -0600, Tom Lendacky wrote:
>>> On 1/17/23 06:16, Gerd Hoffmann via groups.io 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.
>>>
>>> Hi Gerd,
>>>
>>> A problem was reported to me for an SEV-ES guest that I bisected to
>>> this patch. It only occurs when using the OVMF_CODE.fd file without
>>> specifying the OVMF_VARS.fd file (i.e. only the one pflash device on
>>> the qemu command line, but not using the OVMF.fd file). I don't ever
>>> boot without an OVMF_VARS.fd file, so I didn't catch this.
>>>
>>> With this patch, SEV-ES terminates now because it detects doing MMIO
>>> to encrypted memory area at 0xFFC00000 (where the OVMF_VARS.fd file
>>> would normally be mapped). Prior to this commit, an SEV-ES guest
>>> booted without issue in this configuration.
>>>
>>> First, is not specifying an OVMF_VARS.fd a valid configuration for
>>> booting
>>> given the CODE/VARS split build?
>>
>> No.
>
> Ok, good to know.
>
>>
>>> If it is valid, is the lack of the OVMF_VARS.fd resulting in the
>>> 0xFFC00000 address range getting marked reserved now (and thus
>>> mapped encrypted)?
>>
>> I have no clue offhand.  The patch is not supposed to change OVMF
>> behavior.  Adding the HOBs was done by the (increasingly messy)
>> PlatformScanOrAdd64BitE820Ram() function before, with this patch in
>> place PlatformScanE820() + PlatformAddHobCB() handle it instead.  The
>> end result should be identical though.
>>
>> OVMF does MMIO access @ 0xFFC00000, to check whenever it finds flash
>> there or not (to handle the -bios OVMF.fd case).  That happens at a
>> completely different place though (see
>> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c).
>>
>>> Let me know if you need me to provide any output or testing if you
>>> can't boot an SEV-ES guest.
>>
>> Yes, the firmware log hopefully gives clues what is going on here.
>
> So here are the differences (with some debug message that I added)
> between booting at:
>
> 124b76505133 ("OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB")
>
>   PlatformScanOrAdd64BitE820Ram: Reserved: Base=0xFEFFC000
>   Length=0x4000
>   ...
>   *** DEBUG: AmdSevDxeEntryPoint:120 - Clearing encryption bit for
> FF000000 to FFFFFFFF - MMIO=0
>   *** DEBUG: AmdSevDxeEntryPoint:120 - Clearing encryption bit for
> 180000000 to 7FFFFFFFFFFF - MMIO=0
>   ...
>   QEMU Flash: Failed to find probe location
>   QEMU flash was not detected. Writable FVB is not being installed.
>
> and
>
> 328076cfdf45 ("OvmfPkg/PlatformInitLib: Add PlatformAddHobCB")
>
>   PlatformAddHobCB: Reserved [0xFEFFC000, 0xFF000000)
>   PlatformAddHobCB: HighMemory [0x100000000, 0x180000000)
>   ...
>   *** DEBUG: AmdSevDxeEntryPoint:120 - Clearing encryption bit for
> 1FDFFC000 to 7FFFFFFFFFFF - MMIO=0
>   ...
>   MMIO using encrypted memory: FFC00000
>   !!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID
>   - 000000 !!!!
>
>
> So before the patch in question, we see that AmdSevDxeEntryPoint() in
> OvmfPkg/AmdSevDxe/AmdSevDxe.c found an entry in the GCD map for
> 0xFF000000 to 0xFFFFFFFF that was marked as
> EfiGcdMemoryTypeNonExistent and so the mapping was changed to
> unencrypted. But after that patch, that entry is not present and so
> the 0xFFC00000 address is mapped encrypted and results in the failure.

Thanks for reporting this.  I overlooked an issue in commit
328076cfdf45, but now I think I'm seeing it.

OVMF's Platform PEI (nowadays: Platform Init Lib) provides two
*families* of internal helper functions, for creating HOBs:

  PlatformAddXxxBaseSizeHob
  PlatformAddXxxRangeHob

The first family takes base and *size*, the second family takes base and
*end*.  For Xxx, you can substitute IoMemory, Memory, and
ReservedMemory.  (Well, for ReservedMemory, we don't have the "Range"
variant.)  Implementation-wise, the "Range" variant is always a thin
wrapper around the "BaseSize" variant.

The issue in commit 328076cfdf45 is the following:

- Before commit 328076cfdf45, PlatformScanOrAdd64BitE820Ram() would add
  (a) system memory with PlatformAddMemoryRangeHob(), that is, as a
  *range*, and (b) reserved memory directly with
  BuildResourceDescriptorHob(), which takes a base and a *size*.

- After commit 328076cfdf45, the PlatformAddHobCB() callback calculates
  a *range* uniformly, and then passes it to both (a)
  PlatformAddMemoryRangeHob(), for adding system memory, after rounding,
  and (b) BuildResourceDescriptorHob(), for adding reserved memory.  The
  bug is that for (b), we pass "base + size" where
  BuildResourceDescriptorHob() only expects "size", so internally the
  "end" will be determined not as "base + size", but as "base + (base +
  size)".

Can you try this patch?

> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index 5aeeeff89f57..38cece9173e8 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -200,7 +200,7 @@ PlatformAddHobCB (
>
>        break;
>      case EfiAcpiAddressRangeReserved:
> -      BuildResourceDescriptorHob (EFI_RESOURCE_MEMORY_RESERVED, 0, Base, End);
> +      BuildResourceDescriptorHob (EFI_RESOURCE_MEMORY_RESERVED, 0, Base, End - Base);
>        DEBUG ((DEBUG_INFO, "%a: Reserved [0x%Lx, 0x%Lx)\n", __FUNCTION__, Base, End));
>        break;
>      default:

Sorry about missing the bug in review.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99307): https://edk2.groups.io/g/devel/message/99307
Mute This Topic: https://groups.io/mt/96328402/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 1/30/23 16:23, Laszlo Ersek wrote:
> Hi Tom,
> 
> On 1/25/23 16:35, Tom Lendacky wrote:
>> On 1/25/23 03:11, Gerd Hoffmann wrote:
>>> On Tue, Jan 24, 2023 at 04:33:48PM -0600, Tom Lendacky wrote:
>>>> On 1/17/23 06:16, Gerd Hoffmann via groups.io 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.
>>>>
>>>> Hi Gerd,
>>>>
>>>> A problem was reported to me for an SEV-ES guest that I bisected to
>>>> this patch. It only occurs when using the OVMF_CODE.fd file without
>>>> specifying the OVMF_VARS.fd file (i.e. only the one pflash device on
>>>> the qemu command line, but not using the OVMF.fd file). I don't ever
>>>> boot without an OVMF_VARS.fd file, so I didn't catch this.
>>>>
>>>> With this patch, SEV-ES terminates now because it detects doing MMIO
>>>> to encrypted memory area at 0xFFC00000 (where the OVMF_VARS.fd file
>>>> would normally be mapped). Prior to this commit, an SEV-ES guest
>>>> booted without issue in this configuration.
>>>>
>>>> First, is not specifying an OVMF_VARS.fd a valid configuration for
>>>> booting
>>>> given the CODE/VARS split build?
>>>
>>> No.
>>
>> Ok, good to know.
>>
>>>
>>>> If it is valid, is the lack of the OVMF_VARS.fd resulting in the
>>>> 0xFFC00000 address range getting marked reserved now (and thus
>>>> mapped encrypted)?
>>>
>>> I have no clue offhand.  The patch is not supposed to change OVMF
>>> behavior.  Adding the HOBs was done by the (increasingly messy)
>>> PlatformScanOrAdd64BitE820Ram() function before, with this patch in
>>> place PlatformScanE820() + PlatformAddHobCB() handle it instead.  The
>>> end result should be identical though.
>>>
>>> OVMF does MMIO access @ 0xFFC00000, to check whenever it finds flash
>>> there or not (to handle the -bios OVMF.fd case).  That happens at a
>>> completely different place though (see
>>> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c).
>>>
>>>> Let me know if you need me to provide any output or testing if you
>>>> can't boot an SEV-ES guest.
>>>
>>> Yes, the firmware log hopefully gives clues what is going on here.
>>
>> So here are the differences (with some debug message that I added)
>> between booting at:
>>
>> 124b76505133 ("OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB")
>>
>>   PlatformScanOrAdd64BitE820Ram: Reserved: Base=0xFEFFC000
>>   Length=0x4000
>>   ...
>>   *** DEBUG: AmdSevDxeEntryPoint:120 - Clearing encryption bit for
>> FF000000 to FFFFFFFF - MMIO=0
>>   *** DEBUG: AmdSevDxeEntryPoint:120 - Clearing encryption bit for
>> 180000000 to 7FFFFFFFFFFF - MMIO=0
>>   ...
>>   QEMU Flash: Failed to find probe location
>>   QEMU flash was not detected. Writable FVB is not being installed.
>>
>> and
>>
>> 328076cfdf45 ("OvmfPkg/PlatformInitLib: Add PlatformAddHobCB")
>>
>>   PlatformAddHobCB: Reserved [0xFEFFC000, 0xFF000000)
>>   PlatformAddHobCB: HighMemory [0x100000000, 0x180000000)
>>   ...
>>   *** DEBUG: AmdSevDxeEntryPoint:120 - Clearing encryption bit for
>> 1FDFFC000 to 7FFFFFFFFFFF - MMIO=0
>>   ...
>>   MMIO using encrypted memory: FFC00000
>>   !!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID
>>   - 000000 !!!!
>>
>>
>> So before the patch in question, we see that AmdSevDxeEntryPoint() in
>> OvmfPkg/AmdSevDxe/AmdSevDxe.c found an entry in the GCD map for
>> 0xFF000000 to 0xFFFFFFFF that was marked as
>> EfiGcdMemoryTypeNonExistent and so the mapping was changed to
>> unencrypted. But after that patch, that entry is not present and so
>> the 0xFFC00000 address is mapped encrypted and results in the failure.
> 
> Thanks for reporting this.  I overlooked an issue in commit
> 328076cfdf45, but now I think I'm seeing it.
> 
> OVMF's Platform PEI (nowadays: Platform Init Lib) provides two
> *families* of internal helper functions, for creating HOBs:
> 
>   PlatformAddXxxBaseSizeHob
>   PlatformAddXxxRangeHob
> 
> The first family takes base and *size*, the second family takes base and
> *end*.  For Xxx, you can substitute IoMemory, Memory, and
> ReservedMemory.  (Well, for ReservedMemory, we don't have the "Range"
> variant.)  Implementation-wise, the "Range" variant is always a thin
> wrapper around the "BaseSize" variant.
> 
> The issue in commit 328076cfdf45 is the following:
> 
> - Before commit 328076cfdf45, PlatformScanOrAdd64BitE820Ram() would add
>   (a) system memory with PlatformAddMemoryRangeHob(), that is, as a
>   *range*, and (b) reserved memory directly with
>   BuildResourceDescriptorHob(), which takes a base and a *size*.
> 
> - After commit 328076cfdf45, the PlatformAddHobCB() callback calculates
>   a *range* uniformly, and then passes it to both (a)
>   PlatformAddMemoryRangeHob(), for adding system memory, after rounding,
>   and (b) BuildResourceDescriptorHob(), for adding reserved memory.  The
>   bug is that for (b), we pass "base + size" where
>   BuildResourceDescriptorHob() only expects "size", so internally the
>   "end" will be determined not as "base + size", but as "base + (base +
>   size)".
> 
> Can you try this patch?
> 
>> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
>> index 5aeeeff89f57..38cece9173e8 100644
>> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
>> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
>> @@ -200,7 +200,7 @@ PlatformAddHobCB (
>>
>>        break;
>>      case EfiAcpiAddressRangeReserved:
>> -      BuildResourceDescriptorHob (EFI_RESOURCE_MEMORY_RESERVED, 0, Base, End);
>> +      BuildResourceDescriptorHob (EFI_RESOURCE_MEMORY_RESERVED, 0, Base, End - Base);
>>        DEBUG ((DEBUG_INFO, "%a: Reserved [0x%Lx, 0x%Lx)\n", __FUNCTION__, Base, End));
>>        break;
>>      default:
> 
> Sorry about missing the bug in review.

Apologies, just noticed commit f25ee5476343 ("OvmfPkg: fix
BuildResourceDescriptorHob call in PlatformAddHobCB()", 2023-01-26).

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99308): https://edk2.groups.io/g/devel/message/99308
Mute This Topic: https://groups.io/mt/96328402/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 1/25/23 09:35, Tom Lendacky wrote:
> On 1/25/23 03:11, Gerd Hoffmann wrote:
>> On Tue, Jan 24, 2023 at 04:33:48PM -0600, Tom Lendacky wrote:
>>> On 1/17/23 06:16, Gerd Hoffmann via groups.io 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.
>>>
>>> Hi Gerd,
>>>
>>> A problem was reported to me for an SEV-ES guest that I bisected to this
>>> patch. It only occurs when using the OVMF_CODE.fd file without specifying
>>> the OVMF_VARS.fd file (i.e. only the one pflash device on the qemu command
>>> line, but not using the OVMF.fd file). I don't ever boot without an
>>> OVMF_VARS.fd file, so I didn't catch this.
>>>
>>> With this patch, SEV-ES terminates now because it detects doing MMIO to
>>> encrypted memory area at 0xFFC00000 (where the OVMF_VARS.fd file would
>>> normally be mapped). Prior to this commit, an SEV-ES guest booted without
>>> issue in this configuration.
>>>
>>> First, is not specifying an OVMF_VARS.fd a valid configuration for booting
>>> given the CODE/VARS split build?
>>
>> No.
> 
> Ok, good to know.
> 
>>
>>> If it is valid, is the lack of the OVMF_VARS.fd resulting in the 
>>> 0xFFC00000
>>> address range getting marked reserved now (and thus mapped encrypted)?
>>
>> I have no clue offhand.  The patch is not supposed to change OVMF
>> behavior.  Adding the HOBs was done by the (increasingly messy)
>> PlatformScanOrAdd64BitE820Ram() function before, with this patch in
>> place PlatformScanE820() + PlatformAddHobCB() handle it instead.  The
>> end result should be identical though.
>>
>> OVMF does MMIO access @ 0xFFC00000, to check whenever it finds flash
>> there or not (to handle the -bios OVMF.fd case).  That happens at a
>> completely different place though (see
>> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c).
>>
>>> Let me know if you need me to provide any output or testing if you can't
>>> boot an SEV-ES guest.
>>
>> Yes, the firmware log hopefully gives clues what is going on here.
> 
> So here are the differences (with some debug message that I added) between
> booting at:
> 
> 124b76505133 ("OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB")
> 
>    PlatformScanOrAdd64BitE820Ram: Reserved: Base=0xFEFFC000 Length=0x4000
>    ...
>    *** DEBUG: AmdSevDxeEntryPoint:120 - Clearing encryption bit for 
> FF000000 to FFFFFFFF - MMIO=0
>    *** DEBUG: AmdSevDxeEntryPoint:120 - Clearing encryption bit for 
> 180000000 to 7FFFFFFFFFFF - MMIO=0
>    ...
>    QEMU Flash: Failed to find probe location
>    QEMU flash was not detected. Writable FVB is not being installed.
> 
> and
> 
> 328076cfdf45 ("OvmfPkg/PlatformInitLib: Add PlatformAddHobCB")
> 
>    PlatformAddHobCB: Reserved [0xFEFFC000, 0xFF000000)
>    PlatformAddHobCB: HighMemory [0x100000000, 0x180000000)
>    ...
>    *** DEBUG: AmdSevDxeEntryPoint:120 - Clearing encryption bit for 
> 1FDFFC000 to 7FFFFFFFFFFF - MMIO=0
>    ...
>    MMIO using encrypted memory: FFC00000
>    !!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID - 
> 00000000 !!!!
> 
> 
> So before the patch in question, we see that AmdSevDxeEntryPoint() in
> OvmfPkg/AmdSevDxe/AmdSevDxe.c found an entry in the GCD map for 0xFF000000
> to 0xFFFFFFFF that was marked as EfiGcdMemoryTypeNonExistent and so the
> mapping was changed to unencrypted. But after that patch, that entry is
> not present and so the 0xFFC00000 address is mapped encrypted and results
> in the failure.

This issue also causes use of the OVMF.fd file usage to fail for both SEV
and SEV-ES. With this patch using the combined file gives:

   Firmware Volume for Variable Store is corrupted
   ASSERT_EFI_ERROR (Status = Volume Corrupt)
   ASSERT [VariableRuntimeDxe] /root/kernels/ovmf-build-X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c(546): !(((INTN)(RETURN_STATUS)(Status)) < 0)

I believe for the same reason, that the mapping is encrypted, which causes
the signature and GUID checks to fail.

Thanks,
Tom

> 
> Thanks,
> Tom
> 
>>
>> thanks,
>>    Gerd
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99028): https://edk2.groups.io/g/devel/message/99028
Mute This Topic: https://groups.io/mt/96328402/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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 <kraxel@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 <lersek@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, 17 Jan 2023 at 16:03, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 1/17/23 13:16, Gerd Hoffmann wrote:

> > +          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).
>

As it turns out, yes. Uncrustify demands a newline here, or else ....


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98711): https://edk2.groups.io/g/devel/message/98711
Mute This Topic: https://groups.io/mt/96328402/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, 17 Jan 2023 at 16:03, Laszlo Ersek <lersek@redhat.com> wrote:
>
> 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 <kraxel@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 <lersek@redhat.com>
>

Thanks Laszlo - I'll fix up the above locally.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98707): https://edk2.groups.io/g/devel/message/98707
Mute This Topic: https://groups.io/mt/96328402/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-