public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [RFC] [PATCH V3 0/3] Add IOMMU support.
@ 2017-04-04  7:06 Jiewen Yao
  2017-04-04  7:06 ` [RFC] [PATCH V3 1/3] MdeModulePkg/Include: Add IOMMU protocol definition Jiewen Yao
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Jiewen Yao @ 2017-04-04  7:06 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ruiyu Ni, Leo Duran, Brijesh Singh, Ard Biesheuvel

================ V3 ==============
1) Add Remap capability (from Ard Biesheuvel)
Add EDKII_IOMMU_REMAP_ADDRESS API in IOMMU_PROTOCOL.

NOTE: The code is not fully validated yet.
The purpose is to collect feedback to decide the next step.

================ V2 ==============
1) Enhance Unmap() in PciIo (From Ruiyu Ni)
Maintain a local list of MapInfo and match it in Unmap.

2) CopyMem for ReadOperation in PciIo after SetAttribute (Leo Duran)
Fix a bug in V1 that copy mem for read happen before SetAttribute,
which will break AMD SEV solution.

================ V1 ==============

This patch series adds IOMMU protocol and updates the consumer
to support IOMMU based DMA access in UEFI.

This patch series can support the BmDmaLib request for AMD SEV.
submitted by Duran, Leo <leo.duran@amd.com> and Brijesh Singh <brijesh.ksingh@gmail.com>.
https://lists.01.org/pipermail/edk2-devel/2017-March/008109.html, and
https://lists.01.org/pipermail/edk2-devel/2017-March/008820.html.
We can have an AMD SEV specific IOMMU driver to produce IOMMU protocol,
and clear SEV in IOMMU->SetAttribute().

This patch series can also support Intel VTd based DMA protection,
requested by Jiewen Yao <jiewen.yao@intel.com>, discussed in
https://lists.01.org/pipermail/edk2-devel/2017-March/008157.html.
We can have an Intel VTd specific IOMMU driver to produce IOMMU protocol,
and update VTd engine to grant or deny access in IOMMU->SetAttribute().

This patch series does not provide a full Intel VTd driver, which
will be provide in other patch in the future.

The purpose of this patch series to review if this IOMMU protocol design
can meet all DMA access and management requirement.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Leo Duran <leo.duran@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>


Jiewen Yao (3):
  MdeModulePkg/Include: Add IOMMU protocol definition.
  MdeModulePkg/PciHostBridge: Add IOMMU support.
  MdeModulePkg/PciBus: Add IOMMU support.

 MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c                    |  12 +
 MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h                    |  19 ++
 MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf               |   1 +
 MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c                     | 225 ++++++++++++-
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c      |   3 +
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf |   1 +
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h      |   8 +
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 350 ++++++++++++++++++--
 MdeModulePkg/Include/Protocol/IoMmu.h                      | 196 +++++++++++
 MdeModulePkg/MdeModulePkg.dec                              |   3 +
 10 files changed, 772 insertions(+), 46 deletions(-)
 create mode 100644 MdeModulePkg/Include/Protocol/IoMmu.h

-- 
2.7.4.windows.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [RFC] [PATCH V3 1/3] MdeModulePkg/Include: Add IOMMU protocol definition.
  2017-04-04  7:06 [RFC] [PATCH V3 0/3] Add IOMMU support Jiewen Yao
@ 2017-04-04  7:06 ` Jiewen Yao
  2017-04-17 13:42   ` Ard Biesheuvel
  2017-04-04  7:06 ` [RFC] [PATCH V3 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support Jiewen Yao
  2017-04-04  7:06 ` [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: " Jiewen Yao
  2 siblings, 1 reply; 10+ messages in thread
From: Jiewen Yao @ 2017-04-04  7:06 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ruiyu Ni, Leo Duran, Brijesh Singh, Ard Biesheuvel

This protocol is to abstract DMA access from IOMMU.
1) Intel "DMAR" ACPI table.
2) AMD "IVRS" ACPI table
3) ARM "IORT" ACPI table.

There might be multiple IOMMU engines on one platform.
For example, one for graphic and one for rest PCI devices
(such as ATA/USB).
All IOMMU engines are reported by one ACPI table.

All IOMMU protocol provider should be based upon ACPI table.
This single IOMMU protocol can handle multiple IOMMU engines on one system.

This IOMMU protocol provider can use UEFI device path to distinguish
if the device is graphic or ATA/USB, and find out corresponding
IOMMU engine.

The IOMMU protocol provides 2 capabilities:
A) Set DMA access attribute - such as write/read control.
B) Remap DMA memory - such as remap above 4GiB system memory address
to below 4GiB device address.

4) AMD "SEV" feature.
We can have an AMD SEV specific IOMMU driver to produce IOMMU protocol,
and manage SEV bit in SetAttribute(), and return UNSUPPORTED
for SetRemapAddress/GetRemapAddress().

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Leo Duran <leo.duran@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
---
 MdeModulePkg/Include/Protocol/IoMmu.h | 196 ++++++++++++++++++++
 MdeModulePkg/MdeModulePkg.dec         |   3 +
 2 files changed, 199 insertions(+)

diff --git a/MdeModulePkg/Include/Protocol/IoMmu.h b/MdeModulePkg/Include/Protocol/IoMmu.h
new file mode 100644
index 0000000..766b918
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/IoMmu.h
@@ -0,0 +1,196 @@
+/** @file
+  EFI IOMMU Protocol.
+
+Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+This program and the accompanying materials are licensed and made available under
+the terms and conditions of the BSD License that 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 __IOMMU_H__
+#define __IOMMU_H__
+
+//
+// IOMMU Protocol GUID value
+//
+#define EDKII_IOMMU_PROTOCOL_GUID \
+    { \
+      0x4e939de9, 0xd948, 0x4b0f, { 0x88, 0xed, 0xe6, 0xe1, 0xce, 0x51, 0x7c, 0x1e } \
+    }
+
+//
+// Forward reference for pure ANSI compatability
+//
+typedef struct _EDKII_IOMMU_PROTOCOL  EDKII_IOMMU_PROTOCOL;
+
+//
+// Revision The revision to which the IOMMU interface adheres.
+//          All future revisions must be backwards compatible.
+//          If a future version is not back wards compatible it is not the same GUID.
+//
+#define EDKII_IOMMU_PROTOCOL_REVISION 0x00010000
+
+//
+// IOMMU attribute.
+// These types can be "ORed" together as needed.
+// Any undefined bits are reserved and must be zero.
+//
+#define EDKII_IOMMU_ATTRIBUTE_READ   0x1
+#define EDKII_IOMMU_ATTRIBUTE_WRITE  0x2
+
+/**
+  Get IOMMU page size.
+
+  This is the minimal supported page size for a IOMMU.
+  For example, if an IOMMU supports 4KiB, 2MiB and 1GiB,
+  4KiB should be returned.
+
+  @param[in]  This      Protocol instance pointer.
+  @param[out] PageSize  The minimal supported page size for a IOMMU.
+
+  @retval EFI_SUCCESS            The page size is returned.
+  @retval EFI_INVALID_PARAMETER  PageSize is NULL.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EDKII_IOMMU_GET_PAGE_SIZE)(
+  IN  EDKII_IOMMU_PROTOCOL  *This,
+  OUT UINTN                 *PageSize
+  );
+
+/**
+  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_ATTRIBUTE_READ.
+     The memory for BusMasterWrite need set EDKII_IOMMU_ATTRIBUTE_WRITE.
+     The memory for BusMasterCommonBuffer need set EDKII_IOMMU_ATTRIBUTE_READ|EDKII_IOMMU_ATTRIBUTE_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_ATTRIBUTE_READ and/or EDKII_IOMMU_ATTRIBUTE_WRITE.
+
+  @param[in]  This              The protocol instance pointer.
+  @param[in]  DeviceHandle      The device who initiates the DMA access request.
+  @param[in]  DeviceAddress     The base of device memory address to be used as the DMA memory.
+  @param[in]  Length            The length of device memory address to be used as the DMA memory.
+  @param[in]  IoMmuAttribute    The IOMMU attribute.
+
+  @retval EFI_SUCCESS            The IoMmuAttribute is set for the memory range specified by DeviceAddress and Length.
+  @retval EFI_INVALID_PARAMETER  DeviceHandle is an invalid handle.
+  @retval EFI_INVALID_PARAMETER  DeviceAddress is not IoMmu Page size aligned.
+  @retval EFI_INVALID_PARAMETER  Length is not IoMmu Page size aligned.
+  @retval EFI_INVALID_PARAMETER  Length is 0.
+  @retval EFI_INVALID_PARAMETER  IoMmuAttribute specified an illegal combination of attributes.
+  @retval EFI_UNSUPPORTED        DeviceHandle is unknown by the IOMMU.
+  @retval EFI_UNSUPPORTED        The bit mask of IoMmuAttribute is not supported by the IOMMU.
+  @retval EFI_UNSUPPORTED        The IOMMU does not support the memory range specified by DeviceAddress and Length.
+  @retval EFI_OUT_OF_RESOURCES   There are not enough resources available to modify the IOMMU attribute.
+  @retval EFI_DEVICE_ERROR       The IOMMU device reported an error while attempting the operation.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EDKII_IOMMU_SET_ATTRIBUTE)(
+  IN EDKII_IOMMU_PROTOCOL  *This,
+  IN EFI_HANDLE            DeviceHandle,
+  IN EFI_PHYSICAL_ADDRESS  DeviceAddress,
+  IN UINT64                Length,
+  IN UINT64                IoMmuAttribute
+  );
+
+/**
+  Remap a device address to host address.
+
+  For example, this function may support map an above 4GiB host address
+  to a below 4GiB device address.
+
+  @param[in]  This              The protocol instance pointer.
+  @param[in]  DeviceHandle      The device who initiates the DMA access request.
+  @param[in]  DeviceAddress     The base of device memory address to be used as the DMA memory.
+  @param[in]  HostAddress       The base of host memory address to be used as the DMA memory.
+  @param[in]  Length            The length of device memory address to be used as the DMA memory.
+
+  @retval EFI_SUCCESS            The HostAddress is remmapped for the memory range specified by DeviceAddress and Length.
+  @retval EFI_INVALID_PARAMETER  DeviceHandle is not NULL, and it is an invalid handle.
+  @retval EFI_INVALID_PARAMETER  DeviceAddress is not IoMmu Page size aligned.
+  @retval EFI_INVALID_PARAMETER  HostAddress is not IoMmu Page size aligned.
+  @retval EFI_INVALID_PARAMETER  Length is not IoMmu Page size aligned.
+  @retval EFI_INVALID_PARAMETER  Length is 0.
+  @retval EFI_UNSUPPORTED        DeviceHandle is unknown by the IOMMU.
+  @retval EFI_UNSUPPORTED        The remap function is unsupported by the IOMMU.
+  @retval EFI_UNSUPPORTED        The IOMMU does not support the memory range specified by DeviceAddress and Length.
+  @retval EFI_OUT_OF_RESOURCES   There are not enough resources available to modify the IOMMU attribute.
+  @retval EFI_DEVICE_ERROR       The IOMMU device reported an error while attempting the operation.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EDKII_IOMMU_SET_REMAP_ADDRESS)(
+  IN EDKII_IOMMU_PROTOCOL  *This,
+  IN EFI_HANDLE            DeviceHandle, OPTIONAL
+  IN EFI_PHYSICAL_ADDRESS  DeviceAddress,
+  IN VOID                  *HostAddress,
+  IN UINT64                Length
+  );
+
+/**
+  Return a remapped host address from a device address.
+
+  @param[in]  This              The protocol instance pointer.
+  @param[in]  DeviceHandle      The device who initiates the DMA access request.
+  @param[in]  DeviceAddress     The base of device memory address to be used as the DMA memory.
+  @param[out] HostAddress       The base of host memory address to be used as the DMA memory.
+
+  @retval EFI_SUCCESS            The HostAddress remmapped for the memory range specified by DeviceAddress is returned.
+  @retval EFI_INVALID_PARAMETER  DeviceHandle is not NULL, and it is an invalid handle.
+  @retval EFI_INVALID_PARAMETER  DeviceAddress is not IoMmu Page size aligned.
+  @retval EFI_INVALID_PARAMETER  HostAddress is NULL.
+  @retval EFI_UNSUPPORTED        DeviceHandle is unknown by the IOMMU.
+  @retval EFI_UNSUPPORTED        The remap function is unsupported by the IOMMU.
+  @retval EFI_NOT_FOUND          The HostAddress for DeviceAddress is not found.
+  @retval EFI_DEVICE_ERROR       The IOMMU device reported an error while attempting the operation.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EDKII_IOMMU_GET_REMAP_ADDRESS)(
+  IN EDKII_IOMMU_PROTOCOL  *This,
+  IN EFI_HANDLE            DeviceHandle, OPTIONAL
+  IN EFI_PHYSICAL_ADDRESS  DeviceAddress,
+  OUT VOID                 **HostAddress
+  );
+
+///
+/// IOMMU Protocol structure.
+///
+struct _EDKII_IOMMU_PROTOCOL {
+  UINT64                              Revision;
+  EDKII_IOMMU_GET_PAGE_SIZE           GetPageSize;
+  EDKII_IOMMU_SET_ATTRIBUTE           SetAttribute;
+  EDKII_IOMMU_SET_REMAP_ADDRESS       SetRemapAddress;
+  EDKII_IOMMU_GET_REMAP_ADDRESS       GetRemapAddress;
+};
+
+///
+/// IOMMU Protocol GUID variable.
+///
+extern EFI_GUID gEdkiiIoMmuProtocolGuid;
+
+#endif
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 356b3e1..db596b6 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -540,6 +540,9 @@
   ## Include/Protocol/NonDiscoverableDevice.h
   gEdkiiNonDiscoverableDeviceProtocolGuid = { 0x0d51905b, 0xb77e, 0x452a, {0xa2, 0xc0, 0xec, 0xa0, 0xcc, 0x8d, 0x51, 0x4a } }
 
+  ## Include/Protocol/IoMmu.h
+  gEdkiiIoMmuProtocolGuid = { 0x4e939de9, 0xd948, 0x4b0f, { 0x88, 0xed, 0xe6, 0xe1, 0xce, 0x51, 0x7c, 0x1e } }
+
 #
 # [Error.gEfiMdeModulePkgTokenSpaceGuid]
 #   0x80000001 | Invalid value provided.
-- 
2.7.4.windows.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [RFC] [PATCH V3 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.
  2017-04-04  7:06 [RFC] [PATCH V3 0/3] Add IOMMU support Jiewen Yao
  2017-04-04  7:06 ` [RFC] [PATCH V3 1/3] MdeModulePkg/Include: Add IOMMU protocol definition Jiewen Yao
@ 2017-04-04  7:06 ` Jiewen Yao
  2017-04-17 19:33   ` Duran, Leo
  2017-04-04  7:06 ` [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: " Jiewen Yao
  2 siblings, 1 reply; 10+ messages in thread
From: Jiewen Yao @ 2017-04-04  7:06 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ruiyu Ni, Leo Duran, Brijesh Singh, Ard Biesheuvel

The responsibility of PciHostBridge is to allocate IOMMU
page aligned memory for Map and AllocateBuffer,
because PciHostBridge driver already handles Map() request
to allocate another buffer for DMA read/write.

If the max address requirement can not be satisfied,
PciHostBridge may also allocate any IOMMU page aligned memory
and use IOMMU Remapping feature to map to lower address to
satisfy device requirement.

PciHostBridge does not set IOMMU attribute because it does
not know which device request the DMA. This work is done
by PciBus driver.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Leo Duran <leo.duran@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c      |   3 +
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf |   1 +
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h      |   8 +
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 350 ++++++++++++++++++--
 4 files changed, 326 insertions(+), 36 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index 9005dee..35233a7 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -28,6 +28,9 @@ GLOBAL_REMOVE_IF_UNREFERENCED CHAR16 *mPciResourceTypeStr[] = {
   L"I/O", L"Mem", L"PMem", L"Mem64", L"PMem64", L"Bus"
 };
 
+EDKII_IOMMU_PROTOCOL        *gIoMmuProtocol;
+UINTN                       mIoMmuPageSize = 1;
+
 /**
   Ensure the compatibility of an IO space descriptor with the IO aperture.
 
diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
index d8b0439..2d3c8c9 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
@@ -49,6 +49,7 @@
   gEfiDevicePathProtocolGuid                      ## BY_START
   gEfiPciRootBridgeIoProtocolGuid                 ## BY_START
   gEfiPciHostBridgeResourceAllocationProtocolGuid ## BY_START
+  gEdkiiIoMmuProtocolGuid                         ## CONSUMES
 
 [Depex]
   gEfiCpuIo2ProtocolGuid AND
diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
index 13185b4..77c3490 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
@@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Protocol/CpuIo2.h>
 #include <Protocol/DevicePath.h>
 #include <Protocol/PciRootBridgeIo.h>
+#include <Protocol/IoMmu.h>
 #include <Library/DebugLib.h>
 #include <Library/DevicePathLib.h>
 #include <Library/BaseMemoryLib.h>
@@ -50,8 +51,11 @@ typedef struct {
   EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION Operation;
   UINTN                                     NumberOfBytes;
   UINTN                                     NumberOfPages;
+  UINTN                                     MappedNumberOfBytes;
+  UINTN                                     MappedNumberOfPages;
   EFI_PHYSICAL_ADDRESS                      HostAddress;
   EFI_PHYSICAL_ADDRESS                      MappedHostAddress;
+  BOOLEAN                                   RemapNonExisting;
 } MAP_INFO;
 #define MAP_INFO_FROM_LINK(a) CR (a, MAP_INFO, Link, MAP_INFO_SIGNATURE)
 
@@ -575,4 +579,8 @@ RootBridgeIoConfiguration (
 
 extern EFI_METRONOME_ARCH_PROTOCOL *mMetronome;
 extern EFI_CPU_IO2_PROTOCOL         *mCpuIo;
+
+extern EDKII_IOMMU_PROTOCOL        *gIoMmuProtocol;
+extern UINTN                       mIoMmuPageSize;
+
 #endif
diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index 8af131b..2a17eb1 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -1022,6 +1022,121 @@ RootBridgeIoPciWrite (
 }
 
 /**
+  Allocates one or more 4KB pages of a certain memory type at a specified alignment.
+
+  Allocates the number of 4KB pages specified by Pages of a certain memory type with an alignment
+  specified by Alignment.  The allocated buffer is returned.  If Pages is 0, then NULL is returned.
+  If there is not enough memory at the specified alignment remaining to satisfy the request, then
+  NULL is returned.
+  If Alignment is not a power of two and Alignment is not zero, then ASSERT().
+  If Pages plus EFI_SIZE_TO_PAGES (Alignment) overflows, then ASSERT().
+
+  @param  Type                  The type of allocation to perform.
+  @param  MemoryType            The type of memory to allocate.
+  @param  Pages                 The number of 4 KB pages to allocate.
+  @param  Alignment             The requested alignment of the allocation.  Must be a power of two.
+                                If Alignment is zero, then byte alignment is used.
+  @param  Address               Pointer to a physical address.
+
+  @return Memory Allocation Status.
+
+**/
+EFI_STATUS
+InternalAllocateAlignedPagesWithAllocateType (
+  IN EFI_ALLOCATE_TYPE         Type,
+  IN EFI_MEMORY_TYPE           MemoryType,
+  IN UINTN                     Pages,
+  IN UINTN                     Alignment,
+  IN OUT EFI_PHYSICAL_ADDRESS  *Address
+  )
+{
+  EFI_STATUS            Status;
+  EFI_PHYSICAL_ADDRESS  Memory;
+  UINTN                 AlignedMemory;
+  UINTN                 AlignmentMask;
+  UINTN                 UnalignedPages;
+  UINTN                 RealPages;
+
+  //
+  // Alignment must be a power of two or zero.
+  //
+  ASSERT ((Alignment & (Alignment - 1)) == 0);
+
+  if (Pages == 0) {
+    return EFI_INVALID_PARAMETER;
+  }
+  if (Alignment > EFI_PAGE_SIZE) {
+    //
+    // Calculate the total number of pages since alignment is larger than page size.
+    //
+    AlignmentMask  = Alignment - 1;
+    RealPages      = Pages + EFI_SIZE_TO_PAGES (Alignment);
+    //
+    // Make sure that Pages plus EFI_SIZE_TO_PAGES (Alignment) does not overflow.
+    //
+    ASSERT (RealPages > Pages);
+
+    Memory         = *Address;
+    Status         = gBS->AllocatePages (Type, MemoryType, RealPages, &Memory);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+    AlignedMemory  = ((UINTN) Memory + AlignmentMask) & ~AlignmentMask;
+    UnalignedPages = EFI_SIZE_TO_PAGES (AlignedMemory - (UINTN) Memory);
+    if (UnalignedPages > 0) {
+      //
+      // Free first unaligned page(s).
+      //
+      Status = gBS->FreePages (Memory, UnalignedPages);
+      ASSERT_EFI_ERROR (Status);
+    }
+    Memory         = (EFI_PHYSICAL_ADDRESS) (AlignedMemory + EFI_PAGES_TO_SIZE (Pages));
+    UnalignedPages = RealPages - Pages - UnalignedPages;
+    if (UnalignedPages > 0) {
+      //
+      // Free last unaligned page(s).
+      //
+      Status = gBS->FreePages (Memory, UnalignedPages);
+      ASSERT_EFI_ERROR (Status);
+    }
+  } else {
+    //
+    // Do not over-allocate pages in this case.
+    //
+    Memory = *Address;
+    Status = gBS->AllocatePages (Type, MemoryType, Pages, &Memory);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+    AlignedMemory  = (UINTN) Memory;
+  }
+  *Address = AlignedMemory;
+  return EFI_SUCCESS;
+}
+
+/**
+  Return if a value is aligned.
+
+  @param Value the value to be checked
+  @param Alignment the alignment to be checked with.
+
+  @retval TRUE  The value is aligned
+  @retval FALSE The value is not aligned.
+**/
+BOOLEAN
+InternalIsAlgined (
+  IN UINTN  Value,
+  IN UINTN  Alignment
+  )
+{
+  if (Value == ALIGN_VALUE(Value, Alignment)) {
+    return TRUE;
+  } else {
+    return FALSE;
+  }
+}
+
+/**
   Provides the PCI controller-specific address needed to access
   system memory for DMA.
 
@@ -1057,6 +1172,9 @@ RootBridgeIoMap (
   PCI_ROOT_BRIDGE_INSTANCE                          *RootBridge;
   EFI_PHYSICAL_ADDRESS                              PhysicalAddress;
   MAP_INFO                                          *MapInfo;
+  EFI_PHYSICAL_ADDRESS                              MaxAddress;
+  BOOLEAN                                           NeedMap;
+  BOOLEAN                                           NeedAllocateNonExisting;
 
   if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
       Mapping == NULL) {
@@ -1072,13 +1190,41 @@ RootBridgeIoMap (
 
   RootBridge = ROOT_BRIDGE_FROM_THIS (This);
 
+  if (gIoMmuProtocol == NULL) {
+    gBS->LocateProtocol (
+          &gEdkiiIoMmuProtocolGuid,
+          NULL,
+          (VOID **) &gIoMmuProtocol
+          );
+    if (gIoMmuProtocol != NULL) {
+      gIoMmuProtocol->GetPageSize (gIoMmuProtocol, &mIoMmuPageSize);
+      ASSERT ((mIoMmuPageSize & (mIoMmuPageSize - 1)) == 0);
+    }
+  }
+
   PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
+
+  MaxAddress = (EFI_PHYSICAL_ADDRESS)-1;
+  NeedMap = FALSE;
+  NeedAllocateNonExisting = FALSE;
+
   if ((!RootBridge->DmaAbove4G ||
        (Operation != EfiPciOperationBusMasterRead64 &&
         Operation != EfiPciOperationBusMasterWrite64 &&
         Operation != EfiPciOperationBusMasterCommonBuffer64)) &&
       ((PhysicalAddress + *NumberOfBytes) > SIZE_4GB)) {
+    NeedMap = TRUE;
+    MaxAddress = SIZE_4GB - 1;
+  }
 
+  if (gIoMmuProtocol != NULL) {
+    if ((!InternalIsAlgined (*NumberOfBytes, mIoMmuPageSize)) ||
+        (!InternalIsAlgined ((UINTN)HostAddress, mIoMmuPageSize))) {
+      NeedMap = TRUE;
+    }
+  }
+
+  if (NeedMap) {
     //
     // 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
@@ -1090,9 +1236,17 @@ RootBridgeIoMap (
       //
       // 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.
+      // an error if there is no IOMMU.
       //
-      return EFI_UNSUPPORTED;
+      if (gIoMmuProtocol == NULL) {
+        return EFI_UNSUPPORTED;
+      } else {
+        //
+        // We can try to allocate non-existing memory for below 4GiB address
+        // and use IOMMU to remap it.
+        //
+        NeedAllocateNonExisting = TRUE;
+      }
     }
 
     //
@@ -1113,21 +1267,81 @@ RootBridgeIoMap (
     MapInfo->NumberOfBytes     = *NumberOfBytes;
     MapInfo->NumberOfPages     = EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes);
     MapInfo->HostAddress       = PhysicalAddress;
-    MapInfo->MappedHostAddress = SIZE_4GB - 1;
+    MapInfo->MappedHostAddress = MaxAddress;
+    MapInfo->MappedNumberOfBytes = ALIGN_VALUE (MapInfo->NumberOfBytes, mIoMmuPageSize);
+    MapInfo->MappedNumberOfPages = EFI_SIZE_TO_PAGES (MapInfo->MappedNumberOfBytes);
+    MapInfo->RemapNonExisting    = FALSE;
 
-    //
-    // Allocate a buffer below 4GB to map the transfer to.
-    //
-    Status = gBS->AllocatePages (
-                    AllocateMaxAddress,
-                    EfiBootServicesData,
-                    MapInfo->NumberOfPages,
-                    &MapInfo->MappedHostAddress
-                    );
-    if (EFI_ERROR (Status)) {
-      FreePool (MapInfo);
-      *NumberOfBytes = 0;
-      return Status;
+    if (!((Operation == EfiPciOperationBusMasterCommonBuffer) ||
+          (Operation == EfiPciOperationBusMasterCommonBuffer64))) {
+      //
+      // Allocate a buffer below 4GB to map the transfer to.
+      //
+      Status = InternalAllocateAlignedPagesWithAllocateType (
+                      AllocateMaxAddress,
+                      EfiBootServicesData,
+                      MapInfo->MappedNumberOfPages,
+                      mIoMmuPageSize,
+                      &MapInfo->MappedHostAddress
+                      );
+      if (EFI_ERROR (Status)) {
+        if (gIoMmuProtocol == NULL) {
+          FreePool (MapInfo);
+          *NumberOfBytes = 0;
+          return Status;
+        }
+        //
+        // We can try to allocate non-existing memory for below 4GiB address
+        // and use IOMMU to remap it.
+        //
+        NeedAllocateNonExisting = TRUE;
+      }
+    }
+
+    if (NeedAllocateNonExisting) {
+      if ((!InternalIsAlgined (*NumberOfBytes, mIoMmuPageSize)) ||
+          (!InternalIsAlgined ((UINTN)HostAddress, mIoMmuPageSize))) {
+        //
+        // If original memory is not IOMMU aligned, we cannot remap it.
+        //
+        FreePool (MapInfo);
+        *NumberOfBytes = 0;
+        return EFI_UNSUPPORTED;
+      }
+      //
+      // If the code runs to here, it means:
+      // 1) We need remap a common buffer to below 4G non-existing memory.
+      // 2) We need rempa a read/write buffer to below 4G non-existing memory.
+      //
+      MapInfo->MappedHostAddress = MaxAddress;
+      Status = gDS->AllocateMemorySpace (
+                      EfiGcdAllocateMaxAddressSearchTopDown,
+                      EfiGcdMemoryTypeNonExistent,
+                      mIoMmuPageSize,
+                      MapInfo->MappedNumberOfBytes,
+                      &MapInfo->MappedHostAddress,
+                      gImageHandle,
+                      RootBridge->Handle
+                      );
+      if (EFI_ERROR(Status)) {
+        FreePool (MapInfo);
+        *NumberOfBytes = 0;
+        return Status;
+      }
+      Status = gIoMmuProtocol->SetRemapAddress (
+                                 gIoMmuProtocol,
+                                 NULL,
+                                 MapInfo->MappedHostAddress,
+                                 (VOID *)(UINTN)MapInfo->HostAddress,
+                                 MapInfo->MappedNumberOfBytes
+                                 );
+      if (EFI_ERROR(Status)) {
+        gDS->FreeMemorySpace (MapInfo->MappedHostAddress, MapInfo->MappedNumberOfBytes);
+        FreePool (MapInfo);
+        *NumberOfBytes = 0;
+        return Status;
+      }
+      MapInfo->RemapNonExisting = TRUE;
     }
 
     //
@@ -1135,19 +1349,26 @@ RootBridgeIoMap (
     // 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 == EfiPciOperationBusMasterRead ||
-        Operation == EfiPciOperationBusMasterRead64) {
-      CopyMem (
-        (VOID *) (UINTN) MapInfo->MappedHostAddress,
-        (VOID *) (UINTN) MapInfo->HostAddress,
-        MapInfo->NumberOfBytes
-        );
+    // This action can be skipped if there is IOMMU, because PciBus does
+    // the action after setting IOMMU attributes.
+    //
+    if (gIoMmuProtocol == NULL) {
+      if (Operation == EfiPciOperationBusMasterRead ||
+          Operation == EfiPciOperationBusMasterRead64) {
+        CopyMem (
+          (VOID *) (UINTN) MapInfo->MappedHostAddress,
+          (VOID *) (UINTN) MapInfo->HostAddress,
+          MapInfo->NumberOfBytes
+          );
+      }
     }
 
     InsertTailList (&RootBridge->Maps, &MapInfo->Link);
 
     //
     // The DeviceAddress is the address of the maped buffer below 4GB
+    // NOTE: It can be a valid system memory address.
+    //       Or a non-existing memory but mapped by IOMMU.
     //
     *DeviceAddress = MapInfo->MappedHostAddress;
     //
@@ -1228,19 +1449,42 @@ RootBridgeIoUnmap (
   // 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 == EfiPciOperationBusMasterWrite ||
-      MapInfo->Operation == EfiPciOperationBusMasterWrite64) {
-    CopyMem (
-      (VOID *) (UINTN) MapInfo->HostAddress,
-      (VOID *) (UINTN) MapInfo->MappedHostAddress,
-      MapInfo->NumberOfBytes
-      );
+  // This action can be skipped if there is IOMMU, because PciBus does
+  // the action.
+  //
+  if (gIoMmuProtocol == NULL) {
+    if (MapInfo->Operation == EfiPciOperationBusMasterWrite ||
+        MapInfo->Operation == EfiPciOperationBusMasterWrite64) {
+      CopyMem (
+        (VOID *) (UINTN) MapInfo->HostAddress,
+        (VOID *) (UINTN) MapInfo->MappedHostAddress,
+        MapInfo->NumberOfBytes
+        );
+    }
+  }
+
+  if (gIoMmuProtocol != NULL) {
+    //
+    // Free GCD non existing memory.
+    //
+    if (MapInfo->RemapNonExisting) {
+      gIoMmuProtocol->SetRemapAddress (
+                        gIoMmuProtocol,
+                        NULL,
+                        MapInfo->MappedHostAddress,
+                        (VOID *)(UINTN)MapInfo->MappedHostAddress,
+                        MapInfo->MappedNumberOfBytes
+                        );
+      gDS->FreeMemorySpace (MapInfo->MappedHostAddress, MapInfo->MappedNumberOfBytes);
+      FreePool (Mapping);
+      return EFI_SUCCESS;
+    }
   }
 
   //
   // Free the mapped buffer and the MAP_INFO structure.
   //
-  gBS->FreePages (MapInfo->MappedHostAddress, MapInfo->NumberOfPages);
+  gBS->FreePages (MapInfo->MappedHostAddress, MapInfo->MappedNumberOfPages);
   FreePool (Mapping);
   return EFI_SUCCESS;
 }
@@ -1285,7 +1529,7 @@ RootBridgeIoAllocateBuffer (
   EFI_STATUS                Status;
   EFI_PHYSICAL_ADDRESS      PhysicalAddress;
   PCI_ROOT_BRIDGE_INSTANCE  *RootBridge;
-  EFI_ALLOCATE_TYPE         AllocateType;
+  UINTN                     Size;
 
   //
   // Validate Attributes
@@ -1312,25 +1556,52 @@ RootBridgeIoAllocateBuffer (
 
   RootBridge = ROOT_BRIDGE_FROM_THIS (This);
 
-  AllocateType = AllocateAnyPages;
+  PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (-1);
   if (!RootBridge->DmaAbove4G ||
       (Attributes & EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0) {
     //
     // Limit allocations to memory below 4GB
     //
-    AllocateType    = AllocateMaxAddress;
     PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (SIZE_4GB - 1);
   }
-  Status = gBS->AllocatePages (
-                  AllocateType,
+  if (gIoMmuProtocol != NULL) {
+    Size = EFI_PAGES_TO_SIZE(Pages);
+    Size = ALIGN_VALUE(EFI_PAGES_TO_SIZE(Pages), mIoMmuPageSize);
+    Pages = EFI_SIZE_TO_PAGES (Size);
+  }
+  Status = InternalAllocateAlignedPagesWithAllocateType (
+                  AllocateMaxAddress,
                   MemoryType,
                   Pages,
+                  mIoMmuPageSize,
                   &PhysicalAddress
                   );
   if (!EFI_ERROR (Status)) {
     *HostAddress = (VOID *) (UINTN) PhysicalAddress;
+    return EFI_SUCCESS;
   }
 
+  if (gIoMmuProtocol != NULL) {
+    //
+    // Try to allocate AnyAddress here.
+    //
+    // We can try to allocate non-existing memory for below 4GiB address
+    // and use IOMMU to remap it later.
+    //
+    PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (-1);
+    Status = InternalAllocateAlignedPagesWithAllocateType (
+               AllocateMaxAddress,
+               MemoryType,
+               Pages,
+               mIoMmuPageSize,
+               &PhysicalAddress
+               );
+    if (EFI_ERROR(Status)) {
+      return Status;
+    }
+    *HostAddress = (VOID *) (UINTN) PhysicalAddress;
+    return EFI_SUCCESS;
+  }
   return Status;
 }
 
@@ -1356,6 +1627,13 @@ RootBridgeIoFreeBuffer (
   OUT VOID                             *HostAddress
   )
 {
+  UINTN                     Size;
+
+  if (gIoMmuProtocol != NULL) {
+    Size = EFI_PAGES_TO_SIZE(Pages);
+    Size = ALIGN_VALUE(EFI_PAGES_TO_SIZE(Pages), mIoMmuPageSize);
+    Pages = EFI_SIZE_TO_PAGES (Size);
+  }
   return gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress, Pages);
 }
 
-- 
2.7.4.windows.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: Add IOMMU support.
  2017-04-04  7:06 [RFC] [PATCH V3 0/3] Add IOMMU support Jiewen Yao
  2017-04-04  7:06 ` [RFC] [PATCH V3 1/3] MdeModulePkg/Include: Add IOMMU protocol definition Jiewen Yao
  2017-04-04  7:06 ` [RFC] [PATCH V3 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support Jiewen Yao
@ 2017-04-04  7:06 ` Jiewen Yao
  2017-04-17 19:58   ` Duran, Leo
  2 siblings, 1 reply; 10+ messages in thread
From: Jiewen Yao @ 2017-04-04  7:06 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ruiyu Ni, Leo Duran, Brijesh Singh, Ard Biesheuvel

The responsibility of PciBus driver is to set IOMMU attribute,
because only PciBus knows which device submits DMA access request.

PciBus driver guarantee that the request to PciHostBridge is
IOMMU page aligned memory, as such PciHostBridge can allocate
non-existent memory for device memory, to satisfy remap requirement.

PciBus driver does not assume device address is same
as the mapped host address, because IOMMU may remap it.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Leo Duran <leo.duran@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c      |  12 ++
 MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h      |  19 ++
 MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf |   1 +
 MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c       | 225 +++++++++++++++++++-
 4 files changed, 247 insertions(+), 10 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
index f3be47a..c9ee4de 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
@@ -42,6 +42,8 @@ UINT64                                        gAllZero             = 0;
 
 EFI_PCI_PLATFORM_PROTOCOL                     *gPciPlatformProtocol;
 EFI_PCI_OVERRIDE_PROTOCOL                     *gPciOverrideProtocol;
+EDKII_IOMMU_PROTOCOL                          *gIoMmuProtocol;
+UINTN                                         mIoMmuPageSize = 1;
 
 
 GLOBAL_REMOVE_IF_UNREFERENCED EFI_PCI_HOTPLUG_REQUEST_PROTOCOL mPciHotPlugRequest = {
@@ -256,6 +258,16 @@ PciBusDriverBindingStart (
   }
 
   gBS->LocateProtocol (
+        &gEdkiiIoMmuProtocolGuid,
+        NULL,
+        (VOID **) &gIoMmuProtocol
+        );
+  if (gIoMmuProtocol != NULL) {
+    gIoMmuProtocol->GetPageSize (gIoMmuProtocol, &mIoMmuPageSize);
+    ASSERT ((mIoMmuPageSize & (mIoMmuPageSize - 1)) == 0);
+  }
+
+  gBS->LocateProtocol (
          &gEfiIncompatiblePciDeviceSupportProtocolGuid,
          NULL,
          (VOID **) &gIncompatiblePciDeviceSupport
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
index 39ba8b9..185d89c 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
@@ -32,6 +32,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Protocol/IncompatiblePciDeviceSupport.h>
 #include <Protocol/PciOverride.h>
 #include <Protocol/PciEnumerationComplete.h>
+#include <Protocol/IoMmu.h>
 
 #include <Library/DebugLib.h>
 #include <Library/UefiDriverEntryPoint.h>
@@ -289,6 +290,8 @@ struct _PCI_IO_DEVICE {
   // This field is used to support this case.
   //
   UINT16                                    BridgeIoAlignment;
+
+  LIST_ENTRY                                Maps;
 };
 
 #define PCI_IO_DEVICE_FROM_PCI_IO_THIS(a) \
@@ -304,6 +307,20 @@ struct _PCI_IO_DEVICE {
   CR (a, PCI_IO_DEVICE, LoadFile2, PCI_IO_DEVICE_SIGNATURE)
 
 
+#define PCI_IO_MAP_INFO_SIGNATURE  SIGNATURE_32 ('p', 'm', 'a', 'p')
+typedef struct {
+  UINT32                                    Signature;
+  LIST_ENTRY                                Link;
+  EFI_PCI_IO_PROTOCOL_OPERATION             Operation;
+  VOID                                      *HostAddress;
+  EFI_PHYSICAL_ADDRESS                      DeviceAddress;
+  UINTN                                     NumberOfBytes;
+  VOID                                      *AlignedHostAddress;
+  UINTN                                     AlignedNumberOfBytes;
+  VOID                                      *MappedHostAddress;
+  VOID                                      *PciRootBridgeIoMapping;
+} PCI_IO_MAP_INFO;
+#define PCI_IO_MAP_INFO_FROM_LINK(a) CR (a, PCI_IO_MAP_INFO, Link, PCI_IO_MAP_INFO_SIGNATURE)
 
 //
 // Global Variables
@@ -321,6 +338,8 @@ extern EFI_PCI_PLATFORM_PROTOCOL                    *gPciPlatformProtocol;
 extern EFI_PCI_OVERRIDE_PROTOCOL                    *gPciOverrideProtocol;
 extern BOOLEAN                                      mReserveIsaAliases;
 extern BOOLEAN                                      mReserveVgaAliases;
+extern EDKII_IOMMU_PROTOCOL                         *gIoMmuProtocol;
+extern UINTN                                        mIoMmuPageSize;
 
 /**
   Macro that checks whether device is a GFX device.
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
index a3ab11f..5da094f 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
@@ -95,6 +95,7 @@
   gEfiPciRootBridgeIoProtocolGuid                 ## TO_START
   gEfiIncompatiblePciDeviceSupportProtocolGuid    ## SOMETIMES_CONSUMES
   gEfiLoadFile2ProtocolGuid                       ## SOMETIMES_PRODUCES
+  gEdkiiIoMmuProtocolGuid                         ## SOMETIMES_CONSUMES
 
 [FeaturePcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdPciBusHotplugDeviceSupport      ## CONSUMES
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
index f72598d..31b8c32 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
@@ -58,6 +58,7 @@ InitializePciIoInstance (
   )
 {
   CopyMem (&PciIoDevice->PciIo, &mPciIoInterface, sizeof (EFI_PCI_IO_PROTOCOL));
+  InitializeListHead (&PciIoDevice->Maps);
 }
 
 /**
@@ -936,6 +937,28 @@ PciIoCopyMem (
 }
 
 /**
+  Return if a value is aligned.
+
+  @param Value the value to be checked
+  @param Alignment the alignment to be checked with.
+
+  @retval TRUE  The value is aligned
+  @retval FALSE The value is not aligned.
+**/
+BOOLEAN
+InternalIsAlgined (
+  IN UINTN  Value,
+  IN UINTN  Alignment
+  )
+{
+  if (Value == ALIGN_VALUE(Value, Alignment)) {
+    return TRUE;
+  } else {
+    return FALSE;
+  }
+}
+
+/**
   Provides the PCI controller-specific addresses needed to access system memory.
 
   @param  This                  A pointer to the EFI_PCI_IO_PROTOCOL instance.
@@ -967,6 +990,9 @@ PciIoMap (
 {
   EFI_STATUS    Status;
   PCI_IO_DEVICE *PciIoDevice;
+  PCI_IO_MAP_INFO       *PciIoMapInfo;
+  UINT64                IoMmuAttribute;
+  EFI_STATUS            RemapStatus;
 
   PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
 
@@ -982,15 +1008,60 @@ PciIoMap (
     Operation = (EFI_PCI_IO_PROTOCOL_OPERATION) (Operation + EfiPciOperationBusMasterRead64);
   }
 
-  Status = PciIoDevice->PciRootBridgeIo->Map (
-                                          PciIoDevice->PciRootBridgeIo,
-                                          (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION) Operation,
-                                          HostAddress,
-                                          NumberOfBytes,
-                                          DeviceAddress,
-                                          Mapping
-                                          );
-
+  if (gIoMmuProtocol != NULL) {
+    PciIoMapInfo = AllocatePool (sizeof(*PciIoMapInfo));
+    if (PciIoMapInfo == NULL) {
+      return EFI_OUT_OF_RESOURCES;
+    }
+    PciIoMapInfo->Signature              = PCI_IO_MAP_INFO_SIGNATURE;
+    PciIoMapInfo->Operation              = Operation;
+    PciIoMapInfo->NumberOfBytes          = *NumberOfBytes;
+    PciIoMapInfo->DeviceAddress          = *DeviceAddress;
+    PciIoMapInfo->HostAddress            = HostAddress;
+    //
+    // For non common buffer, we always allocate a new memory if IOMMU exists.
+    // because the original memory might not be DMA capable.
+    //
+    // For common buffer, it is not needed, because common buffer allocate via PciIoAllocateBuffer.
+    // We cannot use AllocateAlignedPages here, because there might be more restriction in PciIoAllocateBuffer().
+    //
+    PciIoMapInfo->AlignedNumberOfBytes   = ALIGN_VALUE (PciIoMapInfo->NumberOfBytes, mIoMmuPageSize);
+    if (PciIoMapInfo->Operation != EfiPciIoOperationBusMasterCommonBuffer) {
+      PciIoMapInfo->AlignedHostAddress = AllocateAlignedPages (EFI_SIZE_TO_PAGES(PciIoMapInfo->AlignedNumberOfBytes), mIoMmuPageSize);
+      if (PciIoMapInfo->AlignedHostAddress == NULL) {
+        FreePool (PciIoMapInfo);
+        return EFI_OUT_OF_RESOURCES;
+      }
+    } else {
+      //
+      // For common buffer, the HostAddress must be allocated via PciIoAllocateBuffer.
+      //
+      if (!InternalIsAlgined((UINTN)PciIoMapInfo->HostAddress, mIoMmuPageSize)) {
+        FreePool (PciIoMapInfo);
+        DEBUG ((DEBUG_ERROR, "PciIoMap - map unaligned common buffer with IOMMU\n"));
+        return EFI_UNSUPPORTED;
+      }
+      PciIoMapInfo->AlignedHostAddress = PciIoMapInfo->HostAddress;
+    }
+    PciIoMapInfo->PciRootBridgeIoMapping = NULL;
+    Status = PciIoDevice->PciRootBridgeIo->Map (
+                                            PciIoDevice->PciRootBridgeIo,
+                                            (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION) Operation,
+                                            PciIoMapInfo->AlignedHostAddress,
+                                            &PciIoMapInfo->AlignedNumberOfBytes,
+                                            &PciIoMapInfo->DeviceAddress,
+                                            &PciIoMapInfo->PciRootBridgeIoMapping
+                                            );
+  } else {
+    Status = PciIoDevice->PciRootBridgeIo->Map (
+                                            PciIoDevice->PciRootBridgeIo,
+                                            (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION) Operation,
+                                            HostAddress,
+                                            NumberOfBytes,
+                                            DeviceAddress,
+                                            Mapping
+                                            );
+  }
   if (EFI_ERROR (Status)) {
     REPORT_STATUS_CODE_WITH_DEVICE_PATH (
       EFI_ERROR_CODE | EFI_ERROR_MINOR,
@@ -999,6 +1070,63 @@ PciIoMap (
       );
   }
 
+  if (gIoMmuProtocol != NULL) {
+    if (EFI_ERROR(Status)) {
+      if (PciIoMapInfo->Operation != EfiPciIoOperationBusMasterCommonBuffer) {
+        FreePages (PciIoMapInfo->AlignedHostAddress, EFI_SIZE_TO_PAGES(PciIoMapInfo->AlignedNumberOfBytes));
+      }
+      FreePool (PciIoMapInfo);
+    } else {
+      *DeviceAddress = PciIoMapInfo->DeviceAddress;
+      *Mapping = PciIoMapInfo;
+
+      switch (Operation) {
+      case EfiPciIoOperationBusMasterRead:
+        IoMmuAttribute = EDKII_IOMMU_ATTRIBUTE_READ;
+        break;
+      case EfiPciIoOperationBusMasterWrite:
+        IoMmuAttribute = EDKII_IOMMU_ATTRIBUTE_WRITE;
+        break;
+      case EfiPciIoOperationBusMasterCommonBuffer:
+        IoMmuAttribute = EDKII_IOMMU_ATTRIBUTE_READ | EDKII_IOMMU_ATTRIBUTE_WRITE;
+        break;
+      default:
+        ASSERT(FALSE);
+        return EFI_INVALID_PARAMETER;
+      }
+      //
+      // PciHostBridge should map IOMMU page aligned HostAddress.
+      //
+      gIoMmuProtocol->SetAttribute (
+                        gIoMmuProtocol,
+                        PciIoDevice->Handle,
+                        PciIoMapInfo->DeviceAddress,
+                        PciIoMapInfo->AlignedNumberOfBytes,
+                        IoMmuAttribute
+                        );
+      //
+      // We need do copy mem after IoMmu->SetAttribute(),
+      // because it might change IOMMU state.
+      //
+      RemapStatus = gIoMmuProtocol->GetRemapAddress (
+                                      gIoMmuProtocol,
+                                      PciIoDevice->Handle,
+                                      PciIoMapInfo->DeviceAddress,
+                                      &PciIoMapInfo->MappedHostAddress
+                                      );
+      if (EFI_ERROR(RemapStatus)) {
+        PciIoMapInfo->MappedHostAddress = (VOID *)(UINTN)PciIoMapInfo->DeviceAddress;
+      }
+      if (Operation == EfiPciIoOperationBusMasterRead) {
+        CopyMem (
+          PciIoMapInfo->MappedHostAddress,
+          PciIoMapInfo->HostAddress,
+          PciIoMapInfo->NumberOfBytes
+          );
+      }
+      InsertTailList (&PciIoDevice->Maps, &PciIoMapInfo->Link);
+    }
+  }
   return Status;
 }
 
@@ -1021,9 +1149,48 @@ PciIoUnmap (
 {
   EFI_STATUS    Status;
   PCI_IO_DEVICE *PciIoDevice;
+  PCI_IO_MAP_INFO *PciIoMapInfo;
+  LIST_ENTRY      *Link;
 
   PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
 
+  PciIoMapInfo = NULL;
+  if (gIoMmuProtocol != NULL) {
+    PciIoMapInfo = NULL;
+    for (Link = GetFirstNode (&PciIoDevice->Maps)
+         ; !IsNull (&PciIoDevice->Maps, Link)
+         ; Link = GetNextNode (&PciIoDevice->Maps, Link)
+         ) {
+      PciIoMapInfo = PCI_IO_MAP_INFO_FROM_LINK (Link);
+      if (PciIoMapInfo == Mapping) {
+        break;
+      }
+    }
+    //
+    // Mapping is not a valid value returned by Map()
+    //
+    if (PciIoMapInfo != Mapping) {
+      DEBUG ((DEBUG_INFO, "PciIoUnmap - PciIoMapInfo not found!\n"));
+      return EFI_INVALID_PARAMETER;
+    }
+    RemoveEntryList (&PciIoMapInfo->Link);
+    Mapping = PciIoMapInfo->PciRootBridgeIoMapping;
+
+    //
+    // PciHostBridge should map IOMMU page aligned HostAddress.
+    //
+    // We need do copy mem before PciRootBridgeIo->Unmap(),
+    // because it might free mapped host address.
+    //
+    if (PciIoMapInfo->Operation == EfiPciIoOperationBusMasterWrite) {
+      CopyMem (
+        PciIoMapInfo->HostAddress,
+        PciIoMapInfo->MappedHostAddress,
+        PciIoMapInfo->NumberOfBytes
+        );
+    }
+  }
+
   Status = PciIoDevice->PciRootBridgeIo->Unmap (
                                           PciIoDevice->PciRootBridgeIo,
                                           Mapping
@@ -1037,6 +1204,25 @@ PciIoUnmap (
       );
   }
 
+  if (gIoMmuProtocol != NULL) {
+    if (!EFI_ERROR(Status)) {
+      //
+      // PciHostBridge should map IOMMU page aligned HostAddress.
+      //
+      gIoMmuProtocol->SetAttribute (
+                        gIoMmuProtocol,
+                        PciIoDevice->Handle,
+                        PciIoMapInfo->DeviceAddress,
+                        PciIoMapInfo->AlignedNumberOfBytes,
+                        0
+                        );
+      if (PciIoMapInfo->Operation != EfiPciIoOperationBusMasterCommonBuffer) {
+        FreePages (PciIoMapInfo->AlignedHostAddress, EFI_SIZE_TO_PAGES(PciIoMapInfo->AlignedNumberOfBytes));
+      }
+      FreePool (PciIoMapInfo);
+    }
+  }
+
   return Status;
 }
 
@@ -1073,6 +1259,7 @@ PciIoAllocateBuffer (
 {
   EFI_STATUS    Status;
   PCI_IO_DEVICE *PciIoDevice;
+  UINTN         Size;
 
   if ((Attributes &
       (~(EFI_PCI_ATTRIBUTE_MEMORY_WRITE_COMBINE | EFI_PCI_ATTRIBUTE_MEMORY_CACHED))) != 0){
@@ -1085,6 +1272,12 @@ PciIoAllocateBuffer (
     Attributes |= EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE;
   }
 
+  if (gIoMmuProtocol != NULL) {
+    Size = EFI_PAGES_TO_SIZE(Pages);
+    Size = ALIGN_VALUE(Size, mIoMmuPageSize);
+    Pages = EFI_SIZE_TO_PAGES (Size);
+  }
+
   Status = PciIoDevice->PciRootBridgeIo->AllocateBuffer (
                                           PciIoDevice->PciRootBridgeIo,
                                           Type,
@@ -1101,7 +1294,9 @@ PciIoAllocateBuffer (
       PciIoDevice->DevicePath
       );
   }
-
+  //
+  // No need to set attribute here, it is done in Map.
+  //
   return Status;
 }
 
@@ -1127,9 +1322,16 @@ PciIoFreeBuffer (
 {
   EFI_STATUS    Status;
   PCI_IO_DEVICE *PciIoDevice;
+  UINTN         Size;
 
   PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
 
+  if (gIoMmuProtocol != NULL) {
+    Size = EFI_PAGES_TO_SIZE(Pages);
+    Size = ALIGN_VALUE(Size, mIoMmuPageSize);
+    Pages = EFI_SIZE_TO_PAGES (Size);
+  }
+
   Status = PciIoDevice->PciRootBridgeIo->FreeBuffer (
                                           PciIoDevice->PciRootBridgeIo,
                                           Pages,
@@ -1144,6 +1346,9 @@ PciIoFreeBuffer (
       );
   }
 
+  //
+  // No need to set attribute here, it is done in Unmap.
+  //
   return Status;
 }
 
-- 
2.7.4.windows.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [RFC] [PATCH V3 1/3] MdeModulePkg/Include: Add IOMMU protocol definition.
  2017-04-04  7:06 ` [RFC] [PATCH V3 1/3] MdeModulePkg/Include: Add IOMMU protocol definition Jiewen Yao
@ 2017-04-17 13:42   ` Ard Biesheuvel
  2017-04-17 15:13     ` Yao, Jiewen
  0 siblings, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2017-04-17 13:42 UTC (permalink / raw)
  To: Jiewen Yao; +Cc: edk2-devel@lists.01.org, Ruiyu Ni, Leo Duran, Brijesh Singh

On 4 April 2017 at 08:06, Jiewen Yao <jiewen.yao@intel.com> wrote:
> This protocol is to abstract DMA access from IOMMU.
> 1) Intel "DMAR" ACPI table.
> 2) AMD "IVRS" ACPI table
> 3) ARM "IORT" ACPI table.
>
> There might be multiple IOMMU engines on one platform.
> For example, one for graphic and one for rest PCI devices
> (such as ATA/USB).
> All IOMMU engines are reported by one ACPI table.
>
> All IOMMU protocol provider should be based upon ACPI table.
> This single IOMMU protocol can handle multiple IOMMU engines on one system.
>
> This IOMMU protocol provider can use UEFI device path to distinguish
> if the device is graphic or ATA/USB, and find out corresponding
> IOMMU engine.
>
> The IOMMU protocol provides 2 capabilities:
> A) Set DMA access attribute - such as write/read control.
> B) Remap DMA memory - such as remap above 4GiB system memory address
> to below 4GiB device address.
>
> 4) AMD "SEV" feature.
> We can have an AMD SEV specific IOMMU driver to produce IOMMU protocol,
> and manage SEV bit in SetAttribute(), and return UNSUPPORTED
> for SetRemapAddress/GetRemapAddress().
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Leo Duran <leo.duran@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  MdeModulePkg/Include/Protocol/IoMmu.h | 196 ++++++++++++++++++++
>  MdeModulePkg/MdeModulePkg.dec         |   3 +
>  2 files changed, 199 insertions(+)
>
> diff --git a/MdeModulePkg/Include/Protocol/IoMmu.h b/MdeModulePkg/Include/Protocol/IoMmu.h
> new file mode 100644
> index 0000000..766b918
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/IoMmu.h
> @@ -0,0 +1,196 @@
> +/** @file
> +  EFI IOMMU Protocol.
> +
> +Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +This program and the accompanying materials are licensed and made available under
> +the terms and conditions of the BSD License that 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 __IOMMU_H__
> +#define __IOMMU_H__
> +
> +//
> +// IOMMU Protocol GUID value
> +//
> +#define EDKII_IOMMU_PROTOCOL_GUID \
> +    { \
> +      0x4e939de9, 0xd948, 0x4b0f, { 0x88, 0xed, 0xe6, 0xe1, 0xce, 0x51, 0x7c, 0x1e } \
> +    }
> +
> +//
> +// Forward reference for pure ANSI compatability
> +//
> +typedef struct _EDKII_IOMMU_PROTOCOL  EDKII_IOMMU_PROTOCOL;
> +
> +//
> +// Revision The revision to which the IOMMU interface adheres.
> +//          All future revisions must be backwards compatible.
> +//          If a future version is not back wards compatible it is not the same GUID.
> +//
> +#define EDKII_IOMMU_PROTOCOL_REVISION 0x00010000
> +
> +//
> +// IOMMU attribute.
> +// These types can be "ORed" together as needed.
> +// Any undefined bits are reserved and must be zero.
> +//
> +#define EDKII_IOMMU_ATTRIBUTE_READ   0x1
> +#define EDKII_IOMMU_ATTRIBUTE_WRITE  0x2
> +
> +/**
> +  Get IOMMU page size.
> +
> +  This is the minimal supported page size for a IOMMU.
> +  For example, if an IOMMU supports 4KiB, 2MiB and 1GiB,
> +  4KiB should be returned.
> +
> +  @param[in]  This      Protocol instance pointer.
> +  @param[out] PageSize  The minimal supported page size for a IOMMU.
> +
> +  @retval EFI_SUCCESS            The page size is returned.
> +  @retval EFI_INVALID_PARAMETER  PageSize is NULL.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_IOMMU_GET_PAGE_SIZE)(
> +  IN  EDKII_IOMMU_PROTOCOL  *This,
> +  OUT UINTN                 *PageSize
> +  );
> +

When do we expect to need this? Is it common on x86 to have IOMMUs
that don't support 4 KB page size?

> +/**
> +  Set IOMMU attribute for a system memory.
> +
> +  If the IOMMU protocol exists, the system memory cannot be used
> +  for DMA by default.
> +

I suppose this is required for encryption support, but isn't it overly
restrictive in the general case?
On ARM systems, many drivers exist that are not PCI based but do
perform DMA. (i.e., DmaLib)

> +  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_ATTRIBUTE_READ.
> +     The memory for BusMasterWrite need set EDKII_IOMMU_ATTRIBUTE_WRITE.
> +     The memory for BusMasterCommonBuffer need set EDKII_IOMMU_ATTRIBUTE_READ|EDKII_IOMMU_ATTRIBUTE_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_ATTRIBUTE_READ and/or EDKII_IOMMU_ATTRIBUTE_WRITE.
> +
> +  @param[in]  This              The protocol instance pointer.
> +  @param[in]  DeviceHandle      The device who initiates the DMA access request.
> +  @param[in]  DeviceAddress     The base of device memory address to be used as the DMA memory.
> +  @param[in]  Length            The length of device memory address to be used as the DMA memory.
> +  @param[in]  IoMmuAttribute    The IOMMU attribute.
> +
> +  @retval EFI_SUCCESS            The IoMmuAttribute is set for the memory range specified by DeviceAddress and Length.
> +  @retval EFI_INVALID_PARAMETER  DeviceHandle is an invalid handle.
> +  @retval EFI_INVALID_PARAMETER  DeviceAddress is not IoMmu Page size aligned.
> +  @retval EFI_INVALID_PARAMETER  Length is not IoMmu Page size aligned.
> +  @retval EFI_INVALID_PARAMETER  Length is 0.
> +  @retval EFI_INVALID_PARAMETER  IoMmuAttribute specified an illegal combination of attributes.
> +  @retval EFI_UNSUPPORTED        DeviceHandle is unknown by the IOMMU.
> +  @retval EFI_UNSUPPORTED        The bit mask of IoMmuAttribute is not supported by the IOMMU.
> +  @retval EFI_UNSUPPORTED        The IOMMU does not support the memory range specified by DeviceAddress and Length.
> +  @retval EFI_OUT_OF_RESOURCES   There are not enough resources available to modify the IOMMU attribute.
> +  @retval EFI_DEVICE_ERROR       The IOMMU device reported an error while attempting the operation.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_IOMMU_SET_ATTRIBUTE)(
> +  IN EDKII_IOMMU_PROTOCOL  *This,
> +  IN EFI_HANDLE            DeviceHandle,
> +  IN EFI_PHYSICAL_ADDRESS  DeviceAddress,
> +  IN UINT64                Length,
> +  IN UINT64                IoMmuAttribute
> +  );
> +
> +/**
> +  Remap a device address to host address.
> +
> +  For example, this function may support map an above 4GiB host address
> +  to a below 4GiB device address.
> +
> +  @param[in]  This              The protocol instance pointer.
> +  @param[in]  DeviceHandle      The device who initiates the DMA access request.
> +  @param[in]  DeviceAddress     The base of device memory address to be used as the DMA memory.
> +  @param[in]  HostAddress       The base of host memory address to be used as the DMA memory.
> +  @param[in]  Length            The length of device memory address to be used as the DMA memory.
> +
> +  @retval EFI_SUCCESS            The HostAddress is remmapped for the memory range specified by DeviceAddress and Length.
> +  @retval EFI_INVALID_PARAMETER  DeviceHandle is not NULL, and it is an invalid handle.
> +  @retval EFI_INVALID_PARAMETER  DeviceAddress is not IoMmu Page size aligned.
> +  @retval EFI_INVALID_PARAMETER  HostAddress is not IoMmu Page size aligned.
> +  @retval EFI_INVALID_PARAMETER  Length is not IoMmu Page size aligned.
> +  @retval EFI_INVALID_PARAMETER  Length is 0.
> +  @retval EFI_UNSUPPORTED        DeviceHandle is unknown by the IOMMU.
> +  @retval EFI_UNSUPPORTED        The remap function is unsupported by the IOMMU.
> +  @retval EFI_UNSUPPORTED        The IOMMU does not support the memory range specified by DeviceAddress and Length.
> +  @retval EFI_OUT_OF_RESOURCES   There are not enough resources available to modify the IOMMU attribute.
> +  @retval EFI_DEVICE_ERROR       The IOMMU device reported an error while attempting the operation.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_IOMMU_SET_REMAP_ADDRESS)(
> +  IN EDKII_IOMMU_PROTOCOL  *This,
> +  IN EFI_HANDLE            DeviceHandle, OPTIONAL
> +  IN EFI_PHYSICAL_ADDRESS  DeviceAddress,
> +  IN VOID                  *HostAddress,
> +  IN UINT64                Length
> +  );
> +
> +/**
> +  Return a remapped host address from a device address.
> +
> +  @param[in]  This              The protocol instance pointer.
> +  @param[in]  DeviceHandle      The device who initiates the DMA access request.
> +  @param[in]  DeviceAddress     The base of device memory address to be used as the DMA memory.
> +  @param[out] HostAddress       The base of host memory address to be used as the DMA memory.
> +
> +  @retval EFI_SUCCESS            The HostAddress remmapped for the memory range specified by DeviceAddress is returned.
> +  @retval EFI_INVALID_PARAMETER  DeviceHandle is not NULL, and it is an invalid handle.
> +  @retval EFI_INVALID_PARAMETER  DeviceAddress is not IoMmu Page size aligned.
> +  @retval EFI_INVALID_PARAMETER  HostAddress is NULL.
> +  @retval EFI_UNSUPPORTED        DeviceHandle is unknown by the IOMMU.
> +  @retval EFI_UNSUPPORTED        The remap function is unsupported by the IOMMU.
> +  @retval EFI_NOT_FOUND          The HostAddress for DeviceAddress is not found.
> +  @retval EFI_DEVICE_ERROR       The IOMMU device reported an error while attempting the operation.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_IOMMU_GET_REMAP_ADDRESS)(
> +  IN EDKII_IOMMU_PROTOCOL  *This,
> +  IN EFI_HANDLE            DeviceHandle, OPTIONAL
> +  IN EFI_PHYSICAL_ADDRESS  DeviceAddress,
> +  OUT VOID                 **HostAddress
> +  );
> +
> +///
> +/// IOMMU Protocol structure.
> +///
> +struct _EDKII_IOMMU_PROTOCOL {
> +  UINT64                              Revision;
> +  EDKII_IOMMU_GET_PAGE_SIZE           GetPageSize;
> +  EDKII_IOMMU_SET_ATTRIBUTE           SetAttribute;
> +  EDKII_IOMMU_SET_REMAP_ADDRESS       SetRemapAddress;
> +  EDKII_IOMMU_GET_REMAP_ADDRESS       GetRemapAddress;
> +};
> +
> +///
> +/// IOMMU Protocol GUID variable.
> +///
> +extern EFI_GUID gEdkiiIoMmuProtocolGuid;
> +
> +#endif
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 356b3e1..db596b6 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -540,6 +540,9 @@
>    ## Include/Protocol/NonDiscoverableDevice.h
>    gEdkiiNonDiscoverableDeviceProtocolGuid = { 0x0d51905b, 0xb77e, 0x452a, {0xa2, 0xc0, 0xec, 0xa0, 0xcc, 0x8d, 0x51, 0x4a } }
>
> +  ## Include/Protocol/IoMmu.h
> +  gEdkiiIoMmuProtocolGuid = { 0x4e939de9, 0xd948, 0x4b0f, { 0x88, 0xed, 0xe6, 0xe1, 0xce, 0x51, 0x7c, 0x1e } }
> +
>  #
>  # [Error.gEfiMdeModulePkgTokenSpaceGuid]
>  #   0x80000001 | Invalid value provided.

I see how this protocol intends to cater for a lot of different use
cases, but the one that will be the most common on 64-bit ARM is a
simple static translation between the start of DRAM (which may be > 4
GB) and PCI address range [ 0x0, 4 GB ), and a 1:1 translation for all
other masters.

This may require bounce buffering for non-coherent mappings, but it
can be achieved with a very simple driver. I implemented a proof of
concept based on Leo's BmDmaLib here
https://github.com/ardbiesheuvel/OpenPlatformPkg/commit/dc251f0bf7a85205fb3590ccc9788b072a2bdcb8

Implementing IOMMU protocol as specified here, with remap() entry
points, forces me to manage the IOMMU page tables dynamically, which
is a level of complexity that is really not needed for my use case,
especially given the required TLB maintenance and other concerns that
don't really exist for a static mapping.

Also, could you explain why we need to change the root bridge *and*
the bus driver?


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] [PATCH V3 1/3] MdeModulePkg/Include: Add IOMMU protocol definition.
  2017-04-17 13:42   ` Ard Biesheuvel
@ 2017-04-17 15:13     ` Yao, Jiewen
  0 siblings, 0 replies; 10+ messages in thread
From: Yao, Jiewen @ 2017-04-17 15:13 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: edk2-devel@lists.01.org, Ni, Ruiyu, Leo Duran, Brijesh Singh

Hi Ard
Thanks for the feedback.

Detailed comment below:


From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
Sent: Monday, April 17, 2017 9:43 PM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel@lists.01.org; Ni, Ruiyu <ruiyu.ni@intel.com>; Leo Duran <leo.duran@amd.com>; Brijesh Singh <brijesh.singh@amd.com>
Subject: Re: [RFC] [PATCH V3 1/3] MdeModulePkg/Include: Add IOMMU protocol definition.

On 4 April 2017 at 08:06, Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
> This protocol is to abstract DMA access from IOMMU.
> 1) Intel "DMAR" ACPI table.
> 2) AMD "IVRS" ACPI table
> 3) ARM "IORT" ACPI table.
>
> There might be multiple IOMMU engines on one platform.
> For example, one for graphic and one for rest PCI devices
> (such as ATA/USB).
> All IOMMU engines are reported by one ACPI table.
>
> All IOMMU protocol provider should be based upon ACPI table.
> This single IOMMU protocol can handle multiple IOMMU engines on one system.
>
> This IOMMU protocol provider can use UEFI device path to distinguish
> if the device is graphic or ATA/USB, and find out corresponding
> IOMMU engine.
>
> The IOMMU protocol provides 2 capabilities:
> A) Set DMA access attribute - such as write/read control.
> B) Remap DMA memory - such as remap above 4GiB system memory address
> to below 4GiB device address.
>
> 4) AMD "SEV" feature.
> We can have an AMD SEV specific IOMMU driver to produce IOMMU protocol,
> and manage SEV bit in SetAttribute(), and return UNSUPPORTED
> for SetRemapAddress/GetRemapAddress().
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>
> Cc: Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>
> Cc: Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> ---
>  MdeModulePkg/Include/Protocol/IoMmu.h | 196 ++++++++++++++++++++
>  MdeModulePkg/MdeModulePkg.dec         |   3 +
>  2 files changed, 199 insertions(+)
>
> diff --git a/MdeModulePkg/Include/Protocol/IoMmu.h b/MdeModulePkg/Include/Protocol/IoMmu.h
> new file mode 100644
> index 0000000..766b918
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/IoMmu.h
> @@ -0,0 +1,196 @@
> +/** @file
> +  EFI IOMMU Protocol.
> +
> +Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +This program and the accompanying materials are licensed and made available under
> +the terms and conditions of the BSD License that 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 __IOMMU_H__
> +#define __IOMMU_H__
> +
> +//
> +// IOMMU Protocol GUID value
> +//
> +#define EDKII_IOMMU_PROTOCOL_GUID \
> +    { \
> +      0x4e939de9, 0xd948, 0x4b0f, { 0x88, 0xed, 0xe6, 0xe1, 0xce, 0x51, 0x7c, 0x1e } \
> +    }
> +
> +//
> +// Forward reference for pure ANSI compatability
> +//
> +typedef struct _EDKII_IOMMU_PROTOCOL  EDKII_IOMMU_PROTOCOL;
> +
> +//
> +// Revision The revision to which the IOMMU interface adheres.
> +//          All future revisions must be backwards compatible.
> +//          If a future version is not back wards compatible it is not the same GUID.
> +//
> +#define EDKII_IOMMU_PROTOCOL_REVISION 0x00010000
> +
> +//
> +// IOMMU attribute.
> +// These types can be "ORed" together as needed.
> +// Any undefined bits are reserved and must be zero.
> +//
> +#define EDKII_IOMMU_ATTRIBUTE_READ   0x1
> +#define EDKII_IOMMU_ATTRIBUTE_WRITE  0x2
> +
> +/**
> +  Get IOMMU page size.
> +
> +  This is the minimal supported page size for a IOMMU.
> +  For example, if an IOMMU supports 4KiB, 2MiB and 1GiB,
> +  4KiB should be returned.
> +
> +  @param[in]  This      Protocol instance pointer.
> +  @param[out] PageSize  The minimal supported page size for a IOMMU.
> +
> +  @retval EFI_SUCCESS            The page size is returned.
> +  @retval EFI_INVALID_PARAMETER  PageSize is NULL.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_IOMMU_GET_PAGE_SIZE)(
> +  IN  EDKII_IOMMU_PROTOCOL  *This,
> +  OUT UINTN                 *PageSize
> +  );
> +

When do we expect to need this? Is it common on x86 to have IOMMUs
that don't support 4 KB page size?
[Jiewen] Yes. X86 IOMMU does support 4KB.
I am not knowledgeable enough to know all other architectures. So this is a generic design.
Of course, if we can confirm all arch support 4KB IOMMU, this API can be removed. ☺
Ard, can you help to double confirm for ARM/AArch64?


> +/**
> +  Set IOMMU attribute for a system memory.
> +
> +  If the IOMMU protocol exists, the system memory cannot be used
> +  for DMA by default.
> +

I suppose this is required for encryption support, but isn't it overly
restrictive in the general case?
On ARM systems, many drivers exist that are not PCI based but do
perform DMA. (i.e., DmaLib)
[Jiewen] This is not for encryption support.
This is to prevent DMA attack for security concern. When IOMMU driver launches, it should set all memory to be not DMA capable. Then only requested DMA memory is open for the device.

In your below case, you just want to remap above 4GiB to below 4GiB. It is not for security.
Maybe we can use weak term – the system memory *might not* 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_ATTRIBUTE_READ.
> +     The memory for BusMasterWrite need set EDKII_IOMMU_ATTRIBUTE_WRITE.
> +     The memory for BusMasterCommonBuffer need set EDKII_IOMMU_ATTRIBUTE_READ|EDKII_IOMMU_ATTRIBUTE_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_ATTRIBUTE_READ and/or EDKII_IOMMU_ATTRIBUTE_WRITE.
> +
> +  @param[in]  This              The protocol instance pointer.
> +  @param[in]  DeviceHandle      The device who initiates the DMA access request.
> +  @param[in]  DeviceAddress     The base of device memory address to be used as the DMA memory.
> +  @param[in]  Length            The length of device memory address to be used as the DMA memory.
> +  @param[in]  IoMmuAttribute    The IOMMU attribute.
> +
> +  @retval EFI_SUCCESS            The IoMmuAttribute is set for the memory range specified by DeviceAddress and Length.
> +  @retval EFI_INVALID_PARAMETER  DeviceHandle is an invalid handle.
> +  @retval EFI_INVALID_PARAMETER  DeviceAddress is not IoMmu Page size aligned.
> +  @retval EFI_INVALID_PARAMETER  Length is not IoMmu Page size aligned.
> +  @retval EFI_INVALID_PARAMETER  Length is 0.
> +  @retval EFI_INVALID_PARAMETER  IoMmuAttribute specified an illegal combination of attributes.
> +  @retval EFI_UNSUPPORTED        DeviceHandle is unknown by the IOMMU.
> +  @retval EFI_UNSUPPORTED        The bit mask of IoMmuAttribute is not supported by the IOMMU.
> +  @retval EFI_UNSUPPORTED        The IOMMU does not support the memory range specified by DeviceAddress and Length.
> +  @retval EFI_OUT_OF_RESOURCES   There are not enough resources available to modify the IOMMU attribute.
> +  @retval EFI_DEVICE_ERROR       The IOMMU device reported an error while attempting the operation.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_IOMMU_SET_ATTRIBUTE)(
> +  IN EDKII_IOMMU_PROTOCOL  *This,
> +  IN EFI_HANDLE            DeviceHandle,
> +  IN EFI_PHYSICAL_ADDRESS  DeviceAddress,
> +  IN UINT64                Length,
> +  IN UINT64                IoMmuAttribute
> +  );
> +
> +/**
> +  Remap a device address to host address.
> +
> +  For example, this function may support map an above 4GiB host address
> +  to a below 4GiB device address.
> +
> +  @param[in]  This              The protocol instance pointer.
> +  @param[in]  DeviceHandle      The device who initiates the DMA access request.
> +  @param[in]  DeviceAddress     The base of device memory address to be used as the DMA memory.
> +  @param[in]  HostAddress       The base of host memory address to be used as the DMA memory.
> +  @param[in]  Length            The length of device memory address to be used as the DMA memory.
> +
> +  @retval EFI_SUCCESS            The HostAddress is remmapped for the memory range specified by DeviceAddress and Length.
> +  @retval EFI_INVALID_PARAMETER  DeviceHandle is not NULL, and it is an invalid handle.
> +  @retval EFI_INVALID_PARAMETER  DeviceAddress is not IoMmu Page size aligned.
> +  @retval EFI_INVALID_PARAMETER  HostAddress is not IoMmu Page size aligned.
> +  @retval EFI_INVALID_PARAMETER  Length is not IoMmu Page size aligned.
> +  @retval EFI_INVALID_PARAMETER  Length is 0.
> +  @retval EFI_UNSUPPORTED        DeviceHandle is unknown by the IOMMU.
> +  @retval EFI_UNSUPPORTED        The remap function is unsupported by the IOMMU.
> +  @retval EFI_UNSUPPORTED        The IOMMU does not support the memory range specified by DeviceAddress and Length.
> +  @retval EFI_OUT_OF_RESOURCES   There are not enough resources available to modify the IOMMU attribute.
> +  @retval EFI_DEVICE_ERROR       The IOMMU device reported an error while attempting the operation.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_IOMMU_SET_REMAP_ADDRESS)(
> +  IN EDKII_IOMMU_PROTOCOL  *This,
> +  IN EFI_HANDLE            DeviceHandle, OPTIONAL
> +  IN EFI_PHYSICAL_ADDRESS  DeviceAddress,
> +  IN VOID                  *HostAddress,
> +  IN UINT64                Length
> +  );
> +
> +/**
> +  Return a remapped host address from a device address.
> +
> +  @param[in]  This              The protocol instance pointer.
> +  @param[in]  DeviceHandle      The device who initiates the DMA access request.
> +  @param[in]  DeviceAddress     The base of device memory address to be used as the DMA memory.
> +  @param[out] HostAddress       The base of host memory address to be used as the DMA memory.
> +
> +  @retval EFI_SUCCESS            The HostAddress remmapped for the memory range specified by DeviceAddress is returned.
> +  @retval EFI_INVALID_PARAMETER  DeviceHandle is not NULL, and it is an invalid handle.
> +  @retval EFI_INVALID_PARAMETER  DeviceAddress is not IoMmu Page size aligned.
> +  @retval EFI_INVALID_PARAMETER  HostAddress is NULL.
> +  @retval EFI_UNSUPPORTED        DeviceHandle is unknown by the IOMMU.
> +  @retval EFI_UNSUPPORTED        The remap function is unsupported by the IOMMU.
> +  @retval EFI_NOT_FOUND          The HostAddress for DeviceAddress is not found.
> +  @retval EFI_DEVICE_ERROR       The IOMMU device reported an error while attempting the operation.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EDKII_IOMMU_GET_REMAP_ADDRESS)(
> +  IN EDKII_IOMMU_PROTOCOL  *This,
> +  IN EFI_HANDLE            DeviceHandle, OPTIONAL
> +  IN EFI_PHYSICAL_ADDRESS  DeviceAddress,
> +  OUT VOID                 **HostAddress
> +  );
> +
> +///
> +/// IOMMU Protocol structure.
> +///
> +struct _EDKII_IOMMU_PROTOCOL {
> +  UINT64                              Revision;
> +  EDKII_IOMMU_GET_PAGE_SIZE           GetPageSize;
> +  EDKII_IOMMU_SET_ATTRIBUTE           SetAttribute;
> +  EDKII_IOMMU_SET_REMAP_ADDRESS       SetRemapAddress;
> +  EDKII_IOMMU_GET_REMAP_ADDRESS       GetRemapAddress;
> +};
> +
> +///
> +/// IOMMU Protocol GUID variable.
> +///
> +extern EFI_GUID gEdkiiIoMmuProtocolGuid;
> +
> +#endif
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 356b3e1..db596b6 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -540,6 +540,9 @@
>    ## Include/Protocol/NonDiscoverableDevice.h
>    gEdkiiNonDiscoverableDeviceProtocolGuid = { 0x0d51905b, 0xb77e, 0x452a, {0xa2, 0xc0, 0xec, 0xa0, 0xcc, 0x8d, 0x51, 0x4a } }
>
> +  ## Include/Protocol/IoMmu.h
> +  gEdkiiIoMmuProtocolGuid = { 0x4e939de9, 0xd948, 0x4b0f, { 0x88, 0xed, 0xe6, 0xe1, 0xce, 0x51, 0x7c, 0x1e } }
> +
>  #
>  # [Error.gEfiMdeModulePkgTokenSpaceGuid]
>  #   0x80000001 | Invalid value provided.

I see how this protocol intends to cater for a lot of different use
cases, but the one that will be the most common on 64-bit ARM is a
simple static translation between the start of DRAM (which may be > 4
GB) and PCI address range [ 0x0, 4 GB ), and a 1:1 translation for all
other masters.

This may require bounce buffering for non-coherent mappings, but it
can be achieved with a very simple driver. I implemented a proof of
concept based on Leo's BmDmaLib here
https://github.com/ardbiesheuvel/OpenPlatformPkg/commit/dc251f0bf7a85205fb3590ccc9788b072a2bdcb8

Implementing IOMMU protocol as specified here, with remap() entry
points, forces me to manage the IOMMU page tables dynamically, which
is a level of complexity that is really not needed for my use case,
especially given the required TLB maintenance and other concerns that
don't really exist for a static mapping.
[Jiewen] I agree. In your case, it should be very simple.

You can configure a static IOMMU table at the driver enterpoint, but there is no need to update it.

Very good feedback. Let me think about it.
Maybe an IOMMU->GetRemapAddress() in the beginning can make it simple.

If I have new version patch, I will provide IOMMU sample code for your 4G-remap and Leo’s SEV case.


Also, could you explain why we need to change the root bridge *and*
the bus driver?

[Jiewen] Very good question.
In my case, I need to know which PCI device submits DMA request, so that I can configure IOMMU for *this PCI device* only. I am not able to do that in PciHostBridge driver, because the PCI device information is lost.
*That is the biggest reason that I am not able to use Leo’s BmDmaLib directly.* I must update PciBus driver.

PciBus driver can do remap action by itself to make requested memory to be IOMMU page aligned. But that is also not enough.

1)       PciHostBridge driver may need do remap with IOMMU page alignment due to 4GiB issue.

2)       PciHostBridge driver CopyMem is useless, because CopyMem should happen after SetAttribute to clear SEV bit.

If we can assume 4KB IOMMU, if double copy is not a problem, maybe we can eliminate PciHostBridge update.

Thank you
Yao Jiewen



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] [PATCH V3 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.
  2017-04-04  7:06 ` [RFC] [PATCH V3 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support Jiewen Yao
@ 2017-04-17 19:33   ` Duran, Leo
  2017-04-18  3:31     ` Yao, Jiewen
  0 siblings, 1 reply; 10+ messages in thread
From: Duran, Leo @ 2017-04-17 19:33 UTC (permalink / raw)
  To: 'Jiewen Yao', edk2-devel@lists.01.org
  Cc: Ruiyu Ni, Singh, Brijesh, Ard Biesheuvel

Hi Yao,

Just a couple of quick comments:

1) gIoMmuProtocol is declared (global) by PciHostBridge.c, but it is not initialized to NULL.

2) Would it be OK to do a one-time LocateProtocol() of gIoMmuProtocol  in InitializePciHostBridge()?
BTW, besides the global declaration, gIoMmuProtocol  is currently not used by PciHostBridge.c 

Leo

> -----Original Message-----
> From: Jiewen Yao [mailto:jiewen.yao@intel.com]
> Sent: Tuesday, April 04, 2017 2:06 AM
> To: edk2-devel@lists.01.org
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>; Duran, Leo <leo.duran@amd.com>;
> Singh, Brijesh <brijesh.singh@amd.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: [RFC] [PATCH V3 2/3] MdeModulePkg/PciHostBridge: Add IOMMU
> support.
> 
> The responsibility of PciHostBridge is to allocate IOMMU page aligned
> memory for Map and AllocateBuffer, because PciHostBridge driver already
> handles Map() request to allocate another buffer for DMA read/write.
> 
> If the max address requirement can not be satisfied, PciHostBridge may also
> allocate any IOMMU page aligned memory and use IOMMU Remapping
> feature to map to lower address to satisfy device requirement.
> 
> PciHostBridge does not set IOMMU attribute because it does not know
> which device request the DMA. This work is done by PciBus driver.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Leo Duran <leo.duran@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c      |   3 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf |   1 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h      |   8 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 350
> ++++++++++++++++++--
>  4 files changed, 326 insertions(+), 36 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> index 9005dee..35233a7 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> @@ -28,6 +28,9 @@ GLOBAL_REMOVE_IF_UNREFERENCED CHAR16
> *mPciResourceTypeStr[] = {
>    L"I/O", L"Mem", L"PMem", L"Mem64", L"PMem64", L"Bus"
>  };
> 
> +EDKII_IOMMU_PROTOCOL        *gIoMmuProtocol;
> +UINTN                       mIoMmuPageSize = 1;
> +
>  /**
>    Ensure the compatibility of an IO space descriptor with the IO aperture.
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> index d8b0439..2d3c8c9 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> @@ -49,6 +49,7 @@
>    gEfiDevicePathProtocolGuid                      ## BY_START
>    gEfiPciRootBridgeIoProtocolGuid                 ## BY_START
>    gEfiPciHostBridgeResourceAllocationProtocolGuid ## BY_START
> +  gEdkiiIoMmuProtocolGuid                         ## CONSUMES
> 
>  [Depex]
>    gEfiCpuIo2ProtocolGuid AND
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> index 13185b4..77c3490 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> @@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Protocol/CpuIo2.h>
>  #include <Protocol/DevicePath.h>
>  #include <Protocol/PciRootBridgeIo.h>
> +#include <Protocol/IoMmu.h>
>  #include <Library/DebugLib.h>
>  #include <Library/DevicePathLib.h>
>  #include <Library/BaseMemoryLib.h>
> @@ -50,8 +51,11 @@ typedef struct {
>    EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION Operation;
>    UINTN                                     NumberOfBytes;
>    UINTN                                     NumberOfPages;
> +  UINTN                                     MappedNumberOfBytes;
> +  UINTN                                     MappedNumberOfPages;
>    EFI_PHYSICAL_ADDRESS                      HostAddress;
>    EFI_PHYSICAL_ADDRESS                      MappedHostAddress;
> +  BOOLEAN                                   RemapNonExisting;
>  } MAP_INFO;
>  #define MAP_INFO_FROM_LINK(a) CR (a, MAP_INFO, Link,
> MAP_INFO_SIGNATURE)
> 
> @@ -575,4 +579,8 @@ RootBridgeIoConfiguration (
> 
>  extern EFI_METRONOME_ARCH_PROTOCOL *mMetronome;
>  extern EFI_CPU_IO2_PROTOCOL         *mCpuIo;
> +
> +extern EDKII_IOMMU_PROTOCOL        *gIoMmuProtocol;
> +extern UINTN                       mIoMmuPageSize;
> +
>  #endif
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index 8af131b..2a17eb1 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -1022,6 +1022,121 @@ RootBridgeIoPciWrite (  }
> 
>  /**
> +  Allocates one or more 4KB pages of a certain memory type at a specified
> alignment.
> +
> +  Allocates the number of 4KB pages specified by Pages of a certain
> + memory type with an alignment  specified by Alignment.  The allocated
> buffer is returned.  If Pages is 0, then NULL is returned.
> +  If there is not enough memory at the specified alignment remaining to
> + satisfy the request, then  NULL is returned.
> +  If Alignment is not a power of two and Alignment is not zero, then
> ASSERT().
> +  If Pages plus EFI_SIZE_TO_PAGES (Alignment) overflows, then ASSERT().
> +
> +  @param  Type                  The type of allocation to perform.
> +  @param  MemoryType            The type of memory to allocate.
> +  @param  Pages                 The number of 4 KB pages to allocate.
> +  @param  Alignment             The requested alignment of the allocation.
> Must be a power of two.
> +                                If Alignment is zero, then byte alignment is used.
> +  @param  Address               Pointer to a physical address.
> +
> +  @return Memory Allocation Status.
> +
> +**/
> +EFI_STATUS
> +InternalAllocateAlignedPagesWithAllocateType (
> +  IN EFI_ALLOCATE_TYPE         Type,
> +  IN EFI_MEMORY_TYPE           MemoryType,
> +  IN UINTN                     Pages,
> +  IN UINTN                     Alignment,
> +  IN OUT EFI_PHYSICAL_ADDRESS  *Address
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EFI_PHYSICAL_ADDRESS  Memory;
> +  UINTN                 AlignedMemory;
> +  UINTN                 AlignmentMask;
> +  UINTN                 UnalignedPages;
> +  UINTN                 RealPages;
> +
> +  //
> +  // Alignment must be a power of two or zero.
> +  //
> +  ASSERT ((Alignment & (Alignment - 1)) == 0);
> +
> +  if (Pages == 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  if (Alignment > EFI_PAGE_SIZE) {
> +    //
> +    // Calculate the total number of pages since alignment is larger than page
> size.
> +    //
> +    AlignmentMask  = Alignment - 1;
> +    RealPages      = Pages + EFI_SIZE_TO_PAGES (Alignment);
> +    //
> +    // Make sure that Pages plus EFI_SIZE_TO_PAGES (Alignment) does not
> overflow.
> +    //
> +    ASSERT (RealPages > Pages);
> +
> +    Memory         = *Address;
> +    Status         = gBS->AllocatePages (Type, MemoryType, RealPages,
> &Memory);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    AlignedMemory  = ((UINTN) Memory + AlignmentMask) &
> ~AlignmentMask;
> +    UnalignedPages = EFI_SIZE_TO_PAGES (AlignedMemory - (UINTN)
> Memory);
> +    if (UnalignedPages > 0) {
> +      //
> +      // Free first unaligned page(s).
> +      //
> +      Status = gBS->FreePages (Memory, UnalignedPages);
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +    Memory         = (EFI_PHYSICAL_ADDRESS) (AlignedMemory +
> EFI_PAGES_TO_SIZE (Pages));
> +    UnalignedPages = RealPages - Pages - UnalignedPages;
> +    if (UnalignedPages > 0) {
> +      //
> +      // Free last unaligned page(s).
> +      //
> +      Status = gBS->FreePages (Memory, UnalignedPages);
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +  } else {
> +    //
> +    // Do not over-allocate pages in this case.
> +    //
> +    Memory = *Address;
> +    Status = gBS->AllocatePages (Type, MemoryType, Pages, &Memory);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    AlignedMemory  = (UINTN) Memory;
> +  }
> +  *Address = AlignedMemory;
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Return if a value is aligned.
> +
> +  @param Value the value to be checked
> +  @param Alignment the alignment to be checked with.
> +
> +  @retval TRUE  The value is aligned
> +  @retval FALSE The value is not aligned.
> +**/
> +BOOLEAN
> +InternalIsAlgined (
> +  IN UINTN  Value,
> +  IN UINTN  Alignment
> +  )
> +{
> +  if (Value == ALIGN_VALUE(Value, Alignment)) {
> +    return TRUE;
> +  } else {
> +    return FALSE;
> +  }
> +}
> +
> +/**
>    Provides the PCI controller-specific address needed to access
>    system memory for DMA.
> 
> @@ -1057,6 +1172,9 @@ RootBridgeIoMap (
>    PCI_ROOT_BRIDGE_INSTANCE                          *RootBridge;
>    EFI_PHYSICAL_ADDRESS                              PhysicalAddress;
>    MAP_INFO                                          *MapInfo;
> +  EFI_PHYSICAL_ADDRESS                              MaxAddress;
> +  BOOLEAN                                           NeedMap;
> +  BOOLEAN                                           NeedAllocateNonExisting;
> 
>    if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress ==
> NULL ||
>        Mapping == NULL) {
> @@ -1072,13 +1190,41 @@ RootBridgeIoMap (
> 
>    RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> 
> +  if (gIoMmuProtocol == NULL) {
> +    gBS->LocateProtocol (
> +          &gEdkiiIoMmuProtocolGuid,
> +          NULL,
> +          (VOID **) &gIoMmuProtocol
> +          );
> +    if (gIoMmuProtocol != NULL) {
> +      gIoMmuProtocol->GetPageSize (gIoMmuProtocol, &mIoMmuPageSize);
> +      ASSERT ((mIoMmuPageSize & (mIoMmuPageSize - 1)) == 0);
> +    }
> +  }
> +
>    PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
> +
> +  MaxAddress = (EFI_PHYSICAL_ADDRESS)-1;  NeedMap = FALSE;
> + NeedAllocateNonExisting = FALSE;
> +
>    if ((!RootBridge->DmaAbove4G ||
>         (Operation != EfiPciOperationBusMasterRead64 &&
>          Operation != EfiPciOperationBusMasterWrite64 &&
>          Operation != EfiPciOperationBusMasterCommonBuffer64)) &&
>        ((PhysicalAddress + *NumberOfBytes) > SIZE_4GB)) {
> +    NeedMap = TRUE;
> +    MaxAddress = SIZE_4GB - 1;
> +  }
> 
> +  if (gIoMmuProtocol != NULL) {
> +    if ((!InternalIsAlgined (*NumberOfBytes, mIoMmuPageSize)) ||
> +        (!InternalIsAlgined ((UINTN)HostAddress, mIoMmuPageSize))) {
> +      NeedMap = TRUE;
> +    }
> +  }
> +
> +  if (NeedMap) {
>      //
>      // 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
> @@ -1090,9 +1236,17 @@ RootBridgeIoMap (
>        //
>        // 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.
> +      // an error if there is no IOMMU.
>        //
> -      return EFI_UNSUPPORTED;
> +      if (gIoMmuProtocol == NULL) {
> +        return EFI_UNSUPPORTED;
> +      } else {
> +        //
> +        // We can try to allocate non-existing memory for below 4GiB address
> +        // and use IOMMU to remap it.
> +        //
> +        NeedAllocateNonExisting = TRUE;
> +      }
>      }
> 
>      //
> @@ -1113,21 +1267,81 @@ RootBridgeIoMap (
>      MapInfo->NumberOfBytes     = *NumberOfBytes;
>      MapInfo->NumberOfPages     = EFI_SIZE_TO_PAGES (MapInfo-
> >NumberOfBytes);
>      MapInfo->HostAddress       = PhysicalAddress;
> -    MapInfo->MappedHostAddress = SIZE_4GB - 1;
> +    MapInfo->MappedHostAddress = MaxAddress;
> +    MapInfo->MappedNumberOfBytes = ALIGN_VALUE (MapInfo-
> >NumberOfBytes, mIoMmuPageSize);
> +    MapInfo->MappedNumberOfPages = EFI_SIZE_TO_PAGES (MapInfo-
> >MappedNumberOfBytes);
> +    MapInfo->RemapNonExisting    = FALSE;
> 
> -    //
> -    // Allocate a buffer below 4GB to map the transfer to.
> -    //
> -    Status = gBS->AllocatePages (
> -                    AllocateMaxAddress,
> -                    EfiBootServicesData,
> -                    MapInfo->NumberOfPages,
> -                    &MapInfo->MappedHostAddress
> -                    );
> -    if (EFI_ERROR (Status)) {
> -      FreePool (MapInfo);
> -      *NumberOfBytes = 0;
> -      return Status;
> +    if (!((Operation == EfiPciOperationBusMasterCommonBuffer) ||
> +          (Operation == EfiPciOperationBusMasterCommonBuffer64))) {
> +      //
> +      // Allocate a buffer below 4GB to map the transfer to.
> +      //
> +      Status = InternalAllocateAlignedPagesWithAllocateType (
> +                      AllocateMaxAddress,
> +                      EfiBootServicesData,
> +                      MapInfo->MappedNumberOfPages,
> +                      mIoMmuPageSize,
> +                      &MapInfo->MappedHostAddress
> +                      );
> +      if (EFI_ERROR (Status)) {
> +        if (gIoMmuProtocol == NULL) {
> +          FreePool (MapInfo);
> +          *NumberOfBytes = 0;
> +          return Status;
> +        }
> +        //
> +        // We can try to allocate non-existing memory for below 4GiB address
> +        // and use IOMMU to remap it.
> +        //
> +        NeedAllocateNonExisting = TRUE;
> +      }
> +    }
> +
> +    if (NeedAllocateNonExisting) {
> +      if ((!InternalIsAlgined (*NumberOfBytes, mIoMmuPageSize)) ||
> +          (!InternalIsAlgined ((UINTN)HostAddress, mIoMmuPageSize))) {
> +        //
> +        // If original memory is not IOMMU aligned, we cannot remap it.
> +        //
> +        FreePool (MapInfo);
> +        *NumberOfBytes = 0;
> +        return EFI_UNSUPPORTED;
> +      }
> +      //
> +      // If the code runs to here, it means:
> +      // 1) We need remap a common buffer to below 4G non-existing
> memory.
> +      // 2) We need rempa a read/write buffer to below 4G non-existing
> memory.
> +      //
> +      MapInfo->MappedHostAddress = MaxAddress;
> +      Status = gDS->AllocateMemorySpace (
> +                      EfiGcdAllocateMaxAddressSearchTopDown,
> +                      EfiGcdMemoryTypeNonExistent,
> +                      mIoMmuPageSize,
> +                      MapInfo->MappedNumberOfBytes,
> +                      &MapInfo->MappedHostAddress,
> +                      gImageHandle,
> +                      RootBridge->Handle
> +                      );
> +      if (EFI_ERROR(Status)) {
> +        FreePool (MapInfo);
> +        *NumberOfBytes = 0;
> +        return Status;
> +      }
> +      Status = gIoMmuProtocol->SetRemapAddress (
> +                                 gIoMmuProtocol,
> +                                 NULL,
> +                                 MapInfo->MappedHostAddress,
> +                                 (VOID *)(UINTN)MapInfo->HostAddress,
> +                                 MapInfo->MappedNumberOfBytes
> +                                 );
> +      if (EFI_ERROR(Status)) {
> +        gDS->FreeMemorySpace (MapInfo->MappedHostAddress, MapInfo-
> >MappedNumberOfBytes);
> +        FreePool (MapInfo);
> +        *NumberOfBytes = 0;
> +        return Status;
> +      }
> +      MapInfo->RemapNonExisting = TRUE;
>      }
> 
>      //
> @@ -1135,19 +1349,26 @@ RootBridgeIoMap (
>      // 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 == EfiPciOperationBusMasterRead ||
> -        Operation == EfiPciOperationBusMasterRead64) {
> -      CopyMem (
> -        (VOID *) (UINTN) MapInfo->MappedHostAddress,
> -        (VOID *) (UINTN) MapInfo->HostAddress,
> -        MapInfo->NumberOfBytes
> -        );
> +    // This action can be skipped if there is IOMMU, because PciBus does
> +    // the action after setting IOMMU attributes.
> +    //
> +    if (gIoMmuProtocol == NULL) {
> +      if (Operation == EfiPciOperationBusMasterRead ||
> +          Operation == EfiPciOperationBusMasterRead64) {
> +        CopyMem (
> +          (VOID *) (UINTN) MapInfo->MappedHostAddress,
> +          (VOID *) (UINTN) MapInfo->HostAddress,
> +          MapInfo->NumberOfBytes
> +          );
> +      }
>      }
> 
>      InsertTailList (&RootBridge->Maps, &MapInfo->Link);
> 
>      //
>      // The DeviceAddress is the address of the maped buffer below 4GB
> +    // NOTE: It can be a valid system memory address.
> +    //       Or a non-existing memory but mapped by IOMMU.
>      //
>      *DeviceAddress = MapInfo->MappedHostAddress;
>      //
> @@ -1228,19 +1449,42 @@ RootBridgeIoUnmap (
>    // 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 == EfiPciOperationBusMasterWrite ||
> -      MapInfo->Operation == EfiPciOperationBusMasterWrite64) {
> -    CopyMem (
> -      (VOID *) (UINTN) MapInfo->HostAddress,
> -      (VOID *) (UINTN) MapInfo->MappedHostAddress,
> -      MapInfo->NumberOfBytes
> -      );
> +  // This action can be skipped if there is IOMMU, because PciBus does
> + // the action.
> +  //
> +  if (gIoMmuProtocol == NULL) {
> +    if (MapInfo->Operation == EfiPciOperationBusMasterWrite ||
> +        MapInfo->Operation == EfiPciOperationBusMasterWrite64) {
> +      CopyMem (
> +        (VOID *) (UINTN) MapInfo->HostAddress,
> +        (VOID *) (UINTN) MapInfo->MappedHostAddress,
> +        MapInfo->NumberOfBytes
> +        );
> +    }
> +  }
> +
> +  if (gIoMmuProtocol != NULL) {
> +    //
> +    // Free GCD non existing memory.
> +    //
> +    if (MapInfo->RemapNonExisting) {
> +      gIoMmuProtocol->SetRemapAddress (
> +                        gIoMmuProtocol,
> +                        NULL,
> +                        MapInfo->MappedHostAddress,
> +                        (VOID *)(UINTN)MapInfo->MappedHostAddress,
> +                        MapInfo->MappedNumberOfBytes
> +                        );
> +      gDS->FreeMemorySpace (MapInfo->MappedHostAddress, MapInfo-
> >MappedNumberOfBytes);
> +      FreePool (Mapping);
> +      return EFI_SUCCESS;
> +    }
>    }
> 
>    //
>    // Free the mapped buffer and the MAP_INFO structure.
>    //
> -  gBS->FreePages (MapInfo->MappedHostAddress, MapInfo-
> >NumberOfPages);
> +  gBS->FreePages (MapInfo->MappedHostAddress,
> + MapInfo->MappedNumberOfPages);
>    FreePool (Mapping);
>    return EFI_SUCCESS;
>  }
> @@ -1285,7 +1529,7 @@ RootBridgeIoAllocateBuffer (
>    EFI_STATUS                Status;
>    EFI_PHYSICAL_ADDRESS      PhysicalAddress;
>    PCI_ROOT_BRIDGE_INSTANCE  *RootBridge;
> -  EFI_ALLOCATE_TYPE         AllocateType;
> +  UINTN                     Size;
> 
>    //
>    // Validate Attributes
> @@ -1312,25 +1556,52 @@ RootBridgeIoAllocateBuffer (
> 
>    RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> 
> -  AllocateType = AllocateAnyPages;
> +  PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (-1);
>    if (!RootBridge->DmaAbove4G ||
>        (Attributes & EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0) {
>      //
>      // Limit allocations to memory below 4GB
>      //
> -    AllocateType    = AllocateMaxAddress;
>      PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (SIZE_4GB - 1);
>    }
> -  Status = gBS->AllocatePages (
> -                  AllocateType,
> +  if (gIoMmuProtocol != NULL) {
> +    Size = EFI_PAGES_TO_SIZE(Pages);
> +    Size = ALIGN_VALUE(EFI_PAGES_TO_SIZE(Pages), mIoMmuPageSize);
> +    Pages = EFI_SIZE_TO_PAGES (Size);
> +  }
> +  Status = InternalAllocateAlignedPagesWithAllocateType (
> +                  AllocateMaxAddress,
>                    MemoryType,
>                    Pages,
> +                  mIoMmuPageSize,
>                    &PhysicalAddress
>                    );
>    if (!EFI_ERROR (Status)) {
>      *HostAddress = (VOID *) (UINTN) PhysicalAddress;
> +    return EFI_SUCCESS;
>    }
> 
> +  if (gIoMmuProtocol != NULL) {
> +    //
> +    // Try to allocate AnyAddress here.
> +    //
> +    // We can try to allocate non-existing memory for below 4GiB address
> +    // and use IOMMU to remap it later.
> +    //
> +    PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (-1);
> +    Status = InternalAllocateAlignedPagesWithAllocateType (
> +               AllocateMaxAddress,
> +               MemoryType,
> +               Pages,
> +               mIoMmuPageSize,
> +               &PhysicalAddress
> +               );
> +    if (EFI_ERROR(Status)) {
> +      return Status;
> +    }
> +    *HostAddress = (VOID *) (UINTN) PhysicalAddress;
> +    return EFI_SUCCESS;
> +  }
>    return Status;
>  }
> 
> @@ -1356,6 +1627,13 @@ RootBridgeIoFreeBuffer (
>    OUT VOID                             *HostAddress
>    )
>  {
> +  UINTN                     Size;
> +
> +  if (gIoMmuProtocol != NULL) {
> +    Size = EFI_PAGES_TO_SIZE(Pages);
> +    Size = ALIGN_VALUE(EFI_PAGES_TO_SIZE(Pages), mIoMmuPageSize);
> +    Pages = EFI_SIZE_TO_PAGES (Size);
> +  }
>    return gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress,
> Pages);  }
> 
> --
> 2.7.4.windows.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: Add IOMMU support.
  2017-04-04  7:06 ` [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: " Jiewen Yao
@ 2017-04-17 19:58   ` Duran, Leo
  2017-04-18  3:45     ` Yao, Jiewen
  0 siblings, 1 reply; 10+ messages in thread
From: Duran, Leo @ 2017-04-17 19:58 UTC (permalink / raw)
  To: 'Jiewen Yao', edk2-devel@lists.01.org
  Cc: Ruiyu Ni, Singh, Brijesh, Ard Biesheuvel

Hi Yao,

Regarding RootBridgeIoMap()

I'm wondering if may be possible to simplify the logic requiring flags "NeedMap" and "NeedAllocateNonExisting"?
For example, it seems like (NeedAllocateNonExisting==TRUE) implies (gIoMmuProtocol != NULL), but that did not seem obvious at a glance.

Leo.

> -----Original Message-----
> From: Jiewen Yao [mailto:jiewen.yao@intel.com]
> Sent: Tuesday, April 04, 2017 2:06 AM
> To: edk2-devel@lists.01.org
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>; Duran, Leo <leo.duran@amd.com>;
> Singh, Brijesh <brijesh.singh@amd.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: Add IOMMU support.
> 
> The responsibility of PciBus driver is to set IOMMU attribute, because only
> PciBus knows which device submits DMA access request.
> 
> PciBus driver guarantee that the request to PciHostBridge is IOMMU page
> aligned memory, as such PciHostBridge can allocate non-existent memory for
> device memory, to satisfy remap requirement.
> 
> PciBus driver does not assume device address is same as the mapped host
> address, because IOMMU may remap it.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Leo Duran <leo.duran@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c      |  12 ++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h      |  19 ++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf |   1 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c       | 225
> +++++++++++++++++++-
>  4 files changed, 247 insertions(+), 10 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> index f3be47a..c9ee4de 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> @@ -42,6 +42,8 @@ UINT64                                        gAllZero             = 0;
> 
>  EFI_PCI_PLATFORM_PROTOCOL                     *gPciPlatformProtocol;
>  EFI_PCI_OVERRIDE_PROTOCOL                     *gPciOverrideProtocol;
> +EDKII_IOMMU_PROTOCOL                          *gIoMmuProtocol;
> +UINTN                                         mIoMmuPageSize = 1;
> 
> 
>  GLOBAL_REMOVE_IF_UNREFERENCED
> EFI_PCI_HOTPLUG_REQUEST_PROTOCOL mPciHotPlugRequest = { @@ -
> 256,6 +258,16 @@ PciBusDriverBindingStart (
>    }
> 
>    gBS->LocateProtocol (
> +        &gEdkiiIoMmuProtocolGuid,
> +        NULL,
> +        (VOID **) &gIoMmuProtocol
> +        );
> +  if (gIoMmuProtocol != NULL) {
> +    gIoMmuProtocol->GetPageSize (gIoMmuProtocol, &mIoMmuPageSize);
> +    ASSERT ((mIoMmuPageSize & (mIoMmuPageSize - 1)) == 0);  }
> +
> +  gBS->LocateProtocol (
>           &gEfiIncompatiblePciDeviceSupportProtocolGuid,
>           NULL,
>           (VOID **) &gIncompatiblePciDeviceSupport diff --git
> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> index 39ba8b9..185d89c 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> @@ -32,6 +32,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Protocol/IncompatiblePciDeviceSupport.h>
>  #include <Protocol/PciOverride.h>
>  #include <Protocol/PciEnumerationComplete.h>
> +#include <Protocol/IoMmu.h>
> 
>  #include <Library/DebugLib.h>
>  #include <Library/UefiDriverEntryPoint.h> @@ -289,6 +290,8 @@ struct
> _PCI_IO_DEVICE {
>    // This field is used to support this case.
>    //
>    UINT16                                    BridgeIoAlignment;
> +
> +  LIST_ENTRY                                Maps;
>  };
> 
>  #define PCI_IO_DEVICE_FROM_PCI_IO_THIS(a) \ @@ -304,6 +307,20 @@
> struct _PCI_IO_DEVICE {
>    CR (a, PCI_IO_DEVICE, LoadFile2, PCI_IO_DEVICE_SIGNATURE)
> 
> 
> +#define PCI_IO_MAP_INFO_SIGNATURE  SIGNATURE_32 ('p', 'm', 'a', 'p')
> +typedef struct {
> +  UINT32                                    Signature;
> +  LIST_ENTRY                                Link;
> +  EFI_PCI_IO_PROTOCOL_OPERATION             Operation;
> +  VOID                                      *HostAddress;
> +  EFI_PHYSICAL_ADDRESS                      DeviceAddress;
> +  UINTN                                     NumberOfBytes;
> +  VOID                                      *AlignedHostAddress;
> +  UINTN                                     AlignedNumberOfBytes;
> +  VOID                                      *MappedHostAddress;
> +  VOID                                      *PciRootBridgeIoMapping;
> +} PCI_IO_MAP_INFO;
> +#define PCI_IO_MAP_INFO_FROM_LINK(a) CR (a, PCI_IO_MAP_INFO,
> Link,
> +PCI_IO_MAP_INFO_SIGNATURE)
> 
>  //
>  // Global Variables
> @@ -321,6 +338,8 @@ extern EFI_PCI_PLATFORM_PROTOCOL
> *gPciPlatformProtocol;
>  extern EFI_PCI_OVERRIDE_PROTOCOL                    *gPciOverrideProtocol;
>  extern BOOLEAN                                      mReserveIsaAliases;
>  extern BOOLEAN                                      mReserveVgaAliases;
> +extern EDKII_IOMMU_PROTOCOL                         *gIoMmuProtocol;
> +extern UINTN                                        mIoMmuPageSize;
> 
>  /**
>    Macro that checks whether device is a GFX device.
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> index a3ab11f..5da094f 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> @@ -95,6 +95,7 @@
>    gEfiPciRootBridgeIoProtocolGuid                 ## TO_START
>    gEfiIncompatiblePciDeviceSupportProtocolGuid    ##
> SOMETIMES_CONSUMES
>    gEfiLoadFile2ProtocolGuid                       ## SOMETIMES_PRODUCES
> +  gEdkiiIoMmuProtocolGuid                         ## SOMETIMES_CONSUMES
> 
>  [FeaturePcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPciBusHotplugDeviceSupport      ##
> CONSUMES
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> index f72598d..31b8c32 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> @@ -58,6 +58,7 @@ InitializePciIoInstance (
>    )
>  {
>    CopyMem (&PciIoDevice->PciIo, &mPciIoInterface, sizeof
> (EFI_PCI_IO_PROTOCOL));
> +  InitializeListHead (&PciIoDevice->Maps);
>  }
> 
>  /**
> @@ -936,6 +937,28 @@ PciIoCopyMem (
>  }
> 
>  /**
> +  Return if a value is aligned.
> +
> +  @param Value the value to be checked
> +  @param Alignment the alignment to be checked with.
> +
> +  @retval TRUE  The value is aligned
> +  @retval FALSE The value is not aligned.
> +**/
> +BOOLEAN
> +InternalIsAlgined (
> +  IN UINTN  Value,
> +  IN UINTN  Alignment
> +  )
> +{
> +  if (Value == ALIGN_VALUE(Value, Alignment)) {
> +    return TRUE;
> +  } else {
> +    return FALSE;
> +  }
> +}
> +
> +/**
>    Provides the PCI controller-specific addresses needed to access system
> memory.
> 
>    @param  This                  A pointer to the EFI_PCI_IO_PROTOCOL instance.
> @@ -967,6 +990,9 @@ PciIoMap (
>  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  PCI_IO_MAP_INFO       *PciIoMapInfo;
> +  UINT64                IoMmuAttribute;
> +  EFI_STATUS            RemapStatus;
> 
>    PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
> 
> @@ -982,15 +1008,60 @@ PciIoMap (
>      Operation = (EFI_PCI_IO_PROTOCOL_OPERATION) (Operation +
> EfiPciOperationBusMasterRead64);
>    }
> 
> -  Status = PciIoDevice->PciRootBridgeIo->Map (
> -                                          PciIoDevice->PciRootBridgeIo,
> -                                          (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION)
> Operation,
> -                                          HostAddress,
> -                                          NumberOfBytes,
> -                                          DeviceAddress,
> -                                          Mapping
> -                                          );
> -
> +  if (gIoMmuProtocol != NULL) {
> +    PciIoMapInfo = AllocatePool (sizeof(*PciIoMapInfo));
> +    if (PciIoMapInfo == NULL) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +    PciIoMapInfo->Signature              = PCI_IO_MAP_INFO_SIGNATURE;
> +    PciIoMapInfo->Operation              = Operation;
> +    PciIoMapInfo->NumberOfBytes          = *NumberOfBytes;
> +    PciIoMapInfo->DeviceAddress          = *DeviceAddress;
> +    PciIoMapInfo->HostAddress            = HostAddress;
> +    //
> +    // For non common buffer, we always allocate a new memory if IOMMU
> exists.
> +    // because the original memory might not be DMA capable.
> +    //
> +    // For common buffer, it is not needed, because common buffer allocate
> via PciIoAllocateBuffer.
> +    // We cannot use AllocateAlignedPages here, because there might be
> more restriction in PciIoAllocateBuffer().
> +    //
> +    PciIoMapInfo->AlignedNumberOfBytes   = ALIGN_VALUE (PciIoMapInfo-
> >NumberOfBytes, mIoMmuPageSize);
> +    if (PciIoMapInfo->Operation !=
> EfiPciIoOperationBusMasterCommonBuffer) {
> +      PciIoMapInfo->AlignedHostAddress = AllocateAlignedPages
> (EFI_SIZE_TO_PAGES(PciIoMapInfo->AlignedNumberOfBytes),
> mIoMmuPageSize);
> +      if (PciIoMapInfo->AlignedHostAddress == NULL) {
> +        FreePool (PciIoMapInfo);
> +        return EFI_OUT_OF_RESOURCES;
> +      }
> +    } else {
> +      //
> +      // For common buffer, the HostAddress must be allocated via
> PciIoAllocateBuffer.
> +      //
> +      if (!InternalIsAlgined((UINTN)PciIoMapInfo->HostAddress,
> mIoMmuPageSize)) {
> +        FreePool (PciIoMapInfo);
> +        DEBUG ((DEBUG_ERROR, "PciIoMap - map unaligned common buffer
> with IOMMU\n"));
> +        return EFI_UNSUPPORTED;
> +      }
> +      PciIoMapInfo->AlignedHostAddress = PciIoMapInfo->HostAddress;
> +    }
> +    PciIoMapInfo->PciRootBridgeIoMapping = NULL;
> +    Status = PciIoDevice->PciRootBridgeIo->Map (
> +                                            PciIoDevice->PciRootBridgeIo,
> +
> (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION) Operation,
> +                                            PciIoMapInfo->AlignedHostAddress,
> +                                            &PciIoMapInfo->AlignedNumberOfBytes,
> +                                            &PciIoMapInfo->DeviceAddress,
> +                                            &PciIoMapInfo->PciRootBridgeIoMapping
> +                                            );  } else {
> +    Status = PciIoDevice->PciRootBridgeIo->Map (
> +                                            PciIoDevice->PciRootBridgeIo,
> +
> (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION) Operation,
> +                                            HostAddress,
> +                                            NumberOfBytes,
> +                                            DeviceAddress,
> +                                            Mapping
> +                                            );  }
>    if (EFI_ERROR (Status)) {
>      REPORT_STATUS_CODE_WITH_DEVICE_PATH (
>        EFI_ERROR_CODE | EFI_ERROR_MINOR, @@ -999,6 +1070,63 @@
> PciIoMap (
>        );
>    }
> 
> +  if (gIoMmuProtocol != NULL) {
> +    if (EFI_ERROR(Status)) {
> +      if (PciIoMapInfo->Operation !=
> EfiPciIoOperationBusMasterCommonBuffer) {
> +        FreePages (PciIoMapInfo->AlignedHostAddress,
> EFI_SIZE_TO_PAGES(PciIoMapInfo->AlignedNumberOfBytes));
> +      }
> +      FreePool (PciIoMapInfo);
> +    } else {
> +      *DeviceAddress = PciIoMapInfo->DeviceAddress;
> +      *Mapping = PciIoMapInfo;
> +
> +      switch (Operation) {
> +      case EfiPciIoOperationBusMasterRead:
> +        IoMmuAttribute = EDKII_IOMMU_ATTRIBUTE_READ;
> +        break;
> +      case EfiPciIoOperationBusMasterWrite:
> +        IoMmuAttribute = EDKII_IOMMU_ATTRIBUTE_WRITE;
> +        break;
> +      case EfiPciIoOperationBusMasterCommonBuffer:
> +        IoMmuAttribute = EDKII_IOMMU_ATTRIBUTE_READ |
> EDKII_IOMMU_ATTRIBUTE_WRITE;
> +        break;
> +      default:
> +        ASSERT(FALSE);
> +        return EFI_INVALID_PARAMETER;
> +      }
> +      //
> +      // PciHostBridge should map IOMMU page aligned HostAddress.
> +      //
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        PciIoMapInfo->DeviceAddress,
> +                        PciIoMapInfo->AlignedNumberOfBytes,
> +                        IoMmuAttribute
> +                        );
> +      //
> +      // We need do copy mem after IoMmu->SetAttribute(),
> +      // because it might change IOMMU state.
> +      //
> +      RemapStatus = gIoMmuProtocol->GetRemapAddress (
> +                                      gIoMmuProtocol,
> +                                      PciIoDevice->Handle,
> +                                      PciIoMapInfo->DeviceAddress,
> +                                      &PciIoMapInfo->MappedHostAddress
> +                                      );
> +      if (EFI_ERROR(RemapStatus)) {
> +        PciIoMapInfo->MappedHostAddress = (VOID *)(UINTN)PciIoMapInfo-
> >DeviceAddress;
> +      }
> +      if (Operation == EfiPciIoOperationBusMasterRead) {
> +        CopyMem (
> +          PciIoMapInfo->MappedHostAddress,
> +          PciIoMapInfo->HostAddress,
> +          PciIoMapInfo->NumberOfBytes
> +          );
> +      }
> +      InsertTailList (&PciIoDevice->Maps, &PciIoMapInfo->Link);
> +    }
> +  }
>    return Status;
>  }
> 
> @@ -1021,9 +1149,48 @@ PciIoUnmap (
>  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  PCI_IO_MAP_INFO *PciIoMapInfo;
> +  LIST_ENTRY      *Link;
> 
>    PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
> 
> +  PciIoMapInfo = NULL;
> +  if (gIoMmuProtocol != NULL) {
> +    PciIoMapInfo = NULL;
> +    for (Link = GetFirstNode (&PciIoDevice->Maps)
> +         ; !IsNull (&PciIoDevice->Maps, Link)
> +         ; Link = GetNextNode (&PciIoDevice->Maps, Link)
> +         ) {
> +      PciIoMapInfo = PCI_IO_MAP_INFO_FROM_LINK (Link);
> +      if (PciIoMapInfo == Mapping) {
> +        break;
> +      }
> +    }
> +    //
> +    // Mapping is not a valid value returned by Map()
> +    //
> +    if (PciIoMapInfo != Mapping) {
> +      DEBUG ((DEBUG_INFO, "PciIoUnmap - PciIoMapInfo not found!\n"));
> +      return EFI_INVALID_PARAMETER;
> +    }
> +    RemoveEntryList (&PciIoMapInfo->Link);
> +    Mapping = PciIoMapInfo->PciRootBridgeIoMapping;
> +
> +    //
> +    // PciHostBridge should map IOMMU page aligned HostAddress.
> +    //
> +    // We need do copy mem before PciRootBridgeIo->Unmap(),
> +    // because it might free mapped host address.
> +    //
> +    if (PciIoMapInfo->Operation == EfiPciIoOperationBusMasterWrite) {
> +      CopyMem (
> +        PciIoMapInfo->HostAddress,
> +        PciIoMapInfo->MappedHostAddress,
> +        PciIoMapInfo->NumberOfBytes
> +        );
> +    }
> +  }
> +
>    Status = PciIoDevice->PciRootBridgeIo->Unmap (
>                                            PciIoDevice->PciRootBridgeIo,
>                                            Mapping @@ -1037,6 +1204,25 @@ PciIoUnmap (
>        );
>    }
> 
> +  if (gIoMmuProtocol != NULL) {
> +    if (!EFI_ERROR(Status)) {
> +      //
> +      // PciHostBridge should map IOMMU page aligned HostAddress.
> +      //
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        PciIoMapInfo->DeviceAddress,
> +                        PciIoMapInfo->AlignedNumberOfBytes,
> +                        0
> +                        );
> +      if (PciIoMapInfo->Operation !=
> EfiPciIoOperationBusMasterCommonBuffer) {
> +        FreePages (PciIoMapInfo->AlignedHostAddress,
> EFI_SIZE_TO_PAGES(PciIoMapInfo->AlignedNumberOfBytes));
> +      }
> +      FreePool (PciIoMapInfo);
> +    }
> +  }
> +
>    return Status;
>  }
> 
> @@ -1073,6 +1259,7 @@ PciIoAllocateBuffer (  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  UINTN         Size;
> 
>    if ((Attributes &
>        (~(EFI_PCI_ATTRIBUTE_MEMORY_WRITE_COMBINE |
> EFI_PCI_ATTRIBUTE_MEMORY_CACHED))) != 0){ @@ -1085,6 +1272,12 @@
> PciIoAllocateBuffer (
>      Attributes |= EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE;
>    }
> 
> +  if (gIoMmuProtocol != NULL) {
> +    Size = EFI_PAGES_TO_SIZE(Pages);
> +    Size = ALIGN_VALUE(Size, mIoMmuPageSize);
> +    Pages = EFI_SIZE_TO_PAGES (Size);
> +  }
> +
>    Status = PciIoDevice->PciRootBridgeIo->AllocateBuffer (
>                                            PciIoDevice->PciRootBridgeIo,
>                                            Type, @@ -1101,7 +1294,9 @@ PciIoAllocateBuffer (
>        PciIoDevice->DevicePath
>        );
>    }
> -
> +  //
> +  // No need to set attribute here, it is done in Map.
> +  //
>    return Status;
>  }
> 
> @@ -1127,9 +1322,16 @@ PciIoFreeBuffer (  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  UINTN         Size;
> 
>    PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
> 
> +  if (gIoMmuProtocol != NULL) {
> +    Size = EFI_PAGES_TO_SIZE(Pages);
> +    Size = ALIGN_VALUE(Size, mIoMmuPageSize);
> +    Pages = EFI_SIZE_TO_PAGES (Size);
> +  }
> +
>    Status = PciIoDevice->PciRootBridgeIo->FreeBuffer (
>                                            PciIoDevice->PciRootBridgeIo,
>                                            Pages, @@ -1144,6 +1346,9 @@ PciIoFreeBuffer (
>        );
>    }
> 
> +  //
> +  // No need to set attribute here, it is done in Unmap.
> +  //
>    return Status;
>  }
> 
> --
> 2.7.4.windows.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] [PATCH V3 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.
  2017-04-17 19:33   ` Duran, Leo
@ 2017-04-18  3:31     ` Yao, Jiewen
  0 siblings, 0 replies; 10+ messages in thread
From: Yao, Jiewen @ 2017-04-18  3:31 UTC (permalink / raw)
  To: Duran, Leo, edk2-devel@lists.01.org
  Cc: Ni, Ruiyu, Singh, Brijesh, Ard Biesheuvel

Thanks Leo.
Answer below:

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Duran, Leo
Sent: Tuesday, April 18, 2017 3:33 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Singh, Brijesh <brijesh.singh@amd.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2] [RFC] [PATCH V3 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.

Hi Yao,

Just a couple of quick comments:

1) gIoMmuProtocol is declared (global) by PciHostBridge.c, but it is not initialized to NULL.
[Jiewen] Our PECOFF loader zero it. I do not believe it will bring any real impact.
But I agree with you that we can explicit assign to NULL to avoid confusing.


2) Would it be OK to do a one-time LocateProtocol() of gIoMmuProtocol  in InitializePciHostBridge()?
BTW, besides the global declaration, gIoMmuProtocol  is currently not used by PciHostBridge.c
[Jiewen] It is hard, because of dependency.
We are not able to guarantee IOMMU driver runs before PciHostBridge.
We cannot add dependency for PciHostBridge, because IOMMU is an optional feature.


Leo

> -----Original Message-----
> From: Jiewen Yao [mailto:jiewen.yao@intel.com]
> Sent: Tuesday, April 04, 2017 2:06 AM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Duran, Leo <leo.duran@amd.com<mailto:leo.duran@amd.com>>;
> Singh, Brijesh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>
> Subject: [RFC] [PATCH V3 2/3] MdeModulePkg/PciHostBridge: Add IOMMU
> support.
>
> The responsibility of PciHostBridge is to allocate IOMMU page aligned
> memory for Map and AllocateBuffer, because PciHostBridge driver already
> handles Map() request to allocate another buffer for DMA read/write.
>
> If the max address requirement can not be satisfied, PciHostBridge may also
> allocate any IOMMU page aligned memory and use IOMMU Remapping
> feature to map to lower address to satisfy device requirement.
>
> PciHostBridge does not set IOMMU attribute because it does not know
> which device request the DMA. This work is done by PciBus driver.
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>
> Cc: Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>
> Cc: Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> ---
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c      |   3 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf |   1 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h      |   8 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 350
> ++++++++++++++++++--
>  4 files changed, 326 insertions(+), 36 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> index 9005dee..35233a7 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> @@ -28,6 +28,9 @@ GLOBAL_REMOVE_IF_UNREFERENCED CHAR16
> *mPciResourceTypeStr[] = {
>    L"I/O", L"Mem", L"PMem", L"Mem64", L"PMem64", L"Bus"
>  };
>
> +EDKII_IOMMU_PROTOCOL        *gIoMmuProtocol;
> +UINTN                       mIoMmuPageSize = 1;
> +
>  /**
>    Ensure the compatibility of an IO space descriptor with the IO aperture.
>
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> index d8b0439..2d3c8c9 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> @@ -49,6 +49,7 @@
>    gEfiDevicePathProtocolGuid                      ## BY_START
>    gEfiPciRootBridgeIoProtocolGuid                 ## BY_START
>    gEfiPciHostBridgeResourceAllocationProtocolGuid ## BY_START
> +  gEdkiiIoMmuProtocolGuid                         ## CONSUMES
>
>  [Depex]
>    gEfiCpuIo2ProtocolGuid AND
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> index 13185b4..77c3490 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> @@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Protocol/CpuIo2.h>
>  #include <Protocol/DevicePath.h>
>  #include <Protocol/PciRootBridgeIo.h>
> +#include <Protocol/IoMmu.h>
>  #include <Library/DebugLib.h>
>  #include <Library/DevicePathLib.h>
>  #include <Library/BaseMemoryLib.h>
> @@ -50,8 +51,11 @@ typedef struct {
>    EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION Operation;
>    UINTN                                     NumberOfBytes;
>    UINTN                                     NumberOfPages;
> +  UINTN                                     MappedNumberOfBytes;
> +  UINTN                                     MappedNumberOfPages;
>    EFI_PHYSICAL_ADDRESS                      HostAddress;
>    EFI_PHYSICAL_ADDRESS                      MappedHostAddress;
> +  BOOLEAN                                   RemapNonExisting;
>  } MAP_INFO;
>  #define MAP_INFO_FROM_LINK(a) CR (a, MAP_INFO, Link,
> MAP_INFO_SIGNATURE)
>
> @@ -575,4 +579,8 @@ RootBridgeIoConfiguration (
>
>  extern EFI_METRONOME_ARCH_PROTOCOL *mMetronome;
>  extern EFI_CPU_IO2_PROTOCOL         *mCpuIo;
> +
> +extern EDKII_IOMMU_PROTOCOL        *gIoMmuProtocol;
> +extern UINTN                       mIoMmuPageSize;
> +
>  #endif
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index 8af131b..2a17eb1 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -1022,6 +1022,121 @@ RootBridgeIoPciWrite (  }
>
>  /**
> +  Allocates one or more 4KB pages of a certain memory type at a specified
> alignment.
> +
> +  Allocates the number of 4KB pages specified by Pages of a certain
> + memory type with an alignment  specified by Alignment.  The allocated
> buffer is returned.  If Pages is 0, then NULL is returned.
> +  If there is not enough memory at the specified alignment remaining to
> + satisfy the request, then  NULL is returned.
> +  If Alignment is not a power of two and Alignment is not zero, then
> ASSERT().
> +  If Pages plus EFI_SIZE_TO_PAGES (Alignment) overflows, then ASSERT().
> +
> +  @param  Type                  The type of allocation to perform.
> +  @param  MemoryType            The type of memory to allocate.
> +  @param  Pages                 The number of 4 KB pages to allocate.
> +  @param  Alignment             The requested alignment of the allocation.
> Must be a power of two.
> +                                If Alignment is zero, then byte alignment is used.
> +  @param  Address               Pointer to a physical address.
> +
> +  @return Memory Allocation Status.
> +
> +**/
> +EFI_STATUS
> +InternalAllocateAlignedPagesWithAllocateType (
> +  IN EFI_ALLOCATE_TYPE         Type,
> +  IN EFI_MEMORY_TYPE           MemoryType,
> +  IN UINTN                     Pages,
> +  IN UINTN                     Alignment,
> +  IN OUT EFI_PHYSICAL_ADDRESS  *Address
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EFI_PHYSICAL_ADDRESS  Memory;
> +  UINTN                 AlignedMemory;
> +  UINTN                 AlignmentMask;
> +  UINTN                 UnalignedPages;
> +  UINTN                 RealPages;
> +
> +  //
> +  // Alignment must be a power of two or zero.
> +  //
> +  ASSERT ((Alignment & (Alignment - 1)) == 0);
> +
> +  if (Pages == 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  if (Alignment > EFI_PAGE_SIZE) {
> +    //
> +    // Calculate the total number of pages since alignment is larger than page
> size.
> +    //
> +    AlignmentMask  = Alignment - 1;
> +    RealPages      = Pages + EFI_SIZE_TO_PAGES (Alignment);
> +    //
> +    // Make sure that Pages plus EFI_SIZE_TO_PAGES (Alignment) does not
> overflow.
> +    //
> +    ASSERT (RealPages > Pages);
> +
> +    Memory         = *Address;
> +    Status         = gBS->AllocatePages (Type, MemoryType, RealPages,
> &Memory);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    AlignedMemory  = ((UINTN) Memory + AlignmentMask) &
> ~AlignmentMask;
> +    UnalignedPages = EFI_SIZE_TO_PAGES (AlignedMemory - (UINTN)
> Memory);
> +    if (UnalignedPages > 0) {
> +      //
> +      // Free first unaligned page(s).
> +      //
> +      Status = gBS->FreePages (Memory, UnalignedPages);
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +    Memory         = (EFI_PHYSICAL_ADDRESS) (AlignedMemory +
> EFI_PAGES_TO_SIZE (Pages));
> +    UnalignedPages = RealPages - Pages - UnalignedPages;
> +    if (UnalignedPages > 0) {
> +      //
> +      // Free last unaligned page(s).
> +      //
> +      Status = gBS->FreePages (Memory, UnalignedPages);
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +  } else {
> +    //
> +    // Do not over-allocate pages in this case.
> +    //
> +    Memory = *Address;
> +    Status = gBS->AllocatePages (Type, MemoryType, Pages, &Memory);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    AlignedMemory  = (UINTN) Memory;
> +  }
> +  *Address = AlignedMemory;
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Return if a value is aligned.
> +
> +  @param Value the value to be checked
> +  @param Alignment the alignment to be checked with.
> +
> +  @retval TRUE  The value is aligned
> +  @retval FALSE The value is not aligned.
> +**/
> +BOOLEAN
> +InternalIsAlgined (
> +  IN UINTN  Value,
> +  IN UINTN  Alignment
> +  )
> +{
> +  if (Value == ALIGN_VALUE(Value, Alignment)) {
> +    return TRUE;
> +  } else {
> +    return FALSE;
> +  }
> +}
> +
> +/**
>    Provides the PCI controller-specific address needed to access
>    system memory for DMA.
>
> @@ -1057,6 +1172,9 @@ RootBridgeIoMap (
>    PCI_ROOT_BRIDGE_INSTANCE                          *RootBridge;
>    EFI_PHYSICAL_ADDRESS                              PhysicalAddress;
>    MAP_INFO                                          *MapInfo;
> +  EFI_PHYSICAL_ADDRESS                              MaxAddress;
> +  BOOLEAN                                           NeedMap;
> +  BOOLEAN                                           NeedAllocateNonExisting;
>
>    if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress ==
> NULL ||
>        Mapping == NULL) {
> @@ -1072,13 +1190,41 @@ RootBridgeIoMap (
>
>    RootBridge = ROOT_BRIDGE_FROM_THIS (This);
>
> +  if (gIoMmuProtocol == NULL) {
> +    gBS->LocateProtocol (
> +          &gEdkiiIoMmuProtocolGuid,
> +          NULL,
> +          (VOID **) &gIoMmuProtocol
> +          );
> +    if (gIoMmuProtocol != NULL) {
> +      gIoMmuProtocol->GetPageSize (gIoMmuProtocol, &mIoMmuPageSize);
> +      ASSERT ((mIoMmuPageSize & (mIoMmuPageSize - 1)) == 0);
> +    }
> +  }
> +
>    PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
> +
> +  MaxAddress = (EFI_PHYSICAL_ADDRESS)-1;  NeedMap = FALSE;
> + NeedAllocateNonExisting = FALSE;
> +
>    if ((!RootBridge->DmaAbove4G ||
>         (Operation != EfiPciOperationBusMasterRead64 &&
>          Operation != EfiPciOperationBusMasterWrite64 &&
>          Operation != EfiPciOperationBusMasterCommonBuffer64)) &&
>        ((PhysicalAddress + *NumberOfBytes) > SIZE_4GB)) {
> +    NeedMap = TRUE;
> +    MaxAddress = SIZE_4GB - 1;
> +  }
>
> +  if (gIoMmuProtocol != NULL) {
> +    if ((!InternalIsAlgined (*NumberOfBytes, mIoMmuPageSize)) ||
> +        (!InternalIsAlgined ((UINTN)HostAddress, mIoMmuPageSize))) {
> +      NeedMap = TRUE;
> +    }
> +  }
> +
> +  if (NeedMap) {
>      //
>      // 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
> @@ -1090,9 +1236,17 @@ RootBridgeIoMap (
>        //
>        // 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.
> +      // an error if there is no IOMMU.
>        //
> -      return EFI_UNSUPPORTED;
> +      if (gIoMmuProtocol == NULL) {
> +        return EFI_UNSUPPORTED;
> +      } else {
> +        //
> +        // We can try to allocate non-existing memory for below 4GiB address
> +        // and use IOMMU to remap it.
> +        //
> +        NeedAllocateNonExisting = TRUE;
> +      }
>      }
>
>      //
> @@ -1113,21 +1267,81 @@ RootBridgeIoMap (
>      MapInfo->NumberOfBytes     = *NumberOfBytes;
>      MapInfo->NumberOfPages     = EFI_SIZE_TO_PAGES (MapInfo-
> >NumberOfBytes);
>      MapInfo->HostAddress       = PhysicalAddress;
> -    MapInfo->MappedHostAddress = SIZE_4GB - 1;
> +    MapInfo->MappedHostAddress = MaxAddress;
> +    MapInfo->MappedNumberOfBytes = ALIGN_VALUE (MapInfo-
> >NumberOfBytes, mIoMmuPageSize);
> +    MapInfo->MappedNumberOfPages = EFI_SIZE_TO_PAGES (MapInfo-
> >MappedNumberOfBytes);
> +    MapInfo->RemapNonExisting    = FALSE;
>
> -    //
> -    // Allocate a buffer below 4GB to map the transfer to.
> -    //
> -    Status = gBS->AllocatePages (
> -                    AllocateMaxAddress,
> -                    EfiBootServicesData,
> -                    MapInfo->NumberOfPages,
> -                    &MapInfo->MappedHostAddress
> -                    );
> -    if (EFI_ERROR (Status)) {
> -      FreePool (MapInfo);
> -      *NumberOfBytes = 0;
> -      return Status;
> +    if (!((Operation == EfiPciOperationBusMasterCommonBuffer) ||
> +          (Operation == EfiPciOperationBusMasterCommonBuffer64))) {
> +      //
> +      // Allocate a buffer below 4GB to map the transfer to.
> +      //
> +      Status = InternalAllocateAlignedPagesWithAllocateType (
> +                      AllocateMaxAddress,
> +                      EfiBootServicesData,
> +                      MapInfo->MappedNumberOfPages,
> +                      mIoMmuPageSize,
> +                      &MapInfo->MappedHostAddress
> +                      );
> +      if (EFI_ERROR (Status)) {
> +        if (gIoMmuProtocol == NULL) {
> +          FreePool (MapInfo);
> +          *NumberOfBytes = 0;
> +          return Status;
> +        }
> +        //
> +        // We can try to allocate non-existing memory for below 4GiB address
> +        // and use IOMMU to remap it.
> +        //
> +        NeedAllocateNonExisting = TRUE;
> +      }
> +    }
> +
> +    if (NeedAllocateNonExisting) {
> +      if ((!InternalIsAlgined (*NumberOfBytes, mIoMmuPageSize)) ||
> +          (!InternalIsAlgined ((UINTN)HostAddress, mIoMmuPageSize))) {
> +        //
> +        // If original memory is not IOMMU aligned, we cannot remap it.
> +        //
> +        FreePool (MapInfo);
> +        *NumberOfBytes = 0;
> +        return EFI_UNSUPPORTED;
> +      }
> +      //
> +      // If the code runs to here, it means:
> +      // 1) We need remap a common buffer to below 4G non-existing
> memory.
> +      // 2) We need rempa a read/write buffer to below 4G non-existing
> memory.
> +      //
> +      MapInfo->MappedHostAddress = MaxAddress;
> +      Status = gDS->AllocateMemorySpace (
> +                      EfiGcdAllocateMaxAddressSearchTopDown,
> +                      EfiGcdMemoryTypeNonExistent,
> +                      mIoMmuPageSize,
> +                      MapInfo->MappedNumberOfBytes,
> +                      &MapInfo->MappedHostAddress,
> +                      gImageHandle,
> +                      RootBridge->Handle
> +                      );
> +      if (EFI_ERROR(Status)) {
> +        FreePool (MapInfo);
> +        *NumberOfBytes = 0;
> +        return Status;
> +      }
> +      Status = gIoMmuProtocol->SetRemapAddress (
> +                                 gIoMmuProtocol,
> +                                 NULL,
> +                                 MapInfo->MappedHostAddress,
> +                                 (VOID *)(UINTN)MapInfo->HostAddress,
> +                                 MapInfo->MappedNumberOfBytes
> +                                 );
> +      if (EFI_ERROR(Status)) {
> +        gDS->FreeMemorySpace (MapInfo->MappedHostAddress, MapInfo-
> >MappedNumberOfBytes);
> +        FreePool (MapInfo);
> +        *NumberOfBytes = 0;
> +        return Status;
> +      }
> +      MapInfo->RemapNonExisting = TRUE;
>      }
>
>      //
> @@ -1135,19 +1349,26 @@ RootBridgeIoMap (
>      // 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 == EfiPciOperationBusMasterRead ||
> -        Operation == EfiPciOperationBusMasterRead64) {
> -      CopyMem (
> -        (VOID *) (UINTN) MapInfo->MappedHostAddress,
> -        (VOID *) (UINTN) MapInfo->HostAddress,
> -        MapInfo->NumberOfBytes
> -        );
> +    // This action can be skipped if there is IOMMU, because PciBus does
> +    // the action after setting IOMMU attributes.
> +    //
> +    if (gIoMmuProtocol == NULL) {
> +      if (Operation == EfiPciOperationBusMasterRead ||
> +          Operation == EfiPciOperationBusMasterRead64) {
> +        CopyMem (
> +          (VOID *) (UINTN) MapInfo->MappedHostAddress,
> +          (VOID *) (UINTN) MapInfo->HostAddress,
> +          MapInfo->NumberOfBytes
> +          );
> +      }
>      }
>
>      InsertTailList (&RootBridge->Maps, &MapInfo->Link);
>
>      //
>      // The DeviceAddress is the address of the maped buffer below 4GB
> +    // NOTE: It can be a valid system memory address.
> +    //       Or a non-existing memory but mapped by IOMMU.
>      //
>      *DeviceAddress = MapInfo->MappedHostAddress;
>      //
> @@ -1228,19 +1449,42 @@ RootBridgeIoUnmap (
>    // 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 == EfiPciOperationBusMasterWrite ||
> -      MapInfo->Operation == EfiPciOperationBusMasterWrite64) {
> -    CopyMem (
> -      (VOID *) (UINTN) MapInfo->HostAddress,
> -      (VOID *) (UINTN) MapInfo->MappedHostAddress,
> -      MapInfo->NumberOfBytes
> -      );
> +  // This action can be skipped if there is IOMMU, because PciBus does
> + // the action.
> +  //
> +  if (gIoMmuProtocol == NULL) {
> +    if (MapInfo->Operation == EfiPciOperationBusMasterWrite ||
> +        MapInfo->Operation == EfiPciOperationBusMasterWrite64) {
> +      CopyMem (
> +        (VOID *) (UINTN) MapInfo->HostAddress,
> +        (VOID *) (UINTN) MapInfo->MappedHostAddress,
> +        MapInfo->NumberOfBytes
> +        );
> +    }
> +  }
> +
> +  if (gIoMmuProtocol != NULL) {
> +    //
> +    // Free GCD non existing memory.
> +    //
> +    if (MapInfo->RemapNonExisting) {
> +      gIoMmuProtocol->SetRemapAddress (
> +                        gIoMmuProtocol,
> +                        NULL,
> +                        MapInfo->MappedHostAddress,
> +                        (VOID *)(UINTN)MapInfo->MappedHostAddress,
> +                        MapInfo->MappedNumberOfBytes
> +                        );
> +      gDS->FreeMemorySpace (MapInfo->MappedHostAddress, MapInfo-
> >MappedNumberOfBytes);
> +      FreePool (Mapping);
> +      return EFI_SUCCESS;
> +    }
>    }
>
>    //
>    // Free the mapped buffer and the MAP_INFO structure.
>    //
> -  gBS->FreePages (MapInfo->MappedHostAddress, MapInfo-
> >NumberOfPages);
> +  gBS->FreePages (MapInfo->MappedHostAddress,
> + MapInfo->MappedNumberOfPages);
>    FreePool (Mapping);
>    return EFI_SUCCESS;
>  }
> @@ -1285,7 +1529,7 @@ RootBridgeIoAllocateBuffer (
>    EFI_STATUS                Status;
>    EFI_PHYSICAL_ADDRESS      PhysicalAddress;
>    PCI_ROOT_BRIDGE_INSTANCE  *RootBridge;
> -  EFI_ALLOCATE_TYPE         AllocateType;
> +  UINTN                     Size;
>
>    //
>    // Validate Attributes
> @@ -1312,25 +1556,52 @@ RootBridgeIoAllocateBuffer (
>
>    RootBridge = ROOT_BRIDGE_FROM_THIS (This);
>
> -  AllocateType = AllocateAnyPages;
> +  PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (-1);
>    if (!RootBridge->DmaAbove4G ||
>        (Attributes & EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0) {
>      //
>      // Limit allocations to memory below 4GB
>      //
> -    AllocateType    = AllocateMaxAddress;
>      PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (SIZE_4GB - 1);
>    }
> -  Status = gBS->AllocatePages (
> -                  AllocateType,
> +  if (gIoMmuProtocol != NULL) {
> +    Size = EFI_PAGES_TO_SIZE(Pages);
> +    Size = ALIGN_VALUE(EFI_PAGES_TO_SIZE(Pages), mIoMmuPageSize);
> +    Pages = EFI_SIZE_TO_PAGES (Size);
> +  }
> +  Status = InternalAllocateAlignedPagesWithAllocateType (
> +                  AllocateMaxAddress,
>                    MemoryType,
>                    Pages,
> +                  mIoMmuPageSize,
>                    &PhysicalAddress
>                    );
>    if (!EFI_ERROR (Status)) {
>      *HostAddress = (VOID *) (UINTN) PhysicalAddress;
> +    return EFI_SUCCESS;
>    }
>
> +  if (gIoMmuProtocol != NULL) {
> +    //
> +    // Try to allocate AnyAddress here.
> +    //
> +    // We can try to allocate non-existing memory for below 4GiB address
> +    // and use IOMMU to remap it later.
> +    //
> +    PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (-1);
> +    Status = InternalAllocateAlignedPagesWithAllocateType (
> +               AllocateMaxAddress,
> +               MemoryType,
> +               Pages,
> +               mIoMmuPageSize,
> +               &PhysicalAddress
> +               );
> +    if (EFI_ERROR(Status)) {
> +      return Status;
> +    }
> +    *HostAddress = (VOID *) (UINTN) PhysicalAddress;
> +    return EFI_SUCCESS;
> +  }
>    return Status;
>  }
>
> @@ -1356,6 +1627,13 @@ RootBridgeIoFreeBuffer (
>    OUT VOID                             *HostAddress
>    )
>  {
> +  UINTN                     Size;
> +
> +  if (gIoMmuProtocol != NULL) {
> +    Size = EFI_PAGES_TO_SIZE(Pages);
> +    Size = ALIGN_VALUE(EFI_PAGES_TO_SIZE(Pages), mIoMmuPageSize);
> +    Pages = EFI_SIZE_TO_PAGES (Size);
> +  }
>    return gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress,
> Pages);  }
>
> --
> 2.7.4.windows.1

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: Add IOMMU support.
  2017-04-17 19:58   ` Duran, Leo
@ 2017-04-18  3:45     ` Yao, Jiewen
  0 siblings, 0 replies; 10+ messages in thread
From: Yao, Jiewen @ 2017-04-18  3:45 UTC (permalink / raw)
  To: Duran, Leo, edk2-devel@lists.01.org
  Cc: Ni, Ruiyu, Singh, Brijesh, Ard Biesheuvel

Yes, I agree. It is a little complicated.

NeedAllocateNonExisting is TRUE when:

A. IoMMU is present  (AND)

B. We need remap a Buffer  below 4GiB, but we cannot find enough memory there.

Because CommonBuffer and Read/WriteBuffer is handled in different way, we have to separate #B to be 2 cases.

B.1 We need remap a CommBuffer, because no below 4GiB CommBuffer returned in AllocateBuffer API. (OR)

B.2 We need remap a Read/Write buffer below 4GiB, but we cannot find enough memory there.

As a result: NeedAllocateNonExisting means (A AND (B.1 OR B.2))

Thank you
Yao Jiewen


From: Duran, Leo [mailto:leo.duran@amd.com]
Sent: Tuesday, April 18, 2017 3:58 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Singh, Brijesh <brijesh.singh@amd.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: RE: [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: Add IOMMU support.

Hi Yao,

Regarding RootBridgeIoMap()

I'm wondering if may be possible to simplify the logic requiring flags "NeedMap" and "NeedAllocateNonExisting"?
For example, it seems like (NeedAllocateNonExisting==TRUE) implies (gIoMmuProtocol != NULL), but that did not seem obvious at a glance.

Leo.

> -----Original Message-----
> From: Jiewen Yao [mailto:jiewen.yao@intel.com]
> Sent: Tuesday, April 04, 2017 2:06 AM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Duran, Leo <leo.duran@amd.com<mailto:leo.duran@amd.com>>;
> Singh, Brijesh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>
> Subject: [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: Add IOMMU support.
>
> The responsibility of PciBus driver is to set IOMMU attribute, because only
> PciBus knows which device submits DMA access request.
>
> PciBus driver guarantee that the request to PciHostBridge is IOMMU page
> aligned memory, as such PciHostBridge can allocate non-existent memory for
> device memory, to satisfy remap requirement.
>
> PciBus driver does not assume device address is same as the mapped host
> address, because IOMMU may remap it.
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>
> Cc: Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>
> Cc: Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c      |  12 ++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h      |  19 ++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf |   1 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c       | 225
> +++++++++++++++++++-
>  4 files changed, 247 insertions(+), 10 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> index f3be47a..c9ee4de 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> @@ -42,6 +42,8 @@ UINT64                                        gAllZero             = 0;
>
>  EFI_PCI_PLATFORM_PROTOCOL                     *gPciPlatformProtocol;
>  EFI_PCI_OVERRIDE_PROTOCOL                     *gPciOverrideProtocol;
> +EDKII_IOMMU_PROTOCOL                          *gIoMmuProtocol;
> +UINTN                                         mIoMmuPageSize = 1;
>
>
>  GLOBAL_REMOVE_IF_UNREFERENCED
> EFI_PCI_HOTPLUG_REQUEST_PROTOCOL mPciHotPlugRequest = { @@ -
> 256,6 +258,16 @@ PciBusDriverBindingStart (
>    }
>
>    gBS->LocateProtocol (
> +        &gEdkiiIoMmuProtocolGuid,
> +        NULL,
> +        (VOID **) &gIoMmuProtocol
> +        );
> +  if (gIoMmuProtocol != NULL) {
> +    gIoMmuProtocol->GetPageSize (gIoMmuProtocol, &mIoMmuPageSize);
> +    ASSERT ((mIoMmuPageSize & (mIoMmuPageSize - 1)) == 0);  }
> +
> +  gBS->LocateProtocol (
>           &gEfiIncompatiblePciDeviceSupportProtocolGuid,
>           NULL,
>           (VOID **) &gIncompatiblePciDeviceSupport diff --git
> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> index 39ba8b9..185d89c 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> @@ -32,6 +32,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Protocol/IncompatiblePciDeviceSupport.h>
>  #include <Protocol/PciOverride.h>
>  #include <Protocol/PciEnumerationComplete.h>
> +#include <Protocol/IoMmu.h>
>
>  #include <Library/DebugLib.h>
>  #include <Library/UefiDriverEntryPoint.h> @@ -289,6 +290,8 @@ struct
> _PCI_IO_DEVICE {
>    // This field is used to support this case.
>    //
>    UINT16                                    BridgeIoAlignment;
> +
> +  LIST_ENTRY                                Maps;
>  };
>
>  #define PCI_IO_DEVICE_FROM_PCI_IO_THIS(a) \ @@ -304,6 +307,20 @@
> struct _PCI_IO_DEVICE {
>    CR (a, PCI_IO_DEVICE, LoadFile2, PCI_IO_DEVICE_SIGNATURE)
>
>
> +#define PCI_IO_MAP_INFO_SIGNATURE  SIGNATURE_32 ('p', 'm', 'a', 'p')
> +typedef struct {
> +  UINT32                                    Signature;
> +  LIST_ENTRY                                Link;
> +  EFI_PCI_IO_PROTOCOL_OPERATION             Operation;
> +  VOID                                      *HostAddress;
> +  EFI_PHYSICAL_ADDRESS                      DeviceAddress;
> +  UINTN                                     NumberOfBytes;
> +  VOID                                      *AlignedHostAddress;
> +  UINTN                                     AlignedNumberOfBytes;
> +  VOID                                      *MappedHostAddress;
> +  VOID                                      *PciRootBridgeIoMapping;
> +} PCI_IO_MAP_INFO;
> +#define PCI_IO_MAP_INFO_FROM_LINK(a) CR (a, PCI_IO_MAP_INFO,
> Link,
> +PCI_IO_MAP_INFO_SIGNATURE)
>
>  //
>  // Global Variables
> @@ -321,6 +338,8 @@ extern EFI_PCI_PLATFORM_PROTOCOL
> *gPciPlatformProtocol;
>  extern EFI_PCI_OVERRIDE_PROTOCOL                    *gPciOverrideProtocol;
>  extern BOOLEAN                                      mReserveIsaAliases;
>  extern BOOLEAN                                      mReserveVgaAliases;
> +extern EDKII_IOMMU_PROTOCOL                         *gIoMmuProtocol;
> +extern UINTN                                        mIoMmuPageSize;
>
>  /**
>    Macro that checks whether device is a GFX device.
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> index a3ab11f..5da094f 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> @@ -95,6 +95,7 @@
>    gEfiPciRootBridgeIoProtocolGuid                 ## TO_START
>    gEfiIncompatiblePciDeviceSupportProtocolGuid    ##
> SOMETIMES_CONSUMES
>    gEfiLoadFile2ProtocolGuid                       ## SOMETIMES_PRODUCES
> +  gEdkiiIoMmuProtocolGuid                         ## SOMETIMES_CONSUMES
>
>  [FeaturePcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPciBusHotplugDeviceSupport      ##
> CONSUMES
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> index f72598d..31b8c32 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> @@ -58,6 +58,7 @@ InitializePciIoInstance (
>    )
>  {
>    CopyMem (&PciIoDevice->PciIo, &mPciIoInterface, sizeof
> (EFI_PCI_IO_PROTOCOL));
> +  InitializeListHead (&PciIoDevice->Maps);
>  }
>
>  /**
> @@ -936,6 +937,28 @@ PciIoCopyMem (
>  }
>
>  /**
> +  Return if a value is aligned.
> +
> +  @param Value the value to be checked
> +  @param Alignment the alignment to be checked with.
> +
> +  @retval TRUE  The value is aligned
> +  @retval FALSE The value is not aligned.
> +**/
> +BOOLEAN
> +InternalIsAlgined (
> +  IN UINTN  Value,
> +  IN UINTN  Alignment
> +  )
> +{
> +  if (Value == ALIGN_VALUE(Value, Alignment)) {
> +    return TRUE;
> +  } else {
> +    return FALSE;
> +  }
> +}
> +
> +/**
>    Provides the PCI controller-specific addresses needed to access system
> memory.
>
>    @param  This                  A pointer to the EFI_PCI_IO_PROTOCOL instance.
> @@ -967,6 +990,9 @@ PciIoMap (
>  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  PCI_IO_MAP_INFO       *PciIoMapInfo;
> +  UINT64                IoMmuAttribute;
> +  EFI_STATUS            RemapStatus;
>
>    PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
>
> @@ -982,15 +1008,60 @@ PciIoMap (
>      Operation = (EFI_PCI_IO_PROTOCOL_OPERATION) (Operation +
> EfiPciOperationBusMasterRead64);
>    }
>
> -  Status = PciIoDevice->PciRootBridgeIo->Map (
> -                                          PciIoDevice->PciRootBridgeIo,
> -                                          (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION)
> Operation,
> -                                          HostAddress,
> -                                          NumberOfBytes,
> -                                          DeviceAddress,
> -                                          Mapping
> -                                          );
> -
> +  if (gIoMmuProtocol != NULL) {
> +    PciIoMapInfo = AllocatePool (sizeof(*PciIoMapInfo));
> +    if (PciIoMapInfo == NULL) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +    PciIoMapInfo->Signature              = PCI_IO_MAP_INFO_SIGNATURE;
> +    PciIoMapInfo->Operation              = Operation;
> +    PciIoMapInfo->NumberOfBytes          = *NumberOfBytes;
> +    PciIoMapInfo->DeviceAddress          = *DeviceAddress;
> +    PciIoMapInfo->HostAddress            = HostAddress;
> +    //
> +    // For non common buffer, we always allocate a new memory if IOMMU
> exists.
> +    // because the original memory might not be DMA capable.
> +    //
> +    // For common buffer, it is not needed, because common buffer allocate
> via PciIoAllocateBuffer.
> +    // We cannot use AllocateAlignedPages here, because there might be
> more restriction in PciIoAllocateBuffer().
> +    //
> +    PciIoMapInfo->AlignedNumberOfBytes   = ALIGN_VALUE (PciIoMapInfo-
> >NumberOfBytes, mIoMmuPageSize);
> +    if (PciIoMapInfo->Operation !=
> EfiPciIoOperationBusMasterCommonBuffer) {
> +      PciIoMapInfo->AlignedHostAddress = AllocateAlignedPages
> (EFI_SIZE_TO_PAGES(PciIoMapInfo->AlignedNumberOfBytes),
> mIoMmuPageSize);
> +      if (PciIoMapInfo->AlignedHostAddress == NULL) {
> +        FreePool (PciIoMapInfo);
> +        return EFI_OUT_OF_RESOURCES;
> +      }
> +    } else {
> +      //
> +      // For common buffer, the HostAddress must be allocated via
> PciIoAllocateBuffer.
> +      //
> +      if (!InternalIsAlgined((UINTN)PciIoMapInfo->HostAddress,
> mIoMmuPageSize)) {
> +        FreePool (PciIoMapInfo);
> +        DEBUG ((DEBUG_ERROR, "PciIoMap - map unaligned common buffer
> with IOMMU\n"));
> +        return EFI_UNSUPPORTED;
> +      }
> +      PciIoMapInfo->AlignedHostAddress = PciIoMapInfo->HostAddress;
> +    }
> +    PciIoMapInfo->PciRootBridgeIoMapping = NULL;
> +    Status = PciIoDevice->PciRootBridgeIo->Map (
> +                                            PciIoDevice->PciRootBridgeIo,
> +
> (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION) Operation,
> +                                            PciIoMapInfo->AlignedHostAddress,
> +                                            &PciIoMapInfo->AlignedNumberOfBytes,
> +                                            &PciIoMapInfo->DeviceAddress,
> +                                            &PciIoMapInfo->PciRootBridgeIoMapping
> +                                            );  } else {
> +    Status = PciIoDevice->PciRootBridgeIo->Map (
> +                                            PciIoDevice->PciRootBridgeIo,
> +
> (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION) Operation,
> +                                            HostAddress,
> +                                            NumberOfBytes,
> +                                            DeviceAddress,
> +                                            Mapping
> +                                            );  }
>    if (EFI_ERROR (Status)) {
>      REPORT_STATUS_CODE_WITH_DEVICE_PATH (
>        EFI_ERROR_CODE | EFI_ERROR_MINOR, @@ -999,6 +1070,63 @@
> PciIoMap (
>        );
>    }
>
> +  if (gIoMmuProtocol != NULL) {
> +    if (EFI_ERROR(Status)) {
> +      if (PciIoMapInfo->Operation !=
> EfiPciIoOperationBusMasterCommonBuffer) {
> +        FreePages (PciIoMapInfo->AlignedHostAddress,
> EFI_SIZE_TO_PAGES(PciIoMapInfo->AlignedNumberOfBytes));
> +      }
> +      FreePool (PciIoMapInfo);
> +    } else {
> +      *DeviceAddress = PciIoMapInfo->DeviceAddress;
> +      *Mapping = PciIoMapInfo;
> +
> +      switch (Operation) {
> +      case EfiPciIoOperationBusMasterRead:
> +        IoMmuAttribute = EDKII_IOMMU_ATTRIBUTE_READ;
> +        break;
> +      case EfiPciIoOperationBusMasterWrite:
> +        IoMmuAttribute = EDKII_IOMMU_ATTRIBUTE_WRITE;
> +        break;
> +      case EfiPciIoOperationBusMasterCommonBuffer:
> +        IoMmuAttribute = EDKII_IOMMU_ATTRIBUTE_READ |
> EDKII_IOMMU_ATTRIBUTE_WRITE;
> +        break;
> +      default:
> +        ASSERT(FALSE);
> +        return EFI_INVALID_PARAMETER;
> +      }
> +      //
> +      // PciHostBridge should map IOMMU page aligned HostAddress.
> +      //
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        PciIoMapInfo->DeviceAddress,
> +                        PciIoMapInfo->AlignedNumberOfBytes,
> +                        IoMmuAttribute
> +                        );
> +      //
> +      // We need do copy mem after IoMmu->SetAttribute(),
> +      // because it might change IOMMU state.
> +      //
> +      RemapStatus = gIoMmuProtocol->GetRemapAddress (
> +                                      gIoMmuProtocol,
> +                                      PciIoDevice->Handle,
> +                                      PciIoMapInfo->DeviceAddress,
> +                                      &PciIoMapInfo->MappedHostAddress
> +                                      );
> +      if (EFI_ERROR(RemapStatus)) {
> +        PciIoMapInfo->MappedHostAddress = (VOID *)(UINTN)PciIoMapInfo-
> >DeviceAddress;
> +      }
> +      if (Operation == EfiPciIoOperationBusMasterRead) {
> +        CopyMem (
> +          PciIoMapInfo->MappedHostAddress,
> +          PciIoMapInfo->HostAddress,
> +          PciIoMapInfo->NumberOfBytes
> +          );
> +      }
> +      InsertTailList (&PciIoDevice->Maps, &PciIoMapInfo->Link);
> +    }
> +  }
>    return Status;
>  }
>
> @@ -1021,9 +1149,48 @@ PciIoUnmap (
>  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  PCI_IO_MAP_INFO *PciIoMapInfo;
> +  LIST_ENTRY      *Link;
>
>    PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
>
> +  PciIoMapInfo = NULL;
> +  if (gIoMmuProtocol != NULL) {
> +    PciIoMapInfo = NULL;
> +    for (Link = GetFirstNode (&PciIoDevice->Maps)
> +         ; !IsNull (&PciIoDevice->Maps, Link)
> +         ; Link = GetNextNode (&PciIoDevice->Maps, Link)
> +         ) {
> +      PciIoMapInfo = PCI_IO_MAP_INFO_FROM_LINK (Link);
> +      if (PciIoMapInfo == Mapping) {
> +        break;
> +      }
> +    }
> +    //
> +    // Mapping is not a valid value returned by Map()
> +    //
> +    if (PciIoMapInfo != Mapping) {
> +      DEBUG ((DEBUG_INFO, "PciIoUnmap - PciIoMapInfo not found!\n"));
> +      return EFI_INVALID_PARAMETER;
> +    }
> +    RemoveEntryList (&PciIoMapInfo->Link);
> +    Mapping = PciIoMapInfo->PciRootBridgeIoMapping;
> +
> +    //
> +    // PciHostBridge should map IOMMU page aligned HostAddress.
> +    //
> +    // We need do copy mem before PciRootBridgeIo->Unmap(),
> +    // because it might free mapped host address.
> +    //
> +    if (PciIoMapInfo->Operation == EfiPciIoOperationBusMasterWrite) {
> +      CopyMem (
> +        PciIoMapInfo->HostAddress,
> +        PciIoMapInfo->MappedHostAddress,
> +        PciIoMapInfo->NumberOfBytes
> +        );
> +    }
> +  }
> +
>    Status = PciIoDevice->PciRootBridgeIo->Unmap (
>                                            PciIoDevice->PciRootBridgeIo,
>                                            Mapping @@ -1037,6 +1204,25 @@ PciIoUnmap (
>        );
>    }
>
> +  if (gIoMmuProtocol != NULL) {
> +    if (!EFI_ERROR(Status)) {
> +      //
> +      // PciHostBridge should map IOMMU page aligned HostAddress.
> +      //
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        PciIoMapInfo->DeviceAddress,
> +                        PciIoMapInfo->AlignedNumberOfBytes,
> +                        0
> +                        );
> +      if (PciIoMapInfo->Operation !=
> EfiPciIoOperationBusMasterCommonBuffer) {
> +        FreePages (PciIoMapInfo->AlignedHostAddress,
> EFI_SIZE_TO_PAGES(PciIoMapInfo->AlignedNumberOfBytes));
> +      }
> +      FreePool (PciIoMapInfo);
> +    }
> +  }
> +
>    return Status;
>  }
>
> @@ -1073,6 +1259,7 @@ PciIoAllocateBuffer (  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  UINTN         Size;
>
>    if ((Attributes &
>        (~(EFI_PCI_ATTRIBUTE_MEMORY_WRITE_COMBINE |
> EFI_PCI_ATTRIBUTE_MEMORY_CACHED))) != 0){ @@ -1085,6 +1272,12 @@
> PciIoAllocateBuffer (
>      Attributes |= EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE;
>    }
>
> +  if (gIoMmuProtocol != NULL) {
> +    Size = EFI_PAGES_TO_SIZE(Pages);
> +    Size = ALIGN_VALUE(Size, mIoMmuPageSize);
> +    Pages = EFI_SIZE_TO_PAGES (Size);
> +  }
> +
>    Status = PciIoDevice->PciRootBridgeIo->AllocateBuffer (
>                                            PciIoDevice->PciRootBridgeIo,
>                                            Type, @@ -1101,7 +1294,9 @@ PciIoAllocateBuffer (
>        PciIoDevice->DevicePath
>        );
>    }
> -
> +  //
> +  // No need to set attribute here, it is done in Map.
> +  //
>    return Status;
>  }
>
> @@ -1127,9 +1322,16 @@ PciIoFreeBuffer (  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  UINTN         Size;
>
>    PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
>
> +  if (gIoMmuProtocol != NULL) {
> +    Size = EFI_PAGES_TO_SIZE(Pages);
> +    Size = ALIGN_VALUE(Size, mIoMmuPageSize);
> +    Pages = EFI_SIZE_TO_PAGES (Size);
> +  }
> +
>    Status = PciIoDevice->PciRootBridgeIo->FreeBuffer (
>                                            PciIoDevice->PciRootBridgeIo,
>                                            Pages, @@ -1144,6 +1346,9 @@ PciIoFreeBuffer (
>        );
>    }
>
> +  //
> +  // No need to set attribute here, it is done in Unmap.
> +  //
>    return Status;
>  }
>
> --
> 2.7.4.windows.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-04-18  3:45 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-04  7:06 [RFC] [PATCH V3 0/3] Add IOMMU support Jiewen Yao
2017-04-04  7:06 ` [RFC] [PATCH V3 1/3] MdeModulePkg/Include: Add IOMMU protocol definition Jiewen Yao
2017-04-17 13:42   ` Ard Biesheuvel
2017-04-17 15:13     ` Yao, Jiewen
2017-04-04  7:06 ` [RFC] [PATCH V3 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support Jiewen Yao
2017-04-17 19:33   ` Duran, Leo
2017-04-18  3:31     ` Yao, Jiewen
2017-04-04  7:06 ` [RFC] [PATCH V3 3/3] MdeModulePkg/PciBus: " Jiewen Yao
2017-04-17 19:58   ` Duran, Leo
2017-04-18  3:45     ` Yao, Jiewen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox