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

Mon Jan 30 15:23:35 UTC 2023

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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-