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.133.124]) by mx.groups.io with SMTP id smtpd.web11.43965.1675092299551893214 for ; Mon, 30 Jan 2023 07:24:59 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=OWUCGcRU; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1675092298; 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=DFXhzbAHp0dgk81dzPZr8QBY1+JQvKWJVc9qFrjtqJU=; b=OWUCGcRUekNbfluHy1sTZaMaTVFiNDmOAHuBdvUOxJindUKUjx4dqu3EZ4OT+EPtpa3b7m JYELYRSYe1KbhX/mOrUnQAV+I58/Cm/fhFvL866vUuuchDBPGUA8e6CdCjvdCZWWCnKCsA CzzoSi3QMmdOvGHFfxFdXpH7WDPWVXo= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-558-UQJU0NDjMEWIUZ3kCDe8Pg-1; Mon, 30 Jan 2023 10:24:57 -0500 X-MC-Unique: UQJU0NDjMEWIUZ3kCDe8Pg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id CC1403C42205; Mon, 30 Jan 2023 15:24:56 +0000 (UTC) Received: from [10.39.194.96] (unknown [10.39.194.96]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4D9EE40C2064; Mon, 30 Jan 2023 15:24:55 +0000 (UTC) Message-ID: <194998b7-5880-e9f6-270a-c1e4fb006348@redhat.com> Date: Mon, 30 Jan 2023 16:24:54 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v4 3/5] OvmfPkg/PlatformInitLib: Add PlatformAddHobCB From: "Laszlo Ersek" 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> <282b7739-5d72-3d81-efe1-f26b30ed993c@redhat.com> In-Reply-To: <282b7739-5d72-3d81-efe1-f26b30ed993c@redhat.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.1 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 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