From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.109159.1673369620333103533 for ; Tue, 10 Jan 2023 08:53:40 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=hFAz2uDY; spf=pass (domain: redhat.com, ip: 170.10.129.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673369618; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=s7cofS66pDm2rbp+GyIacrrFHZd/58AIO2fblUQot3M=; b=hFAz2uDYXcthgnjSmkL4EBNuONgulB9yDsk8TQhyaqP1u+MP8dbIeFuq1v6MtJOxkXNOw1 8Elyh/UpVg/HuhIXcPJedx8ZKfJGSdctwVF+Q28UAeZqdJkc+CsYN+XSvMTkjI9vXLYxXE 0Tf2zvIP3nBhD8nHQ/jTGakWpjqQH3k= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-635-sxkMHWAKM0e2fMk9ozkU_A-1; Tue, 10 Jan 2023 11:53:33 -0500 X-MC-Unique: sxkMHWAKM0e2fMk9ozkU_A-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 31DEB1C0878F; Tue, 10 Jan 2023 16:53:33 +0000 (UTC) Received: from [10.39.192.22] (unknown [10.39.192.22]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A2AD1C16031; Tue, 10 Jan 2023 16:53:31 +0000 (UTC) Message-ID: <043b03d6-0a6b-c533-255b-24a7805d5cca@redhat.com> Date: Tue, 10 Jan 2023 17:53:30 +0100 MIME-Version: 1.0 Subject: Re: [PATCH v2 2/4] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB To: Gerd Hoffmann , devel@edk2.groups.io Cc: Pawel Polawski , Jiewen Yao , Oliver Steffen , Jordan Justen , Ard Biesheuvel References: <20230110082123.159521-1-kraxel@redhat.com> <20230110082123.159521-3-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20230110082123.159521-3-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Quoting the hunks out of order: On 1/10/23 09:21, 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. > > 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 > --- > 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 OK. > diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c > index a2a4dc043f2e..63329c4e796a 100644 > --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c > +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c > @@ -279,6 +277,33 @@ PlatformGetFirstNonAddressCB ( > } > } > > +/** > + Store the low (below 4G) memory size in > + PlatformInfoHob->LowMemory > +**/ > +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 = Candidate; > + } > +} > + > /** > Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map, call the > passed callback for each entry. (1) This looks like a faithful conversion / extraction, but I'd vaguely noticed something earlier, in the original code. Namely, when the exclusive end of the range is exactly 4GB, that should still qualify as low memory, shouldn't it? Not that it matters in practice, because just below 4GB, we'll never ever have RAM on QEMU (pc or q35 -- I think even microvm, too). But, for clarity, changing the limit condition as a separate patch (afterwards) might make sense. For now, this conversion is faithful. > @@ -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 = GetHighestSystemMemoryAddressFromPvhMemmap (TRUE); > + return; > } OK, so the way this function looks now is new to me. :) (2) Here you are converting a UINT64 from GetHighestSystemMemoryAddressFromPvhMemmap() to UINT32; I think that might trip up some MSVC compilers. I suggest preserving the cast. > > - Status = PlatformScanOrAdd64BitE820Ram (FALSE, &LowerMemorySize, NULL); > - if ((Status == EFI_SUCCESS) && (LowerMemorySize > 0)) { > - return (UINT32)LowerMemorySize; > + Status = PlatformScanE820 (PlatformGetLowMemoryCB, PlatformInfoHob); (3) Similar comment as under the previous patch: please set PlatformInfoHob->LowMemory = 0; before calling PlatformScanE820(), to preserve if (LowMemory != NULL) { *LowMemory = 0; } from PlatformScanOrAdd64BitE820Ram(). (I realize the platform info HOB could be zero-filled upon allocation -- but then at least for consistency with the 4GB+ search initialization, a comment could be justified.) > + if ((Status == EFI_SUCCESS) && (PlatformInfoHob->LowMemory > 0)) { > + return; > } (4) Side comment (for the future): Status == EFI_SUCCESS is more idiomatically written in edk2 as !EFI_ERROR (Status) > > // > @@ -430,7 +455,7 @@ PlatformGetSystemMemorySizeBelow4gb ( > Cmos0x34 = (UINT8)PlatformCmosRead8 (0x34); > Cmos0x35 = (UINT8)PlatformCmosRead8 (0x35); > > - return (UINT32)(((UINTN)((Cmos0x35 << 8) + Cmos0x34) << 16) + SIZE_16MB); > + PlatformInfoHob->LowMemory = ((((UINTN)Cmos0x35 << 8) + Cmos0x34) << 16) + SIZE_16MB; > } > (5) The UINT32 cast has been dropped; if UINTN is UINT64, this might again trigger a truncation warning from MSVC. (6) There is no need to change the scope that the original UINTN cast applies to. Pre-patch, the UINTN cast is bound to the following sub-expression: ((Cmos0x35 << 8) + Cmos0x34) here the variables are UINTN8, so they are promoted to INT32. The maximum mathematical value is 0xFFFF here, having with in INT32 is safe. We then cast it to UINTN (= at least UINT32) and then shift it further left by 16 bits (max: 0xFFFF_0000). The max value we may get after adding 16M is then 0x1_00FF_0000, which is above 4GB; it's up to QEMU not to provide such CMOS content then. Either way, there's no risk of INT32 overflow pre-patch. The post-patch code is certainly *easier to read*, so I don't have anything against re-binding the UINTN cast; but it should not be hidden in this patch. It makes it harder to verify the code refactoring. So anyway, what I need to remember from this point onward is that *each* call to PlatformGetSystemMemorySizeBelow4gb() doesn't just fetch and return a value, but *overwrites* "PlatformInfoHob->LowMemory". OK, let's continue with the callers of PlatformGetSystemMemorySizeBelow4gb() -- again, quoting hunks out of order: > 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 So I really like how small this hunk is, and I wondered why it differed so much from the rest, where you removed the local variables. I understand now: because PublishPeiMemory() actually modifies "LowerMemorySize" in multiple steps. OK then, I see both points; here we need to keep "LowerMemorySize", because we can't modify the platform info HOB; but in the rest of the hunks, it's better if we just remove the useless locals. OK. > diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c > index a2a4dc043f2e..63329c4e796a 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; OK. > @@ -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)); (7) Request for a *separate* follow-up patch: Commit 2a0bd3bffc80 ("OvmfPkg/PlatformInitLib: q35 mtrr setup fix", 2022-09-28) changed the structure of PlatformQemuUc32BaseInitialization() to the following one: - microvm: do nothing, just return - q35: depend on LowerMemorySize (change from 2a0bd3bffc80) - cloudhv: constant assignments, then return - i440fx: depend on LowerMemorySize Therefore we now have to branches (an explicit q35 branch and a "default" or "remaining" i440fx branch) that fetch LowerMemorySize. That code duplication is causing some churn for the present patch. I suggest reordering the branches as follows: - microvm: do nothing, just return - cloudhv: constant assignments, then return - grab LowerMemorySize -- commonly needed for the rest! - handle q35 - handle i440fx as default / fallback. This will centralize the LowerMemorySize fetching. > + 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 > )); OK. > STATIC > @@ -965,7 +990,6 @@ PlatformQemuInitializeRam ( > IN EFI_HOB_PLATFORM_INFO *PlatformInfoHob > ) > { > - UINT64 LowerMemorySize; > UINT64 UpperMemorySize; > MTRR_SETTINGS MtrrSettings; > EFI_STATUS Status; > @@ -975,7 +999,7 @@ PlatformQemuInitializeRam ( > // > // Determine total memory size available > // > - LowerMemorySize = PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob); > + PlatformGetSystemMemorySizeBelow4gb (PlatformInfoHob); > > if (PlatformInfoHob->BootMode == BOOT_ON_S3_RESUME) { > // > @@ -1009,14 +1033,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); > } > > // OK. I think I used UINT64 here because these HOB-adding functions take 64-bit params. But UINT32 should work fine too. (8) Note that a bit lower, we have a comment: // // We'd like to keep the following ranges uncached: // - [640 KB, 1 MB) // - [LowerMemorySize, 4 GB) // The "LowerMemorySize" reference in this commit is actually my fault; it's an oversight / omission from commit 49edde15230a ("OvmfPkg/PlatformPei: set 32-bit UC area at PciBase / PciExBarBase (pc/q35)", 2019-06-03). The comment should now say "PlatformInfoHob->Uc32Base". If you can submit a separate patch for that, that would be great; it's not too important though. (9) BTW, still regarding commit 2a0bd3bffc80 ("OvmfPkg/PlatformInitLib: q35 mtrr setup fix", 2022-09-28) -- does the following code comment still apply? // // Set the memory range from the start of the 32-bit MMIO area (32-bit PCI // MMIO aperture on i440fx, PCIEXBAR on q35) to 4GB as uncacheable. // Because, in case the new branch introduced by commit 2a0bd3bffc80 takes effect (namely, "Uc32Base = BASE_2GB"), I'm not sure where the PCIEXBAR starts, and then the above comment may no longer hold. > @@ -1194,9 +1218,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 > ); OK. > diff --git a/OvmfPkg/Library/PeilessStartupLib/Hob.c b/OvmfPkg/Library/PeilessStartupLib/Hob.c > index 630ce445ebec..784a8ba194de 100644 > --- a/OvmfPkg/Library/PeilessStartupLib/Hob.c > +++ b/OvmfPkg/Library/PeilessStartupLib/Hob.c > @@ -41,8 +41,9 @@ ConstructSecHobList ( > EFI_HOB_PLATFORM_INFO PlatformInfoHob; > > ZeroMem (&PlatformInfoHob, sizeof (PlatformInfoHob)); > + PlatformGetSystemMemorySizeBelow4gb (&PlatformInfoHob); > PlatformInfoHob.HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID); > - LowMemorySize = PlatformGetSystemMemorySizeBelow4gb (&PlatformInfoHob); > + LowMemorySize = PlatformInfoHob.LowMemory; > ASSERT (LowMemorySize != 0); > LowMemoryStart = FixedPcdGet32 (PcdOvmfDxeMemFvBase) + FixedPcdGet32 (PcdOvmfDxeMemFvSize); > LowMemorySize -= LowMemoryStart; (10) I think this PlatformGetSystemMemorySizeBelow4gb() call is not placed correctly. PlatformGetSystemMemorySizeBelow4gb() depends on "HostBridgeDevId", but this hunk reorders the setting of "HostBridgeDevId" against the call to PlatformGetSystemMemorySizeBelow4gb(). > 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 (); Seems OK. > 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); This change looks OK, but, similarly to my question (9): (11) Is the following comment still up to date: // // The MMCONFIG area is expected to fall between the top of low RAM and // the base of the 32-bit PCI host aperture. // with regard to the new branch introduced by commit 2a0bd3bffc80 ("OvmfPkg/PlatformInitLib: q35 mtrr setup fix", 2022-09-28)? > } else { > - ASSERT (TopOfLowRam <= PlatformInfoHob->Uc32Base); > + ASSERT (PlatformInfoHob->LowMemory <= PlatformInfoHob->Uc32Base); > PciBase = PlatformInfoHob->Uc32Base; > } > OK. Thanks, Laszlo