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