Re: [edk2-devel] [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB

["Laszlo Ersek" <lersek@redhat.com>]

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