From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by mx.groups.io with SMTP id smtpd.web11.3477.1574685970407764406 for ; Mon, 25 Nov 2019 04:46:10 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=eMA3+cLu; spf=pass (domain: linaro.org, ip: 209.85.221.66, mailfrom: leif.lindholm@linaro.org) Received: by mail-wr1-f66.google.com with SMTP id i12so17821955wrn.11 for ; Mon, 25 Nov 2019 04:46:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=u2kP5ZxL1vReA0Z9G7EomYyKG82+LzSWW3zm7SzNUhs=; b=eMA3+cLuyB+QLYx9K5LcMoIu6C+0GKJ5dEzURaltv/NkqrEmrqkodc099pMEEuVqY2 IMBzgpeh3sywz17qy7eXcdOqkO8EW/eyZeEzWIppT9ziB0q2IztmFOpZtzteVr55s4EV LSqI7Ljrk4DbBMlxUiaWpEvNXS+zhYmEkmb7SXIw0kfLvcdPTDPl9TxRO3IqCa0Tf7Wi RxBaigPQAIFDT9IVoq2xvXDDbzu4b/eoqOQK0AtWqMuDQ47jmZ+4nDA3Qzi/sHP3GjoH QxQ6op2iXXRqdOq9X+JY2FJhVi4PbzY1fZ59PL567TptPzOVL8iUsYZw7y7uxMg6ccmw 9DuA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=u2kP5ZxL1vReA0Z9G7EomYyKG82+LzSWW3zm7SzNUhs=; b=HhkZCwUtXOyz5a8gjl1bpIDAe+I6j3T8UMVJVIKASfxwJnD6x/q51OV5pz0vQgR5gT nr5NhrhuosWJEH/Z7seOI0g7+RQ9UNayWiLVxVEYDh710j6tkePtZp1yszFPrnn3mKd0 OGbMCVW9fRAw0GmqFRifdartToNqYF80c8Ef7Yaop6T5MZuci0xVW8EBPI5xufCHdbA0 4mNYajdyWH0DuGXHCABUxr7Xz5iphCHyCrurln/qW0THuhNPxlHobgYp86JBqp9rs5Q6 615B8r2gqFBobNBwR7Cs2icFHg3qKSAiFxHYbisvrpQt10RDbTtzg8uyCMjsA6W7V0K9 Xc4w== X-Gm-Message-State: APjAAAUwBGB0GZh9lJwolzMA9iuOg4xK8nIudkM4t1By2xeIDIjaGZl8 WGVR0abEOUFtJmPbNQOn3EFXSw== X-Google-Smtp-Source: APXvYqyNZqqZ0nEADntitrBgwa/J7HePZKrYgb4R0UQkige8bSa0M6EnrxhZiCkX8zZ7fkYoXTOP4w== X-Received: by 2002:a5d:4481:: with SMTP id j1mr28763134wrq.99.1574685968567; Mon, 25 Nov 2019 04:46:08 -0800 (PST) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id k14sm10552858wrw.46.2019.11.25.04.46.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 25 Nov 2019 04:46:07 -0800 (PST) Date: Mon, 25 Nov 2019 12:46:06 +0000 From: "Leif Lindholm" To: Ard Biesheuvel Cc: devel@edk2.groups.io Subject: Re: [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits Message-ID: <20191125124606.GO7359@bivouac.eciton.net> References: <20191121083227.2850-1-ard.biesheuvel@linaro.org> <20191121083227.2850-2-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20191121083227.2850-2-ard.biesheuvel@linaro.org> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. > + > + 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 >