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.8368.1622814248736737380 for ; Fri, 04 Jun 2021 06:44:08 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Bs1iYYQb; 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=1622814247; 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=A1oGsvBtqRo8akOXbbGUENn4rjK6MBHHWE4uH0HeZ7g=; b=Bs1iYYQbXjaUaZH3XOeAJZihn0AKOQkEK7ygv+US9tdWjTkO2+4F4RiLkf/seaxXg05TtN 8eJMLJMBcNxLVl4BzeomblvbzG3XR69Em5jupEeVc6yD3dbHD3O5T1vWjfAdDfxAuvLpEO 6+aSnCd3AXbGeHThFm7RclHPAgSpigE= 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-346-BP-URgeDODmC4Rc7ra33zQ-1; Fri, 04 Jun 2021 09:44:04 -0400 X-MC-Unique: BP-URgeDODmC4Rc7ra33zQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E05E2107ACC7; Fri, 4 Jun 2021 13:44:02 +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 3E68417A99; Fri, 4 Jun 2021 13:44:00 +0000 (UTC) Subject: Re: [PATCH RFC v3 02/22] OvmfPkg/MemEncryptSevLib: add MemEncryptSevSnpEnabled() 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-3-brijesh.singh@amd.com> From: "Laszlo Ersek" Message-ID: <7c5d2ab9-8775-f725-bc7e-16548f4f4f9d@redhat.com> Date: Fri, 4 Jun 2021 15:43:58 +0200 MIME-Version: 1.0 In-Reply-To: <20210526231118.12946-3-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 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 > > Create a function that can be used to determine if VM is running as an > SEV-SNP guest. > > 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/Include/Library/MemEncryptSevLib.h | 12 +++++++++ > .../DxeMemEncryptSevLibInternal.c | 27 +++++++++++++++++++ > .../PeiMemEncryptSevLibInternal.c | 27 +++++++++++++++++++ > .../SecMemEncryptSevLibInternal.c | 19 +++++++++++++ > 4 files changed, 85 insertions(+) > > diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h > index 76d06c206c8b..2425d8ba0a36 100644 > --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h > +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h > @@ -66,6 +66,18 @@ typedef enum { > MemEncryptSevAddressRangeError, > } MEM_ENCRYPT_SEV_ADDRESS_RANGE_STATE; > > +/** > + Returns a boolean to indicate whether SEV-SNP is enabled > + > + @retval TRUE SEV-SNP is enabled > + @retval FALSE SEV-SNP is not enabled > +**/ > +BOOLEAN > +EFIAPI > +MemEncryptSevSnpIsEnabled ( > + VOID > + ); > + > /** > Returns a boolean to indicate whether SEV-ES is enabled. > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > index 2816f859a0c4..057129723824 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c > @@ -19,6 +19,7 @@ > > STATIC BOOLEAN mSevStatus = FALSE; > STATIC BOOLEAN mSevEsStatus = FALSE; > +STATIC BOOLEAN mSevSnpStatus = FALSE; > STATIC BOOLEAN mSevStatusChecked = FALSE; > > STATIC UINT64 mSevEncryptionMask = 0; > @@ -82,11 +83,37 @@ InternalMemEncryptSevStatus ( > if (Msr.Bits.SevEsBit) { > mSevEsStatus = TRUE; > } > + > + // > + // Check MSR_0xC0010131 Bit 2 (Sev-Snp Enabled) > + // > + if (Msr.Bits.SevSnpBit) { > + mSevSnpStatus = TRUE; > + } > } > > mSevStatusChecked = TRUE; > } > > +/** > + Returns a boolean to indicate whether SEV-SNP is enabled. > + > + @retval TRUE SEV-SNP is enabled > + @retval FALSE SEV-SNP is not enabled > +**/ > +BOOLEAN > +EFIAPI > +MemEncryptSevSnpIsEnabled ( > + VOID > + ) > +{ > + if (!mSevStatusChecked) { > + InternalMemEncryptSevStatus (); > + } > + > + return mSevSnpStatus; > +} > + > /** > Returns a boolean to indicate whether SEV-ES is enabled. > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > index e2fd109d120f..b561f211f577 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c > @@ -19,6 +19,7 @@ > > STATIC BOOLEAN mSevStatus = FALSE; > STATIC BOOLEAN mSevEsStatus = FALSE; > +STATIC BOOLEAN mSevSnpStatus = FALSE; > STATIC BOOLEAN mSevStatusChecked = FALSE; > > STATIC UINT64 mSevEncryptionMask = 0; > @@ -82,11 +83,37 @@ InternalMemEncryptSevStatus ( > if (Msr.Bits.SevEsBit) { > mSevEsStatus = TRUE; > } > + > + // > + // Check MSR_0xC0010131 Bit 2 (Sev-Snp Enabled) > + // > + if (Msr.Bits.SevSnpBit) { > + mSevSnpStatus = TRUE; > + } > } > > mSevStatusChecked = TRUE; > } > > +/** > + Returns a boolean to indicate whether SEV-SNP is enabled. > + > + @retval TRUE SEV-SNP is enabled > + @retval FALSE SEV-SNP is not enabled > +**/ > +BOOLEAN > +EFIAPI > +MemEncryptSevSnpIsEnabled ( > + VOID > + ) > +{ > + if (!mSevStatusChecked) { > + InternalMemEncryptSevStatus (); > + } > + > + return mSevSnpStatus; > +} > + > /** > Returns a boolean to indicate whether SEV-ES is enabled. > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > index 56d8f3f3183f..69852779e2ff 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c > @@ -62,6 +62,25 @@ InternalMemEncryptSevStatus ( > return ReadSevMsr ? AsmReadMsr32 (MSR_SEV_STATUS) : 0; > } > > +/** > + Returns a boolean to indicate whether SEV-SNP is enabled. > + > + @retval TRUE SEV-SNP is enabled > + @retval FALSE SEV-SNP is not enabled > +**/ > +BOOLEAN > +EFIAPI > +MemEncryptSevSnpIsEnabled ( > + VOID > + ) > +{ > + MSR_SEV_STATUS_REGISTER Msr; > + > + Msr.Uint32 = InternalMemEncryptSevStatus (); > + > + return Msr.Bits.SevSnpBit ? TRUE : FALSE; According to the edk2 coding style, this should be written as: return (BOOLEAN)(Msr.Bits.SevSnpBit != 0); primarily because "Msr.Bits.SevSnpBit" does not have type BOOLEAN, so it should not be used stand-alone in a logical context -- instead, it should be compared with zero explicitly. However, I see that the other two functions MemEncryptSevEsIsEnabled() and MemEncryptSevIsEnabled() in this library instance use the same pattern. I agree consistency is more important here. And I don't think we should add more patches just for updating the style there. These functions are closely located together in the same source file; if we ever update the style, it's not a big deal to update them together. Reviewed-by: Laszlo Ersek Thanks Laszlo > +} > + > /** > Returns a boolean to indicate whether SEV-ES is enabled. > >