Re: [PATCH 5/5] EmbeddedPkg/PlatformPciIoDxe: add support for non-coherent DMA

[Leif Lindholm <leif.lindholm@linaro.org>]

On Mon, Oct 31, 2016 at 06:13:10PM +0000, Ard Biesheuvel wrote:
> Add support for bounce buffering using uncached mappings when DMA mappings
> are not aligned to the CPU's DMA buffer alignment.

This description does not appear to match the subject line?
And the contents of the patch seems to do both.

Anyway, I'll pass on a proper technical review until my head is a bit
clearer, but a few comments/questions below.

> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.c      | 204 ++++++++++++++++++++
>  EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.h      |   5 +
>  EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.c   |  17 +-
>  EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.inf |   2 +
>  4 files changed, 221 insertions(+), 7 deletions(-)
> 
> diff --git a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.c b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.c
> index 97ed19353347..658d096c73c1 100644
> --- a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.c
> +++ b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.c
> @@ -15,6 +15,8 @@
>  
>  #include "PlatformPciIo.h"
>  
> +#include <Library/DxeServicesTableLib.h>
> +
>  #include <Protocol/PciRootBridgeIo.h>
>  
>  typedef struct {
> @@ -454,6 +456,201 @@ CoherentPciIoFreeBuffer (
>    return EFI_SUCCESS;
>  }
>  
> +STATIC
> +EFI_STATUS
> +NonCoherentPciIoFreeBuffer (
> +  IN  EFI_PCI_IO_PROTOCOL         *This,
> +  IN  UINTN                       Pages,
> +  IN  VOID                        *HostAddress
> +  )
> +{
> +  EFI_STATUS        Status;
> +
> +  Status = gDS->SetMemorySpaceAttributes (
> +                  (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress,
> +                  EFI_PAGES_TO_SIZE (Pages),
> +                  EFI_MEMORY_WB);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  FreePages (HostAddress, Pages);
> +  return EFI_SUCCESS;
> +}
> +
> +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
> +  )
> +{
> +  EFI_STATUS        Status;
> +
> +  Status = CoherentPciIoAllocateBuffer (This, Type, MemoryType, Pages,
> +             HostAddress, Attributes);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = gDS->SetMemorySpaceAttributes (
> +                  (EFI_PHYSICAL_ADDRESS)(UINTN)*HostAddress,
> +                  EFI_PAGES_TO_SIZE (Pages),
> +                  EFI_MEMORY_WC);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = gCpu->FlushDataCache (
> +                   gCpu,
> +                   (EFI_PHYSICAL_ADDRESS)(UINTN)*HostAddress,
> +                   EFI_PAGES_TO_SIZE (Pages),
> +                   EfiCpuFlushTypeInvalidate);
> +  if (EFI_ERROR (Status)) {
> +    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
> +  )
> +{
> +  PLATFORM_PCI_IO_DEV               *Dev;
> +  EFI_STATUS                        Status;
> +  PLATFORM_PCI_IO_MAP_INFO          *MapInfo;
> +  UINTN                             AlignMask;
> +  VOID                              *AllocAddress;
> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR   GcdDescriptor;
> +  BOOLEAN                           UncachedMapping;
> +
> +  MapInfo = (PLATFORM_PCI_IO_MAP_INFO *)AllocatePool (sizeof *MapInfo);
> +  if (MapInfo == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  MapInfo->HostAddress = HostAddress;
> +  MapInfo->Operation = Operation;
> +  MapInfo->NumberOfBytes = *NumberOfBytes;
> +
> +  //
> +  // Bounce buffering is not possible for consistent mappings, so
> +  // check we are mapping a cached buffer for consistent DMA
> +  //
> +  if (Operation == EfiPciIoOperationBusMasterCommonBuffer) {
> +    Status = gDS->GetMemorySpaceDescriptor (
> +                    (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress,
> +                    &GcdDescriptor);
> +    if (!EFI_ERROR (Status)) {
> +      UncachedMapping = (GcdDescriptor.Attributes & EFI_MEMORY_WC) != 0;
> +    } else {
> +      UncachedMapping = FALSE;
> +    }
> +  }
> +
> +  //
> +  // If this device does not support 64-bit DMA addressing, we need to allocate
> +  // a bounce buffer and copy over the data if HostAddress >= 4 GB. We also need
> +  // to allocate a bounce buffer if the mapping is not aligned to the CPU's
> +  // DMA buffer alignment.
> +  //
> +  Dev = PLATFORM_PCI_IO_DEV_FROM_PCI_IO(This);
> +  AlignMask = gCpu->DmaBufferAlignment - 1;
> +  if (((Dev->Attributes & EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0 &&
> +       (UINTN) HostAddress >= SIZE_4GB) ||
> +       ((((UINTN) HostAddress || *NumberOfBytes) & AlignMask) != 0 &&
> +        !UncachedMapping)) {
> +
> +    Status = NonCoherentPciIoAllocateBuffer (This, AllocateAnyPages,
> +               EfiBootServicesData, EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes),
> +               &AllocAddress, 0);
> +    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.
> +    //
> +    gCpu->FlushDataCache (gCpu, (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
> +  )
> +{
> +  PLATFORM_PCI_IO_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 *)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) {
> +      gCpu->FlushDataCache (gCpu,
> +              (EFI_PHYSICAL_ADDRESS)(UINTN)MapInfo->HostAddress,
> +              MapInfo->NumberOfBytes, EfiCpuFlushTypeInvalidate);
> +    }
> +  }
> +  FreePool (MapInfo);
> +  return EFI_SUCCESS;
> +}
>  
>  STATIC
>  EFI_STATUS
> @@ -646,4 +843,11 @@ InitializePciIoProtocol (
>  
>    // Copy protocol structure
>    CopyMem(&PlatformPciIoDev->PciIo, &PciIoTemplate, sizeof PciIoTemplate);
> +
> +  if (PlatformPciIoDev->PlatformPciIo->DmaType == PlatformPciIoDmaNonCoherent) {
> +    PlatformPciIoDev->PciIo.AllocateBuffer = NonCoherentPciIoAllocateBuffer;
> +    PlatformPciIoDev->PciIo.FreeBuffer = NonCoherentPciIoFreeBuffer;
> +    PlatformPciIoDev->PciIo.Map = NonCoherentPciIoMap;
> +    PlatformPciIoDev->PciIo.Unmap = NonCoherentPciIoUnmap;
> +  }
>  }
> diff --git a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.h b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.h
> index 8fd8dc5e4a11..b7b792b85ae4 100644
> --- a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.h
> +++ b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.h
> @@ -12,6 +12,8 @@
>  
>  **/
>  
> +#include <PiDxe.h>
> +
>  #include <Library/BaseMemoryLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/MemoryAllocationLib.h>
> @@ -20,6 +22,7 @@
>  
>  #include <IndustryStandard/Pci.h>
>  
> +#include <Protocol/Cpu.h>
>  #include <Protocol/PciIo.h>
>  #include <Protocol/PlatformPciIo.h>
>  
> @@ -28,6 +31,8 @@
>  #define PLATFORM_PCI_IO_DEV_FROM_PCI_IO(PciIoPointer) \
>            CR (PciIoPointer, PLATFORM_PCI_IO_DEV, PciIo, PLATFORM_PCI_IO_SIG)
>  
> +extern EFI_CPU_ARCH_PROTOCOL      *gCpu;
> +
>  typedef struct {
>    UINT32                  Signature;
>    //
> diff --git a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.c b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.c
> index 7f3306e7e891..fa4719686a6d 100644
> --- a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.c
> +++ b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.c
> @@ -17,6 +17,8 @@
>  #include <Protocol/ComponentName.h>
>  #include <Protocol/DriverBinding.h>
>  
> +EFI_CPU_ARCH_PROTOCOL      *gCpu;
> +

Should this not really be pulled in from some header file?

Maybe including it straight from DxeMain.h isn't entirely kosher even
if this code does end up in MdeModulePkg, but surely the declaration
there should also be pulled in from somewhere under UefiCpuPkg/Include?

>  //
>  // Probe, start and stop functions of this driver, called by the DXE core for
>  // specific devices.
> @@ -60,13 +62,9 @@ PlatformPciIoDriverBindingSupported (
>    case PlatformPciIoDeviceEhci:
>    case PlatformPciIoDeviceXhci:
>    case PlatformPciIoDeviceAhci:
> -    //
> -    // Restricted to DMA coherent for now
> -    //
> -    if (PlatformPciIo->DmaType == PlatformPciIoDmaCoherent) {
> -      Status = EFI_SUCCESS;
> -      break;
> -    }
> +    Status = EFI_SUCCESS;
> +    break;
> +
>    default:
>      Status = EFI_UNSUPPORTED;
>    }
> @@ -257,6 +255,11 @@ PlatformPciIoDxeEntryPoint (
>    IN EFI_SYSTEM_TABLE *SystemTable
>    )
>  {
> +  EFI_STATUS      Status;
> +
> +  Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&gCpu);
> +  ASSERT_EFI_ERROR(Status);
> +
>    return EfiLibInstallDriverBindingComponentName2 (
>             ImageHandle,
>             SystemTable,
> diff --git a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.inf b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.inf
> index 2b0baf06732c..670dcc5a9ff2 100644
> --- a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.inf
> +++ b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.inf
> @@ -31,11 +31,13 @@ [Packages]
>  [LibraryClasses]
>    BaseMemoryLib
>    DebugLib
> +  DxeServicesTableLib
>    MemoryAllocationLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
>    UefiLib
>  
>  [Protocols]
> +  gEfiCpuArchProtocolGuid         ## CONSUMES
>    gEfiPciIoProtocolGuid           ## BY_START
>    gPlatformPciIoProtocolGuid      ## TO_START
> -- 
> 2.7.4
>