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

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

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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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;
>  }
> 

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

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

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

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel