From: "Laszlo Ersek" <lersek@redhat.com>
To: Tom Lendacky <thomas.lendacky@amd.com>,
Gerd Hoffmann <kraxel@redhat.com>
Cc: devel@edk2.groups.io, Jiewen Yao <jiewen.yao@intel.com>,
Oliver Steffen <osteffen@redhat.com>,
Ard Biesheuvel <ardb+tianocore@kernel.org>,
Pawel Polawski <ppolawsk@redhat.com>,
Jordan Justen <jordan.l.justen@intel.com>
Subject: Re: [edk2-devel] [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB
Date: Mon, 30 Jan 2023 16:23:35 +0100 [thread overview]
Message-ID: <282b7739-5d72-3d81-efe1-f26b30ed993c@redhat.com> (raw)
In-Reply-To: <e3a59dc6-58f9-a500-e1aa-122d1b8eb279@amd.com>
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
next prev parent reply other threads:[~2023-01-30 15:23 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-17 12:16 [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts Gerd Hoffmann
2023-01-17 12:16 ` [PATCH v4 1/5] OvmfPkg/PlatformInitLib: Add PlatformScanE820 and GetFirstNonAddressCB Gerd Hoffmann
2023-01-17 14:30 ` Laszlo Ersek
2023-01-17 14:46 ` Laszlo Ersek
2023-01-17 14:48 ` [edk2-devel] " Ard Biesheuvel
2023-01-17 12:16 ` [PATCH v4 2/5] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB Gerd Hoffmann
2023-01-17 14:45 ` Laszlo Ersek
2023-01-17 12:16 ` [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB Gerd Hoffmann
2023-01-17 15:00 ` Laszlo Ersek
2023-01-17 15:06 ` Ard Biesheuvel
2023-01-17 16:04 ` Ard Biesheuvel
2023-01-24 22:33 ` [edk2-devel] " Lendacky, Thomas
2023-01-25 9:11 ` Gerd Hoffmann
2023-01-25 15:35 ` Lendacky, Thomas
2023-01-25 16:32 ` Lendacky, Thomas
2023-01-30 15:23 ` Laszlo Ersek [this message]
2023-01-30 15:24 ` Laszlo Ersek
2023-01-17 12:16 ` [PATCH v4 4/5] OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB Gerd Hoffmann
2023-01-17 15:05 ` Laszlo Ersek
2023-01-17 12:16 ` [PATCH v4 5/5] OvmfPkg/PlatformInitLib: reorder PlatformQemuUc32BaseInitialization Gerd Hoffmann
2023-01-17 14:49 ` Laszlo Ersek
2023-01-17 16:38 ` [edk2-devel] [PATCH v4 0/5] OvmfPkg: check 64bit mmio window for resource conflicts Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=282b7739-5d72-3d81-efe1-f26b30ed993c@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox