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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 1/25/23 09: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 - 
> 00000000 !!!!
> 
> 
> 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.

This issue also causes use of the OVMF.fd file usage to fail for both SEV
and SEV-ES. With this patch using the combined file gives:

   Firmware Volume for Variable Store is corrupted
   ASSERT_EFI_ERROR (Status = Volume Corrupt)
   ASSERT [VariableRuntimeDxe] /root/kernels/ovmf-build-X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c(546): !(((INTN)(RETURN_STATUS)(Status)) < 0)

I believe for the same reason, that the mapping is encrypted, which causes
the signature and GUID checks to fail.

Thanks,
Tom

> 
> Thanks,
> Tom
> 
>>
>> thanks,
>>    Gerd
>>