From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.43921.1675092223694291865 for ; Mon, 30 Jan 2023 07:23:44 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=GROIvYci; spf=pass (domain: redhat.com, ip: 170.10.129.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1675092222; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Zbg8UcGKIDISPGVmGuasKo+Mzutatw2WFTb9eVRIjUU=; b=GROIvYciU66nF+mFvwFW09wjZYStnl59c/zNL81ege/4lB/USzuzjObnIHhzg+dNYF14q7 jfKrB1pgzGQYvGa1BPmru3QC3aOxUXHZLYT6bTtEVthCMCCdWVqhMBr/dBAo8BPROv23SP 2oo6PiVG9gLT5bA5TMeGnh5KnJdYygQ= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-367-tbfVKbgMNF-PB1FFUaykgA-1; Mon, 30 Jan 2023 10:23:39 -0500 X-MC-Unique: tbfVKbgMNF-PB1FFUaykgA-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B737A100F906; Mon, 30 Jan 2023 15:23:38 +0000 (UTC) Received: from [10.39.194.96] (unknown [10.39.194.96]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D5A85492B00; Mon, 30 Jan 2023 15:23:36 +0000 (UTC) Message-ID: <282b7739-5d72-3d81-efe1-f26b30ed993c@redhat.com> Date: Mon, 30 Jan 2023 16:23:35 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB To: Tom Lendacky , Gerd Hoffmann Cc: devel@edk2.groups.io, Jiewen Yao , Oliver Steffen , Ard Biesheuvel , Pawel Polawski , Jordan Justen References: <20230117121629.2149112-1-kraxel@redhat.com> <20230117121629.2149112-4-kraxel@redhat.com> <20230125091101.rtxlviabpxg5uqq3@sirius.home.kraxel.org> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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