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

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>
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                    |  10 ++
 MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf               |   1 +
 MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c                     | 100 ++++++++++++
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c      |   3 +
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf |   1 +
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h      |   7 +
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 172 +++++++++++++++++++-
 MdeModulePkg/Include/Protocol/IoMmu.h                      | 132 +++++++++++++++
 MdeModulePkg/MdeModulePkg.dec                              |   3 +
 10 files changed, 436 insertions(+), 5 deletions(-)
 create mode 100644 MdeModulePkg/Include/Protocol/IoMmu.h

-- 
2.7.4.windows.1



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

* [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol definition.
  2017-03-25  9:28 [RFC] [PATCH 0/3] Add IOMMU support Jiewen Yao
@ 2017-03-25  9:28 ` Jiewen Yao
  2017-03-28 22:38   ` Ard Biesheuvel
  2017-03-25  9:28 ` [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support Jiewen Yao
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 23+ messages in thread
From: Jiewen Yao @ 2017-03-25  9:28 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ruiyu Ni, Leo Duran, Brijesh Singh

This protocol is to abstract IOMMU access.

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

diff --git a/MdeModulePkg/Include/Protocol/IoMmu.h b/MdeModulePkg/Include/Protocol/IoMmu.h
new file mode 100644
index 0000000..55b9c78
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/IoMmu.h
@@ -0,0 +1,132 @@
+/** @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
+
+/**
+  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 it is.
+  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  This              The protocol instance pointer.
+  @param  DeviceHandle      The device who initiates the DMA access request.
+  @param  BaseAddress       The base of system memory address to be used as the DMA memory.
+  @param  Length            The length of system memory address to be used as the DMA memory.
+  @param  IoMmuAttribute    The IOMMU attribute.
+
+  @retval EFI_SUCCESS            The IoMmuAttribute is set for the memory range specified by BaseAddress and Length.
+  @retval EFI_INVALID_PARAMETER  DeviceHandle is an invalid handle.
+  @retval EFI_INVALID_PARAMETER  BaseAddress 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 BaseAddress 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 UINT64                BaseAddress,
+  IN UINT64                Length,
+  IN UINT64                IoMmuAttribute
+  );
+
+/**
+  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  This      Protocol instance pointer.
+  @param  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
+  );
+
+///
+/// IOMMU Protocol structure.
+///
+struct _EDKII_IOMMU_PROTOCOL {
+  UINT64                       Revision;
+  EDKII_IOMMU_GET_PAGE_SIZE    GetPageSize;
+  EDKII_IOMMU_SET_ATTRIBUTE    SetAttribute;
+};
+
+///
+/// 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] 23+ messages in thread

* [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.
  2017-03-25  9:28 [RFC] [PATCH 0/3] Add IOMMU support Jiewen Yao
  2017-03-25  9:28 ` [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol definition Jiewen Yao
@ 2017-03-25  9:28 ` Jiewen Yao
  2017-03-27  5:31   ` Ni, Ruiyu
  2017-03-25  9:28 ` [RFC] [PATCH 3/3] MdeModulePkg/PciBus: " Jiewen Yao
  2017-03-27 19:50 ` [RFC] [PATCH 0/3] " Duran, Leo
  3 siblings, 1 reply; 23+ messages in thread
From: Jiewen Yao @ 2017-03-25  9:28 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ruiyu Ni, Leo Duran, Brijesh Singh

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.

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>
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      |   7 +
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 172 +++++++++++++++++++-
 4 files changed, 178 insertions(+), 5 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..4d21d10 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,6 +51,8 @@ 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;
 } MAP_INFO;
@@ -575,4 +578,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..47ea697 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 FALSE;
+  } else {
+    return FALSE;
+  }
+}
+
+/**
   Provides the PCI controller-specific address needed to access
   system memory for DMA.
 
@@ -1057,6 +1172,8 @@ RootBridgeIoMap (
   PCI_ROOT_BRIDGE_INSTANCE                          *RootBridge;
   EFI_PHYSICAL_ADDRESS                              PhysicalAddress;
   MAP_INFO                                          *MapInfo;
+  BOOLEAN                                           NeedMap;
+  EFI_PHYSICAL_ADDRESS                              MaxAddress;
 
   if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
       Mapping == NULL) {
@@ -1072,12 +1189,40 @@ 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;
+
+  NeedMap = FALSE;
+  MaxAddress = (UINT64)-1;
+
   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
@@ -1113,15 +1258,18 @@ 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);
 
     //
     // Allocate a buffer below 4GB to map the transfer to.
     //
-    Status = gBS->AllocatePages (
+    Status = InternalAllocateAlignedPagesWithAllocateType (
                     AllocateMaxAddress,
                     EfiBootServicesData,
-                    MapInfo->NumberOfPages,
+                    MapInfo->MappedNumberOfPages,
+                    mIoMmuPageSize,
                     &MapInfo->MappedHostAddress
                     );
     if (EFI_ERROR (Status)) {
@@ -1240,7 +1388,7 @@ RootBridgeIoUnmap (
   //
   // 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;
 }
@@ -1286,6 +1434,7 @@ RootBridgeIoAllocateBuffer (
   EFI_PHYSICAL_ADDRESS      PhysicalAddress;
   PCI_ROOT_BRIDGE_INSTANCE  *RootBridge;
   EFI_ALLOCATE_TYPE         AllocateType;
+  UINTN                     Size;
 
   //
   // Validate Attributes
@@ -1321,10 +1470,16 @@ RootBridgeIoAllocateBuffer (
     AllocateType    = AllocateMaxAddress;
     PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (SIZE_4GB - 1);
   }
-  Status = gBS->AllocatePages (
+  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 (
                   AllocateType,
                   MemoryType,
                   Pages,
+                  mIoMmuPageSize,
                   &PhysicalAddress
                   );
   if (!EFI_ERROR (Status)) {
@@ -1356,6 +1511,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] 23+ messages in thread

* [RFC] [PATCH 3/3] MdeModulePkg/PciBus: Add IOMMU support.
  2017-03-25  9:28 [RFC] [PATCH 0/3] Add IOMMU support Jiewen Yao
  2017-03-25  9:28 ` [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol definition Jiewen Yao
  2017-03-25  9:28 ` [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support Jiewen Yao
@ 2017-03-25  9:28 ` Jiewen Yao
  2017-03-27  5:39   ` Ni, Ruiyu
  2017-03-27 19:50 ` [RFC] [PATCH 0/3] " Duran, Leo
  3 siblings, 1 reply; 23+ messages in thread
From: Jiewen Yao @ 2017-03-25  9:28 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ruiyu Ni, Leo Duran, Brijesh Singh

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

PciBus driver assumes that PciHostBridge driver can allocate
IOMMU page aligned memory, if IOMMU protocol exists.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Leo Duran <leo.duran@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
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      |  10 ++
 MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf |   1 +
 MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c       | 100 ++++++++++++++++++++
 4 files changed, 123 insertions(+)

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..6f96696 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>
@@ -304,6 +305,13 @@ 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;
+  UINTN                                     NumberOfBytes;
+  EFI_PHYSICAL_ADDRESS                      DeviceAddress;
+  VOID                                      *PciRootBridgeIoMapping;
+} PCI_IO_MAP_INFO;
 
 //
 // Global Variables
@@ -319,6 +327,8 @@ extern UINT64                                       gAllOne;
 extern UINT64                                       gAllZero;
 extern EFI_PCI_PLATFORM_PROTOCOL                    *gPciPlatformProtocol;
 extern EFI_PCI_OVERRIDE_PROTOCOL                    *gPciOverrideProtocol;
+extern EDKII_IOMMU_PROTOCOL                         *gIoMmuProtocol;
+extern UINTN                                        mIoMmuPageSize;
 extern BOOLEAN                                      mReserveIsaAliases;
 extern BOOLEAN                                      mReserveVgaAliases;
 
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..01786c1 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
@@ -967,6 +967,8 @@ PciIoMap (
 {
   EFI_STATUS    Status;
   PCI_IO_DEVICE *PciIoDevice;
+  PCI_IO_MAP_INFO       *PciIoMapInfo;
+  UINT64                IoMmuAttribute;
 
   PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
 
@@ -999,6 +1001,46 @@ PciIoMap (
       );
   }
 
+  if (gIoMmuProtocol != NULL) {
+    if (!EFI_ERROR(Status)) {
+      PciIoMapInfo = AllocatePool (sizeof(*PciIoMapInfo));
+      if (PciIoMapInfo == NULL) {
+        PciIoDevice->PciRootBridgeIo->Unmap (PciIoDevice->PciRootBridgeIo, *Mapping);
+        return EFI_OUT_OF_RESOURCES;
+      }
+
+      PciIoMapInfo->Signature              = PCI_IO_MAP_INFO_SIGNATURE;
+      PciIoMapInfo->NumberOfBytes          = *NumberOfBytes;
+      PciIoMapInfo->DeviceAddress          = *DeviceAddress;
+      PciIoMapInfo->PciRootBridgeIoMapping = *Mapping;
+      *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 Status;
+      }
+      //
+      // PciHostBridge should map IOMMU page aligned HostAddress.
+      //
+      gIoMmuProtocol->SetAttribute (
+                        gIoMmuProtocol,
+                        PciIoDevice->Handle,
+                        PciIoMapInfo->DeviceAddress,
+                        ALIGN_VALUE(PciIoMapInfo->NumberOfBytes, mIoMmuPageSize),
+                        IoMmuAttribute
+                        );
+    }
+  }
   return Status;
 }
 
@@ -1021,9 +1063,19 @@ PciIoUnmap (
 {
   EFI_STATUS    Status;
   PCI_IO_DEVICE *PciIoDevice;
+  PCI_IO_MAP_INFO *PciIoMapInfo;
 
   PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
 
+  PciIoMapInfo = NULL;
+  if (gIoMmuProtocol != NULL) {
+    PciIoMapInfo = Mapping;
+    if (PciIoMapInfo->Signature != PCI_IO_MAP_INFO_SIGNATURE) {
+      return EFI_INVALID_PARAMETER;
+    }
+    Mapping = PciIoMapInfo->PciRootBridgeIoMapping;
+  }
+
   Status = PciIoDevice->PciRootBridgeIo->Unmap (
                                           PciIoDevice->PciRootBridgeIo,
                                           Mapping
@@ -1037,6 +1089,22 @@ PciIoUnmap (
       );
   }
 
+  if (gIoMmuProtocol != NULL) {
+    if (!EFI_ERROR(Status)) {
+      //
+      // PciHostBridge should map IOMMU page aligned HostAddress.
+      //
+      gIoMmuProtocol->SetAttribute (
+                        gIoMmuProtocol,
+                        PciIoDevice->Handle,
+                        PciIoMapInfo->DeviceAddress,
+                        ALIGN_VALUE(PciIoMapInfo->NumberOfBytes, mIoMmuPageSize),
+                        0
+                        );
+      FreePool (PciIoMapInfo);
+    }
+  }
+
   return Status;
 }
 
@@ -1073,6 +1141,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){
@@ -1102,6 +1171,21 @@ PciIoAllocateBuffer (
       );
   }
 
+  if (gIoMmuProtocol != NULL) {
+    if (!EFI_ERROR(Status)) {
+      //
+      // PciHostBridge should allocate IOMMU page aligned HostAddress.
+      //
+      Size = EFI_PAGES_TO_SIZE(Pages);
+      gIoMmuProtocol->SetAttribute (
+                        gIoMmuProtocol,
+                        PciIoDevice->Handle,
+                        (UINTN)*HostAddress,
+                        ALIGN_VALUE(Size, mIoMmuPageSize),
+                        EDKII_IOMMU_ATTRIBUTE_READ | EDKII_IOMMU_ATTRIBUTE_WRITE
+                        );
+    }
+  }
   return Status;
 }
 
@@ -1127,6 +1211,7 @@ PciIoFreeBuffer (
 {
   EFI_STATUS    Status;
   PCI_IO_DEVICE *PciIoDevice;
+  UINTN         Size;
 
   PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
 
@@ -1144,6 +1229,21 @@ PciIoFreeBuffer (
       );
   }
 
+  if (gIoMmuProtocol != NULL) {
+    if (!EFI_ERROR(Status)) {
+      //
+      // PciHostBridge should allocate IOMMU page aligned HostAddress.
+      //
+      Size = EFI_PAGES_TO_SIZE(Pages);
+      gIoMmuProtocol->SetAttribute (
+                        gIoMmuProtocol,
+                        PciIoDevice->Handle,
+                        (UINTN)HostAddress,
+                        ALIGN_VALUE(Size, mIoMmuPageSize),
+                        0
+                        );
+    }
+  }
   return Status;
 }
 
-- 
2.7.4.windows.1



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

* Re: [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.
  2017-03-25  9:28 ` [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support Jiewen Yao
@ 2017-03-27  5:31   ` Ni, Ruiyu
  2017-03-27  6:01     ` Yao, Jiewen
  0 siblings, 1 reply; 23+ messages in thread
From: Ni, Ruiyu @ 2017-03-27  5:31 UTC (permalink / raw)
  To: Yao, Jiewen, edk2-devel@lists.01.org; +Cc: Leo Duran, Brijesh Singh

Jiewen,
#1. In RootBridgeIoMap(), NeedMap is TRUE when:
 1). Host or device doesn't support above 4GB DMA access
Or
 2). The memory size is not multiple of IO MMU page size

For case 2), we only need to call BS.AllocatePages with AllocateAnyAddress allocation type,
but I saw you still use AllocateMaxAddress allocation type.
Can we guarantee that (AllocateMaxAddress plus MaxAddress = MAX_UINT64) equals to
(AllocateAnyAddress)? I thought the first combination requires to allocate from very top
physical memory, but the second (AllocateAnyAddress) doesn't have such requirement.

#2. In InternalAllocateAlignedPagesWithAllocateType ():
      #a). AlignedMemory  = ((UINTN) Memory + AlignmentMask) & ~AlignmentMask;
      Can we use ALIGNED_POINTER() macro?
      #b). I think we do not need to free last unaligned pages. Because we only over-allocate
      several pages in the beginning. Please correct me if I am wrong.
      #c). The function name is too long. Can we rename it to IAllocatePages()? The parameter
      Type and Alignment indicate the allocation honors Type and Alignment.



Thanks/Ray

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Saturday, March 25, 2017 5:29 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Leo Duran <leo.duran@amd.com>;
> Brijesh Singh <brijesh.singh@amd.com>
> Subject: [RFC] [PATCH 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.
> 
> 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>
> 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      |   7 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 172
> +++++++++++++++++++-
>  4 files changed, 178 insertions(+), 5 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..4d21d10 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,6 +51,8 @@ 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;
>  } MAP_INFO;
> @@ -575,4 +578,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..47ea697 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 FALSE;
> +  } else {
> +    return FALSE;
> +  }
> +}
> +
> +/**
>    Provides the PCI controller-specific address needed to access
>    system memory for DMA.
> 
> @@ -1057,6 +1172,8 @@ RootBridgeIoMap (
>    PCI_ROOT_BRIDGE_INSTANCE                          *RootBridge;
>    EFI_PHYSICAL_ADDRESS                              PhysicalAddress;
>    MAP_INFO                                          *MapInfo;
> +  BOOLEAN                                           NeedMap;
> +  EFI_PHYSICAL_ADDRESS                              MaxAddress;
> 
>    if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress ==
> NULL ||
>        Mapping == NULL) {
> @@ -1072,12 +1189,40 @@ 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;
> +
> +  NeedMap = FALSE;
> +  MaxAddress = (UINT64)-1;
> +
>    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
> @@ -1113,15 +1258,18 @@ 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);
> 
>      //
>      // Allocate a buffer below 4GB to map the transfer to.
>      //
> -    Status = gBS->AllocatePages (
> +    Status = InternalAllocateAlignedPagesWithAllocateType (
>                      AllocateMaxAddress,
>                      EfiBootServicesData,
> -                    MapInfo->NumberOfPages,
> +                    MapInfo->MappedNumberOfPages,
> +                    mIoMmuPageSize,
>                      &MapInfo->MappedHostAddress
>                      );
>      if (EFI_ERROR (Status)) {
> @@ -1240,7 +1388,7 @@ RootBridgeIoUnmap (
>    //
>    // 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;
>  }
> @@ -1286,6 +1434,7 @@ RootBridgeIoAllocateBuffer (
>    EFI_PHYSICAL_ADDRESS      PhysicalAddress;
>    PCI_ROOT_BRIDGE_INSTANCE  *RootBridge;
>    EFI_ALLOCATE_TYPE         AllocateType;
> +  UINTN                     Size;
> 
>    //
>    // Validate Attributes
> @@ -1321,10 +1470,16 @@ RootBridgeIoAllocateBuffer (
>      AllocateType    = AllocateMaxAddress;
>      PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (SIZE_4GB - 1);
>    }
> -  Status = gBS->AllocatePages (
> +  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 (
>                    AllocateType,
>                    MemoryType,
>                    Pages,
> +                  mIoMmuPageSize,
>                    &PhysicalAddress
>                    );
>    if (!EFI_ERROR (Status)) {
> @@ -1356,6 +1511,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] 23+ messages in thread

* Re: [RFC] [PATCH 3/3] MdeModulePkg/PciBus: Add IOMMU support.
  2017-03-25  9:28 ` [RFC] [PATCH 3/3] MdeModulePkg/PciBus: " Jiewen Yao
@ 2017-03-27  5:39   ` Ni, Ruiyu
  2017-03-27  6:15     ` Yao, Jiewen
  0 siblings, 1 reply; 23+ messages in thread
From: Ni, Ruiyu @ 2017-03-27  5:39 UTC (permalink / raw)
  To: Yao, Jiewen, edk2-devel@lists.01.org; +Cc: Leo Duran, Brijesh Singh

#1. If PciHostBridge driver doesn't consume IOMMU protocol, will the change in PciBus driver cause any
       negative impact? For example, causing system hang? Do we need to introduce a bit to let PciHostBridge
       report the capability bit about IOMMU through PciHostBridge.GetAllocationAttribute()?
#2, Can you add a Maps field into the PciIoDevice structure? So that the PciIoMapInfo can be inserted to
       the list and we can check whether the Mapping is invalid by checking the existence in the list.
       This is better than checking the memory signature (PciIoMapInfo->Signature).
#3.  I saw PciIo.Map/Allocate sets the IOMMU attribute and Unmap/Free clears the IOMMU attribute.
        Do we have the case that the original IOMMU attribute is not 0? If that may happen, we may need to
        have an additional interface in IOMMU protocol to get the original attribute.

Thanks/Ray

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Saturday, March 25, 2017 5:29 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Leo Duran <leo.duran@amd.com>;
> Brijesh Singh <brijesh.singh@amd.com>
> Subject: [RFC] [PATCH 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 assumes that PciHostBridge driver can allocate IOMMU page
> aligned memory, if IOMMU protocol exists.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Leo Duran <leo.duran@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> 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      |  10 ++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf |   1 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c       | 100
> ++++++++++++++++++++
>  4 files changed, 123 insertions(+)
> 
> 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..6f96696 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> @@ -304,6 +305,13 @@ 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;
> +  UINTN                                     NumberOfBytes;
> +  EFI_PHYSICAL_ADDRESS                      DeviceAddress;
> +  VOID                                      *PciRootBridgeIoMapping;
> +} PCI_IO_MAP_INFO;
> 
>  //
>  // Global Variables
> @@ -319,6 +327,8 @@ extern UINT64                                       gAllOne;
>  extern UINT64                                       gAllZero;
>  extern EFI_PCI_PLATFORM_PROTOCOL                    *gPciPlatformProtocol;
>  extern EFI_PCI_OVERRIDE_PROTOCOL                    *gPciOverrideProtocol;
> +extern EDKII_IOMMU_PROTOCOL                         *gIoMmuProtocol;
> +extern UINTN                                        mIoMmuPageSize;
>  extern BOOLEAN                                      mReserveIsaAliases;
>  extern BOOLEAN                                      mReserveVgaAliases;
> 
> 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..01786c1 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> @@ -967,6 +967,8 @@ PciIoMap (
>  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  PCI_IO_MAP_INFO       *PciIoMapInfo;
> +  UINT64                IoMmuAttribute;
> 
>    PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
> 
> @@ -999,6 +1001,46 @@ PciIoMap (
>        );
>    }
> 
> +  if (gIoMmuProtocol != NULL) {
> +    if (!EFI_ERROR(Status)) {
> +      PciIoMapInfo = AllocatePool (sizeof(*PciIoMapInfo));
> +      if (PciIoMapInfo == NULL) {
> +        PciIoDevice->PciRootBridgeIo->Unmap (PciIoDevice->PciRootBridgeIo,
> *Mapping);
> +        return EFI_OUT_OF_RESOURCES;
> +      }
> +
> +      PciIoMapInfo->Signature              = PCI_IO_MAP_INFO_SIGNATURE;
> +      PciIoMapInfo->NumberOfBytes          = *NumberOfBytes;
> +      PciIoMapInfo->DeviceAddress          = *DeviceAddress;
> +      PciIoMapInfo->PciRootBridgeIoMapping = *Mapping;
> +      *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 Status;
> +      }
> +      //
> +      // PciHostBridge should map IOMMU page aligned HostAddress.
> +      //
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        PciIoMapInfo->DeviceAddress,
> +                        ALIGN_VALUE(PciIoMapInfo->NumberOfBytes,
> mIoMmuPageSize),
> +                        IoMmuAttribute
> +                        );
> +    }
> +  }
>    return Status;
>  }
> 
> @@ -1021,9 +1063,19 @@ PciIoUnmap (
>  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  PCI_IO_MAP_INFO *PciIoMapInfo;
> 
>    PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
> 
> +  PciIoMapInfo = NULL;
> +  if (gIoMmuProtocol != NULL) {
> +    PciIoMapInfo = Mapping;
> +    if (PciIoMapInfo->Signature != PCI_IO_MAP_INFO_SIGNATURE) {
> +      return EFI_INVALID_PARAMETER;
> +    }
> +    Mapping = PciIoMapInfo->PciRootBridgeIoMapping;
> +  }
> +
>    Status = PciIoDevice->PciRootBridgeIo->Unmap (
>                                            PciIoDevice->PciRootBridgeIo,
>                                            Mapping @@ -1037,6 +1089,22 @@ PciIoUnmap (
>        );
>    }
> 
> +  if (gIoMmuProtocol != NULL) {
> +    if (!EFI_ERROR(Status)) {
> +      //
> +      // PciHostBridge should map IOMMU page aligned HostAddress.
> +      //
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        PciIoMapInfo->DeviceAddress,
> +                        ALIGN_VALUE(PciIoMapInfo->NumberOfBytes,
> mIoMmuPageSize),
> +                        0
> +                        );
> +      FreePool (PciIoMapInfo);
> +    }
> +  }
> +
>    return Status;
>  }
> 
> @@ -1073,6 +1141,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){ @@ -1102,6 +1171,21 @@
> PciIoAllocateBuffer (
>        );
>    }
> 
> +  if (gIoMmuProtocol != NULL) {
> +    if (!EFI_ERROR(Status)) {
> +      //
> +      // PciHostBridge should allocate IOMMU page aligned HostAddress.
> +      //
> +      Size = EFI_PAGES_TO_SIZE(Pages);
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        (UINTN)*HostAddress,
> +                        ALIGN_VALUE(Size, mIoMmuPageSize),
> +                        EDKII_IOMMU_ATTRIBUTE_READ |
> EDKII_IOMMU_ATTRIBUTE_WRITE
> +                        );
> +    }
> +  }
>    return Status;
>  }
> 
> @@ -1127,6 +1211,7 @@ PciIoFreeBuffer (
>  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  UINTN         Size;
> 
>    PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
> 
> @@ -1144,6 +1229,21 @@ PciIoFreeBuffer (
>        );
>    }
> 
> +  if (gIoMmuProtocol != NULL) {
> +    if (!EFI_ERROR(Status)) {
> +      //
> +      // PciHostBridge should allocate IOMMU page aligned HostAddress.
> +      //
> +      Size = EFI_PAGES_TO_SIZE(Pages);
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        (UINTN)HostAddress,
> +                        ALIGN_VALUE(Size, mIoMmuPageSize),
> +                        0
> +                        );
> +    }
> +  }
>    return Status;
>  }
> 
> --
> 2.7.4.windows.1



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

* Re: [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.
  2017-03-27  5:31   ` Ni, Ruiyu
@ 2017-03-27  6:01     ` Yao, Jiewen
  2017-03-27  6:18       ` Ni, Ruiyu
  0 siblings, 1 reply; 23+ messages in thread
From: Yao, Jiewen @ 2017-03-27  6:01 UTC (permalink / raw)
  To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Leo Duran, Brijesh Singh

Answer below:

From: Ni, Ruiyu
Sent: Monday, March 27, 2017 1:32 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
Cc: Leo Duran <leo.duran@amd.com>; Brijesh Singh <brijesh.singh@amd.com>
Subject: RE: [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.

Jiewen,
#1. In RootBridgeIoMap(), NeedMap is TRUE when:
 1). Host or device doesn't support above 4GB DMA access
Or
 2). The memory size is not multiple of IO MMU page size

For case 2), we only need to call BS.AllocatePages with AllocateAnyAddress allocation type,
but I saw you still use AllocateMaxAddress allocation type.
Can we guarantee that (AllocateMaxAddress plus MaxAddress = MAX_UINT64) equals to
(AllocateAnyAddress)?
[Jiewen] Yes.

 I thought the first combination requires to allocate from very top
physical memory, but the second (AllocateAnyAddress) doesn't have such requirement.
[Jiewen] That is not true.
UEFI spec: "Allocation requests of Type AllocateMaxAddress allocate any available range of pages whose uppermost address is less than or equal to the address pointed to by Memory on input."
It means "any available range".




#2. In InternalAllocateAlignedPagesWithAllocateType ():
      #a). AlignedMemory  = ((UINTN) Memory + AlignmentMask) & ~AlignmentMask;
      Can we use ALIGNED_POINTER() macro?
      #b). I think we do not need to free last unaligned pages. Because we only over-allocate
      several pages in the beginning. Please correct me if I am wrong.
      #c). The function name is too long. Can we rename it to IAllocatePages()? The parameter
      Type and Alignment indicate the allocation honors Type and Alignment.
[Jiewen] I copied the code from MemoryAllocationLib.





Thanks/Ray

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Saturday, March 25, 2017 5:29 PM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>;
> Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
> Subject: [RFC] [PATCH 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.
>
> 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>>
> 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      |   7 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 172
> +++++++++++++++++++-
>  4 files changed, 178 insertions(+), 5 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..4d21d10 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,6 +51,8 @@ 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;
>  } MAP_INFO;
> @@ -575,4 +578,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..47ea697 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 FALSE;
> +  } else {
> +    return FALSE;
> +  }
> +}
> +
> +/**
>    Provides the PCI controller-specific address needed to access
>    system memory for DMA.
>
> @@ -1057,6 +1172,8 @@ RootBridgeIoMap (
>    PCI_ROOT_BRIDGE_INSTANCE                          *RootBridge;
>    EFI_PHYSICAL_ADDRESS                              PhysicalAddress;
>    MAP_INFO                                          *MapInfo;
> +  BOOLEAN                                           NeedMap;
> +  EFI_PHYSICAL_ADDRESS                              MaxAddress;
>
>    if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress ==
> NULL ||
>        Mapping == NULL) {
> @@ -1072,12 +1189,40 @@ 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;
> +
> +  NeedMap = FALSE;
> +  MaxAddress = (UINT64)-1;
> +
>    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
> @@ -1113,15 +1258,18 @@ 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);
>
>      //
>      // Allocate a buffer below 4GB to map the transfer to.
>      //
> -    Status = gBS->AllocatePages (
> +    Status = InternalAllocateAlignedPagesWithAllocateType (
>                      AllocateMaxAddress,
>                      EfiBootServicesData,
> -                    MapInfo->NumberOfPages,
> +                    MapInfo->MappedNumberOfPages,
> +                    mIoMmuPageSize,
>                      &MapInfo->MappedHostAddress
>                      );
>      if (EFI_ERROR (Status)) {
> @@ -1240,7 +1388,7 @@ RootBridgeIoUnmap (
>    //
>    // 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;
>  }
> @@ -1286,6 +1434,7 @@ RootBridgeIoAllocateBuffer (
>    EFI_PHYSICAL_ADDRESS      PhysicalAddress;
>    PCI_ROOT_BRIDGE_INSTANCE  *RootBridge;
>    EFI_ALLOCATE_TYPE         AllocateType;
> +  UINTN                     Size;
>
>    //
>    // Validate Attributes
> @@ -1321,10 +1470,16 @@ RootBridgeIoAllocateBuffer (
>      AllocateType    = AllocateMaxAddress;
>      PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (SIZE_4GB - 1);
>    }
> -  Status = gBS->AllocatePages (
> +  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 (
>                    AllocateType,
>                    MemoryType,
>                    Pages,
> +                  mIoMmuPageSize,
>                    &PhysicalAddress
>                    );
>    if (!EFI_ERROR (Status)) {
> @@ -1356,6 +1511,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] 23+ messages in thread

* Re: [RFC] [PATCH 3/3] MdeModulePkg/PciBus: Add IOMMU support.
  2017-03-27  5:39   ` Ni, Ruiyu
@ 2017-03-27  6:15     ` Yao, Jiewen
  2017-03-27  6:23       ` Ni, Ruiyu
  0 siblings, 1 reply; 23+ messages in thread
From: Yao, Jiewen @ 2017-03-27  6:15 UTC (permalink / raw)
  To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Leo Duran, Brijesh Singh

Answer below:

From: Ni, Ruiyu
Sent: Monday, March 27, 2017 1:40 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
Cc: Leo Duran <leo.duran@amd.com>; Brijesh Singh <brijesh.singh@amd.com>
Subject: RE: [RFC] [PATCH 3/3] MdeModulePkg/PciBus: Add IOMMU support.

#1. If PciHostBridge driver doesn't consume IOMMU protocol, will the change in PciBus driver cause any
       negative impact? For example, causing system hang? Do we need to introduce a bit to let PciHostBridge
       report the capability bit about IOMMU through PciHostBridge.GetAllocationAttribute()?
[Jiewen] That is a very good question.

I thought it before, but I finally give up the idea to define capability bit, because I want to make things simple.
I assume the system integration engineer should carefully pick up all drivers and validate the combination.

If the system integration engineer choose to integrate IOMMU driver, he/she should also choose a PciHostBridge which IOMMU capability.

I do not deny the goodness of such capability. But I want to add them later until we see the real usage.



#2, Can you add a Maps field into the PciIoDevice structure? So that the PciIoMapInfo can be inserted to
       the list and we can check whether the Mapping is invalid by checking the existence in the list.
       This is better than checking the memory signature (PciIoMapInfo->Signature).
[Jiewen] I am not clear about this.
Can you provide some sample code to show your idea?
Maybe just submit a patch based upon current code. If it is good, I can integrate the patch directly.


#3.  I saw PciIo.Map/Allocate sets the IOMMU attribute and Unmap/Free clears the IOMMU attribute.
        Do we have the case that the original IOMMU attribute is not 0? If that may happen, we may need to
        have an additional interface in IOMMU protocol to get the original attribute.
[Jiewen] Good question again.
I added below word in IOMMU protocol header file.

Allocation success mean no one use this memory before, so that it should be DMA blocked.


+/**

+  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).





Thanks/Ray

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Saturday, March 25, 2017 5:29 PM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>;
> Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
> Subject: [RFC] [PATCH 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 assumes that PciHostBridge driver can allocate IOMMU page
> aligned memory, if IOMMU protocol exists.
>
> 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>>
> 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      |  10 ++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf |   1 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c       | 100
> ++++++++++++++++++++
>  4 files changed, 123 insertions(+)
>
> 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..6f96696 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> @@ -304,6 +305,13 @@ 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;
> +  UINTN                                     NumberOfBytes;
> +  EFI_PHYSICAL_ADDRESS                      DeviceAddress;
> +  VOID                                      *PciRootBridgeIoMapping;
> +} PCI_IO_MAP_INFO;
>
>  //
>  // Global Variables
> @@ -319,6 +327,8 @@ extern UINT64                                       gAllOne;
>  extern UINT64                                       gAllZero;
>  extern EFI_PCI_PLATFORM_PROTOCOL                    *gPciPlatformProtocol;
>  extern EFI_PCI_OVERRIDE_PROTOCOL                    *gPciOverrideProtocol;
> +extern EDKII_IOMMU_PROTOCOL                         *gIoMmuProtocol;
> +extern UINTN                                        mIoMmuPageSize;
>  extern BOOLEAN                                      mReserveIsaAliases;
>  extern BOOLEAN                                      mReserveVgaAliases;
>
> 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..01786c1 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> @@ -967,6 +967,8 @@ PciIoMap (
>  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  PCI_IO_MAP_INFO       *PciIoMapInfo;
> +  UINT64                IoMmuAttribute;
>
>    PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
>
> @@ -999,6 +1001,46 @@ PciIoMap (
>        );
>    }
>
> +  if (gIoMmuProtocol != NULL) {
> +    if (!EFI_ERROR(Status)) {
> +      PciIoMapInfo = AllocatePool (sizeof(*PciIoMapInfo));
> +      if (PciIoMapInfo == NULL) {
> +        PciIoDevice->PciRootBridgeIo->Unmap (PciIoDevice->PciRootBridgeIo,
> *Mapping);
> +        return EFI_OUT_OF_RESOURCES;
> +      }
> +
> +      PciIoMapInfo->Signature              = PCI_IO_MAP_INFO_SIGNATURE;
> +      PciIoMapInfo->NumberOfBytes          = *NumberOfBytes;
> +      PciIoMapInfo->DeviceAddress          = *DeviceAddress;
> +      PciIoMapInfo->PciRootBridgeIoMapping = *Mapping;
> +      *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 Status;
> +      }
> +      //
> +      // PciHostBridge should map IOMMU page aligned HostAddress.
> +      //
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        PciIoMapInfo->DeviceAddress,
> +                        ALIGN_VALUE(PciIoMapInfo->NumberOfBytes,
> mIoMmuPageSize),
> +                        IoMmuAttribute
> +                        );
> +    }
> +  }
>    return Status;
>  }
>
> @@ -1021,9 +1063,19 @@ PciIoUnmap (
>  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  PCI_IO_MAP_INFO *PciIoMapInfo;
>
>    PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
>
> +  PciIoMapInfo = NULL;
> +  if (gIoMmuProtocol != NULL) {
> +    PciIoMapInfo = Mapping;
> +    if (PciIoMapInfo->Signature != PCI_IO_MAP_INFO_SIGNATURE) {
> +      return EFI_INVALID_PARAMETER;
> +    }
> +    Mapping = PciIoMapInfo->PciRootBridgeIoMapping;
> +  }
> +
>    Status = PciIoDevice->PciRootBridgeIo->Unmap (
>                                            PciIoDevice->PciRootBridgeIo,
>                                            Mapping @@ -1037,6 +1089,22 @@ PciIoUnmap (
>        );
>    }
>
> +  if (gIoMmuProtocol != NULL) {
> +    if (!EFI_ERROR(Status)) {
> +      //
> +      // PciHostBridge should map IOMMU page aligned HostAddress.
> +      //
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        PciIoMapInfo->DeviceAddress,
> +                        ALIGN_VALUE(PciIoMapInfo->NumberOfBytes,
> mIoMmuPageSize),
> +                        0
> +                        );
> +      FreePool (PciIoMapInfo);
> +    }
> +  }
> +
>    return Status;
>  }
>
> @@ -1073,6 +1141,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){ @@ -1102,6 +1171,21 @@
> PciIoAllocateBuffer (
>        );
>    }
>
> +  if (gIoMmuProtocol != NULL) {
> +    if (!EFI_ERROR(Status)) {
> +      //
> +      // PciHostBridge should allocate IOMMU page aligned HostAddress.
> +      //
> +      Size = EFI_PAGES_TO_SIZE(Pages);
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        (UINTN)*HostAddress,
> +                        ALIGN_VALUE(Size, mIoMmuPageSize),
> +                        EDKII_IOMMU_ATTRIBUTE_READ |
> EDKII_IOMMU_ATTRIBUTE_WRITE
> +                        );
> +    }
> +  }
>    return Status;
>  }
>
> @@ -1127,6 +1211,7 @@ PciIoFreeBuffer (
>  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  UINTN         Size;
>
>    PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
>
> @@ -1144,6 +1229,21 @@ PciIoFreeBuffer (
>        );
>    }
>
> +  if (gIoMmuProtocol != NULL) {
> +    if (!EFI_ERROR(Status)) {
> +      //
> +      // PciHostBridge should allocate IOMMU page aligned HostAddress.
> +      //
> +      Size = EFI_PAGES_TO_SIZE(Pages);
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        (UINTN)HostAddress,
> +                        ALIGN_VALUE(Size, mIoMmuPageSize),
> +                        0
> +                        );
> +    }
> +  }
>    return Status;
>  }
>
> --
> 2.7.4.windows.1


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

* Re: [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.
  2017-03-27  6:01     ` Yao, Jiewen
@ 2017-03-27  6:18       ` Ni, Ruiyu
  2017-03-27  6:23         ` Yao, Jiewen
  0 siblings, 1 reply; 23+ messages in thread
From: Ni, Ruiyu @ 2017-03-27  6:18 UTC (permalink / raw)
  To: Yao, Jiewen, edk2-devel@lists.01.org; +Cc: Leo Duran, Brijesh Singh

For #1, thank you for the explanation. I have no concerns.
For #2, I agree copying code is a very fast way to do implementation.
              But it is also a very fast way to spread incorrectness. So copying is not encouraged.
              Can you check my comments in details to see whether they make sense?


Thanks/Ray

From: Yao, Jiewen
Sent: Monday, March 27, 2017 2:01 PM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
Cc: Leo Duran <leo.duran@amd.com>; Brijesh Singh <brijesh.singh@amd.com>
Subject: RE: [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.

Answer below:

From: Ni, Ruiyu
Sent: Monday, March 27, 2017 1:32 PM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>; Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
Subject: RE: [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.

Jiewen,
#1. In RootBridgeIoMap(), NeedMap is TRUE when:
 1). Host or device doesn't support above 4GB DMA access
Or
 2). The memory size is not multiple of IO MMU page size

For case 2), we only need to call BS.AllocatePages with AllocateAnyAddress allocation type,
but I saw you still use AllocateMaxAddress allocation type.
Can we guarantee that (AllocateMaxAddress plus MaxAddress = MAX_UINT64) equals to
(AllocateAnyAddress)?
[Jiewen] Yes.

 I thought the first combination requires to allocate from very top
physical memory, but the second (AllocateAnyAddress) doesn't have such requirement.
[Jiewen] That is not true.
UEFI spec: "Allocation requests of Type AllocateMaxAddress allocate any available range of pages whose uppermost address is less than or equal to the address pointed to by Memory on input."
It means "any available range".




#2. In InternalAllocateAlignedPagesWithAllocateType ():
      #a). AlignedMemory  = ((UINTN) Memory + AlignmentMask) & ~AlignmentMask;
      Can we use ALIGNED_POINTER() macro?
      #b). I think we do not need to free last unaligned pages. Because we only over-allocate
      several pages in the beginning. Please correct me if I am wrong.
      #c). The function name is too long. Can we rename it to IAllocatePages()? The parameter
      Type and Alignment indicate the allocation honors Type and Alignment.
[Jiewen] I copied the code from MemoryAllocationLib.





Thanks/Ray

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Saturday, March 25, 2017 5:29 PM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>;
> Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
> Subject: [RFC] [PATCH 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.
>
> 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>>
> 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      |   7 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 172
> +++++++++++++++++++-
>  4 files changed, 178 insertions(+), 5 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..4d21d10 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,6 +51,8 @@ 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;
>  } MAP_INFO;
> @@ -575,4 +578,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..47ea697 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 FALSE;
> +  } else {
> +    return FALSE;
> +  }
> +}
> +
> +/**
>    Provides the PCI controller-specific address needed to access
>    system memory for DMA.
>
> @@ -1057,6 +1172,8 @@ RootBridgeIoMap (
>    PCI_ROOT_BRIDGE_INSTANCE                          *RootBridge;
>    EFI_PHYSICAL_ADDRESS                              PhysicalAddress;
>    MAP_INFO                                          *MapInfo;
> +  BOOLEAN                                           NeedMap;
> +  EFI_PHYSICAL_ADDRESS                              MaxAddress;
>
>    if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress ==
> NULL ||
>        Mapping == NULL) {
> @@ -1072,12 +1189,40 @@ 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;
> +
> +  NeedMap = FALSE;
> +  MaxAddress = (UINT64)-1;
> +
>    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
> @@ -1113,15 +1258,18 @@ 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);
>
>      //
>      // Allocate a buffer below 4GB to map the transfer to.
>      //
> -    Status = gBS->AllocatePages (
> +    Status = InternalAllocateAlignedPagesWithAllocateType (
>                      AllocateMaxAddress,
>                      EfiBootServicesData,
> -                    MapInfo->NumberOfPages,
> +                    MapInfo->MappedNumberOfPages,
> +                    mIoMmuPageSize,
>                      &MapInfo->MappedHostAddress
>                      );
>      if (EFI_ERROR (Status)) {
> @@ -1240,7 +1388,7 @@ RootBridgeIoUnmap (
>    //
>    // 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;
>  }
> @@ -1286,6 +1434,7 @@ RootBridgeIoAllocateBuffer (
>    EFI_PHYSICAL_ADDRESS      PhysicalAddress;
>    PCI_ROOT_BRIDGE_INSTANCE  *RootBridge;
>    EFI_ALLOCATE_TYPE         AllocateType;
> +  UINTN                     Size;
>
>    //
>    // Validate Attributes
> @@ -1321,10 +1470,16 @@ RootBridgeIoAllocateBuffer (
>      AllocateType    = AllocateMaxAddress;
>      PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (SIZE_4GB - 1);
>    }
> -  Status = gBS->AllocatePages (
> +  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 (
>                    AllocateType,
>                    MemoryType,
>                    Pages,
> +                  mIoMmuPageSize,
>                    &PhysicalAddress
>                    );
>    if (!EFI_ERROR (Status)) {
> @@ -1356,6 +1511,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] 23+ messages in thread

* Re: [RFC] [PATCH 3/3] MdeModulePkg/PciBus: Add IOMMU support.
  2017-03-27  6:15     ` Yao, Jiewen
@ 2017-03-27  6:23       ` Ni, Ruiyu
  0 siblings, 0 replies; 23+ messages in thread
From: Ni, Ruiyu @ 2017-03-27  6:23 UTC (permalink / raw)
  To: Yao, Jiewen, edk2-devel@lists.01.org; +Cc: Leo Duran, Brijesh Singh

#1. If PciHostBridge driver doesn't consume IOMMU protocol, will the change in PciBus driver cause any
       negative impact?
#2. Please check the PciHostBridgeDxe driver (searching “Maps”). It contains the code to manage the mapping in a list.
       Actually you modified the Map/Unmap code, you should have seen the code if you look a bit more. 😊

Thanks/Ray

From: Yao, Jiewen
Sent: Monday, March 27, 2017 2:16 PM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
Cc: Leo Duran <leo.duran@amd.com>; Brijesh Singh <brijesh.singh@amd.com>
Subject: RE: [RFC] [PATCH 3/3] MdeModulePkg/PciBus: Add IOMMU support.

Answer below:

From: Ni, Ruiyu
Sent: Monday, March 27, 2017 1:40 PM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>; Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
Subject: RE: [RFC] [PATCH 3/3] MdeModulePkg/PciBus: Add IOMMU support.

#1. If PciHostBridge driver doesn't consume IOMMU protocol, will the change in PciBus driver cause any
       negative impact? For example, causing system hang? Do we need to introduce a bit to let PciHostBridge
       report the capability bit about IOMMU through PciHostBridge.GetAllocationAttribute()?
[Jiewen] That is a very good question.

I thought it before, but I finally give up the idea to define capability bit, because I want to make things simple.
I assume the system integration engineer should carefully pick up all drivers and validate the combination.

If the system integration engineer choose to integrate IOMMU driver, he/she should also choose a PciHostBridge which IOMMU capability.

I do not deny the goodness of such capability. But I want to add them later until we see the real usage.



#2, Can you add a Maps field into the PciIoDevice structure? So that the PciIoMapInfo can be inserted to
       the list and we can check whether the Mapping is invalid by checking the existence in the list.
       This is better than checking the memory signature (PciIoMapInfo->Signature).
[Jiewen] I am not clear about this.
Can you provide some sample code to show your idea?
Maybe just submit a patch based upon current code. If it is good, I can integrate the patch directly.


#3.  I saw PciIo.Map/Allocate sets the IOMMU attribute and Unmap/Free clears the IOMMU attribute.
        Do we have the case that the original IOMMU attribute is not 0? If that may happen, we may need to
        have an additional interface in IOMMU protocol to get the original attribute.
[Jiewen] Good question again.
I added below word in IOMMU protocol header file.

Allocation success mean no one use this memory before, so that it should be DMA blocked.


+/**

+  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).





Thanks/Ray

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Saturday, March 25, 2017 5:29 PM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>;
> Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
> Subject: [RFC] [PATCH 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 assumes that PciHostBridge driver can allocate IOMMU page
> aligned memory, if IOMMU protocol exists.
>
> 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>>
> 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      |  10 ++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf |   1 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c       | 100
> ++++++++++++++++++++
>  4 files changed, 123 insertions(+)
>
> 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..6f96696 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> @@ -304,6 +305,13 @@ 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;
> +  UINTN                                     NumberOfBytes;
> +  EFI_PHYSICAL_ADDRESS                      DeviceAddress;
> +  VOID                                      *PciRootBridgeIoMapping;
> +} PCI_IO_MAP_INFO;
>
>  //
>  // Global Variables
> @@ -319,6 +327,8 @@ extern UINT64                                       gAllOne;
>  extern UINT64                                       gAllZero;
>  extern EFI_PCI_PLATFORM_PROTOCOL                    *gPciPlatformProtocol;
>  extern EFI_PCI_OVERRIDE_PROTOCOL                    *gPciOverrideProtocol;
> +extern EDKII_IOMMU_PROTOCOL                         *gIoMmuProtocol;
> +extern UINTN                                        mIoMmuPageSize;
>  extern BOOLEAN                                      mReserveIsaAliases;
>  extern BOOLEAN                                      mReserveVgaAliases;
>
> 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..01786c1 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> @@ -967,6 +967,8 @@ PciIoMap (
>  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  PCI_IO_MAP_INFO       *PciIoMapInfo;
> +  UINT64                IoMmuAttribute;
>
>    PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
>
> @@ -999,6 +1001,46 @@ PciIoMap (
>        );
>    }
>
> +  if (gIoMmuProtocol != NULL) {
> +    if (!EFI_ERROR(Status)) {
> +      PciIoMapInfo = AllocatePool (sizeof(*PciIoMapInfo));
> +      if (PciIoMapInfo == NULL) {
> +        PciIoDevice->PciRootBridgeIo->Unmap (PciIoDevice->PciRootBridgeIo,
> *Mapping);
> +        return EFI_OUT_OF_RESOURCES;
> +      }
> +
> +      PciIoMapInfo->Signature              = PCI_IO_MAP_INFO_SIGNATURE;
> +      PciIoMapInfo->NumberOfBytes          = *NumberOfBytes;
> +      PciIoMapInfo->DeviceAddress          = *DeviceAddress;
> +      PciIoMapInfo->PciRootBridgeIoMapping = *Mapping;
> +      *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 Status;
> +      }
> +      //
> +      // PciHostBridge should map IOMMU page aligned HostAddress.
> +      //
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        PciIoMapInfo->DeviceAddress,
> +                        ALIGN_VALUE(PciIoMapInfo->NumberOfBytes,
> mIoMmuPageSize),
> +                        IoMmuAttribute
> +                        );
> +    }
> +  }
>    return Status;
>  }
>
> @@ -1021,9 +1063,19 @@ PciIoUnmap (
>  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  PCI_IO_MAP_INFO *PciIoMapInfo;
>
>    PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
>
> +  PciIoMapInfo = NULL;
> +  if (gIoMmuProtocol != NULL) {
> +    PciIoMapInfo = Mapping;
> +    if (PciIoMapInfo->Signature != PCI_IO_MAP_INFO_SIGNATURE) {
> +      return EFI_INVALID_PARAMETER;
> +    }
> +    Mapping = PciIoMapInfo->PciRootBridgeIoMapping;
> +  }
> +
>    Status = PciIoDevice->PciRootBridgeIo->Unmap (
>                                            PciIoDevice->PciRootBridgeIo,
>                                            Mapping @@ -1037,6 +1089,22 @@ PciIoUnmap (
>        );
>    }
>
> +  if (gIoMmuProtocol != NULL) {
> +    if (!EFI_ERROR(Status)) {
> +      //
> +      // PciHostBridge should map IOMMU page aligned HostAddress.
> +      //
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        PciIoMapInfo->DeviceAddress,
> +                        ALIGN_VALUE(PciIoMapInfo->NumberOfBytes,
> mIoMmuPageSize),
> +                        0
> +                        );
> +      FreePool (PciIoMapInfo);
> +    }
> +  }
> +
>    return Status;
>  }
>
> @@ -1073,6 +1141,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){ @@ -1102,6 +1171,21 @@
> PciIoAllocateBuffer (
>        );
>    }
>
> +  if (gIoMmuProtocol != NULL) {
> +    if (!EFI_ERROR(Status)) {
> +      //
> +      // PciHostBridge should allocate IOMMU page aligned HostAddress.
> +      //
> +      Size = EFI_PAGES_TO_SIZE(Pages);
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        (UINTN)*HostAddress,
> +                        ALIGN_VALUE(Size, mIoMmuPageSize),
> +                        EDKII_IOMMU_ATTRIBUTE_READ |
> EDKII_IOMMU_ATTRIBUTE_WRITE
> +                        );
> +    }
> +  }
>    return Status;
>  }
>
> @@ -1127,6 +1211,7 @@ PciIoFreeBuffer (
>  {
>    EFI_STATUS    Status;
>    PCI_IO_DEVICE *PciIoDevice;
> +  UINTN         Size;
>
>    PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
>
> @@ -1144,6 +1229,21 @@ PciIoFreeBuffer (
>        );
>    }
>
> +  if (gIoMmuProtocol != NULL) {
> +    if (!EFI_ERROR(Status)) {
> +      //
> +      // PciHostBridge should allocate IOMMU page aligned HostAddress.
> +      //
> +      Size = EFI_PAGES_TO_SIZE(Pages);
> +      gIoMmuProtocol->SetAttribute (
> +                        gIoMmuProtocol,
> +                        PciIoDevice->Handle,
> +                        (UINTN)HostAddress,
> +                        ALIGN_VALUE(Size, mIoMmuPageSize),
> +                        0
> +                        );
> +    }
> +  }
>    return Status;
>  }
>
> --
> 2.7.4.windows.1

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

* Re: [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.
  2017-03-27  6:18       ` Ni, Ruiyu
@ 2017-03-27  6:23         ` Yao, Jiewen
  2017-03-27  6:25           ` Ni, Ruiyu
  0 siblings, 1 reply; 23+ messages in thread
From: Yao, Jiewen @ 2017-03-27  6:23 UTC (permalink / raw)
  To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Leo Duran, Brijesh Singh

Yes. I agree.
If there is an error, we should fix it at all places.

I will also invite the module owner to double check your comment.

Thank you
Yao Jiewen

From: Ni, Ruiyu
Sent: Monday, March 27, 2017 2:19 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
Cc: Leo Duran <leo.duran@amd.com>; Brijesh Singh <brijesh.singh@amd.com>
Subject: RE: [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.

For #1, thank you for the explanation. I have no concerns.
For #2, I agree copying code is a very fast way to do implementation.
              But it is also a very fast way to spread incorrectness. So copying is not encouraged.
              Can you check my comments in details to see whether they make sense?


Thanks/Ray

From: Yao, Jiewen
Sent: Monday, March 27, 2017 2:01 PM
To: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>; Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
Subject: RE: [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.

Answer below:

From: Ni, Ruiyu
Sent: Monday, March 27, 2017 1:32 PM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>; Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
Subject: RE: [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.

Jiewen,
#1. In RootBridgeIoMap(), NeedMap is TRUE when:
 1). Host or device doesn't support above 4GB DMA access
Or
 2). The memory size is not multiple of IO MMU page size

For case 2), we only need to call BS.AllocatePages with AllocateAnyAddress allocation type,
but I saw you still use AllocateMaxAddress allocation type.
Can we guarantee that (AllocateMaxAddress plus MaxAddress = MAX_UINT64) equals to
(AllocateAnyAddress)?
[Jiewen] Yes.

 I thought the first combination requires to allocate from very top
physical memory, but the second (AllocateAnyAddress) doesn't have such requirement.
[Jiewen] That is not true.
UEFI spec: "Allocation requests of Type AllocateMaxAddress allocate any available range of pages whose uppermost address is less than or equal to the address pointed to by Memory on input."
It means "any available range".




#2. In InternalAllocateAlignedPagesWithAllocateType ():
      #a). AlignedMemory  = ((UINTN) Memory + AlignmentMask) & ~AlignmentMask;
      Can we use ALIGNED_POINTER() macro?
      #b). I think we do not need to free last unaligned pages. Because we only over-allocate
      several pages in the beginning. Please correct me if I am wrong.
      #c). The function name is too long. Can we rename it to IAllocatePages()? The parameter
      Type and Alignment indicate the allocation honors Type and Alignment.
[Jiewen] I copied the code from MemoryAllocationLib.





Thanks/Ray

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Saturday, March 25, 2017 5:29 PM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>;
> Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
> Subject: [RFC] [PATCH 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.
>
> 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>>
> 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      |   7 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 172
> +++++++++++++++++++-
>  4 files changed, 178 insertions(+), 5 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..4d21d10 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,6 +51,8 @@ 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;
>  } MAP_INFO;
> @@ -575,4 +578,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..47ea697 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 FALSE;
> +  } else {
> +    return FALSE;
> +  }
> +}
> +
> +/**
>    Provides the PCI controller-specific address needed to access
>    system memory for DMA.
>
> @@ -1057,6 +1172,8 @@ RootBridgeIoMap (
>    PCI_ROOT_BRIDGE_INSTANCE                          *RootBridge;
>    EFI_PHYSICAL_ADDRESS                              PhysicalAddress;
>    MAP_INFO                                          *MapInfo;
> +  BOOLEAN                                           NeedMap;
> +  EFI_PHYSICAL_ADDRESS                              MaxAddress;
>
>    if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress ==
> NULL ||
>        Mapping == NULL) {
> @@ -1072,12 +1189,40 @@ 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;
> +
> +  NeedMap = FALSE;
> +  MaxAddress = (UINT64)-1;
> +
>    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
> @@ -1113,15 +1258,18 @@ 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);
>
>      //
>      // Allocate a buffer below 4GB to map the transfer to.
>      //
> -    Status = gBS->AllocatePages (
> +    Status = InternalAllocateAlignedPagesWithAllocateType (
>                      AllocateMaxAddress,
>                      EfiBootServicesData,
> -                    MapInfo->NumberOfPages,
> +                    MapInfo->MappedNumberOfPages,
> +                    mIoMmuPageSize,
>                      &MapInfo->MappedHostAddress
>                      );
>      if (EFI_ERROR (Status)) {
> @@ -1240,7 +1388,7 @@ RootBridgeIoUnmap (
>    //
>    // 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;
>  }
> @@ -1286,6 +1434,7 @@ RootBridgeIoAllocateBuffer (
>    EFI_PHYSICAL_ADDRESS      PhysicalAddress;
>    PCI_ROOT_BRIDGE_INSTANCE  *RootBridge;
>    EFI_ALLOCATE_TYPE         AllocateType;
> +  UINTN                     Size;
>
>    //
>    // Validate Attributes
> @@ -1321,10 +1470,16 @@ RootBridgeIoAllocateBuffer (
>      AllocateType    = AllocateMaxAddress;
>      PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (SIZE_4GB - 1);
>    }
> -  Status = gBS->AllocatePages (
> +  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 (
>                    AllocateType,
>                    MemoryType,
>                    Pages,
> +                  mIoMmuPageSize,
>                    &PhysicalAddress
>                    );
>    if (!EFI_ERROR (Status)) {
> @@ -1356,6 +1511,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] 23+ messages in thread

* Re: [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.
  2017-03-27  6:23         ` Yao, Jiewen
@ 2017-03-27  6:25           ` Ni, Ruiyu
  0 siblings, 0 replies; 23+ messages in thread
From: Ni, Ruiyu @ 2017-03-27  6:25 UTC (permalink / raw)
  To: Yao, Jiewen, edk2-devel@lists.01.org; +Cc: Leo Duran, Brijesh Singh

Thank you!

Thanks/Ray

From: Yao, Jiewen
Sent: Monday, March 27, 2017 2:24 PM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
Cc: Leo Duran <leo.duran@amd.com>; Brijesh Singh <brijesh.singh@amd.com>
Subject: RE: [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.

Yes. I agree.
If there is an error, we should fix it at all places.

I will also invite the module owner to double check your comment.

Thank you
Yao Jiewen

From: Ni, Ruiyu
Sent: Monday, March 27, 2017 2:19 PM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>; Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
Subject: RE: [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.

For #1, thank you for the explanation. I have no concerns.
For #2, I agree copying code is a very fast way to do implementation.
              But it is also a very fast way to spread incorrectness. So copying is not encouraged.
              Can you check my comments in details to see whether they make sense?


Thanks/Ray

From: Yao, Jiewen
Sent: Monday, March 27, 2017 2:01 PM
To: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>; Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
Subject: RE: [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.

Answer below:

From: Ni, Ruiyu
Sent: Monday, March 27, 2017 1:32 PM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>; Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
Subject: RE: [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support.

Jiewen,
#1. In RootBridgeIoMap(), NeedMap is TRUE when:
 1). Host or device doesn't support above 4GB DMA access
Or
 2). The memory size is not multiple of IO MMU page size

For case 2), we only need to call BS.AllocatePages with AllocateAnyAddress allocation type,
but I saw you still use AllocateMaxAddress allocation type.
Can we guarantee that (AllocateMaxAddress plus MaxAddress = MAX_UINT64) equals to
(AllocateAnyAddress)?
[Jiewen] Yes.

 I thought the first combination requires to allocate from very top
physical memory, but the second (AllocateAnyAddress) doesn't have such requirement.
[Jiewen] That is not true.
UEFI spec: "Allocation requests of Type AllocateMaxAddress allocate any available range of pages whose uppermost address is less than or equal to the address pointed to by Memory on input."
It means "any available range".




#2. In InternalAllocateAlignedPagesWithAllocateType ():
      #a). AlignedMemory  = ((UINTN) Memory + AlignmentMask) & ~AlignmentMask;
      Can we use ALIGNED_POINTER() macro?
      #b). I think we do not need to free last unaligned pages. Because we only over-allocate
      several pages in the beginning. Please correct me if I am wrong.
      #c). The function name is too long. Can we rename it to IAllocatePages()? The parameter
      Type and Alignment indicate the allocation honors Type and Alignment.
[Jiewen] I copied the code from MemoryAllocationLib.





Thanks/Ray

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Saturday, March 25, 2017 5:29 PM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>;
> Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
> Subject: [RFC] [PATCH 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.
>
> 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>>
> 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      |   7 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 172
> +++++++++++++++++++-
>  4 files changed, 178 insertions(+), 5 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..4d21d10 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,6 +51,8 @@ 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;
>  } MAP_INFO;
> @@ -575,4 +578,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..47ea697 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 FALSE;
> +  } else {
> +    return FALSE;
> +  }
> +}
> +
> +/**
>    Provides the PCI controller-specific address needed to access
>    system memory for DMA.
>
> @@ -1057,6 +1172,8 @@ RootBridgeIoMap (
>    PCI_ROOT_BRIDGE_INSTANCE                          *RootBridge;
>    EFI_PHYSICAL_ADDRESS                              PhysicalAddress;
>    MAP_INFO                                          *MapInfo;
> +  BOOLEAN                                           NeedMap;
> +  EFI_PHYSICAL_ADDRESS                              MaxAddress;
>
>    if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress ==
> NULL ||
>        Mapping == NULL) {
> @@ -1072,12 +1189,40 @@ 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;
> +
> +  NeedMap = FALSE;
> +  MaxAddress = (UINT64)-1;
> +
>    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
> @@ -1113,15 +1258,18 @@ 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);
>
>      //
>      // Allocate a buffer below 4GB to map the transfer to.
>      //
> -    Status = gBS->AllocatePages (
> +    Status = InternalAllocateAlignedPagesWithAllocateType (
>                      AllocateMaxAddress,
>                      EfiBootServicesData,
> -                    MapInfo->NumberOfPages,
> +                    MapInfo->MappedNumberOfPages,
> +                    mIoMmuPageSize,
>                      &MapInfo->MappedHostAddress
>                      );
>      if (EFI_ERROR (Status)) {
> @@ -1240,7 +1388,7 @@ RootBridgeIoUnmap (
>    //
>    // 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;
>  }
> @@ -1286,6 +1434,7 @@ RootBridgeIoAllocateBuffer (
>    EFI_PHYSICAL_ADDRESS      PhysicalAddress;
>    PCI_ROOT_BRIDGE_INSTANCE  *RootBridge;
>    EFI_ALLOCATE_TYPE         AllocateType;
> +  UINTN                     Size;
>
>    //
>    // Validate Attributes
> @@ -1321,10 +1470,16 @@ RootBridgeIoAllocateBuffer (
>      AllocateType    = AllocateMaxAddress;
>      PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (SIZE_4GB - 1);
>    }
> -  Status = gBS->AllocatePages (
> +  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 (
>                    AllocateType,
>                    MemoryType,
>                    Pages,
> +                  mIoMmuPageSize,
>                    &PhysicalAddress
>                    );
>    if (!EFI_ERROR (Status)) {
> @@ -1356,6 +1511,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] 23+ messages in thread

* Re: [RFC] [PATCH 0/3] Add IOMMU support.
  2017-03-25  9:28 [RFC] [PATCH 0/3] Add IOMMU support Jiewen Yao
                   ` (2 preceding siblings ...)
  2017-03-25  9:28 ` [RFC] [PATCH 3/3] MdeModulePkg/PciBus: " Jiewen Yao
@ 2017-03-27 19:50 ` Duran, Leo
  2017-03-28  1:40   ` Yao, Jiewen
  3 siblings, 1 reply; 23+ messages in thread
From: Duran, Leo @ 2017-03-27 19:50 UTC (permalink / raw)
  To: 'Jiewen Yao', edk2-devel@lists.01.org; +Cc: Ruiyu Ni, Singh, Brijesh

Hi Yao,

This patch-set, in its current form, does not address all of the required SEV functionality for PcIHostBridgeIo.
Basically, we need to intercept all 4 I/O Operations:
- IoMap()
- IoUnmap()
- IoAllocateBuffer()
- IoFreeBuffer()

SEV I/O intercepts would do this:
1) IoMap()
- Allocate an accessible bounce buffer (AllocateType depends on consumer's capabilities).
- Clear SEV mask on retuned mapped DMA buffer
- On DMA Read: CopyMem() from consumer buffer to mapped buffer (bounce operation)

2) IoUnmap()
- On DMA Write: CopyMem() from mapped buffer to consumer buffer (bounce operation)
- Restore SEV mask on mapped DMA buffer

3) IoAllocateBuffer()
- Allocate an accessible buffer (AllocateType depends on consumer's capabilities).
- Clear SEV mask on allocated buffer
- return allocated buffer

4) IoFreeBuffer()
- Restore SEV mask on allocated buffer
- Free allocated buffer

For an sample on how we've intercepted BmDmaLib operations, please refer to the patch-sets posted by Brijesh:
https://lists.01.org/pipermail/edk2-devel/2017-March/008838.html
https://lists.01.org/pipermail/edk2-devel/2017-March/008840.html

Thanks,
Leo

> -----Original Message-----
> From: Jiewen Yao [mailto:jiewen.yao@intel.com]
> Sent: Saturday, March 25, 2017 4:29 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>
> Subject: [RFC] [PATCH 0/3] Add IOMMU support.
> 
> 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>
> 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                    |  10 ++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf               |   1 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c                     | 100 ++++++++++++
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c      |   3 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf |   1 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h      |   7 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 172
> +++++++++++++++++++-
>  MdeModulePkg/Include/Protocol/IoMmu.h                      | 132
> +++++++++++++++
>  MdeModulePkg/MdeModulePkg.dec                              |   3 +
>  10 files changed, 436 insertions(+), 5 deletions(-)  create mode 100644
> MdeModulePkg/Include/Protocol/IoMmu.h
> 
> --
> 2.7.4.windows.1



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

* Re: [RFC] [PATCH 0/3] Add IOMMU support.
  2017-03-27 19:50 ` [RFC] [PATCH 0/3] " Duran, Leo
@ 2017-03-28  1:40   ` Yao, Jiewen
  2017-03-28  1:54     ` Yao, Jiewen
  0 siblings, 1 reply; 23+ messages in thread
From: Yao, Jiewen @ 2017-03-28  1:40 UTC (permalink / raw)
  To: Duran, Leo, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Singh, Brijesh

Hi Leo
I do intercept 4 I/O operation in *PciIo*, instead of *PciRootBridgeIo*.

See: https://lists.01.org/pipermail/edk2-devel/2017-March/009021.html

There are 4 gIoMmuProtocol->SetAttribute() I believe you can clear/set SEV action there.

Would you please double check that?

Thank you
Yao Jiewen


From: Duran, Leo [mailto:leo.duran@amd.com]
Sent: Tuesday, March 28, 2017 3:51 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>
Subject: RE: [RFC] [PATCH 0/3] Add IOMMU support.

Hi Yao,

This patch-set, in its current form, does not address all of the required SEV functionality for PcIHostBridgeIo.
Basically, we need to intercept all 4 I/O Operations:
- IoMap()
- IoUnmap()
- IoAllocateBuffer()
- IoFreeBuffer()

SEV I/O intercepts would do this:
1) IoMap()
- Allocate an accessible bounce buffer (AllocateType depends on consumer's capabilities).
- Clear SEV mask on retuned mapped DMA buffer
- On DMA Read: CopyMem() from consumer buffer to mapped buffer (bounce operation)

2) IoUnmap()
- On DMA Write: CopyMem() from mapped buffer to consumer buffer (bounce operation)
- Restore SEV mask on mapped DMA buffer

3) IoAllocateBuffer()
- Allocate an accessible buffer (AllocateType depends on consumer's capabilities).
- Clear SEV mask on allocated buffer
- return allocated buffer

4) IoFreeBuffer()
- Restore SEV mask on allocated buffer
- Free allocated buffer

For an sample on how we've intercepted BmDmaLib operations, please refer to the patch-sets posted by Brijesh:
https://lists.01.org/pipermail/edk2-devel/2017-March/008838.html
https://lists.01.org/pipermail/edk2-devel/2017-March/008840.html

Thanks,
Leo

> -----Original Message-----
> From: Jiewen Yao [mailto:jiewen.yao@intel.com]
> Sent: Saturday, March 25, 2017 4:29 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>>
> Subject: [RFC] [PATCH 0/3] Add IOMMU support.
>
> 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<mailto:leo.duran@amd.com>> and Brijesh Singh
> <brijesh.ksingh@gmail.com<mailto: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<mailto: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<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>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto: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                    |  10 ++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf               |   1 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c                     | 100 ++++++++++++
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c      |   3 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf |   1 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h      |   7 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 172
> +++++++++++++++++++-
>  MdeModulePkg/Include/Protocol/IoMmu.h                      | 132
> +++++++++++++++
>  MdeModulePkg/MdeModulePkg.dec                              |   3 +
>  10 files changed, 436 insertions(+), 5 deletions(-)  create mode 100644
> MdeModulePkg/Include/Protocol/IoMmu.h
>
> --
> 2.7.4.windows.1


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

* Re: [RFC] [PATCH 0/3] Add IOMMU support.
  2017-03-28  1:40   ` Yao, Jiewen
@ 2017-03-28  1:54     ` Yao, Jiewen
  2017-03-28  2:24       ` Ni, Ruiyu
  0 siblings, 1 reply; 23+ messages in thread
From: Yao, Jiewen @ 2017-03-28  1:54 UTC (permalink / raw)
  To: Yao, Jiewen, Duran, Leo, edk2-devel@lists.01.org
  Cc: Ni, Ruiyu, Singh, Brijesh

I forget mentioning that I have to do it in PciIo instead of PciRootBridgeIo.
The reason is that Intel VTd solution need know which PCI device submit the DMA access request, so that it can only modify the IOMMU paging for this specific device.

I believe AMD IOMMU has similar requirement.

However, this info is not needed, if you just want to enable AMD SEV solution. In that case, you can ignore the DeviceHandle, and just update SEV clear/set bit for that memory.


BTW: I just realize I miss one thing. I call gIoMmuProtocol->SetAttribute, after it is returned from PciRootBridgeIo.

For AMD SEC, I think we need do CopyMem again, if this is DMA read request. I will update my patch for that.


Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
Sent: Tuesday, March 28, 2017 9:41 AM
To: Duran, Leo <leo.duran@amd.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Singh, Brijesh <brijesh.singh@amd.com>
Subject: Re: [edk2] [RFC] [PATCH 0/3] Add IOMMU support.

Hi Leo
I do intercept 4 I/O operation in *PciIo*, instead of *PciRootBridgeIo*.

See: https://lists.01.org/pipermail/edk2-devel/2017-March/009021.html

There are 4 gIoMmuProtocol->SetAttribute() I believe you can clear/set SEV action there.

Would you please double check that?

Thank you
Yao Jiewen


From: Duran, Leo [mailto:leo.duran@amd.com]
Sent: Tuesday, March 28, 2017 3:51 AM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Singh, Brijesh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
Subject: RE: [RFC] [PATCH 0/3] Add IOMMU support.

Hi Yao,

This patch-set, in its current form, does not address all of the required SEV functionality for PcIHostBridgeIo.
Basically, we need to intercept all 4 I/O Operations:
- IoMap()
- IoUnmap()
- IoAllocateBuffer()
- IoFreeBuffer()

SEV I/O intercepts would do this:
1) IoMap()
- Allocate an accessible bounce buffer (AllocateType depends on consumer's capabilities).
- Clear SEV mask on retuned mapped DMA buffer
- On DMA Read: CopyMem() from consumer buffer to mapped buffer (bounce operation)

2) IoUnmap()
- On DMA Write: CopyMem() from mapped buffer to consumer buffer (bounce operation)
- Restore SEV mask on mapped DMA buffer

3) IoAllocateBuffer()
- Allocate an accessible buffer (AllocateType depends on consumer's capabilities).
- Clear SEV mask on allocated buffer
- return allocated buffer

4) IoFreeBuffer()
- Restore SEV mask on allocated buffer
- Free allocated buffer

For an sample on how we've intercepted BmDmaLib operations, please refer to the patch-sets posted by Brijesh:
https://lists.01.org/pipermail/edk2-devel/2017-March/008838.html
https://lists.01.org/pipermail/edk2-devel/2017-March/008840.html

Thanks,
Leo

> -----Original Message-----
> From: Jiewen Yao [mailto:jiewen.yao@intel.com]
> Sent: Saturday, March 25, 2017 4:29 AM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com%3cmailto:ruiyu.ni@intel.com>>>; Duran, Leo <leo.duran@amd.com<mailto:leo.duran@amd.com<mailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com>>>;
> Singh, Brijesh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com<mailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com>>>
> Subject: [RFC] [PATCH 0/3] Add IOMMU support.
>
> 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<mailto:leo.duran@amd.com<mailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com>>> and Brijesh Singh
> <brijesh.ksingh@gmail.com<mailto:brijesh.ksingh@gmail.com<mailto:brijesh.ksingh@gmail.com%3cmailto: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<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto: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<mailto:ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com%3cmailto:ruiyu.ni@intel.com>>>
> Cc: Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com<mailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com>>>
> Cc: Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com<mailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com>>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto: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                    |  10 ++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf               |   1 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c                     | 100 ++++++++++++
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c      |   3 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf |   1 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h      |   7 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 172
> +++++++++++++++++++-
>  MdeModulePkg/Include/Protocol/IoMmu.h                      | 132
> +++++++++++++++
>  MdeModulePkg/MdeModulePkg.dec                              |   3 +
>  10 files changed, 436 insertions(+), 5 deletions(-)  create mode 100644
> MdeModulePkg/Include/Protocol/IoMmu.h
>
> --
> 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] 23+ messages in thread

* Re: [RFC] [PATCH 0/3] Add IOMMU support.
  2017-03-28  1:54     ` Yao, Jiewen
@ 2017-03-28  2:24       ` Ni, Ruiyu
  2017-03-28  2:32         ` Yao, Jiewen
  0 siblings, 1 reply; 23+ messages in thread
From: Ni, Ruiyu @ 2017-03-28  2:24 UTC (permalink / raw)
  To: Yao, Jiewen, Duran, Leo, edk2-devel@lists.01.org; +Cc: Singh, Brijesh

Jiewen,
For performance consideration, do you think we may define a bit as a hint to tell PciHostBridge (or PciBus, I don't know) whether a post-CopyMem is needed?

Thanks/Ray

From: Yao, Jiewen
Sent: Tuesday, March 28, 2017 9:55 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; Duran, Leo <leo.duran@amd.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Singh, Brijesh <brijesh.singh@amd.com>
Subject: RE: [RFC] [PATCH 0/3] Add IOMMU support.

I forget mentioning that I have to do it in PciIo instead of PciRootBridgeIo.
The reason is that Intel VTd solution need know which PCI device submit the DMA access request, so that it can only modify the IOMMU paging for this specific device.

I believe AMD IOMMU has similar requirement.

However, this info is not needed, if you just want to enable AMD SEV solution. In that case, you can ignore the DeviceHandle, and just update SEV clear/set bit for that memory.


BTW: I just realize I miss one thing. I call gIoMmuProtocol->SetAttribute, after it is returned from PciRootBridgeIo.

For AMD SEC, I think we need do CopyMem again, if this is DMA read request. I will update my patch for that.


Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
Sent: Tuesday, March 28, 2017 9:41 AM
To: Duran, Leo <leo.duran@amd.com<mailto:leo.duran@amd.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Singh, Brijesh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
Subject: Re: [edk2] [RFC] [PATCH 0/3] Add IOMMU support.

Hi Leo
I do intercept 4 I/O operation in *PciIo*, instead of *PciRootBridgeIo*.

See: https://lists.01.org/pipermail/edk2-devel/2017-March/009021.html

There are 4 gIoMmuProtocol->SetAttribute() I believe you can clear/set SEV action there.

Would you please double check that?

Thank you
Yao Jiewen


From: Duran, Leo [mailto:leo.duran@amd.com]
Sent: Tuesday, March 28, 2017 3:51 AM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Singh, Brijesh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
Subject: RE: [RFC] [PATCH 0/3] Add IOMMU support.

Hi Yao,

This patch-set, in its current form, does not address all of the required SEV functionality for PcIHostBridgeIo.
Basically, we need to intercept all 4 I/O Operations:
- IoMap()
- IoUnmap()
- IoAllocateBuffer()
- IoFreeBuffer()

SEV I/O intercepts would do this:
1) IoMap()
- Allocate an accessible bounce buffer (AllocateType depends on consumer's capabilities).
- Clear SEV mask on retuned mapped DMA buffer
- On DMA Read: CopyMem() from consumer buffer to mapped buffer (bounce operation)

2) IoUnmap()
- On DMA Write: CopyMem() from mapped buffer to consumer buffer (bounce operation)
- Restore SEV mask on mapped DMA buffer

3) IoAllocateBuffer()
- Allocate an accessible buffer (AllocateType depends on consumer's capabilities).
- Clear SEV mask on allocated buffer
- return allocated buffer

4) IoFreeBuffer()
- Restore SEV mask on allocated buffer
- Free allocated buffer

For an sample on how we've intercepted BmDmaLib operations, please refer to the patch-sets posted by Brijesh:
https://lists.01.org/pipermail/edk2-devel/2017-March/008838.html
https://lists.01.org/pipermail/edk2-devel/2017-March/008840.html

Thanks,
Leo

> -----Original Message-----
> From: Jiewen Yao [mailto:jiewen.yao@intel.com]
> Sent: Saturday, March 25, 2017 4:29 AM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com%3cmailto:ruiyu.ni@intel.com>>>; Duran, Leo <leo.duran@amd.com<mailto:leo.duran@amd.com<mailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com>>>;
> Singh, Brijesh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com<mailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com>>>
> Subject: [RFC] [PATCH 0/3] Add IOMMU support.
>
> 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<mailto:leo.duran@amd.com<mailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com>>> and Brijesh Singh
> <brijesh.ksingh@gmail.com<mailto:brijesh.ksingh@gmail.com<mailto:brijesh.ksingh@gmail.com%3cmailto: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<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto: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<mailto:ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com%3cmailto:ruiyu.ni@intel.com>>>
> Cc: Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com<mailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com>>>
> Cc: Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com<mailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com>>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto: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                    |  10 ++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf               |   1 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c                     | 100 ++++++++++++
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c      |   3 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf |   1 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h      |   7 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 172
> +++++++++++++++++++-
>  MdeModulePkg/Include/Protocol/IoMmu.h                      | 132
> +++++++++++++++
>  MdeModulePkg/MdeModulePkg.dec                              |   3 +
>  10 files changed, 436 insertions(+), 5 deletions(-)  create mode 100644
> MdeModulePkg/Include/Protocol/IoMmu.h
>
> --
> 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] 23+ messages in thread

* Re: [RFC] [PATCH 0/3] Add IOMMU support.
  2017-03-28  2:24       ` Ni, Ruiyu
@ 2017-03-28  2:32         ` Yao, Jiewen
  0 siblings, 0 replies; 23+ messages in thread
From: Yao, Jiewen @ 2017-03-28  2:32 UTC (permalink / raw)
  To: Ni, Ruiyu, Duran, Leo, edk2-devel@lists.01.org
  Cc: Singh, Brijesh, Yao, Jiewen

I think IOMMU protocol can be the hint.

From: Ni, Ruiyu
Sent: Tuesday, March 28, 2017 10:25 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; Duran, Leo <leo.duran@amd.com>; edk2-devel@lists.01.org
Cc: Singh, Brijesh <brijesh.singh@amd.com>
Subject: RE: [RFC] [PATCH 0/3] Add IOMMU support.

Jiewen,
For performance consideration, do you think we may define a bit as a hint to tell PciHostBridge (or PciBus, I don't know) whether a post-CopyMem is needed?

Thanks/Ray

From: Yao, Jiewen
Sent: Tuesday, March 28, 2017 9:55 AM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Duran, Leo <leo.duran@amd.com<mailto:leo.duran@amd.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Singh, Brijesh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
Subject: RE: [RFC] [PATCH 0/3] Add IOMMU support.

I forget mentioning that I have to do it in PciIo instead of PciRootBridgeIo.
The reason is that Intel VTd solution need know which PCI device submit the DMA access request, so that it can only modify the IOMMU paging for this specific device.

I believe AMD IOMMU has similar requirement.

However, this info is not needed, if you just want to enable AMD SEV solution. In that case, you can ignore the DeviceHandle, and just update SEV clear/set bit for that memory.


BTW: I just realize I miss one thing. I call gIoMmuProtocol->SetAttribute, after it is returned from PciRootBridgeIo.

For AMD SEC, I think we need do CopyMem again, if this is DMA read request. I will update my patch for that.


Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
Sent: Tuesday, March 28, 2017 9:41 AM
To: Duran, Leo <leo.duran@amd.com<mailto:leo.duran@amd.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Singh, Brijesh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
Subject: Re: [edk2] [RFC] [PATCH 0/3] Add IOMMU support.

Hi Leo
I do intercept 4 I/O operation in *PciIo*, instead of *PciRootBridgeIo*.

See: https://lists.01.org/pipermail/edk2-devel/2017-March/009021.html

There are 4 gIoMmuProtocol->SetAttribute() I believe you can clear/set SEV action there.

Would you please double check that?

Thank you
Yao Jiewen


From: Duran, Leo [mailto:leo.duran@amd.com]
Sent: Tuesday, March 28, 2017 3:51 AM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Singh, Brijesh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
Subject: RE: [RFC] [PATCH 0/3] Add IOMMU support.

Hi Yao,

This patch-set, in its current form, does not address all of the required SEV functionality for PcIHostBridgeIo.
Basically, we need to intercept all 4 I/O Operations:
- IoMap()
- IoUnmap()
- IoAllocateBuffer()
- IoFreeBuffer()

SEV I/O intercepts would do this:
1) IoMap()
- Allocate an accessible bounce buffer (AllocateType depends on consumer's capabilities).
- Clear SEV mask on retuned mapped DMA buffer
- On DMA Read: CopyMem() from consumer buffer to mapped buffer (bounce operation)

2) IoUnmap()
- On DMA Write: CopyMem() from mapped buffer to consumer buffer (bounce operation)
- Restore SEV mask on mapped DMA buffer

3) IoAllocateBuffer()
- Allocate an accessible buffer (AllocateType depends on consumer's capabilities).
- Clear SEV mask on allocated buffer
- return allocated buffer

4) IoFreeBuffer()
- Restore SEV mask on allocated buffer
- Free allocated buffer

For an sample on how we've intercepted BmDmaLib operations, please refer to the patch-sets posted by Brijesh:
https://lists.01.org/pipermail/edk2-devel/2017-March/008838.html
https://lists.01.org/pipermail/edk2-devel/2017-March/008840.html

Thanks,
Leo

> -----Original Message-----
> From: Jiewen Yao [mailto:jiewen.yao@intel.com]
> Sent: Saturday, March 25, 2017 4:29 AM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com%3cmailto:ruiyu.ni@intel.com>>>; Duran, Leo <leo.duran@amd.com<mailto:leo.duran@amd.com<mailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com>>>;
> Singh, Brijesh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com<mailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com>>>
> Subject: [RFC] [PATCH 0/3] Add IOMMU support.
>
> 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<mailto:leo.duran@amd.com<mailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com>>> and Brijesh Singh
> <brijesh.ksingh@gmail.com<mailto:brijesh.ksingh@gmail.com<mailto:brijesh.ksingh@gmail.com%3cmailto: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<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto: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<mailto:ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com%3cmailto:ruiyu.ni@intel.com>>>
> Cc: Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com<mailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com>>>
> Cc: Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com<mailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com>>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto: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                    |  10 ++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf               |   1 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c                     | 100 ++++++++++++
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c      |   3 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf |   1 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h      |   7 +
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c    | 172
> +++++++++++++++++++-
>  MdeModulePkg/Include/Protocol/IoMmu.h                      | 132
> +++++++++++++++
>  MdeModulePkg/MdeModulePkg.dec                              |   3 +
>  10 files changed, 436 insertions(+), 5 deletions(-)  create mode 100644
> MdeModulePkg/Include/Protocol/IoMmu.h
>
> --
> 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] 23+ messages in thread

* Re: [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol definition.
  2017-03-25  9:28 ` [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol definition Jiewen Yao
@ 2017-03-28 22:38   ` Ard Biesheuvel
  2017-03-28 23:02     ` Kinney, Michael D
  0 siblings, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2017-03-28 22:38 UTC (permalink / raw)
  To: Jiewen Yao; +Cc: edk2-devel@lists.01.org, Ruiyu Ni, Brijesh Singh, Leo Duran

On 25 March 2017 at 09:28, Jiewen Yao <jiewen.yao@intel.com> wrote:
> This protocol is to abstract IOMMU access.
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Leo Duran <leo.duran@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>

Hello all,

It would be very useful for a IOMMU protocol such as this one to
support non-identity mappings between the host and the PCI bus.

On 64-bit ARM systems, no RAM may exist below 4 GB, and if a IOMMU is
available, it could be used to remap RAM for DMA in a way that makes
it 32-bit addressable for PCI masters that are not 64-bit capable.

The PCI protocols in UEFI already allow such non-identity mappings. If
a IOMMU protocol is introduced, it makes sense to allow it to be
involved in establishing the device address when performing a map
operation.

-- 
Ard.


> ---
>  MdeModulePkg/Include/Protocol/IoMmu.h | 132 ++++++++++++++++++++
>  MdeModulePkg/MdeModulePkg.dec         |   3 +
>  2 files changed, 135 insertions(+)
>
> diff --git a/MdeModulePkg/Include/Protocol/IoMmu.h b/MdeModulePkg/Include/Protocol/IoMmu.h
> new file mode 100644
> index 0000000..55b9c78
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/IoMmu.h
> @@ -0,0 +1,132 @@
> +/** @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
> +
> +/**
> +  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 it is.
> +  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  This              The protocol instance pointer.
> +  @param  DeviceHandle      The device who initiates the DMA access request.
> +  @param  BaseAddress       The base of system memory address to be used as the DMA memory.
> +  @param  Length            The length of system memory address to be used as the DMA memory.
> +  @param  IoMmuAttribute    The IOMMU attribute.
> +
> +  @retval EFI_SUCCESS            The IoMmuAttribute is set for the memory range specified by BaseAddress and Length.
> +  @retval EFI_INVALID_PARAMETER  DeviceHandle is an invalid handle.
> +  @retval EFI_INVALID_PARAMETER  BaseAddress 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 BaseAddress 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 UINT64                BaseAddress,
> +  IN UINT64                Length,
> +  IN UINT64                IoMmuAttribute
> +  );
> +
> +/**
> +  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  This      Protocol instance pointer.
> +  @param  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
> +  );
> +
> +///
> +/// IOMMU Protocol structure.
> +///
> +struct _EDKII_IOMMU_PROTOCOL {
> +  UINT64                       Revision;
> +  EDKII_IOMMU_GET_PAGE_SIZE    GetPageSize;
> +  EDKII_IOMMU_SET_ATTRIBUTE    SetAttribute;
> +};
> +
> +///
> +/// 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
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


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

* Re: [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol definition.
  2017-03-28 22:38   ` Ard Biesheuvel
@ 2017-03-28 23:02     ` Kinney, Michael D
  2017-03-28 23:45       ` Yao, Jiewen
  0 siblings, 1 reply; 23+ messages in thread
From: Kinney, Michael D @ 2017-03-28 23:02 UTC (permalink / raw)
  To: Ard Biesheuvel, Yao, Jiewen, Kinney, Michael D
  Cc: Ni, Ruiyu, Leo Duran, edk2-devel@lists.01.org, Brijesh Singh

Ard,

I agree.  And I think the IOMMU protocol should also support flexible
double buffer operations so the extra copies by the host CPU can be
avoided if the logical DMA address can directly map to the original
buffer.

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
> Biesheuvel
> Sent: Tuesday, March 28, 2017 3:39 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Leo Duran <leo.duran@amd.com>; edk2-
> devel@lists.01.org; Brijesh Singh <brijesh.singh@amd.com>
> Subject: Re: [edk2] [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol
> definition.
> 
> On 25 March 2017 at 09:28, Jiewen Yao <jiewen.yao@intel.com> wrote:
> > This protocol is to abstract IOMMU access.
> >
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Leo Duran <leo.duran@amd.com>
> > Cc: Brijesh Singh <brijesh.singh@amd.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> 
> Hello all,
> 
> It would be very useful for a IOMMU protocol such as this one to
> support non-identity mappings between the host and the PCI bus.
> 
> On 64-bit ARM systems, no RAM may exist below 4 GB, and if a IOMMU is
> available, it could be used to remap RAM for DMA in a way that makes
> it 32-bit addressable for PCI masters that are not 64-bit capable.
> 
> The PCI protocols in UEFI already allow such non-identity mappings. If
> a IOMMU protocol is introduced, it makes sense to allow it to be
> involved in establishing the device address when performing a map
> operation.
> 
> --
> Ard.
> 
> 
> > ---
> >  MdeModulePkg/Include/Protocol/IoMmu.h | 132 ++++++++++++++++++++
> >  MdeModulePkg/MdeModulePkg.dec         |   3 +
> >  2 files changed, 135 insertions(+)
> >
> > diff --git a/MdeModulePkg/Include/Protocol/IoMmu.h
> b/MdeModulePkg/Include/Protocol/IoMmu.h
> > new file mode 100644
> > index 0000000..55b9c78
> > --- /dev/null
> > +++ b/MdeModulePkg/Include/Protocol/IoMmu.h
> > @@ -0,0 +1,132 @@
> > +/** @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
> > +
> > +/**
> > +  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 it is.
> > +  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  This              The protocol instance pointer.
> > +  @param  DeviceHandle      The device who initiates the DMA access request.
> > +  @param  BaseAddress       The base of system memory address to be used as the
> DMA memory.
> > +  @param  Length            The length of system memory address to be used as
> the DMA memory.
> > +  @param  IoMmuAttribute    The IOMMU attribute.
> > +
> > +  @retval EFI_SUCCESS            The IoMmuAttribute is set for the memory range
> specified by BaseAddress and Length.
> > +  @retval EFI_INVALID_PARAMETER  DeviceHandle is an invalid handle.
> > +  @retval EFI_INVALID_PARAMETER  BaseAddress 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 BaseAddress 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 UINT64                BaseAddress,
> > +  IN UINT64                Length,
> > +  IN UINT64                IoMmuAttribute
> > +  );
> > +
> > +/**
> > +  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  This      Protocol instance pointer.
> > +  @param  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
> > +  );
> > +
> > +///
> > +/// IOMMU Protocol structure.
> > +///
> > +struct _EDKII_IOMMU_PROTOCOL {
> > +  UINT64                       Revision;
> > +  EDKII_IOMMU_GET_PAGE_SIZE    GetPageSize;
> > +  EDKII_IOMMU_SET_ATTRIBUTE    SetAttribute;
> > +};
> > +
> > +///
> > +/// 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
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


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

* Re: [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol definition.
  2017-03-28 23:02     ` Kinney, Michael D
@ 2017-03-28 23:45       ` Yao, Jiewen
  2017-03-29 14:22         ` Ard Biesheuvel
  0 siblings, 1 reply; 23+ messages in thread
From: Yao, Jiewen @ 2017-03-28 23:45 UTC (permalink / raw)
  To: Kinney, Michael D, Ard Biesheuvel
  Cc: Ni, Ruiyu, Leo Duran, edk2-devel@lists.01.org, Brijesh Singh

Agree. That is a good idea.

I will add that in V2 patch.

Thank you
Yao Jiewen

From: Kinney, Michael D
Sent: Wednesday, March 29, 2017 7:03 AM
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Leo Duran <leo.duran@amd.com>; edk2-devel@lists.01.org; Brijesh Singh <brijesh.singh@amd.com>
Subject: RE: [edk2] [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol definition.

Ard,

I agree.  And I think the IOMMU protocol should also support flexible
double buffer operations so the extra copies by the host CPU can be
avoided if the logical DMA address can directly map to the original
buffer.

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
> Biesheuvel
> Sent: Tuesday, March 28, 2017 3:39 PM
> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com>>; edk2-
> devel@lists.01.org<mailto:devel@lists.01.org>; Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
> Subject: Re: [edk2] [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol
> definition.
>
> On 25 March 2017 at 09:28, Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
> > This protocol is to abstract IOMMU access.
> >
> > 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>>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>
> Hello all,
>
> It would be very useful for a IOMMU protocol such as this one to
> support non-identity mappings between the host and the PCI bus.
>
> On 64-bit ARM systems, no RAM may exist below 4 GB, and if a IOMMU is
> available, it could be used to remap RAM for DMA in a way that makes
> it 32-bit addressable for PCI masters that are not 64-bit capable.
>
> The PCI protocols in UEFI already allow such non-identity mappings. If
> a IOMMU protocol is introduced, it makes sense to allow it to be
> involved in establishing the device address when performing a map
> operation.
>
> --
> Ard.
>
>
> > ---
> >  MdeModulePkg/Include/Protocol/IoMmu.h | 132 ++++++++++++++++++++
> >  MdeModulePkg/MdeModulePkg.dec         |   3 +
> >  2 files changed, 135 insertions(+)
> >
> > diff --git a/MdeModulePkg/Include/Protocol/IoMmu.h
> b/MdeModulePkg/Include/Protocol/IoMmu.h
> > new file mode 100644
> > index 0000000..55b9c78
> > --- /dev/null
> > +++ b/MdeModulePkg/Include/Protocol/IoMmu.h
> > @@ -0,0 +1,132 @@
> > +/** @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
> > +
> > +/**
> > +  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 it is.
> > +  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  This              The protocol instance pointer.
> > +  @param  DeviceHandle      The device who initiates the DMA access request.
> > +  @param  BaseAddress       The base of system memory address to be used as the
> DMA memory.
> > +  @param  Length            The length of system memory address to be used as
> the DMA memory.
> > +  @param  IoMmuAttribute    The IOMMU attribute.
> > +
> > +  @retval EFI_SUCCESS            The IoMmuAttribute is set for the memory range
> specified by BaseAddress and Length.
> > +  @retval EFI_INVALID_PARAMETER  DeviceHandle is an invalid handle.
> > +  @retval EFI_INVALID_PARAMETER  BaseAddress 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 BaseAddress 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 UINT64                BaseAddress,
> > +  IN UINT64                Length,
> > +  IN UINT64                IoMmuAttribute
> > +  );
> > +
> > +/**
> > +  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  This      Protocol instance pointer.
> > +  @param  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
> > +  );
> > +
> > +///
> > +/// IOMMU Protocol structure.
> > +///
> > +struct _EDKII_IOMMU_PROTOCOL {
> > +  UINT64                       Revision;
> > +  EDKII_IOMMU_GET_PAGE_SIZE    GetPageSize;
> > +  EDKII_IOMMU_SET_ATTRIBUTE    SetAttribute;
> > +};
> > +
> > +///
> > +/// 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
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> 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] 23+ messages in thread

* Re: [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol definition.
  2017-03-28 23:45       ` Yao, Jiewen
@ 2017-03-29 14:22         ` Ard Biesheuvel
  2017-03-30 12:37           ` Yao, Jiewen
  0 siblings, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2017-03-29 14:22 UTC (permalink / raw)
  To: Yao, Jiewen
  Cc: Kinney, Michael D, Ni, Ruiyu, Leo Duran, edk2-devel@lists.01.org,
	Brijesh Singh

On 29 March 2017 at 00:45, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> Agree. That is a good idea.
>
>
>
> I will add that in V2 patch.
>

Hello Jiewen,

As a bit of background, what I have in mind is an enhancement of the
PCI root bridge I/O allocate, map and unmap methods so that situations
that would currently lead to failure or to suboptimal performance are
left for the IOMMU protocol to handle if one is present. Note that
this may imply having IOMMU protocol instances for each PCI root
bridge, and for other masters as well. (For example, AMD Seattle has
separate IOMMUs for PCI and for the networking controllers, which are
wired to the internal interconnect directly)

So in RootBridgeIoMap(), for instance, we have this condition

  PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
  if ((!RootBridge->DmaAbove4G ||
       (Operation != EfiPciOperationBusMasterRead64 &&
        Operation != EfiPciOperationBusMasterWrite64 &&
        Operation != EfiPciOperationBusMasterCommonBuffer64)) &&
      ((PhysicalAddress + *NumberOfBytes) > SIZE_4GB)) {

to decide whether bounce buffering is necessary (or even possible).
The mapping between DeviceAddress and HostAddress could be supplied by
the IOMMU protocol instance, which also means we should reinterpret
DmaAbove4G and other variables related to 32-bit addressing to apply
to the device address and not the physical address.

Similarly, in RootBridgeIoAllocateBuffer(), a failure to allocate
below 4 GB may not be an error if the IOMMU protocol instance can
provide a 32-bit addressable mapping for it.

I am aware that this complicates matters for you, but having IOMMU
support in the generic PCI host bridge driver is extremely useful for
us. I am happy to help out in any way I can.

Thanks,
Ard.


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

* Re: [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol definition.
  2017-03-29 14:22         ` Ard Biesheuvel
@ 2017-03-30 12:37           ` Yao, Jiewen
  2017-03-30 13:54             ` Ard Biesheuvel
  0 siblings, 1 reply; 23+ messages in thread
From: Yao, Jiewen @ 2017-03-30 12:37 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Kinney, Michael D, Ni, Ruiyu, Leo Duran, edk2-devel@lists.01.org,
	Brijesh Singh

Thanks for the info.

Comment inline.




From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
Sent: Wednesday, March 29, 2017 10:23 PM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Leo Duran <leo.duran@amd.com>; edk2-devel@lists.01.org; Brijesh Singh <brijesh.singh@amd.com>
Subject: Re: [edk2] [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol definition.

On 29 March 2017 at 00:45, Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
> Agree. That is a good idea.
>
>
>
> I will add that in V2 patch.
>

Hello Jiewen,

As a bit of background, what I have in mind is an enhancement of the
PCI root bridge I/O allocate, map and unmap methods so that situations
that would currently lead to failure or to suboptimal performance are
left for the IOMMU protocol to handle if one is present. Note that
this may imply having IOMMU protocol instances for each PCI root
bridge, and for other masters as well. (For example, AMD Seattle has
separate IOMMUs for PCI and for the networking controllers, which are
wired to the internal interconnect directly)

[Jiewen] I am not very sure what do you mean.

Fist, let me explain Intel platform.

There might be multiple IOMMU engines on one platform. one for graphic and one for rest PCI device (such as ATA/USB).
But all IOMMU engines are reported by one “DMAR” ACPI table.

In such case, the Intel IOMMU driver just produces one IOMMU protocol, based upon DMAR ACPI table.

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.

I know AMD has “IVRS” ACPI table and ARM has “IORT” ACPI table.

In such case, I assume AMD may have one IOMMU protocol based upon IVRS table, and ARM may have one IOMMU protocol based upon IORT table.
And this single IOMMU protocol provider can handle multiple IOMMU engines on one system.

Is that understand same as yours?




So in RootBridgeIoMap(), for instance, we have this condition

  PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
  if ((!RootBridge->DmaAbove4G ||
       (Operation != EfiPciOperationBusMasterRead64 &&
        Operation != EfiPciOperationBusMasterWrite64 &&
        Operation != EfiPciOperationBusMasterCommonBuffer64)) &&
      ((PhysicalAddress + *NumberOfBytes) > SIZE_4GB)) {

to decide whether bounce buffering is necessary (or even possible).
The mapping between DeviceAddress and HostAddress could be supplied by
the IOMMU protocol instance, which also means we should reinterpret
DmaAbove4G and other variables related to 32-bit addressing to apply
to the device address and not the physical address.

Similarly, in RootBridgeIoAllocateBuffer(), a failure to allocate
below 4 GB may not be an error if the IOMMU protocol instance can
provide a 32-bit addressable mapping for it.

[Jiewen] It is a good idea to remap based upon IOMMU.

However, one potential problem is that the memory size if not IOMMU page aligned.
In such case, PciRootBridge driver has to allocate another IOMMU page aligned memory for DMA buffer.

I believe the benefit will be got, only if the device driver who submit DMA request allocate IOMMU page aligned memory for DMA request.



I am aware that this complicates matters for you, but having IOMMU
support in the generic PCI host bridge driver is extremely useful for
us. I am happy to help out in any way I can.
[Jiewen] Yes, I definitely need comment for ARM/AMD/other system architecture.



Thanks,
Ard.

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

* Re: [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol definition.
  2017-03-30 12:37           ` Yao, Jiewen
@ 2017-03-30 13:54             ` Ard Biesheuvel
  0 siblings, 0 replies; 23+ messages in thread
From: Ard Biesheuvel @ 2017-03-30 13:54 UTC (permalink / raw)
  To: Yao, Jiewen
  Cc: Kinney, Michael D, Ni, Ruiyu, Leo Duran, edk2-devel@lists.01.org,
	Brijesh Singh

On 30 March 2017 at 13:37, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> Thanks for the info.
>
>
>
> Comment inline.
>
>
>
>
>
>
>
>
>
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Wednesday, March 29, 2017 10:23 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Leo Duran <leo.duran@amd.com>;
> edk2-devel@lists.01.org; Brijesh Singh <brijesh.singh@amd.com>
> Subject: Re: [edk2] [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol
> definition.
>
>
>
> On 29 March 2017 at 00:45, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>> Agree. That is a good idea.
>>
>>
>>
>> I will add that in V2 patch.
>>
>
> Hello Jiewen,
>
> As a bit of background, what I have in mind is an enhancement of the
> PCI root bridge I/O allocate, map and unmap methods so that situations
> that would currently lead to failure or to suboptimal performance are
> left for the IOMMU protocol to handle if one is present. Note that
> this may imply having IOMMU protocol instances for each PCI root
> bridge, and for other masters as well. (For example, AMD Seattle has
> separate IOMMUs for PCI and for the networking controllers, which are
> wired to the internal interconnect directly)
>
>
>
> [Jiewen] I am not very sure what do you mean.
>
>
>
> Fist, let me explain Intel platform.
>
>
>
> There might be multiple IOMMU engines on one platform. one for graphic and
> one for rest PCI device (such as ATA/USB).
>
> But all IOMMU engines are reported by one “DMAR” ACPI table.
>
>
>
> In such case, the Intel IOMMU driver just produces one IOMMU protocol, based
> upon DMAR ACPI table.
>
>
>
> 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.
>
>
>
> I know AMD has “IVRS” ACPI table and ARM has “IORT” ACPI table.
>
>
>
> In such case, I assume AMD may have one IOMMU protocol based upon IVRS
> table, and ARM may have one IOMMU protocol based upon IORT table.
>
> And this single IOMMU protocol provider can handle multiple IOMMU engines on
> one system.
>
>
>
> Is that understand same as yours?
>

OK, that works for me.

>
>
>
>
>
>
> So in RootBridgeIoMap(), for instance, we have this condition
>
>   PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
>   if ((!RootBridge->DmaAbove4G ||
>        (Operation != EfiPciOperationBusMasterRead64 &&
>         Operation != EfiPciOperationBusMasterWrite64 &&
>         Operation != EfiPciOperationBusMasterCommonBuffer64)) &&
>       ((PhysicalAddress + *NumberOfBytes) > SIZE_4GB)) {
>
> to decide whether bounce buffering is necessary (or even possible).
> The mapping between DeviceAddress and HostAddress could be supplied by
> the IOMMU protocol instance, which also means we should reinterpret
> DmaAbove4G and other variables related to 32-bit addressing to apply
> to the device address and not the physical address.
>
> Similarly, in RootBridgeIoAllocateBuffer(), a failure to allocate
> below 4 GB may not be an error if the IOMMU protocol instance can
> provide a 32-bit addressable mapping for it.
>
> [Jiewen] It is a good idea to remap based upon IOMMU.
>
>
>
> However, one potential problem is that the memory size if not IOMMU page
> aligned.
>
> In such case, PciRootBridge driver has to allocate another IOMMU page
> aligned memory for DMA buffer.
>
>
>
> I believe the benefit will be got, only if the device driver who submit DMA
> request allocate IOMMU page aligned memory for DMA request.
>

Well, allocating memory and remapping host memory into the PCI address
space are not the same thing. In the absence of concerns about
isolation, I don't think it should be a problem to round up IOMMU
mappings to sizes it can handle [efficiently].


>
>
>
>
>
> I am aware that this complicates matters for you, but having IOMMU
> support in the generic PCI host bridge driver is extremely useful for
> us. I am happy to help out in any way I can.
> [Jiewen] Yes, I definitely need comment for ARM/AMD/other system
> architecture.
>
>
>
>
>
>
> Thanks,
> Ard.


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

end of thread, other threads:[~2017-03-30 13:54 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-25  9:28 [RFC] [PATCH 0/3] Add IOMMU support Jiewen Yao
2017-03-25  9:28 ` [PATCH 1/3] MdeModulePkg/Include: Add IOMMU protocol definition Jiewen Yao
2017-03-28 22:38   ` Ard Biesheuvel
2017-03-28 23:02     ` Kinney, Michael D
2017-03-28 23:45       ` Yao, Jiewen
2017-03-29 14:22         ` Ard Biesheuvel
2017-03-30 12:37           ` Yao, Jiewen
2017-03-30 13:54             ` Ard Biesheuvel
2017-03-25  9:28 ` [RFC] [PATCH 2/3] MdeModulePkg/PciHostBridge: Add IOMMU support Jiewen Yao
2017-03-27  5:31   ` Ni, Ruiyu
2017-03-27  6:01     ` Yao, Jiewen
2017-03-27  6:18       ` Ni, Ruiyu
2017-03-27  6:23         ` Yao, Jiewen
2017-03-27  6:25           ` Ni, Ruiyu
2017-03-25  9:28 ` [RFC] [PATCH 3/3] MdeModulePkg/PciBus: " Jiewen Yao
2017-03-27  5:39   ` Ni, Ruiyu
2017-03-27  6:15     ` Yao, Jiewen
2017-03-27  6:23       ` Ni, Ruiyu
2017-03-27 19:50 ` [RFC] [PATCH 0/3] " Duran, Leo
2017-03-28  1:40   ` Yao, Jiewen
2017-03-28  1:54     ` Yao, Jiewen
2017-03-28  2:24       ` Ni, Ruiyu
2017-03-28  2:32         ` Yao, Jiewen

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