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.8793.1622816116086937509 for ; Fri, 04 Jun 2021 07:15:16 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=coQkyzml; 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=1622816115; 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=ozWgpl420c8fEF3WTxluC/1USJf8+3vp96/tmDKCex8=; b=coQkyzmlX+AJ0MJArTMgAWftotOCZGehLyNB8RvhQOvo5T8EQ5bg+wdnnw2hKhskew+41T A+S0RnUhaEzg3nVTfUT4/tuiKpcZXrYLYpvh9X/qUrlygHJqMDtTE9gVJdqSEE/uFhiwIg qAEYdG6vvIquLRRC6T2Ai8kE51qL1d4= 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-408-Tbh1Nh3DPw2K3JhS71XwbQ-1; Fri, 04 Jun 2021 10:15:11 -0400 X-MC-Unique: Tbh1Nh3DPw2K3JhS71XwbQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 12C37107ACF6; Fri, 4 Jun 2021 14:15:10 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-217.ams2.redhat.com [10.36.112.217]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4695F39A5F; Fri, 4 Jun 2021 14:15:07 +0000 (UTC) Subject: Re: [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field 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 References: <20210526231118.12946-1-brijesh.singh@amd.com> <20210526231118.12946-4-brijesh.singh@amd.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 4 Jun 2021 16:15:05 +0200 MIME-Version: 1.0 In-Reply-To: <20210526231118.12946-4-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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 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 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)) >