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.web08.9513.1623140254882570315 for ; Tue, 08 Jun 2021 01:17:35 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=enHfodvR; 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=1623140254; 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=Vv9BkVSQk9P8cGy4xW08W+4dc9cOIEgnpCP+zhJ4v7o=; b=enHfodvRyOCce33kCC6myBXVHiLXz7wOhZAigm5Ud6Tttwv0NNqU/UmAmMApdZwXgjsosE toLvG4t2rvlvsN1L/sEX1GZp7Gr67AOb6iU7euoWJfIXsepmOOYclKRXri3CAg1MTImjxs 8GwWMDeZ+G+DP/Ao1e/R9ZtyDtFCTQk= 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-398-Tqx7JgUPOuOU3wyA8YfUHQ-1; Tue, 08 Jun 2021 04:17:29 -0400 X-MC-Unique: Tqx7JgUPOuOU3wyA8YfUHQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7586380ED8E; Tue, 8 Jun 2021 08:17:27 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-27.ams2.redhat.com [10.36.113.27]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6FAB860C04; Tue, 8 Jun 2021 08:17:24 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field To: Brijesh Singh , devel@edk2.groups.io, James Bottomley , Min Xu , Jiewen Yao , Tom Lendacky , Jordan Justen , Erdem Aktas , Eric Dong , Ray Ni , Rahul Kumar Cc: Ard Biesheuvel References: <20210526231118.12946-1-brijesh.singh@amd.com> <20210526231118.12946-4-brijesh.singh@amd.com> <75ef7f40-4a2b-5aeb-7859-d8a5cfdd7f2d@redhat.com> From: "Laszlo Ersek" Message-ID: <3aa3454f-9fd8-c04e-618d-fcca8ffe6a3b@redhat.com> Date: Tue, 8 Jun 2021 10:17:22 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 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: 8bit On 06/07/21 15:00, Brijesh Singh wrote: > Hi Laszlo, > > > On 6/7/21 6:20 AM, Laszlo Ersek via groups.io wrote: >> On 06/04/21 16:15, Laszlo Ersek wrote: >>> On 05/27/21 01:10, Brijesh Singh wrote: >>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7C4b1c71cc4cef4dd250f608d929a640cb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586616293720447%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Wulen9mRG3lzBHDOwrwP6%2BnV6F95LgnsgDxuEadi9Ng%3D&reserved=0 >>>> >>>> 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. > We must check the SNP Enabled inside the assembly code for the page > invalidate functions, and I decided to cache the value. A similar > SNP-enabled check is required in SEC phase before the > ProcessLibraryConstrctorList() is called. There are two options on how > we can go about doing the SNP enabled check inside the SEC phase > 1. Call the SEV_STATUS MSR after reading the > SEC_SEV_ES_WORK_AREA.SevEnabled. As you said, we need to be sure that ES > is enabled before calling the SEV_STATUS MSR. > 2. SEV_STATUS MSR is read in Reset vector for the SNP enabled check > purpose. Extend the SevEsWorkArea to cache the state. > >  I chose #2 because it avoids checking for ES enabled before checking > the SNP enabled. I understand that in the current code path, SNP check > is called inside the SevEsProtocolCheck() -- ES is already enabled, and > its safe to call SEV_STATUS MSR. What if we need to check for the SNP > state outside the ES-specific code block in the future? Then we will > need to extend the SevEsWorkArea. What would be the reason for this, ever? I think this ties in with another point (or question) I raised elsewhere: the assembly code in the reset vector suggests *anyway* that SNP is only available if ES is available, but I couldn't verify that from any specs. If this dependency is an architectural fact (that is, if ES is absent, then SNP may never be present), then I wouldn't like to introduce a separate field for SNP presence in the SEC_SEV_ES_WORK_AREA structure. I consider "data" the backbone (I consider "information flow" in a somewhat declarative, not imperative, sense). Code just organizes itself around data. > So I went ahead with option #2. I have > no problem reverting to option #1 and simplify the patch. I hope you > understand that sometimes it's difficult to foresee which option will be > preferred by the community, so I, as a contributor, can do one thing > and, based on the feedback, change the course. I *entirely* agree with you that it's impossible to predict what the community will prefer. Yet it doesn't hurt to ask questions (or propose approaches) in natural language, when facing a (likely important) choice. More importantly: ahead-of-time discussion is not indispensable, as long as you present your choices in code at once, but also document your choices in natural language, at the same time. I think I'd have been less frustrated and exhausted if your above 2-3 paragraphs had been included anywhere near your patches. Then, I wouldn't have had to reverse-engineer the data flow (the declarative backbone) bottom-up from the code, and dig down the history and original justification of SEC_SEV_ES_WORK_AREA. If you *did* describe all this somewhere, before, and I missed it, then I sincerely apologize. I do unconditionally apologize about my tone; I've been spread thin for a long time now, and heavy context switches are virtually unbearable. It's an explanation, but not an excuse, for sure. So please forgive me for being rude. I'll try to remedy that with some time away from work. Thanks Laszlo > > Thanks > >> ( >> >> 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)) >>>> >>> >>> >>> >>> >>> >> >> >> >> >> >