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

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

On Wed, Nov 02, 2016 at 01:43:42PM +0000, Ard Biesheuvel wrote:
> 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

Ah, that makes a lot more sense then, thanks.

I also confusingly added the comment here, rather than 15 lines
earlier where I was intending to.

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

Right, as mCpu that makes complete sense.

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