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