[edk2-devel] [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
Ard Biesheuvel via Groups.Io
ard.biesheuvel=linaro.org at groups.io
Mon Nov 25 12:52:24 UTC 2019
On Mon, 25 Nov 2019 at 13:46, Leif Lindholm <leif.lindholm at 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 at 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
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51236): https://edk2.groups.io/g/devel/message/51236
Mute This Topic: https://groups.io/mt/61068915/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list