* [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts
@ 2023-01-17 12:16 Gerd Hoffmann
2023-01-17 12:16 ` [PATCH v4 1/5] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB Gerd Hoffmann
` (5 more replies)
0 siblings, 6 replies; 22+ messages in thread
From: Gerd Hoffmann @ 2023-01-17 12:16 UTC (permalink / raw)
To: devel
Cc: Jiewen Yao, Oliver Steffen, László Érsek,
Ard Biesheuvel, Pawel Polawski, Jordan Justen, Gerd Hoffmann
v4:
- made mmio window move a bit more robust.
- dropped patches for moving MMCONFIG, they'll come as separate patch
series later.
v3:
- Add / fix comments, add notes to commit messages.
- Make functions static.
- Logging tweaks.
- Fix windows compiler warnings.
- Add patches (5,6,7) moving MMCONFIG to 0xe0000000, simplifying code
and reducing differences between 'pc' and 'q35' along the way.
Eventually we want split them into a separate series, but some of
this was discussed in v2 review, so I just appended them here for
now.
v2:
- split up PlatformScanOrAdd64BitE820Ram() into scan function with
callbacks, store results in PlatformInfoHob struct.
Gerd Hoffmann (5):
OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB
OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB
OvmfPkg/PlatformInitLib: Add PlatformAddHobCB
OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB
OvmfPkg/PlatformInitLib: reorder PlatformQemuUc32BaseInitialization
OvmfPkg/Include/Library/PlatformInitLib.h | 3 +-
OvmfPkg/Library/PeilessStartupLib/Hob.c | 3 +-
.../PeilessStartupLib/PeilessStartup.c | 7 +-
OvmfPkg/Library/PlatformInitLib/MemDetect.c | 356 ++++++++++--------
OvmfPkg/Library/PlatformInitLib/Platform.c | 7 +-
OvmfPkg/PlatformPei/MemDetect.c | 3 +-
6 files changed, 214 insertions(+), 165 deletions(-)
--
2.39.0
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4 1/5] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB
2023-01-17 12:16 [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts Gerd Hoffmann
@ 2023-01-17 12:16 ` Gerd Hoffmann
2023-01-17 14:30 ` Laszlo Ersek
2023-01-17 14:46 ` Laszlo Ersek
2023-01-17 12:16 ` [PATCH v4 2/5] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB Gerd Hoffmann
` (4 subsequent siblings)
5 siblings, 2 replies; 22+ messages in thread
From: Gerd Hoffmann @ 2023-01-17 12:16 UTC (permalink / raw)
To: devel
Cc: Jiewen Yao, Oliver Steffen, László Érsek,
Ard Biesheuvel, Pawel Polawski, Jordan Justen, Gerd Hoffmann
First step replacing the PlatformScanOrAdd64BitE820Ram() function.
Add a PlatformScanE820() function which loops over the e280 entries
from FwCfg and calls a callback for each of them.
Add a GetFirstNonAddressCB() function which will store the first free
address (right after the last RAM block) in
PlatformInfoHob->FirstNonAddress. This replaces calls to
PlatformScanOrAdd64BitE820Ram() with non-NULL MaxAddress.
Write any actions done (setting FirstNonAddress) to the firmware log
with INFO loglevel.
Also drop local FirstNonAddress variables and use
PlatformInfoHob->FirstNonAddress instead everywhere.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
OvmfPkg/Library/PlatformInitLib/MemDetect.c | 115 ++++++++++++++++----
1 file changed, 92 insertions(+), 23 deletions(-)
diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index 882805269b3e..1ea91f78cefd 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -251,6 +251,83 @@ PlatformScanOrAdd64BitE820Ram (
return EFI_SUCCESS;
}
+typedef VOID (*E820_SCAN_CALLBACK) (
+ EFI_E820_ENTRY64 *E820Entry,
+ EFI_HOB_PLATFORM_INFO *PlatformInfoHob
+ );
+
+/**
+ Store first address not used by e820 RAM entries in
+ PlatformInfoHob->FirstNonAddress
+**/
+VOID
+PlatformGetFirstNonAddressCB (
+ IN EFI_E820_ENTRY64 *E820Entry,
+ IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
+ )
+{
+ UINT64 Candidate;
+
+ if (E820Entry->Type != EfiAcpiAddressRangeMemory) {
+ return;
+ }
+
+ Candidate = E820Entry->BaseAddr + E820Entry->Length;
+ if (PlatformInfoHob->FirstNonAddress < Candidate) {
+ DEBUG ((DEBUG_INFO, "%a: FirstNonAddress=0x%Lx\n", __FUNCTION__, Candidate));
+ PlatformInfoHob->FirstNonAddress = Candidate;
+ }
+}
+
+/**
+ Iterate over the entries in QEMU's fw_cfg E820 RAM map, call the
+ passed callback for each entry.
+
+ @param[in] Callback The callback function to be called.
+
+ @param[in out] PlatformInfoHob PlatformInfo struct which is passed
+ through to the callback.
+
+ @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
+PlatformScanE820 (
+ IN E820_SCAN_CALLBACK Callback,
+ IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
+ )
+{
+ 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;
+ }
+
+ QemuFwCfgSelectItem (FwCfgItem);
+ for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) {
+ QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry);
+ Callback (&E820Entry, PlatformInfoHob);
+ }
+
+ return EFI_SUCCESS;
+}
+
/**
Returns PVH memmap
@@ -384,23 +461,17 @@ PlatformGetSystemMemorySizeAbove4gb (
Return the highest address that DXE could possibly use, plus one.
**/
STATIC
-UINT64
+VOID
PlatformGetFirstNonAddress (
IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
)
{
- UINT64 FirstNonAddress;
UINT32 FwCfgPciMmio64Mb;
EFI_STATUS Status;
FIRMWARE_CONFIG_ITEM FwCfgItem;
UINTN FwCfgSize;
UINT64 HotPlugMemoryEnd;
- //
- // set FirstNonAddress to suppress incorrect compiler/analyzer warnings
- //
- FirstNonAddress = 0;
-
//
// If QEMU presents an E820 map, then get the highest exclusive >=4GB RAM
// address from it. This can express an address >= 4GB+1TB.
@@ -408,9 +479,10 @@ PlatformGetFirstNonAddress (
// Otherwise, get the flat size of the memory above 4GB from the CMOS (which
// can only express a size smaller than 1TB), and add it to 4GB.
//
- Status = PlatformScanOrAdd64BitE820Ram (FALSE, NULL, &FirstNonAddress);
+ PlatformInfoHob->FirstNonAddress = BASE_4GB;
+ Status = PlatformScanE820 (PlatformGetFirstNonAddressCB, PlatformInfoHob);
if (EFI_ERROR (Status)) {
- FirstNonAddress = BASE_4GB + PlatformGetSystemMemorySizeAbove4gb ();
+ PlatformInfoHob->FirstNonAddress = BASE_4GB + PlatformGetSystemMemorySizeAbove4gb ();
}
//
@@ -420,7 +492,7 @@ PlatformGetFirstNonAddress (
//
#ifdef MDE_CPU_IA32
if (!FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
- return FirstNonAddress;
+ return;
}
#endif
@@ -473,7 +545,7 @@ PlatformGetFirstNonAddress (
// determines the highest address plus one. The memory hotplug area (see
// below) plays no role for the firmware in this case.
//
- return FirstNonAddress;
+ return;
}
//
@@ -497,15 +569,15 @@ PlatformGetFirstNonAddress (
HotPlugMemoryEnd
));
- ASSERT (HotPlugMemoryEnd >= FirstNonAddress);
- FirstNonAddress = HotPlugMemoryEnd;
+ ASSERT (HotPlugMemoryEnd >= PlatformInfoHob->FirstNonAddress);
+ PlatformInfoHob->FirstNonAddress = HotPlugMemoryEnd;
}
//
// SeaBIOS aligns both boundaries of the 64-bit PCI host aperture to 1GB, so
// that the host can map it with 1GB hugepages. Follow suit.
//
- PlatformInfoHob->PcdPciMmio64Base = ALIGN_VALUE (FirstNonAddress, (UINT64)SIZE_1GB);
+ PlatformInfoHob->PcdPciMmio64Base = ALIGN_VALUE (PlatformInfoHob->FirstNonAddress, (UINT64)SIZE_1GB);
PlatformInfoHob->PcdPciMmio64Size = ALIGN_VALUE (PlatformInfoHob->PcdPciMmio64Size, (UINT64)SIZE_1GB);
//
@@ -519,8 +591,8 @@ PlatformGetFirstNonAddress (
//
// The useful address space ends with the 64-bit PCI host aperture.
//
- FirstNonAddress = PlatformInfoHob->PcdPciMmio64Base + PlatformInfoHob->PcdPciMmio64Size;
- return FirstNonAddress;
+ PlatformInfoHob->FirstNonAddress = PlatformInfoHob->PcdPciMmio64Base + PlatformInfoHob->PcdPciMmio64Size;
+ return;
}
/*
@@ -781,7 +853,6 @@ PlatformAddressWidthInitialization (
IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
)
{
- UINT64 FirstNonAddress;
UINT8 PhysMemAddressWidth;
EFI_STATUS Status;
@@ -794,7 +865,7 @@ PlatformAddressWidthInitialization (
// First scan host-provided hardware information to assess if the address
// space is already known. If so, guest must use those values.
//
- Status = PlatformScanHostProvided64BitPciMmioEnd (&FirstNonAddress);
+ Status = PlatformScanHostProvided64BitPciMmioEnd (&PlatformInfoHob->FirstNonAddress);
if (EFI_ERROR (Status)) {
//
@@ -806,13 +877,12 @@ PlatformAddressWidthInitialization (
// The DXL IPL keys off of the physical address bits advertized in the CPU
// HOB. To conserve memory, we calculate the minimum address width here.
//
- FirstNonAddress = PlatformGetFirstNonAddress (PlatformInfoHob);
+ PlatformGetFirstNonAddress (PlatformInfoHob);
}
PlatformAddressWidthFromCpuid (PlatformInfoHob, TRUE);
if (PlatformInfoHob->PhysMemAddressWidth != 0) {
// physical address width is known
- PlatformInfoHob->FirstNonAddress = FirstNonAddress;
PlatformDynamicMmioWindow (PlatformInfoHob);
return;
}
@@ -823,13 +893,13 @@ PlatformAddressWidthInitialization (
// -> try be conservstibe to stay below the guaranteed minimum of
// 36 phys bits (aka 64 GB).
//
- PhysMemAddressWidth = (UINT8)HighBitSet64 (FirstNonAddress);
+ PhysMemAddressWidth = (UINT8)HighBitSet64 (PlatformInfoHob->FirstNonAddress);
//
// If FirstNonAddress is not an integral power of two, then we need an
// additional bit.
//
- if ((FirstNonAddress & (FirstNonAddress - 1)) != 0) {
+ if ((PlatformInfoHob->FirstNonAddress & (PlatformInfoHob->FirstNonAddress - 1)) != 0) {
++PhysMemAddressWidth;
}
@@ -857,7 +927,6 @@ PlatformAddressWidthInitialization (
ASSERT (PhysMemAddressWidth <= 48);
#endif
- PlatformInfoHob->FirstNonAddress = FirstNonAddress;
PlatformInfoHob->PhysMemAddressWidth = PhysMemAddressWidth;
}
--
2.39.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 2/5] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB
2023-01-17 12:16 [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts Gerd Hoffmann
2023-01-17 12:16 ` [PATCH v4 1/5] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB Gerd Hoffmann
@ 2023-01-17 12:16 ` Gerd Hoffmann
2023-01-17 14:45 ` Laszlo Ersek
2023-01-17 12:16 ` [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB Gerd Hoffmann
` (3 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Gerd Hoffmann @ 2023-01-17 12:16 UTC (permalink / raw)
To: devel
Cc: Jiewen Yao, Oliver Steffen, László Érsek,
Ard Biesheuvel, Pawel Polawski, Jordan Justen, Gerd Hoffmann
Add PlatformGetLowMemoryCB() callback function for use with
PlatformScanE820(). It stores the low memory size in
PlatformInfoHob->LowMemory. This replaces calls to
PlatformScanOrAdd64BitE820Ram() with non-NULL LowMemory.
Write any actions done (setting LowMemory) to the firmware log
with INFO loglevel.
Also change PlatformGetSystemMemorySizeBelow4gb() to likewise set
PlatformInfoHob->LowMemory instead of returning the value. Update
all Callers to the new convention.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
OvmfPkg/Include/Library/PlatformInitLib.h | 3 +-
OvmfPkg/Library/PeilessStartupLib/Hob.c | 3 +-
.../PeilessStartupLib/PeilessStartup.c | 7 +-
OvmfPkg/Library/PlatformInitLib/MemDetect.c | 69 +++++++++++++------
OvmfPkg/Library/PlatformInitLib/Platform.c | 7 +-
OvmfPkg/PlatformPei/MemDetect.c | 3 +-
6 files changed, 59 insertions(+), 33 deletions(-)
diff --git a/OvmfPkg/Include/Library/PlatformInitLib.h b/OvmfPkg/Include/Library/PlatformInitLib.h
index bf6f90a5761c..051b31191194 100644
--- a/OvmfPkg/Include/Library/PlatformInitLib.h
+++ b/OvmfPkg/Include/Library/PlatformInitLib.h
@@ -26,6 +26,7 @@ typedef struct {
BOOLEAN Q35SmramAtDefaultSmbase;
UINT16 Q35TsegMbytes;
+ UINT32 LowMemory;
UINT64 FirstNonAddress;
UINT8 PhysMemAddressWidth;
UINT32 Uc32Base;
@@ -144,7 +145,7 @@ PlatformQemuUc32BaseInitialization (
IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
);
-UINT32
+VOID
EFIAPI
PlatformGetSystemMemorySizeBelow4gb (
IN EFI_HOB_PLATFORM_INFO *PlatformInfoHob
diff --git a/OvmfPkg/Library/PeilessStartupLib/Hob.c b/OvmfPkg/Library/PeilessStartupLib/Hob.c
index 630ce445ebec..318b74c95d8e 100644
--- a/OvmfPkg/Library/PeilessStartupLib/Hob.c
+++ b/OvmfPkg/Library/PeilessStartupLib/Hob.c
@@ -42,7 +42,8 @@ ConstructSecHobList (
ZeroMem (&PlatformInfoHob, sizeof (PlatformInfoHob));
PlatformInfoHob.HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
- LowMemorySize = PlatformGetSystemMemorySizeBelow4gb (&PlatformInfoHob);
+ PlatformGetSystemMemorySizeBelow4gb (&PlatformInfoHob);
+ LowMemorySize = PlatformInfoHob.LowMemory;
ASSERT (LowMemorySize != 0);
LowMemoryStart = FixedPcdGet32 (PcdOvmfDxeMemFvBase) + FixedPcdGet32 (PcdOvmfDxeMemFvSize);
LowMemorySize -= LowMemoryStart;
diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
index 380e71597206..928120d183ba 100644
--- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
+++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
@@ -41,8 +41,7 @@ InitializePlatform (
EFI_HOB_PLATFORM_INFO *PlatformInfoHob
)
{
- UINT32 LowerMemorySize;
- VOID *VariableStore;
+ VOID *VariableStore;
DEBUG ((DEBUG_INFO, "InitializePlatform in Pei-less boot\n"));
PlatformDebugDumpCmos ();
@@ -70,14 +69,14 @@ InitializePlatform (
PlatformInfoHob->PcdCpuBootLogicalProcessorNumber
));
- LowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
+ PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
PlatformQemuUc32BaseInitialization (PlatformInfoHob);
DEBUG ((
DEBUG_INFO,
"Uc32Base = 0x%x, Uc32Size = 0x%x, LowerMemorySize = 0x%x\n",
PlatformInfoHob->Uc32Base,
PlatformInfoHob->Uc32Size,
- LowerMemorySize
+ PlatformInfoHob->LowMemory
));
VariableStore = PlatformReserveEmuVariableNvStore ();
diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index 1ea91f78cefd..dfaa05a1c24f 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -51,18 +51,16 @@ PlatformQemuUc32BaseInitialization (
IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
)
{
- UINT32 LowerMemorySize;
-
if (PlatformInfoHob->HostBridgeDevId == 0xffff /* microvm */) {
return;
}
if (PlatformInfoHob->HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
- LowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
+ PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
ASSERT (PcdGet64 (PcdPciExpressBaseAddress) <= MAX_UINT32);
- ASSERT (PcdGet64 (PcdPciExpressBaseAddress) >= LowerMemorySize);
+ ASSERT (PcdGet64 (PcdPciExpressBaseAddress) >= PlatformInfoHob->LowMemory);
- if (LowerMemorySize <= BASE_2GB) {
+ if (PlatformInfoHob->LowMemory <= BASE_2GB) {
// Newer qemu with gigabyte aligned memory,
// 32-bit pci mmio window is 2G -> 4G then.
PlatformInfoHob->Uc32Base = BASE_2GB;
@@ -92,8 +90,8 @@ PlatformQemuUc32BaseInitialization (
// variable MTRR suffices by truncating the size to a whole power of two,
// while keeping the end affixed to 4GB. This will round the base up.
//
- LowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
- PlatformInfoHob->Uc32Size = GetPowerOfTwo32 ((UINT32)(SIZE_4GB - LowerMemorySize));
+ PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
+ PlatformInfoHob->Uc32Size = GetPowerOfTwo32 ((UINT32)(SIZE_4GB - PlatformInfoHob->LowMemory));
PlatformInfoHob->Uc32Base = (UINT32)(SIZE_4GB - PlatformInfoHob->Uc32Size);
//
// Assuming that LowerMemorySize is at least 1 byte, Uc32Size is at most 2GB.
@@ -101,13 +99,13 @@ PlatformQemuUc32BaseInitialization (
//
ASSERT (PlatformInfoHob->Uc32Base >= BASE_2GB);
- if (PlatformInfoHob->Uc32Base != LowerMemorySize) {
+ if (PlatformInfoHob->Uc32Base != PlatformInfoHob->LowMemory) {
DEBUG ((
DEBUG_VERBOSE,
"%a: rounded UC32 base from 0x%x up to 0x%x, for "
"an UC32 size of 0x%x\n",
__FUNCTION__,
- LowerMemorySize,
+ PlatformInfoHob->LowMemory,
PlatformInfoHob->Uc32Base,
PlatformInfoHob->Uc32Size
));
@@ -279,6 +277,33 @@ PlatformGetFirstNonAddressCB (
}
}
+/**
+ Store the low (below 4G) memory size in
+ PlatformInfoHob->LowMemory
+**/
+STATIC VOID
+PlatformGetLowMemoryCB (
+ IN EFI_E820_ENTRY64 *E820Entry,
+ IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
+ )
+{
+ UINT64 Candidate;
+
+ if (E820Entry->Type != EfiAcpiAddressRangeMemory) {
+ return;
+ }
+
+ Candidate = E820Entry->BaseAddr + E820Entry->Length;
+ if (Candidate >= BASE_4GB) {
+ return;
+ }
+
+ if (PlatformInfoHob->LowMemory < Candidate) {
+ DEBUG ((DEBUG_INFO, "%a: LowMemory=0x%Lx\n", __FUNCTION__, Candidate));
+ PlatformInfoHob->LowMemory = (UINT32)Candidate;
+ }
+}
+
/**
Iterate over the entries in QEMU's fw_cfg E820 RAM map, call the
passed callback for each entry.
@@ -395,14 +420,13 @@ GetHighestSystemMemoryAddressFromPvhMemmap (
return HighestAddress;
}
-UINT32
+VOID
EFIAPI
PlatformGetSystemMemorySizeBelow4gb (
IN EFI_HOB_PLATFORM_INFO *PlatformInfoHob
)
{
EFI_STATUS Status;
- UINT64 LowerMemorySize = 0;
UINT8 Cmos0x34;
UINT8 Cmos0x35;
@@ -410,12 +434,13 @@ PlatformGetSystemMemorySizeBelow4gb (
(CcProbe () != CcGuestTypeIntelTdx))
{
// Get the information from PVH memmap
- return (UINT32)GetHighestSystemMemoryAddressFromPvhMemmap (TRUE);
+ PlatformInfoHob->LowMemory = (UINT32)GetHighestSystemMemoryAddressFromPvhMemmap (TRUE);
+ return;
}
- Status = PlatformScanOrAdd64BitE820Ram (FALSE, &LowerMemorySize, NULL);
- if ((Status == EFI_SUCCESS) && (LowerMemorySize > 0)) {
- return (UINT32)LowerMemorySize;
+ Status = PlatformScanE820 (PlatformGetLowMemoryCB, PlatformInfoHob);
+ if (!EFI_ERROR (Status) && (PlatformInfoHob->LowMemory > 0)) {
+ return;
}
//
@@ -430,7 +455,7 @@ PlatformGetSystemMemorySizeBelow4gb (
Cmos0x34 = (UINT8)PlatformCmosRead8 (0x34);
Cmos0x35 = (UINT8)PlatformCmosRead8 (0x35);
- return (UINT32)(((UINTN)((Cmos0x35 << 8) + Cmos0x34) << 16) + SIZE_16MB);
+ PlatformInfoHob->LowMemory = (UINT32)(((UINTN)((Cmos0x35 << 8) + Cmos0x34) << 16) + SIZE_16MB);
}
STATIC
@@ -966,7 +991,6 @@ PlatformQemuInitializeRam (
IN EFI_HOB_PLATFORM_INFO *PlatformInfoHob
)
{
- UINT64 LowerMemorySize;
UINT64 UpperMemorySize;
MTRR_SETTINGS MtrrSettings;
EFI_STATUS Status;
@@ -976,7 +1000,7 @@ PlatformQemuInitializeRam (
//
// Determine total memory size available
//
- LowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
+ PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
if (PlatformInfoHob->BootMode == BOOT_ON_S3_RESUME) {
//
@@ -1010,14 +1034,14 @@ PlatformQemuInitializeRam (
UINT32 TsegSize;
TsegSize = PlatformInfoHob->Q35TsegMbytes * SIZE_1MB;
- PlatformAddMemoryRangeHob (BASE_1MB, LowerMemorySize - TsegSize);
+ PlatformAddMemoryRangeHob (BASE_1MB, PlatformInfoHob->LowMemory - TsegSize);
PlatformAddReservedMemoryBaseSizeHob (
- LowerMemorySize - TsegSize,
+ PlatformInfoHob->LowMemory - TsegSize,
TsegSize,
TRUE
);
} else {
- PlatformAddMemoryRangeHob (BASE_1MB, LowerMemorySize);
+ PlatformAddMemoryRangeHob (BASE_1MB, PlatformInfoHob->LowMemory);
}
//
@@ -1195,9 +1219,10 @@ PlatformQemuInitializeRamForS3 (
// Make sure the TSEG area that we reported as a reserved memory resource
// cannot be used for reserved memory allocations.
//
+ PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
TsegSize = PlatformInfoHob->Q35TsegMbytes * SIZE_1MB;
BuildMemoryAllocationHob (
- PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob) - TsegSize,
+ PlatformInfoHob->LowMemory - TsegSize,
TsegSize,
EfiReservedMemoryType
);
diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c b/OvmfPkg/Library/PlatformInitLib/Platform.c
index 3e13c5d4b34f..9ab0342fd8c0 100644
--- a/OvmfPkg/Library/PlatformInitLib/Platform.c
+++ b/OvmfPkg/Library/PlatformInitLib/Platform.c
@@ -128,7 +128,6 @@ PlatformMemMapInitialization (
{
UINT64 PciIoBase;
UINT64 PciIoSize;
- UINT32 TopOfLowRam;
UINT64 PciExBarBase;
UINT32 PciBase;
UINT32 PciSize;
@@ -150,7 +149,7 @@ PlatformMemMapInitialization (
return;
}
- TopOfLowRam = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
+ PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
PciExBarBase = 0;
if (PlatformInfoHob->HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
//
@@ -158,11 +157,11 @@ PlatformMemMapInitialization (
// the base of the 32-bit PCI host aperture.
//
PciExBarBase = PcdGet64 (PcdPciExpressBaseAddress);
- ASSERT (TopOfLowRam <= PciExBarBase);
+ ASSERT (PlatformInfoHob->LowMemory <= PciExBarBase);
ASSERT (PciExBarBase <= MAX_UINT32 - SIZE_256MB);
PciBase = (UINT32)(PciExBarBase + SIZE_256MB);
} else {
- ASSERT (TopOfLowRam <= PlatformInfoHob->Uc32Base);
+ ASSERT (PlatformInfoHob->LowMemory <= PlatformInfoHob->Uc32Base);
PciBase = PlatformInfoHob->Uc32Base;
}
diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 3d8375320dcb..41d186986ba8 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -271,7 +271,8 @@ PublishPeiMemory (
UINT32 S3AcpiReservedMemoryBase;
UINT32 S3AcpiReservedMemorySize;
- LowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
+ PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
+ LowerMemorySize = PlatformInfoHob->LowMemory;
if (PlatformInfoHob->SmmSmramRequire) {
//
// TSEG is chipped from the end of low RAM
--
2.39.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB
2023-01-17 12:16 [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts Gerd Hoffmann
2023-01-17 12:16 ` [PATCH v4 1/5] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB Gerd Hoffmann
2023-01-17 12:16 ` [PATCH v4 2/5] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB Gerd Hoffmann
@ 2023-01-17 12:16 ` Gerd Hoffmann
2023-01-17 15:00 ` Laszlo Ersek
2023-01-24 22:33 ` [edk2-devel] " Lendacky, Thomas
2023-01-17 12:16 ` [PATCH v4 4/5] OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB Gerd Hoffmann
` (2 subsequent siblings)
5 siblings, 2 replies; 22+ messages in thread
From: Gerd Hoffmann @ 2023-01-17 12:16 UTC (permalink / raw)
To: devel
Cc: Jiewen Yao, Oliver Steffen, László Érsek,
Ard Biesheuvel, Pawel Polawski, Jordan Justen, 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@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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 4/5] OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB
2023-01-17 12:16 [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts Gerd Hoffmann
` (2 preceding siblings ...)
2023-01-17 12:16 ` [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB Gerd Hoffmann
@ 2023-01-17 12:16 ` Gerd Hoffmann
2023-01-17 15:05 ` Laszlo Ersek
2023-01-17 12:16 ` [PATCH v4 5/5] OvmfPkg/PlatformInitLib: reorder PlatformQemuUc32BaseInitialization Gerd Hoffmann
2023-01-17 16:38 ` [edk2-devel] [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts Ard Biesheuvel
5 siblings, 1 reply; 22+ messages in thread
From: Gerd Hoffmann @ 2023-01-17 12:16 UTC (permalink / raw)
To: devel
Cc: Jiewen Yao, Oliver Steffen, László Érsek,
Ard Biesheuvel, Pawel Polawski, Jordan Justen, Gerd Hoffmann
Add PlatformReservationConflictCB() callback function for use with
PlatformScanE820(). It checks whenever the 64bit PCI MMIO window
overlaps with a reservation from qemu. If so move down the MMIO window
to resolve the conflict.
Write any actions done (moving mmio window) to the firmware log with
INFO loglevel.
This happens on (virtual) AMD machines with 1TB address space,
because the AMD IOMMU uses an address window just below 1TB.
Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4251
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
OvmfPkg/Library/PlatformInitLib/MemDetect.c | 45 +++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index c626fc49cf6c..1ce692e77ecb 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -213,6 +213,50 @@ PlatformAddHobCB (
}
}
+/**
+ Check whenever the 64bit PCI MMIO window overlaps with a reservation
+ from qemu. If so move down the MMIO window to resolve the conflict.
+
+ This happens on (virtual) AMD machines with 1TB address space,
+ because the AMD IOMMU uses an address window just below 1TB.
+**/
+STATIC VOID
+PlatformReservationConflictCB (
+ IN EFI_E820_ENTRY64 *E820Entry,
+ IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
+ )
+{
+ UINT64 IntersectionBase;
+ UINT64 IntersectionEnd;
+ UINT64 NewBase;
+
+ IntersectionBase = MAX (
+ E820Entry->BaseAddr,
+ PlatformInfoHob->PcdPciMmio64Base
+ );
+ IntersectionEnd = MIN (
+ E820Entry->BaseAddr + E820Entry->Length,
+ PlatformInfoHob->PcdPciMmio64Base +
+ PlatformInfoHob->PcdPciMmio64Size
+ );
+
+ if (IntersectionBase >= IntersectionEnd) {
+ return; // no overlap
+ }
+
+ NewBase = E820Entry->BaseAddr - PlatformInfoHob->PcdPciMmio64Size;
+ NewBase = NewBase & ~(PlatformInfoHob->PcdPciMmio64Size - 1);
+
+ DEBUG ((
+ DEBUG_INFO,
+ "%a: move mmio: 0x%Lx => %Lx\n",
+ __FUNCTION__,
+ PlatformInfoHob->PcdPciMmio64Base,
+ NewBase
+ ));
+ PlatformInfoHob->PcdPciMmio64Base = NewBase;
+}
+
/**
Iterate over the entries in QEMU's fw_cfg E820 RAM map, call the
passed callback for each entry.
@@ -650,6 +694,7 @@ PlatformDynamicMmioWindow (
DEBUG ((DEBUG_INFO, "%a: MMIO Space 0x%Lx (%Ld GB)\n", __func__, MmioSpace, RShiftU64 (MmioSpace, 30)));
PlatformInfoHob->PcdPciMmio64Size = MmioSpace;
PlatformInfoHob->PcdPciMmio64Base = AddrSpace - MmioSpace;
+ PlatformScanE820 (PlatformReservationConflictCB, PlatformInfoHob);
} else {
DEBUG ((DEBUG_INFO, "%a: using classic mmio window\n", __func__));
}
--
2.39.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 5/5] OvmfPkg/PlatformInitLib: reorder PlatformQemuUc32BaseInitialization
2023-01-17 12:16 [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts Gerd Hoffmann
` (3 preceding siblings ...)
2023-01-17 12:16 ` [PATCH v4 4/5] OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB Gerd Hoffmann
@ 2023-01-17 12:16 ` Gerd Hoffmann
2023-01-17 14:49 ` Laszlo Ersek
2023-01-17 16:38 ` [edk2-devel] [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts Ard Biesheuvel
5 siblings, 1 reply; 22+ messages in thread
From: Gerd Hoffmann @ 2023-01-17 12:16 UTC (permalink / raw)
To: devel
Cc: Jiewen Yao, Oliver Steffen, László Érsek,
Ard Biesheuvel, Pawel Polawski, Jordan Justen, Gerd Hoffmann
First handle the cases which do not need know the value of
PlatformInfoHob->LowMemory (microvm and cloudhv). Then call
PlatformGetSystemMemorySizeBelow4gb() to get LowMemory. Finally handle
the cases (q35 and pc) which need to look at LowMemory,
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
OvmfPkg/Library/PlatformInitLib/MemDetect.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index 1ce692e77ecb..d6984ee96eb4 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -55,8 +55,15 @@ PlatformQemuUc32BaseInitialization (
return;
}
+ if (PlatformInfoHob->HostBridgeDevId == CLOUDHV_DEVICE_ID) {
+ PlatformInfoHob->Uc32Size = CLOUDHV_MMIO_HOLE_SIZE;
+ PlatformInfoHob->Uc32Base = CLOUDHV_MMIO_HOLE_ADDRESS;
+ return;
+ }
+
+ PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
+
if (PlatformInfoHob->HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
- PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
ASSERT (PcdGet64 (PcdPciExpressBaseAddress) <= MAX_UINT32);
ASSERT (PcdGet64 (PcdPciExpressBaseAddress) >= PlatformInfoHob->LowMemory);
@@ -78,19 +85,12 @@ PlatformQemuUc32BaseInitialization (
return;
}
- if (PlatformInfoHob->HostBridgeDevId == CLOUDHV_DEVICE_ID) {
- PlatformInfoHob->Uc32Size = CLOUDHV_MMIO_HOLE_SIZE;
- PlatformInfoHob->Uc32Base = CLOUDHV_MMIO_HOLE_ADDRESS;
- return;
- }
-
ASSERT (PlatformInfoHob->HostBridgeDevId == INTEL_82441_DEVICE_ID);
//
// On i440fx, start with the [LowerMemorySize, 4GB) range. Make sure one
// variable MTRR suffices by truncating the size to a whole power of two,
// while keeping the end affixed to 4GB. This will round the base up.
//
- PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob);
PlatformInfoHob->Uc32Size = GetPowerOfTwo32 ((UINT32)(SIZE_4GB - PlatformInfoHob->LowMemory));
PlatformInfoHob->Uc32Base = (UINT32)(SIZE_4GB - PlatformInfoHob->Uc32Size);
//
--
2.39.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/5] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB
2023-01-17 12:16 ` [PATCH v4 1/5] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB Gerd Hoffmann
@ 2023-01-17 14:30 ` Laszlo Ersek
2023-01-17 14:46 ` Laszlo Ersek
1 sibling, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2023-01-17 14:30 UTC (permalink / raw)
To: Gerd Hoffmann, devel
Cc: Jiewen Yao, Oliver Steffen, Ard Biesheuvel, Pawel Polawski,
Jordan Justen
On 1/17/23 13:16, Gerd Hoffmann wrote:
> First step replacing the PlatformScanOrAdd64BitE820Ram() function.
>
> Add a PlatformScanE820() function which loops over the e280 entries
> from FwCfg and calls a callback for each of them.
>
> Add a GetFirstNonAddressCB() function which will store the first free
> address (right after the last RAM block) in
> PlatformInfoHob->FirstNonAddress. This replaces calls to
> PlatformScanOrAdd64BitE820Ram() with non-NULL MaxAddress.
>
> Write any actions done (setting FirstNonAddress) to the firmware log
> with INFO loglevel.
>
> Also drop local FirstNonAddress variables and use
> PlatformInfoHob->FirstNonAddress instead everywhere.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> OvmfPkg/Library/PlatformInitLib/MemDetect.c | 115 ++++++++++++++++----
> 1 file changed, 92 insertions(+), 23 deletions(-)
>
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index 882805269b3e..1ea91f78cefd 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -251,6 +251,83 @@ PlatformScanOrAdd64BitE820Ram (
> return EFI_SUCCESS;
> }
>
> +typedef VOID (*E820_SCAN_CALLBACK) (
> + EFI_E820_ENTRY64 *E820Entry,
> + EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> + );
> +
> +/**
> + Store first address not used by e820 RAM entries in
> + PlatformInfoHob->FirstNonAddress
> +**/
> +VOID
> +PlatformGetFirstNonAddressCB (
> + IN EFI_E820_ENTRY64 *E820Entry,
> + IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> + )
> +{
> + UINT64 Candidate;
> +
> + if (E820Entry->Type != EfiAcpiAddressRangeMemory) {
> + return;
> + }
> +
> + Candidate = E820Entry->BaseAddr + E820Entry->Length;
> + if (PlatformInfoHob->FirstNonAddress < Candidate) {
> + DEBUG ((DEBUG_INFO, "%a: FirstNonAddress=0x%Lx\n", __FUNCTION__, Candidate));
> + PlatformInfoHob->FirstNonAddress = Candidate;
> + }
> +}
> +
> +/**
> + Iterate over the entries in QEMU's fw_cfg E820 RAM map, call the
> + passed callback for each entry.
> +
> + @param[in] Callback The callback function to be called.
> +
> + @param[in out] PlatformInfoHob PlatformInfo struct which is passed
> + through to the callback.
> +
> + @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
> +PlatformScanE820 (
> + IN E820_SCAN_CALLBACK Callback,
> + IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> + )
> +{
> + 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;
> + }
> +
> + QemuFwCfgSelectItem (FwCfgItem);
> + for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) {
> + QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry);
> + Callback (&E820Entry, PlatformInfoHob);
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
> Returns PVH memmap
>
> @@ -384,23 +461,17 @@ PlatformGetSystemMemorySizeAbove4gb (
> Return the highest address that DXE could possibly use, plus one.
> **/
> STATIC
> -UINT64
> +VOID
> PlatformGetFirstNonAddress (
> IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> )
> {
> - UINT64 FirstNonAddress;
> UINT32 FwCfgPciMmio64Mb;
> EFI_STATUS Status;
> FIRMWARE_CONFIG_ITEM FwCfgItem;
> UINTN FwCfgSize;
> UINT64 HotPlugMemoryEnd;
>
> - //
> - // set FirstNonAddress to suppress incorrect compiler/analyzer warnings
> - //
> - FirstNonAddress = 0;
> -
> //
> // If QEMU presents an E820 map, then get the highest exclusive >=4GB RAM
> // address from it. This can express an address >= 4GB+1TB.
> @@ -408,9 +479,10 @@ PlatformGetFirstNonAddress (
> // Otherwise, get the flat size of the memory above 4GB from the CMOS (which
> // can only express a size smaller than 1TB), and add it to 4GB.
> //
> - Status = PlatformScanOrAdd64BitE820Ram (FALSE, NULL, &FirstNonAddress);
> + PlatformInfoHob->FirstNonAddress = BASE_4GB;
> + Status = PlatformScanE820 (PlatformGetFirstNonAddressCB, PlatformInfoHob);
> if (EFI_ERROR (Status)) {
> - FirstNonAddress = BASE_4GB + PlatformGetSystemMemorySizeAbove4gb ();
> + PlatformInfoHob->FirstNonAddress = BASE_4GB + PlatformGetSystemMemorySizeAbove4gb ();
> }
>
> //
> @@ -420,7 +492,7 @@ PlatformGetFirstNonAddress (
> //
> #ifdef MDE_CPU_IA32
> if (!FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
> - return FirstNonAddress;
> + return;
> }
>
> #endif
> @@ -473,7 +545,7 @@ PlatformGetFirstNonAddress (
> // determines the highest address plus one. The memory hotplug area (see
> // below) plays no role for the firmware in this case.
> //
> - return FirstNonAddress;
> + return;
> }
>
> //
> @@ -497,15 +569,15 @@ PlatformGetFirstNonAddress (
> HotPlugMemoryEnd
> ));
>
> - ASSERT (HotPlugMemoryEnd >= FirstNonAddress);
> - FirstNonAddress = HotPlugMemoryEnd;
> + ASSERT (HotPlugMemoryEnd >= PlatformInfoHob->FirstNonAddress);
> + PlatformInfoHob->FirstNonAddress = HotPlugMemoryEnd;
> }
>
> //
> // SeaBIOS aligns both boundaries of the 64-bit PCI host aperture to 1GB, so
> // that the host can map it with 1GB hugepages. Follow suit.
> //
> - PlatformInfoHob->PcdPciMmio64Base = ALIGN_VALUE (FirstNonAddress, (UINT64)SIZE_1GB);
> + PlatformInfoHob->PcdPciMmio64Base = ALIGN_VALUE (PlatformInfoHob->FirstNonAddress, (UINT64)SIZE_1GB);
> PlatformInfoHob->PcdPciMmio64Size = ALIGN_VALUE (PlatformInfoHob->PcdPciMmio64Size, (UINT64)SIZE_1GB);
>
> //
> @@ -519,8 +591,8 @@ PlatformGetFirstNonAddress (
> //
> // The useful address space ends with the 64-bit PCI host aperture.
> //
> - FirstNonAddress = PlatformInfoHob->PcdPciMmio64Base + PlatformInfoHob->PcdPciMmio64Size;
> - return FirstNonAddress;
> + PlatformInfoHob->FirstNonAddress = PlatformInfoHob->PcdPciMmio64Base + PlatformInfoHob->PcdPciMmio64Size;
> + return;
> }
>
> /*
> @@ -781,7 +853,6 @@ PlatformAddressWidthInitialization (
> IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> )
> {
> - UINT64 FirstNonAddress;
> UINT8 PhysMemAddressWidth;
> EFI_STATUS Status;
>
> @@ -794,7 +865,7 @@ PlatformAddressWidthInitialization (
> // First scan host-provided hardware information to assess if the address
> // space is already known. If so, guest must use those values.
> //
> - Status = PlatformScanHostProvided64BitPciMmioEnd (&FirstNonAddress);
> + Status = PlatformScanHostProvided64BitPciMmioEnd (&PlatformInfoHob->FirstNonAddress);
>
> if (EFI_ERROR (Status)) {
> //
> @@ -806,13 +877,12 @@ PlatformAddressWidthInitialization (
> // The DXL IPL keys off of the physical address bits advertized in the CPU
> // HOB. To conserve memory, we calculate the minimum address width here.
> //
> - FirstNonAddress = PlatformGetFirstNonAddress (PlatformInfoHob);
> + PlatformGetFirstNonAddress (PlatformInfoHob);
> }
>
> PlatformAddressWidthFromCpuid (PlatformInfoHob, TRUE);
> if (PlatformInfoHob->PhysMemAddressWidth != 0) {
> // physical address width is known
> - PlatformInfoHob->FirstNonAddress = FirstNonAddress;
> PlatformDynamicMmioWindow (PlatformInfoHob);
> return;
> }
> @@ -823,13 +893,13 @@ PlatformAddressWidthInitialization (
> // -> try be conservstibe to stay below the guaranteed minimum of
> // 36 phys bits (aka 64 GB).
> //
> - PhysMemAddressWidth = (UINT8)HighBitSet64 (FirstNonAddress);
> + PhysMemAddressWidth = (UINT8)HighBitSet64 (PlatformInfoHob->FirstNonAddress);
>
> //
> // If FirstNonAddress is not an integral power of two, then we need an
> // additional bit.
> //
> - if ((FirstNonAddress & (FirstNonAddress - 1)) != 0) {
> + if ((PlatformInfoHob->FirstNonAddress & (PlatformInfoHob->FirstNonAddress - 1)) != 0) {
> ++PhysMemAddressWidth;
> }
>
> @@ -857,7 +927,6 @@ PlatformAddressWidthInitialization (
> ASSERT (PhysMemAddressWidth <= 48);
> #endif
>
> - PlatformInfoHob->FirstNonAddress = FirstNonAddress;
> PlatformInfoHob->PhysMemAddressWidth = PhysMemAddressWidth;
> }
>
(I'm no longer subscribed to the list, but I got a direct copy of this,
and I decided to look at it.)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/5] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB
2023-01-17 12:16 ` [PATCH v4 2/5] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB Gerd Hoffmann
@ 2023-01-17 14:45 ` Laszlo Ersek
0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2023-01-17 14:45 UTC (permalink / raw)
To: Gerd Hoffmann, devel
Cc: Jiewen Yao, Oliver Steffen, Ard Biesheuvel, Pawel Polawski,
Jordan Justen
On 1/17/23 13:16, Gerd Hoffmann wrote:
> Add PlatformGetLowMemoryCB() callback function for use with
> PlatformScanE820(). It stores the low memory size in
> PlatformInfoHob->LowMemory. This replaces calls to
> PlatformScanOrAdd64BitE820Ram() with non-NULL LowMemory.
>
> Write any actions done (setting LowMemory) to the firmware log
> with INFO loglevel.
>
> Also change PlatformGetSystemMemorySizeBelow4gb() to likewise set
> PlatformInfoHob->LowMemory instead of returning the value. Update
> all Callers to the new convention.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> OvmfPkg/Include/Library/PlatformInitLib.h | 3 +-
> OvmfPkg/Library/PeilessStartupLib/Hob.c | 3 +-
> .../PeilessStartupLib/PeilessStartup.c | 7 +-
> OvmfPkg/Library/PlatformInitLib/MemDetect.c | 69 +++++++++++++------
> OvmfPkg/Library/PlatformInitLib/Platform.c | 7 +-
> OvmfPkg/PlatformPei/MemDetect.c | 3 +-
> 6 files changed, 59 insertions(+), 33 deletions(-)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/5] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB
2023-01-17 12:16 ` [PATCH v4 1/5] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB Gerd Hoffmann
2023-01-17 14:30 ` Laszlo Ersek
@ 2023-01-17 14:46 ` Laszlo Ersek
2023-01-17 14:48 ` [edk2-devel] " Ard Biesheuvel
1 sibling, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2023-01-17 14:46 UTC (permalink / raw)
To: Gerd Hoffmann, devel
Cc: Jiewen Yao, Oliver Steffen, Ard Biesheuvel, Pawel Polawski,
Jordan Justen
On 1/17/23 13:16, Gerd Hoffmann wrote:
> +/**
> + Store first address not used by e820 RAM entries in
> + PlatformInfoHob->FirstNonAddress
> +**/
> +VOID
> +PlatformGetFirstNonAddressCB (
> + IN EFI_E820_ENTRY64 *E820Entry,
> + IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> + )
> +{
> + UINT64 Candidate;
> +
> + if (E820Entry->Type != EfiAcpiAddressRangeMemory) {
> + return;
> + }
> +
> + Candidate = E820Entry->BaseAddr + E820Entry->Length;
> + if (PlatformInfoHob->FirstNonAddress < Candidate) {
> + DEBUG ((DEBUG_INFO, "%a: FirstNonAddress=0x%Lx\n", __FUNCTION__, Candidate));
> + PlatformInfoHob->FirstNonAddress = Candidate;
> + }
> +}
This could have been made STATIC (like the other three callbacks), but
it's not important.
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 1/5] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB
2023-01-17 14:46 ` Laszlo Ersek
@ 2023-01-17 14:48 ` Ard Biesheuvel
0 siblings, 0 replies; 22+ messages in thread
From: Ard Biesheuvel @ 2023-01-17 14:48 UTC (permalink / raw)
To: devel, lersek
Cc: Gerd Hoffmann, Jiewen Yao, Oliver Steffen, Ard Biesheuvel,
Pawel Polawski, Jordan Justen
On Tue, 17 Jan 2023 at 15:47, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 1/17/23 13:16, Gerd Hoffmann wrote:
>
> > +/**
> > + Store first address not used by e820 RAM entries in
> > + PlatformInfoHob->FirstNonAddress
> > +**/
> > +VOID
> > +PlatformGetFirstNonAddressCB (
> > + IN EFI_E820_ENTRY64 *E820Entry,
> > + IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> > + )
> > +{
> > + UINT64 Candidate;
> > +
> > + if (E820Entry->Type != EfiAcpiAddressRangeMemory) {
> > + return;
> > + }
> > +
> > + Candidate = E820Entry->BaseAddr + E820Entry->Length;
> > + if (PlatformInfoHob->FirstNonAddress < Candidate) {
> > + DEBUG ((DEBUG_INFO, "%a: FirstNonAddress=0x%Lx\n", __FUNCTION__, Candidate));
> > + PlatformInfoHob->FirstNonAddress = Candidate;
> > + }
> > +}
>
> This could have been made STATIC (like the other three callbacks), but
> it's not important.
>
I can fix that up at merge time
Thanks,
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 5/5] OvmfPkg/PlatformInitLib: reorder PlatformQemuUc32BaseInitialization
2023-01-17 12:16 ` [PATCH v4 5/5] OvmfPkg/PlatformInitLib: reorder PlatformQemuUc32BaseInitialization Gerd Hoffmann
@ 2023-01-17 14:49 ` Laszlo Ersek
0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2023-01-17 14:49 UTC (permalink / raw)
To: Gerd Hoffmann, devel
Cc: Jiewen Yao, Oliver Steffen, Ard Biesheuvel, Pawel Polawski,
Jordan Justen
On 1/17/23 13:16, Gerd Hoffmann wrote:
> First handle the cases which do not need know the value of
> PlatformInfoHob->LowMemory (microvm and cloudhv). Then call
> PlatformGetSystemMemorySizeBelow4gb() to get LowMemory. Finally handle
> the cases (q35 and pc) which need to look at LowMemory,
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> OvmfPkg/Library/PlatformInitLib/MemDetect.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB
2023-01-17 12:16 ` [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB Gerd Hoffmann
@ 2023-01-17 15:00 ` Laszlo Ersek
2023-01-17 15:06 ` Ard Biesheuvel
2023-01-17 16:04 ` Ard Biesheuvel
2023-01-24 22:33 ` [edk2-devel] " Lendacky, Thomas
1 sibling, 2 replies; 22+ messages in thread
From: Laszlo Ersek @ 2023-01-17 15:00 UTC (permalink / raw)
To: Gerd Hoffmann, devel
Cc: Jiewen Yao, Oliver Steffen, Ard Biesheuvel, Pawel Polawski,
Jordan Justen
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>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/5] OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB
2023-01-17 12:16 ` [PATCH v4 4/5] OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB Gerd Hoffmann
@ 2023-01-17 15:05 ` Laszlo Ersek
0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2023-01-17 15:05 UTC (permalink / raw)
To: Gerd Hoffmann, devel
Cc: Jiewen Yao, Oliver Steffen, Ard Biesheuvel, Pawel Polawski,
Jordan Justen
On 1/17/23 13:16, Gerd Hoffmann wrote:
> Add PlatformReservationConflictCB() callback function for use with
> PlatformScanE820(). It checks whenever the 64bit PCI MMIO window
> overlaps with a reservation from qemu. If so move down the MMIO window
> to resolve the conflict.
>
> Write any actions done (moving mmio window) to the firmware log with
> INFO loglevel.
>
> This happens on (virtual) AMD machines with 1TB address space,
> because the AMD IOMMU uses an address window just below 1TB.
>
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4251
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> OvmfPkg/Library/PlatformInitLib/MemDetect.c | 45 +++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index c626fc49cf6c..1ce692e77ecb 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -213,6 +213,50 @@ PlatformAddHobCB (
> }
> }
>
> +/**
> + Check whenever the 64bit PCI MMIO window overlaps with a reservation
> + from qemu. If so move down the MMIO window to resolve the conflict.
> +
> + This happens on (virtual) AMD machines with 1TB address space,
> + because the AMD IOMMU uses an address window just below 1TB.
> +**/
> +STATIC VOID
> +PlatformReservationConflictCB (
> + IN EFI_E820_ENTRY64 *E820Entry,
> + IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> + )
> +{
> + UINT64 IntersectionBase;
> + UINT64 IntersectionEnd;
> + UINT64 NewBase;
> +
> + IntersectionBase = MAX (
> + E820Entry->BaseAddr,
> + PlatformInfoHob->PcdPciMmio64Base
> + );
> + IntersectionEnd = MIN (
> + E820Entry->BaseAddr + E820Entry->Length,
> + PlatformInfoHob->PcdPciMmio64Base +
> + PlatformInfoHob->PcdPciMmio64Size
> + );
> +
> + if (IntersectionBase >= IntersectionEnd) {
> + return; // no overlap
> + }
> +
> + NewBase = E820Entry->BaseAddr - PlatformInfoHob->PcdPciMmio64Size;
> + NewBase = NewBase & ~(PlatformInfoHob->PcdPciMmio64Size - 1);
> +
> + DEBUG ((
> + DEBUG_INFO,
> + "%a: move mmio: 0x%Lx => %Lx\n",
> + __FUNCTION__,
> + PlatformInfoHob->PcdPciMmio64Base,
> + NewBase
> + ));
> + PlatformInfoHob->PcdPciMmio64Base = NewBase;
> +}
> +
> /**
> Iterate over the entries in QEMU's fw_cfg E820 RAM map, call the
> passed callback for each entry.
> @@ -650,6 +694,7 @@ PlatformDynamicMmioWindow (
> DEBUG ((DEBUG_INFO, "%a: MMIO Space 0x%Lx (%Ld GB)\n", __func__, MmioSpace, RShiftU64 (MmioSpace, 30)));
> PlatformInfoHob->PcdPciMmio64Size = MmioSpace;
> PlatformInfoHob->PcdPciMmio64Base = AddrSpace - MmioSpace;
> + PlatformScanE820 (PlatformReservationConflictCB, PlatformInfoHob);
> } else {
> DEBUG ((DEBUG_INFO, "%a: using classic mmio window\n", __func__));
> }
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB
2023-01-17 15:00 ` Laszlo Ersek
@ 2023-01-17 15:06 ` Ard Biesheuvel
2023-01-17 16:04 ` Ard Biesheuvel
1 sibling, 0 replies; 22+ messages in thread
From: Ard Biesheuvel @ 2023-01-17 15:06 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Gerd Hoffmann, devel, Jiewen Yao, Oliver Steffen, Ard Biesheuvel,
Pawel Polawski, Jordan Justen
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB
2023-01-17 15:00 ` Laszlo Ersek
2023-01-17 15:06 ` Ard Biesheuvel
@ 2023-01-17 16:04 ` Ard Biesheuvel
1 sibling, 0 replies; 22+ messages in thread
From: Ard Biesheuvel @ 2023-01-17 16:04 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Gerd Hoffmann, devel, Jiewen Yao, Oliver Steffen, Ard Biesheuvel,
Pawel Polawski, Jordan Justen
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 ....
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts
2023-01-17 12:16 [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts Gerd Hoffmann
` (4 preceding siblings ...)
2023-01-17 12:16 ` [PATCH v4 5/5] OvmfPkg/PlatformInitLib: reorder PlatformQemuUc32BaseInitialization Gerd Hoffmann
@ 2023-01-17 16:38 ` Ard Biesheuvel
5 siblings, 0 replies; 22+ messages in thread
From: Ard Biesheuvel @ 2023-01-17 16:38 UTC (permalink / raw)
To: devel, kraxel
Cc: Jiewen Yao, Oliver Steffen, László Érsek,
Ard Biesheuvel, Pawel Polawski, Jordan Justen
On Tue, 17 Jan 2023 at 13:16, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> v4:
> - made mmio window move a bit more robust.
> - dropped patches for moving MMCONFIG, they'll come as separate patch
> series later.
>
> v3:
> - Add / fix comments, add notes to commit messages.
> - Make functions static.
> - Logging tweaks.
> - Fix windows compiler warnings.
> - Add patches (5,6,7) moving MMCONFIG to 0xe0000000, simplifying code
> and reducing differences between 'pc' and 'q35' along the way.
> Eventually we want split them into a separate series, but some of
> this was discussed in v2 review, so I just appended them here for
> now.
> v2:
> - split up PlatformScanOrAdd64BitE820Ram() into scan function with
> callbacks, store results in PlatformInfoHob struct.
>
> Gerd Hoffmann (5):
> OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB
> OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB
> OvmfPkg/PlatformInitLib: Add PlatformAddHobCB
> OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB
> OvmfPkg/PlatformInitLib: reorder PlatformQemuUc32BaseInitialization
>
Merged now, thanks,
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB
2023-01-17 12:16 ` [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB Gerd Hoffmann
2023-01-17 15:00 ` Laszlo Ersek
@ 2023-01-24 22:33 ` Lendacky, Thomas
2023-01-25 9:11 ` Gerd Hoffmann
1 sibling, 1 reply; 22+ messages in thread
From: Lendacky, Thomas @ 2023-01-24 22:33 UTC (permalink / raw)
To: devel, kraxel
Cc: Jiewen Yao, Oliver Steffen, László Érsek,
Ard Biesheuvel, Pawel Polawski, Jordan Justen
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) {
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB
2023-01-24 22:33 ` [edk2-devel] " Lendacky, Thomas
@ 2023-01-25 9:11 ` Gerd Hoffmann
2023-01-25 15:35 ` Lendacky, Thomas
0 siblings, 1 reply; 22+ messages in thread
From: Gerd Hoffmann @ 2023-01-25 9:11 UTC (permalink / raw)
To: Tom Lendacky
Cc: devel, Jiewen Yao, Oliver Steffen, László Érsek,
Ard Biesheuvel, Pawel Polawski, Jordan Justen
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB
2023-01-25 9:11 ` Gerd Hoffmann
@ 2023-01-25 15:35 ` Lendacky, Thomas
2023-01-25 16:32 ` Lendacky, Thomas
2023-01-30 15:23 ` Laszlo Ersek
0 siblings, 2 replies; 22+ messages in thread
From: Lendacky, Thomas @ 2023-01-25 15:35 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: devel, Jiewen Yao, Oliver Steffen, László Érsek,
Ard Biesheuvel, Pawel Polawski, Jordan Justen
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
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB
2023-01-25 15:35 ` Lendacky, Thomas
@ 2023-01-25 16:32 ` Lendacky, Thomas
2023-01-30 15:23 ` Laszlo Ersek
1 sibling, 0 replies; 22+ messages in thread
From: Lendacky, Thomas @ 2023-01-25 16:32 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: devel, Jiewen Yao, Oliver Steffen, László Érsek,
Ard Biesheuvel, Pawel Polawski, Jordan Justen
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
>>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB
2023-01-25 15:35 ` Lendacky, Thomas
2023-01-25 16:32 ` Lendacky, Thomas
@ 2023-01-30 15:23 ` Laszlo Ersek
2023-01-30 15:24 ` Laszlo Ersek
1 sibling, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2023-01-30 15:23 UTC (permalink / raw)
To: Tom Lendacky, Gerd Hoffmann
Cc: devel, Jiewen Yao, Oliver Steffen, Ard Biesheuvel, Pawel Polawski,
Jordan Justen
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB
2023-01-30 15:23 ` Laszlo Ersek
@ 2023-01-30 15:24 ` Laszlo Ersek
0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2023-01-30 15:24 UTC (permalink / raw)
To: Tom Lendacky, Gerd Hoffmann
Cc: devel, Jiewen Yao, Oliver Steffen, Ard Biesheuvel, Pawel Polawski,
Jordan Justen
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
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2023-01-30 15:24 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-17 12:16 [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts Gerd Hoffmann
2023-01-17 12:16 ` [PATCH v4 1/5] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB Gerd Hoffmann
2023-01-17 14:30 ` Laszlo Ersek
2023-01-17 14:46 ` Laszlo Ersek
2023-01-17 14:48 ` [edk2-devel] " Ard Biesheuvel
2023-01-17 12:16 ` [PATCH v4 2/5] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB Gerd Hoffmann
2023-01-17 14:45 ` Laszlo Ersek
2023-01-17 12:16 ` [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB Gerd Hoffmann
2023-01-17 15:00 ` Laszlo Ersek
2023-01-17 15:06 ` Ard Biesheuvel
2023-01-17 16:04 ` Ard Biesheuvel
2023-01-24 22:33 ` [edk2-devel] " Lendacky, Thomas
2023-01-25 9:11 ` Gerd Hoffmann
2023-01-25 15:35 ` Lendacky, Thomas
2023-01-25 16:32 ` Lendacky, Thomas
2023-01-30 15:23 ` Laszlo Ersek
2023-01-30 15:24 ` Laszlo Ersek
2023-01-17 12:16 ` [PATCH v4 4/5] OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB Gerd Hoffmann
2023-01-17 15:05 ` Laszlo Ersek
2023-01-17 12:16 ` [PATCH v4 5/5] OvmfPkg/PlatformInitLib: reorder PlatformQemuUc32BaseInitialization Gerd Hoffmann
2023-01-17 14:49 ` Laszlo Ersek
2023-01-17 16:38 ` [edk2-devel] [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox