[edk2-devel] [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
Philippe Mathieu-Daudé via Groups.Io
philmd=redhat.com at groups.io
Tue Nov 26 11:59:12 UTC 2019
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,
Phil.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51277): https://edk2.groups.io/g/devel/message/51277
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