[edk2-devel] [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
Ard Biesheuvel via Groups.Io
ard.biesheuvel=linaro.org at groups.io
Tue Nov 26 12:02:19 UTC 2019
On Tue, 26 Nov 2019 at 12:59, Philippe Mathieu-Daudé <philmd at redhat.com> wrote:
>
> On 11/26/19 11:44 AM, Philippe Mathieu-Daudé wrote:
> > On 11/26/19 12:12 AM, Ard Biesheuvel via Groups.Io 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 | 165
> >> ++++++++++++++++++--
> >> EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf | 1 +
> >> 3 files changed, 160 insertions(+), 12 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..115345765435 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);
>
> The previous line made me think twice (due to the cast), but I can't
> find a simpler way to write the same code.
>
> >> + 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,30 @@ 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;
> >> + }
> >> +
> >> + if (Map->Operation == MapOperationBusMasterRead) {
> >> + CopyMem (Map->BufferAddress, (VOID *)(UINTN)Map->HostAddress,
> >> + *NumberOfBytes);
> >> + }
> >> + mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, AllocSize,
> >> + EfiCpuFlushTypeWriteBack);
> >> +
> >> + *DeviceAddress = HostToDeviceAddress (Map->BufferAddress);
> >> + } else if (Operation != MapOperationBusMasterRead &&
> >> ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
> >> ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
> >> @@ -128,12 +249,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;
> >> }
> >> //
> >> @@ -199,6 +315,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 +351,7 @@ DmaUnmap (
> >> MAP_INFO_INSTANCE *Map;
> >> EFI_STATUS Status;
> >> VOID *Buffer;
> >> + UINTN AllocSize;
> >> if (Mapping == NULL) {
> >> ASSERT (FALSE);
> >> @@ -238,7 +361,17 @@ DmaUnmap (
> >> Map = (MAP_INFO_INSTANCE *)Mapping;
> >> Status = EFI_SUCCESS;
> >> - if (Map->DoubleBuffer) {
> >> + if (((UINTN)Map->HostAddress + Map->NumberOfBytes) >
> >> mDmaHostAddressLimit) {
> >> + AllocSize = ALIGN_VALUE (Map->NumberOfBytes,
> >> mCpu->DmaBufferAlignment);
> >> + if (Map->Operation == MapOperationBusMasterWrite) {
> >> + mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, AllocSize,
> >> + EfiCpuFlushTypeInvalidate);
> >> + CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress,
> >> + Map->NumberOfBytes);
> >> + }
> >> + FreePages (Map->BufferAddress, EFI_SIZE_TO_PAGES (AllocSize));
> >> + } else if (Map->DoubleBuffer) {
> >> +
> >> ASSERT (Map->Operation == MapOperationBusMasterWrite);
> >> if (Map->Operation != MapOperationBusMasterWrite) {
> >> @@ -335,10 +468,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 +611,15 @@ NonCoherentDmaLibConstructor (
> >> {
> >> InitializeListHead (&UncachedAllocationList);
> >> + //
> >> + // Ensure that the combination of DMA addressing offset and limit
> >> produces
> >> + // a sane value.
> >> + //
> >> + ASSERT (PcdGet64 (PcdDmaDeviceLimit) > PcdGet64 (PcdDmaDeviceOffset));
> >> +
> >> + mDmaHostAddressLimit = PcdGet64 (PcdDmaDeviceLimit) -
> >> + PcdGet64 (PcdDmaDeviceOffset);
> >> +
> >> // 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
> >>
> >
> > For the 82% of the patch I understood:
> > Reviewed-by: Philippe Mathieu-Daude <philmd at redhat.com>
>
> Leif explained me that "I've reviewed it, but don't fully understand it,
> but I think it's valuable" translate into a Acked-by on EDK2, so please
> replace the previous Reviewed-by by:
> Acked-by: Philippe Mathieu-Daude <philmd at redhat.com>
>
Thanks!
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51278): https://edk2.groups.io/g/devel/message/51278
Mute This Topic: https://groups.io/mt/61950463/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