From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web11.34514.1623064826502933271 for ; Mon, 07 Jun 2021 04:20:26 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=HU3ws7+r; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1623064825; h=from:from:reply-to: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=AH5lZKyvydzCJsTePuoTU51vaYkxJYHAyLXGDKGLv/c=; b=HU3ws7+r9BwD7Jz69kHREnWAlqhypyBx3CRj+aQscffbq1yA1rK9c1FDveY08RpvYUFbWN oFR2HEihlCnG179yWPAAzepEKM2dkPc3AGN2foyt/W1suX7R9hC0CWzEL4K4mRclTfak4O XOhnWpbrUWR6n2aSjTVbqzIvZXIGR0s= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-583-y2aiDRwsM7SICWnBCvhy-g-1; Mon, 07 Jun 2021 07:20:22 -0400 X-MC-Unique: y2aiDRwsM7SICWnBCvhy-g-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 30738801AF1; Mon, 7 Jun 2021 11:20:20 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-75.ams2.redhat.com [10.36.114.75]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 827085D6D3; Mon, 7 Jun 2021 11:20:17 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field From: "Laszlo Ersek" To: Brijesh Singh , James Bottomley , Min Xu , Jiewen Yao , Tom Lendacky , Jordan Justen , Erdem Aktas , Eric Dong , Ray Ni , Rahul Kumar , devel@edk2.groups.io Cc: Ard Biesheuvel Reply-To: devel@edk2.groups.io, lersek@redhat.com References: <20210526231118.12946-1-brijesh.singh@amd.com> <20210526231118.12946-4-brijesh.singh@amd.com> Message-ID: <75ef7f40-4a2b-5aeb-7859-d8a5cfdd7f2d@redhat.com> Date: Mon, 7 Jun 2021 13:20:15 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/04/21 16:15, Laszlo Ersek wrote: > On 05/27/21 01:10, Brijesh Singh wrote: >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275 >> >> Extend the workarea to include the SEV-SNP enabled fields. This will be set >> when SEV-SNP is active in the guest VM. >> >> Cc: James Bottomley >> Cc: Min Xu >> Cc: Jiewen Yao >> Cc: Tom Lendacky >> Cc: Jordan Justen >> Cc: Ard Biesheuvel >> Cc: Laszlo Ersek >> Cc: Erdem Aktas >> Signed-off-by: Brijesh Singh >> --- >> OvmfPkg/PlatformPei/PlatformPei.inf | 1 + >> OvmfPkg/Include/Library/MemEncryptSevLib.h | 3 ++- >> OvmfPkg/PlatformPei/AmdSev.c | 26 ++++++++++++++++++++++ >> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 12 ++++++++++ >> OvmfPkg/ResetVector/ResetVector.nasmb | 1 + >> 5 files changed, 42 insertions(+), 1 deletion(-) > > (1) Please split this in two patches -- the PlatformPei changes should > be a separate patch. And, I think those should come second, the > ResetVector + header file change should come first. > >> >> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf >> index 6ef77ba7bb21..bc1dcac48343 100644 >> --- a/OvmfPkg/PlatformPei/PlatformPei.inf >> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf >> @@ -110,6 +110,7 @@ [Pcd] >> gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize >> gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled >> + gUefiCpuPkgTokenSpaceGuid.PcdSevSnpIsEnabled >> >> [FixedPcd] >> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress >> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h >> index 2425d8ba0a36..24507de55c5d 100644 >> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h >> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h >> @@ -49,7 +49,8 @@ typedef struct { >> // >> typedef struct _SEC_SEV_ES_WORK_AREA { >> UINT8 SevEsEnabled; >> - UINT8 Reserved1[7]; >> + UINT8 SevSnpEnabled; >> + UINT8 Reserved2[6]; >> >> UINT64 RandomData; >> >> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c >> index a8bf610022ba..67b78fd5fa36 100644 >> --- a/OvmfPkg/PlatformPei/AmdSev.c >> +++ b/OvmfPkg/PlatformPei/AmdSev.c >> @@ -22,6 +22,27 @@ >> >> #include "Platform.h" >> >> +/** >> + >> + Initialize SEV-SNP support if running as an SEV-SNP guest. >> + >> + **/ >> +STATIC >> +VOID >> +AmdSevSnpInitialize ( >> + VOID >> + ) >> +{ >> + RETURN_STATUS PcdStatus; >> + >> + if (!MemEncryptSevSnpIsEnabled ()) { >> + return; >> + } >> + >> + PcdStatus = PcdSetBoolS (PcdSevSnpIsEnabled, TRUE); >> + ASSERT_RETURN_ERROR (PcdStatus); >> +} >> + >> /** >> >> Initialize SEV-ES support if running as an SEV-ES guest. >> @@ -209,4 +230,9 @@ AmdSevInitialize ( >> // Check and perform SEV-ES initialization if required. >> // >> AmdSevEsInitialize (); >> + >> + // >> + // Check and perform SEV-SNP initialization if required. >> + // >> + AmdSevSnpInitialize (); >> } >> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm >> index 5fae8986d9da..6838cdeec9c3 100644 >> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm >> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm >> @@ -81,6 +81,11 @@ CheckSevFeatures: >> ; the MSR check below will set the first byte of the workarea to one. >> mov byte[SEV_ES_WORK_AREA], 0 >> >> + ; Set the SevSnpEnabled field in workarea to zero to communicate to the SEC >> + ; phase that SEV-SNP is not enabled. If SEV-SNP is enabled, this function >> + ; will set it to 1. >> + mov byte[SEV_ES_WORK_AREA_SNP], 0 >> + >> ; >> ; Set up exception handlers to check for SEV-ES >> ; Load temporary RAM stack based on PCDs (see SevEsIdtVmmComm for >> @@ -136,6 +141,13 @@ CheckSevFeatures: >> ; phase that SEV-ES is enabled. >> mov byte[SEV_ES_WORK_AREA], 1 >> >> + bt eax, 2 >> + jnc GetSevEncBit >> + >> + ; Set the second byte of the workarea to one to communicate to the SEC >> + ; phase that the SEV-SNP is enabled >> + mov byte[SEV_ES_WORK_AREA_SNP], 1 >> + >> GetSevEncBit: >> ; Get pte bit position to enable memory encryption >> ; CPUID Fn8000_001F[EBX] - Bits 5:0 > > (2) Please mention in the commit message (of the ResetVector patch), > and/or a comment here in the code, that SEV-SNP is never enabled if > SEV-ES is disabled. > > Section "15.34.10 SEV_STATUS MSR" in the APM (doc#24593 v3.37) does not > spell out this dependency. > > Furthermore, the mSevStatus / mSevEsStatus / mSevSnpStatus variable > assignments in patch#2 do not form a "dependency cascade" like the one > seen here in the reset vector code. > > While "SEV-ES depends on SEV" seems obvious to me (and so the related, > existent jumps in the assembly code are not surprising), the statement > "SEV-SNP depends on SEV-ES" is not *that* obvious to me. Thus a comment > would be welcome. > > For *both* patches split out of this one: > > Reviewed-by: Laszlo Ersek (3) Actually, no. This patch should be reduced to the following files only: - OvmfPkg/PlatformPei/AmdSev.c - OvmfPkg/PlatformPei/PlatformPei.inf and the following changes should be dropped completely: - OvmfPkg/Include/Library/MemEncryptSevLib.h - OvmfPkg/ResetVector/Ia32/PageTables64.asm - OvmfPkg/ResetVector/ResetVector.nasmb Specifically, the "SEC_SEV_ES_WORK_AREA.SevSnpEnabled" field should never be introduced. The reason is apparent only from patch #10 -- "OvmfPkg/PlatformPei: register GHCB gpa for the SEV-SNP guest". The core idea is that in patch#10, in the SEC module, you can implement SevSnpIsEnabled() by just reading MSR_SEV_STATUS, and checking the SNP bit. Namely, while the SevSnpIsEnabled() call is made in SevEsProtocolCheck(), i.e., before exception handling is set up in the SEC module -- and so you indeed cannot call CPUID --, you don't *have* to call CPUID at that call site. Where you call SevSnpIsEnabled() in SevEsProtocolCheck(), you already know that SEV-ES is enabled, so it's safe to just read the exact same SEV status MSR that the SEV-ES status comes from in the first place, without any CPUID safety check. ( General request: please be explicit in the commit messages about the data flow between modules, and why you are doing what you are doing. Arriving at the above analysis took me 3+ hours this morning, when -- while reviewing patch#4 -- I took issue with the proliferation of the new fields in SEC_SEV_ES_WORK_AREA. SEC_SEV_ES_WORK_AREA is *not* a convenience dump. We should restrict its use as much as possible. I double-checked how SEC_SEV_ES_WORK_AREA had evolved historically: * SEC_SEV_ES_WORK_AREA.SevEsEnabled: 1 43c3df78460d OvmfPkg: Reserve a page in memory for the SEV-ES usage 2 0731236fc108 OvmfPkg/PlatformPei: Reserve SEV-ES work area if S3 is supported 3 8a2732186a53 OvmfPkg/ResetVector: Add support for a 32-bit SEV check 4 13e5492bfdf3 OvmfPkg/Sec: Add #VC exception handling for Sec phase The "SEC_SEV_ES_WORK_AREA.SevEsEnabled" field is important for the following reason: - in an SEV-ES guest, just learning about SEV requires exception handling; thus, the Reset Vector sets up exception handling *unconditionally*, - in SEC, we deal with exception handling regardless of SEV-ES, but *how* we do that is conditional on SEV-ES. This means that caching the SEV-ES presence from the Reset Vector to SEC makes a lot of sense. Given that in the Reset Vector we have unconditional exception handling, and then in SEC we have a cached result, we are allowed to only have conditional exception handling in SEC. * SEC_SEV_ES_WORK_AREA.RandomData, SEC_SEV_ES_WORK_AREA.EncryptionMask: 1 7cb96c47a94e OvmfPkg/ResetVector: Validate the encryption bit position for SEV/SEV-ES 2 bd0c1c8e225b OvmfPkg/ResetVector: Perform a simple SEV-ES sanity check 3 3b32be7e7192 OvmfPkg/ResetVector: Save the encryption mask at boot time 4 b97dc4b92ba1 OvmfPkg/MemEncryptSevLib: Add an interface to retrieve the encryption mask The "RandomData" and "EncryptionMask" fields in the SEC_SEV_ES_WORK_AREA structure seem justified because they implement some serious work (which must be done as early as possible, i.e., in the Reset Vector), *and* caching the result of that work for the rest of the firmware saves significant complexity (and the result is security-related even). "SEC_SEV_ES_WORK_AREA.SevSnpEnabled" is unlike any of these three fields. ) Thanks Laszlo >> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb >> index 5fbacaed5f9d..1971557b1c00 100644 >> --- a/OvmfPkg/ResetVector/ResetVector.nasmb >> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb >> @@ -73,6 +73,7 @@ >> %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase)) >> %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize)) >> %define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase)) >> + %define SEV_ES_WORK_AREA_SNP (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 1) >> %define SEV_ES_WORK_AREA_RDRAND (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 8) >> %define SEV_ES_WORK_AREA_ENC_MASK (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 16) >> %define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize)) >> > > > > > >