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 8381321D49C88 for ; Fri, 4 Aug 2017 18:30:43 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4EBC7883AB; Sat, 5 Aug 2017 01:32:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4EBC7883AB Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-20.phx2.redhat.com [10.3.116.20]) by smtp.corp.redhat.com (Postfix) with ESMTP id D554361F25; Sat, 5 Aug 2017 01:32:54 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Tom Lendacky , Jordan Justen References: <1501801872-22626-1-git-send-email-brijesh.singh@amd.com> <1501801872-22626-2-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <606aa163-8564-9275-8477-fe13ecd7afc9@redhat.com> Date: Sat, 5 Aug 2017 03:32:53 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1501801872-22626-2-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Sat, 05 Aug 2017 01:32:56 +0000 (UTC) Subject: Re: [PATCH v3 1/1] OvmfPkg/QemuFwCfgLib: Use BusMasterCommonBuffer to map FW_CFG_DMA_ACCESS 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: Sat, 05 Aug 2017 01:30:43 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/04/17 01:11, Brijesh Singh wrote: > Commit 09719a01b11b (OvmfPkg/QemuFwCfgLib: Implement SEV internal function > for Dxe phase) uses IOMMU protocol to allocate and free FW_CFG_DMA_ACCESS > buffer when SEV is active. During initial commits we made assumption that > IOMMU.AllocateBuffer() will provide PlainTextAddress (i.e C-bit cleared). > This assumption was wrong, the AllocateBuffer() protocol member is not > expected to produce a buffer that is immediatly usable, and client is > required to call Map() uncondtionally with BusMasterCommonBuffer[64] to > get a mapping which is accessable by both host and device. > > The patch refactors code a bit and add the support to Map() > FW_CFG_DMA_ACCESS buffer using BusMasterCommonBuffer operation after > allocation and Unamp() before free. > > The complete discussion about this and recommendation from Laszlo can be > found here [1] > > [1] https://lists.01.org/pipermail/edk2-devel/2017-July/012652.html > > Suggested-by: Laszlo Ersek > Cc: Jordan Justen > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.0 For the future: please use version 1.1 of the agreement: https://github.com/tianocore/edk2/commit/b6538c118ae8 > Signed-off-by: Brijesh Singh > --- > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h | 42 +-- > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c | 330 ++++++++++++++++---- > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 130 -------- > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c | 99 +++--- > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c | 58 +--- > 5 files changed, 367 insertions(+), 292 deletions(-) > [lersek@redhat.com: convert pointers to UINTN before converting to UINT64] [lersek@redhat.com: fix argument indentation in multi-line function call] [lersek@redhat.com: explicitly compare pointers to NULL] Reviewed-by: Laszlo Ersek Regression-tested-by: Laszlo Ersek Pushed as commit f6c909ae5d65. (See the fixups below, formatted with function context for easier understanding.) Thanks Laszlo > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > index 576941749cf2..8b98208591e6 100644 > --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > @@ -232,34 +232,34 @@ VOID > FreeFwCfgDmaAccessBuffer ( > IN VOID *Access, > IN VOID *Mapping > ) > { > UINTN NumPages; > EFI_STATUS Status; > > NumPages = EFI_SIZE_TO_PAGES (sizeof (FW_CFG_DMA_ACCESS)); > > Status = mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_ERROR, > "%a:%a failed to UnMap() Mapping 0x%Lx\n", gEfiCallerBaseName, > - __FUNCTION__, (UINT64)Mapping)); > + __FUNCTION__, (UINT64)(UINTN)Mapping)); > ASSERT (FALSE); > CpuDeadLoop (); > } > > Status = mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, Access); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_ERROR, > "%a:%a failed to Free() 0x%Lx\n", gEfiCallerBaseName, __FUNCTION__, > - (UINT64) Access)); > + (UINT64)(UINTN)Access)); > ASSERT (FALSE); > CpuDeadLoop (); > } > } > > /** > Function is used for mapping host address to device address. The buffer must > be unmapped with UnmapDmaDataBuffer (). > > **/ > @@ -268,44 +268,44 @@ VOID > MapFwCfgDmaDataBuffer ( > IN BOOLEAN IsWrite, > IN VOID *HostAddress, > IN UINT32 Size, > OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, > OUT VOID **MapInfo > ) > { > EFI_STATUS Status; > UINTN NumberOfBytes; > VOID *Mapping; > EFI_PHYSICAL_ADDRESS PhysicalAddress; > > NumberOfBytes = Size; > Status = mIoMmuProtocol->Map ( > mIoMmuProtocol, > (IsWrite ? > EdkiiIoMmuOperationBusMasterRead64 : > EdkiiIoMmuOperationBusMasterWrite64), > HostAddress, > &NumberOfBytes, > &PhysicalAddress, > &Mapping > ); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_ERROR, > "%a:%a failed to Map() Address 0x%Lx Size 0x%Lx\n", gEfiCallerBaseName, > - __FUNCTION__, (UINT64) HostAddress, (UINT64)Size)); > + __FUNCTION__, (UINT64)(UINTN)HostAddress, (UINT64)Size)); > ASSERT (FALSE); > CpuDeadLoop (); > } > > if (NumberOfBytes < Size) { > mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping); > DEBUG ((DEBUG_ERROR, > "%a:%a failed to Map() - requested 0x%x got 0x%Lx\n", gEfiCallerBaseName, > __FUNCTION__, Size, (UINT64)NumberOfBytes)); > ASSERT (FALSE); > CpuDeadLoop (); > } > > *DeviceAddress = PhysicalAddress; > *MapInfo = Mapping; > } > @@ -315,31 +315,31 @@ VOID > UnmapFwCfgDmaDataBuffer ( > IN VOID *Mapping > ) > { > EFI_STATUS Status; > > Status = mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_ERROR, > "%a:%a failed to UnMap() Mapping 0x%Lx\n", gEfiCallerBaseName, > - __FUNCTION__, (UINT64)Mapping)); > + __FUNCTION__, (UINT64)(UINTN)Mapping)); > ASSERT (FALSE); > CpuDeadLoop (); > } > } > > /** > Transfer an array of bytes, or skip a number of bytes, using the DMA > interface. > > @param[in] Size Size in bytes to transfer or skip. > > @param[in,out] Buffer Buffer to read data into or write data from. Ignored, > and may be NULL, if Size is zero, or Control is > FW_CFG_DMA_CTL_SKIP. > > @param[in] Control One of the following: > FW_CFG_DMA_CTL_WRITE - write to fw_cfg from Buffer. > FW_CFG_DMA_CTL_READ - read from fw_cfg into Buffer. > FW_CFG_DMA_CTL_SKIP - skip bytes in fw_cfg. > **/ > @@ -347,106 +347,106 @@ VOID > InternalQemuFwCfgDmaBytes ( > IN UINT32 Size, > IN OUT VOID *Buffer OPTIONAL, > IN UINT32 Control > ) > { > volatile FW_CFG_DMA_ACCESS LocalAccess; > volatile FW_CFG_DMA_ACCESS *Access; > UINT32 AccessHigh, AccessLow; > UINT32 Status; > VOID *AccessMapping, *DataMapping; > VOID *DataBuffer; > > ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ || > Control == FW_CFG_DMA_CTL_SKIP); > > if (Size == 0) { > return; > } > > Access = &LocalAccess; > AccessMapping = NULL; > DataMapping = NULL; > DataBuffer = Buffer; > > // > // When SEV is enabled, map Buffer to DMA address before issuing the DMA > // request > // > if (MemEncryptSevIsEnabled ()) { > VOID *AccessBuffer; > EFI_PHYSICAL_ADDRESS DataBufferAddress; > > // > // Allocate DMA Access buffer > // > AllocFwCfgDmaAccessBuffer (&AccessBuffer, &AccessMapping); > > Access = AccessBuffer; > > // > // Map actual data buffer > // > if (Control != FW_CFG_DMA_CTL_SKIP) { > MapFwCfgDmaDataBuffer ( > - Control == FW_CFG_DMA_CTL_WRITE, > - Buffer, > - Size, > - &DataBufferAddress, > - &DataMapping > - ); > + Control == FW_CFG_DMA_CTL_WRITE, > + Buffer, > + Size, > + &DataBufferAddress, > + &DataMapping > + ); > > DataBuffer = (VOID *) (UINTN) DataBufferAddress; > } > } > > Access->Control = SwapBytes32 (Control); > Access->Length = SwapBytes32 (Size); > Access->Address = SwapBytes64 ((UINTN)DataBuffer); > > // > // Delimit the transfer from (a) modifications to Access, (b) in case of a > // write, from writes to Buffer by the caller. > // > MemoryFence (); > > // > // Start the transfer. > // > AccessHigh = (UINT32)RShiftU64 ((UINTN)Access, 32); > AccessLow = (UINT32)(UINTN)Access; > IoWrite32 (FW_CFG_IO_DMA_ADDRESS, SwapBytes32 (AccessHigh)); > IoWrite32 (FW_CFG_IO_DMA_ADDRESS + 4, SwapBytes32 (AccessLow)); > > // > // Don't look at Access.Control before starting the transfer. > // > MemoryFence (); > > // > // Wait for the transfer to complete. > // > do { > Status = SwapBytes32 (Access->Control); > ASSERT ((Status & FW_CFG_DMA_CTL_ERROR) == 0); > } while (Status != 0); > > // > // After a read, the caller will want to use Buffer. > // > MemoryFence (); > > // > // If Access buffer was dynamically allocated then free it. > // > - if (AccessMapping) { > + if (AccessMapping != NULL) { > FreeFwCfgDmaAccessBuffer ((VOID *)Access, AccessMapping); > } > > // > // If DataBuffer was mapped then unmap it. > // > - if (DataMapping) { > + if (DataMapping != NULL) { > UnmapFwCfgDmaDataBuffer (DataMapping); > } > }