Re: [RFC v4 06/13] OvmfPkg:AmdSevDxe: add AmdSevDxe driver

[Laszlo Ersek <lersek@redhat.com>]

On 05/11/17 17:19, Laszlo Ersek wrote:
> comments below:
> 
> On 05/11/17 00:09, Brijesh Singh wrote:
>> When SEV is enabled, the MMIO memory range must be mapped as unencrypted
>> (i.e C-bit cleared) and DMA must be performed on unencrypted memory.
>>
>> The patch adds a DXE driver that runs early in boot and clears the memory
>> encryption attribute from MMIO/NonExistent memory ranges and installs a
>> IOMMU protocol to provide the DMA support for PCIHostBridge and other drivers.
> 
> (1) Please mention that the C bit is cleared for MMIO GCD entries in
> order to cover the ranges that were added during the PEI phase (through
> memory resource descriptor HOBs).
> 
> Also mention that the NonExistent ranges are processed in order to
> cover, in advance, MMIO ranges added later in the DXE phase by various
> device drivers, via the appropriate DXE memory space services.
> 
> Finally, please mention that the approach is not transparent for later
> addition of system memory ranges to the GCD memory space map. (Such
> ranges should be encrypted.) OVMF does not do such a thing at the
> moment, so this approach should be OK.
> 
> I think we should also credit Jiewen for both ideas, namely the IOMMU
> stuff and the handling of NonExistent ranges (in anticipation of future
> MMIO additions), so please add
> 
> Suggested-by: Jiewen Yao <jiewen.yao@intel.com>

(5) Please mention that the driver is being added to the APRIORI DXE
file for a separate reason as well (not just for the early clearing of
the C bit on MMIO/NonExistent): OvmfPkg's DXE phase modules that tailor
their behavior to SEV presence will assume that the IOMMU protocol
exported by this driver is available *at once*.

(If you fix this up as well, you can add my R-b -- see under (4).)

Thanks,
Laszlo

> 
>>
>> The driver produces IOMMU protocol introduce by Jiewen
>> https://lists.01.org/pipermail/edk2-devel/2017-May/010462.html
>>
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Leo Duran <leo.duran@amd.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>  OvmfPkg/OvmfPkgIa32X64.dsc      |   1 +
>>  OvmfPkg/OvmfPkgX64.dsc          |   1 +
>>  OvmfPkg/OvmfPkgIa32X64.fdf      |   2 +
>>  OvmfPkg/OvmfPkgX64.fdf          |   2 +
>>  OvmfPkg/AmdSevDxe/AmdSevDxe.inf |  49 +++
>>  OvmfPkg/AmdSevDxe/AmdSevIommu.h |  43 ++
>>  OvmfPkg/AmdSevDxe/AmdSevMmio.h  |  41 ++
>>  OvmfPkg/AmdSevDxe/AmdSevDxe.c   |  52 +++
>>  OvmfPkg/AmdSevDxe/AmdSevIommu.c | 459 ++++++++++++++++++++
>>  OvmfPkg/AmdSevDxe/AmdSevMmio.c  |  50 +++
>>  10 files changed, 700 insertions(+)
>>
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index 9403f76ce862..ee6f98d68b73 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -827,6 +827,7 @@ [Components.X64]
>>  !endif
>>  
>>    OvmfPkg/PlatformDxe/Platform.inf
>> +  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>>  
>>  !if $(SMM_REQUIRE) == TRUE
>>    OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index e137143f7afa..b5f26e06e60b 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -825,6 +825,7 @@ [Components]
>>  !endif
>>  
>>    OvmfPkg/PlatformDxe/Platform.inf
>> +  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>>  
>>  !if $(SMM_REQUIRE) == TRUE
>>    OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
>> index 5233314139bc..12871860d001 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
>> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
>> @@ -190,6 +190,7 @@ [FV.DXEFV]
>>  APRIORI DXE {
>>    INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
>>    INF  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
>> +  INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>>  !if $(SMM_REQUIRE) == FALSE
>>    INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
>>  !endif
>> @@ -351,6 +352,7 @@ [FV.DXEFV]
>>  INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
>>  INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
>>  INF  OvmfPkg/PlatformDxe/Platform.inf
>> +INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>>  
>>  !if $(SMM_REQUIRE) == TRUE
>>  INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>> index 36150101e784..ae6e66a1c08d 100644
>> --- a/OvmfPkg/OvmfPkgX64.fdf
>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>> @@ -190,6 +190,7 @@ [FV.DXEFV]
>>  APRIORI DXE {
>>    INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
>>    INF  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
>> +  INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>>  !if $(SMM_REQUIRE) == FALSE
>>    INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
>>  !endif
>> @@ -351,6 +352,7 @@ [FV.DXEFV]
>>  INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
>>  INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
>>  INF  OvmfPkg/PlatformDxe/Platform.inf
>> +INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>>  
>>  !if $(SMM_REQUIRE) == TRUE
>>  INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
>> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>> new file mode 100644
>> index 000000000000..775dda9be386
>> --- /dev/null
>> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>> @@ -0,0 +1,49 @@
>> +#/** @file
>> +#
>> +#  Driver clears the encryption attribute from MMIO regions and installs IOMMU
>> +#  protcol to provides DMA support for PciHostBridge and others
>> +#
>> +#  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
>> +#
>> +#  This program and the accompanying materials
>> +#  are licensed and made available under the terms and conditions of the BSD
>> +#  License which accompanies this distribution.  The full text of the license may
>> +#  be found at http://opensource.org/licenses/bsd-license.php
>> +#
>> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +#
>> +#**/
>> +
>> +[Defines]
>> +  INF_VERSION                    = 1.25
>> +  BASE_NAME                      = AmdSevDxe
>> +  FILE_GUID                      = 2ec9da37-ee35-4de9-86c5-6d9a81dc38a7
>> +  MODULE_TYPE                    = DXE_DRIVER
>> +  VERSION_STRING                 = 1.0
>> +  ENTRY_POINT                    = AmdSevDxeEntryPoint
>> +
>> +[Sources]
>> +  AmdSevDxe.c
>> +  AmdSevIommu.c
>> +  AmdSevMmio.c
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  MdeModulePkg/MdeModulePkg.dec
>> +  OvmfPkg/OvmfPkg.dec
>> +
>> +[LibraryClasses]
>> +  BaseLib
>> +  UefiLib
>> +  UefiDriverEntryPoint
>> +  UefiBootServicesTableLib
>> +  DxeServicesTableLib
>> +  DebugLib
>> +  MemEncryptSevLib
>> +
>> +[Protocols]
>> +  gEdkiiIoMmuProtocolGuid                     ## PRODUCES
>> +
>> +[Depex]
>> +  TRUE
>> diff --git a/OvmfPkg/AmdSevDxe/AmdSevIommu.h b/OvmfPkg/AmdSevDxe/AmdSevIommu.h
>> new file mode 100644
>> index 000000000000..5712cb57052d
>> --- /dev/null
>> +++ b/OvmfPkg/AmdSevDxe/AmdSevIommu.h
>> @@ -0,0 +1,43 @@
>> +/** @file
>> +
>> +  The protocol provides support to allocate, free, map and umap a DMA buffer for
>> +  bus master (e.g PciHostBridge). When SEV is enabled, the DMA operations must
>> +  be performed on unencrypted buffer hence protocol clear the encryption bit
>> +  from the DMA buffer.
>> +
>> +  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>> +  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
>> +  This program and the accompanying materials are licensed and made available
>> +  under the terms and conditions of the BSD License which accompanies this
>> +  distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef __AMDSEVIOMMU_H_
>> +#define __AMDSEVIOMMU_H
>> +
>> +#include <Protocol/IoMmu.h>
>> +
>> +#include <Library/BaseLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/MemEncryptSevLib.h>
>> +
>> +/**
>> +  Install IOMMU protocol to provide the DMA support for PciHostBridge and
>> +  MemEncryptSevLib.
>> +
>> +**/
>> +VOID
>> +EFIAPI
>> +AmdSevInstallIommuProtocol (
>> +  VOID
>> +  );
>> +
>> +#endif
>> diff --git a/OvmfPkg/AmdSevDxe/AmdSevMmio.h b/OvmfPkg/AmdSevDxe/AmdSevMmio.h
>> new file mode 100644
>> index 000000000000..c6191025d921
>> --- /dev/null
>> +++ b/OvmfPkg/AmdSevDxe/AmdSevMmio.h
>> @@ -0,0 +1,41 @@
>> +/** @file
>> +
>> +  Implements routines to clear C-bit from MMIO Memory Range
>> +
>> +  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
>> +
>> +  This program and the accompanying materials are licensed and made available
>> +  under the terms and conditions of the BSD License which accompanies this
>> +  distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef __AMDSEVMMIO_H_
>> +#define __AMDSEVMMIO_H
>> +
>> +#include <Library/BaseLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/DxeServicesTableLib.h>
>> +#include <Library/MemEncryptSevLib.h>
>> +
>> +/**
>> +
>> +  Iterate through the GCD map and clear the C-bit from MMIO and NonExistent
>> +  memory space. The NonExistent memory space will be used for mapping the MMIO
>> +  space added later (eg PciRootBridge). By clearing both known NonExistent
>> +  memory space can gurantee that any MMIO mapped later will have C-bit cleared.
>> +*/
>> +VOID
>> +EFIAPI
>> +AmdSevClearEncMaskMmioRange (
>> +  VOID
>> +  );
>> +
>> +#endif
>> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
>> new file mode 100644
>> index 000000000000..e22e7ef7314f
>> --- /dev/null
>> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
>> @@ -0,0 +1,52 @@
>> +/** @file
>> +
>> +  AMD Sev Dxe driver. The driver runs early in DXE phase and clears C-bit from
>> +  MMIO space and installs EDKII_IOMMU_PROTOCOL to provide the support for DMA
>> +  operations when SEV is enabled.
>> +
>> +  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
>> +
>> +  This program and the accompanying materials
>> +  are licensed and made available under the terms and conditions of the BSD
>> +  License which accompanies this distribution.  The full text of the license may
>> +  be found at http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#include <PiDxe.h>
>> +
>> +#include <Library/MemEncryptSevLib.h>
>> +
>> +#include "AmdSevMmio.h"
>> +#include "AmdSevIommu.h"
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +AmdSevDxeEntryPoint (
>> +  IN EFI_HANDLE         ImageHandle,
>> +  IN EFI_SYSTEM_TABLE   *SystemTable
>> +  )
>> +{
>> +  //
>> +  // Do nothing when SEV is not enabled
>> +  //
>> +  if (!MemEncryptSevIsEnabled ()) {
>> +    return EFI_SUCCESS;
>> +  }
> 
> (2) The status code should be EFI_UNSUPPORTED or EFI_ABORTED. Returning
> with EFI_SUCCESS will uselessly keep the driver in memory.
> 
>> +
>> +  //
>> +  // Clear C-bit from MMIO Memory Range
>> +  //
>> +  AmdSevClearEncMaskMmioRange ();
>> +
>> +  //
>> +  // Install IOMMU protocol to provide DMA support for PCIHostBridgeIo and
>> +  // AmdSevMemEncryptLib.
> 
> (3) What is AmdSevMemEncryptLib? Is this comment perhaps stale?
> 
>> +  //
>> +  AmdSevInstallIommuProtocol ();
>> +
>> +  return EFI_SUCCESS;
>> +}
>> diff --git a/OvmfPkg/AmdSevDxe/AmdSevIommu.c b/OvmfPkg/AmdSevDxe/AmdSevIommu.c
>> new file mode 100644
>> index 000000000000..9b35469ca34f
>> --- /dev/null
>> +++ b/OvmfPkg/AmdSevDxe/AmdSevIommu.c
>> @@ -0,0 +1,459 @@
>> +/** @file
>> +  AmdSevIommu related function
>> +
>> +  The protocol provides support to allocate, free, map and umap a DMA buffer for
>> +  bus master (e.g PciHostBridge). When SEV is enabled, the DMA operations must
>> +  be performed on unencrypted buffer hence we use a bounce buffer to map the host
>> +  buffer into an unencrypted buffer.
>> +
>> +  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
>> +
>> +  This program and the accompanying materials are licensed and made available
>> +  under the terms and conditions of the BSD License which accompanies this
>> +  distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#include "AmdSevIommu.h"
>> +
>> +typedef struct {
>> +  EDKII_IOMMU_OPERATION                     Operation;
>> +  UINTN                                     NumberOfBytes;
>> +  UINTN                                     NumberOfPages;
>> +  EFI_PHYSICAL_ADDRESS                      HostAddress;
>> +  EFI_PHYSICAL_ADDRESS                      DeviceAddress;
>> +} MAP_INFO;
>> +
>> +#define NO_MAPPING             (VOID *) (UINTN) -1
>> +
>> +/**
>> +  Provides the controller-specific addresses required to access system memory from a
>> +  DMA bus master. On SEV guest, the DMA operations must be performed on shared
>> +  buffer hence we allocate a bounce buffer to map the HostAddress to a DeviceAddress.
>> +  The Encryption attribute is removed from the DeviceAddress buffer.
>> +
>> +  @param  This                  The protocol instance pointer.
>> +  @param  Operation             Indicates if the bus master is going to read or
>> +                                write to system memory.
>> +  @param  HostAddress           The system memory address to map to the PCI controller.
>> +  @param  NumberOfBytes         On input the number of bytes to map. On output
>> +                                the number of bytes
>> +                                that were mapped.
>> +  @param  DeviceAddress         The resulting map address for the bus master PCI
>> +                                controller to use to
>> +                                access the hosts HostAddress.
>> +  @param  Mapping               A resulting value to pass to Unmap().
>> +
>> +  @retval EFI_SUCCESS           The range was mapped for the returned NumberOfBytes.
>> +  @retval EFI_UNSUPPORTED       The HostAddress cannot be mapped as a common buffer.
>> +  @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
>> +  @retval EFI_OUT_OF_RESOURCES  The request could not be completed due to a lack
>> +                                of resources.
>> +  @retval EFI_DEVICE_ERROR      The system hardware could not map the requested address.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +IoMmuMap (
>> +  IN     EDKII_IOMMU_PROTOCOL                       *This,
>> +  IN     EDKII_IOMMU_OPERATION                      Operation,
>> +  IN     VOID                                       *HostAddress,
>> +  IN OUT UINTN                                      *NumberOfBytes,
>> +  OUT    EFI_PHYSICAL_ADDRESS                       *DeviceAddress,
>> +  OUT    VOID                                       **Mapping
>> +  )
>> +{
>> +  EFI_STATUS                                        Status;
>> +  EFI_PHYSICAL_ADDRESS                              PhysicalAddress;
>> +  MAP_INFO                                          *MapInfo;
>> +  EFI_PHYSICAL_ADDRESS                              DmaMemoryTop;
>> +  EFI_ALLOCATE_TYPE                                 AllocateType;
>> +
>> +  if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
>> +      Mapping == NULL) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  //
>> +  // Make sure that Operation is valid
>> +  //
>> +  if ((UINT32) Operation >= EdkiiIoMmuOperationMaximum) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +  PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
>> +
>> +  DmaMemoryTop = (UINTN)-1;
>> +  AllocateType = AllocateAnyPages;
>> +
>> +  if (((Operation != EdkiiIoMmuOperationBusMasterRead64 &&
>> +        Operation != EdkiiIoMmuOperationBusMasterWrite64 &&
>> +        Operation != EdkiiIoMmuOperationBusMasterCommonBuffer64)) &&
>> +      ((PhysicalAddress + *NumberOfBytes) > SIZE_4GB)) {
>> +    //
>> +    // If the root bridge or the device cannot handle performing DMA above
>> +    // 4GB but any part of the DMA transfer being mapped is above 4GB, then
>> +    // map the DMA transfer to a buffer below 4GB.
>> +    //
>> +    DmaMemoryTop = SIZE_4GB - 1;
>> +    AllocateType = AllocateMaxAddress;
>> +
>> +    if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
>> +        Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
>> +        //
>> +        // Common Buffer operations can not be remapped.  If the common buffer
>> +        // if above 4GB, then it is not possible to generate a mapping, so return
>> +        // an error.
>> +        //
>> +        return EFI_UNSUPPORTED;
>> +    }
>> +  }
>> +
>> +  //
>> +  // 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.
>> +  //
>> +  MapInfo = AllocatePool (sizeof (MAP_INFO));
>> +  if (MapInfo == NULL) {
>> +    *NumberOfBytes = 0;
>> +    return EFI_OUT_OF_RESOURCES;
>> +  }
>> +
>> +  //
>> +  // Initialize the MAP_INFO structure
>> +  //
>> +  MapInfo->Operation         = Operation;
>> +  MapInfo->NumberOfBytes     = *NumberOfBytes;
>> +  MapInfo->NumberOfPages     = EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes);
>> +  MapInfo->HostAddress       = PhysicalAddress;
>> +  MapInfo->DeviceAddress     = DmaMemoryTop;
>> +
>> +  //
>> +  // Allocate a buffer to map the transfer to.
>> +  //
>> +  Status = gBS->AllocatePages (
>> +                  AllocateType,
>> +                  EfiBootServicesData,
>> +                  MapInfo->NumberOfPages,
>> +                  &MapInfo->DeviceAddress
>> +                  );
>> +  if (EFI_ERROR (Status)) {
>> +    FreePool (MapInfo);
>> +    *NumberOfBytes = 0;
>> +    return Status;
>> +  }
>> +
>> +  //
>> +  // Clear the memory encryption mask from the device buffer
>> +  //
>> +  Status = MemEncryptSevClearPageEncMask (0, MapInfo->DeviceAddress, MapInfo->NumberOfPages, TRUE);
>> +  ASSERT_EFI_ERROR(Status);
>> +
>> +  //
>> +  // If this is a read operation from the Bus Master's point of view,
>> +  // then copy the contents of the real buffer into the mapped buffer
>> +  // so the Bus Master can read the contents of the real buffer.
>> +  //
>> +  if (Operation == EdkiiIoMmuOperationBusMasterRead ||
>> +      Operation == EdkiiIoMmuOperationBusMasterRead64) {
>> +    CopyMem (
>> +      (VOID *) (UINTN) MapInfo->DeviceAddress,
>> +      (VOID *) (UINTN) MapInfo->HostAddress,
>> +      MapInfo->NumberOfBytes
>> +      );
>> +  }
>> +
>> +  //
>> +  // The DeviceAddress is the address of the maped buffer below 4GB
>> +  //
>> +  *DeviceAddress = MapInfo->DeviceAddress;
>> +
>> +  //
>> +  // Return a pointer to the MAP_INFO structure in Mapping
>> +  //
>> +  *Mapping       = MapInfo;
>> +
>> +  DEBUG ((DEBUG_VERBOSE, "%a Host 0x%Lx Device 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
>> +        __FUNCTION__, MapInfo->DeviceAddress, MapInfo->HostAddress,
>> +        MapInfo->NumberOfPages, MapInfo->NumberOfBytes));
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +/**
>> +  Completes the Map() operation and releases any corresponding resources.
>> +
>> +  @param  This                  The protocol instance pointer.
>> +  @param  Mapping               The mapping value returned from Map().
>> +
>> +  @retval EFI_SUCCESS           The range was unmapped.
>> +  @retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by Map().
>> +  @retval EFI_DEVICE_ERROR      The data was not committed to the target system memory.
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +IoMmuUnmap (
>> +  IN  EDKII_IOMMU_PROTOCOL                     *This,
>> +  IN  VOID                                     *Mapping
>> +  )
>> +{
>> +  MAP_INFO                 *MapInfo;
>> +  EFI_STATUS               Status;
>> +
>> +  if (Mapping == NULL) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  //
>> +  // See if the Map() operation associated with this Unmap() required a mapping
>> +  // buffer. If a mapping buffer was not required, then this function simply
>> +  // buffer. If a mapping buffer was not required, then this function simply
>> +  //
>> +  if (Mapping == NO_MAPPING) {
>> +    return EFI_SUCCESS;
>> +  }
>> +
>> +  MapInfo = (MAP_INFO *)Mapping;
>> +
>> +  //
>> +  // If this is a write operation from the Bus Master's point of view,
>> +  // then copy the contents of the mapped buffer into the real buffer
>> +  // so the processor can read the contents of the real buffer.
>> +  //
>> +  if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite ||
>> +      MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite64) {
>> +    CopyMem (
>> +      (VOID *) (UINTN) MapInfo->HostAddress,
>> +      (VOID *) (UINTN) MapInfo->DeviceAddress,
>> +      MapInfo->NumberOfBytes
>> +      );
>> +  }
>> +
>> +  DEBUG ((DEBUG_VERBOSE, "%a Host 0x%Lx Device 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
>> +        __FUNCTION__, MapInfo->DeviceAddress, MapInfo->HostAddress,
>> +        MapInfo->NumberOfPages, MapInfo->NumberOfBytes));
>> +  //
>> +  // Restore the memory encryption mask
>> +  //
>> +  Status = MemEncryptSevSetPageEncMask (0, MapInfo->DeviceAddress, MapInfo->NumberOfPages, TRUE);
>> +  ASSERT_EFI_ERROR(Status);
>> +
>> +  //
>> +  // Free the mapped buffer and the MAP_INFO structure.
>> +  //
>> +  gBS->FreePages (MapInfo->DeviceAddress, MapInfo->NumberOfPages);
>> +  FreePool (Mapping);
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +/**
>> +  Allocates pages that are suitable for an OperationBusMasterCommonBuffer or
>> +  OperationBusMasterCommonBuffer64 mapping.
>> +
>> +  @param  This                  The protocol instance pointer.
>> +  @param  Type                  This parameter is not used and must be ignored.
>> +  @param  MemoryType            The type of memory to allocate, EfiBootServicesData
>> +                                or EfiRuntimeServicesData.
>> +  @param  Pages                 The number of pages to allocate.
>> +  @param  HostAddress           A pointer to store the base system memory address
>> +                                of the allocated range.
>> +  @param  Attributes            The requested bit mask of attributes for the allocated range.
>> +
>> +  @retval EFI_SUCCESS           The requested memory pages were allocated.
>> +  @retval EFI_UNSUPPORTED       Attributes is unsupported. The only legal attribute
>> +                                bits are MEMORY_WRITE_COMBINE and MEMORY_CACHED.
>> +  @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
>> +  @retval EFI_OUT_OF_RESOURCES  The memory pages could not be allocated.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +IoMmuAllocateBuffer (
>> +  IN     EDKII_IOMMU_PROTOCOL                     *This,
>> +  IN     EFI_ALLOCATE_TYPE                        Type,
>> +  IN     EFI_MEMORY_TYPE                          MemoryType,
>> +  IN     UINTN                                    Pages,
>> +  IN OUT VOID                                     **HostAddress,
>> +  IN     UINT64                                   Attributes
>> +  )
>> +{
>> +  EFI_STATUS                Status;
>> +  EFI_PHYSICAL_ADDRESS      PhysicalAddress;
>> +
>> +  //
>> +  // Validate Attributes
>> +  //
>> +  if ((Attributes & EDKII_IOMMU_ATTRIBUTE_INVALID_FOR_ALLOCATE_BUFFER) != 0) {
>> +    return EFI_UNSUPPORTED;
>> +  }
>> +
>> +  //
>> +  // Check for invalid inputs
>> +  //
>> +  if (HostAddress == NULL) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  //
>> +  // The only valid memory types are EfiBootServicesData and
>> +  // EfiRuntimeServicesData
>> +  //
>> +  if (MemoryType != EfiBootServicesData &&
>> +      MemoryType != EfiRuntimeServicesData) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  PhysicalAddress = (UINTN)-1;
>> +  if ((Attributes & EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0) {
>> +    //
>> +    // Limit allocations to memory below 4GB
>> +    //
>> +    PhysicalAddress = SIZE_4GB - 1;
>> +  }
>> +  Status = gBS->AllocatePages (
>> +                  AllocateMaxAddress,
>> +                  MemoryType,
>> +                  Pages,
>> +                  &PhysicalAddress
>> +                  );
>> +  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));
>> +  return Status;
>> +}
>> +
>> +/**
>> +  Frees memory that was allocated with AllocateBuffer().
>> +
>> +  @param  This                  The protocol instance pointer.
>> +  @param  Pages                 The number of pages to free.
>> +  @param  HostAddress           The base system memory address of the allocated range.
>> +
>> +  @retval EFI_SUCCESS           The requested memory pages were freed.
>> +  @retval EFI_INVALID_PARAMETER The memory range specified by HostAddress and Pages
>> +                                was not allocated with AllocateBuffer().
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +IoMmuFreeBuffer (
>> +  IN  EDKII_IOMMU_PROTOCOL                     *This,
>> +  IN  UINTN                                    Pages,
>> +  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);
>> +}
>> +
>> +
>> +/**
>> +  Set IOMMU attribute for a system memory.
>> +
>> +  If the IOMMU protocol exists, the system memory cannot be used
>> +  for DMA by default.
>> +
>> +  When a device requests a DMA access for a system memory,
>> +  the device driver need use SetAttribute() to update the IOMMU
>> +  attribute to request DMA access (read and/or write).
>> +
>> +  The DeviceHandle is used to identify which device submits the request.
>> +  The IOMMU implementation need translate the device path to an IOMMU device ID,
>> +  and set IOMMU hardware register accordingly.
>> +  1) DeviceHandle can be a standard PCI device.
>> +     The memory for BusMasterRead need set EDKII_IOMMU_ACCESS_READ.
>> +     The memory for BusMasterWrite need set EDKII_IOMMU_ACCESS_WRITE.
>> +     The memory for BusMasterCommonBuffer need set EDKII_IOMMU_ACCESS_READ|EDKII_IOMMU_ACCESS_WRITE.
>> +     After the memory is used, the memory need set 0 to keep it being protected.
>> +  2) DeviceHandle can be an ACPI device (ISA, I2C, SPI, etc).
>> +     The memory for DMA access need set EDKII_IOMMU_ACCESS_READ and/or EDKII_IOMMU_ACCESS_WRITE.
>> +
>> +  @param[in]  This              The protocol instance pointer.
>> +  @param[in]  DeviceHandle      The device who initiates the DMA access request.
>> +  @param[in]  Mapping           The mapping value returned from Map().
>> +  @param[in]  IoMmuAccess       The IOMMU access.
>> +
>> +  @retval EFI_SUCCESS            The IoMmuAccess is set for the memory range specified by DeviceAddress and Length.
>> +  @retval EFI_INVALID_PARAMETER  DeviceHandle is an invalid handle.
>> +  @retval EFI_INVALID_PARAMETER  Mapping is not a value that was returned by Map().
>> +  @retval EFI_INVALID_PARAMETER  IoMmuAccess specified an illegal combination of access.
>> +  @retval EFI_UNSUPPORTED        DeviceHandle is unknown by the IOMMU.
>> +  @retval EFI_UNSUPPORTED        The bit mask of IoMmuAccess is not supported by the IOMMU.
>> +  @retval EFI_UNSUPPORTED        The IOMMU does not support the memory range specified by Mapping.
>> +  @retval EFI_OUT_OF_RESOURCES   There are not enough resources available to modify the IOMMU access.
>> +  @retval EFI_DEVICE_ERROR       The IOMMU device reported an error while attempting the operation.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +IoMmuSetAttribute (
>> +  IN EDKII_IOMMU_PROTOCOL  *This,
>> +  IN EFI_HANDLE            DeviceHandle,
>> +  IN VOID                  *Mapping,
>> +  IN UINT64                IoMmuAccess
>> +  )
>> +{
>> +  return EFI_UNSUPPORTED;
>> +}
>> +
>> +EDKII_IOMMU_PROTOCOL  mAmdSev = {
>> +  EDKII_IOMMU_PROTOCOL_REVISION,
>> +  IoMmuSetAttribute,
>> +  IoMmuMap,
>> +  IoMmuUnmap,
>> +  IoMmuAllocateBuffer,
>> +  IoMmuFreeBuffer,
>> +};
>> +
>> +/**
>> +  Initialize Iommu Protocol.
>> +
>> +**/
>> +VOID
>> +EFIAPI
>> +AmdSevInstallIommuProtocol (
>> +  VOID
>> +  )
>> +{
>> +  EFI_STATUS  Status;
>> +  EFI_HANDLE  Handle;
>> +
>> +  Handle = NULL;
>> +  Status = gBS->InstallMultipleProtocolInterfaces (
>> +                  &Handle,
>> +                  &gEdkiiIoMmuProtocolGuid, &mAmdSev,
>> +                  NULL
>> +                  );
>> +  ASSERT_EFI_ERROR (Status);
>> +}
>> diff --git a/OvmfPkg/AmdSevDxe/AmdSevMmio.c b/OvmfPkg/AmdSevDxe/AmdSevMmio.c
>> new file mode 100644
>> index 000000000000..b623f82b7baa
>> --- /dev/null
>> +++ b/OvmfPkg/AmdSevDxe/AmdSevMmio.c
>> @@ -0,0 +1,50 @@
>> +/** @file
>> +
>> +  Implements routines to clear C-bit from MMIO Memory Range
>> +
>> +  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
>> +
>> +  This program and the accompanying materials
>> +  are licensed and made available under the terms and conditions of the BSD License
>> +  which accompanies this distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#include "AmdSevMmio.h"
>> +
>> +/**
>> +
>> +  Iterate through the GCD map and clear the C-bit from MMIO and NonExistent
>> +  memory space. The NonExistent memory space will be used for mapping the MMIO
>> +  space added later (eg PciRootBridge). By clearing both known NonExistent
>> +  memory space can gurantee that any MMIO mapped later will have C-bit cleared.
>> +*/
>> +VOID
>> +EFIAPI
>> +AmdSevClearEncMaskMmioRange (
>> +  VOID
>> +  )
>> +{
>> +  EFI_STATUS                       Status;
>> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *AllDescMap;
>> +  UINTN                            NumEntries;
>> +  UINTN                            Index;
>> +
>> +  Status = gDS->GetMemorySpaceMap (&NumEntries, &AllDescMap);
>> +  if (Status == EFI_SUCCESS) {
>> +    for (Index = 0; Index < NumEntries; Index++) {
>> +      CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Desc;
>> +
>> +      Desc = &AllDescMap[Index];
>> +      if (Desc->GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo ||
>> +          Desc->GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
>> +        Status = MemEncryptSevClearPageEncMask (0, Desc->BaseAddress, EFI_SIZE_TO_PAGES(Desc->Length), FALSE);
>> +        ASSERT_EFI_ERROR(Status);
>> +      }
>> +    }
> 
> (4) Right here I think you have a memory leak; gDS->GetMemorySpaceMap()
> allocates AllDescMap dynamically (on success). Please free it with
> FreePool().
> 
> 
> Regarding the IOMMU protocol implementation, I'm going to rely on
> Jiewen's review -- thank you Jiewen very much for that!
> 
> With the above fixed:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks,
> Laszlo
> 
> 
>> +  }
>> +}
>>
>