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 <[email protected]> > Cc: Jordan Justen <[email protected]> > Cc: Laszlo Ersek <[email protected]> > 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 <[email protected]> > --- > 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(-) > [[email protected]: convert pointers to UINTN before converting to UINT64] [[email protected]: fix argument indentation in multi-line function call] [[email protected]: explicitly compare pointers to NULL] Reviewed-by: Laszlo Ersek <[email protected]> Regression-tested-by: Laszlo Ersek <[email protected]> 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); > } > } _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
