Re: [PATCH v3 6/6] MdeModulePkg: Modify PciHostBridgeDxe to use new BmDmaLib class library

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

Thanks/Ray

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Leo Duran
> Sent: Friday, February 10, 2017 12:40 AM
> To: edk2-devel@ml01.01.org
> Cc: Tian, Feng <feng.tian@intel.com>; Leo Duran <leo.duran@amd.com>;
> Zeng, Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH v3 6/6] MdeModulePkg: Modify PciHostBridgeDxe to
> use new BmDmaLib class library
> 
> The BmDmaLib class library provides an abstraction layer for Bus-master DMA
> operations as currently implemented by the PciHostBridgeDxe driver. The
> intent is to allow override of the library as required by specific hardware
> implementations, such as AMD's Secure Encrypted Virtualization (SEV).
> 
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> signed-off-by: Leo Duran <leo.duran@amd.com>
> ---
>  .../Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf  |   1 +
>  .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h       |  13 +-
>  .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 272 +++++----------------
>  MdeModulePkg/MdeModulePkg.dsc                      |   1 +
>  4 files changed, 67 insertions(+), 220 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> index d8b0439..35bb5c4 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> @@ -42,6 +42,7 @@ [LibraryClasses]
>    BaseLib
>    PciSegmentLib
>    PciHostBridgeLib
> +  BmDmaLib
> 
>  [Protocols]
>    gEfiMetronomeArchProtocolGuid                   ## CONSUMES
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> index 13185b4..c125fcd 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
> @@ -34,6 +34,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/BaseLib.h>
>  #include <Library/PciSegmentLib.h>
> +#include <Library/BmDmaLib.h>
>  #include "PciHostResource.h"
> 
> 
> @@ -43,17 +44,6 @@ typedef enum {
>    PciOperation
>  } OPERATION_TYPE;
> 
> -#define MAP_INFO_SIGNATURE  SIGNATURE_32 ('_', 'm', 'a', 'p') -typedef
> struct {
> -  UINT32                                    Signature;
> -  LIST_ENTRY                                Link;
> -  EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION Operation;
> -  UINTN                                     NumberOfBytes;
> -  UINTN                                     NumberOfPages;
> -  EFI_PHYSICAL_ADDRESS                      HostAddress;
> -  EFI_PHYSICAL_ADDRESS                      MappedHostAddress;
> -} MAP_INFO;
> -#define MAP_INFO_FROM_LINK(a) CR (a, MAP_INFO, Link,
> MAP_INFO_SIGNATURE)
> 
>  #define PCI_ROOT_BRIDGE_SIGNATURE SIGNATURE_32 ('_', 'p', 'r', 'b')
> 
> @@ -79,7 +69,6 @@ typedef struct {
>    EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL   RootBridgeIo;
> 
>    BOOLEAN                           ResourceSubmitted;
> -  LIST_ENTRY                        Maps;
>  } PCI_ROOT_BRIDGE_INSTANCE;
> 
>  #define ROOT_BRIDGE_FROM_THIS(a) CR (a, PCI_ROOT_BRIDGE_INSTANCE,
> RootBridgeIo, PCI_ROOT_BRIDGE_SIGNATURE) diff --git
> a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index 8af131b..8a74bf6 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -17,7 +17,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #include "PciRootBridge.h"
>  #include "PciHostResource.h"
> 
> -#define NO_MAPPING  (VOID *) (UINTN) -1
> 
>  //
>  // Lookup table for increment values based on transfer widths @@ -55,6
> +54,39 @@ UINT8 mOutStride[] = {
>    0  // EfiPciWidthFillUint64
>  };
> 
> +
> +BM_DMA_OPERATION
> +ConvertDmaOperation (
> +  IN     EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION  RbOperation
> +  )
> +{
> +  BM_DMA_OPERATION BmOperation;
> +
> +  switch (RbOperation) {
> +  case EfiPciOperationBusMasterRead:
> +  case EfiPciOperationBusMasterRead64:
> +    BmOperation = DmaOperationBusMasterRead;
> +    break;
> +
> +  case EfiPciOperationBusMasterWrite:
> +  case EfiPciOperationBusMasterWrite64:
> +    BmOperation = DmaOperationBusMasterWrite;
> +    break;
> +
> +  case EfiPciOperationBusMasterCommonBuffer:
> +  case EfiPciOperationBusMasterCommonBuffer64:
> +    BmOperation = DmaOperationBusMasterCommonBuffer;
> +    break;
> +
> +  default:
> +    BmOperation = DmaOperationBusMasterMaximum;
> +    break;
> +  }
> +
> +  return BmOperation;
> +}
> +
> +
>  /**
>    Construct the Pci Root Bridge instance.
> 
> @@ -168,7 +200,6 @@ CreateRootBridge (
>      TypeMax * sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) + sizeof
> (EFI_ACPI_END_TAG_DESCRIPTOR)
>      );
>    ASSERT (RootBridge->ConfigBuffer != NULL);
> -  InitializeListHead (&RootBridge->Maps);
> 
>    CopyMem (&RootBridge->Bus, &Bridge->Bus, sizeof
> (PCI_ROOT_BRIDGE_APERTURE));
>    CopyMem (&RootBridge->Io, &Bridge->Io, sizeof
> (PCI_ROOT_BRIDGE_APERTURE)); @@ -1053,118 +1084,27 @@
> RootBridgeIoMap (
>    OUT    VOID                                       **Mapping
>    )
>  {
> -  EFI_STATUS                                        Status;
> -  PCI_ROOT_BRIDGE_INSTANCE                          *RootBridge;
> -  EFI_PHYSICAL_ADDRESS                              PhysicalAddress;
> -  MAP_INFO                                          *MapInfo;
> -
> -  if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress ==
> NULL ||
> -      Mapping == NULL) {
> -    return EFI_INVALID_PARAMETER;
> -  }
> -
> -  //
> -  // Make sure that Operation is valid
> -  //
> -  if ((UINT32) Operation >= EfiPciOperationMaximum) {
> -    return EFI_INVALID_PARAMETER;
> -  }
> +  PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> +  BOOLEAN                  DmaAbove4GB;
> +  BM_DMA_OPERATION         BmOperation;
> 
>    RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> -
> -  PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
> -  if ((!RootBridge->DmaAbove4G ||
> -       (Operation != EfiPciOperationBusMasterRead64 &&
> -        Operation != EfiPciOperationBusMasterWrite64 &&
> -        Operation != EfiPciOperationBusMasterCommonBuffer64)) &&
> -      ((PhysicalAddress + *NumberOfBytes) > SIZE_4GB)) {
> -
> -    //
> -    // If the root bridge or the device cannot handle performing DMA above
> -    // 4GB but any part of the DMA transfer being mapped is above 4GB, then
> -    // map the DMA transfer to a buffer below 4GB.
> -    //
> -
> -    if (Operation == EfiPciOperationBusMasterCommonBuffer ||
> -        Operation == EfiPciOperationBusMasterCommonBuffer64) {
> -      //
> -      // Common Buffer operations can not be remapped.  If the common
> buffer
> -      // if above 4GB, then it is not possible to generate a mapping, so return
> -      // an error.
> -      //
> -      return EFI_UNSUPPORTED;
> -    }
> -
> -    //
> -    // Allocate a MAP_INFO structure to remember the mapping when
> Unmap() is
> -    // called later.
> -    //
> -    MapInfo = AllocatePool (sizeof (MAP_INFO));
> -    if (MapInfo == NULL) {
> -      *NumberOfBytes = 0;
> -      return EFI_OUT_OF_RESOURCES;
> -    }
> -
> -    //
> -    // Initialize the MAP_INFO structure
> -    //
> -    MapInfo->Signature         = MAP_INFO_SIGNATURE;
> -    MapInfo->Operation         = Operation;
> -    MapInfo->NumberOfBytes     = *NumberOfBytes;
> -    MapInfo->NumberOfPages     = EFI_SIZE_TO_PAGES (MapInfo-
> >NumberOfBytes);
> -    MapInfo->HostAddress       = PhysicalAddress;
> -    MapInfo->MappedHostAddress = SIZE_4GB - 1;
> -
> -    //
> -    // Allocate a buffer below 4GB to map the transfer to.
> -    //
> -    Status = gBS->AllocatePages (
> -                    AllocateMaxAddress,
> -                    EfiBootServicesData,
> -                    MapInfo->NumberOfPages,
> -                    &MapInfo->MappedHostAddress
> -                    );
> -    if (EFI_ERROR (Status)) {
> -      FreePool (MapInfo);
> -      *NumberOfBytes = 0;
> -      return Status;
> -    }
> -
> -    //
> -    // If this is a read operation from the Bus Master's point of view,
> -    // then copy the contents of the real buffer into the mapped buffer
> -    // so the Bus Master can read the contents of the real buffer.
> -    //
> -    if (Operation == EfiPciOperationBusMasterRead ||
> -        Operation == EfiPciOperationBusMasterRead64) {
> -      CopyMem (
> -        (VOID *) (UINTN) MapInfo->MappedHostAddress,
> -        (VOID *) (UINTN) MapInfo->HostAddress,
> -        MapInfo->NumberOfBytes
> -        );
> -    }
> -
> -    InsertTailList (&RootBridge->Maps, &MapInfo->Link);
> -
> -    //
> -    // The DeviceAddress is the address of the maped buffer below 4GB
> -    //
> -    *DeviceAddress = MapInfo->MappedHostAddress;
> -    //
> -    // Return a pointer to the MAP_INFO structure in Mapping
> -    //
> -    *Mapping       = MapInfo;
> -  } else {
> -    //
> -    // If the root bridge CAN handle performing DMA above 4GB or
> -    // the transfer is below 4GB, so the DeviceAddress is simply the
> -    // HostAddress
> -    //
> -    *DeviceAddress = PhysicalAddress;
> -    *Mapping       = NO_MAPPING;
> -  }
> -
> -  return EFI_SUCCESS;
> +  DmaAbove4GB = RootBridge->DmaAbove4G && (
> +                  Operation == EfiPciOperationBusMasterRead64 ||
> +                  Operation == EfiPciOperationBusMasterWrite64 ||
> +                  Operation == EfiPciOperationBusMasterCommonBuffer64
> +                  );
> +
> +  BmOperation = ConvertDmaOperation (Operation);
> +
> +  return BmDmaMap (
> +           DmaAbove4GB,
> +           BmOperation,
> +           HostAddress,
> +           NumberOfBytes,
> +           DeviceAddress,
> +           Mapping
> +           );
>  }
> 
>  /**
> @@ -1191,58 +1131,7 @@ RootBridgeIoUnmap (
>    IN VOID                             *Mapping
>    )
>  {
> -  MAP_INFO                 *MapInfo;
> -  LIST_ENTRY               *Link;
> -  PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> -
> -  RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> -  //
> -  // See if the Map() operation associated with this Unmap() required a
> mapping
> -  // buffer. If a mapping buffer was not required, then this function simply
> -  // returns EFI_SUCCESS.
> -  //
> -  if (Mapping == NO_MAPPING) {
> -    return EFI_SUCCESS;
> -  }
> -
> -  MapInfo = NO_MAPPING;
> -  for (Link = GetFirstNode (&RootBridge->Maps)
> -       ; !IsNull (&RootBridge->Maps, Link)
> -       ; Link = GetNextNode (&RootBridge->Maps, Link)
> -       ) {
> -    MapInfo = MAP_INFO_FROM_LINK (Link);
> -    if (MapInfo == Mapping) {
> -      break;
> -    }
> -  }
> -  //
> -  // Mapping is not a valid value returned by Map()
> -  //
> -  if (MapInfo != Mapping) {
> -    return EFI_INVALID_PARAMETER;
> -  }
> -  RemoveEntryList (&MapInfo->Link);
> -
> -  //
> -  // If this is a write operation from the Bus Master's point of view,
> -  // then copy the contents of the mapped buffer into the real buffer
> -  // so the processor can read the contents of the real buffer.
> -  //
> -  if (MapInfo->Operation == EfiPciOperationBusMasterWrite ||
> -      MapInfo->Operation == EfiPciOperationBusMasterWrite64) {
> -    CopyMem (
> -      (VOID *) (UINTN) MapInfo->HostAddress,
> -      (VOID *) (UINTN) MapInfo->MappedHostAddress,
> -      MapInfo->NumberOfBytes
> -      );
> -  }
> -
> -  //
> -  // Free the mapped buffer and the MAP_INFO structure.
> -  //
> -  gBS->FreePages (MapInfo->MappedHostAddress, MapInfo-
> >NumberOfPages);
> -  FreePool (Mapping);
> -  return EFI_SUCCESS;
> +  return BmDmaUnmap (Mapping);
>  }
> 
>  /**
> @@ -1282,56 +1171,23 @@ RootBridgeIoAllocateBuffer (
>    IN  UINT64                           Attributes
>    )
>  {
> -  EFI_STATUS                Status;
> -  EFI_PHYSICAL_ADDRESS      PhysicalAddress;
> -  PCI_ROOT_BRIDGE_INSTANCE  *RootBridge;
> -  EFI_ALLOCATE_TYPE         AllocateType;
> +  PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> +  BOOLEAN                  DmaAbove4GB;
> 
> -  //
> -  // Validate Attributes
> -  //
>    if ((Attributes & EFI_PCI_ATTRIBUTE_INVALID_FOR_ALLOCATE_BUFFER) !=
> 0) {
>      return EFI_UNSUPPORTED;
>    }
> 
> -  //
> -  // Check for invalid inputs
> -  //
> -  if (HostAddress == NULL) {
> -    return EFI_INVALID_PARAMETER;
> -  }
> -
> -  //
> -  // The only valid memory types are EfiBootServicesData and
> -  // EfiRuntimeServicesData
> -  //
> -  if (MemoryType != EfiBootServicesData &&
> -      MemoryType != EfiRuntimeServicesData) {
> -    return EFI_INVALID_PARAMETER;
> -  }
> -
>    RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> +  DmaAbove4GB = RootBridge->DmaAbove4G &&
> +                  (Attributes & EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE)
> + != 0;
> 
> -  AllocateType = AllocateAnyPages;
> -  if (!RootBridge->DmaAbove4G ||
> -      (Attributes & EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0) {
> -    //
> -    // Limit allocations to memory below 4GB
> -    //
> -    AllocateType    = AllocateMaxAddress;
> -    PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (SIZE_4GB - 1);
> -  }
> -  Status = gBS->AllocatePages (
> -                  AllocateType,
> -                  MemoryType,
> -                  Pages,
> -                  &PhysicalAddress
> -                  );
> -  if (!EFI_ERROR (Status)) {
> -    *HostAddress = (VOID *) (UINTN) PhysicalAddress;
> -  }
> -
> -  return Status;
> +  return BmDmaAllocateBuffer (
> +           DmaAbove4GB,
> +           MemoryType,
> +           Pages,
> +           HostAddress
> +           );
>  }
> 
>  /**
> @@ -1356,7 +1212,7 @@ RootBridgeIoFreeBuffer (
>    OUT VOID                             *HostAddress
>    )
>  {
> -  return gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress,
> Pages);
> +  return BmDmaFreeBuffer (Pages, HostAddress);
>  }
> 
>  /**
> diff --git a/MdeModulePkg/MdeModulePkg.dsc
> b/MdeModulePkg/MdeModulePkg.dsc index b343c19..47aa9b9 100644
> --- a/MdeModulePkg/MdeModulePkg.dsc
> +++ b/MdeModulePkg/MdeModulePkg.dsc
> @@ -83,6 +83,7 @@ [LibraryClasses]
>    PalLib|MdePkg/Library/BasePalLibNull/BasePalLibNull.inf
> 
> CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/Custo
> mizedDisplayLib.inf
> 
> FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBuffer
> BltLib.inf
> +  BmDmaLib|MdeModulePkg/Library/DxeBmDmaLib/DxeBmDmaLib.inf
>    #
>    # Misc
>    #
> --
> 1.9.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel