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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 1 November 2016 at 22:43, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 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.
>

Yes, I was a bit sloppy with this one. The subject line needs to be
duplicated into the long log.

> 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?
>

No. But it should be called mCpu not gCpu

> 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?
>

It is simply a reference we hold to an architectural protocol, so it
is no different from any other driver that finds a protocol using
LocateProtocol and stashes the pointer.

>>  //
>>  // 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
>>