[PATCH v1 0/4] OvmfPkg : IoMmuDxe: BusMasterCommonBuffer support when SEV is active

[PATCH v1 0/4] OvmfPkg : IoMmuDxe: BusMasterCommonBuffer support when SEV is active

[Brijesh Singh]

The patch series implements multiple recommendation came during IOMMU 
support discussion [1] for the SEV guest. Non of these patches fixes
Virtio support for SEV guest, instead it fixes the SEV IoMmuDxe support
to comply with EFI PCI protocol spec on DMA bus master accesses.

I did some performace measurement and do not see notiable performace impact
with unoptimized in-place encrypt/decrypt.

[1] https://lists.01.org/pipermail/edk2-devel/2017-July/012448.html

Brijesh Singh (4):
  OvmfPkg: IommuDxe: Do not clear C-bit in Allocate() and Free()
  OvmfPkg: IommuDxe: Provide support for mapping BusMasterCommonBuffer
    operation
  OvmfPkg: IommuDxe: Zero the shared page(s) on Unmap()
  OvmfPkg : QemuFwCfgLib: Map DMA buffer with CommonBuffer when SEV is
    enable

 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h |  42 ++--
 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c                      | 206 ++++++++++++----
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c         | 247 ++++++++++++++++----
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c         | 131 -----------
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c         | 101 +++++---
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c         |  56 ++---
 6 files changed, 458 insertions(+), 325 deletions(-)

-- 
Brijesh Singh
2.7.4

[PATCH v1 1/4] OvmfPkg: IommuDxe: Do not clear C-bit in Allocate() and Free()

[Brijesh Singh]

As per the discussion [1], the buffer allocated using IOMMU->AllocateBuffer()
need not result in a buffer that is immediately usable for the
DMA device operation. Client is required to call Map() unconditionally
with BusMasterCommonBuffer before performing the desired DMA bus
master operation.

[1]https://lists.01.org/pipermail/edk2-devel/2017-July/012652.html

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

diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 9e78058b7242..cc3c979d4484 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -333,12 +333,6 @@ IoMmuAllocateBuffer (
                   );
   if (!EFI_ERROR (Status)) {
     *HostAddress = (VOID *) (UINTN) PhysicalAddress;
-
-    //
-    // Clear memory encryption mask
-    //
-    Status = MemEncryptSevClearPageEncMask (0, PhysicalAddress, Pages, TRUE);
-    ASSERT_EFI_ERROR(Status);
   }
 
   DEBUG ((DEBUG_VERBOSE, "%a Address 0x%Lx Pages 0x%Lx\n", __FUNCTION__, PhysicalAddress, Pages));
@@ -365,14 +359,6 @@ IoMmuFreeBuffer (
   IN  VOID                                     *HostAddress
   )
 {
-  EFI_STATUS  Status;
-
-  //
-  // Set memory encryption mask
-  //
-  Status = MemEncryptSevSetPageEncMask (0, (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress, Pages, TRUE);
-  ASSERT_EFI_ERROR(Status);
-
   DEBUG ((DEBUG_VERBOSE, "%a Address 0x%Lx Pages 0x%Lx\n", __FUNCTION__, (UINTN)HostAddress, Pages));
   return gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress, Pages);
 }
-- 
2.7.4

Re: [PATCH v1 1/4] OvmfPkg: IommuDxe: Do not clear C-bit in Allocate() and Free()

[Laszlo Ersek]

On 07/31/17 21:31, Brijesh Singh wrote:
> As per the discussion [1], the buffer allocated using IOMMU->AllocateBuffer()
> need not result in a buffer that is immediately usable for the
> DMA device operation. Client is required to call Map() unconditionally
> with BusMasterCommonBuffer before performing the desired DMA bus
> master operation.
> 
> [1]https://lists.01.org/pipermail/edk2-devel/2017-July/012652.html
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> index 9e78058b7242..cc3c979d4484 100644
> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> @@ -333,12 +333,6 @@ IoMmuAllocateBuffer (
>                    );
>    if (!EFI_ERROR (Status)) {
>      *HostAddress = (VOID *) (UINTN) PhysicalAddress;
> -
> -    //
> -    // Clear memory encryption mask
> -    //
> -    Status = MemEncryptSevClearPageEncMask (0, PhysicalAddress, Pages, TRUE);
> -    ASSERT_EFI_ERROR(Status);
>    }
>  
>    DEBUG ((DEBUG_VERBOSE, "%a Address 0x%Lx Pages 0x%Lx\n", __FUNCTION__, PhysicalAddress, Pages));
> @@ -365,14 +359,6 @@ IoMmuFreeBuffer (
>    IN  VOID                                     *HostAddress
>    )
>  {
> -  EFI_STATUS  Status;
> -
> -  //
> -  // Set memory encryption mask
> -  //
> -  Status = MemEncryptSevSetPageEncMask (0, (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress, Pages, TRUE);
> -  ASSERT_EFI_ERROR(Status);
> -
>    DEBUG ((DEBUG_VERBOSE, "%a Address 0x%Lx Pages 0x%Lx\n", __FUNCTION__, (UINTN)HostAddress, Pages));
>    return gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress, Pages);
>  }
> 

This looks good to me, but "IommuDxe" in the subject should be spelled
"IoMmuDxe". With that change,

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

Thanks
Laszlo

[PATCH v1 2/4] OvmfPkg: IommuDxe: Provide support for mapping BusMasterCommonBuffer operation

[Brijesh Singh]

The current implementation was making assumption that AllocateBuffer()
returns a buffer with C-bit cleared. Hence when we were asked to
Map() with BusMasterCommonBuffer, we do not change the C-bit on
host buffer.

In previous patch, we changed the AllocateBuffer() to not clear
C-bit during allocation. The patch adds support for handling the
BusMasterCommonBuffer operations when SEV is active.

A typical DMA Bus master Common Operation follows the below step:

1. Client calls AllocateBuffer() to allocate a common buffer
2. Client fill some data in common buffer (optional)
3. Client calls Map() with BusMasterCommonBuffer
4. Programs the DMA bus master with the device address returned by Map()
5. The common buffer can now be accessed equally by the processor and
   the DMA bus master.
6. Client calls Unmap()
7. Client calls FreeBuffer()

In order to handle steps #2 (in which common buffer may contain
data), we perform in-place encryption to ensure that device
address returned by the Map() contains the correct data after
we clear the C-bit during Map().

In my measurement I do not see any noticable perform degradation when
performing in-place encryption/decryption on common buffer.

Suggested-by: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 190 +++++++++++++++++---
 1 file changed, 164 insertions(+), 26 deletions(-)

diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index cc3c979d4484..5ae54482fffe 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -28,7 +28,127 @@ typedef struct {
   EFI_PHYSICAL_ADDRESS                      DeviceAddress;
 } MAP_INFO;
 
-#define NO_MAPPING             (VOID *) (UINTN) -1
+/**
+
+  The function is used for mapping and unmapping the Host buffer with
+  BusMasterCommonBuffer. Since the buffer can be accessed equally by the
+  processor and the DMA bus master hence we can not use the bounce buffer.
+
+  The function changes the underlying encryption mask of the pages that maps the
+  host buffer. It also ensures that buffer contents are updated with the desired
+  state.
+
+**/
+STATIC
+EFI_STATUS
+SetBufferAsEncDec (
+  IN MAP_INFO *MapInfo,
+  IN BOOLEAN  Enc
+  )
+{
+  EFI_STATUS            Status;
+  EFI_PHYSICAL_ADDRESS  TempBuffer;
+
+  //
+  // Allocate an intermediate buffer to hold the host buffer contents
+  //
+  Status = gBS->AllocatePages (
+                  AllocateAnyPages,
+                  EfiBootServicesData,
+                  MapInfo->NumberOfPages,
+                  &TempBuffer
+                  );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  //
+  // If the host buffer has C-bit cleared, then make sure the intermediate
+  // buffer matches with same encryption mask.
+  //
+  if (!Enc) {
+    Status = MemEncryptSevClearPageEncMask (0, MapInfo->DeviceAddress,
+               MapInfo->NumberOfPages, TRUE);
+    ASSERT_EFI_ERROR (Status);
+  }
+
+  //
+  // Copy the data from host buffer into a temporary buffer. At this
+  // time both host and intermediate buffer will have same encryption
+  // mask.
+  //
+  CopyMem (
+    (VOID *) (UINTN) TempBuffer,
+    (VOID *) (UINTN)MapInfo->HostAddress,
+    MapInfo->NumberOfBytes);
+
+  //
+  // Now change the encryption mask of the host buffer
+  //
+  if (Enc) {
+    Status = MemEncryptSevSetPageEncMask (0, MapInfo->HostAddress,
+               MapInfo->NumberOfPages, TRUE);
+    ASSERT_EFI_ERROR (Status);
+  } else {
+    Status = MemEncryptSevClearPageEncMask (0, MapInfo->HostAddress,
+               MapInfo->NumberOfPages, TRUE);
+    ASSERT_EFI_ERROR (Status);
+  }
+
+  //
+  // Copy the data from intermediate buffer into host buffer. At this
+  // time encryption masks will be different on host and intermediate
+  // buffer and the hardware will perform encryption/decryption on
+  // accesses.
+  //
+  CopyMem (
+    (VOID *) (UINTN)MapInfo->HostAddress,
+    (VOID *) (UINTN)TempBuffer,
+    MapInfo->NumberOfBytes);
+
+  //
+  // Restore the encryption mask of the intermediate buffer
+  //
+  if (!Enc) {
+    Status = MemEncryptSevSetPageEncMask (0, MapInfo->DeviceAddress,
+               MapInfo->NumberOfPages, TRUE);
+    ASSERT_EFI_ERROR (Status);
+  }
+
+  //
+  // Free the intermediate buffer
+  //
+  gBS->FreePages (TempBuffer, MapInfo->NumberOfPages);
+  return EFI_SUCCESS;
+}
+
+/**
+  This function will be called by Map() when mapping the buffer buffer to
+  BusMasterCommonBuffer type.
+
+**/
+STATIC
+EFI_STATUS
+SetHostBufferAsEncrypted (
+  IN MAP_INFO *MapInfo
+  )
+{
+  return SetBufferAsEncDec (MapInfo, TRUE);
+}
+
+/**
+  This function will be called by Unmap() when unmapping host buffer
+  from the BusMasterCommonBuffer type.
+
+**/
+STATIC
+EFI_STATUS
+SetHostBufferAsDecrypted (
+  IN MAP_INFO *MapInfo
+  )
+{
+  return SetBufferAsEncDec (MapInfo, FALSE);
+}
 
 /**
   Provides the controller-specific addresses required to access system memory from a
@@ -113,18 +233,6 @@ IoMmuMap (
   }
 
   //
-  // CommandBuffer was allocated by us (AllocateBuffer) and is already in
-  // unencryted buffer so no need to create bounce buffer
-  //
-  if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
-      Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
-    *Mapping = NO_MAPPING;
-    *DeviceAddress = PhysicalAddress;
-
-    return EFI_SUCCESS;
-  }
-
-  //
   // Allocate a MAP_INFO structure to remember the mapping when Unmap() is
   // called later.
   //
@@ -144,6 +252,25 @@ IoMmuMap (
   MapInfo->DeviceAddress     = DmaMemoryTop;
 
   //
+  // If the requested Map() operation is BusMasterCommandBuffer then map
+  // using internal function otherwise allocate a bounce buffer to map
+  // the host buffer to device buffer
+  //
+  if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
+      Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
+
+    Status = SetHostBufferAsDecrypted (MapInfo);
+    if (EFI_ERROR (Status)) {
+      FreePool (MapInfo);
+      *NumberOfBytes = 0;
+      return Status;
+    }
+
+    MapInfo->DeviceAddress = MapInfo->HostAddress;
+    goto Done;
+  }
+
+  //
   // Allocate a buffer to map the transfer to.
   //
   Status = gBS->AllocatePages (
@@ -178,6 +305,7 @@ IoMmuMap (
       );
   }
 
+Done:
   //
   // The DeviceAddress is the address of the maped buffer below 4GB
   //
@@ -219,18 +347,25 @@ IoMmuUnmap (
     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 CommonBuffer operation from the Bus Master's point of
+  // view then Map() have cleared the memory encryption mask from Host
+  // buffer. Lets restore the memory encryption mask before returning
+  //
+  if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
+      MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
+
+    Status = SetHostBufferAsEncrypted (MapInfo);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+
+    goto Done;
+  }
+
+  //
   // 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.
@@ -244,9 +379,6 @@ IoMmuUnmap (
       );
   }
 
-  DEBUG ((DEBUG_VERBOSE, "%a Device 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
-        __FUNCTION__, MapInfo->DeviceAddress, MapInfo->HostAddress,
-        MapInfo->NumberOfPages, MapInfo->NumberOfBytes));
   //
   // Restore the memory encryption mask
   //
@@ -254,9 +386,15 @@ IoMmuUnmap (
   ASSERT_EFI_ERROR(Status);
 
   //
-  // Free the mapped buffer and the MAP_INFO structure.
+  // Free the bounce buffer
   //
   gBS->FreePages (MapInfo->DeviceAddress, MapInfo->NumberOfPages);
+
+Done:
+  DEBUG ((DEBUG_VERBOSE, "%a Device 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
+        __FUNCTION__, MapInfo->DeviceAddress, MapInfo->HostAddress,
+        MapInfo->NumberOfPages, MapInfo->NumberOfBytes));
+
   FreePool (Mapping);
   return EFI_SUCCESS;
 }
-- 
2.7.4

Re: [PATCH v1 2/4] OvmfPkg: IommuDxe: Provide support for mapping BusMasterCommonBuffer operation

[Ard Biesheuvel]

On 31 July 2017 at 20:31, Brijesh Singh <brijesh.singh@amd.com> wrote:
> The current implementation was making assumption that AllocateBuffer()
> returns a buffer with C-bit cleared. Hence when we were asked to
> Map() with BusMasterCommonBuffer, we do not change the C-bit on
> host buffer.
>
> In previous patch, we changed the AllocateBuffer() to not clear
> C-bit during allocation. The patch adds support for handling the
> BusMasterCommonBuffer operations when SEV is active.
>
> A typical DMA Bus master Common Operation follows the below step:
>
> 1. Client calls AllocateBuffer() to allocate a common buffer
> 2. Client fill some data in common buffer (optional)
> 3. Client calls Map() with BusMasterCommonBuffer
> 4. Programs the DMA bus master with the device address returned by Map()
> 5. The common buffer can now be accessed equally by the processor and
>    the DMA bus master.
> 6. Client calls Unmap()
> 7. Client calls FreeBuffer()
>
> In order to handle steps #2 (in which common buffer may contain
> data), we perform in-place encryption to ensure that device
> address returned by the Map() contains the correct data after
> we clear the C-bit during Map().
>
> In my measurement I do not see any noticable perform degradation when
> performing in-place encryption/decryption on common buffer.
>
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 190 +++++++++++++++++---
>  1 file changed, 164 insertions(+), 26 deletions(-)
>

Hello Brijesh,

I haven't looked in detail at the existing code, but please don't
conflate the device address with the address of a bounce buffer. These
are very different things, although the confusion is understandable
(and precedented) when not used to dealing with non-1:1 DMA.

The device address is what gets programmed into the device's DMA
registers. If there is a fixed [non-zero] offset between the device's
view of memory and the host's (as may be the case with PCI, or
generally when using an IOMMU), then the device is the only one who
should attempt to perform memory accesses using this address. So
please void SetMem() or other CPU dereferences involving the device
address, and treat it as an opaque handle instead.

In your case, you are dealing with a bounce buffer. So call it bounce
buffer in the MapInfo struct. Imagine when dealing with a non-linear
host to PCI mapping, you will still need to perform an additional
translation to derive the device address from the bounce buffer
address.


> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> index cc3c979d4484..5ae54482fffe 100644
> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> @@ -28,7 +28,127 @@ typedef struct {
>    EFI_PHYSICAL_ADDRESS                      DeviceAddress;
>  } MAP_INFO;
>
> -#define NO_MAPPING             (VOID *) (UINTN) -1
> +/**
> +
> +  The function is used for mapping and unmapping the Host buffer with
> +  BusMasterCommonBuffer. Since the buffer can be accessed equally by the
> +  processor and the DMA bus master hence we can not use the bounce buffer.
> +
> +  The function changes the underlying encryption mask of the pages that maps the
> +  host buffer. It also ensures that buffer contents are updated with the desired
> +  state.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +SetBufferAsEncDec (
> +  IN MAP_INFO *MapInfo,
> +  IN BOOLEAN  Enc
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EFI_PHYSICAL_ADDRESS  TempBuffer;
> +
> +  //
> +  // Allocate an intermediate buffer to hold the host buffer contents
> +  //
> +  Status = gBS->AllocatePages (
> +                  AllocateAnyPages,
> +                  EfiBootServicesData,
> +                  MapInfo->NumberOfPages,
> +                  &TempBuffer
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  //
> +  // If the host buffer has C-bit cleared, then make sure the intermediate
> +  // buffer matches with same encryption mask.
> +  //
> +  if (!Enc) {
> +    Status = MemEncryptSevClearPageEncMask (0, MapInfo->DeviceAddress,
> +               MapInfo->NumberOfPages, TRUE);
> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
> +  //
> +  // Copy the data from host buffer into a temporary buffer. At this
> +  // time both host and intermediate buffer will have same encryption
> +  // mask.
> +  //
> +  CopyMem (
> +    (VOID *) (UINTN) TempBuffer,
> +    (VOID *) (UINTN)MapInfo->HostAddress,
> +    MapInfo->NumberOfBytes);
> +
> +  //
> +  // Now change the encryption mask of the host buffer
> +  //
> +  if (Enc) {
> +    Status = MemEncryptSevSetPageEncMask (0, MapInfo->HostAddress,
> +               MapInfo->NumberOfPages, TRUE);
> +    ASSERT_EFI_ERROR (Status);
> +  } else {
> +    Status = MemEncryptSevClearPageEncMask (0, MapInfo->HostAddress,
> +               MapInfo->NumberOfPages, TRUE);
> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
> +  //
> +  // Copy the data from intermediate buffer into host buffer. At this
> +  // time encryption masks will be different on host and intermediate
> +  // buffer and the hardware will perform encryption/decryption on
> +  // accesses.
> +  //
> +  CopyMem (
> +    (VOID *) (UINTN)MapInfo->HostAddress,
> +    (VOID *) (UINTN)TempBuffer,
> +    MapInfo->NumberOfBytes);
> +
> +  //
> +  // Restore the encryption mask of the intermediate buffer
> +  //
> +  if (!Enc) {
> +    Status = MemEncryptSevSetPageEncMask (0, MapInfo->DeviceAddress,
> +               MapInfo->NumberOfPages, TRUE);
> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
> +  //
> +  // Free the intermediate buffer
> +  //
> +  gBS->FreePages (TempBuffer, MapInfo->NumberOfPages);
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  This function will be called by Map() when mapping the buffer buffer to
> +  BusMasterCommonBuffer type.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +SetHostBufferAsEncrypted (
> +  IN MAP_INFO *MapInfo
> +  )
> +{
> +  return SetBufferAsEncDec (MapInfo, TRUE);
> +}
> +
> +/**
> +  This function will be called by Unmap() when unmapping host buffer
> +  from the BusMasterCommonBuffer type.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +SetHostBufferAsDecrypted (
> +  IN MAP_INFO *MapInfo
> +  )
> +{
> +  return SetBufferAsEncDec (MapInfo, FALSE);
> +}
>
>  /**
>    Provides the controller-specific addresses required to access system memory from a
> @@ -113,18 +233,6 @@ IoMmuMap (
>    }
>
>    //
> -  // CommandBuffer was allocated by us (AllocateBuffer) and is already in
> -  // unencryted buffer so no need to create bounce buffer
> -  //
> -  if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
> -      Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
> -    *Mapping = NO_MAPPING;
> -    *DeviceAddress = PhysicalAddress;
> -
> -    return EFI_SUCCESS;
> -  }
> -
> -  //
>    // Allocate a MAP_INFO structure to remember the mapping when Unmap() is
>    // called later.
>    //
> @@ -144,6 +252,25 @@ IoMmuMap (
>    MapInfo->DeviceAddress     = DmaMemoryTop;
>
>    //
> +  // If the requested Map() operation is BusMasterCommandBuffer then map
> +  // using internal function otherwise allocate a bounce buffer to map
> +  // the host buffer to device buffer
> +  //
> +  if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
> +      Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
> +
> +    Status = SetHostBufferAsDecrypted (MapInfo);
> +    if (EFI_ERROR (Status)) {
> +      FreePool (MapInfo);
> +      *NumberOfBytes = 0;
> +      return Status;
> +    }
> +
> +    MapInfo->DeviceAddress = MapInfo->HostAddress;
> +    goto Done;
> +  }
> +
> +  //
>    // Allocate a buffer to map the transfer to.
>    //
>    Status = gBS->AllocatePages (
> @@ -178,6 +305,7 @@ IoMmuMap (
>        );
>    }
>
> +Done:
>    //
>    // The DeviceAddress is the address of the maped buffer below 4GB
>    //
> @@ -219,18 +347,25 @@ IoMmuUnmap (
>      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 CommonBuffer operation from the Bus Master's point of
> +  // view then Map() have cleared the memory encryption mask from Host
> +  // buffer. Lets restore the memory encryption mask before returning
> +  //
> +  if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
> +      MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
> +
> +    Status = SetHostBufferAsEncrypted (MapInfo);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +
> +    goto Done;
> +  }
> +
> +  //
>    // 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.
> @@ -244,9 +379,6 @@ IoMmuUnmap (
>        );
>    }
>
> -  DEBUG ((DEBUG_VERBOSE, "%a Device 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
> -        __FUNCTION__, MapInfo->DeviceAddress, MapInfo->HostAddress,
> -        MapInfo->NumberOfPages, MapInfo->NumberOfBytes));
>    //
>    // Restore the memory encryption mask
>    //
> @@ -254,9 +386,15 @@ IoMmuUnmap (
>    ASSERT_EFI_ERROR(Status);
>
>    //
> -  // Free the mapped buffer and the MAP_INFO structure.
> +  // Free the bounce buffer
>    //
>    gBS->FreePages (MapInfo->DeviceAddress, MapInfo->NumberOfPages);
> +
> +Done:
> +  DEBUG ((DEBUG_VERBOSE, "%a Device 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
> +        __FUNCTION__, MapInfo->DeviceAddress, MapInfo->HostAddress,
> +        MapInfo->NumberOfPages, MapInfo->NumberOfBytes));
> +
>    FreePool (Mapping);
>    return EFI_SUCCESS;
>  }
> --
> 2.7.4
>

Re: [PATCH v1 2/4] OvmfPkg: IommuDxe: Provide support for mapping BusMasterCommonBuffer operation

[Brijesh Singh]

On 07/31/2017 02:49 PM, Ard Biesheuvel wrote:
> On 31 July 2017 at 20:31, Brijesh Singh <brijesh.singh@amd.com> wrote:
>> The current implementation was making assumption that AllocateBuffer()
>> returns a buffer with C-bit cleared. Hence when we were asked to
>> Map() with BusMasterCommonBuffer, we do not change the C-bit on
>> host buffer.
>>
>> In previous patch, we changed the AllocateBuffer() to not clear
>> C-bit during allocation. The patch adds support for handling the
>> BusMasterCommonBuffer operations when SEV is active.
>>
>> A typical DMA Bus master Common Operation follows the below step:
>>
>> 1. Client calls AllocateBuffer() to allocate a common buffer
>> 2. Client fill some data in common buffer (optional)
>> 3. Client calls Map() with BusMasterCommonBuffer
>> 4. Programs the DMA bus master with the device address returned by Map()
>> 5. The common buffer can now be accessed equally by the processor and
>>     the DMA bus master.
>> 6. Client calls Unmap()
>> 7. Client calls FreeBuffer()
>>
>> In order to handle steps #2 (in which common buffer may contain
>> data), we perform in-place encryption to ensure that device
>> address returned by the Map() contains the correct data after
>> we clear the C-bit during Map().
>>
>> In my measurement I do not see any noticable perform degradation when
>> performing in-place encryption/decryption on common buffer.
>>
>> Suggested-by: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 190 +++++++++++++++++---
>>   1 file changed, 164 insertions(+), 26 deletions(-)
>>
> 
> Hello Brijesh,
> 
> I haven't looked in detail at the existing code, but please don't
> conflate the device address with the address of a bounce buffer. These
> are very different things, although the confusion is understandable
> (and precedented) when not used to dealing with non-1:1 DMA.
> 
> The device address is what gets programmed into the device's DMA
> registers. If there is a fixed [non-zero] offset between the device's
> view of memory and the host's (as may be the case with PCI, or
> generally when using an IOMMU), then the device is the only one who
> should attempt to perform memory accesses using this address. So
> please void SetMem() or other CPU dereferences involving the device
> address, and treat it as an opaque handle instead.
> 

> In your case, you are dealing with a bounce buffer. So call it bounce
> buffer in the MapInfo struct. Imagine when dealing with a non-linear
> host to PCI mapping, you will still need to perform an additional
> translation to derive the device address from the bounce buffer
> address.
> 

Agreed.

Initially, AmdSevIoMmu.c code was derived from PciRootBridgeIo and MAP_INFO
structure was literally copied. I will probably send a separate patch
to fix the structure member and update the comments to reflect its true
meaning.

Thanks
Brijesh

Re: [PATCH v1 2/4] OvmfPkg: IommuDxe: Provide support for mapping BusMasterCommonBuffer operation

[Laszlo Ersek]

On 07/31/17 22:27, Brijesh Singh wrote:
> 
> 
> On 07/31/2017 02:49 PM, Ard Biesheuvel wrote:
>> On 31 July 2017 at 20:31, Brijesh Singh <brijesh.singh@amd.com> wrote:
>>> The current implementation was making assumption that AllocateBuffer()
>>> returns a buffer with C-bit cleared. Hence when we were asked to
>>> Map() with BusMasterCommonBuffer, we do not change the C-bit on
>>> host buffer.
>>>
>>> In previous patch, we changed the AllocateBuffer() to not clear
>>> C-bit during allocation. The patch adds support for handling the
>>> BusMasterCommonBuffer operations when SEV is active.
>>>
>>> A typical DMA Bus master Common Operation follows the below step:
>>>
>>> 1. Client calls AllocateBuffer() to allocate a common buffer
>>> 2. Client fill some data in common buffer (optional)
>>> 3. Client calls Map() with BusMasterCommonBuffer
>>> 4. Programs the DMA bus master with the device address returned by Map()
>>> 5. The common buffer can now be accessed equally by the processor and
>>>     the DMA bus master.
>>> 6. Client calls Unmap()
>>> 7. Client calls FreeBuffer()
>>>
>>> In order to handle steps #2 (in which common buffer may contain
>>> data), we perform in-place encryption to ensure that device
>>> address returned by the Map() contains the correct data after
>>> we clear the C-bit during Map().
>>>
>>> In my measurement I do not see any noticable perform degradation when
>>> performing in-place encryption/decryption on common buffer.
>>>
>>> Suggested-by: Laszlo Ersek <lersek@redhat.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>> ---
>>>   OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 190 +++++++++++++++++---
>>>   1 file changed, 164 insertions(+), 26 deletions(-)
>>>
>>
>> Hello Brijesh,
>>
>> I haven't looked in detail at the existing code, but please don't
>> conflate the device address with the address of a bounce buffer. These
>> are very different things, although the confusion is understandable
>> (and precedented) when not used to dealing with non-1:1 DMA.
>>
>> The device address is what gets programmed into the device's DMA
>> registers. If there is a fixed [non-zero] offset between the device's
>> view of memory and the host's (as may be the case with PCI, or
>> generally when using an IOMMU), then the device is the only one who
>> should attempt to perform memory accesses using this address. So
>> please void SetMem() or other CPU dereferences involving the device
>> address, and treat it as an opaque handle instead.
>>
> 
>> In your case, you are dealing with a bounce buffer. So call it bounce
>> buffer in the MapInfo struct. Imagine when dealing with a non-linear
>> host to PCI mapping, you will still need to perform an additional
>> translation to derive the device address from the bounce buffer
>> address.
>>
> 
> Agreed.
> 
> Initially, AmdSevIoMmu.c code was derived from PciRootBridgeIo and MAP_INFO
> structure was literally copied. I will probably send a separate patch
> to fix the structure member and update the comments to reflect its true
> meaning.

Yes, we should call it "PlainTextAddress" or something similar.

Thanks
Laszlo

Re: [PATCH v1 2/4] OvmfPkg: IommuDxe: Provide support for mapping BusMasterCommonBuffer operation

[Laszlo Ersek]

On 07/31/17 21:31, Brijesh Singh wrote:
> The current implementation was making assumption that AllocateBuffer()
> returns a buffer with C-bit cleared. Hence when we were asked to
> Map() with BusMasterCommonBuffer, we do not change the C-bit on
> host buffer.
> 
> In previous patch, we changed the AllocateBuffer() to not clear
> C-bit during allocation. The patch adds support for handling the
> BusMasterCommonBuffer operations when SEV is active.
> 
> A typical DMA Bus master Common Operation follows the below step:
> 
> 1. Client calls AllocateBuffer() to allocate a common buffer
> 2. Client fill some data in common buffer (optional)
> 3. Client calls Map() with BusMasterCommonBuffer
> 4. Programs the DMA bus master with the device address returned by Map()
> 5. The common buffer can now be accessed equally by the processor and
>    the DMA bus master.
> 6. Client calls Unmap()
> 7. Client calls FreeBuffer()
> 
> In order to handle steps #2 (in which common buffer may contain
> data), we perform in-place encryption to ensure that device
> address returned by the Map() contains the correct data after
> we clear the C-bit during Map().
> 
> In my measurement I do not see any noticable perform degradation when
> performing in-place encryption/decryption on common buffer.
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 190 +++++++++++++++++---
>  1 file changed, 164 insertions(+), 26 deletions(-)
> 
> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> index cc3c979d4484..5ae54482fffe 100644
> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> @@ -28,7 +28,127 @@ typedef struct {
>    EFI_PHYSICAL_ADDRESS                      DeviceAddress;
>  } MAP_INFO;
>  
> -#define NO_MAPPING             (VOID *) (UINTN) -1
> +/**
> +
> +  The function is used for mapping and unmapping the Host buffer with
> +  BusMasterCommonBuffer. Since the buffer can be accessed equally by the
> +  processor and the DMA bus master hence we can not use the bounce buffer.
> +
> +  The function changes the underlying encryption mask of the pages that maps the
> +  host buffer. It also ensures that buffer contents are updated with the desired
> +  state.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +SetBufferAsEncDec (
> +  IN MAP_INFO *MapInfo,
> +  IN BOOLEAN  Enc
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EFI_PHYSICAL_ADDRESS  TempBuffer;
> +
> +  //
> +  // Allocate an intermediate buffer to hold the host buffer contents
> +  //
> +  Status = gBS->AllocatePages (
> +                  AllocateAnyPages,
> +                  EfiBootServicesData,
> +                  MapInfo->NumberOfPages,
> +                  &TempBuffer
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }

This is not right. This function is called (indirectly) from
IoMmuUnmap(), which is the function that we'll call (also indirectly)
from the ExitBootServices() callbacks. That means we cannot allocate or
release dynamic memory here. This is why I suggested the page-sized
static buffer, and page-wise copying.

More on this later, below the end of this function.

> +
> +  //
> +  // If the host buffer has C-bit cleared, then make sure the intermediate
> +  // buffer matches with same encryption mask.
> +  //
> +  if (!Enc) {
> +    Status = MemEncryptSevClearPageEncMask (0, MapInfo->DeviceAddress,
> +               MapInfo->NumberOfPages, TRUE);
> +    ASSERT_EFI_ERROR (Status);
> +  }

As a separate issue, I don't think this is right. The auxiliary buffer
that we use for in-place encryption or decryption should *always* be
encrypted. (I mentioned this in my email, "Introduce a static UINT8
array with EFI_PAGE_SIZE bytes (this will always remain in encrypted
memory).", but I guess it was easy to miss.)

> +
> +  //
> +  // Copy the data from host buffer into a temporary buffer. At this
> +  // time both host and intermediate buffer will have same encryption
> +  // mask.
> +  //

The comment should say "copy original buffer into temporary buffer".
"host" buffer is very confusing here, as the virtualization host is
exactly what may not yet have access to the buffer. In PCI bus master
terminology, "host buffer" is valid, I think, but saying just "host
buffer" is very confusing.

Also, "same encryption mask" is not desirable (see above), so please
remove that sentence.

> +  CopyMem (
> +    (VOID *) (UINTN) TempBuffer,
> +    (VOID *) (UINTN)MapInfo->HostAddress,
> +    MapInfo->NumberOfBytes);
> +
> +  //
> +  // Now change the encryption mask of the host buffer
> +  //
> +  if (Enc) {
> +    Status = MemEncryptSevSetPageEncMask (0, MapInfo->HostAddress,
> +               MapInfo->NumberOfPages, TRUE);
> +    ASSERT_EFI_ERROR (Status);

What guarantees that this function call will never fail?

If it fails, we should propagate the error (the function prototype
allows for that), and roll back partial changes along the way.

If there is no way to undo partial actions when
MemEncryptSevSetPageEncMask() fails, then the ASSERT() is fine, but we
need an additional CpuDeadLoop() as well.

> +  } else {
> +    Status = MemEncryptSevClearPageEncMask (0, MapInfo->HostAddress,
> +               MapInfo->NumberOfPages, TRUE);
> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
> +  //
> +  // Copy the data from intermediate buffer into host buffer. At this
> +  // time encryption masks will be different on host and intermediate
> +  // buffer and the hardware will perform encryption/decryption on
> +  // accesses.
> +  //
> +  CopyMem (
> +    (VOID *) (UINTN)MapInfo->HostAddress,
> +    (VOID *) (UINTN)TempBuffer,
> +    MapInfo->NumberOfBytes);
> +
> +  //
> +  // Restore the encryption mask of the intermediate buffer
> +  //
> +  if (!Enc) {
> +    Status = MemEncryptSevSetPageEncMask (0, MapInfo->DeviceAddress,
> +               MapInfo->NumberOfPages, TRUE);
> +    ASSERT_EFI_ERROR (Status);
> +  }

Again, the intermediate buffer should always remain encrypted.

> +
> +  //
> +  // Free the intermediate buffer
> +  //
> +  gBS->FreePages (TempBuffer, MapInfo->NumberOfPages);
> +  return EFI_SUCCESS;
> +}

In summary for this function: I've now read section "7.10.8
Encrypt-in-Place" of AMD pub #24593. Earlier I wrote,

    Introduce a static UINT8 array with EFI_PAGE_SIZE bytes (this
    will always remain in encrypted memory). Update the C bit with a
    single function call for the entire range (like now) -- this will
    not affect the guest-readability of the pages --, then bounce each
    page within the range to the static buffer and back to its original
    place.

With regard to 7.10.8., this appears to be wrong. My idea was based on
the expectation that changing the C bit will not affect *guest reads*
from the same area. According to 7.10.8, my assumption was wrong: the
algorithm in 7.10.8 uses a cache-line sized bounce buffer and *two*
separate mappings for the area under in-place encryption. The reading
always occurs through the original mapping.

Now, this requires us to do one of two things:

- Alternative 1: we can use the static, single page-sized buffer, in a
loop. But then we must also break up the central
MemEncryptSevSetPageEncMask() / MemEncryptSevClearPageEncMask() calls to
small, page-sized calls. Copy out a page, change the C bit for that one
page, copy back the page. Lather, rinse, repeat. This would likely be
slow, and would guarantee that 2M pages would be split into 4K pages.

- Alternative 2: we can stick with the large (unified) CopyMem() and
C-bit changing function calls, but we still can't allocate memory for
that in the SetBufferAsEncDec() function. Therefore you will have to
allocate *twice* the requested number of pages in AllocateBuffer(),
return the address of the lower half to the caller, and use the upper
half as intermediate buffer in SetBufferAsEncDec(). This is safe because
CommonBuffer[64] Map/Unmap requires the caller to pass in allocations
from AllocateBuffer().

> +
> +/**
> +  This function will be called by Map() when mapping the buffer buffer to
> +  BusMasterCommonBuffer type.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +SetHostBufferAsEncrypted (
> +  IN MAP_INFO *MapInfo
> +  )
> +{
> +  return SetBufferAsEncDec (MapInfo, TRUE);
> +}
> +
> +/**
> +  This function will be called by Unmap() when unmapping host buffer
> +  from the BusMasterCommonBuffer type.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +SetHostBufferAsDecrypted (
> +  IN MAP_INFO *MapInfo
> +  )
> +{
> +  return SetBufferAsEncDec (MapInfo, FALSE);
> +}
>  
>  /**
>    Provides the controller-specific addresses required to access system memory from a
> @@ -113,18 +233,6 @@ IoMmuMap (
>    }
>  
>    //
> -  // CommandBuffer was allocated by us (AllocateBuffer) and is already in
> -  // unencryted buffer so no need to create bounce buffer
> -  //
> -  if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
> -      Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
> -    *Mapping = NO_MAPPING;
> -    *DeviceAddress = PhysicalAddress;
> -
> -    return EFI_SUCCESS;
> -  }
> -
> -  //
>    // Allocate a MAP_INFO structure to remember the mapping when Unmap() is
>    // called later.
>    //

Please implement the free-list idea that I outlined earlier. Unmap()
must not call FreePool() on the MAP_INFO structures.

> @@ -144,6 +252,25 @@ IoMmuMap (
>    MapInfo->DeviceAddress     = DmaMemoryTop;
>  
>    //
> +  // If the requested Map() operation is BusMasterCommandBuffer then map
> +  // using internal function otherwise allocate a bounce buffer to map
> +  // the host buffer to device buffer
> +  //
> +  if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
> +      Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
> +
> +    Status = SetHostBufferAsDecrypted (MapInfo);
> +    if (EFI_ERROR (Status)) {
> +      FreePool (MapInfo);
> +      *NumberOfBytes = 0;
> +      return Status;
> +    }
> +
> +    MapInfo->DeviceAddress = MapInfo->HostAddress;
> +    goto Done;
> +  }

Please use a regular "else" branch here; we should use "goto" only for
jumping to error handling code.

Thanks,
Laszlo

> +
> +  //
>    // Allocate a buffer to map the transfer to.
>    //
>    Status = gBS->AllocatePages (
> @@ -178,6 +305,7 @@ IoMmuMap (
>        );
>    }
>  
> +Done:
>    //
>    // The DeviceAddress is the address of the maped buffer below 4GB
>    //
> @@ -219,18 +347,25 @@ IoMmuUnmap (
>      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 CommonBuffer operation from the Bus Master's point of
> +  // view then Map() have cleared the memory encryption mask from Host
> +  // buffer. Lets restore the memory encryption mask before returning
> +  //
> +  if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
> +      MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
> +
> +    Status = SetHostBufferAsEncrypted (MapInfo);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +
> +    goto Done;
> +  }
> +
> +  //
>    // 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.
> @@ -244,9 +379,6 @@ IoMmuUnmap (
>        );
>    }
>  
> -  DEBUG ((DEBUG_VERBOSE, "%a Device 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
> -        __FUNCTION__, MapInfo->DeviceAddress, MapInfo->HostAddress,
> -        MapInfo->NumberOfPages, MapInfo->NumberOfBytes));
>    //
>    // Restore the memory encryption mask
>    //
> @@ -254,9 +386,15 @@ IoMmuUnmap (
>    ASSERT_EFI_ERROR(Status);
>  
>    //
> -  // Free the mapped buffer and the MAP_INFO structure.
> +  // Free the bounce buffer
>    //
>    gBS->FreePages (MapInfo->DeviceAddress, MapInfo->NumberOfPages);
> +
> +Done:
> +  DEBUG ((DEBUG_VERBOSE, "%a Device 0x%Lx Host 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
> +        __FUNCTION__, MapInfo->DeviceAddress, MapInfo->HostAddress,
> +        MapInfo->NumberOfPages, MapInfo->NumberOfBytes));
> +
>    FreePool (Mapping);
>    return EFI_SUCCESS;
>  }
> 

Re: [PATCH v1 2/4] OvmfPkg: IommuDxe: Provide support for mapping BusMasterCommonBuffer operation

[Brijesh Singh]

Thanks Laszlo.


On 8/1/17 4:59 PM, Laszlo Ersek wrote:
> On 07/31/17 21:31, Brijesh Singh wrote:
> +  
[Snip]
>> The function is used for mapping and unmapping the Host buffer with
>> +  BusMasterCommonBuffer. Since the buffer can be accessed equally by the
>> +  processor and the DMA bus master hence we can not use the bounce buffer.
>> +
>> +  The function changes the underlying encryption mask of the pages that maps the
>> +  host buffer. It also ensures that buffer contents are updated with the desired
>> +  state.
>> +
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +SetBufferAsEncDec (
>> +  IN MAP_INFO *MapInfo,
>> +  IN BOOLEAN  Enc
>> +  )
>> +{
>> +  EFI_STATUS            Status;
>> +  EFI_PHYSICAL_ADDRESS  TempBuffer;
>> +
>> +  //
>> +  // Allocate an intermediate buffer to hold the host buffer contents
>> +  //
>> +  Status = gBS->AllocatePages (
>> +                  AllocateAnyPages,
>> +                  EfiBootServicesData,
>> +                  MapInfo->NumberOfPages,
>> +                  &TempBuffer
>> +                  );
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
> This is not right. This function is called (indirectly) from
> IoMmuUnmap(), which is the function that we'll call (also indirectly)
> from the ExitBootServices() callbacks. That means we cannot allocate or
> release dynamic memory here. This is why I suggested the page-sized
> static buffer, and page-wise copying.
>
> More on this later, below the end of this function.

Agree with your point, see more below

>> +
>> +  //
>> +  // If the host buffer has C-bit cleared, then make sure the intermediate
>> +  // buffer matches with same encryption mask.
>> +  //
>> +  if (!Enc) {
>> +    Status = MemEncryptSevClearPageEncMask (0, MapInfo->DeviceAddress,
>> +               MapInfo->NumberOfPages, TRUE);
>> +    ASSERT_EFI_ERROR (Status);
>> +  }
> As a separate issue, I don't think this is right. The auxiliary buffer
> that we use for in-place encryption or decryption should *always* be
> encrypted. (I mentioned this in my email, "Introduce a static UINT8
> array with EFI_PAGE_SIZE bytes (this will always remain in encrypted
> memory).", but I guess it was easy to miss.)

See more below

>> +
>> +  //
>> +  // Copy the data from host buffer into a temporary buffer. At this
>> +  // time both host and intermediate buffer will have same encryption
>> +  // mask.
>> +  //
> The comment should say "copy original buffer into temporary buffer".
> "host" buffer is very confusing here, as the virtualization host is
> exactly what may not yet have access to the buffer. In PCI bus master
> terminology, "host buffer" is valid, I think, but saying just "host
> buffer" is very confusing.

Will do.

> Also, "same encryption mask" is not desirable (see above), so please
> remove that sentence.
>
>> +  CopyMem (
>> +    (VOID *) (UINTN) TempBuffer,
>> +    (VOID *) (UINTN)MapInfo->HostAddress,
>> +    MapInfo->NumberOfBytes);
>> +
>> +  //
>> +  // Now change the encryption mask of the host buffer
>> +  //
>> +  if (Enc) {
>> +    Status = MemEncryptSevSetPageEncMask (0, MapInfo->HostAddress,
>> +               MapInfo->NumberOfPages, TRUE);
>> +    ASSERT_EFI_ERROR (Status);
> What guarantees that this function call will never fail?
>
> If it fails, we should propagate the error (the function prototype
> allows for that), and roll back partial changes along the way.
>
> If there is no way to undo partial actions when
> MemEncryptSevSetPageEncMask() fails, then the ASSERT() is fine, but we
> need an additional CpuDeadLoop() as well.

I will propagate the error.
>> +  } else {
>> +    Status = MemEncryptSevClearPageEncMask (0, MapInfo->HostAddress,
>> +               MapInfo->NumberOfPages, TRUE);
>> +    ASSERT_EFI_ERROR (Status);
>> +  }
>> +
>> +  //
>> +  // Copy the data from intermediate buffer into host buffer. At this
>> +  // time encryption masks will be different on host and intermediate
>> +  // buffer and the hardware will perform encryption/decryption on
>> +  // accesses.
>> +  //
>> +  CopyMem (
>> +    (VOID *) (UINTN)MapInfo->HostAddress,
>> +    (VOID *) (UINTN)TempBuffer,
>> +    MapInfo->NumberOfBytes);
>> +
>> +  //
>> +  // Restore the encryption mask of the intermediate buffer
>> +  //
>> +  if (!Enc) {
>> +    Status = MemEncryptSevSetPageEncMask (0, MapInfo->DeviceAddress,
>> +               MapInfo->NumberOfPages, TRUE);
>> +    ASSERT_EFI_ERROR (Status);
>> +  }
> Again, the intermediate buffer should always remain encrypted.
>
>> +
>> +  //
>> +  // Free the intermediate buffer
>> +  //
>> +  gBS->FreePages (TempBuffer, MapInfo->NumberOfPages);
>> +  return EFI_SUCCESS;
>> +}
> In summary for this function: I've now read section "7.10.8
> Encrypt-in-Place" of AMD pub #24593. Earlier I wrote,
>
>     Introduce a static UINT8 array with EFI_PAGE_SIZE bytes (this
>     will always remain in encrypted memory). Update the C bit with a
>     single function call for the entire range (like now) -- this will
>     not affect the guest-readability of the pages --, then bounce each
>     page within the range to the static buffer and back to its original
>     place.
>
> With regard to 7.10.8., this appears to be wrong. My idea was based on
> the expectation that changing the C bit will not affect *guest reads*
> from the same area. According to 7.10.8, my assumption was wrong: the
> algorithm in 7.10.8 uses a cache-line sized bounce buffer and *two*
> separate mappings for the area under in-place encryption. The reading
> always occurs through the original mapping.
>
> Now, this requires us to do one of two things:
>
> - Alternative 1: we can use the static, single page-sized buffer, in a
> loop. But then we must also break up the central
> MemEncryptSevSetPageEncMask() / MemEncryptSevClearPageEncMask() calls to
> small, page-sized calls. Copy out a page, change the C bit for that one
> page, copy back the page. Lather, rinse, repeat. This would likely be
> slow, and would guarantee that 2M pages would be split into 4K pages.

Yes, while implementing the code I had similar concern in my mind hence
I dropped the looping per page idea. So far, I have not seen really huge
request for BusMasterCommonBuffer Map(). The max I remember was 16 pages
when using the Ata drivers. It may not be as bad as we are thinking.
> - Alternative 2: we can stick with the large (unified) CopyMem() and
> C-bit changing function calls, but we still can't allocate memory for
> that in the SetBufferAsEncDec() function. Therefore you will have to
> allocate *twice* the requested number of pages in AllocateBuffer(),
> return the address of the lower half to the caller, and use the upper
> half as intermediate buffer in SetBufferAsEncDec(). This is safe because
> CommonBuffer[64] Map/Unmap requires the caller to pass in allocations
> from AllocateBuffer().

Yes, we could do something like this. thank for tip. This will certainly
will not affect the perform. I may give this as a first try in next rev.

>> +
>> +/**
>> +  This function will be called by Map() when mapping the buffer buffer to
>> +  BusMasterCommonBuffer type.
>> +
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +SetHostBufferAsEncrypted (
>> +  IN MAP_INFO *MapInfo
>> +  )
>> +{
>> +  return SetBufferAsEncDec (MapInfo, TRUE);
>> +}
>> +
>> +/**
>> +  This function will be called by Unmap() when unmapping host buffer
>> +  from the BusMasterCommonBuffer type.
>> +
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +SetHostBufferAsDecrypted (
>> +  IN MAP_INFO *MapInfo
>> +  )
>> +{
>> +  return SetBufferAsEncDec (MapInfo, FALSE);
>> +}
>>  
>>  /**
>>    Provides the controller-specific addresses required to access system memory from a
>> @@ -113,18 +233,6 @@ IoMmuMap (
>>    }
>>  
>>    //
>> -  // CommandBuffer was allocated by us (AllocateBuffer) and is already in
>> -  // unencryted buffer so no need to create bounce buffer
>> -  //
>> -  if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
>> -      Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
>> -    *Mapping = NO_MAPPING;
>> -    *DeviceAddress = PhysicalAddress;
>> -
>> -    return EFI_SUCCESS;
>> -  }
>> -
>> -  //
>>    // Allocate a MAP_INFO structure to remember the mapping when Unmap() is
>>    // called later.
>>    //
> Please implement the free-list idea that I outlined earlier. Unmap()
> must not call FreePool() on the MAP_INFO structures.

Sorry it was my bad. I missed implementing that feedback. I will fix it
in next rev. As you have outlined in previous feedback that Unmap() can
be called from ExitBootService() hence i will refrain from using
FreePool() or MemEncryptSev*() functions in this sequence (except when
freeing the actual bounce buffer).

>> @@ -144,6 +252,25 @@ IoMmuMap (
>>    MapInfo->DeviceAddress     = DmaMemoryTop;
>>  
>>    //
>> +  // If the requested Map() operation is BusMasterCommandBuffer then map
>> +  // using internal function otherwise allocate a bounce buffer to map
>> +  // the host buffer to device buffer
>> +  //
>> +  if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
>> +      Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
>> +
>> +    Status = SetHostBufferAsDecrypted (MapInfo);
>> +    if (EFI_ERROR (Status)) {
>> +      FreePool (MapInfo);
>> +      *NumberOfBytes = 0;
>> +      return Status;
>> +    }
>> +
>> +    MapInfo->DeviceAddress = MapInfo->HostAddress;
>> +    goto Done;
>> +  }
> Please use a regular "else" branch here; we should use "goto" only for
> jumping to error handling code.
Will do thanks

Re: [PATCH v1 2/4] OvmfPkg: IommuDxe: Provide support for mapping BusMasterCommonBuffer operation

[Laszlo Ersek]

On 08/02/17 01:51, Brijesh Singh wrote:
> On 8/1/17 4:59 PM, Laszlo Ersek wrote:


>> Please implement the free-list idea that I outlined earlier. Unmap()
>> must not call FreePool() on the MAP_INFO structures.
> 
> Sorry it was my bad. I missed implementing that feedback. I will fix it
> in next rev. As you have outlined in previous feedback that Unmap() can
> be called from ExitBootService() hence i will refrain from using
> FreePool() or MemEncryptSev*() functions in this sequence (except when
> freeing the actual bounce buffer).

No, you have to distinguish the C-bit's management and memory releasing.

Unmap must restore the C bit on the bounce buffer, for all of
BusMasterRead, BusMasterWrite, and BusMasterCommonBuffer. This is
required regardless of whether the bounce buffer was allocated
implicitly (for BusMasterRead and BusMasterWrite) or explicitly (for
BusMasterCommonBuffer, in AllocateBuffer()).

Unmap must zero and release the implicitly allocated bounce buffer for
BusMasterRead, BusMasterWrite.

Unmap must neither zero nor release the explicitly allocated buffer for
BusMasterCommonBuffer. For that operation, FreeBuffer() must do both steps.

Unmap must put MAP_INFO back on the internal free list for
BusMasterCommonBuffer. For BusMasterRead and BusMasterWrite, Unmap can
release MAP_INFO with FreePool().

Thanks
Laszlo

[PATCH v1 3/4] OvmfPkg: IommuDxe: Zero the shared page(s) on Unmap()

[Brijesh Singh]

To support the Map(), we allocate bounce buffer with C-bit cleared,
the buffer is referred as a DeviceAddress. Typically, DeviceAddress
is used as communication block between guest and hypervisor. When
guest is done with communication block, it calls Unmap().The Unmap()
free's the DeviceAddress, if we do not clear the content of shared
communication block during Unmap() then data remains readble to the
hypervisor for an unpredicatable time. Let's zero the bounce buffer
after we are done using it.

I did some benchmark and did not see any measure perform impact of
zeroing the page(s).

Suggested-by: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 5ae54482fffe..04e3725ff7e6 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -67,8 +67,7 @@ SetBufferAsEncDec (
   // buffer matches with same encryption mask.
   //
   if (!Enc) {
-    Status = MemEncryptSevClearPageEncMask (0, MapInfo->DeviceAddress,
-               MapInfo->NumberOfPages, TRUE);
+    Status = MemEncryptSevClearPageEncMask (0, TempBuffer, MapInfo->NumberOfPages, TRUE);
     ASSERT_EFI_ERROR (Status);
   }
 
@@ -79,7 +78,7 @@ SetBufferAsEncDec (
   //
   CopyMem (
     (VOID *) (UINTN) TempBuffer,
-    (VOID *) (UINTN)MapInfo->HostAddress,
+    (VOID *) (UINTN) MapInfo->HostAddress,
     MapInfo->NumberOfBytes);
 
   //
@@ -109,11 +108,8 @@ SetBufferAsEncDec (
   //
   // Restore the encryption mask of the intermediate buffer
   //
-  if (!Enc) {
-    Status = MemEncryptSevSetPageEncMask (0, MapInfo->DeviceAddress,
-               MapInfo->NumberOfPages, TRUE);
-    ASSERT_EFI_ERROR (Status);
-  }
+  Status = MemEncryptSevSetPageEncMask (0, TempBuffer, MapInfo->NumberOfPages, TRUE);
+  ASSERT_EFI_ERROR (Status);
 
   //
   // Free the intermediate buffer
@@ -386,6 +382,12 @@ IoMmuUnmap (
   ASSERT_EFI_ERROR(Status);
 
   //
+  // Zero the shared memory so that hypervisor no longer able to get intelligentable
+  // data.
+  //
+  SetMem ((VOID *) (UINTN)MapInfo->DeviceAddress, MapInfo->NumberOfBytes, 0);
+
+  //
   // Free the bounce buffer
   //
   gBS->FreePages (MapInfo->DeviceAddress, MapInfo->NumberOfPages);
-- 
2.7.4

Re: [PATCH v1 3/4] OvmfPkg: IommuDxe: Zero the shared page(s) on Unmap()

[Brijesh Singh]

I just realized that this patch contains some changes which should be
part of Patch 2/3. Please ignore this patch for now. I will fix it as part
of v2 when addressing review feedback. thanks

-Brijesh

On 07/31/2017 02:31 PM, Brijesh Singh wrote:
> To support the Map(), we allocate bounce buffer with C-bit cleared,
> the buffer is referred as a DeviceAddress. Typically, DeviceAddress
> is used as communication block between guest and hypervisor. When
> guest is done with communication block, it calls Unmap().The Unmap()
> free's the DeviceAddress, if we do not clear the content of shared
> communication block during Unmap() then data remains readble to the
> hypervisor for an unpredicatable time. Let's zero the bounce buffer
> after we are done using it.
> 
> I did some benchmark and did not see any measure perform impact of
> zeroing the page(s).
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>   OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 18 ++++++++++--------
>   1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> index 5ae54482fffe..04e3725ff7e6 100644
> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> @@ -67,8 +67,7 @@ SetBufferAsEncDec (
>     // buffer matches with same encryption mask.
>     //
>     if (!Enc) {
> -    Status = MemEncryptSevClearPageEncMask (0, MapInfo->DeviceAddress,
> -               MapInfo->NumberOfPages, TRUE);
> +    Status = MemEncryptSevClearPageEncMask (0, TempBuffer, MapInfo->NumberOfPages, TRUE);
>       ASSERT_EFI_ERROR (Status);
>     }
>   
> @@ -79,7 +78,7 @@ SetBufferAsEncDec (
>     //
>     CopyMem (
>       (VOID *) (UINTN) TempBuffer,
> -    (VOID *) (UINTN)MapInfo->HostAddress,
> +    (VOID *) (UINTN) MapInfo->HostAddress,
>       MapInfo->NumberOfBytes);
>   
>     //
> @@ -109,11 +108,8 @@ SetBufferAsEncDec (
>     //
>     // Restore the encryption mask of the intermediate buffer
>     //
> -  if (!Enc) {
> -    Status = MemEncryptSevSetPageEncMask (0, MapInfo->DeviceAddress,
> -               MapInfo->NumberOfPages, TRUE);
> -    ASSERT_EFI_ERROR (Status);
> -  }
> +  Status = MemEncryptSevSetPageEncMask (0, TempBuffer, MapInfo->NumberOfPages, TRUE);
> +  ASSERT_EFI_ERROR (Status);
>   
>     //
>     // Free the intermediate buffer
> @@ -386,6 +382,12 @@ IoMmuUnmap (
>     ASSERT_EFI_ERROR(Status);
>   
>     //
> +  // Zero the shared memory so that hypervisor no longer able to get intelligentable
> +  // data.
> +  //
> +  SetMem ((VOID *) (UINTN)MapInfo->DeviceAddress, MapInfo->NumberOfBytes, 0);
> +
> +  //
>     // Free the bounce buffer
>     //
>     gBS->FreePages (MapInfo->DeviceAddress, MapInfo->NumberOfPages);
> 

Re: [PATCH v1 3/4] OvmfPkg: IommuDxe: Zero the shared page(s) on Unmap()

[Laszlo Ersek]

On 07/31/17 21:31, Brijesh Singh wrote:
> To support the Map(), we allocate bounce buffer with C-bit cleared,
> the buffer is referred as a DeviceAddress. Typically, DeviceAddress
> is used as communication block between guest and hypervisor. When
> guest is done with communication block, it calls Unmap().The Unmap()
> free's the DeviceAddress, if we do not clear the content of shared
> communication block during Unmap() then data remains readble to the
> hypervisor for an unpredicatable time. Let's zero the bounce buffer
> after we are done using it.
> 
> I did some benchmark and did not see any measure perform impact of
> zeroing the page(s).
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> index 5ae54482fffe..04e3725ff7e6 100644
> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> @@ -67,8 +67,7 @@ SetBufferAsEncDec (
>    // buffer matches with same encryption mask.
>    //
>    if (!Enc) {
> -    Status = MemEncryptSevClearPageEncMask (0, MapInfo->DeviceAddress,
> -               MapInfo->NumberOfPages, TRUE);
> +    Status = MemEncryptSevClearPageEncMask (0, TempBuffer, MapInfo->NumberOfPages, TRUE);
>      ASSERT_EFI_ERROR (Status);
>    }
>  
> @@ -79,7 +78,7 @@ SetBufferAsEncDec (
>    //
>    CopyMem (
>      (VOID *) (UINTN) TempBuffer,
> -    (VOID *) (UINTN)MapInfo->HostAddress,
> +    (VOID *) (UINTN) MapInfo->HostAddress,
>      MapInfo->NumberOfBytes);
>  
>    //
> @@ -109,11 +108,8 @@ SetBufferAsEncDec (
>    //
>    // Restore the encryption mask of the intermediate buffer
>    //
> -  if (!Enc) {
> -    Status = MemEncryptSevSetPageEncMask (0, MapInfo->DeviceAddress,
> -               MapInfo->NumberOfPages, TRUE);
> -    ASSERT_EFI_ERROR (Status);
> -  }
> +  Status = MemEncryptSevSetPageEncMask (0, TempBuffer, MapInfo->NumberOfPages, TRUE);
> +  ASSERT_EFI_ERROR (Status);
>  
>    //
>    // Free the intermediate buffer
> @@ -386,6 +382,12 @@ IoMmuUnmap (
>    ASSERT_EFI_ERROR(Status);
>  
>    //
> +  // Zero the shared memory so that hypervisor no longer able to get intelligentable
> +  // data.
> +  //
> +  SetMem ((VOID *) (UINTN)MapInfo->DeviceAddress, MapInfo->NumberOfBytes, 0);

Please use ZeroMem().

Furthermore, ZeroMem() should occur just before every FreePages() call:
- when Unmap() releases the implicitly allocated bounce buffer
- when FreeBuffer() releases the explicitly allocated common buffer
  (I thought I spelled this out in my previous email(s), but in
  retrospect it seems I only intended to :/ )
- in the virtio drivers' exit-boot-services callbacks, FreeBuffer()
  can't be called (only Unmap(), after the virtio reset), so the
  ZeroMem() should be done manually there.

Thanks
Laszlo

> +
> +  //
>    // Free the bounce buffer
>    //
>    gBS->FreePages (MapInfo->DeviceAddress, MapInfo->NumberOfPages);
> 

Re: [PATCH v1 3/4] OvmfPkg: IommuDxe: Zero the shared page(s) on Unmap()

[Brijesh Singh]

On 8/2/17 2:37 AM, Laszlo Ersek wrote:
> //
>> +  // Zero the shared memory so that hypervisor no longer able to get intelligentable
>> +  // data.
>> +  //
>> +  SetMem ((VOID *) (UINTN)MapInfo->DeviceAddress, MapInfo->NumberOfBytes, 0);
> Please use ZeroMem().
>
> Furthermore, ZeroMem() should occur just before every FreePages() call:
> - when Unmap() releases the implicitly allocated bounce buffer
> - when FreeBuffer() releases the explicitly allocated common buffer
>   (I thought I spelled this out in my previous email(s), but in
>   retrospect it seems I only intended to :/ )
> - in the virtio drivers' exit-boot-services callbacks, FreeBuffer()
>   can't be called (only Unmap(), after the virtio reset), so the
>   ZeroMem() should be done manually there.

Not sure why do we need to ZeroMem() when FreeBuffer()  is called for
explicitly allocated common buffer ? I thought before calling the
FreeBuffer() on common buffer, client will call Unmap() which will
restore the C-bit state on the common buffer and also update the
contents (i.e now common buffer will contain encrypted data).

Re: [PATCH v1 3/4] OvmfPkg: IommuDxe: Zero the shared page(s) on Unmap()

[Laszlo Ersek]

On 08/02/17 13:22, Brijesh Singh wrote:
> 
> 
> On 8/2/17 2:37 AM, Laszlo Ersek wrote:
>> //
>>> +  // Zero the shared memory so that hypervisor no longer able to get intelligentable
>>> +  // data.
>>> +  //
>>> +  SetMem ((VOID *) (UINTN)MapInfo->DeviceAddress, MapInfo->NumberOfBytes, 0);
>> Please use ZeroMem().
>>
>> Furthermore, ZeroMem() should occur just before every FreePages() call:
>> - when Unmap() releases the implicitly allocated bounce buffer
>> - when FreeBuffer() releases the explicitly allocated common buffer
>>   (I thought I spelled this out in my previous email(s), but in
>>   retrospect it seems I only intended to :/ )
>> - in the virtio drivers' exit-boot-services callbacks, FreeBuffer()
>>   can't be called (only Unmap(), after the virtio reset), so the
>>   ZeroMem() should be done manually there.
> 
> Not sure why do we need to ZeroMem() when FreeBuffer()  is called for
> explicitly allocated common buffer ? I thought before calling the
> FreeBuffer() on common buffer, client will call Unmap() which will
> restore the C-bit state on the common buffer and also update the
> contents (i.e now common buffer will contain encrypted data).
> 

My bad, you are totally right -- when I wrote the above, I actually
reviewed the "BusMasterCommonBuffer" section of my earlier message

http://mid.mail-archive.com/e1137140-f5d5-7eb8-0162-952694b24f96@redhat.com

and I totally missed that in that message I had written

"Client calls Unmap(). Unmap() restores the C bit in one fell swoop, and
encrypts the buffer in-place (by bouncing it page-wise to the static
array and back)."

Sigh. Need more rest.

Thanks for catching my error!
Laszlo

[PATCH v1 4/4] OvmfPkg : QemuFwCfgLib: Map DMA buffer with CommonBuffer when SEV is enable

[Brijesh Singh]

Commit 09719a01b11b (OvmfPkg/QemuFwCfgLib: Implement SEV internal function for Dxe phase)
uses IOMMU protocol to allocate and free DMA buffer used by
QemuFwCfgDxe when SEV is enabled. During the initial commit we
made assumption that IOMMU.AllocateBuffer() will provide a buffer
with C-bit cleared. But inorder to comply with EFI PCI protocol,
recent changes in IoMmuDxe driver does not provide a buffer with
C-bit cleared. If the DMA buffer need to be bi-directional (i.e
processor and the DMA bus master can access the buffer equally)
then buffer must be Map() with BusMasterCommonBuffer operation.

The patch refactors the code and add the support to Map() the
FW_CFG_DMA_ACCESS buffer using BusMasterCommonBuffer operation.

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>
Contributed-under: TianoCore Contribution Agreement 1.0
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
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         | 131 -----------
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c         | 101 +++++---
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c         |  56 ++---
 5 files changed, 292 insertions(+), 285 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 d4e09c434e6a..7f42f38d1c05 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -45,137 +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 warnigns
-  // 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

Re: [PATCH v1 4/4] OvmfPkg : QemuFwCfgLib: Map DMA buffer with CommonBuffer when SEV is enable

[Laszlo Ersek]

On 07/31/17 21:31, Brijesh Singh wrote:

> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> index d4e09c434e6a..7f42f38d1c05 100644
> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c

> -  // set NumPages to suppress incorrect compiler/analyzer warnigns
> -  // set NumPages to suppress incorrect compiler/analyzer warnings
> -  //
> -  NumPages = 0;

In order to review this patch more easily, I tried to apply it locally.
It failed, due to the above lines.

I think you may have an earlier mismerge in your local tree. Namely,
before committing e508e069a809 ("OvmfPkg/QemuFwCfgLib: Suppress GCC49
IA32 build failure", 2017-07-11), I fixed up the comment:

    [lersek@redhat.com: s/warnigns/warnings/ in the code comment]

and when you pulled master later, you may have ended up with a merge
rather than a fast-forward, in your local tree.

Please fix this and I'll review this patch in the next iteration.
(Applying the patch locally helps a bit with that, because removed and
added code are unfortunately intermixed in this patch, and so the
colorizing done by "git diff --color" helps.)

Thanks
Laszlo