From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A006A21A16E49 for ; Thu, 11 May 2017 09:24:48 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D5277C059738; Thu, 11 May 2017 16:24:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D5277C059738 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com D5277C059738 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-15.phx2.redhat.com [10.3.116.15]) by smtp.corp.redhat.com (Postfix) with ESMTP id 70E535DD65; Thu, 11 May 2017 16:24:46 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org References: <1494454162-9940-1-git-send-email-brijesh.singh@amd.com> <1494454162-9940-10-git-send-email-brijesh.singh@amd.com> Cc: Thomas.Lendacky@amd.com, leo.duran@amd.com, Jordan Justen From: Laszlo Ersek Message-ID: <08fc7dad-dc56-7b2e-ed71-9764986491a0@redhat.com> Date: Thu, 11 May 2017 18:24:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1494454162-9940-10-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Thu, 11 May 2017 16:24:48 +0000 (UTC) Subject: Re: [RFC v4 09/13] OvmfPkg/QemuFwCfgLib: Implement SEV internal function for SEC phase X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 May 2017 16:24:48 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 05/11/17 00:09, Brijesh Singh wrote: > Cc: Jordan Justen > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Brijesh Singh > --- > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf | 1 + > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c | 57 ++++++++++++++++++++ > 2 files changed, 58 insertions(+) > > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf > index 7a96575d1851..b782ac6c0aa2 100644 > --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf > +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf > @@ -45,4 +45,5 @@ [LibraryClasses] > DebugLib > IoLib > MemoryAllocationLib > + MemEncryptSevLib This will not compile. More below. > > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c > index 465ccbe90dad..cd04cc814063 100644 > --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c > +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c > @@ -6,6 +6,7 @@ > > Copyright (C) 2013, Red Hat, Inc. > Copyright (c) 2011 - 2013, Intel Corporation. All rights reserved.
> + Copyright (c) 2017, Advanced Micro Devices. All rights reserved.
> > This program and the accompanying materials are licensed and made available > under the terms and conditions of the BSD License which accompanies this > @@ -18,6 +19,7 @@ > > #include > #include > +#include > > #include "QemuFwCfgLibInternal.h" > > @@ -94,3 +96,58 @@ InternalQemuFwCfgDmaIsAvailable ( > { > return FALSE; > } > + > +/** > + > + Returns a boolean indicating whether SEV is enabled > + > + @retval TRUE SEV is enabled > + @retval FALSE SEV is disabled > +**/ > +BOOLEAN > +InternalQemuFwCfgSevIsEnabled ( > + VOID > + ) > +{ > + return MemEncryptSevIsEnabled (); > +} So, this is not right. We have one instance of MemEncryptSevLib, namely BaseMemEncryptSevLib. It uses / needs writeable static variables, therefore it is not usable in SEC phase modules. QemuFwCfgSecLib.inf is restricted to SEC type client modules, so the above function call would not work. Thankfully, this patch won't even compile (showcasing that the edk2 build system works fine). Namely, corresponding to the writeable static variable requirement in BaseMemEncryptSevLib, we restricted that library instance to the following client module types: PEIM DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_DRIVER Whereas, QemuFwCfgSecLib.inf itself is restricted to SEC. The result is, if you try to build QemuFwCfgSecLib.inf into a SEC module, the client module type restrictions inherited from BaseMemEncryptSevLib will cause a build conflict. Otherwise, if you try to build QemuFwCfgSecLib.inf into, say, a PEIM, then QemuFwCfgSecLib.inf's own SEC restriction will cause a build conflict. So this patch cannot compile. You didn't find this in your testing because OVMF currently has no SEC phase module that uses fw_cfg -- QemuFwCfgSecLib.inf is never built. You can manually build it like this, for example (note the "-m" option): build -a X64 -p OvmfPkg/OvmfPkgX64.dsc -t GCC48 -b DEBUG \ -m OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf (a) One solution is what I wrote in : > (2) Implement InternalQemuFwCfgSevIsEnabled() in "QemuFwCfgSec.c" > without using global variables (i.e., with a CPUID on each call, on > AMD processors, and return constant FALSE on Intel processors). So basically you should open-code MemEncryptSevIsEnabled() here, without using any global variables. (b) Now, a "by the book" solution would be to introduce a SEC instance of MemEncryptSevLib as well, which would execute CPUID on every MemEncryptSevIsEnabled() call, and return RETURN_UNSUPPORTED from both MemEncryptSevClearPageEncMask() and MemEncryptSevSetPageEncMask(), regardless of architecture (IA32 vs X64). Then this patch would compile as-is. (c) But, I don't want to contribute to the proliferation, or growth, of library instances that are never used in reality. So I dislike both (a) and (b) above. The only reason we care about QemuFwCfgLib, with regard to SEV, is because QemuFwCfgLib *sometimes* uses DMA, and SEV has consequences for DMA. Notice though that the SEC instance of InternalQemuFwCfgDmaIsAvailable() returns constant FALSE. This is why it is fine to put ASSERT(FALSE) / CpuDeadLoop() in the bounce buffer alloc / dealloc routines: they will never be called. With the same argument (i.e., we'll never use DMA fw_cfg in SEC), it is entirely irrelevant for this lib instance whether SEV is present or not. So I suggest to simply return FALSE from InternalQemuFwCfgSevIsEnabled() above, and to remark there, in a comment, that InternalQemuFwCfgDmaIsAvailable() returns constant FALSE, hence SEV availability is irrelevant. Thanks, Laszlo > + > +/** > + Allocate a bounce buffer for SEV DMA. > + > + @param[in] NumPage Number of pages. > + @param[out] Buffer Allocated DMA Buffer pointer > + > +**/ > +VOID > +InternalQemuFwCfgSevDmaAllocateBuffer ( > + IN UINT32 NumPages, > + OUT VOID **Buffer > + ) > +{ > + // > + // We should never reach here > + // > + ASSERT (FALSE); > + CpuDeadLoop (); > +} > + > +/** > + Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer > + > + @param[in] NumPage Number of pages. > + @param[in] Buffer DMA Buffer pointer > + > +**/ > +VOID > +InternalQemuFwCfgSevDmaFreeBuffer ( > + IN VOID *Buffer, > + IN UINT32 NumPages > + ) > +{ > + // > + // We should never reach here > + // > + ASSERT (FALSE); > + CpuDeadLoop (); > +} >