* [PATCH v2 0/1] OvmfPkg/QemuFwCfgLib: Use BusMasterCommandBuffer to map FW_CFG_DMA_ACCESS
@ 2017-08-03 16:09 Brijesh Singh
2017-08-03 16:10 ` [PATCH v2 1/1] OvmfPkg : QemuFwCfgLib: " Brijesh Singh
0 siblings, 1 reply; 9+ messages in thread
From: Brijesh Singh @ 2017-08-03 16:09 UTC (permalink / raw)
To: edk2-devel; +Cc: Tom Lendacky, Ard Biesheuvel, Brijesh Singh
The patch applies on top of Laszlo's IoMmuDxe cleanup series [1].
Commit is also available at https://github.com/codomania/edk2/tree/qemufwcfg-sev
[1] https://lists.01.org/pipermail/edk2-devel/2017-August/012808.html
Changes since v1:
* Drop Patch 1 through 3, they are covered by Laszlo's cleanup series
* Rebase to the latest
Brijesh Singh (1):
OvmfPkg : QemuFwCfgLib: Use BusMasterCommandBuffer to map
FW_CFG_DMA_ACCESS
OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h | 42 ++--
OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c | 247 ++++++++++++++++----
OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 130 -----------
OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c | 101 +++++---
OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c | 56 ++---
5 files changed, 292 insertions(+), 284 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2 1/1] OvmfPkg : QemuFwCfgLib: Use BusMasterCommandBuffer to map FW_CFG_DMA_ACCESS 2017-08-03 16:09 [PATCH v2 0/1] OvmfPkg/QemuFwCfgLib: Use BusMasterCommandBuffer to map FW_CFG_DMA_ACCESS Brijesh Singh @ 2017-08-03 16:10 ` Brijesh Singh 2017-08-03 20:51 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Brijesh Singh @ 2017-08-03 16:10 UTC (permalink / raw) To: edk2-devel Cc: Tom Lendacky, Ard Biesheuvel, Brijesh Singh, Jordan Justen, Laszlo Ersek 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 <lersek@redhat.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h | 42 ++-- OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c | 247 ++++++++++++++++---- OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 130 ----------- OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c | 101 +++++--- OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c | 56 ++--- 5 files changed, 292 insertions(+), 284 deletions(-) diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h index 8cfa7913ffae..327f1d37e17d 100644 --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h @@ -45,39 +45,25 @@ InternalQemuFwCfgDmaIsAvailable ( ); /** - Returns a boolean indicating whether SEV support is enabled + Transfer an array of bytes, or skip a number of bytes, using the DMA + interface. - @retval TRUE SEV is enabled - @retval FALSE SEV is disabled -**/ -BOOLEAN -InternalQemuFwCfgSevIsEnabled ( - VOID - ); + @param[in] Size Size in bytes to transfer or skip. -/** - Allocate a bounce buffer for SEV DMA. - - @param[out] Buffer Allocated DMA Buffer pointer - @param[in] NumPage Number of pages. + @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. **/ VOID -InternalQemuFwCfgSevDmaAllocateBuffer ( - OUT VOID **Buffer, - IN UINT32 NumPages +InternalQemuFwCfgDmaBytes ( + IN UINT32 Size, + IN OUT VOID *Buffer OPTIONAL, + IN UINT32 Control ); -/** - 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 - ); #endif diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c index f8eb03bc3a9a..e03647bae3bd 100644 --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c @@ -20,6 +20,7 @@ #include <Protocol/IoMmu.h> #include <Library/BaseLib.h> +#include <Library/IoLib.h> #include <Library/DebugLib.h> #include <Library/QemuFwCfgLib.h> #include <Library/UefiBootServicesTableLib.h> @@ -157,74 +158,228 @@ InternalQemuFwCfgDmaIsAvailable ( } /** - Allocate a bounce buffer for SEV DMA. - - @param[in] NumPage Number of pages. - @param[out] Buffer Allocated DMA Buffer pointer + Function is used for allocating a bi-directional FW_CFG_DMA_ACCESS used + between Host and device to exchange the information. The buffer must be free'd + using FreeFwCfgDmaAccessBuffer (). **/ -VOID -InternalQemuFwCfgSevDmaAllocateBuffer ( - OUT VOID **Buffer, - IN UINT32 NumPages +STATIC +EFI_STATUS +AllocFwCfgDmaAccessBuffer ( + OUT VOID **Access, + OUT VOID **Mapping ) { - EFI_STATUS Status; + UINTN Size; + UINTN NumPages; + EFI_STATUS Status; + VOID *HostAddress; + EFI_PHYSICAL_ADDRESS DmaAddress; - ASSERT (mIoMmuProtocol != NULL); + Size = sizeof (FW_CFG_DMA_ACCESS); + NumPages = EFI_SIZE_TO_PAGES (Size); + // + // As per UEFI spec, in order to map a host address with BusMasterCommomBuffer64, + // the buffer must be allocated using the IOMMU AllocateBuffer() + // Status = mIoMmuProtocol->AllocateBuffer ( - mIoMmuProtocol, - 0, - EfiBootServicesData, - NumPages, - Buffer, - EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED + mIoMmuProtocol, + AllocateAnyPages, + EfiBootServicesData, + NumPages, + &HostAddress, + EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED ); + + // + // Map the host buffer with BusMasterCommonBuffer64 + // + Status = mIoMmuProtocol->Map ( + mIoMmuProtocol, + EdkiiIoMmuOperationBusMasterCommonBuffer64, + HostAddress, + &Size, + &DmaAddress, + Mapping + ); if (EFI_ERROR (Status)) { - DEBUG ((DEBUG_ERROR, - "%a:%a failed to allocate %u pages\n", gEfiCallerBaseName, __FUNCTION__, - NumPages)); - ASSERT (FALSE); - CpuDeadLoop (); + mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, HostAddress); } - DEBUG ((DEBUG_VERBOSE, - "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName, __FUNCTION__, - (UINT64)(UINTN)Buffer, NumPages)); + *Access = HostAddress; + return Status; +} + +/** + Function is to used for freeing the Access buffer allocated using + AllocFwCfgDmaAccessBuffer() + +**/ +STATIC +VOID +FreeFwCfgDmaAccessBuffer ( + IN VOID *Access, + IN VOID *Mapping + ) +{ + UINTN NumPages; + + NumPages = EFI_SIZE_TO_PAGES (sizeof (FW_CFG_DMA_ACCESS)); + + mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping); + + mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, Access); +} + +/** + Function is used for mapping host address to device address. The buffer must + be unmapped with UnmapDmaDataBuffer (). + +**/ +STATIC +EFI_STATUS +MapFwCfgDmaDataBuffer ( + IN BOOLEAN IsWrite, + IN VOID *HostAddress, + IN UINT32 Size, + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, + OUT VOID **Mapping + ) +{ + EFI_STATUS Status; + UINTN NumberOfBytes; + + NumberOfBytes = Size; + Status = mIoMmuProtocol->Map ( + mIoMmuProtocol, + IsWrite ? EdkiiIoMmuOperationBusMasterRead64 : EdkiiIoMmuOperationBusMasterWrite64, + HostAddress, + &NumberOfBytes, + DeviceAddress, + Mapping + ); + return Status; +} + +EFI_STATUS +UnmapFwCfgDmaDataBuffer ( + IN VOID *Mapping + ) +{ + return mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping); } /** - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer + Transfer an array of bytes, or skip a number of bytes, using the DMA + interface. - @param[in] NumPage Number of pages. - @param[in] Buffer DMA Buffer pointer + @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. **/ VOID -InternalQemuFwCfgSevDmaFreeBuffer ( - IN VOID *Buffer, - IN UINT32 NumPages +InternalQemuFwCfgDmaBytes ( + IN UINT32 Size, + IN OUT VOID *Buffer OPTIONAL, + IN UINT32 Control ) { - EFI_STATUS Status; + volatile FW_CFG_DMA_ACCESS LocalAccess; + volatile FW_CFG_DMA_ACCESS *Access; + UINT32 AccessHigh, AccessLow; + UINT32 Status; + VOID *AccessMapping, *DataMapping; - ASSERT (mIoMmuProtocol != NULL); + ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ || + Control == FW_CFG_DMA_CTL_SKIP); - 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 (); + if (Size == 0) { + return; } - DEBUG ((DEBUG_VERBOSE, - "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName,__FUNCTION__, - (UINT64)(UINTN)Buffer, NumPages)); + // + // When SEV is enabled, map Buffer to DMA address before issuing the DMA request + // + if (MemEncryptSevIsEnabled ()) { + VOID *AccessBuffer; + EFI_PHYSICAL_ADDRESS DataBuffer; + + // + // Allocate DMA Access buffer + // + Status = AllocFwCfgDmaAccessBuffer (&AccessBuffer, &AccessMapping); + ASSERT_EFI_ERROR (Status); + + Access = AccessBuffer; + + // + // Map actual data buffer + // + if (Buffer) { + Status = MapFwCfgDmaDataBuffer (Control == FW_CFG_DMA_CTL_WRITE ? 1 : 0, + Buffer, Size, &DataBuffer, &DataMapping); + ASSERT_EFI_ERROR (Status); + + Buffer = (VOID *) (UINTN) DataBuffer; + } + } else { + Access = &LocalAccess; + AccessMapping = NULL; + DataMapping = NULL; + } + + Access->Control = SwapBytes32 (Control); + Access->Length = SwapBytes32 (Size); + Access->Address = SwapBytes64 ((UINTN)Buffer); + + // + // 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) { + FreeFwCfgDmaAccessBuffer ((VOID *)Access, AccessMapping); + } + + if (DataMapping) { + UnmapFwCfgDmaDataBuffer (DataMapping); + } } diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c index d3bf75498d60..7f42f38d1c05 100644 --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c @@ -45,136 +45,6 @@ QemuFwCfgSelectItem ( IoWrite16 (FW_CFG_IO_SELECTOR, (UINT16)(UINTN) QemuFwCfgItem); } - -/** - 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. -**/ -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; - UINT32 NumPages; - VOID *DmaBuffer, *BounceBuffer; - - ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ || - Control == FW_CFG_DMA_CTL_SKIP); - - if (Size == 0) { - return; - } - - // - // set NumPages to suppress incorrect compiler/analyzer warnings - // - NumPages = 0; - - // - // When SEV is enabled then allocate DMA bounce buffer - // - if (InternalQemuFwCfgSevIsEnabled ()) { - UINTN TotalSize; - - TotalSize = sizeof (*Access); - // - // Skip operation does not need buffer - // - if (Control != FW_CFG_DMA_CTL_SKIP) { - TotalSize += Size; - } - - // - // Allocate SEV DMA buffer - // - NumPages = (UINT32)EFI_SIZE_TO_PAGES (TotalSize); - InternalQemuFwCfgSevDmaAllocateBuffer (&BounceBuffer, NumPages); - - Access = BounceBuffer; - DmaBuffer = (UINT8*)BounceBuffer + sizeof (*Access); - - // - // Decrypt data from encrypted guest buffer into DMA buffer - // - if (Control == FW_CFG_DMA_CTL_WRITE) { - CopyMem (DmaBuffer, Buffer, Size); - } - } else { - Access = &LocalAccess; - DmaBuffer = Buffer; - BounceBuffer = NULL; - } - - Access->Control = SwapBytes32 (Control); - Access->Length = SwapBytes32 (Size); - Access->Address = SwapBytes64 ((UINTN)DmaBuffer); - - // - // 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 Bounce buffer was allocated then copy the data into guest buffer and - // free the bounce buffer - // - if (BounceBuffer != NULL) { - // - // Encrypt the data from DMA buffer into guest buffer - // - if (Control == FW_CFG_DMA_CTL_READ) { - CopyMem (Buffer, DmaBuffer, Size); - } - - InternalQemuFwCfgSevDmaFreeBuffer (BounceBuffer, NumPages); - } -} - - /** Reads firmware configuration bytes into a buffer diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c index 40f89c3b53e2..bc649b40aec3 100644 --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c @@ -16,6 +16,7 @@ **/ #include <Library/BaseLib.h> +#include <Library/IoLib.h> #include <Library/DebugLib.h> #include <Library/QemuFwCfgLib.h> #include <Library/MemEncryptSevLib.h> @@ -85,7 +86,7 @@ QemuFwCfgInitialize ( // (which need to allocate dynamic memory and allocating a PAGE size'd // buffer can be challenge in PEI phase) // - if (InternalQemuFwCfgSevIsEnabled ()) { + if (MemEncryptSevIsEnabled ()) { DEBUG ((DEBUG_INFO, "SEV: QemuFwCfg fallback to IO Port interface.\n")); } else { mQemuFwCfgDmaSupported = TRUE; @@ -129,56 +130,80 @@ InternalQemuFwCfgDmaIsAvailable ( } /** + Transfer an array of bytes, or skip a number of bytes, using the DMA + interface. - Returns a boolean indicating whether SEV is enabled + @param[in] Size Size in bytes to transfer or skip. - @retval TRUE SEV is enabled - @retval FALSE SEV is disabled + @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. **/ -BOOLEAN -InternalQemuFwCfgSevIsEnabled ( - VOID +VOID +InternalQemuFwCfgDmaBytes ( + IN UINT32 Size, + IN OUT VOID *Buffer OPTIONAL, + IN UINT32 Control ) { - return MemEncryptSevIsEnabled (); -} + volatile FW_CFG_DMA_ACCESS Access; + UINT32 AccessHigh, AccessLow; + UINT32 Status; -/** - Allocate a bounce buffer for SEV DMA. + ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ || + Control == FW_CFG_DMA_CTL_SKIP); - @param[in] NumPage Number of pages. - @param[out] Buffer Allocated DMA Buffer pointer + if (Size == 0) { + return; + } + + if (MemEncryptSevIsEnabled ()) { + // + // SEV does not support DMA operations in PEI stage, we should + // not have reached here. + // + ASSERT (FALSE); + } + + Access.Control = SwapBytes32 (Control); + Access.Length = SwapBytes32 (Size); + Access.Address = SwapBytes64 ((UINTN)Buffer); -**/ -VOID -InternalQemuFwCfgSevDmaAllocateBuffer ( - OUT VOID **Buffer, - IN UINT32 NumPages - ) -{ // - // We should never reach here + // Delimit the transfer from (a) modifications to Access, (b) in case of a + // write, from writes to Buffer by the caller. // - ASSERT (FALSE); - CpuDeadLoop (); -} + 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)); -/** - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer + // + // Don't look at Access.Control before starting the transfer. + // + MemoryFence (); - @param[in] NumPage Number of pages. - @param[in] Buffer DMA Buffer pointer + // + // Wait for the transfer to complete. + // + do { + Status = SwapBytes32 (Access.Control); + ASSERT ((Status & FW_CFG_DMA_CTL_ERROR) == 0); + } while (Status != 0); -**/ -VOID -InternalQemuFwCfgSevDmaFreeBuffer ( - IN VOID *Buffer, - IN UINT32 NumPages - ) -{ // - // We should never reach here + // After a read, the caller will want to use Buffer. // - ASSERT (FALSE); - CpuDeadLoop (); + MemoryFence (); } + diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c index 071b8d9b91d4..62ddb69d3b4d 100644 --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c @@ -97,53 +97,25 @@ InternalQemuFwCfgDmaIsAvailable ( } /** + Transfer an array of bytes, or skip a number of bytes, using the DMA + interface. - Returns a boolean indicating whether SEV is enabled + @param[in] Size Size in bytes to transfer or skip. - @retval TRUE SEV is enabled - @retval FALSE SEV is disabled -**/ -BOOLEAN -InternalQemuFwCfgSevIsEnabled ( - VOID - ) -{ - // - // DMA is not supported in SEC phase hence SEV support is irrelevant - // - return FALSE; -} - -/** - 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 - ) -{ - // - // We should never reach here - // - ASSERT (FALSE); -} - -/** - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer - - @param[in] NumPage Number of pages. - @param[in] Buffer DMA Buffer pointer + @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. **/ VOID -InternalQemuFwCfgSevDmaFreeBuffer ( - IN VOID *Buffer, - IN UINT32 NumPages +InternalQemuFwCfgDmaBytes ( + IN UINT32 Size, + IN OUT VOID *Buffer OPTIONAL, + IN UINT32 Control ) { // -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] OvmfPkg : QemuFwCfgLib: Use BusMasterCommandBuffer to map FW_CFG_DMA_ACCESS 2017-08-03 16:10 ` [PATCH v2 1/1] OvmfPkg : QemuFwCfgLib: " Brijesh Singh @ 2017-08-03 20:51 ` Laszlo Ersek 2017-08-03 22:07 ` Brijesh Singh 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2017-08-03 20:51 UTC (permalink / raw) To: Brijesh Singh, edk2-devel; +Cc: Tom Lendacky, Jordan Justen, Ard Biesheuvel On 08/03/17 18:10, 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 <lersek@redhat.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h | 42 ++-- > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c | 247 ++++++++++++++++---- > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 130 ----------- > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c | 101 +++++--- > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c | 56 ++--- > 5 files changed, 292 insertions(+), 284 deletions(-) (1) please remove the space characters in front of the colons (":") in the subject. (2) Still for the subject, it is "CommonBuffer", not "CommandBuffer". So the subject should be OvmfPkg: QemuFwCfgLib: Use BusMasterCommonBuffer to map FW_CFG_DMA_ACCESS (3) Please rewrap the commit message to 74 characters. > > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h > index 8cfa7913ffae..327f1d37e17d 100644 > --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h > +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h > @@ -45,39 +45,25 @@ InternalQemuFwCfgDmaIsAvailable ( > ); > > /** > - Returns a boolean indicating whether SEV support is enabled > + Transfer an array of bytes, or skip a number of bytes, using the DMA > + interface. > > - @retval TRUE SEV is enabled > - @retval FALSE SEV is disabled > -**/ > -BOOLEAN > -InternalQemuFwCfgSevIsEnabled ( > - VOID > - ); > + @param[in] Size Size in bytes to transfer or skip. > > -/** > - Allocate a bounce buffer for SEV DMA. > - > - @param[out] Buffer Allocated DMA Buffer pointer > - @param[in] NumPage Number of pages. > + @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. > **/ > VOID > -InternalQemuFwCfgSevDmaAllocateBuffer ( > - OUT VOID **Buffer, > - IN UINT32 NumPages > +InternalQemuFwCfgDmaBytes ( > + IN UINT32 Size, > + IN OUT VOID *Buffer OPTIONAL, > + IN UINT32 Control > ); > > -/** > - 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 > - ); > #endif > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c (4) You forgot to remove the InternalQemuFwCfgSevIsEnabled() function from this file. > index f8eb03bc3a9a..e03647bae3bd 100644 > --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > @@ -20,6 +20,7 @@ > #include <Protocol/IoMmu.h> > > #include <Library/BaseLib.h> > +#include <Library/IoLib.h> > #include <Library/DebugLib.h> > #include <Library/QemuFwCfgLib.h> > #include <Library/UefiBootServicesTableLib.h> > @@ -157,74 +158,228 @@ InternalQemuFwCfgDmaIsAvailable ( > } > > /** > - Allocate a bounce buffer for SEV DMA. > - > - @param[in] NumPage Number of pages. > - @param[out] Buffer Allocated DMA Buffer pointer > + Function is used for allocating a bi-directional FW_CFG_DMA_ACCESS used > + between Host and device to exchange the information. The buffer must be free'd > + using FreeFwCfgDmaAccessBuffer (). > > **/ > -VOID > -InternalQemuFwCfgSevDmaAllocateBuffer ( > - OUT VOID **Buffer, > - IN UINT32 NumPages > +STATIC > +EFI_STATUS > +AllocFwCfgDmaAccessBuffer ( > + OUT VOID **Access, > + OUT VOID **Mapping > ) > { > - EFI_STATUS Status; > + UINTN Size; > + UINTN NumPages; > + EFI_STATUS Status; > + VOID *HostAddress; > + EFI_PHYSICAL_ADDRESS DmaAddress; > > - ASSERT (mIoMmuProtocol != NULL); > + Size = sizeof (FW_CFG_DMA_ACCESS); > + NumPages = EFI_SIZE_TO_PAGES (Size); > > + // > + // As per UEFI spec, in order to map a host address with BusMasterCommomBuffer64, (5) This line is too long. Also, you have one too many space chars in front of the comma. > + // the buffer must be allocated using the IOMMU AllocateBuffer() > + // > Status = mIoMmuProtocol->AllocateBuffer ( > - mIoMmuProtocol, > - 0, > - EfiBootServicesData, > - NumPages, > - Buffer, > - EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED > + mIoMmuProtocol, > + AllocateAnyPages, > + EfiBootServicesData, > + NumPages, > + &HostAddress, > + EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED (6) Please OR this attribute with EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE, so that the buffer can be allocated from 64-bit memory. > ); (7) The closing paren should be aligned with the arguments. (8) Please handle any errors returned by AllocateBuffer(), by propagating the error. > + > + // > + // Map the host buffer with BusMasterCommonBuffer64 > + // > + Status = mIoMmuProtocol->Map ( > + mIoMmuProtocol, > + EdkiiIoMmuOperationBusMasterCommonBuffer64, > + HostAddress, > + &Size, > + &DmaAddress, > + Mapping > + ); (9) Please add a check here that, on success, Size is still equal to sizeof(FW_CFG_DMA_ACCESS). (Theoretically it could be smaller, even on success.) If it is smaller, then both the map and allocate operations should be rolled back, and we should return EFI_OUT_OF_RESOURCES. For this, error handling labels at the end of the function are likely the best. (10) We should only set the output parameters Mapping and Access only if the entire function succeeds. > if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, > - "%a:%a failed to allocate %u pages\n", gEfiCallerBaseName, __FUNCTION__, > - NumPages)); > - ASSERT (FALSE); > - CpuDeadLoop (); > + mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, HostAddress); > } > > - DEBUG ((DEBUG_VERBOSE, > - "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName, __FUNCTION__, > - (UINT64)(UINTN)Buffer, NumPages)); > + *Access = HostAddress; > + return Status; > +} > + > +/** > + Function is to used for freeing the Access buffer allocated using > + AllocFwCfgDmaAccessBuffer() > + > +**/ > +STATIC > +VOID > +FreeFwCfgDmaAccessBuffer ( > + IN VOID *Access, > + IN VOID *Mapping > + ) > +{ > + UINTN NumPages; > + > + NumPages = EFI_SIZE_TO_PAGES (sizeof (FW_CFG_DMA_ACCESS)); > + > + mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping); > + > + mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, Access); > +} > + > +/** > + Function is used for mapping host address to device address. The buffer must > + be unmapped with UnmapDmaDataBuffer (). > + > +**/ > +STATIC > +EFI_STATUS > +MapFwCfgDmaDataBuffer ( > + IN BOOLEAN IsWrite, > + IN VOID *HostAddress, > + IN UINT32 Size, > + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, > + OUT VOID **Mapping > + ) > +{ > + EFI_STATUS Status; > + UINTN NumberOfBytes; > + > + NumberOfBytes = Size; > + Status = mIoMmuProtocol->Map ( > + mIoMmuProtocol, > + IsWrite ? EdkiiIoMmuOperationBusMasterRead64 : EdkiiIoMmuOperationBusMasterWrite64, (11) Please make sure that the file is not wider than 80 characters -- the above can be broken up like (IsWrite ? EdkiiIoMmuOperationBusMasterRead64 : EdkiiIoMmuOperationBusMasterWrite64) And if that's still too wide, then a separate variable should be used. > + HostAddress, > + &NumberOfBytes, > + DeviceAddress, > + Mapping > + ); (12) Same comment as (9): on success, we should check that NumberOfBytes has the original value (i.e., Size). Otherwise, roll back the Map(), and return EFI_OUT_OF_RESOURCES. (13) Same comment as (10): we should only set the output parameters (DeviceAddress and Mapping) if the entire function succeeds. (14) Here's an alternative to the explicit / cascading error handling recommended above: given that all of these functions will be called from InternalQemuFwCfgSevDmaFreeBuffer(), which cannot propagate errors anyway, you might as well hang immediately upon error. For this, please preserve the following triplet, used earlier in InternalQemuFwCfgSevDmaAllocateBuffer(): - DEBUG_ERROR message with gEfiCallerBaseName, - ASSERT (FALSE), - CpuDeadLoop(). Correspondingly, none of the helper functions would have to return EFI_STATUS. > + return Status; > +} > + > +EFI_STATUS > +UnmapFwCfgDmaDataBuffer ( > + IN VOID *Mapping > + ) > +{ > + return mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping); > } (15) I think it's fine if we ignore (don't propagate) the error here -- please change the return type to VOID (similarly to FreeFwCfgDmaAccessBuffer()). (16) Please make this function STATIC. > > /** > - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer > + Transfer an array of bytes, or skip a number of bytes, using the DMA > + interface. > > - @param[in] NumPage Number of pages. > - @param[in] Buffer DMA Buffer pointer > + @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. > **/ > VOID > -InternalQemuFwCfgSevDmaFreeBuffer ( > - IN VOID *Buffer, > - IN UINT32 NumPages > +InternalQemuFwCfgDmaBytes ( > + IN UINT32 Size, > + IN OUT VOID *Buffer OPTIONAL, > + IN UINT32 Control > ) > { > - EFI_STATUS Status; > + volatile FW_CFG_DMA_ACCESS LocalAccess; > + volatile FW_CFG_DMA_ACCESS *Access; > + UINT32 AccessHigh, AccessLow; > + UINT32 Status; > + VOID *AccessMapping, *DataMapping; > > - ASSERT (mIoMmuProtocol != NULL); > + ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ || > + Control == FW_CFG_DMA_CTL_SKIP); > > - 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 (); > + if (Size == 0) { > + return; > } > > - DEBUG ((DEBUG_VERBOSE, > - "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName,__FUNCTION__, > - (UINT64)(UINTN)Buffer, NumPages)); > + // > + // When SEV is enabled, map Buffer to DMA address before issuing the DMA request (17) This line is too long. > + // > + if (MemEncryptSevIsEnabled ()) { > + VOID *AccessBuffer; > + EFI_PHYSICAL_ADDRESS DataBuffer; > + > + // > + // Allocate DMA Access buffer > + // > + Status = AllocFwCfgDmaAccessBuffer (&AccessBuffer, &AccessMapping); > + ASSERT_EFI_ERROR (Status); (18) You can drop the assert according to cmment (14). > + > + Access = AccessBuffer; > + > + // > + // Map actual data buffer > + // > + if (Buffer) { (19) Please don't rely on Buffer being NULL for SKIP operations; that may or may not be true. Instead, please check for (Control != FW_CFG_DMA_CTL_SKIP) > + Status = MapFwCfgDmaDataBuffer (Control == FW_CFG_DMA_CTL_WRITE ? 1 : 0, > + Buffer, Size, &DataBuffer, &DataMapping); (20) Please break every argument to a separate line. (21) The following expression: Control == FW_CFG_DMA_CTL_WRITE ? 1 : 0 should be simplified as: Control == FW_CFG_DMA_CTL_WRITE > + ASSERT_EFI_ERROR (Status); (22) You can drop this, according to (14). > + > + Buffer = (VOID *) (UINTN) DataBuffer; (23) Ugh, please don't overwrite a function parameter. Instead, please introduce an additional variable. For example, you could rename "DataBuffer" to "DataBufferAddress", and introduce "DataBuffer" as a (VOID*). In the non-SEV case, you could set DataBuffer to Buffer, and in the SEV case, you could set DataBuffer to (VOID*)(UINTN)DataBufferAddress. Then use DataBuffer for setting Access->Address. > + } > + } else { > + Access = &LocalAccess; > + AccessMapping = NULL; > + DataMapping = NULL; > + } > + > + Access->Control = SwapBytes32 (Control); > + Access->Length = SwapBytes32 (Size); > + Access->Address = SwapBytes64 ((UINTN)Buffer); > + > + // > + // 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) { > + FreeFwCfgDmaAccessBuffer ((VOID *)Access, AccessMapping); > + } > + > + if (DataMapping) { > + UnmapFwCfgDmaDataBuffer (DataMapping); > + } (24) The DataMapping check is not valid. You have a code path where DataMapping is not set: Buffer==NULL (i.e., SKIP operation) with SEV enabled. I guess the simplest way to fix this is to eliminate the non-SEV branch near the top, and make all those assignments unconditional: Access = &LocalAccess; AccessMapping = NULL; DataMapping = NULL; DataBuffer = Buffer; // // When SEV is enabled... // if (MemEncryptSevIsEnabled ()) { ... > } > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > index d3bf75498d60..7f42f38d1c05 100644 > --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > @@ -45,136 +45,6 @@ QemuFwCfgSelectItem ( > IoWrite16 (FW_CFG_IO_SELECTOR, (UINT16)(UINTN) QemuFwCfgItem); > } > > - > -/** > - 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. > -**/ > -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; > - UINT32 NumPages; > - VOID *DmaBuffer, *BounceBuffer; > - > - ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ || > - Control == FW_CFG_DMA_CTL_SKIP); > - > - if (Size == 0) { > - return; > - } > - > - // > - // set NumPages to suppress incorrect compiler/analyzer warnings > - // > - NumPages = 0; > - > - // > - // When SEV is enabled then allocate DMA bounce buffer > - // > - if (InternalQemuFwCfgSevIsEnabled ()) { > - UINTN TotalSize; > - > - TotalSize = sizeof (*Access); > - // > - // Skip operation does not need buffer > - // > - if (Control != FW_CFG_DMA_CTL_SKIP) { > - TotalSize += Size; > - } > - > - // > - // Allocate SEV DMA buffer > - // > - NumPages = (UINT32)EFI_SIZE_TO_PAGES (TotalSize); > - InternalQemuFwCfgSevDmaAllocateBuffer (&BounceBuffer, NumPages); > - > - Access = BounceBuffer; > - DmaBuffer = (UINT8*)BounceBuffer + sizeof (*Access); > - > - // > - // Decrypt data from encrypted guest buffer into DMA buffer > - // > - if (Control == FW_CFG_DMA_CTL_WRITE) { > - CopyMem (DmaBuffer, Buffer, Size); > - } > - } else { > - Access = &LocalAccess; > - DmaBuffer = Buffer; > - BounceBuffer = NULL; > - } > - > - Access->Control = SwapBytes32 (Control); > - Access->Length = SwapBytes32 (Size); > - Access->Address = SwapBytes64 ((UINTN)DmaBuffer); > - > - // > - // 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 Bounce buffer was allocated then copy the data into guest buffer and > - // free the bounce buffer > - // > - if (BounceBuffer != NULL) { > - // > - // Encrypt the data from DMA buffer into guest buffer > - // > - if (Control == FW_CFG_DMA_CTL_READ) { > - CopyMem (Buffer, DmaBuffer, Size); > - } > - > - InternalQemuFwCfgSevDmaFreeBuffer (BounceBuffer, NumPages); > - } > -} > - > - > /** > Reads firmware configuration bytes into a buffer > > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c > index 40f89c3b53e2..bc649b40aec3 100644 > --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c > +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c > @@ -16,6 +16,7 @@ > **/ > > #include <Library/BaseLib.h> > +#include <Library/IoLib.h> > #include <Library/DebugLib.h> > #include <Library/QemuFwCfgLib.h> > #include <Library/MemEncryptSevLib.h> > @@ -85,7 +86,7 @@ QemuFwCfgInitialize ( > // (which need to allocate dynamic memory and allocating a PAGE size'd > // buffer can be challenge in PEI phase) > // > - if (InternalQemuFwCfgSevIsEnabled ()) { > + if (MemEncryptSevIsEnabled ()) { > DEBUG ((DEBUG_INFO, "SEV: QemuFwCfg fallback to IO Port interface.\n")); > } else { > mQemuFwCfgDmaSupported = TRUE; > @@ -129,56 +130,80 @@ InternalQemuFwCfgDmaIsAvailable ( > } > > /** > + Transfer an array of bytes, or skip a number of bytes, using the DMA > + interface. > > - Returns a boolean indicating whether SEV is enabled > + @param[in] Size Size in bytes to transfer or skip. > > - @retval TRUE SEV is enabled > - @retval FALSE SEV is disabled > + @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. > **/ > -BOOLEAN > -InternalQemuFwCfgSevIsEnabled ( > - VOID > +VOID > +InternalQemuFwCfgDmaBytes ( > + IN UINT32 Size, > + IN OUT VOID *Buffer OPTIONAL, > + IN UINT32 Control > ) > { > - return MemEncryptSevIsEnabled (); > -} > + volatile FW_CFG_DMA_ACCESS Access; > + UINT32 AccessHigh, AccessLow; > + UINT32 Status; > > -/** > - Allocate a bounce buffer for SEV DMA. > + ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ || > + Control == FW_CFG_DMA_CTL_SKIP); > > - @param[in] NumPage Number of pages. > - @param[out] Buffer Allocated DMA Buffer pointer > + if (Size == 0) { > + return; > + } > + > + if (MemEncryptSevIsEnabled ()) { > + // > + // SEV does not support DMA operations in PEI stage, we should > + // not have reached here. > + // > + ASSERT (FALSE); > + } (25) This is good; you are basically restoring / copying the pre-7cfe445d7f4b state of the InternalQemuFwCfgDmaBytes() function, plus adding this assert. But, it can be written better like this: // // ... keep the same comment ... // ASSERT (!MemEncryptSevIsEnabled ()); > + > + Access.Control = SwapBytes32 (Control); > + Access.Length = SwapBytes32 (Size); > + Access.Address = SwapBytes64 ((UINTN)Buffer); > > -**/ > -VOID > -InternalQemuFwCfgSevDmaAllocateBuffer ( > - OUT VOID **Buffer, > - IN UINT32 NumPages > - ) > -{ > // > - // We should never reach here > + // Delimit the transfer from (a) modifications to Access, (b) in case of a > + // write, from writes to Buffer by the caller. > // > - ASSERT (FALSE); > - CpuDeadLoop (); > -} > + 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)); > > -/** > - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer > + // > + // Don't look at Access.Control before starting the transfer. > + // > + MemoryFence (); > > - @param[in] NumPage Number of pages. > - @param[in] Buffer DMA Buffer pointer > + // > + // Wait for the transfer to complete. > + // > + do { > + Status = SwapBytes32 (Access.Control); > + ASSERT ((Status & FW_CFG_DMA_CTL_ERROR) == 0); > + } while (Status != 0); > > -**/ > -VOID > -InternalQemuFwCfgSevDmaFreeBuffer ( > - IN VOID *Buffer, > - IN UINT32 NumPages > - ) > -{ > // > - // We should never reach here > + // After a read, the caller will want to use Buffer. > // > - ASSERT (FALSE); > - CpuDeadLoop (); > + MemoryFence (); > } > + > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c > index 071b8d9b91d4..62ddb69d3b4d 100644 > --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c > +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c > @@ -97,53 +97,25 @@ InternalQemuFwCfgDmaIsAvailable ( > } > > /** > + Transfer an array of bytes, or skip a number of bytes, using the DMA > + interface. > > - Returns a boolean indicating whether SEV is enabled > + @param[in] Size Size in bytes to transfer or skip. > > - @retval TRUE SEV is enabled > - @retval FALSE SEV is disabled > -**/ > -BOOLEAN > -InternalQemuFwCfgSevIsEnabled ( > - VOID > - ) > -{ > - // > - // DMA is not supported in SEC phase hence SEV support is irrelevant > - // > - return FALSE; > -} > - > -/** > - 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 > - ) > -{ > - // > - // We should never reach here > - // > - ASSERT (FALSE); > -} > - > -/** > - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer > - > - @param[in] NumPage Number of pages. > - @param[in] Buffer DMA Buffer pointer > + @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. > **/ > VOID > -InternalQemuFwCfgSevDmaFreeBuffer ( > - IN VOID *Buffer, > - IN UINT32 NumPages > +InternalQemuFwCfgDmaBytes ( > + IN UINT32 Size, > + IN OUT VOID *Buffer OPTIONAL, > + IN UINT32 Control > ) > { > // > (26) Can you add a CpuDeadLoop() here as well (i.e., in the SEC instance), after the ASSERT()? I know the previous version of the SEC instance doesn't use CpuDeadLoop(), but I mentioned it here: http://mid.mail-archive.com/2b9361cc-03f9-a403-98aa-c1cf5bfd17c0@redhat.com Thanks! Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] OvmfPkg : QemuFwCfgLib: Use BusMasterCommandBuffer to map FW_CFG_DMA_ACCESS 2017-08-03 20:51 ` Laszlo Ersek @ 2017-08-03 22:07 ` Brijesh Singh 2017-08-03 22:27 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Brijesh Singh @ 2017-08-03 22:07 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel Cc: brijesh.singh, Tom Lendacky, Jordan Justen, Ard Biesheuvel Hi Laszlo, Thanks for the detail review, I will soon send v3 with all your feedback addressed. I must admit that I have constant struggle with formating issues in EDKII contributions. While browsing the code, several packages have code and comment exceeding 79 char. But looking at your previous feedbacks somehow I was under assumption that comments should be <= 79 chars but code can exceed 79 char limit. I think my understanding was wrong. I will update my vi setting to warn me when I exceed 79 char limit. Sorry about all those formating errors, and thanks for being so patience. -Brijesh On 08/03/2017 03:51 PM, Laszlo Ersek wrote: > On 08/03/17 18:10, 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 <lersek@redhat.com> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h | 42 ++-- >> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c | 247 ++++++++++++++++---- >> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 130 ----------- >> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c | 101 +++++--- >> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c | 56 ++--- >> 5 files changed, 292 insertions(+), 284 deletions(-) > > (1) please remove the space characters in front of the colons (":") in > the subject. > > (2) Still for the subject, it is "CommonBuffer", not "CommandBuffer". > > So the subject should be > > OvmfPkg: QemuFwCfgLib: Use BusMasterCommonBuffer to map FW_CFG_DMA_ACCESS > > (3) Please rewrap the commit message to 74 characters. > >> >> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h >> index 8cfa7913ffae..327f1d37e17d 100644 >> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h >> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h >> @@ -45,39 +45,25 @@ InternalQemuFwCfgDmaIsAvailable ( >> ); >> >> /** >> - Returns a boolean indicating whether SEV support is enabled >> + Transfer an array of bytes, or skip a number of bytes, using the DMA >> + interface. >> >> - @retval TRUE SEV is enabled >> - @retval FALSE SEV is disabled >> -**/ >> -BOOLEAN >> -InternalQemuFwCfgSevIsEnabled ( >> - VOID >> - ); >> + @param[in] Size Size in bytes to transfer or skip. >> >> -/** >> - Allocate a bounce buffer for SEV DMA. >> - >> - @param[out] Buffer Allocated DMA Buffer pointer >> - @param[in] NumPage Number of pages. >> + @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. >> **/ >> VOID >> -InternalQemuFwCfgSevDmaAllocateBuffer ( >> - OUT VOID **Buffer, >> - IN UINT32 NumPages >> +InternalQemuFwCfgDmaBytes ( >> + IN UINT32 Size, >> + IN OUT VOID *Buffer OPTIONAL, >> + IN UINT32 Control >> ); >> >> -/** >> - 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 >> - ); >> #endif >> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > > (4) You forgot to remove the InternalQemuFwCfgSevIsEnabled() function > from this file. > >> index f8eb03bc3a9a..e03647bae3bd 100644 >> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c >> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c >> @@ -20,6 +20,7 @@ >> #include <Protocol/IoMmu.h> >> >> #include <Library/BaseLib.h> >> +#include <Library/IoLib.h> >> #include <Library/DebugLib.h> >> #include <Library/QemuFwCfgLib.h> >> #include <Library/UefiBootServicesTableLib.h> >> @@ -157,74 +158,228 @@ InternalQemuFwCfgDmaIsAvailable ( >> } >> >> /** >> - Allocate a bounce buffer for SEV DMA. >> - >> - @param[in] NumPage Number of pages. >> - @param[out] Buffer Allocated DMA Buffer pointer >> + Function is used for allocating a bi-directional FW_CFG_DMA_ACCESS used >> + between Host and device to exchange the information. The buffer must be free'd >> + using FreeFwCfgDmaAccessBuffer (). >> >> **/ >> -VOID >> -InternalQemuFwCfgSevDmaAllocateBuffer ( >> - OUT VOID **Buffer, >> - IN UINT32 NumPages >> +STATIC >> +EFI_STATUS >> +AllocFwCfgDmaAccessBuffer ( >> + OUT VOID **Access, >> + OUT VOID **Mapping >> ) >> { >> - EFI_STATUS Status; >> + UINTN Size; >> + UINTN NumPages; >> + EFI_STATUS Status; >> + VOID *HostAddress; >> + EFI_PHYSICAL_ADDRESS DmaAddress; >> >> - ASSERT (mIoMmuProtocol != NULL); >> + Size = sizeof (FW_CFG_DMA_ACCESS); >> + NumPages = EFI_SIZE_TO_PAGES (Size); >> >> + // >> + // As per UEFI spec, in order to map a host address with BusMasterCommomBuffer64, > > (5) This line is too long. Also, you have one too many space chars in > front of the comma. > >> + // the buffer must be allocated using the IOMMU AllocateBuffer() >> + // >> Status = mIoMmuProtocol->AllocateBuffer ( >> - mIoMmuProtocol, >> - 0, >> - EfiBootServicesData, >> - NumPages, >> - Buffer, >> - EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED >> + mIoMmuProtocol, >> + AllocateAnyPages, >> + EfiBootServicesData, >> + NumPages, >> + &HostAddress, >> + EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED > > (6) Please OR this attribute with > EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE, so that the buffer can be > allocated from 64-bit memory. > >> ); > > (7) The closing paren should be aligned with the arguments. > > (8) Please handle any errors returned by AllocateBuffer(), by > propagating the error. > >> + >> + // >> + // Map the host buffer with BusMasterCommonBuffer64 >> + // >> + Status = mIoMmuProtocol->Map ( >> + mIoMmuProtocol, >> + EdkiiIoMmuOperationBusMasterCommonBuffer64, >> + HostAddress, >> + &Size, >> + &DmaAddress, >> + Mapping >> + ); > > (9) Please add a check here that, on success, Size is still equal to > sizeof(FW_CFG_DMA_ACCESS). (Theoretically it could be smaller, even on > success.) > > If it is smaller, then both the map and allocate operations should be > rolled back, and we should return EFI_OUT_OF_RESOURCES. For this, error > handling labels at the end of the function are likely the best. > > (10) We should only set the output parameters Mapping and Access only if > the entire function succeeds. > >> if (EFI_ERROR (Status)) { >> - DEBUG ((DEBUG_ERROR, >> - "%a:%a failed to allocate %u pages\n", gEfiCallerBaseName, __FUNCTION__, >> - NumPages)); >> - ASSERT (FALSE); >> - CpuDeadLoop (); >> + mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, HostAddress); >> } >> >> - DEBUG ((DEBUG_VERBOSE, >> - "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName, __FUNCTION__, >> - (UINT64)(UINTN)Buffer, NumPages)); >> + *Access = HostAddress; >> + return Status; >> +} >> + >> +/** >> + Function is to used for freeing the Access buffer allocated using >> + AllocFwCfgDmaAccessBuffer() >> + >> +**/ >> +STATIC >> +VOID >> +FreeFwCfgDmaAccessBuffer ( >> + IN VOID *Access, >> + IN VOID *Mapping >> + ) >> +{ >> + UINTN NumPages; >> + >> + NumPages = EFI_SIZE_TO_PAGES (sizeof (FW_CFG_DMA_ACCESS)); >> + >> + mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping); >> + >> + mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, Access); >> +} >> + >> +/** >> + Function is used for mapping host address to device address. The buffer must >> + be unmapped with UnmapDmaDataBuffer (). >> + >> +**/ >> +STATIC >> +EFI_STATUS >> +MapFwCfgDmaDataBuffer ( >> + IN BOOLEAN IsWrite, >> + IN VOID *HostAddress, >> + IN UINT32 Size, >> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, >> + OUT VOID **Mapping >> + ) >> +{ >> + EFI_STATUS Status; >> + UINTN NumberOfBytes; >> + >> + NumberOfBytes = Size; >> + Status = mIoMmuProtocol->Map ( >> + mIoMmuProtocol, >> + IsWrite ? EdkiiIoMmuOperationBusMasterRead64 : EdkiiIoMmuOperationBusMasterWrite64, > > (11) Please make sure that the file is not wider than 80 characters -- > the above can be broken up like > > (IsWrite ? > EdkiiIoMmuOperationBusMasterRead64 : > EdkiiIoMmuOperationBusMasterWrite64) > > And if that's still too wide, then a separate variable should be used. > > >> + HostAddress, >> + &NumberOfBytes, >> + DeviceAddress, >> + Mapping >> + ); > > (12) Same comment as (9): on success, we should check that NumberOfBytes > has the original value (i.e., Size). Otherwise, roll back the Map(), and > return EFI_OUT_OF_RESOURCES. > > (13) Same comment as (10): we should only set the output parameters > (DeviceAddress and Mapping) if the entire function succeeds. > > (14) Here's an alternative to the explicit / cascading error handling > recommended above: given that all of these functions will be called from > InternalQemuFwCfgSevDmaFreeBuffer(), which cannot propagate errors > anyway, you might as well hang immediately upon error. For this, please > preserve the following triplet, used earlier in > InternalQemuFwCfgSevDmaAllocateBuffer(): > - DEBUG_ERROR message with gEfiCallerBaseName, > - ASSERT (FALSE), > - CpuDeadLoop(). > > Correspondingly, none of the helper functions would have to return > EFI_STATUS. > >> + return Status; >> +} >> + >> +EFI_STATUS >> +UnmapFwCfgDmaDataBuffer ( >> + IN VOID *Mapping >> + ) >> +{ >> + return mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping); >> } > > (15) I think it's fine if we ignore (don't propagate) the error here -- > please change the return type to VOID (similarly to > FreeFwCfgDmaAccessBuffer()). > > (16) Please make this function STATIC. > >> >> /** >> - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer >> + Transfer an array of bytes, or skip a number of bytes, using the DMA >> + interface. >> >> - @param[in] NumPage Number of pages. >> - @param[in] Buffer DMA Buffer pointer >> + @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. >> **/ >> VOID >> -InternalQemuFwCfgSevDmaFreeBuffer ( >> - IN VOID *Buffer, >> - IN UINT32 NumPages >> +InternalQemuFwCfgDmaBytes ( >> + IN UINT32 Size, >> + IN OUT VOID *Buffer OPTIONAL, >> + IN UINT32 Control >> ) >> { >> - EFI_STATUS Status; >> + volatile FW_CFG_DMA_ACCESS LocalAccess; >> + volatile FW_CFG_DMA_ACCESS *Access; >> + UINT32 AccessHigh, AccessLow; >> + UINT32 Status; >> + VOID *AccessMapping, *DataMapping; >> >> - ASSERT (mIoMmuProtocol != NULL); >> + ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ || >> + Control == FW_CFG_DMA_CTL_SKIP); >> >> - 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 (); >> + if (Size == 0) { >> + return; >> } >> >> - DEBUG ((DEBUG_VERBOSE, >> - "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName,__FUNCTION__, >> - (UINT64)(UINTN)Buffer, NumPages)); >> + // >> + // When SEV is enabled, map Buffer to DMA address before issuing the DMA request > > (17) This line is too long. > >> + // >> + if (MemEncryptSevIsEnabled ()) { >> + VOID *AccessBuffer; >> + EFI_PHYSICAL_ADDRESS DataBuffer; >> + >> + // >> + // Allocate DMA Access buffer >> + // >> + Status = AllocFwCfgDmaAccessBuffer (&AccessBuffer, &AccessMapping); >> + ASSERT_EFI_ERROR (Status); > > (18) You can drop the assert according to cmment (14). > >> + >> + Access = AccessBuffer; >> + >> + // >> + // Map actual data buffer >> + // >> + if (Buffer) { > > (19) Please don't rely on Buffer being NULL for SKIP operations; that > may or may not be true. Instead, please check for > > (Control != FW_CFG_DMA_CTL_SKIP) > >> + Status = MapFwCfgDmaDataBuffer (Control == FW_CFG_DMA_CTL_WRITE ? 1 : 0, >> + Buffer, Size, &DataBuffer, &DataMapping); > > (20) Please break every argument to a separate line. > > (21) The following expression: > > Control == FW_CFG_DMA_CTL_WRITE ? 1 : 0 > > should be simplified as: > > Control == FW_CFG_DMA_CTL_WRITE > >> + ASSERT_EFI_ERROR (Status); > > (22) You can drop this, according to (14). > >> + >> + Buffer = (VOID *) (UINTN) DataBuffer; > > (23) Ugh, please don't overwrite a function parameter. Instead, please > introduce an additional variable. > > For example, you could rename "DataBuffer" to "DataBufferAddress", and > introduce "DataBuffer" as a (VOID*). In the non-SEV case, you could set > DataBuffer to Buffer, and in the SEV case, you could set DataBuffer to > (VOID*)(UINTN)DataBufferAddress. > > Then use DataBuffer for setting Access->Address. > >> + } >> + } else { >> + Access = &LocalAccess; >> + AccessMapping = NULL; >> + DataMapping = NULL; >> + } >> + >> + Access->Control = SwapBytes32 (Control); >> + Access->Length = SwapBytes32 (Size); >> + Access->Address = SwapBytes64 ((UINTN)Buffer); >> + >> + // >> + // 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) { >> + FreeFwCfgDmaAccessBuffer ((VOID *)Access, AccessMapping); >> + } >> + >> + if (DataMapping) { >> + UnmapFwCfgDmaDataBuffer (DataMapping); >> + } > > (24) The DataMapping check is not valid. You have a code path where > DataMapping is not set: Buffer==NULL (i.e., SKIP operation) with SEV > enabled. I guess the simplest way to fix this is to eliminate the > non-SEV branch near the top, and make all those assignments unconditional: > > Access = &LocalAccess; > AccessMapping = NULL; > DataMapping = NULL; > DataBuffer = Buffer; > // > // When SEV is enabled... > // > if (MemEncryptSevIsEnabled ()) { > ... > >> } >> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c >> index d3bf75498d60..7f42f38d1c05 100644 >> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c >> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c >> @@ -45,136 +45,6 @@ QemuFwCfgSelectItem ( >> IoWrite16 (FW_CFG_IO_SELECTOR, (UINT16)(UINTN) QemuFwCfgItem); >> } >> >> - >> -/** >> - 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. >> -**/ >> -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; >> - UINT32 NumPages; >> - VOID *DmaBuffer, *BounceBuffer; >> - >> - ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ || >> - Control == FW_CFG_DMA_CTL_SKIP); >> - >> - if (Size == 0) { >> - return; >> - } >> - >> - // >> - // set NumPages to suppress incorrect compiler/analyzer warnings >> - // >> - NumPages = 0; >> - >> - // >> - // When SEV is enabled then allocate DMA bounce buffer >> - // >> - if (InternalQemuFwCfgSevIsEnabled ()) { >> - UINTN TotalSize; >> - >> - TotalSize = sizeof (*Access); >> - // >> - // Skip operation does not need buffer >> - // >> - if (Control != FW_CFG_DMA_CTL_SKIP) { >> - TotalSize += Size; >> - } >> - >> - // >> - // Allocate SEV DMA buffer >> - // >> - NumPages = (UINT32)EFI_SIZE_TO_PAGES (TotalSize); >> - InternalQemuFwCfgSevDmaAllocateBuffer (&BounceBuffer, NumPages); >> - >> - Access = BounceBuffer; >> - DmaBuffer = (UINT8*)BounceBuffer + sizeof (*Access); >> - >> - // >> - // Decrypt data from encrypted guest buffer into DMA buffer >> - // >> - if (Control == FW_CFG_DMA_CTL_WRITE) { >> - CopyMem (DmaBuffer, Buffer, Size); >> - } >> - } else { >> - Access = &LocalAccess; >> - DmaBuffer = Buffer; >> - BounceBuffer = NULL; >> - } >> - >> - Access->Control = SwapBytes32 (Control); >> - Access->Length = SwapBytes32 (Size); >> - Access->Address = SwapBytes64 ((UINTN)DmaBuffer); >> - >> - // >> - // 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 Bounce buffer was allocated then copy the data into guest buffer and >> - // free the bounce buffer >> - // >> - if (BounceBuffer != NULL) { >> - // >> - // Encrypt the data from DMA buffer into guest buffer >> - // >> - if (Control == FW_CFG_DMA_CTL_READ) { >> - CopyMem (Buffer, DmaBuffer, Size); >> - } >> - >> - InternalQemuFwCfgSevDmaFreeBuffer (BounceBuffer, NumPages); >> - } >> -} >> - >> - >> /** >> Reads firmware configuration bytes into a buffer >> >> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c >> index 40f89c3b53e2..bc649b40aec3 100644 >> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c >> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c >> @@ -16,6 +16,7 @@ >> **/ >> >> #include <Library/BaseLib.h> >> +#include <Library/IoLib.h> >> #include <Library/DebugLib.h> >> #include <Library/QemuFwCfgLib.h> >> #include <Library/MemEncryptSevLib.h> >> @@ -85,7 +86,7 @@ QemuFwCfgInitialize ( >> // (which need to allocate dynamic memory and allocating a PAGE size'd >> // buffer can be challenge in PEI phase) >> // >> - if (InternalQemuFwCfgSevIsEnabled ()) { >> + if (MemEncryptSevIsEnabled ()) { >> DEBUG ((DEBUG_INFO, "SEV: QemuFwCfg fallback to IO Port interface.\n")); >> } else { >> mQemuFwCfgDmaSupported = TRUE; >> @@ -129,56 +130,80 @@ InternalQemuFwCfgDmaIsAvailable ( >> } >> >> /** >> + Transfer an array of bytes, or skip a number of bytes, using the DMA >> + interface. >> >> - Returns a boolean indicating whether SEV is enabled >> + @param[in] Size Size in bytes to transfer or skip. >> >> - @retval TRUE SEV is enabled >> - @retval FALSE SEV is disabled >> + @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. >> **/ >> -BOOLEAN >> -InternalQemuFwCfgSevIsEnabled ( >> - VOID >> +VOID >> +InternalQemuFwCfgDmaBytes ( >> + IN UINT32 Size, >> + IN OUT VOID *Buffer OPTIONAL, >> + IN UINT32 Control >> ) >> { >> - return MemEncryptSevIsEnabled (); >> -} >> + volatile FW_CFG_DMA_ACCESS Access; >> + UINT32 AccessHigh, AccessLow; >> + UINT32 Status; >> >> -/** >> - Allocate a bounce buffer for SEV DMA. >> + ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ || >> + Control == FW_CFG_DMA_CTL_SKIP); >> >> - @param[in] NumPage Number of pages. >> - @param[out] Buffer Allocated DMA Buffer pointer >> + if (Size == 0) { >> + return; >> + } >> + >> + if (MemEncryptSevIsEnabled ()) { >> + // >> + // SEV does not support DMA operations in PEI stage, we should >> + // not have reached here. >> + // >> + ASSERT (FALSE); >> + } > > (25) This is good; you are basically restoring / copying the > pre-7cfe445d7f4b state of the InternalQemuFwCfgDmaBytes() function, plus > adding this assert. But, it can be written better like this: > > // > // ... keep the same comment ... > // > ASSERT (!MemEncryptSevIsEnabled ()); > >> + >> + Access.Control = SwapBytes32 (Control); >> + Access.Length = SwapBytes32 (Size); >> + Access.Address = SwapBytes64 ((UINTN)Buffer); >> >> -**/ >> -VOID >> -InternalQemuFwCfgSevDmaAllocateBuffer ( >> - OUT VOID **Buffer, >> - IN UINT32 NumPages >> - ) >> -{ >> // >> - // We should never reach here >> + // Delimit the transfer from (a) modifications to Access, (b) in case of a >> + // write, from writes to Buffer by the caller. >> // >> - ASSERT (FALSE); >> - CpuDeadLoop (); >> -} >> + 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)); >> >> -/** >> - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer >> + // >> + // Don't look at Access.Control before starting the transfer. >> + // >> + MemoryFence (); >> >> - @param[in] NumPage Number of pages. >> - @param[in] Buffer DMA Buffer pointer >> + // >> + // Wait for the transfer to complete. >> + // >> + do { >> + Status = SwapBytes32 (Access.Control); >> + ASSERT ((Status & FW_CFG_DMA_CTL_ERROR) == 0); >> + } while (Status != 0); >> >> -**/ >> -VOID >> -InternalQemuFwCfgSevDmaFreeBuffer ( >> - IN VOID *Buffer, >> - IN UINT32 NumPages >> - ) >> -{ >> // >> - // We should never reach here >> + // After a read, the caller will want to use Buffer. >> // >> - ASSERT (FALSE); >> - CpuDeadLoop (); >> + MemoryFence (); >> } >> + >> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c >> index 071b8d9b91d4..62ddb69d3b4d 100644 >> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c >> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c >> @@ -97,53 +97,25 @@ InternalQemuFwCfgDmaIsAvailable ( >> } >> >> /** >> + Transfer an array of bytes, or skip a number of bytes, using the DMA >> + interface. >> >> - Returns a boolean indicating whether SEV is enabled >> + @param[in] Size Size in bytes to transfer or skip. >> >> - @retval TRUE SEV is enabled >> - @retval FALSE SEV is disabled >> -**/ >> -BOOLEAN >> -InternalQemuFwCfgSevIsEnabled ( >> - VOID >> - ) >> -{ >> - // >> - // DMA is not supported in SEC phase hence SEV support is irrelevant >> - // >> - return FALSE; >> -} >> - >> -/** >> - 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 >> - ) >> -{ >> - // >> - // We should never reach here >> - // >> - ASSERT (FALSE); >> -} >> - >> -/** >> - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer >> - >> - @param[in] NumPage Number of pages. >> - @param[in] Buffer DMA Buffer pointer >> + @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. >> **/ >> VOID >> -InternalQemuFwCfgSevDmaFreeBuffer ( >> - IN VOID *Buffer, >> - IN UINT32 NumPages >> +InternalQemuFwCfgDmaBytes ( >> + IN UINT32 Size, >> + IN OUT VOID *Buffer OPTIONAL, >> + IN UINT32 Control >> ) >> { >> // >> > > (26) Can you add a CpuDeadLoop() here as well (i.e., in the SEC > instance), after the ASSERT()? I know the previous version of the SEC > instance doesn't use CpuDeadLoop(), but I mentioned it here: > > http://mid.mail-archive.com/2b9361cc-03f9-a403-98aa-c1cf5bfd17c0@redhat.com > > Thanks! > Laszlo > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] OvmfPkg : QemuFwCfgLib: Use BusMasterCommandBuffer to map FW_CFG_DMA_ACCESS 2017-08-03 22:07 ` Brijesh Singh @ 2017-08-03 22:27 ` Laszlo Ersek 2017-08-04 17:08 ` Jordan Justen 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2017-08-03 22:27 UTC (permalink / raw) To: Brijesh Singh, edk2-devel; +Cc: Tom Lendacky, Jordan Justen, Ard Biesheuvel On 08/04/17 00:07, Brijesh Singh wrote: > Hi Laszlo, > > Thanks for the detail review, I will soon send v3 with all your feedback > addressed. I must admit that I have constant struggle with formating issues > in EDKII contributions. While browsing the code, several packages have > code and comment exceeding 79 char. But looking at your previous feedbacks > somehow I was under assumption that comments should be <= 79 chars but code > can exceed 79 char limit. I think my understanding was wrong. I will update > my vi setting to warn me when I exceed 79 char limit. In the coding style, you will find: https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/#51-general-rules ---------- 5 Source Files 5.1 General Rules 5.1.1 Lines shall be 120 columns, or less Preferably, limit line lengths to 80 columns or less. When this doesn't leave sufficient space for a good postfix style comment, extend the line to a total of 120 columns. Having some level of uniformity in the expected width of the source is useful for viewing and printing the code. ---------- I stick with 79 chars because they don't wrap in any kind of window sized to 80 columns. (Some terminal emulators / pager programs insert blank lines or wrap unexpectedly when a line is exactly 80 columns.) And, I like to size my windows to 80 columns because I use only one monitor (I dislike using more than one) and with 80 cols/window I can fit two windows side by side conveniently. You will find that users of Windows IDEs / multi-monitor setups totally ignore the 80 columns recommendation, and only stay even within the 120 columns limit as long as it's convenient for them (regardless of "postfix style comments"). For this reason, reading MdePkg / MdeModulePkg source code is a constant struggle for me. And, I tend to rewrap code that's imported to OvmfPkg from those places. Unfortunately, the very long variable names encouraged in edk2, combined with the requirement to break every argument of a multi-line function call to a separate line, provides a really strong incentive to exceed 80 characters. Say you have a protocol member func call with 5 arguments, and when written on a single line, it ends at column 105. Will you stick with that -- and drive me nuts, when I have to read it --, or will you break it into *seven* lines (function name, plus 5 args, plus closing paren)? Most people will opt to drive me nuts. Of course the sensible pattern would be to fill lines up to 79 chars as much as possible, and break the line only when you have to (honoring this limit), but that style has not been accepted, and I've had to abandon it. > Sorry about all those formating errors, and thanks for being so patience. No problem. Thanks, Laszlo > > -Brijesh > > On 08/03/2017 03:51 PM, Laszlo Ersek wrote: >> On 08/03/17 18:10, 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 <lersek@redhat.com> >>> Cc: Jordan Justen <jordan.l.justen@intel.com> >>> Cc: Laszlo Ersek <lersek@redhat.com> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >>> --- >>> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h | 42 ++-- >>> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c | 247 >>> ++++++++++++++++---- >>> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 130 ----------- >>> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c | 101 +++++--- >>> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c | 56 ++--- >>> 5 files changed, 292 insertions(+), 284 deletions(-) >> >> (1) please remove the space characters in front of the colons (":") in >> the subject. >> >> (2) Still for the subject, it is "CommonBuffer", not "CommandBuffer". >> >> So the subject should be >> >> OvmfPkg: QemuFwCfgLib: Use BusMasterCommonBuffer to map FW_CFG_DMA_ACCESS >> >> (3) Please rewrap the commit message to 74 characters. >> >>> >>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h >>> b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h >>> index 8cfa7913ffae..327f1d37e17d 100644 >>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h >>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h >>> @@ -45,39 +45,25 @@ InternalQemuFwCfgDmaIsAvailable ( >>> ); >>> /** >>> - Returns a boolean indicating whether SEV support is enabled >>> + Transfer an array of bytes, or skip a number of bytes, using the DMA >>> + interface. >>> - @retval TRUE SEV is enabled >>> - @retval FALSE SEV is disabled >>> -**/ >>> -BOOLEAN >>> -InternalQemuFwCfgSevIsEnabled ( >>> - VOID >>> - ); >>> + @param[in] Size Size in bytes to transfer or skip. >>> -/** >>> - Allocate a bounce buffer for SEV DMA. >>> - >>> - @param[out] Buffer Allocated DMA Buffer pointer >>> - @param[in] NumPage Number of pages. >>> + @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. >>> **/ >>> VOID >>> -InternalQemuFwCfgSevDmaAllocateBuffer ( >>> - OUT VOID **Buffer, >>> - IN UINT32 NumPages >>> +InternalQemuFwCfgDmaBytes ( >>> + IN UINT32 Size, >>> + IN OUT VOID *Buffer OPTIONAL, >>> + IN UINT32 Control >>> ); >>> -/** >>> - 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 >>> - ); >>> #endif >>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c >>> b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c >> >> (4) You forgot to remove the InternalQemuFwCfgSevIsEnabled() function >> from this file. >> >>> index f8eb03bc3a9a..e03647bae3bd 100644 >>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c >>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c >>> @@ -20,6 +20,7 @@ >>> #include <Protocol/IoMmu.h> >>> #include <Library/BaseLib.h> >>> +#include <Library/IoLib.h> >>> #include <Library/DebugLib.h> >>> #include <Library/QemuFwCfgLib.h> >>> #include <Library/UefiBootServicesTableLib.h> >>> @@ -157,74 +158,228 @@ InternalQemuFwCfgDmaIsAvailable ( >>> } >>> /** >>> - Allocate a bounce buffer for SEV DMA. >>> - >>> - @param[in] NumPage Number of pages. >>> - @param[out] Buffer Allocated DMA Buffer pointer >>> + Function is used for allocating a bi-directional FW_CFG_DMA_ACCESS >>> used >>> + between Host and device to exchange the information. The buffer >>> must be free'd >>> + using FreeFwCfgDmaAccessBuffer (). >>> **/ >>> -VOID >>> -InternalQemuFwCfgSevDmaAllocateBuffer ( >>> - OUT VOID **Buffer, >>> - IN UINT32 NumPages >>> +STATIC >>> +EFI_STATUS >>> +AllocFwCfgDmaAccessBuffer ( >>> + OUT VOID **Access, >>> + OUT VOID **Mapping >>> ) >>> { >>> - EFI_STATUS Status; >>> + UINTN Size; >>> + UINTN NumPages; >>> + EFI_STATUS Status; >>> + VOID *HostAddress; >>> + EFI_PHYSICAL_ADDRESS DmaAddress; >>> - ASSERT (mIoMmuProtocol != NULL); >>> + Size = sizeof (FW_CFG_DMA_ACCESS); >>> + NumPages = EFI_SIZE_TO_PAGES (Size); >>> + // >>> + // As per UEFI spec, in order to map a host address with >>> BusMasterCommomBuffer64, >> >> (5) This line is too long. Also, you have one too many space chars in >> front of the comma. >> >>> + // the buffer must be allocated using the IOMMU AllocateBuffer() >>> + // >>> Status = mIoMmuProtocol->AllocateBuffer ( >>> - mIoMmuProtocol, >>> - 0, >>> - EfiBootServicesData, >>> - NumPages, >>> - Buffer, >>> - EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED >>> + mIoMmuProtocol, >>> + AllocateAnyPages, >>> + EfiBootServicesData, >>> + NumPages, >>> + &HostAddress, >>> + EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED >> >> (6) Please OR this attribute with >> EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE, so that the buffer can be >> allocated from 64-bit memory. >> >>> ); >> >> (7) The closing paren should be aligned with the arguments. >> >> (8) Please handle any errors returned by AllocateBuffer(), by >> propagating the error. >> >>> + >>> + // >>> + // Map the host buffer with BusMasterCommonBuffer64 >>> + // >>> + Status = mIoMmuProtocol->Map ( >>> + mIoMmuProtocol, >>> + >>> EdkiiIoMmuOperationBusMasterCommonBuffer64, >>> + HostAddress, >>> + &Size, >>> + &DmaAddress, >>> + Mapping >>> + ); >> >> (9) Please add a check here that, on success, Size is still equal to >> sizeof(FW_CFG_DMA_ACCESS). (Theoretically it could be smaller, even on >> success.) >> >> If it is smaller, then both the map and allocate operations should be >> rolled back, and we should return EFI_OUT_OF_RESOURCES. For this, error >> handling labels at the end of the function are likely the best. >> >> (10) We should only set the output parameters Mapping and Access only if >> the entire function succeeds. >> >>> if (EFI_ERROR (Status)) { >>> - DEBUG ((DEBUG_ERROR, >>> - "%a:%a failed to allocate %u pages\n", gEfiCallerBaseName, >>> __FUNCTION__, >>> - NumPages)); >>> - ASSERT (FALSE); >>> - CpuDeadLoop (); >>> + mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, HostAddress); >>> } >>> - DEBUG ((DEBUG_VERBOSE, >>> - "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName, __FUNCTION__, >>> - (UINT64)(UINTN)Buffer, NumPages)); >>> + *Access = HostAddress; >>> + return Status; >>> +} >>> + >>> +/** >>> + Function is to used for freeing the Access buffer allocated using >>> + AllocFwCfgDmaAccessBuffer() >>> + >>> +**/ >>> +STATIC >>> +VOID >>> +FreeFwCfgDmaAccessBuffer ( >>> + IN VOID *Access, >>> + IN VOID *Mapping >>> + ) >>> +{ >>> + UINTN NumPages; >>> + >>> + NumPages = EFI_SIZE_TO_PAGES (sizeof (FW_CFG_DMA_ACCESS)); >>> + >>> + mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping); >>> + >>> + mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, Access); >>> +} >>> + >>> +/** >>> + Function is used for mapping host address to device address. The >>> buffer must >>> + be unmapped with UnmapDmaDataBuffer (). >>> + >>> +**/ >>> +STATIC >>> +EFI_STATUS >>> +MapFwCfgDmaDataBuffer ( >>> + IN BOOLEAN IsWrite, >>> + IN VOID *HostAddress, >>> + IN UINT32 Size, >>> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, >>> + OUT VOID **Mapping >>> + ) >>> +{ >>> + EFI_STATUS Status; >>> + UINTN NumberOfBytes; >>> + >>> + NumberOfBytes = Size; >>> + Status = mIoMmuProtocol->Map ( >>> + mIoMmuProtocol, >>> + IsWrite ? >>> EdkiiIoMmuOperationBusMasterRead64 : >>> EdkiiIoMmuOperationBusMasterWrite64, >> >> (11) Please make sure that the file is not wider than 80 characters -- >> the above can be broken up like >> >> (IsWrite ? >> EdkiiIoMmuOperationBusMasterRead64 : >> EdkiiIoMmuOperationBusMasterWrite64) >> >> And if that's still too wide, then a separate variable should be used. >> >> >>> + HostAddress, >>> + &NumberOfBytes, >>> + DeviceAddress, >>> + Mapping >>> + ); >> >> (12) Same comment as (9): on success, we should check that NumberOfBytes >> has the original value (i.e., Size). Otherwise, roll back the Map(), and >> return EFI_OUT_OF_RESOURCES. >> >> (13) Same comment as (10): we should only set the output parameters >> (DeviceAddress and Mapping) if the entire function succeeds. >> >> (14) Here's an alternative to the explicit / cascading error handling >> recommended above: given that all of these functions will be called from >> InternalQemuFwCfgSevDmaFreeBuffer(), which cannot propagate errors >> anyway, you might as well hang immediately upon error. For this, please >> preserve the following triplet, used earlier in >> InternalQemuFwCfgSevDmaAllocateBuffer(): >> - DEBUG_ERROR message with gEfiCallerBaseName, >> - ASSERT (FALSE), >> - CpuDeadLoop(). >> >> Correspondingly, none of the helper functions would have to return >> EFI_STATUS. >> >>> + return Status; >>> +} >>> + >>> +EFI_STATUS >>> +UnmapFwCfgDmaDataBuffer ( >>> + IN VOID *Mapping >>> + ) >>> +{ >>> + return mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping); >>> } >> >> (15) I think it's fine if we ignore (don't propagate) the error here -- >> please change the return type to VOID (similarly to >> FreeFwCfgDmaAccessBuffer()). >> >> (16) Please make this function STATIC. >> >>> /** >>> - Free the DMA buffer allocated using >>> InternalQemuFwCfgSevDmaAllocateBuffer >>> + Transfer an array of bytes, or skip a number of bytes, using the DMA >>> + interface. >>> - @param[in] NumPage Number of pages. >>> - @param[in] Buffer DMA Buffer pointer >>> + @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. >>> **/ >>> VOID >>> -InternalQemuFwCfgSevDmaFreeBuffer ( >>> - IN VOID *Buffer, >>> - IN UINT32 NumPages >>> +InternalQemuFwCfgDmaBytes ( >>> + IN UINT32 Size, >>> + IN OUT VOID *Buffer OPTIONAL, >>> + IN UINT32 Control >>> ) >>> { >>> - EFI_STATUS Status; >>> + volatile FW_CFG_DMA_ACCESS LocalAccess; >>> + volatile FW_CFG_DMA_ACCESS *Access; >>> + UINT32 AccessHigh, AccessLow; >>> + UINT32 Status; >>> + VOID *AccessMapping, *DataMapping; >>> - ASSERT (mIoMmuProtocol != NULL); >>> + ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == >>> FW_CFG_DMA_CTL_READ || >>> + Control == FW_CFG_DMA_CTL_SKIP); >>> - 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 (); >>> + if (Size == 0) { >>> + return; >>> } >>> - DEBUG ((DEBUG_VERBOSE, >>> - "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName,__FUNCTION__, >>> - (UINT64)(UINTN)Buffer, NumPages)); >>> + // >>> + // When SEV is enabled, map Buffer to DMA address before issuing >>> the DMA request >> >> (17) This line is too long. >> >>> + // >>> + if (MemEncryptSevIsEnabled ()) { >>> + VOID *AccessBuffer; >>> + EFI_PHYSICAL_ADDRESS DataBuffer; >>> + >>> + // >>> + // Allocate DMA Access buffer >>> + // >>> + Status = AllocFwCfgDmaAccessBuffer (&AccessBuffer, &AccessMapping); >>> + ASSERT_EFI_ERROR (Status); >> >> (18) You can drop the assert according to cmment (14). >> >>> + >>> + Access = AccessBuffer; >>> + >>> + // >>> + // Map actual data buffer >>> + // >>> + if (Buffer) { >> >> (19) Please don't rely on Buffer being NULL for SKIP operations; that >> may or may not be true. Instead, please check for >> >> (Control != FW_CFG_DMA_CTL_SKIP) >> >>> + Status = MapFwCfgDmaDataBuffer (Control == >>> FW_CFG_DMA_CTL_WRITE ? 1 : 0, >>> + Buffer, Size, &DataBuffer, &DataMapping); >> >> (20) Please break every argument to a separate line. >> >> (21) The following expression: >> >> Control == FW_CFG_DMA_CTL_WRITE ? 1 : 0 >> >> should be simplified as: >> >> Control == FW_CFG_DMA_CTL_WRITE >> >>> + ASSERT_EFI_ERROR (Status); >> >> (22) You can drop this, according to (14). >> >>> + >>> + Buffer = (VOID *) (UINTN) DataBuffer; >> >> (23) Ugh, please don't overwrite a function parameter. Instead, please >> introduce an additional variable. >> >> For example, you could rename "DataBuffer" to "DataBufferAddress", and >> introduce "DataBuffer" as a (VOID*). In the non-SEV case, you could set >> DataBuffer to Buffer, and in the SEV case, you could set DataBuffer to >> (VOID*)(UINTN)DataBufferAddress. >> >> Then use DataBuffer for setting Access->Address. >> >>> + } >>> + } else { >>> + Access = &LocalAccess; >>> + AccessMapping = NULL; >>> + DataMapping = NULL; >>> + } >>> + >>> + Access->Control = SwapBytes32 (Control); >>> + Access->Length = SwapBytes32 (Size); >>> + Access->Address = SwapBytes64 ((UINTN)Buffer); >>> + >>> + // >>> + // 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) { >>> + FreeFwCfgDmaAccessBuffer ((VOID *)Access, AccessMapping); >>> + } >>> + >>> + if (DataMapping) { >>> + UnmapFwCfgDmaDataBuffer (DataMapping); >>> + } >> >> (24) The DataMapping check is not valid. You have a code path where >> DataMapping is not set: Buffer==NULL (i.e., SKIP operation) with SEV >> enabled. I guess the simplest way to fix this is to eliminate the >> non-SEV branch near the top, and make all those assignments >> unconditional: >> >> Access = &LocalAccess; >> AccessMapping = NULL; >> DataMapping = NULL; >> DataBuffer = Buffer; >> // >> // When SEV is enabled... >> // >> if (MemEncryptSevIsEnabled ()) { >> ... >> >>> } >>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c >>> b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c >>> index d3bf75498d60..7f42f38d1c05 100644 >>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c >>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c >>> @@ -45,136 +45,6 @@ QemuFwCfgSelectItem ( >>> IoWrite16 (FW_CFG_IO_SELECTOR, (UINT16)(UINTN) QemuFwCfgItem); >>> } >>> - >>> -/** >>> - 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. >>> -**/ >>> -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; >>> - UINT32 NumPages; >>> - VOID *DmaBuffer, *BounceBuffer; >>> - >>> - ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == >>> FW_CFG_DMA_CTL_READ || >>> - Control == FW_CFG_DMA_CTL_SKIP); >>> - >>> - if (Size == 0) { >>> - return; >>> - } >>> - >>> - // >>> - // set NumPages to suppress incorrect compiler/analyzer warnings >>> - // >>> - NumPages = 0; >>> - >>> - // >>> - // When SEV is enabled then allocate DMA bounce buffer >>> - // >>> - if (InternalQemuFwCfgSevIsEnabled ()) { >>> - UINTN TotalSize; >>> - >>> - TotalSize = sizeof (*Access); >>> - // >>> - // Skip operation does not need buffer >>> - // >>> - if (Control != FW_CFG_DMA_CTL_SKIP) { >>> - TotalSize += Size; >>> - } >>> - >>> - // >>> - // Allocate SEV DMA buffer >>> - // >>> - NumPages = (UINT32)EFI_SIZE_TO_PAGES (TotalSize); >>> - InternalQemuFwCfgSevDmaAllocateBuffer (&BounceBuffer, NumPages); >>> - >>> - Access = BounceBuffer; >>> - DmaBuffer = (UINT8*)BounceBuffer + sizeof (*Access); >>> - >>> - // >>> - // Decrypt data from encrypted guest buffer into DMA buffer >>> - // >>> - if (Control == FW_CFG_DMA_CTL_WRITE) { >>> - CopyMem (DmaBuffer, Buffer, Size); >>> - } >>> - } else { >>> - Access = &LocalAccess; >>> - DmaBuffer = Buffer; >>> - BounceBuffer = NULL; >>> - } >>> - >>> - Access->Control = SwapBytes32 (Control); >>> - Access->Length = SwapBytes32 (Size); >>> - Access->Address = SwapBytes64 ((UINTN)DmaBuffer); >>> - >>> - // >>> - // 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 Bounce buffer was allocated then copy the data into guest >>> buffer and >>> - // free the bounce buffer >>> - // >>> - if (BounceBuffer != NULL) { >>> - // >>> - // Encrypt the data from DMA buffer into guest buffer >>> - // >>> - if (Control == FW_CFG_DMA_CTL_READ) { >>> - CopyMem (Buffer, DmaBuffer, Size); >>> - } >>> - >>> - InternalQemuFwCfgSevDmaFreeBuffer (BounceBuffer, NumPages); >>> - } >>> -} >>> - >>> - >>> /** >>> Reads firmware configuration bytes into a buffer >>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c >>> b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c >>> index 40f89c3b53e2..bc649b40aec3 100644 >>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c >>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c >>> @@ -16,6 +16,7 @@ >>> **/ >>> #include <Library/BaseLib.h> >>> +#include <Library/IoLib.h> >>> #include <Library/DebugLib.h> >>> #include <Library/QemuFwCfgLib.h> >>> #include <Library/MemEncryptSevLib.h> >>> @@ -85,7 +86,7 @@ QemuFwCfgInitialize ( >>> // (which need to allocate dynamic memory and allocating a PAGE >>> size'd >>> // buffer can be challenge in PEI phase) >>> // >>> - if (InternalQemuFwCfgSevIsEnabled ()) { >>> + if (MemEncryptSevIsEnabled ()) { >>> DEBUG ((DEBUG_INFO, "SEV: QemuFwCfg fallback to IO Port >>> interface.\n")); >>> } else { >>> mQemuFwCfgDmaSupported = TRUE; >>> @@ -129,56 +130,80 @@ InternalQemuFwCfgDmaIsAvailable ( >>> } >>> /** >>> + Transfer an array of bytes, or skip a number of bytes, using the DMA >>> + interface. >>> - Returns a boolean indicating whether SEV is enabled >>> + @param[in] Size Size in bytes to transfer or skip. >>> - @retval TRUE SEV is enabled >>> - @retval FALSE SEV is disabled >>> + @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. >>> **/ >>> -BOOLEAN >>> -InternalQemuFwCfgSevIsEnabled ( >>> - VOID >>> +VOID >>> +InternalQemuFwCfgDmaBytes ( >>> + IN UINT32 Size, >>> + IN OUT VOID *Buffer OPTIONAL, >>> + IN UINT32 Control >>> ) >>> { >>> - return MemEncryptSevIsEnabled (); >>> -} >>> + volatile FW_CFG_DMA_ACCESS Access; >>> + UINT32 AccessHigh, AccessLow; >>> + UINT32 Status; >>> -/** >>> - Allocate a bounce buffer for SEV DMA. >>> + ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == >>> FW_CFG_DMA_CTL_READ || >>> + Control == FW_CFG_DMA_CTL_SKIP); >>> - @param[in] NumPage Number of pages. >>> - @param[out] Buffer Allocated DMA Buffer pointer >>> + if (Size == 0) { >>> + return; >>> + } >>> + >>> + if (MemEncryptSevIsEnabled ()) { >>> + // >>> + // SEV does not support DMA operations in PEI stage, we should >>> + // not have reached here. >>> + // >>> + ASSERT (FALSE); >>> + } >> >> (25) This is good; you are basically restoring / copying the >> pre-7cfe445d7f4b state of the InternalQemuFwCfgDmaBytes() function, plus >> adding this assert. But, it can be written better like this: >> >> // >> // ... keep the same comment ... >> // >> ASSERT (!MemEncryptSevIsEnabled ()); >> >>> + >>> + Access.Control = SwapBytes32 (Control); >>> + Access.Length = SwapBytes32 (Size); >>> + Access.Address = SwapBytes64 ((UINTN)Buffer); >>> -**/ >>> -VOID >>> -InternalQemuFwCfgSevDmaAllocateBuffer ( >>> - OUT VOID **Buffer, >>> - IN UINT32 NumPages >>> - ) >>> -{ >>> // >>> - // We should never reach here >>> + // Delimit the transfer from (a) modifications to Access, (b) in >>> case of a >>> + // write, from writes to Buffer by the caller. >>> // >>> - ASSERT (FALSE); >>> - CpuDeadLoop (); >>> -} >>> + 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)); >>> -/** >>> - Free the DMA buffer allocated using >>> InternalQemuFwCfgSevDmaAllocateBuffer >>> + // >>> + // Don't look at Access.Control before starting the transfer. >>> + // >>> + MemoryFence (); >>> - @param[in] NumPage Number of pages. >>> - @param[in] Buffer DMA Buffer pointer >>> + // >>> + // Wait for the transfer to complete. >>> + // >>> + do { >>> + Status = SwapBytes32 (Access.Control); >>> + ASSERT ((Status & FW_CFG_DMA_CTL_ERROR) == 0); >>> + } while (Status != 0); >>> -**/ >>> -VOID >>> -InternalQemuFwCfgSevDmaFreeBuffer ( >>> - IN VOID *Buffer, >>> - IN UINT32 NumPages >>> - ) >>> -{ >>> // >>> - // We should never reach here >>> + // After a read, the caller will want to use Buffer. >>> // >>> - ASSERT (FALSE); >>> - CpuDeadLoop (); >>> + MemoryFence (); >>> } >>> + >>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c >>> b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c >>> index 071b8d9b91d4..62ddb69d3b4d 100644 >>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c >>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c >>> @@ -97,53 +97,25 @@ InternalQemuFwCfgDmaIsAvailable ( >>> } >>> /** >>> + Transfer an array of bytes, or skip a number of bytes, using the DMA >>> + interface. >>> - Returns a boolean indicating whether SEV is enabled >>> + @param[in] Size Size in bytes to transfer or skip. >>> - @retval TRUE SEV is enabled >>> - @retval FALSE SEV is disabled >>> -**/ >>> -BOOLEAN >>> -InternalQemuFwCfgSevIsEnabled ( >>> - VOID >>> - ) >>> -{ >>> - // >>> - // DMA is not supported in SEC phase hence SEV support is irrelevant >>> - // >>> - return FALSE; >>> -} >>> - >>> -/** >>> - 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 >>> - ) >>> -{ >>> - // >>> - // We should never reach here >>> - // >>> - ASSERT (FALSE); >>> -} >>> - >>> -/** >>> - Free the DMA buffer allocated using >>> InternalQemuFwCfgSevDmaAllocateBuffer >>> - >>> - @param[in] NumPage Number of pages. >>> - @param[in] Buffer DMA Buffer pointer >>> + @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. >>> **/ >>> VOID >>> -InternalQemuFwCfgSevDmaFreeBuffer ( >>> - IN VOID *Buffer, >>> - IN UINT32 NumPages >>> +InternalQemuFwCfgDmaBytes ( >>> + IN UINT32 Size, >>> + IN OUT VOID *Buffer OPTIONAL, >>> + IN UINT32 Control >>> ) >>> { >>> // >>> >> >> (26) Can you add a CpuDeadLoop() here as well (i.e., in the SEC >> instance), after the ASSERT()? I know the previous version of the SEC >> instance doesn't use CpuDeadLoop(), but I mentioned it here: >> >> http://mid.mail-archive.com/2b9361cc-03f9-a403-98aa-c1cf5bfd17c0@redhat.com >> >> >> Thanks! >> Laszlo >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] OvmfPkg : QemuFwCfgLib: Use BusMasterCommandBuffer to map FW_CFG_DMA_ACCESS 2017-08-03 22:27 ` Laszlo Ersek @ 2017-08-04 17:08 ` Jordan Justen 2017-08-04 20:25 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Jordan Justen @ 2017-08-04 17:08 UTC (permalink / raw) To: Brijesh Singh, Laszlo Ersek, edk2-devel; +Cc: Tom Lendacky, Ard Biesheuvel On 2017-08-03 15:27:46, Laszlo Ersek wrote: > On 08/04/17 00:07, Brijesh Singh wrote: > > Hi Laszlo, > > > > Thanks for the detail review, I will soon send v3 with all your feedback > > addressed. I must admit that I have constant struggle with formating issues > > in EDKII contributions. While browsing the code, several packages have > > code and comment exceeding 79 char. But looking at your previous feedbacks > > somehow I was under assumption that comments should be <= 79 chars but code > > can exceed 79 char limit. I think my understanding was wrong. I will update > > my vi setting to warn me when I exceed 79 char limit. > > In the coding style, you will find: > > https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/#51-general-rules > > ---------- > 5 Source Files > 5.1 General Rules > 5.1.1 Lines shall be 120 columns, or less > > Preferably, limit line lengths to 80 columns or less. When this doesn't > leave sufficient space for a good postfix style comment, extend the line > to a total of 120 columns. Having some level of uniformity in the > expected width of the source is useful for viewing and printing the code. I personally don't like this wording. It basically says, 'we prefer 80, but if that is too difficult, then 120 is ok'. The only case I've ever seen of where code wouldn't fit reasonably in 80 columns, someone was essentially putting a spreadsheet into code. (This is not exactly a good case to build a code style around.) > ---------- > > I stick with 79 chars because they don't wrap in any kind of window > sized to 80 columns. (Some terminal emulators / pager programs insert > blank lines or wrap unexpectedly when a line is exactly 80 columns.) > And, I like to size my windows to 80 columns because I use only one > monitor (I dislike using more than one) and with 80 cols/window I can > fit two windows side by side conveniently. History (punch cards) and personal workflows (fitting multiple terminals) are often cited as reasons for the 'eighty column rule', but more fundamentally, I prefer this argument: https://softwareengineering.stackexchange.com/a/222998 Essentially, there's a reason newspapers have columns. It is not easy for humans to read very long lines of text. I have another reason why going beyond 80 is not a good idea for code that doesn't apply to normal reading. If you need ~120 columns visible to view some lines, then most lines will end up having a lot of wasted horizontal whitespace because they can commonly fit into 80 columns. -Jordan > You will find that users of Windows IDEs / multi-monitor setups totally > ignore the 80 columns recommendation, and only stay even within the 120 > columns limit as long as it's convenient for them (regardless of > "postfix style comments"). For this reason, reading MdePkg / > MdeModulePkg source code is a constant struggle for me. And, I tend to > rewrap code that's imported to OvmfPkg from those places. > > Unfortunately, the very long variable names encouraged in edk2, combined > with the requirement to break every argument of a multi-line function > call to a separate line, provides a really strong incentive to exceed 80 > characters. Say you have a protocol member func call with 5 arguments, > and when written on a single line, it ends at column 105. Will you stick > with that -- and drive me nuts, when I have to read it --, or will you > break it into *seven* lines (function name, plus 5 args, plus closing > paren)? Most people will opt to drive me nuts. > > Of course the sensible pattern would be to fill lines up to 79 chars as > much as possible, and break the line only when you have to (honoring > this limit), but that style has not been accepted, and I've had to > abandon it. > > > Sorry about all those formating errors, and thanks for being so patience. > > No problem. > > Thanks, > Laszlo > > > > > -Brijesh > > > > On 08/03/2017 03:51 PM, Laszlo Ersek wrote: > >> On 08/03/17 18:10, 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 <lersek@redhat.com> > >>> Cc: Jordan Justen <jordan.l.justen@intel.com> > >>> Cc: Laszlo Ersek <lersek@redhat.com> > >>> Contributed-under: TianoCore Contribution Agreement 1.0 > >>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > >>> --- > >>> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h | 42 ++-- > >>> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c | 247 > >>> ++++++++++++++++---- > >>> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 130 ----------- > >>> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c | 101 +++++--- > >>> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c | 56 ++--- > >>> 5 files changed, 292 insertions(+), 284 deletions(-) > >> > >> (1) please remove the space characters in front of the colons (":") in > >> the subject. > >> > >> (2) Still for the subject, it is "CommonBuffer", not "CommandBuffer". > >> > >> So the subject should be > >> > >> OvmfPkg: QemuFwCfgLib: Use BusMasterCommonBuffer to map FW_CFG_DMA_ACCESS > >> > >> (3) Please rewrap the commit message to 74 characters. > >> > >>> > >>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h > >>> b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h > >>> index 8cfa7913ffae..327f1d37e17d 100644 > >>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h > >>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h > >>> @@ -45,39 +45,25 @@ InternalQemuFwCfgDmaIsAvailable ( > >>> ); > >>> /** > >>> - Returns a boolean indicating whether SEV support is enabled > >>> + Transfer an array of bytes, or skip a number of bytes, using the DMA > >>> + interface. > >>> - @retval TRUE SEV is enabled > >>> - @retval FALSE SEV is disabled > >>> -**/ > >>> -BOOLEAN > >>> -InternalQemuFwCfgSevIsEnabled ( > >>> - VOID > >>> - ); > >>> + @param[in] Size Size in bytes to transfer or skip. > >>> -/** > >>> - Allocate a bounce buffer for SEV DMA. > >>> - > >>> - @param[out] Buffer Allocated DMA Buffer pointer > >>> - @param[in] NumPage Number of pages. > >>> + @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. > >>> **/ > >>> VOID > >>> -InternalQemuFwCfgSevDmaAllocateBuffer ( > >>> - OUT VOID **Buffer, > >>> - IN UINT32 NumPages > >>> +InternalQemuFwCfgDmaBytes ( > >>> + IN UINT32 Size, > >>> + IN OUT VOID *Buffer OPTIONAL, > >>> + IN UINT32 Control > >>> ); > >>> -/** > >>> - 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 > >>> - ); > >>> #endif > >>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > >>> b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > >> > >> (4) You forgot to remove the InternalQemuFwCfgSevIsEnabled() function > >> from this file. > >> > >>> index f8eb03bc3a9a..e03647bae3bd 100644 > >>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > >>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > >>> @@ -20,6 +20,7 @@ > >>> #include <Protocol/IoMmu.h> > >>> #include <Library/BaseLib.h> > >>> +#include <Library/IoLib.h> > >>> #include <Library/DebugLib.h> > >>> #include <Library/QemuFwCfgLib.h> > >>> #include <Library/UefiBootServicesTableLib.h> > >>> @@ -157,74 +158,228 @@ InternalQemuFwCfgDmaIsAvailable ( > >>> } > >>> /** > >>> - Allocate a bounce buffer for SEV DMA. > >>> - > >>> - @param[in] NumPage Number of pages. > >>> - @param[out] Buffer Allocated DMA Buffer pointer > >>> + Function is used for allocating a bi-directional FW_CFG_DMA_ACCESS > >>> used > >>> + between Host and device to exchange the information. The buffer > >>> must be free'd > >>> + using FreeFwCfgDmaAccessBuffer (). > >>> **/ > >>> -VOID > >>> -InternalQemuFwCfgSevDmaAllocateBuffer ( > >>> - OUT VOID **Buffer, > >>> - IN UINT32 NumPages > >>> +STATIC > >>> +EFI_STATUS > >>> +AllocFwCfgDmaAccessBuffer ( > >>> + OUT VOID **Access, > >>> + OUT VOID **Mapping > >>> ) > >>> { > >>> - EFI_STATUS Status; > >>> + UINTN Size; > >>> + UINTN NumPages; > >>> + EFI_STATUS Status; > >>> + VOID *HostAddress; > >>> + EFI_PHYSICAL_ADDRESS DmaAddress; > >>> - ASSERT (mIoMmuProtocol != NULL); > >>> + Size = sizeof (FW_CFG_DMA_ACCESS); > >>> + NumPages = EFI_SIZE_TO_PAGES (Size); > >>> + // > >>> + // As per UEFI spec, in order to map a host address with > >>> BusMasterCommomBuffer64, > >> > >> (5) This line is too long. Also, you have one too many space chars in > >> front of the comma. > >> > >>> + // the buffer must be allocated using the IOMMU AllocateBuffer() > >>> + // > >>> Status = mIoMmuProtocol->AllocateBuffer ( > >>> - mIoMmuProtocol, > >>> - 0, > >>> - EfiBootServicesData, > >>> - NumPages, > >>> - Buffer, > >>> - EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED > >>> + mIoMmuProtocol, > >>> + AllocateAnyPages, > >>> + EfiBootServicesData, > >>> + NumPages, > >>> + &HostAddress, > >>> + EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED > >> > >> (6) Please OR this attribute with > >> EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE, so that the buffer can be > >> allocated from 64-bit memory. > >> > >>> ); > >> > >> (7) The closing paren should be aligned with the arguments. > >> > >> (8) Please handle any errors returned by AllocateBuffer(), by > >> propagating the error. > >> > >>> + > >>> + // > >>> + // Map the host buffer with BusMasterCommonBuffer64 > >>> + // > >>> + Status = mIoMmuProtocol->Map ( > >>> + mIoMmuProtocol, > >>> + > >>> EdkiiIoMmuOperationBusMasterCommonBuffer64, > >>> + HostAddress, > >>> + &Size, > >>> + &DmaAddress, > >>> + Mapping > >>> + ); > >> > >> (9) Please add a check here that, on success, Size is still equal to > >> sizeof(FW_CFG_DMA_ACCESS). (Theoretically it could be smaller, even on > >> success.) > >> > >> If it is smaller, then both the map and allocate operations should be > >> rolled back, and we should return EFI_OUT_OF_RESOURCES. For this, error > >> handling labels at the end of the function are likely the best. > >> > >> (10) We should only set the output parameters Mapping and Access only if > >> the entire function succeeds. > >> > >>> if (EFI_ERROR (Status)) { > >>> - DEBUG ((DEBUG_ERROR, > >>> - "%a:%a failed to allocate %u pages\n", gEfiCallerBaseName, > >>> __FUNCTION__, > >>> - NumPages)); > >>> - ASSERT (FALSE); > >>> - CpuDeadLoop (); > >>> + mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, HostAddress); > >>> } > >>> - DEBUG ((DEBUG_VERBOSE, > >>> - "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName, __FUNCTION__, > >>> - (UINT64)(UINTN)Buffer, NumPages)); > >>> + *Access = HostAddress; > >>> + return Status; > >>> +} > >>> + > >>> +/** > >>> + Function is to used for freeing the Access buffer allocated using > >>> + AllocFwCfgDmaAccessBuffer() > >>> + > >>> +**/ > >>> +STATIC > >>> +VOID > >>> +FreeFwCfgDmaAccessBuffer ( > >>> + IN VOID *Access, > >>> + IN VOID *Mapping > >>> + ) > >>> +{ > >>> + UINTN NumPages; > >>> + > >>> + NumPages = EFI_SIZE_TO_PAGES (sizeof (FW_CFG_DMA_ACCESS)); > >>> + > >>> + mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping); > >>> + > >>> + mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, Access); > >>> +} > >>> + > >>> +/** > >>> + Function is used for mapping host address to device address. The > >>> buffer must > >>> + be unmapped with UnmapDmaDataBuffer (). > >>> + > >>> +**/ > >>> +STATIC > >>> +EFI_STATUS > >>> +MapFwCfgDmaDataBuffer ( > >>> + IN BOOLEAN IsWrite, > >>> + IN VOID *HostAddress, > >>> + IN UINT32 Size, > >>> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, > >>> + OUT VOID **Mapping > >>> + ) > >>> +{ > >>> + EFI_STATUS Status; > >>> + UINTN NumberOfBytes; > >>> + > >>> + NumberOfBytes = Size; > >>> + Status = mIoMmuProtocol->Map ( > >>> + mIoMmuProtocol, > >>> + IsWrite ? > >>> EdkiiIoMmuOperationBusMasterRead64 : > >>> EdkiiIoMmuOperationBusMasterWrite64, > >> > >> (11) Please make sure that the file is not wider than 80 characters -- > >> the above can be broken up like > >> > >> (IsWrite ? > >> EdkiiIoMmuOperationBusMasterRead64 : > >> EdkiiIoMmuOperationBusMasterWrite64) > >> > >> And if that's still too wide, then a separate variable should be used. > >> > >> > >>> + HostAddress, > >>> + &NumberOfBytes, > >>> + DeviceAddress, > >>> + Mapping > >>> + ); > >> > >> (12) Same comment as (9): on success, we should check that NumberOfBytes > >> has the original value (i.e., Size). Otherwise, roll back the Map(), and > >> return EFI_OUT_OF_RESOURCES. > >> > >> (13) Same comment as (10): we should only set the output parameters > >> (DeviceAddress and Mapping) if the entire function succeeds. > >> > >> (14) Here's an alternative to the explicit / cascading error handling > >> recommended above: given that all of these functions will be called from > >> InternalQemuFwCfgSevDmaFreeBuffer(), which cannot propagate errors > >> anyway, you might as well hang immediately upon error. For this, please > >> preserve the following triplet, used earlier in > >> InternalQemuFwCfgSevDmaAllocateBuffer(): > >> - DEBUG_ERROR message with gEfiCallerBaseName, > >> - ASSERT (FALSE), > >> - CpuDeadLoop(). > >> > >> Correspondingly, none of the helper functions would have to return > >> EFI_STATUS. > >> > >>> + return Status; > >>> +} > >>> + > >>> +EFI_STATUS > >>> +UnmapFwCfgDmaDataBuffer ( > >>> + IN VOID *Mapping > >>> + ) > >>> +{ > >>> + return mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping); > >>> } > >> > >> (15) I think it's fine if we ignore (don't propagate) the error here -- > >> please change the return type to VOID (similarly to > >> FreeFwCfgDmaAccessBuffer()). > >> > >> (16) Please make this function STATIC. > >> > >>> /** > >>> - Free the DMA buffer allocated using > >>> InternalQemuFwCfgSevDmaAllocateBuffer > >>> + Transfer an array of bytes, or skip a number of bytes, using the DMA > >>> + interface. > >>> - @param[in] NumPage Number of pages. > >>> - @param[in] Buffer DMA Buffer pointer > >>> + @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. > >>> **/ > >>> VOID > >>> -InternalQemuFwCfgSevDmaFreeBuffer ( > >>> - IN VOID *Buffer, > >>> - IN UINT32 NumPages > >>> +InternalQemuFwCfgDmaBytes ( > >>> + IN UINT32 Size, > >>> + IN OUT VOID *Buffer OPTIONAL, > >>> + IN UINT32 Control > >>> ) > >>> { > >>> - EFI_STATUS Status; > >>> + volatile FW_CFG_DMA_ACCESS LocalAccess; > >>> + volatile FW_CFG_DMA_ACCESS *Access; > >>> + UINT32 AccessHigh, AccessLow; > >>> + UINT32 Status; > >>> + VOID *AccessMapping, *DataMapping; > >>> - ASSERT (mIoMmuProtocol != NULL); > >>> + ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == > >>> FW_CFG_DMA_CTL_READ || > >>> + Control == FW_CFG_DMA_CTL_SKIP); > >>> - 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 (); > >>> + if (Size == 0) { > >>> + return; > >>> } > >>> - DEBUG ((DEBUG_VERBOSE, > >>> - "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName,__FUNCTION__, > >>> - (UINT64)(UINTN)Buffer, NumPages)); > >>> + // > >>> + // When SEV is enabled, map Buffer to DMA address before issuing > >>> the DMA request > >> > >> (17) This line is too long. > >> > >>> + // > >>> + if (MemEncryptSevIsEnabled ()) { > >>> + VOID *AccessBuffer; > >>> + EFI_PHYSICAL_ADDRESS DataBuffer; > >>> + > >>> + // > >>> + // Allocate DMA Access buffer > >>> + // > >>> + Status = AllocFwCfgDmaAccessBuffer (&AccessBuffer, &AccessMapping); > >>> + ASSERT_EFI_ERROR (Status); > >> > >> (18) You can drop the assert according to cmment (14). > >> > >>> + > >>> + Access = AccessBuffer; > >>> + > >>> + // > >>> + // Map actual data buffer > >>> + // > >>> + if (Buffer) { > >> > >> (19) Please don't rely on Buffer being NULL for SKIP operations; that > >> may or may not be true. Instead, please check for > >> > >> (Control != FW_CFG_DMA_CTL_SKIP) > >> > >>> + Status = MapFwCfgDmaDataBuffer (Control == > >>> FW_CFG_DMA_CTL_WRITE ? 1 : 0, > >>> + Buffer, Size, &DataBuffer, &DataMapping); > >> > >> (20) Please break every argument to a separate line. > >> > >> (21) The following expression: > >> > >> Control == FW_CFG_DMA_CTL_WRITE ? 1 : 0 > >> > >> should be simplified as: > >> > >> Control == FW_CFG_DMA_CTL_WRITE > >> > >>> + ASSERT_EFI_ERROR (Status); > >> > >> (22) You can drop this, according to (14). > >> > >>> + > >>> + Buffer = (VOID *) (UINTN) DataBuffer; > >> > >> (23) Ugh, please don't overwrite a function parameter. Instead, please > >> introduce an additional variable. > >> > >> For example, you could rename "DataBuffer" to "DataBufferAddress", and > >> introduce "DataBuffer" as a (VOID*). In the non-SEV case, you could set > >> DataBuffer to Buffer, and in the SEV case, you could set DataBuffer to > >> (VOID*)(UINTN)DataBufferAddress. > >> > >> Then use DataBuffer for setting Access->Address. > >> > >>> + } > >>> + } else { > >>> + Access = &LocalAccess; > >>> + AccessMapping = NULL; > >>> + DataMapping = NULL; > >>> + } > >>> + > >>> + Access->Control = SwapBytes32 (Control); > >>> + Access->Length = SwapBytes32 (Size); > >>> + Access->Address = SwapBytes64 ((UINTN)Buffer); > >>> + > >>> + // > >>> + // 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) { > >>> + FreeFwCfgDmaAccessBuffer ((VOID *)Access, AccessMapping); > >>> + } > >>> + > >>> + if (DataMapping) { > >>> + UnmapFwCfgDmaDataBuffer (DataMapping); > >>> + } > >> > >> (24) The DataMapping check is not valid. You have a code path where > >> DataMapping is not set: Buffer==NULL (i.e., SKIP operation) with SEV > >> enabled. I guess the simplest way to fix this is to eliminate the > >> non-SEV branch near the top, and make all those assignments > >> unconditional: > >> > >> Access = &LocalAccess; > >> AccessMapping = NULL; > >> DataMapping = NULL; > >> DataBuffer = Buffer; > >> // > >> // When SEV is enabled... > >> // > >> if (MemEncryptSevIsEnabled ()) { > >> ... > >> > >>> } > >>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > >>> b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > >>> index d3bf75498d60..7f42f38d1c05 100644 > >>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > >>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > >>> @@ -45,136 +45,6 @@ QemuFwCfgSelectItem ( > >>> IoWrite16 (FW_CFG_IO_SELECTOR, (UINT16)(UINTN) QemuFwCfgItem); > >>> } > >>> - > >>> -/** > >>> - 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. > >>> -**/ > >>> -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; > >>> - UINT32 NumPages; > >>> - VOID *DmaBuffer, *BounceBuffer; > >>> - > >>> - ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == > >>> FW_CFG_DMA_CTL_READ || > >>> - Control == FW_CFG_DMA_CTL_SKIP); > >>> - > >>> - if (Size == 0) { > >>> - return; > >>> - } > >>> - > >>> - // > >>> - // set NumPages to suppress incorrect compiler/analyzer warnings > >>> - // > >>> - NumPages = 0; > >>> - > >>> - // > >>> - // When SEV is enabled then allocate DMA bounce buffer > >>> - // > >>> - if (InternalQemuFwCfgSevIsEnabled ()) { > >>> - UINTN TotalSize; > >>> - > >>> - TotalSize = sizeof (*Access); > >>> - // > >>> - // Skip operation does not need buffer > >>> - // > >>> - if (Control != FW_CFG_DMA_CTL_SKIP) { > >>> - TotalSize += Size; > >>> - } > >>> - > >>> - // > >>> - // Allocate SEV DMA buffer > >>> - // > >>> - NumPages = (UINT32)EFI_SIZE_TO_PAGES (TotalSize); > >>> - InternalQemuFwCfgSevDmaAllocateBuffer (&BounceBuffer, NumPages); > >>> - > >>> - Access = BounceBuffer; > >>> - DmaBuffer = (UINT8*)BounceBuffer + sizeof (*Access); > >>> - > >>> - // > >>> - // Decrypt data from encrypted guest buffer into DMA buffer > >>> - // > >>> - if (Control == FW_CFG_DMA_CTL_WRITE) { > >>> - CopyMem (DmaBuffer, Buffer, Size); > >>> - } > >>> - } else { > >>> - Access = &LocalAccess; > >>> - DmaBuffer = Buffer; > >>> - BounceBuffer = NULL; > >>> - } > >>> - > >>> - Access->Control = SwapBytes32 (Control); > >>> - Access->Length = SwapBytes32 (Size); > >>> - Access->Address = SwapBytes64 ((UINTN)DmaBuffer); > >>> - > >>> - // > >>> - // 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 Bounce buffer was allocated then copy the data into guest > >>> buffer and > >>> - // free the bounce buffer > >>> - // > >>> - if (BounceBuffer != NULL) { > >>> - // > >>> - // Encrypt the data from DMA buffer into guest buffer > >>> - // > >>> - if (Control == FW_CFG_DMA_CTL_READ) { > >>> - CopyMem (Buffer, DmaBuffer, Size); > >>> - } > >>> - > >>> - InternalQemuFwCfgSevDmaFreeBuffer (BounceBuffer, NumPages); > >>> - } > >>> -} > >>> - > >>> - > >>> /** > >>> Reads firmware configuration bytes into a buffer > >>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c > >>> b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c > >>> index 40f89c3b53e2..bc649b40aec3 100644 > >>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c > >>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c > >>> @@ -16,6 +16,7 @@ > >>> **/ > >>> #include <Library/BaseLib.h> > >>> +#include <Library/IoLib.h> > >>> #include <Library/DebugLib.h> > >>> #include <Library/QemuFwCfgLib.h> > >>> #include <Library/MemEncryptSevLib.h> > >>> @@ -85,7 +86,7 @@ QemuFwCfgInitialize ( > >>> // (which need to allocate dynamic memory and allocating a PAGE > >>> size'd > >>> // buffer can be challenge in PEI phase) > >>> // > >>> - if (InternalQemuFwCfgSevIsEnabled ()) { > >>> + if (MemEncryptSevIsEnabled ()) { > >>> DEBUG ((DEBUG_INFO, "SEV: QemuFwCfg fallback to IO Port > >>> interface.\n")); > >>> } else { > >>> mQemuFwCfgDmaSupported = TRUE; > >>> @@ -129,56 +130,80 @@ InternalQemuFwCfgDmaIsAvailable ( > >>> } > >>> /** > >>> + Transfer an array of bytes, or skip a number of bytes, using the DMA > >>> + interface. > >>> - Returns a boolean indicating whether SEV is enabled > >>> + @param[in] Size Size in bytes to transfer or skip. > >>> - @retval TRUE SEV is enabled > >>> - @retval FALSE SEV is disabled > >>> + @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. > >>> **/ > >>> -BOOLEAN > >>> -InternalQemuFwCfgSevIsEnabled ( > >>> - VOID > >>> +VOID > >>> +InternalQemuFwCfgDmaBytes ( > >>> + IN UINT32 Size, > >>> + IN OUT VOID *Buffer OPTIONAL, > >>> + IN UINT32 Control > >>> ) > >>> { > >>> - return MemEncryptSevIsEnabled (); > >>> -} > >>> + volatile FW_CFG_DMA_ACCESS Access; > >>> + UINT32 AccessHigh, AccessLow; > >>> + UINT32 Status; > >>> -/** > >>> - Allocate a bounce buffer for SEV DMA. > >>> + ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == > >>> FW_CFG_DMA_CTL_READ || > >>> + Control == FW_CFG_DMA_CTL_SKIP); > >>> - @param[in] NumPage Number of pages. > >>> - @param[out] Buffer Allocated DMA Buffer pointer > >>> + if (Size == 0) { > >>> + return; > >>> + } > >>> + > >>> + if (MemEncryptSevIsEnabled ()) { > >>> + // > >>> + // SEV does not support DMA operations in PEI stage, we should > >>> + // not have reached here. > >>> + // > >>> + ASSERT (FALSE); > >>> + } > >> > >> (25) This is good; you are basically restoring / copying the > >> pre-7cfe445d7f4b state of the InternalQemuFwCfgDmaBytes() function, plus > >> adding this assert. But, it can be written better like this: > >> > >> // > >> // ... keep the same comment ... > >> // > >> ASSERT (!MemEncryptSevIsEnabled ()); > >> > >>> + > >>> + Access.Control = SwapBytes32 (Control); > >>> + Access.Length = SwapBytes32 (Size); > >>> + Access.Address = SwapBytes64 ((UINTN)Buffer); > >>> -**/ > >>> -VOID > >>> -InternalQemuFwCfgSevDmaAllocateBuffer ( > >>> - OUT VOID **Buffer, > >>> - IN UINT32 NumPages > >>> - ) > >>> -{ > >>> // > >>> - // We should never reach here > >>> + // Delimit the transfer from (a) modifications to Access, (b) in > >>> case of a > >>> + // write, from writes to Buffer by the caller. > >>> // > >>> - ASSERT (FALSE); > >>> - CpuDeadLoop (); > >>> -} > >>> + 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)); > >>> -/** > >>> - Free the DMA buffer allocated using > >>> InternalQemuFwCfgSevDmaAllocateBuffer > >>> + // > >>> + // Don't look at Access.Control before starting the transfer. > >>> + // > >>> + MemoryFence (); > >>> - @param[in] NumPage Number of pages. > >>> - @param[in] Buffer DMA Buffer pointer > >>> + // > >>> + // Wait for the transfer to complete. > >>> + // > >>> + do { > >>> + Status = SwapBytes32 (Access.Control); > >>> + ASSERT ((Status & FW_CFG_DMA_CTL_ERROR) == 0); > >>> + } while (Status != 0); > >>> -**/ > >>> -VOID > >>> -InternalQemuFwCfgSevDmaFreeBuffer ( > >>> - IN VOID *Buffer, > >>> - IN UINT32 NumPages > >>> - ) > >>> -{ > >>> // > >>> - // We should never reach here > >>> + // After a read, the caller will want to use Buffer. > >>> // > >>> - ASSERT (FALSE); > >>> - CpuDeadLoop (); > >>> + MemoryFence (); > >>> } > >>> + > >>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c > >>> b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c > >>> index 071b8d9b91d4..62ddb69d3b4d 100644 > >>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c > >>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c > >>> @@ -97,53 +97,25 @@ InternalQemuFwCfgDmaIsAvailable ( > >>> } > >>> /** > >>> + Transfer an array of bytes, or skip a number of bytes, using the DMA > >>> + interface. > >>> - Returns a boolean indicating whether SEV is enabled > >>> + @param[in] Size Size in bytes to transfer or skip. > >>> - @retval TRUE SEV is enabled > >>> - @retval FALSE SEV is disabled > >>> -**/ > >>> -BOOLEAN > >>> -InternalQemuFwCfgSevIsEnabled ( > >>> - VOID > >>> - ) > >>> -{ > >>> - // > >>> - // DMA is not supported in SEC phase hence SEV support is irrelevant > >>> - // > >>> - return FALSE; > >>> -} > >>> - > >>> -/** > >>> - 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 > >>> - ) > >>> -{ > >>> - // > >>> - // We should never reach here > >>> - // > >>> - ASSERT (FALSE); > >>> -} > >>> - > >>> -/** > >>> - Free the DMA buffer allocated using > >>> InternalQemuFwCfgSevDmaAllocateBuffer > >>> - > >>> - @param[in] NumPage Number of pages. > >>> - @param[in] Buffer DMA Buffer pointer > >>> + @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. > >>> **/ > >>> VOID > >>> -InternalQemuFwCfgSevDmaFreeBuffer ( > >>> - IN VOID *Buffer, > >>> - IN UINT32 NumPages > >>> +InternalQemuFwCfgDmaBytes ( > >>> + IN UINT32 Size, > >>> + IN OUT VOID *Buffer OPTIONAL, > >>> + IN UINT32 Control > >>> ) > >>> { > >>> // > >>> > >> > >> (26) Can you add a CpuDeadLoop() here as well (i.e., in the SEC > >> instance), after the ASSERT()? I know the previous version of the SEC > >> instance doesn't use CpuDeadLoop(), but I mentioned it here: > >> > >> http://mid.mail-archive.com/2b9361cc-03f9-a403-98aa-c1cf5bfd17c0@redhat.com > >> > >> > >> Thanks! > >> Laszlo > >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] OvmfPkg : QemuFwCfgLib: Use BusMasterCommandBuffer to map FW_CFG_DMA_ACCESS 2017-08-04 17:08 ` Jordan Justen @ 2017-08-04 20:25 ` Laszlo Ersek 2017-08-10 18:36 ` Jordan Justen 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2017-08-04 20:25 UTC (permalink / raw) To: Jordan Justen, Brijesh Singh, edk2-devel; +Cc: Tom Lendacky, Ard Biesheuvel On 08/04/17 19:08, Jordan Justen wrote: > On 2017-08-03 15:27:46, Laszlo Ersek wrote: >> On 08/04/17 00:07, Brijesh Singh wrote: >>> Hi Laszlo, >>> >>> Thanks for the detail review, I will soon send v3 with all your feedback >>> addressed. I must admit that I have constant struggle with formating issues >>> in EDKII contributions. While browsing the code, several packages have >>> code and comment exceeding 79 char. But looking at your previous feedbacks >>> somehow I was under assumption that comments should be <= 79 chars but code >>> can exceed 79 char limit. I think my understanding was wrong. I will update >>> my vi setting to warn me when I exceed 79 char limit. >> >> In the coding style, you will find: >> >> https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/#51-general-rules >> >> ---------- >> 5 Source Files >> 5.1 General Rules >> 5.1.1 Lines shall be 120 columns, or less >> >> Preferably, limit line lengths to 80 columns or less. When this doesn't >> leave sufficient space for a good postfix style comment, extend the line >> to a total of 120 columns. Having some level of uniformity in the >> expected width of the source is useful for viewing and printing the code. > > I personally don't like this wording. It basically says, 'we prefer > 80, but if that is too difficult, then 120 is ok'. The only case I've > ever seen of where code wouldn't fit reasonably in 80 columns, someone > was essentially putting a spreadsheet into code. (This is not exactly > a good case to build a code style around.) > >> ---------- >> >> I stick with 79 chars because they don't wrap in any kind of window >> sized to 80 columns. (Some terminal emulators / pager programs insert >> blank lines or wrap unexpectedly when a line is exactly 80 columns.) >> And, I like to size my windows to 80 columns because I use only one >> monitor (I dislike using more than one) and with 80 cols/window I can >> fit two windows side by side conveniently. > > History (punch cards) and personal workflows (fitting multiple > terminals) are often cited as reasons for the 'eighty column rule', > but more fundamentally, I prefer this argument: > > https://softwareengineering.stackexchange.com/a/222998 > > Essentially, there's a reason newspapers have columns. It is not easy > for humans to read very long lines of text. I appreciate that the stackoverflow comment also mentions "If lines are too short, text becomes hard to read because you must constantly jump from one line to the next while reading." > > I have another reason why going beyond 80 is not a good idea for code > that doesn't apply to normal reading. If you need ~120 columns visible > to view some lines, then most lines will end up having a lot of wasted > horizontal whitespace because they can commonly fit into 80 columns. One reason I dislike the "all arguments on separate lines" rule is just this -- it wastes perfectly good horizontal space, and eats up precious vertical space. We have a patch submission process for our documents now, but I don't think maintainers of other Pkgs would bother with following and enforcing a stricter 80 columns rule, even if we got it codified. :/ Thanks Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] OvmfPkg : QemuFwCfgLib: Use BusMasterCommandBuffer to map FW_CFG_DMA_ACCESS 2017-08-04 20:25 ` Laszlo Ersek @ 2017-08-10 18:36 ` Jordan Justen 2017-08-10 19:48 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Jordan Justen @ 2017-08-10 18:36 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel On 2017-08-04 13:25:09, Laszlo Ersek wrote: > > > > I have another reason why going beyond 80 is not a good idea for code > > that doesn't apply to normal reading. If you need ~120 columns visible > > to view some lines, then most lines will end up having a lot of wasted > > horizontal whitespace because they can commonly fit into 80 columns. > > One reason I dislike the "all arguments on separate lines" rule is just > this -- it wastes perfectly good horizontal space, and eats up precious > vertical space. > > We have a patch submission process for our documents now, I think I agree about the multiple args/multiple lines issue. I guess you could try to submit a code style doc change, but I'm not sure how controversial it might be. > but I don't > think maintainers of other Pkgs would bother with following and > enforcing a stricter 80 columns rule, even if we got it codified. :/ I still think we should have it written stricter even if it isn't always followed. It could be a step in the right direction. -Jordan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] OvmfPkg : QemuFwCfgLib: Use BusMasterCommandBuffer to map FW_CFG_DMA_ACCESS 2017-08-10 18:36 ` Jordan Justen @ 2017-08-10 19:48 ` Laszlo Ersek 0 siblings, 0 replies; 9+ messages in thread From: Laszlo Ersek @ 2017-08-10 19:48 UTC (permalink / raw) To: Jordan Justen, edk2-devel On 08/10/17 20:36, Jordan Justen wrote: > On 2017-08-04 13:25:09, Laszlo Ersek wrote: >>> >>> I have another reason why going beyond 80 is not a good idea for code >>> that doesn't apply to normal reading. If you need ~120 columns visible >>> to view some lines, then most lines will end up having a lot of wasted >>> horizontal whitespace because they can commonly fit into 80 columns. >> >> One reason I dislike the "all arguments on separate lines" rule is just >> this -- it wastes perfectly good horizontal space, and eats up precious >> vertical space. >> >> We have a patch submission process for our documents now, > > I think I agree about the multiple args/multiple lines issue. I guess > you could try to submit a code style doc change, but I'm not sure how > controversial it might be. > >> but I don't >> think maintainers of other Pkgs would bother with following and >> enforcing a stricter 80 columns rule, even if we got it codified. :/ > > I still think we should have it written stricter even if it isn't > always followed. It could be a step in the right direction. OK, I'm tagging this for later... Not sure how much later ;) Thanks Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-08-10 19:45 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-08-03 16:09 [PATCH v2 0/1] OvmfPkg/QemuFwCfgLib: Use BusMasterCommandBuffer to map FW_CFG_DMA_ACCESS Brijesh Singh 2017-08-03 16:10 ` [PATCH v2 1/1] OvmfPkg : QemuFwCfgLib: " Brijesh Singh 2017-08-03 20:51 ` Laszlo Ersek 2017-08-03 22:07 ` Brijesh Singh 2017-08-03 22:27 ` Laszlo Ersek 2017-08-04 17:08 ` Jordan Justen 2017-08-04 20:25 ` Laszlo Ersek 2017-08-10 18:36 ` Jordan Justen 2017-08-10 19:48 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox