From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) by mx.groups.io with SMTP id smtpd.web09.3454.1574686344303467160 for ; Mon, 25 Nov 2019 04:52:24 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=BLPxM8Ko; spf=pass (domain: linaro.org, ip: 209.85.128.68, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wm1-f68.google.com with SMTP id z19so15838309wmk.3 for ; Mon, 25 Nov 2019 04:52:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=s3TIuMiJw13L0cNyntH5X9HBSWF6k5P9jSS2O2y1eCM=; b=BLPxM8KojMW9QKoSUKgZIuRnfkxVC91Q3DjLrReXZkbvD+1AIfpsa+RZvbvBRdLJiK XI/EPBwVhxL3z9J4aj4WmJNW+F5iad+/m6iwXzwhDWVqMo4BE0Xtb7VQ2pq/Qy1ywHbf 0m4gRgIZXVKqYo3OpCIchHKUcaAnQFJ4HlwSXJauaCQAmEZPG64fxJ0nXcbHaTodJK7q U7sMYOBiE0JIkgnkxWAoKRNPohcxoyURXoY11VAxC0LVNqGu175CJ4bAX8QSuXoAnh6K OXBX5yXXQe49nLoSRhjqLClkz/sA9ihXsu6/FNCVgp5t6y+Kp7v9pMfkKbwnLImcyIO/ qGbA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=s3TIuMiJw13L0cNyntH5X9HBSWF6k5P9jSS2O2y1eCM=; b=QWsLYqTpCGJpeMQhuj1HH1n0N0/sYfOvW+c56KX8uPOk4mW1LcK104pz5GTl5aoiGh 8jOt1JolZ0C4INzlTITCHrnlBjNYzdUKQUNlXPB/i5yPTDHp90MDjq5tRRPYbP4MHHDr JqyY2cnyy/w61rNEVnK5jO77385I7eCwVBnaxyczwjemsXHukn7MNNhKOqLt0gpyJjJz k3DqoHcXBShpJfedgByDaqo6zPLhgqkW3jeM1kvJw4m4JFZezzesMA7mEjGvxYmT7LxA D78djqz7Tzie26xeNj5YJl1Xfsbo7KK2S3lLkUGCT3pA8nlzMc0xlDEK6U42iWHC3KVN 3VCA== X-Gm-Message-State: APjAAAXw7NaqQMtMIFd0+dIm3PuPt78BxaaSbWzbvkxFTxmKi64ayOyQ lYYIAtHS+HaP8+ehZECeJJ1TyLB/0mjRgsBYlp+BvQ== X-Google-Smtp-Source: APXvYqwK9MbDdT5Hn8sXOon0zuYPxRH5OUnBYq4NcdLGNjq7lHdi3tq42w1bitOVn6uDk/vuJW4Gu9qmsJFWwFu4nK8= X-Received: by 2002:a1c:3d08:: with SMTP id k8mr27560872wma.119.1574686342504; Mon, 25 Nov 2019 04:52:22 -0800 (PST) MIME-Version: 1.0 References: <20191121083227.2850-1-ard.biesheuvel@linaro.org> <20191121083227.2850-2-ard.biesheuvel@linaro.org> <20191125124606.GO7359@bivouac.eciton.net> In-Reply-To: <20191125124606.GO7359@bivouac.eciton.net> From: "Ard Biesheuvel" Date: Mon, 25 Nov 2019 13:52:24 +0100 Message-ID: Subject: Re: [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits To: Leif Lindholm Cc: edk2-devel-groups-io Content-Type: text/plain; charset="UTF-8" On Mon, 25 Nov 2019 at 13:46, Leif Lindholm 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 > > --- > > 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 > >