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 61E0C21A16E26 for ; Thu, 11 May 2017 10:07:07 -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 6F246C04B94E; Thu, 11 May 2017 17:07:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6F246C04B94E Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 6F246C04B94E 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 E0E0F78128; Thu, 11 May 2017 17:07:04 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org References: <1494454162-9940-1-git-send-email-brijesh.singh@amd.com> <1494454162-9940-12-git-send-email-brijesh.singh@amd.com> Cc: Thomas.Lendacky@amd.com, leo.duran@amd.com, Jordan Justen From: Laszlo Ersek Message-ID: Date: Thu, 11 May 2017 19:07:03 +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-12-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.31]); Thu, 11 May 2017 17:07:07 +0000 (UTC) Subject: Re: [RFC v4 11/13] 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: Thu, 11 May 2017 17:07:07 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit comments below: On 05/11/17 00:09, 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 | 4 + > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c | 107 ++++++++++++++++++++ > 2 files changed, 111 insertions(+) > > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxeLib.inf b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxeLib.inf > index 346bb881ffc1..f8df77f788b7 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,7 @@ [LibraryClasses] > DebugLib > IoLib > MemoryAllocationLib > + MemEncryptSevLib > > +[Protocols] > + gEdkiiIoMmuProtocolGuid ## SOMETIMES_CONSUMES > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > index ac05f4c347f3..059666ffa99b 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 "Uefi.h" (1) Not sure what this is needed for, but if it is needed, please use . > + > +#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,21 @@ QemuFwCfgInitialize ( > mQemuFwCfgDmaSupported = TRUE; > DEBUG ((DEBUG_INFO, "QemuFwCfg interface (DMA) is supported.\n")); > } > + > + // > + // When SEV is enabled, the AmdSevDxe driver should have installed the IoMMU > + // protocol which must be used for mapping host buffer to DMA buffer > + // (2) The comment should be stronger. Please state that the IOMMU protocol is guaranteed to be available, because the driver is listed in the APRIORI DXE file. > + if (mQemuFwCfgDmaSupported && MemEncryptSevIsEnabled ()) { > + EFI_STATUS Status; > + > + Status = gBS->LocateProtocol (&gEdkiiIoMmuProtocolGuid, NULL, (VOID **)&mIoMmuProtocol); > + if (EFI_ERROR(Status)) { (3) Space missing before the paren. > + DEBUG ((DEBUG_WARN, "QemuwCfgSevDma: failed to locate IoMmu protocol, disabling DMA support\n")); > + mQemuFwCfgDmaSupported = FALSE; > + } > + } > + (4) Similarly to (2), this is too permissive. Please use DEBUG_ERROR, ASSERT (FALSE), and CpuDeadLoop() here. We must not silently degrade the fw_cfg interface to port-io, because some client modules equate the presence of some fw_cfg files -- which can also be found via port-io -- with DMA and fw_cfg write capability. The expectation of those modules is correct, and we must not break it. > return RETURN_SUCCESS; > } > > @@ -114,3 +152,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 ( > + IN UINT32 NumPages, > + OUT VOID **Buffer > + ) > +{ > + EFI_STATUS Status; > + > + if (!mIoMmuProtocol) { > + // > + // We should never reach here > + // > + ASSERT (FALSE); > + CpuDeadLoop (); > + } (5) In turn, this can be replaced with ASSERT (mIoMmuProtocol != NULL); > + > + Status = mIoMmuProtocol->AllocateBuffer ( > + mIoMmuProtocol, > + 0, > + EfiBootServicesData, > + NumPages, > + Buffer, > + EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED > + ); > + ASSERT_EFI_ERROR (Status); (6) Please add a CpuDeadLoop() here. > + > + DEBUG ((DEBUG_VERBOSE, "QemuFwCfgSevDma allocate buffer 0x%Lx Pages %d\n", (UINTN)Buffer, NumPages)); > + > +} (7) Again I suggest to add gEfiCallerBaseName and __FUNCTION__ to the debug message. (8) Please also wrap it to 79 chars. (9) %Lx is not right for printing a UINTN. There is no conversion specifier that directly matches UINTN, so you'll have to use (UINT64)(UINTN)Buffer for portability between IA32 and X64. (10) NumPages should be printed with %u or %x, it is unsigned. > + > +/** > + 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; > + > + if (!mIoMmuProtocol) { > + // > + // We should never reach here > + // > + ASSERT (FALSE); > + CpuDeadLoop (); > + } > + > + Status = mIoMmuProtocol->FreeBuffer ( > + mIoMmuProtocol, > + NumPages, > + Buffer > + ); > + ASSERT_EFI_ERROR (Status); > + DEBUG ((DEBUG_VERBOSE, "QemuFwCfgSevDma free buffer 0x%Lx Pages %d\n", (UINTN)Buffer, NumPages)); > +} > The same comments apply here. Thanks! Laszlo