Re: [PATCH v3 1/1] OvmfPkg/QemuFwCfgLib: Use BusMasterCommonBuffer to map FW_CFG_DMA_ACCESS

[Laszlo Ersek <lersek@redhat.com>]

On 08/04/17 01:11, Brijesh Singh wrote:
> Commit 09719a01b11b (OvmfPkg/QemuFwCfgLib: Implement SEV internal function
> for Dxe phase) uses IOMMU protocol to allocate and free FW_CFG_DMA_ACCESS
> buffer when SEV is active. During initial commits we made assumption that
> IOMMU.AllocateBuffer() will provide PlainTextAddress (i.e C-bit cleared).
> This assumption was wrong, the AllocateBuffer() protocol member is not
> expected to produce a buffer that is immediatly usable, and client is
> required to call Map() uncondtionally with BusMasterCommonBuffer[64] to
> get a mapping which is accessable by both host and device.
>
> The patch refactors code a bit and add the support to Map()
> FW_CFG_DMA_ACCESS buffer using BusMasterCommonBuffer operation after
> allocation and Unamp() before free.
>
> The complete discussion about this and recommendation from Laszlo can be
> found here [1]
>
> [1] https://lists.01.org/pipermail/edk2-devel/2017-July/012652.html
>
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0

For the future: please use version 1.1 of the agreement:

https://github.com/tianocore/edk2/commit/b6538c118ae8

> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h |  42 +--
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c         | 330 ++++++++++++++++----
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c         | 130 --------
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c         |  99 +++---
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c         |  58 +---
>  5 files changed, 367 insertions(+), 292 deletions(-)
>

[lersek@redhat.com: convert pointers to UINTN before converting to UINT64]
[lersek@redhat.com: fix argument indentation in multi-line function call]
[lersek@redhat.com: explicitly compare pointers to NULL]

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>

Pushed as commit f6c909ae5d65.

(See the fixups below, formatted with function context for easier
understanding.)

Thanks
Laszlo

> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
> index 576941749cf2..8b98208591e6 100644
> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
> @@ -232,34 +232,34 @@ VOID
>  FreeFwCfgDmaAccessBuffer (
>    IN  VOID    *Access,
>    IN  VOID    *Mapping
>    )
>  {
>    UINTN       NumPages;
>    EFI_STATUS  Status;
>
>    NumPages = EFI_SIZE_TO_PAGES (sizeof (FW_CFG_DMA_ACCESS));
>
>    Status = mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR,
>        "%a:%a failed to UnMap() Mapping 0x%Lx\n", gEfiCallerBaseName,
> -      __FUNCTION__, (UINT64)Mapping));
> +      __FUNCTION__, (UINT64)(UINTN)Mapping));
>      ASSERT (FALSE);
>      CpuDeadLoop ();
>    }
>
>    Status = mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, Access);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR,
>        "%a:%a failed to Free() 0x%Lx\n", gEfiCallerBaseName, __FUNCTION__,
> -      (UINT64) Access));
> +      (UINT64)(UINTN)Access));
>      ASSERT (FALSE);
>      CpuDeadLoop ();
>    }
>  }
>
>  /**
>    Function is used for mapping host address to device address. The buffer must
>    be unmapped with UnmapDmaDataBuffer ().
>
>  **/
> @@ -268,44 +268,44 @@ VOID
>  MapFwCfgDmaDataBuffer (
>    IN  BOOLEAN               IsWrite,
>    IN  VOID                  *HostAddress,
>    IN  UINT32                Size,
>    OUT EFI_PHYSICAL_ADDRESS  *DeviceAddress,
>    OUT VOID                  **MapInfo
>    )
>  {
>    EFI_STATUS              Status;
>    UINTN                   NumberOfBytes;
>    VOID                    *Mapping;
>    EFI_PHYSICAL_ADDRESS    PhysicalAddress;
>
>    NumberOfBytes = Size;
>    Status = mIoMmuProtocol->Map (
>                               mIoMmuProtocol,
>                               (IsWrite ?
>                                EdkiiIoMmuOperationBusMasterRead64 :
>                                EdkiiIoMmuOperationBusMasterWrite64),
>                               HostAddress,
>                               &NumberOfBytes,
>                               &PhysicalAddress,
>                               &Mapping
>                               );
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR,
>        "%a:%a failed to Map() Address 0x%Lx Size 0x%Lx\n", gEfiCallerBaseName,
> -      __FUNCTION__, (UINT64) HostAddress, (UINT64)Size));
> +      __FUNCTION__, (UINT64)(UINTN)HostAddress, (UINT64)Size));
>      ASSERT (FALSE);
>      CpuDeadLoop ();
>    }
>
>    if (NumberOfBytes < Size) {
>      mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping);
>      DEBUG ((DEBUG_ERROR,
>        "%a:%a failed to Map() - requested 0x%x got 0x%Lx\n", gEfiCallerBaseName,
>        __FUNCTION__, Size, (UINT64)NumberOfBytes));
>      ASSERT (FALSE);
>      CpuDeadLoop ();
>    }
>
>    *DeviceAddress = PhysicalAddress;
>    *MapInfo = Mapping;
>  }
> @@ -315,31 +315,31 @@ VOID
>  UnmapFwCfgDmaDataBuffer (
>    IN  VOID  *Mapping
>    )
>  {
>    EFI_STATUS  Status;
>
>    Status = mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR,
>        "%a:%a failed to UnMap() Mapping 0x%Lx\n", gEfiCallerBaseName,
> -      __FUNCTION__, (UINT64)Mapping));
> +      __FUNCTION__, (UINT64)(UINTN)Mapping));
>      ASSERT (FALSE);
>      CpuDeadLoop ();
>    }
>  }
>
>  /**
>    Transfer an array of bytes, or skip a number of bytes, using the DMA
>    interface.
>
>    @param[in]     Size     Size in bytes to transfer or skip.
>
>    @param[in,out] Buffer   Buffer to read data into or write data from. Ignored,
>                            and may be NULL, if Size is zero, or Control is
>                            FW_CFG_DMA_CTL_SKIP.
>
>    @param[in]     Control  One of the following:
>                            FW_CFG_DMA_CTL_WRITE - write to fw_cfg from Buffer.
>                            FW_CFG_DMA_CTL_READ  - read from fw_cfg into Buffer.
>                            FW_CFG_DMA_CTL_SKIP  - skip bytes in fw_cfg.
>  **/
> @@ -347,106 +347,106 @@ VOID
>  InternalQemuFwCfgDmaBytes (
>    IN     UINT32   Size,
>    IN OUT VOID     *Buffer OPTIONAL,
>    IN     UINT32   Control
>    )
>  {
>    volatile FW_CFG_DMA_ACCESS LocalAccess;
>    volatile FW_CFG_DMA_ACCESS *Access;
>    UINT32                     AccessHigh, AccessLow;
>    UINT32                     Status;
>    VOID                       *AccessMapping, *DataMapping;
>    VOID                       *DataBuffer;
>
>    ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ ||
>      Control == FW_CFG_DMA_CTL_SKIP);
>
>    if (Size == 0) {
>      return;
>    }
>
>    Access = &LocalAccess;
>    AccessMapping = NULL;
>    DataMapping = NULL;
>    DataBuffer = Buffer;
>
>    //
>    // When SEV is enabled, map Buffer to DMA address before issuing the DMA
>    // request
>    //
>    if (MemEncryptSevIsEnabled ()) {
>      VOID                  *AccessBuffer;
>      EFI_PHYSICAL_ADDRESS  DataBufferAddress;
>
>      //
>      // Allocate DMA Access buffer
>      //
>      AllocFwCfgDmaAccessBuffer (&AccessBuffer, &AccessMapping);
>
>      Access = AccessBuffer;
>
>      //
>      // Map actual data buffer
>      //
>      if (Control != FW_CFG_DMA_CTL_SKIP) {
>        MapFwCfgDmaDataBuffer (
> -                             Control == FW_CFG_DMA_CTL_WRITE,
> -                             Buffer,
> -                             Size,
> -                             &DataBufferAddress,
> -                             &DataMapping
> -                             );
> +        Control == FW_CFG_DMA_CTL_WRITE,
> +        Buffer,
> +        Size,
> +        &DataBufferAddress,
> +        &DataMapping
> +        );
>
>        DataBuffer = (VOID *) (UINTN) DataBufferAddress;
>      }
>    }
>
>    Access->Control = SwapBytes32 (Control);
>    Access->Length  = SwapBytes32 (Size);
>    Access->Address = SwapBytes64 ((UINTN)DataBuffer);
>
>    //
>    // Delimit the transfer from (a) modifications to Access, (b) in case of a
>    // write, from writes to Buffer by the caller.
>    //
>    MemoryFence ();
>
>    //
>    // Start the transfer.
>    //
>    AccessHigh = (UINT32)RShiftU64 ((UINTN)Access, 32);
>    AccessLow  = (UINT32)(UINTN)Access;
>    IoWrite32 (FW_CFG_IO_DMA_ADDRESS,     SwapBytes32 (AccessHigh));
>    IoWrite32 (FW_CFG_IO_DMA_ADDRESS + 4, SwapBytes32 (AccessLow));
>
>    //
>    // Don't look at Access.Control before starting the transfer.
>    //
>    MemoryFence ();
>
>    //
>    // Wait for the transfer to complete.
>    //
>    do {
>      Status = SwapBytes32 (Access->Control);
>      ASSERT ((Status & FW_CFG_DMA_CTL_ERROR) == 0);
>    } while (Status != 0);
>
>    //
>    // After a read, the caller will want to use Buffer.
>    //
>    MemoryFence ();
>
>    //
>    // If Access buffer was dynamically allocated then free it.
>    //
> -  if (AccessMapping) {
> +  if (AccessMapping != NULL) {
>      FreeFwCfgDmaAccessBuffer ((VOID *)Access, AccessMapping);
>    }
>
>    //
>    // If DataBuffer was mapped then unmap it.
>    //
> -  if (DataMapping) {
> +  if (DataMapping != NULL) {
>      UnmapFwCfgDmaDataBuffer (DataMapping);
>    }
>  }