Re: [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits

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

On Mon, 25 Nov 2019 at 13:46, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Nov 21, 2019 at 09:32:26 +0100, Ard Biesheuvel wrote:
> > Implement support for driving peripherals with limited DMA ranges to
> > NonCoherentDmaLib, by adding a device address limit, and taking it,
> > along with the device offset, into account when allocating or mapping
> > DMA buffers.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  EmbeddedPkg/EmbeddedPkg.dec                                 |   6 +
> >  EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c   | 176 +++++++++++++++++---
> >  EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf |   1 +
> >  3 files changed, 158 insertions(+), 25 deletions(-)
> >
> > diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> > index 8812a6db7c30..69922802f473 100644
> > --- a/EmbeddedPkg/EmbeddedPkg.dec
> > +++ b/EmbeddedPkg/EmbeddedPkg.dec
> > @@ -186,6 +186,12 @@ [PcdsFixedAtBuild.common, PcdsDynamic.common]
> >    #
> >    gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0x0|UINT64|0x0000058
> >
> > +  #
> > +  # Highest address value supported by the device for DMA addressing. Note
> > +  # that this value should be strictly greater than PcdDmaDeviceOffset.
> > +  #
> > +  gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xFFFFFFFFFFFFFFFF|UINT64|0x000005A
> > +
> >    #
> >    # Selection between DT and ACPI as a default
> >    #
> > diff --git a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> > index 78220f6358aa..dd3d111e7eb9 100644
> > --- a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> > +++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> > @@ -40,6 +40,8 @@ typedef struct {
> >  STATIC EFI_CPU_ARCH_PROTOCOL      *mCpu;
> >  STATIC LIST_ENTRY                 UncachedAllocationList;
> >
> > +STATIC PHYSICAL_ADDRESS           mDmaHostAddressLimit;
> > +
> >  STATIC
> >  PHYSICAL_ADDRESS
> >  HostToDeviceAddress (
> > @@ -49,6 +51,102 @@ HostToDeviceAddress (
> >    return (PHYSICAL_ADDRESS)(UINTN)Address + PcdGet64 (PcdDmaDeviceOffset);
> >  }
> >
> > +/**
> > +  Allocates one or more 4KB pages of a certain memory type at a specified
> > +  alignment.
> > +
> > +  Allocates the number of 4KB pages specified by Pages of a certain memory type
> > +  with an alignment specified by Alignment. The allocated buffer is returned.
> > +  If Pages is 0, then NULL is returned. If there is not enough memory at the
> > +  specified alignment remaining to satisfy the request, then NULL is returned.
> > +  If Alignment is not a power of two and Alignment is not zero, then ASSERT().
> > +  If Pages plus EFI_SIZE_TO_PAGES (Alignment) overflows, then ASSERT().
> > +
> > +  @param  MemoryType            The type of memory to allocate.
> > +  @param  Pages                 The number of 4 KB pages to allocate.
> > +  @param  Alignment             The requested alignment of the allocation.
> > +                                Must be a power of two.
> > +                                If Alignment is zero, then byte alignment is
> > +                                used.
> > +
> > +  @return A pointer to the allocated buffer or NULL if allocation fails.
> > +
> > +**/
> > +STATIC
> > +VOID *
> > +InternalAllocateAlignedPages (
> > +  IN EFI_MEMORY_TYPE  MemoryType,
> > +  IN UINTN            Pages,
> > +  IN UINTN            Alignment
> > +  )
> > +{
> > +  EFI_STATUS            Status;
> > +  EFI_PHYSICAL_ADDRESS  Memory;
> > +  UINTN                 AlignedMemory;
> > +  UINTN                 AlignmentMask;
> > +  UINTN                 UnalignedPages;
> > +  UINTN                 RealPages;
> > +
> > +  //
> > +  // Alignment must be a power of two or zero.
> > +  //
> > +  ASSERT ((Alignment & (Alignment - 1)) == 0);
> > +
> > +  if (Pages == 0) {
> > +    return NULL;
> > +  }
> > +  if (Alignment > EFI_PAGE_SIZE) {
> > +    //
> > +    // Calculate the total number of pages since alignment is larger than page
> > +    // size.
> > +    //
> > +    AlignmentMask  = Alignment - 1;
> > +    RealPages      = Pages + EFI_SIZE_TO_PAGES (Alignment);
> > +    //
> > +    // Make sure that Pages plus EFI_SIZE_TO_PAGES (Alignment) does not
> > +    // overflow.
> > +    //
> > +    ASSERT (RealPages > Pages);
> > +
> > +    Memory = mDmaHostAddressLimit;
> > +    Status = gBS->AllocatePages (AllocateMaxAddress, MemoryType, RealPages,
> > +                    &Memory);
> > +    if (EFI_ERROR (Status)) {
> > +      return NULL;
> > +    }
> > +    AlignedMemory  = ((UINTN)Memory + AlignmentMask) & ~AlignmentMask;
> > +    UnalignedPages = EFI_SIZE_TO_PAGES (AlignedMemory - (UINTN)Memory);
> > +    if (UnalignedPages > 0) {
> > +      //
> > +      // Free first unaligned page(s).
> > +      //
> > +      Status = gBS->FreePages (Memory, UnalignedPages);
> > +      ASSERT_EFI_ERROR (Status);
> > +    }
> > +    Memory         = AlignedMemory + EFI_PAGES_TO_SIZE (Pages);
> > +    UnalignedPages = RealPages - Pages - UnalignedPages;
> > +    if (UnalignedPages > 0) {
> > +      //
> > +      // Free last unaligned page(s).
> > +      //
> > +      Status = gBS->FreePages (Memory, UnalignedPages);
> > +      ASSERT_EFI_ERROR (Status);
> > +    }
> > +  } else {
> > +    //
> > +    // Do not over-allocate pages in this case.
> > +    //
> > +    Memory = mDmaHostAddressLimit;
> > +    Status = gBS->AllocatePages (AllocateMaxAddress, MemoryType, Pages,
> > +                    &Memory);
> > +    if (EFI_ERROR (Status)) {
> > +      return NULL;
> > +    }
> > +    AlignedMemory = (UINTN)Memory;
> > +  }
> > +  return (VOID *)AlignedMemory;
> > +}
> > +
> >  /**
> >    Provides the DMA controller-specific addresses needed to access system memory.
> >
> > @@ -111,7 +209,22 @@ DmaMap (
> >      return  EFI_OUT_OF_RESOURCES;
> >    }
> >
> > -  if (Operation != MapOperationBusMasterRead &&
> > +  if (((UINTN)HostAddress + *NumberOfBytes) > mDmaHostAddressLimit) {
> > +
> > +    if (Operation == MapOperationBusMasterCommonBuffer) {
> > +      goto CommonBufferError;
> > +    }
> > +
> > +    AllocSize = ALIGN_VALUE (*NumberOfBytes, mCpu->DmaBufferAlignment);
> > +    Map->BufferAddress = InternalAllocateAlignedPages (EfiBootServicesData,
> > +                           EFI_SIZE_TO_PAGES (AllocSize),
> > +                           mCpu->DmaBufferAlignment);
> > +    if (Map->BufferAddress == NULL) {
> > +      Status = EFI_OUT_OF_RESOURCES;
> > +      goto FreeMapInfo;
> > +    }
> > +    *DeviceAddress = HostToDeviceAddress (Map->BufferAddress);
> > +  } else if (Operation != MapOperationBusMasterRead &&
> >        ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
> >         ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
> >
> > @@ -128,12 +241,7 @@ DmaMap (
> >        // on uncached buffers.
> >        //
> >        if (Operation == MapOperationBusMasterCommonBuffer) {
> > -        DEBUG ((DEBUG_ERROR,
> > -          "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only "
> > -          "supported\non memory regions that were allocated using "
> > -          "DmaAllocateBuffer ()\n", __FUNCTION__));
> > -        Status = EFI_UNSUPPORTED;
> > -        goto FreeMapInfo;
> > +        goto CommonBufferError;
> >        }
> >
> >        //
> > @@ -154,14 +262,6 @@ DmaMap (
> >
> >        Buffer = ALIGN_POINTER (Map->BufferAddress, mCpu->DmaBufferAlignment);
> >        *DeviceAddress = HostToDeviceAddress (Buffer);
> > -
> > -      //
> > -      // Get rid of any dirty cachelines covering the double buffer. This
> > -      // prevents them from being written back unexpectedly, potentially
> > -      // overwriting the data we receive from the device.
> > -      //
> > -      mCpu->FlushDataCache (mCpu, (UINTN)Buffer, *NumberOfBytes,
> > -              EfiCpuFlushTypeWriteBack);
> >      } else {
> >        Map->DoubleBuffer  = FALSE;
> >      }
> > @@ -184,13 +284,13 @@ DmaMap (
> >              (GcdDescriptor.Attributes & (EFI_MEMORY_WB | EFI_MEMORY_WT)) == 0);
> >
> >      DEBUG_CODE_END ();
> > -
> > -    // Flush the Data Cache (should not have any effect if the memory region is
> > -    // uncached)
> > -    mCpu->FlushDataCache (mCpu, (UINTN)HostAddress, *NumberOfBytes,
> > -            EfiCpuFlushTypeWriteBackInvalidate);
> >    }
> >
> > +  // Flush the Data Cache (should not have any effect if the memory region is
> > +  // uncached)
> > +  mCpu->FlushDataCache (mCpu, (UINTN)HostAddress, *NumberOfBytes,
> > +          EfiCpuFlushTypeWriteBack);
> > +
> >    Map->HostAddress   = (UINTN)HostAddress;
> >    Map->NumberOfBytes = *NumberOfBytes;
> >    Map->Operation     = Operation;
> > @@ -199,6 +299,12 @@ DmaMap (
> >
> >    return EFI_SUCCESS;
> >
> > +CommonBufferError:
> > +  DEBUG ((DEBUG_ERROR,
> > +    "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only "
> > +    "supported\non memory regions that were allocated using "
> > +    "DmaAllocateBuffer ()\n", __FUNCTION__));
> > +  Status = EFI_UNSUPPORTED;
> >  FreeMapInfo:
> >    FreePool (Map);
> >
> > @@ -229,6 +335,7 @@ DmaUnmap (
> >    MAP_INFO_INSTANCE *Map;
> >    EFI_STATUS        Status;
> >    VOID              *Buffer;
> > +  UINTN             AllocSize;
> >
> >    if (Mapping == NULL) {
> >      ASSERT (FALSE);
> > @@ -238,7 +345,18 @@ DmaUnmap (
> >    Map = (MAP_INFO_INSTANCE *)Mapping;
> >
> >    Status = EFI_SUCCESS;
> > -  if (Map->DoubleBuffer) {
> > +  if (((UINTN)Map->HostAddress + Map->NumberOfBytes) > mDmaHostAddressLimit) {
> > +
> > +    mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, Map->NumberOfBytes,
> > +            EfiCpuFlushTypeInvalidate);
> > +
> > +    CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress,
> > +      Map->NumberOfBytes);
> > +
> > +    AllocSize = ALIGN_VALUE (Map->NumberOfBytes, mCpu->DmaBufferAlignment);
> > +    FreePages (Map->BufferAddress, EFI_SIZE_TO_PAGES (AllocSize));
> > +  } else if (Map->DoubleBuffer) {
> > +
> >      ASSERT (Map->Operation == MapOperationBusMasterWrite);
> >
> >      if (Map->Operation != MapOperationBusMasterWrite) {
> > @@ -335,10 +453,9 @@ DmaAllocateAlignedBuffer (
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > -  if (MemoryType == EfiBootServicesData) {
> > -    Allocation = AllocateAlignedPages (Pages, Alignment);
> > -  } else if (MemoryType == EfiRuntimeServicesData) {
> > -    Allocation = AllocateAlignedRuntimePages (Pages, Alignment);
> > +  if (MemoryType == EfiBootServicesData ||
> > +      MemoryType == EfiRuntimeServicesData) {
> > +    Allocation = InternalAllocateAlignedPages (MemoryType, Pages, Alignment);
> >    } else {
> >      return EFI_INVALID_PARAMETER;
> >    }
> > @@ -479,6 +596,15 @@ NonCoherentDmaLibConstructor (
> >  {
> >    InitializeListHead (&UncachedAllocationList);
> >
> > +  //
> > +  // Ensure that the combination of DMA addressing offset and limit produces
> > +  // a sane value.
> > +  //
> > +  ASSERT (PcdGet64 (PcdDmaDeviceLimit) > PcdGet64 (PcdDmaDeviceOffset));
>
> Is this worth turning into a hard conditional and error return rather
> than an assert? The following statement will end up wrapping downwards
> if this condition is not true.
>

Constructor return values are only used in ASSERT_EFI_ERROR() calls in
the ProcessLibraryConstructorList() routine that gets generated by the
build tools (in AutoGen.c), so returning an error here would
essentially be dead code, given that we would only make it to this
point on builds that are known to ignore it.


> > +
> > +  mDmaHostAddressLimit = PcdGet64 (PcdDmaDeviceLimit) -
> > +                         PcdGet64 (PcdDmaDeviceOffset);
> > +
>
> /
>     Leif
>
> >    // Get the Cpu protocol for later use
> >    return gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&mCpu);
> >  }
> > diff --git a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
> > index 2db751afee91..1a21cfe4ff23 100644
> > --- a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
> > +++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
> > @@ -38,6 +38,7 @@ [Protocols]
> >
> >  [Pcd]
> >    gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset
> > +  gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit
> >
> >  [Depex]
> >    gEfiCpuArchProtocolGuid
> > --
> > 2.17.1
> >