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 60AC821AE3CA6 for ; Wed, 24 May 2017 06:45:13 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AD24CC054C58; Wed, 24 May 2017 13:45:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com AD24CC054C58 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 AD24CC054C58 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-23.phx2.redhat.com [10.3.116.23]) by smtp.corp.redhat.com (Postfix) with ESMTP id 41EBA5C8B5; Wed, 24 May 2017 13:45:11 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org, jordan.l.justen@intel.com Cc: Thomas.Lendacky@amd.com, leo.duran@amd.com References: <1495466592-21641-1-git-send-email-brijesh.singh@amd.com> <1495466592-21641-13-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: Date: Wed, 24 May 2017 15:45:10 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <1495466592-21641-13-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 24 May 2017 13:45:12 +0000 (UTC) Subject: Re: [PATCH v5 12/14] OvmfPkg/QemuFwCfgLib: Implement SEV internal function for Dxe 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: Wed, 24 May 2017 13:45:13 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit comments below: On 05/22/17 17:23, Brijesh Singh wrote: > When SEV is enabled, the DMA must be performed on unencrypted pages. > So when get asked to perfom FWCFG DMA read or write, we allocate a > intermediate (bounce buffer) unencrypted buffer and use this buffer > for DMA read or write. > > > Cc: Jordan Justen > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Brijesh Singh > --- > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxeLib.inf | 7 ++ > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c | 108 ++++++++++++++++++++ > 2 files changed, 115 insertions(+) > > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxeLib.inf b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxeLib.inf > index d7e368e5435d..ec128f82c702 100644 > --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxeLib.inf > +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxeLib.inf > @@ -39,6 +39,7 @@ [Sources] > > [Packages] > MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > OvmfPkg/OvmfPkg.dec > > [LibraryClasses] > @@ -47,4 +48,10 @@ [LibraryClasses] > DebugLib > IoLib > MemoryAllocationLib > + MemEncryptSevLib > > +[Protocols] > + gEdkiiIoMmuProtocolGuid ## SOMETIMES_CONSUMES > + > +[Depex] > + gEdkiiIoMmuProtocolGuid OR gIoMmuDetectedProtocolGuid I'll comment on the depex elsewhere (returning to an earlier spot in the series). Other than that, I have only cosmetic comments: > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > index ac05f4c347f3..bda1a80b5eb9 100644 > --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > @@ -4,6 +4,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 > @@ -14,14 +15,36 @@ > WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > **/ > > +#include > + > +#include > + > +#include > #include > #include > +#include > +#include > > #include "QemuFwCfgLibInternal.h" > > STATIC BOOLEAN mQemuFwCfgSupported = FALSE; > STATIC BOOLEAN mQemuFwCfgDmaSupported; > > +STATIC EDKII_IOMMU_PROTOCOL *mIoMmuProtocol; > +/** > + > + Returns a boolean indicating whether SEV is enabled > + > + @retval TRUE SEV is enabled > + @retval FALSE SEV is disabled > +**/ > +BOOLEAN > +InternalQemuFwCfgSevIsEnabled ( > + VOID > + ) > +{ > + return MemEncryptSevIsEnabled (); > +} > > /** > Returns a boolean indicating if the firmware configuration interface > @@ -79,6 +102,22 @@ QemuFwCfgInitialize ( > mQemuFwCfgDmaSupported = TRUE; > DEBUG ((DEBUG_INFO, "QemuFwCfg interface (DMA) is supported.\n")); > } > + > + if (mQemuFwCfgDmaSupported && MemEncryptSevIsEnabled ()) { > + EFI_STATUS Status; > + > + // > + // IoMmuDxe driver must have installed the IOMMU protocol. If we are not > + // able to locate the protocol then something must have gone wrong. > + // > + Status = gBS->LocateProtocol (&gEdkiiIoMmuProtocolGuid, NULL, (VOID **)&mIoMmuProtocol); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "QemuwCfgSevDma: failed to locate IOMMU protocol\n")); (1) The word "QemuwCfgSevDma" has a typo, it should be "QemuFwCfgSevDma". Also, I recommend printing gEfiCallerBaseName / __FUNCTION__ here as well. (2) A number of DEBUGs are incorrectly indented below. I'm not comfortable giving my R-b in advance, with those fixed, because my comments on the depex (to be made elsewhere in the series) might induce a change to the depex, and I'd like to review that separately. If it turns out that the v5 depex will be fine, then the above two remarks don't necessitate a v6, and I'll be OK to R-b this v5 patch. Thanks, Laszlo > + ASSERT (FALSE); > + CpuDeadLoop (); > + } > + } > + > return RETURN_SUCCESS; > } > > @@ -114,3 +153,72 @@ InternalQemuFwCfgDmaIsAvailable ( > { > return mQemuFwCfgDmaSupported; > } > + > +/** > + Allocate a bounce buffer for SEV DMA. > + > + @param[in] NumPage Number of pages. > + @param[out] Buffer Allocated DMA Buffer pointer > + > +**/ > +VOID > +InternalQemuFwCfgSevDmaAllocateBuffer ( > + OUT VOID **Buffer, > + IN UINT32 NumPages > + ) > +{ > + EFI_STATUS Status; > + > + ASSERT (mIoMmuProtocol != NULL); > + > + Status = mIoMmuProtocol->AllocateBuffer ( > + mIoMmuProtocol, > + 0, > + EfiBootServicesData, > + NumPages, > + Buffer, > + EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED > + ); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a:%a failed to allocate %u pages\n", gEfiCallerBaseName, > + __FUNCTION__, NumPages)); > + ASSERT (FALSE); > + CpuDeadLoop (); > + } > + > + DEBUG ((DEBUG_VERBOSE, "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName, > + __FUNCTION__, (UINT64)(UINTN)Buffer, NumPages)); > +} > + > +/** > + 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 > + ) > +{ > + EFI_STATUS Status; > + > + ASSERT (mIoMmuProtocol != NULL); > + > + Status = mIoMmuProtocol->FreeBuffer ( > + mIoMmuProtocol, > + NumPages, > + Buffer > + ); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a:%a failed to free buffer 0x%Lx pages %u\n", > + gEfiCallerBaseName, __FUNCTION__, (UINT64)(UINTN)Buffer, NumPages)); > + ASSERT (FALSE); > + CpuDeadLoop (); > + } > + > + DEBUG ((DEBUG_VERBOSE, "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName, > + __FUNCTION__, (UINT64)(UINTN)Buffer, NumPages)); > +} >