1. Topics
  2. [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB

[PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB

Gerd Hoffmann
 

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@...>
---
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
Laszlo Ersek
 

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@...>
---
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@...>
Ard Biesheuvel
 

On Tue, 17 Jan 2023 at 16:03, Laszlo Ersek <lersek@...> 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@...>
---
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@...>
Thanks Laszlo - I'll fix up the above locally.
Ard Biesheuvel
 

On Tue, 17 Jan 2023 at 16:03, Laszlo Ersek <lersek@...> 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 ....
Lendacky, Thomas
 

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@...>
---
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) {
Gerd Hoffmann
 

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
Lendacky, Thomas
 

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
Lendacky, Thomas
 

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
Laszlo Ersek
 

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
Laszlo Ersek
 

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
1 - 10 of 10