Re: [PATCH v4 4/5] MdeModulePkg/NonDiscoverablePciDeviceDxe: add support for non-coherent DMA

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

Comments in NonCoherentPciIoAllocateBuffer().

Thanks/Ray

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Friday, November 25, 2016 11:42 PM
> To: edk2-devel@lists.01.org; leif.lindholm@linaro.org; Gao, Liming
> <liming.gao@intel.com>; afish@apple.com; Ni, Ruiyu <ruiyu.ni@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>; mw@semihalf.com; Tian,
> Feng <feng.tian@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [PATCH v4 4/5] MdeModulePkg/NonDiscoverablePciDeviceDxe: add
> support for non-coherent DMA
> 
> Add support for non-coherent DMA, either by performing explicit cache
> maintenance when DMA mappings are aligned to the CPU's DMA buffer
> alignment, or by bounce buffering via uncached mappings otherwise.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> 
> MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci
> DeviceDxe.c   |  14 +-
> 
> MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci
> DeviceDxe.inf |   2 +
> 
> MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci
> DeviceIo.c    | 319 ++++++++++++++++++++
> 
> MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci
> DeviceIo.h    |  29 ++
>  4 files changed, 357 insertions(+), 7 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceDxe.c
> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceDxe.c
> index ee765d7a5d9c..0fcf2b2ec1bf 100644
> ---
> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceDxe.c
> +++
> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> Pc
> +++ iDeviceDxe.c
> @@ -16,6 +16,8 @@
> 
>  #include <Protocol/DriverBinding.h>
> 
> +EFI_CPU_ARCH_PROTOCOL      *mCpu;
> +
>  //
>  // We only support the following device types  // @@ -69,14 +71,7 @@
> NonDiscoverablePciDeviceSupported (
>      return Status;
>    }
> 
> -  //
> -  // Restricted to DMA coherent for now
> -  //
>    Status = EFI_UNSUPPORTED;
> -  if (Device->DmaType != NonDiscoverableDeviceDmaTypeCoherent) {
> -    goto CloseProtocol;
> -  }
> -
>    for (Idx = 0; Idx < ARRAY_SIZE (SupportedNonDiscoverableDevices); Idx++)
> {
>      if (CompareGuid (Device->Type, SupportedNonDiscoverableDevices [Idx]))
> {
>        Status = EFI_SUCCESS;
> @@ -224,6 +219,11 @@ NonDiscoverablePciDeviceDxeEntryPoint (
>    IN EFI_SYSTEM_TABLE *SystemTable
>    )
>  {
> +  EFI_STATUS      Status;
> +
> +  Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID
> + **)&mCpu);  ASSERT_EFI_ERROR(Status);
> +
>    return EfiLibInstallDriverBindingComponentName2 (
>             ImageHandle,
>             SystemTable,
> diff --git
> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceDxe.inf
> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceDxe.inf
> index 996fe310e0e3..5faa8945134c 100644
> ---
> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceDxe.inf
> +++
> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> Pc
> +++ iDeviceDxe.inf
> @@ -32,6 +32,7 @@ [Packages]
>  [LibraryClasses]
>    BaseMemoryLib
>    DebugLib
> +  DxeServicesTableLib
>    MemoryAllocationLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
> @@ -40,6 +41,7 @@ [LibraryClasses]
>  [Protocols]
>    gEfiPciIoProtocolGuid                         ## BY_START
>    gEdkiiNonDiscoverableDeviceProtocolGuid       ## TO_START
> +  gEfiCpuArchProtocolGuid                       ## CONSUMES
> 
>  [Guids]
>    gEdkiiNonDiscoverableAhciDeviceGuid
> diff --git
> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceIo.c
> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceIo.c
> index fd59267a8d66..5a4861a17717 100644
> ---
> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceIo.c
> +++
> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> Pc
> +++ iDeviceIo.c
> @@ -15,6 +15,8 @@
> 
>  #include "NonDiscoverablePciDeviceIo.h"
> 
> +#include <Library/DxeServicesTableLib.h>
> +
>  #include <IndustryStandard/Acpi.h>
> 
>  #include <Protocol/PciRootBridgeIo.h>
> @@ -523,6 +525,316 @@ CoherentPciIoFreeBuffer (
>    return EFI_SUCCESS;
>  }
> 
> +STATIC
> +EFI_STATUS
> +NonCoherentPciIoFreeBuffer (
> +  IN  EFI_PCI_IO_PROTOCOL         *This,
> +  IN  UINTN                       Pages,
> +  IN  VOID                        *HostAddress
> +  )
> +{
> +  NON_DISCOVERABLE_PCI_DEVICE                   *Dev;
> +  LIST_ENTRY                                    *Entry;
> +  EFI_STATUS                                    Status;
> +  NON_DISCOVERABLE_DEVICE_UNCACHED_ALLOCATION   *Alloc;
> +  BOOLEAN                                       Found;
> +
> +  Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This);
> +
> +  Found = FALSE;
> +
> +  //
> +  // Find the uncached allocation list entry associated  // with this
> + allocation  //  for (Entry = Dev->UncachedAllocationList.ForwardLink;
> +       Entry != &Dev->UncachedAllocationList;
> +       Entry = Entry->ForwardLink) {
> +
> +    Alloc = BASE_CR (Entry,
> NON_DISCOVERABLE_DEVICE_UNCACHED_ALLOCATION, List);
> +    if (Alloc->HostAddress == HostAddress && Alloc->NumPages == Pages) {
> +      //
> +      // We are freeing the exact allocation we were given
> +      // before by AllocateBuffer()
> +      //
> +      Found = TRUE;
> +      break;
> +    }
> +  }
> +
> +  if (!Found) {
> +    ASSERT_EFI_ERROR (EFI_NOT_FOUND);
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  RemoveEntryList (&Alloc->List);
> +
> +  Status = gDS->SetMemorySpaceAttributes (
> +                  (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress,
> +                  EFI_PAGES_TO_SIZE (Pages),
> +                  Alloc->Attributes);
> +  if (EFI_ERROR (Status)) {
> +    goto FreeAlloc;
> +  }
> +
> +  //
> +  // If we fail to restore the original attributes, it is better to
> + leak the  // memory than to return it to the heap  //  FreePages
> + (HostAddress, Pages);
> +
> +FreeAlloc:
> +  FreePool (Alloc);
> +  return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
> +NonCoherentPciIoAllocateBuffer (
> +  IN  EFI_PCI_IO_PROTOCOL         *This,
> +  IN  EFI_ALLOCATE_TYPE           Type,
> +  IN  EFI_MEMORY_TYPE             MemoryType,
> +  IN  UINTN                       Pages,
> +  OUT VOID                        **HostAddress,
> +  IN  UINT64                      Attributes
> +  )
> +{
> +  NON_DISCOVERABLE_PCI_DEVICE                 *Dev;
> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR             GcdDescriptor;
> +  EFI_STATUS                                  Status;
> +  UINT64                                      MemType;
> +  NON_DISCOVERABLE_DEVICE_UNCACHED_ALLOCATION *Alloc;
> +
> +  Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This);
> +
> +  Status = CoherentPciIoAllocateBuffer (This, Type, MemoryType, Pages,
> +             HostAddress, Attributes);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = gDS->GetMemorySpaceDescriptor (
> +                  (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress,
> +                  &GcdDescriptor);
> +  if (EFI_ERROR (Status)) {
> +    goto FreeBuffer;

1. in FreeBuffer, NonCoherentPciIoFreeBuffer() is called.
But since the allocation entry hasn't been allocated and inserted
to the list, the NonCoherentPciIoFreeBuffer() will assert and return
EFI_NOT_FOUND, resulting the memory leak.

> +  }
> +
> +  if ((GcdDescriptor.Capabilities & (EFI_MEMORY_WC | EFI_MEMORY_UC))
> == 0) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  //
> +  // Set the preferred memory attributes  //  if ((Attributes &
> + EFI_PCI_ATTRIBUTE_MEMORY_WRITE_COMBINE) != 0 ||
> +      (GcdDescriptor.Capabilities & EFI_MEMORY_UC) == 0) {
> +    //
> +    // Use write combining if it was requested, or if it is the only
> +    // type supported by the region.
> +    //
> +    MemType = EFI_MEMORY_WC;
> +  } else {
> +    MemType = EFI_MEMORY_UC;
> +  }
> +
> +  Alloc = AllocatePool (sizeof *Alloc);  if (Alloc == NULL) {
> +    goto FreeBuffer;

2. Same as #1.
Did you mean to call CoherentPciIoFreeBuffer()?

> +  }
> +
> +  Alloc->HostAddress = *HostAddress;
> +  Alloc->NumPages = Pages;
> +  Alloc->Attributes = GcdDescriptor.Attributes;
> +
> +  //
> +  // Record this allocation in the linked list, so we  // can restore
> + the memory space attributes later  //  InsertHeadList
> + (&Dev->UncachedAllocationList, &Alloc->List);
> +
> +  Status = gDS->SetMemorySpaceAttributes (
> +                  (EFI_PHYSICAL_ADDRESS)(UINTN)*HostAddress,
> +                  EFI_PAGES_TO_SIZE (Pages),
> +                  MemType);
> +  if (EFI_ERROR (Status)) {
> +    goto RemoveList;

3. same as #1.

> +  }
> +
> +  Status = mCpu->FlushDataCache (
> +                   mCpu,
> +                   (EFI_PHYSICAL_ADDRESS)(UINTN)*HostAddress,
> +                   EFI_PAGES_TO_SIZE (Pages),
> +                   EfiCpuFlushTypeInvalidate);  if (EFI_ERROR (Status))
> + {
> +    goto RemoveList;
4. same as #1.

> +  }
> +
> +  return EFI_SUCCESS;
> +
> +RemoveList:
> +  RemoveEntryList (&Alloc->List);
> +  FreePool (Alloc);
> +
> +FreeBuffer:
> +  NonCoherentPciIoFreeBuffer (This, Pages, *HostAddress);
> +  return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
> +NonCoherentPciIoMap (
> +  IN     EFI_PCI_IO_PROTOCOL            *This,
> +  IN     EFI_PCI_IO_PROTOCOL_OPERATION  Operation,
> +  IN     VOID                           *HostAddress,
> +  IN OUT UINTN                          *NumberOfBytes,
> +  OUT    EFI_PHYSICAL_ADDRESS           *DeviceAddress,
> +  OUT    VOID                           **Mapping
> +  )
> +{
> +  NON_DISCOVERABLE_PCI_DEVICE           *Dev;
> +  EFI_STATUS                            Status;
> +  NON_DISCOVERABLE_PCI_DEVICE_MAP_INFO  *MapInfo;
> +  UINTN                                 AlignMask;
> +  VOID                                  *AllocAddress;
> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR       GcdDescriptor;
> +  BOOLEAN                               Bounce;
> +
> +  MapInfo = AllocatePool (sizeof *MapInfo);  if (MapInfo == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  MapInfo->HostAddress = HostAddress;
> +  MapInfo->Operation = Operation;
> +  MapInfo->NumberOfBytes = *NumberOfBytes;
> +
> +  Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This);
> +
> +  //
> +  // If this device does not support 64-bit DMA addressing, we need to
> + allocate  // a bounce buffer and copy over the data in case HostAddress >=
> 4 GB.
> +  //
> +  Bounce = ((Dev->Attributes &
> EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0 &&
> +            (UINTN)HostAddress + *NumberOfBytes > SIZE_4GB);
> +
> +  if (!Bounce) {
> +    switch (Operation) {
> +    case EfiPciIoOperationBusMasterRead:
> +    case EfiPciIoOperationBusMasterWrite:
> +      //
> +      // For streaming DMA, it is sufficient if the buffer is aligned to
> +      // the CPUs DMA buffer alignment.
> +      //
> +      AlignMask = mCpu->DmaBufferAlignment - 1;
> +      if ((((UINTN) HostAddress | *NumberOfBytes) & AlignMask) == 0) {
> +        break;
> +      }
> +      // fall through
> +
> +    case EfiPciIoOperationBusMasterCommonBuffer:
> +      //
> +      // Check whether the host address refers to an uncached mapping.
> +      //
> +      Status = gDS->GetMemorySpaceDescriptor (
> +                      (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress,
> +                      &GcdDescriptor);
> +      if (EFI_ERROR (Status) ||
> +          (GcdDescriptor.Attributes &
> (EFI_MEMORY_WB|EFI_MEMORY_WT)) != 0) {
> +        Bounce = TRUE;
> +      }
> +      break;
> +
> +    default:
> +      ASSERT (FALSE);
> +    }
> +  }
> +
> +  if (Bounce) {
> +    if (Operation == EfiPciIoOperationBusMasterCommonBuffer) {
> +      Status = EFI_DEVICE_ERROR;
> +      goto FreeMapInfo;
> +    }
> +
> +    Status = NonCoherentPciIoAllocateBuffer (This, AllocateAnyPages,
> +               EfiBootServicesData, EFI_SIZE_TO_PAGES (MapInfo-
> >NumberOfBytes),
> +               &AllocAddress, EFI_PCI_ATTRIBUTE_MEMORY_WRITE_COMBINE);
> +    if (EFI_ERROR (Status)) {
> +      goto FreeMapInfo;
> +    }
> +    MapInfo->AllocAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocAddress;
> +    if (Operation == EfiPciIoOperationBusMasterRead) {
> +      gBS->CopyMem (AllocAddress, HostAddress, *NumberOfBytes);
> +    }
> +    *DeviceAddress = MapInfo->AllocAddress;  } else {
> +    MapInfo->AllocAddress = 0;
> +    *DeviceAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress;
> +
> +    //
> +    // We are not using a bounce buffer: the mapping is sufficiently
> +    // aligned to allow us to simply flush the caches. Note that cleaning
> +    // the caches is necessary for both data directions:
> +    // - for bus master read, we want the latest data to be present
> +    //   in main memory
> +    // - for bus master write, we don't want any stale dirty cachelines that
> +    //   may be written back unexpectedly, and clobber the data written to
> +    //   main memory by the device.
> +    //
> +    mCpu->FlushDataCache (mCpu,
> (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress,
> +            *NumberOfBytes, EfiCpuFlushTypeWriteBack);  }
> +
> +  *Mapping = MapInfo;
> +  return EFI_SUCCESS;
> +
> +FreeMapInfo:
> +  FreePool (MapInfo);
> +
> +  return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
> +NonCoherentPciIoUnmap (
> +  IN  EFI_PCI_IO_PROTOCOL          *This,
> +  IN  VOID                         *Mapping
> +  )
> +{
> +  NON_DISCOVERABLE_PCI_DEVICE_MAP_INFO  *MapInfo;
> +
> +  if (Mapping == NULL) {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  MapInfo = Mapping;
> +  if (MapInfo->AllocAddress != 0) {
> +    //
> +    // We are using a bounce buffer: copy back the data if necessary,
> +    // and free the buffer.
> +    //
> +    if (MapInfo->Operation == EfiPciIoOperationBusMasterWrite) {
> +      gBS->CopyMem (MapInfo->HostAddress, (VOID *)(UINTN)MapInfo-
> >AllocAddress,
> +             MapInfo->NumberOfBytes);
> +    }
> +    NonCoherentPciIoFreeBuffer (This,
> +      EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes),
> +      (VOID *)(UINTN)MapInfo->AllocAddress);
> +  } else {
> +    //
> +    // We are *not* using a bounce buffer: if this is a bus master write,
> +    // we have to invalidate the caches so the CPU will see the uncached
> +    // data written by the device.
> +    //
> +    if (MapInfo->Operation == EfiPciIoOperationBusMasterWrite) {
> +      mCpu->FlushDataCache (mCpu,
> +              (EFI_PHYSICAL_ADDRESS)(UINTN)MapInfo->HostAddress,
> +              MapInfo->NumberOfBytes, EfiCpuFlushTypeInvalidate);
> +    }
> +  }
> +  FreePool (MapInfo);
> +  return EFI_SUCCESS;
> +}
> 
>  STATIC
>  EFI_STATUS
> @@ -715,6 +1027,13 @@ InitializePciIoProtocol (
>    // Copy protocol structure
>    CopyMem(&Dev->PciIo, &PciIoTemplate, sizeof PciIoTemplate);
> 
> +  if (Dev->Device->DmaType ==
> NonDiscoverableDeviceDmaTypeNonCoherent) {
> +    Dev->PciIo.AllocateBuffer   = NonCoherentPciIoAllocateBuffer;
> +    Dev->PciIo.FreeBuffer       = NonCoherentPciIoFreeBuffer;
> +    Dev->PciIo.Map              = NonCoherentPciIoMap;
> +    Dev->PciIo.Unmap            = NonCoherentPciIoUnmap;
> +  }
> +
>    if (CompareGuid (Dev->Device->Type,
> &gEdkiiNonDiscoverableAhciDeviceGuid)) {
>      Dev->ConfigSpace.Hdr.ClassCode[0] = PCI_IF_MASS_STORAGE_AHCI;
>      Dev->ConfigSpace.Hdr.ClassCode[1] =
> PCI_CLASS_MASS_STORAGE_SATADPA; diff --git
> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceIo.h
> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceIo.h
> index bc0a3d3258f9..449614862911 100644
> ---
> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceIo.h
> +++
> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> Pc
> +++ iDeviceIo.h
> @@ -15,6 +15,8 @@
>  #ifndef __NON_DISCOVERABLE_PCI_DEVICE_IO_H__
>  #define __NON_DISCOVERABLE_PCI_DEVICE_IO_H__
> 
> +#include <PiDxe.h>
> +
>  #include <Library/BaseMemoryLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/MemoryAllocationLib.h> @@ -25,6 +27,7 @@
> 
>  #include <Protocol/ComponentName.h>
>  #include <Protocol/NonDiscoverableDevice.h>
> +#include <Protocol/Cpu.h>
>  #include <Protocol/PciIo.h>
> 
>  #define NON_DISCOVERABLE_PCI_DEVICE_SIG SIGNATURE_32 ('P', 'P', 'I',
> 'D') @@ -38,6 +41,27 @@
> 
>  #define PCI_MAX_BARS                  6
> 
> +extern EFI_CPU_ARCH_PROTOCOL      *mCpu;
> +
> +typedef struct {
> +  //
> +  // The linked-list next pointer
> +  //
> +  LIST_ENTRY          List;
> +  //
> +  // The address of the uncached allocation
> +  //
> +  VOID                *HostAddress;
> +  //
> +  // The number of pages in the allocation
> +  //
> +  UINTN               NumPages;
> +  //
> +  // The attributes of the allocation
> +  //
> +  UINT64              Attributes;
> +} NON_DISCOVERABLE_DEVICE_UNCACHED_ALLOCATION;
> +
>  typedef struct {
>    UINT32                    Signature;
>    //
> @@ -71,6 +95,11 @@ typedef struct {
>    // Whether this device has been enabled
>    //
>    BOOLEAN                   Enabled;
> +  //
> +  // Linked list to keep track of uncached allocations performed  // on
> + behalf of this device  //
> +  LIST_ENTRY                UncachedAllocationList;
>  } NON_DISCOVERABLE_PCI_DEVICE;
> 
>  VOID
> --
> 2.7.4