At the moment, we have the following distribution of actions between the
IOMMU protocol member functions:

- AllocateBuffer() allocates pages and clears the memory encryption mask.

- FreeBuffer() re-sets the memory encryption mask, and deallocates pages.

- Map() does nothing at all when BusMasterCommonBuffer[64] is requested
  (and AllocateBuffer() was called previously). Otherwise, Map() allocates
  pages, and clears the memory encryption mask.

- Unmap() does nothing when cleaning up a BusMasterCommonBuffer[64]
  operation. Otherwise, Unmap() clears the encryption mask, and frees the
  pages.

This is wrong: the AllocateBuffer() protocol member is not expected to
produce a buffer that is immediately usable, and client code is required
to call Map() unconditionally, even if BusMasterCommonBuffer[64] is the
desired operation. Implement the right distribution of actions as follows:

- AllocateBuffer() allocates pages and does not touch the encryption mask.

- FreeBuffer() deallocates pages and does not touch the encryption mask.

- Map() does not allocate pages when BusMasterCommonBuffer[64] is
  requested, and it allocates pages (bounce buffer) otherwise.  Regardless
  of the BusMaster operation, Map() (and Map() only) clears the memory
  encryption mask.

- Unmap() restores the encryption mask unconditionally. If the operation
  was BusMasterCommonBuffer[64], then Unmap() does not release the pages.
  Otherwise, the pages (bounce buffer) are released.

This approach also ensures that Unmap() can be called from
ExitBootServices() event handlers, for cleaning up
BusMasterCommonBuffer[64] operations. (More specifically, for restoring
the SEV encryption mask on any in-flight buffers, after resetting any
referring devices.) ExitBootServices() event handlers must not change the
UEFI memory map, thus any memory allocation or freeing in Unmap() would
disqualify Unmap() from being called in such a context.

Map()-ing and Unmap()-ing memory for a BusMasterCommonBuffer[64] operation
effectively means in-place decryption and encryption in a SEV context. As
an additional hurdle, section "7.10.8 Encrypt-in-Place" of AMD publication
Nr.24593 implies that we need a separate temporary buffer for decryption
and encryption that will eventually land in-place. Allocating said
temporary buffer in the straightforward way would violate the above
allocation/freeing restrictions on Map()/Unmap(), therefore pre-allocate
this "stash buffer" too in AllocateBuffer(), and free it in FreeBuffer().

To completely rid Unmap() of dynamic memory impact, for
BusMasterCommonBuffer[64] operations, we're going to rework the lifecycle of
the MAP_INFO structures in a later patch.

(The MemEncryptSevSetPageEncMask() call in Unmap() could theoretically
allocate memory internally for page splitting, however this won't happen
in practice: in Unmap() we only restore the memory encryption mask, and
don't genuinely set it. Any page splitting will have occurred in Map()'s
MemEncryptSevClearPageEncMask() call first.)

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 242 +++++++++++++++-----
 1 file changed, 185 insertions(+), 57 deletions(-)

diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 0a85ee6559e7..5049d19e9cb7 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -23,40 +23,64 @@
 typedef struct {
   EDKII_IOMMU_OPERATION                     Operation;
   UINTN                                     NumberOfBytes;
   UINTN                                     NumberOfPages;
   EFI_PHYSICAL_ADDRESS                      CryptedAddress;
   EFI_PHYSICAL_ADDRESS                      PlainTextAddress;
 } MAP_INFO;
 
-#define NO_MAPPING             (VOID *) (UINTN) -1
+#define COMMON_BUFFER_SIG SIGNATURE_64 ('C', 'M', 'N', 'B', 'U', 'F', 'F', 'R')
+
+//
+// The following structure enables Map() and Unmap() to perform in-place
+// decryption and encryption, respectively, for BusMasterCommonBuffer[64]
+// operations, without dynamic memory allocation or release.
+//
+// Both COMMON_BUFFER_HEADER and COMMON_BUFFER_HEADER.StashBuffer are allocated
+// by AllocateBuffer() and released by FreeBuffer().
+//
+#pragma pack (1)
+typedef struct {
+  UINT64 Signature;
+
+  //
+  // Always allocated from EfiBootServicesData type memory, and always
+  // encrypted.
+  //
+  VOID *StashBuffer;
+
+  //
+  // Followed by the actual common buffer, starting at the next page.
+  //
+} COMMON_BUFFER_HEADER;
+#pragma pack ()
 
 /**
   Provides the controller-specific addresses required to access system memory
   from a DMA bus master. On SEV guest, the DMA operations must be performed on
   shared buffer hence we allocate a bounce buffer to map the HostAddress to a
   DeviceAddress. The Encryption attribute is removed from the DeviceAddress
   buffer.
 
   @param  This                  The protocol instance pointer.
   @param  Operation             Indicates if the bus master is going to read or
                                 write to system memory.
   @param  HostAddress           The system memory address to map to the PCI
                                 controller.
   @param  NumberOfBytes         On input the number of bytes to map. On output
                                 the number of bytes that were mapped.
   @param  DeviceAddress         The resulting map address for the bus master
                                 PCI controller to use to access the hosts
                                 HostAddress.
   @param  Mapping               A resulting value to pass to Unmap().
 
   @retval EFI_SUCCESS           The range was mapped for the returned
                                 NumberOfBytes.
   @retval EFI_UNSUPPORTED       The HostAddress cannot be mapped as a common
                                 buffer.
   @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
   @retval EFI_OUT_OF_RESOURCES  The request could not be completed due to a
                                 lack of resources.
   @retval EFI_DEVICE_ERROR      The system hardware could not map the requested
                                 address.
 
 **/
@@ -65,157 +89,175 @@ EFIAPI
 IoMmuMap (
   IN     EDKII_IOMMU_PROTOCOL                       *This,
   IN     EDKII_IOMMU_OPERATION                      Operation,
   IN     VOID                                       *HostAddress,
   IN OUT UINTN                                      *NumberOfBytes,
   OUT    EFI_PHYSICAL_ADDRESS                       *DeviceAddress,
   OUT    VOID                                       **Mapping
   )
 {
   EFI_STATUS                                        Status;
   MAP_INFO                                          *MapInfo;
   EFI_ALLOCATE_TYPE                                 AllocateType;
+  COMMON_BUFFER_HEADER                              *CommonBufferHeader;
+  VOID                                              *DecryptionSource;
 
   if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
       Mapping == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
   //
   // Allocate a MAP_INFO structure to remember the mapping when Unmap() is
   // called later.
   //
   MapInfo = AllocatePool (sizeof (MAP_INFO));
   if (MapInfo == NULL) {
     Status = EFI_OUT_OF_RESOURCES;
     goto Failed;
   }
 
   //
   // Initialize the MAP_INFO structure, except the PlainTextAddress field
   //
   MapInfo->Operation         = Operation;
   MapInfo->NumberOfBytes     = *NumberOfBytes;
   MapInfo->NumberOfPages     = EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes);
   MapInfo->CryptedAddress    = (UINTN)HostAddress;
 
   //
   // In the switch statement below, we point "MapInfo->PlainTextAddress" to the
-  // plaintext buffer, according to Operation.
+  // plaintext buffer, according to Operation. We also set "DecryptionSource".
   //
   MapInfo->PlainTextAddress = MAX_ADDRESS;
   AllocateType = AllocateAnyPages;
+  DecryptionSource = (VOID *)(UINTN)MapInfo->CryptedAddress;
   switch (Operation) {
   //
   // For BusMasterRead[64] and BusMasterWrite[64] operations, a bounce buffer
   // is necessary regardless of whether the original (crypted) buffer crosses
   // the 4GB limit or not -- we have to allocate a separate plaintext buffer.
   // The only variable is whether the plaintext buffer should be under 4GB.
   //
   case EdkiiIoMmuOperationBusMasterRead:
   case EdkiiIoMmuOperationBusMasterWrite:
     MapInfo->PlainTextAddress = BASE_4GB - 1;
     AllocateType = AllocateMaxAddress;
     //
     // fall through
     //
   case EdkiiIoMmuOperationBusMasterRead64:
   case EdkiiIoMmuOperationBusMasterWrite64:
     //
     // Allocate the implicit plaintext bounce buffer.
     //
     Status = gBS->AllocatePages (
                     AllocateType,
                     EfiBootServicesData,
                     MapInfo->NumberOfPages,
                     &MapInfo->PlainTextAddress
                     );
     if (EFI_ERROR (Status)) {
       goto FreeMapInfo;
     }
     break;
 
   //
-  // For BusMasterCommonBuffer[64] operations, a plaintext buffer has been
-  // allocated already, with AllocateBuffer(). We only check whether the
-  // address is low enough for the requested operation.
+  // For BusMasterCommonBuffer[64] operations, a to-be-plaintext buffer and a
+  // stash buffer (for in-place decryption) have been allocated already, with
+  // AllocateBuffer(). We only check whether the address of the to-be-plaintext
+  // buffer is low enough for the requested operation.
   //
   case EdkiiIoMmuOperationBusMasterCommonBuffer:
     if ((MapInfo->CryptedAddress > BASE_4GB) ||
         (EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages) >
          BASE_4GB - MapInfo->CryptedAddress)) {
       //
       // CommonBuffer operations cannot be remapped. If the common buffer is
       // above 4GB, then it is not possible to generate a mapping, so return an
       // error.
       //
       Status = EFI_UNSUPPORTED;
       goto FreeMapInfo;
     }
     //
     // fall through
     //
   case EdkiiIoMmuOperationBusMasterCommonBuffer64:
     //
-    // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer(),
-    // and it is already decrypted.
+    // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer().
     //
     MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
-
     //
-    // Therefore no mapping is necessary.
+    // Stash the crypted data.
     //
-    *DeviceAddress = MapInfo->PlainTextAddress;
-    *Mapping       = NO_MAPPING;
-    FreePool (MapInfo);
-    return EFI_SUCCESS;
+    CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
+                           (UINTN)MapInfo->CryptedAddress - EFI_PAGE_SIZE
+                           );
+    ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG);
+    CopyMem (
+      CommonBufferHeader->StashBuffer,
+      (VOID *)(UINTN)MapInfo->CryptedAddress,
+      MapInfo->NumberOfBytes
+      );
+    //
+    // Point "DecryptionSource" to the stash buffer so that we decrypt
+    // it to the original location, after the switch statement.
+    //
+    DecryptionSource = CommonBufferHeader->StashBuffer;
+    break;
 
   default:
     //
     // Operation is invalid
     //
     Status = EFI_INVALID_PARAMETER;
     goto FreeMapInfo;
   }
 
   //
   // Clear the memory encryption mask on the plaintext buffer.
   //
   Status = MemEncryptSevClearPageEncMask (
              0,
              MapInfo->PlainTextAddress,
              MapInfo->NumberOfPages,
              TRUE
              );
   ASSERT_EFI_ERROR(Status);
 
   //
   // If this is a read operation from the Bus Master's point of view,
   // then copy the contents of the real buffer into the mapped buffer
   // so the Bus Master can read the contents of the real buffer.
   //
+  // For BusMasterCommonBuffer[64] operations, the CopyMem() below will decrypt
+  // the original data (from the stash buffer) back to the original location.
+  //
   if (Operation == EdkiiIoMmuOperationBusMasterRead ||
-      Operation == EdkiiIoMmuOperationBusMasterRead64) {
+      Operation == EdkiiIoMmuOperationBusMasterRead64 ||
+      Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
+      Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
     CopyMem (
       (VOID *) (UINTN) MapInfo->PlainTextAddress,
-      (VOID *) (UINTN) MapInfo->CryptedAddress,
+      DecryptionSource,
       MapInfo->NumberOfBytes
       );
   }
 
   //
   // Populate output parameters.
   //
   *DeviceAddress = MapInfo->PlainTextAddress;
   *Mapping       = MapInfo;
 
   DEBUG ((
     DEBUG_VERBOSE,
     "%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
     __FUNCTION__,
     MapInfo->PlainTextAddress,
     MapInfo->CryptedAddress,
     (UINT64)MapInfo->NumberOfPages,
     (UINT64)MapInfo->NumberOfBytes
     ));
 
   return EFI_SUCCESS;
@@ -245,91 +287,129 @@ EFIAPI
 IoMmuUnmap (
   IN  EDKII_IOMMU_PROTOCOL                     *This,
   IN  VOID                                     *Mapping
   )
 {
   MAP_INFO                 *MapInfo;
   EFI_STATUS               Status;
+  COMMON_BUFFER_HEADER     *CommonBufferHeader;
+  VOID                     *EncryptionTarget;
 
   if (Mapping == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
-  //
-  // See if the Map() operation associated with this Unmap() required a mapping
-  // buffer. If a mapping buffer was not required, then this function simply
-  // buffer. If a mapping buffer was not required, then this function simply
-  //
-  if (Mapping == NO_MAPPING) {
-    return EFI_SUCCESS;
-  }
-
   MapInfo = (MAP_INFO *)Mapping;
 
   //
-  // If this is a write operation from the Bus Master's point of view,
-  // then copy the contents of the mapped buffer into the real buffer
-  // so the processor can read the contents of the real buffer.
+  // For BusMasterWrite[64] operations and BusMasterCommonBuffer[64] operations
+  // we have to encrypt the results, ultimately to the original place (i.e.,
+  // "MapInfo->CryptedAddress").
   //
-  if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite ||
-      MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite64) {
+  // For BusMasterCommonBuffer[64] operations however, this encryption has to
+  // land in-place, so divert the encryption to the stash buffer first.
+  //
+  EncryptionTarget = (VOID *)(UINTN)MapInfo->CryptedAddress;
+
+  switch (MapInfo->Operation) {
+  case EdkiiIoMmuOperationBusMasterCommonBuffer:
+  case EdkiiIoMmuOperationBusMasterCommonBuffer64:
+    ASSERT (MapInfo->PlainTextAddress == MapInfo->CryptedAddress);
+
+    CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
+                           (UINTN)MapInfo->PlainTextAddress - EFI_PAGE_SIZE
+                           );
+    ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG);
+    EncryptionTarget = CommonBufferHeader->StashBuffer;
+    //
+    // fall through
+    //
+
+  case EdkiiIoMmuOperationBusMasterWrite:
+  case EdkiiIoMmuOperationBusMasterWrite64:
     CopyMem (
-      (VOID *) (UINTN) MapInfo->CryptedAddress,
+      EncryptionTarget,
       (VOID *) (UINTN) MapInfo->PlainTextAddress,
       MapInfo->NumberOfBytes
       );
+    break;
+
+  default:
+    //
+    // nothing to encrypt after BusMasterRead[64] operations
+    //
+    break;
   }
 
   DEBUG ((
     DEBUG_VERBOSE,
     "%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
     __FUNCTION__,
     MapInfo->PlainTextAddress,
     MapInfo->CryptedAddress,
     (UINT64)MapInfo->NumberOfPages,
     (UINT64)MapInfo->NumberOfBytes
     ));
+
   //
-  // Restore the memory encryption mask
+  // Restore the memory encryption mask on the area we used to hold the
+  // plaintext.
   //
   Status = MemEncryptSevSetPageEncMask (
              0,
              MapInfo->PlainTextAddress,
              MapInfo->NumberOfPages,
              TRUE
              );
   ASSERT_EFI_ERROR(Status);
-  ZeroMem (
-    (VOID*)(UINTN)MapInfo->PlainTextAddress,
-    EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages)
-    );
 
   //
-  // Free the mapped buffer and the MAP_INFO structure.
+  // For BusMasterCommonBuffer[64] operations, copy the stashed data to the
+  // original (now encrypted) location.
+  //
+  // For all other operations, fill the late bounce buffer (which existed as
+  // plaintext at some point) with zeros, and then release it.
+  //
+  if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
+      MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
+    CopyMem (
+      (VOID *)(UINTN)MapInfo->CryptedAddress,
+      CommonBufferHeader->StashBuffer,
+      MapInfo->NumberOfBytes
+      );
+  } else {
+    ZeroMem (
+      (VOID *)(UINTN)MapInfo->PlainTextAddress,
+      EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages)
+      );
+    gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
+  }
+
+  //
+  // Free the MAP_INFO structure.
   //
-  gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
   FreePool (Mapping);
   return EFI_SUCCESS;
 }
 
 /**
   Allocates pages that are suitable for an OperationBusMasterCommonBuffer or
   OperationBusMasterCommonBuffer64 mapping.
 
   @param  This                  The protocol instance pointer.
   @param  Type                  This parameter is not used and must be ignored.
   @param  MemoryType            The type of memory to allocate,
                                 EfiBootServicesData or EfiRuntimeServicesData.
   @param  Pages                 The number of pages to allocate.
   @param  HostAddress           A pointer to store the base system memory
                                 address of the allocated range.
   @param  Attributes            The requested bit mask of attributes for the
                                 allocated range.
 
   @retval EFI_SUCCESS           The requested memory pages were allocated.
   @retval EFI_UNSUPPORTED       Attributes is unsupported. The only legal
                                 attribute bits are MEMORY_WRITE_COMBINE and
                                 MEMORY_CACHED.
   @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
   @retval EFI_OUT_OF_RESOURCES  The memory pages could not be allocated.
 
 **/
@@ -338,81 +418,116 @@ EFIAPI
 IoMmuAllocateBuffer (
   IN     EDKII_IOMMU_PROTOCOL                     *This,
   IN     EFI_ALLOCATE_TYPE                        Type,
   IN     EFI_MEMORY_TYPE                          MemoryType,
   IN     UINTN                                    Pages,
   IN OUT VOID                                     **HostAddress,
   IN     UINT64                                   Attributes
   )
 {
   EFI_STATUS                Status;
   EFI_PHYSICAL_ADDRESS      PhysicalAddress;
+  VOID                      *StashBuffer;
+  UINTN                     CommonBufferPages;
+  COMMON_BUFFER_HEADER      *CommonBufferHeader;
 
   //
   // Validate Attributes
   //
   if ((Attributes & EDKII_IOMMU_ATTRIBUTE_INVALID_FOR_ALLOCATE_BUFFER) != 0) {
     return EFI_UNSUPPORTED;
   }
 
   //
   // Check for invalid inputs
   //
   if (HostAddress == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
   //
   // The only valid memory types are EfiBootServicesData and
   // EfiRuntimeServicesData
   //
   if (MemoryType != EfiBootServicesData &&
       MemoryType != EfiRuntimeServicesData) {
     return EFI_INVALID_PARAMETER;
   }
 
+  //
+  // We'll need a header page for the COMMON_BUFFER_HEADER structure.
+  //
+  if (Pages > MAX_UINTN - 1) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+  CommonBufferPages = Pages + 1;
+
+  //
+  // Allocate the stash in EfiBootServicesData type memory.
+  //
+  // Map() will temporarily save encrypted data in the stash for
+  // BusMasterCommonBuffer[64] operations, so the data can be decrypted to the
+  // original location.
+  //
+  // Unmap() will temporarily save plaintext data in the stash for
+  // BusMasterCommonBuffer[64] operations, so the data can be encrypted to the
+  // original location.
+  //
+  // StashBuffer always resides in encrypted memory.
+  //
+  StashBuffer = AllocatePages (Pages);
+  if (StashBuffer == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
   PhysicalAddress = (UINTN)-1;
   if ((Attributes & EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0) {
     //
     // Limit allocations to memory below 4GB
     //
     PhysicalAddress = SIZE_4GB - 1;
   }
   Status = gBS->AllocatePages (
                   AllocateMaxAddress,
                   MemoryType,
-                  Pages,
+                  CommonBufferPages,
                   &PhysicalAddress
                   );
-  if (!EFI_ERROR (Status)) {
-    *HostAddress = (VOID *) (UINTN) PhysicalAddress;
-
-    //
-    // Clear memory encryption mask
-    //
-    Status = MemEncryptSevClearPageEncMask (0, PhysicalAddress, Pages, TRUE);
-    ASSERT_EFI_ERROR(Status);
+  if (EFI_ERROR (Status)) {
+    goto FreeStashBuffer;
   }
 
+  CommonBufferHeader = (VOID *)(UINTN)PhysicalAddress;
+  PhysicalAddress += EFI_PAGE_SIZE;
+
+  CommonBufferHeader->Signature = COMMON_BUFFER_SIG;
+  CommonBufferHeader->StashBuffer = StashBuffer;
+
+  *HostAddress = (VOID *)(UINTN)PhysicalAddress;
+
   DEBUG ((
     DEBUG_VERBOSE,
     "%a Address 0x%Lx Pages 0x%Lx\n",
     __FUNCTION__,
     PhysicalAddress,
     (UINT64)Pages
     ));
+  return EFI_SUCCESS;
+
+FreeStashBuffer:
+  FreePages (StashBuffer, Pages);
   return Status;
 }
 
 /**
   Frees memory that was allocated with AllocateBuffer().
 
   @param  This                  The protocol instance pointer.
   @param  Pages                 The number of pages to free.
   @param  HostAddress           The base system memory address of the allocated
                                 range.
 
   @retval EFI_SUCCESS           The requested memory pages were freed.
   @retval EFI_INVALID_PARAMETER The memory range specified by HostAddress and
                                 Pages was not allocated with AllocateBuffer().
 
 **/
@@ -421,79 +536,92 @@ EFIAPI
 IoMmuFreeBuffer (
   IN  EDKII_IOMMU_PROTOCOL                     *This,
   IN  UINTN                                    Pages,
   IN  VOID                                     *HostAddress
   )
 {
-  EFI_STATUS  Status;
+  UINTN                CommonBufferPages;
+  COMMON_BUFFER_HEADER *CommonBufferHeader;
+
+  CommonBufferPages = Pages + 1;
+  CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
+                         (UINTN)HostAddress - EFI_PAGE_SIZE
+                         );
+
+  //
+  // Check the signature.
+  //
+  ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG);
+  if (CommonBufferHeader->Signature != COMMON_BUFFER_SIG) {
+    return EFI_INVALID_PARAMETER;
+  }
 
   //
-  // Set memory encryption mask
+  // Free the stash buffer. This buffer was always encrypted, so no need to
+  // zero it.
   //
-  Status = MemEncryptSevSetPageEncMask (
-             0,
-             (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress,
-             Pages,
-             TRUE
-             );
-  ASSERT_EFI_ERROR(Status);
-  ZeroMem (HostAddress, EFI_PAGES_TO_SIZE (Pages));
+  FreePages (CommonBufferHeader->StashBuffer, Pages);
 
   DEBUG ((
     DEBUG_VERBOSE,
     "%a Address 0x%Lx Pages 0x%Lx\n",
     __FUNCTION__,
     (UINT64)(UINTN)HostAddress,
     (UINT64)Pages
     ));
-  return gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress, Pages);
+
+  //
+  // Release the common buffer itself. Unmap() has re-encrypted it in-place, so
+  // no need to zero it.
+  //
+  return gBS->FreePages ((UINTN)CommonBufferHeader, CommonBufferPages);
 }
 
 
 /**
   Set IOMMU attribute for a system memory.
 
   If the IOMMU protocol exists, the system memory cannot be used
   for DMA by default.
 
   When a device requests a DMA access for a system memory,
   the device driver need use SetAttribute() to update the IOMMU
   attribute to request DMA access (read and/or write).
 
   The DeviceHandle is used to identify which device submits the request.
   The IOMMU implementation need translate the device path to an IOMMU device
   ID, and set IOMMU hardware register accordingly.
   1) DeviceHandle can be a standard PCI device.
      The memory for BusMasterRead need set EDKII_IOMMU_ACCESS_READ.
      The memory for BusMasterWrite need set EDKII_IOMMU_ACCESS_WRITE.
      The memory for BusMasterCommonBuffer need set
      EDKII_IOMMU_ACCESS_READ|EDKII_IOMMU_ACCESS_WRITE.
      After the memory is used, the memory need set 0 to keep it being
      protected.
   2) DeviceHandle can be an ACPI device (ISA, I2C, SPI, etc).
      The memory for DMA access need set EDKII_IOMMU_ACCESS_READ and/or
      EDKII_IOMMU_ACCESS_WRITE.
 
   @param[in]  This              The protocol instance pointer.
   @param[in]  DeviceHandle      The device who initiates the DMA access
                                 request.
   @param[in]  Mapping           The mapping value returned from Map().
   @param[in]  IoMmuAccess       The IOMMU access.
 
   @retval EFI_SUCCESS            The IoMmuAccess is set for the memory range
                                  specified by DeviceAddress and Length.
   @retval EFI_INVALID_PARAMETER  DeviceHandle is an invalid handle.
   @retval EFI_INVALID_PARAMETER  Mapping is not a value that was returned by
                                  Map().
   @retval EFI_INVALID_PARAMETER  IoMmuAccess specified an illegal combination
                                  of access.
   @retval EFI_UNSUPPORTED        DeviceHandle is unknown by the IOMMU.
   @retval EFI_UNSUPPORTED        The bit mask of IoMmuAccess is not supported
                                  by the IOMMU.
   @retval EFI_UNSUPPORTED        The IOMMU does not support the memory range
                                  specified by Mapping.
   @retval EFI_OUT_OF_RESOURCES   There are not enough resources available to
                                  modify the IOMMU access.
   @retval EFI_DEVICE_ERROR       The IOMMU device reported an error while
                                  attempting the operation.
 
 **/
-- 
2.13.1.3.g8be5a757fa67


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 8/2/17 4:24 PM, Laszlo Ersek wrote:

[Snip]
> At the moment, we have the foll+    // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer().
>      //
>      MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
> -
>      //
> -    // Therefore no mapping is necessary.
> +    // Stash the crypted data.
>      //
> -    *DeviceAddress = MapInfo->PlainTextAddress;
> -    *Mapping       = NO_MAPPING;
> -    FreePool (MapInfo);
> -    return EFI_SUCCESS;
> +    CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
> +                           (UINTN)MapInfo->CryptedAddress - EFI_PAGE_SIZE
> +                           );

One question, per spec, is it legal for client to call Map() at some
offset within allocated buffer ?

e.g something like this:

* AllocateBuffer (, 1, &Buffer);
* MapBuffer = Buffer + 10;
* Map (, BusMasterCommonBuffer, MappedBuffer, 10, ..) // Bascially Map
10 bytes from offset 10

If this is legal then we may need to build MapInfo during
AllocateBuffer() to locate the "StashBuffer". So far, I have not came
across this usecase but I wanted to check with you so that we don't fail
on corner cases. Good part if you have ASSERT() so we should be able to
catch them (if any).

> +    ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG);
> +    CopyMem (
> +      CommonBufferHeader->StashBuffer,
> +      (VOID *)(UINTN)MapInfo->CryptedAddress,
> +      MapInfo->NumberOfBytes
> +      );
> +    //
> +    // Point "DecryptionSource" to the stash buffer so that we decrypt
> +    // it to the original location, after the switch statement.
> +    //
> +    DecryptionSource = CommonBufferHeader->StashBuffer;
> +    break;
>  
[Snip]

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

(CC Andrew)

On 08/03/17 01:01, Brijesh Singh wrote:
>
>
> On 8/2/17 4:24 PM, Laszlo Ersek wrote:
>
> [Snip]
>> At the moment, we have the foll+    // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer().
>>      //
>>      MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
>> -
>>      //
>> -    // Therefore no mapping is necessary.
>> +    // Stash the crypted data.
>>      //
>> -    *DeviceAddress = MapInfo->PlainTextAddress;
>> -    *Mapping       = NO_MAPPING;
>> -    FreePool (MapInfo);
>> -    return EFI_SUCCESS;
>> +    CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
>> +                           (UINTN)MapInfo->CryptedAddress - EFI_PAGE_SIZE
>> +                           );
>
> One question, per spec, is it legal for client to call Map() at some
> offset within allocated buffer ?
>
> e.g something like this:
>
> * AllocateBuffer (, 1, &Buffer);
> * MapBuffer = Buffer + 10;
> * Map (, BusMasterCommonBuffer, MappedBuffer, 10, ..) // Bascially Map
> 10 bytes from offset 10

The input/output parameter names seem to counter-indicate such use.
Namely, AllocateBuffer() outputs a "HostAddress" param, and Map() takes
a "HostAddress" param. Plus we have sentences like this:

Under PciIo.Map():

> ... only memory allocated via the AllocateBuffer() interface can be
> mapped for this type of operation ...

Under PciIo.AllocateBuffer():

> The AllocateBuffer() function allocates pages that are suitable for an
> EfiPciOperationBusMasterCommonBuffer or
> EfiPciOperationBusMasterCommonBuffer64 mapping. This means that the
> buffer allocated by this function must support simultaneous access by
> both the processor and a PCI Bus Master. The device address that the
> PCI Bus Master uses to access *the* buffer can be retrieved with a
> call to Map().

This second passage says *the* buffer. (Emphasis mine above.)

> If this is legal then we may need to build MapInfo during
> AllocateBuffer() to locate the "StashBuffer".

Right, in that case we'd have to build a list of allocated ranges (an
interval tree of sorts) in AllocateBuffer, and convert any
CommonBuffer[64] Map() call to its containing allocation with a search.

It would be worse than that, actually... The pattern you have raised
could be taken one step further: do one AllocateBuffer(), and several
CommonBuffer[64] Map()s into it :) What should happen if those maps are
distinct? What should happen if they overlap? :) I can't even imagine
what this would mean for SEV.

... There are guide-like sections in the generic description of
EFI_PCI_IO_PROTOCOL; Andrew quoted them earlier:

  http://mid.mail-archive.com/A29CDE8F-C82A-4C92-ABF8-008A9BF8F230@apple.com

> DMA Bus Master Common Buffer Operation
> ======================================
> * Call AllocateBuffer() to allocate a common buffer.
> * Call Map() for EfiPciIoOperationBusMasterCommonBuffer.
> * Program the DMA Bus Master with the DeviceAddress returned by Map().
> * The common buffer can now be accessed equally by the processor and
>   the DMA bus master.
> * Call Unmap().
> * Call FreeBuffer().

Look at page 854 (printed page number: 784) in UEFI 2.7.

Thus, I don't think the usage you raise is permitted.

Thanks!
Laszlo

> So far, I have not came across this usecase but I wanted to check with
> you so that we don't fail on corner cases. Good part if you have
> ASSERT() so we should be able to catch them (if any).
>
>> +    ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG);
>> +    CopyMem (
>> +      CommonBufferHeader->StashBuffer,
>> +      (VOID *)(UINTN)MapInfo->CryptedAddress,
>> +      MapInfo->NumberOfBytes
>> +      );
>> +    //
>> +    // Point "DecryptionSource" to the stash buffer so that we decrypt
>> +    // it to the original location, after the switch statement.
>> +    //
>> +    DecryptionSource = CommonBufferHeader->StashBuffer;
>> +    break;
>>
> [Snip]
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 8/2/17 7:13 PM, Laszlo Ersek wrote:
> (CC Andrew)
>
> On 08/03/17 01:01, Brijesh Singh wrote:
>>
>> On 8/2/17 4:24 PM, Laszlo Ersek wrote:
>>
>> [Snip]
>>> At the moment, we have the foll+    // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer().
>>>      //
>>>      MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
>>> -
>>>      //
>>> -    // Therefore no mapping is necessary.
>>> +    // Stash the crypted data.
>>>      //
>>> -    *DeviceAddress = MapInfo->PlainTextAddress;
>>> -    *Mapping       = NO_MAPPING;
>>> -    FreePool (MapInfo);
>>> -    return EFI_SUCCESS;
>>> +    CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
>>> +                           (UINTN)MapInfo->CryptedAddress - EFI_PAGE_SIZE
>>> +                           );
>> One question, per spec, is it legal for client to call Map() at some
>> offset within allocated buffer ?
>>
>> e.g something like this:
>>
>> * AllocateBuffer (, 1, &Buffer);
>> * MapBuffer = Buffer + 10;
>> * Map (, BusMasterCommonBuffer, MappedBuffer, 10, ..) // Bascially Map
>> 10 bytes from offset 10
> The input/output parameter names seem to counter-indicate such use.
> Namely, AllocateBuffer() outputs a "HostAddress" param, and Map() takes
> a "HostAddress" param. Plus we have sentences like this:
>
> Under PciIo.Map():
>
>> ... only memory allocated via the AllocateBuffer() interface can be
>> mapped for this type of operation ...
> Under PciIo.AllocateBuffer():
>
>> The AllocateBuffer() function allocates pages that are suitable for an
>> EfiPciOperationBusMasterCommonBuffer or
>> EfiPciOperationBusMasterCommonBuffer64 mapping. This means that the
>> buffer allocated by this function must support simultaneous access by
>> both the processor and a PCI Bus Master. The device address that the
>> PCI Bus Master uses to access *the* buffer can be retrieved with a
>> call to Map().
> This second passage says *the* buffer. (Emphasis mine above.)
>
>> If this is legal then we may need to build MapInfo during
>> AllocateBuffer() to locate the "StashBuffer".
> Right, in that case we'd have to build a list of allocated ranges (an
> interval tree of sorts) in AllocateBuffer, and convert any
> CommonBuffer[64] Map() call to its containing allocation with a search.
>
> It would be worse than that, actually... The pattern you have raised
> could be taken one step further: do one AllocateBuffer(), and several
> CommonBuffer[64] Map()s into it :) What should happen if those maps are
> distinct? What should happen if they overlap? :) I can't even imagine
> what this would mean for SEV.
>
> ... There are guide-like sections in the generic description of
> EFI_PCI_IO_PROTOCOL; Andrew quoted them earlier:
>
>   http://mid.mail-archive.com/A29CDE8F-C82A-4C92-ABF8-008A9BF8F230@apple.com
>
>> DMA Bus Master Common Buffer Operation
>> ======================================
>> * Call AllocateBuffer() to allocate a common buffer.
>> * Call Map() for EfiPciIoOperationBusMasterCommonBuffer.
>> * Program the DMA Bus Master with the DeviceAddress returned by Map().
>> * The common buffer can now be accessed equally by the processor and
>>   the DMA bus master.
>> * Call Unmap().
>> * Call FreeBuffer().
> Look at page 854 (printed page number: 784) in UEFI 2.7.
>
> Thus, I don't think the usage you raise is permitted.

Sounds good. I did a quick test on SEV hardware, everything seems to be
working well. I have started my stresstest and report the result tomorrow.

-Brijesh
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Laszlo,

One minor issue, I got compilation error with GCC48.

/home/brijesh/codomania/edk2-new/edk2/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c: In function ‘IoMmuUnmap’:
/home/brijesh/codomania/edk2-new/edk2/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c:408:25: error: ‘CommonBufferHeader’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
        CommonBufferHeader->StashBuffer,

Looks like we need to initialize CommonBufferHeader = NULL to keep GCC48 happy.

thanks

On 08/02/2017 08:09 PM, Brijesh Singh wrote:
> 
> 
> On 8/2/17 7:13 PM, Laszlo Ersek wrote:
>> (CC Andrew)
>>
>> On 08/03/17 01:01, Brijesh Singh wrote:
>>>
>>> On 8/2/17 4:24 PM, Laszlo Ersek wrote:
>>>
>>> [Snip]
>>>> At the moment, we have the foll+    // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer().
>>>>       //
>>>>       MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
>>>> -
>>>>       //
>>>> -    // Therefore no mapping is necessary.
>>>> +    // Stash the crypted data.
>>>>       //
>>>> -    *DeviceAddress = MapInfo->PlainTextAddress;
>>>> -    *Mapping       = NO_MAPPING;
>>>> -    FreePool (MapInfo);
>>>> -    return EFI_SUCCESS;
>>>> +    CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
>>>> +                           (UINTN)MapInfo->CryptedAddress - EFI_PAGE_SIZE
>>>> +                           );
>>> One question, per spec, is it legal for client to call Map() at some
>>> offset within allocated buffer ?
>>>
>>> e.g something like this:
>>>
>>> * AllocateBuffer (, 1, &Buffer);
>>> * MapBuffer = Buffer + 10;
>>> * Map (, BusMasterCommonBuffer, MappedBuffer, 10, ..) // Bascially Map
>>> 10 bytes from offset 10
>> The input/output parameter names seem to counter-indicate such use.
>> Namely, AllocateBuffer() outputs a "HostAddress" param, and Map() takes
>> a "HostAddress" param. Plus we have sentences like this:
>>
>> Under PciIo.Map():
>>
>>> ... only memory allocated via the AllocateBuffer() interface can be
>>> mapped for this type of operation ...
>> Under PciIo.AllocateBuffer():
>>
>>> The AllocateBuffer() function allocates pages that are suitable for an
>>> EfiPciOperationBusMasterCommonBuffer or
>>> EfiPciOperationBusMasterCommonBuffer64 mapping. This means that the
>>> buffer allocated by this function must support simultaneous access by
>>> both the processor and a PCI Bus Master. The device address that the
>>> PCI Bus Master uses to access *the* buffer can be retrieved with a
>>> call to Map().
>> This second passage says *the* buffer. (Emphasis mine above.)
>>
>>> If this is legal then we may need to build MapInfo during
>>> AllocateBuffer() to locate the "StashBuffer".
>> Right, in that case we'd have to build a list of allocated ranges (an
>> interval tree of sorts) in AllocateBuffer, and convert any
>> CommonBuffer[64] Map() call to its containing allocation with a search.
>>
>> It would be worse than that, actually... The pattern you have raised
>> could be taken one step further: do one AllocateBuffer(), and several
>> CommonBuffer[64] Map()s into it :) What should happen if those maps are
>> distinct? What should happen if they overlap? :) I can't even imagine
>> what this would mean for SEV.
>>
>> ... There are guide-like sections in the generic description of
>> EFI_PCI_IO_PROTOCOL; Andrew quoted them earlier:
>>
>>    http://mid.mail-archive.com/A29CDE8F-C82A-4C92-ABF8-008A9BF8F230@apple.com
>>
>>> DMA Bus Master Common Buffer Operation
>>> ======================================
>>> * Call AllocateBuffer() to allocate a common buffer.
>>> * Call Map() for EfiPciIoOperationBusMasterCommonBuffer.
>>> * Program the DMA Bus Master with the DeviceAddress returned by Map().
>>> * The common buffer can now be accessed equally by the processor and
>>>    the DMA bus master.
>>> * Call Unmap().
>>> * Call FreeBuffer().
>> Look at page 854 (printed page number: 784) in UEFI 2.7.
>>
>> Thus, I don't think the usage you raise is permitted.
> 
> Sounds good. I did a quick test on SEV hardware, everything seems to be
> working well. I have started my stresstest and report the result tomorrow.
> 
> -Brijesh
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 08/03/17 16:35, Brijesh Singh wrote:
> Laszlo,
> 
> One minor issue, I got compilation error with GCC48.
> 
> /home/brijesh/codomania/edk2-new/edk2/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c: In
> function ‘IoMmuUnmap’:
> /home/brijesh/codomania/edk2-new/edk2/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c:408:25:
> error: ‘CommonBufferHeader’ may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
>        CommonBufferHeader->StashBuffer,
> 
> Looks like we need to initialize CommonBufferHeader = NULL to keep GCC48
> happy.

Interesting, I use GCC48 all the time (as part of RHEL7 on my laptop),
and I didn't get this warning. I'll suppress it.

Thank you for the report!
Laszlo

> 
> thanks
> 
> On 08/02/2017 08:09 PM, Brijesh Singh wrote:
>>
>>
>> On 8/2/17 7:13 PM, Laszlo Ersek wrote:
>>> (CC Andrew)
>>>
>>> On 08/03/17 01:01, Brijesh Singh wrote:
>>>>
>>>> On 8/2/17 4:24 PM, Laszlo Ersek wrote:
>>>>
>>>> [Snip]
>>>>> At the moment, we have the foll+    // The buffer at
>>>>> MapInfo->CryptedAddress comes from AllocateBuffer().
>>>>>       //
>>>>>       MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
>>>>> -
>>>>>       //
>>>>> -    // Therefore no mapping is necessary.
>>>>> +    // Stash the crypted data.
>>>>>       //
>>>>> -    *DeviceAddress = MapInfo->PlainTextAddress;
>>>>> -    *Mapping       = NO_MAPPING;
>>>>> -    FreePool (MapInfo);
>>>>> -    return EFI_SUCCESS;
>>>>> +    CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
>>>>> +                           (UINTN)MapInfo->CryptedAddress -
>>>>> EFI_PAGE_SIZE
>>>>> +                           );
>>>> One question, per spec, is it legal for client to call Map() at some
>>>> offset within allocated buffer ?
>>>>
>>>> e.g something like this:
>>>>
>>>> * AllocateBuffer (, 1, &Buffer);
>>>> * MapBuffer = Buffer + 10;
>>>> * Map (, BusMasterCommonBuffer, MappedBuffer, 10, ..) // Bascially Map
>>>> 10 bytes from offset 10
>>> The input/output parameter names seem to counter-indicate such use.
>>> Namely, AllocateBuffer() outputs a "HostAddress" param, and Map() takes
>>> a "HostAddress" param. Plus we have sentences like this:
>>>
>>> Under PciIo.Map():
>>>
>>>> ... only memory allocated via the AllocateBuffer() interface can be
>>>> mapped for this type of operation ...
>>> Under PciIo.AllocateBuffer():
>>>
>>>> The AllocateBuffer() function allocates pages that are suitable for an
>>>> EfiPciOperationBusMasterCommonBuffer or
>>>> EfiPciOperationBusMasterCommonBuffer64 mapping. This means that the
>>>> buffer allocated by this function must support simultaneous access by
>>>> both the processor and a PCI Bus Master. The device address that the
>>>> PCI Bus Master uses to access *the* buffer can be retrieved with a
>>>> call to Map().
>>> This second passage says *the* buffer. (Emphasis mine above.)
>>>
>>>> If this is legal then we may need to build MapInfo during
>>>> AllocateBuffer() to locate the "StashBuffer".
>>> Right, in that case we'd have to build a list of allocated ranges (an
>>> interval tree of sorts) in AllocateBuffer, and convert any
>>> CommonBuffer[64] Map() call to its containing allocation with a search.
>>>
>>> It would be worse than that, actually... The pattern you have raised
>>> could be taken one step further: do one AllocateBuffer(), and several
>>> CommonBuffer[64] Map()s into it :) What should happen if those maps are
>>> distinct? What should happen if they overlap? :) I can't even imagine
>>> what this would mean for SEV.
>>>
>>> ... There are guide-like sections in the generic description of
>>> EFI_PCI_IO_PROTOCOL; Andrew quoted them earlier:
>>>
>>>   
>>> http://mid.mail-archive.com/A29CDE8F-C82A-4C92-ABF8-008A9BF8F230@apple.com
>>>
>>>
>>>> DMA Bus Master Common Buffer Operation
>>>> ======================================
>>>> * Call AllocateBuffer() to allocate a common buffer.
>>>> * Call Map() for EfiPciIoOperationBusMasterCommonBuffer.
>>>> * Program the DMA Bus Master with the DeviceAddress returned by Map().
>>>> * The common buffer can now be accessed equally by the processor and
>>>>    the DMA bus master.
>>>> * Call Unmap().
>>>> * Call FreeBuffer().
>>> Look at page 854 (printed page number: 784) in UEFI 2.7.
>>>
>>> Thus, I don't think the usage you raise is permitted.
>>
>> Sounds good. I did a quick test on SEV hardware, everything seems to be
>> working well. I have started my stresstest and report the result
>> tomorrow.
>>
>> -Brijesh
>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel