From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by mx.groups.io with SMTP id smtpd.web12.10417.1574769739339030198 for ; Tue, 26 Nov 2019 04:02:19 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=whVJEuNH; spf=pass (domain: linaro.org, ip: 209.85.128.66, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wm1-f66.google.com with SMTP id t26so2991683wmi.4 for ; Tue, 26 Nov 2019 04:02:19 -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:content-transfer-encoding; bh=bW7LOeAqMstRCdeVjA+LJkqrg+SEKtpBJ0ToPscGCms=; b=whVJEuNHTqR6la/QZiZRjr3TlxOz2J+bAm49+AAR9UTXz/dliEiHLHro7ADQZOiois rzzUyKM4DzONB4eCxM5UnaISocAq0kTAyzUX7WLInvOBBZh569itPXv8VUwgizgRiUSG On3TICESpa9zqvTbkCqVyNfxfVnSmizu9DG7kWu8N9A7ZgG+SXCaCaTctLoo/kwAXnSj xORGNlLU1qeFDa/TdtpuK/Sl99tEbXeQgdBjJythN3JKZwRvRXlJculxRQgl11rhTUfr AC76YV2w2HN1FAe6/ca2D2lQEjXgMWlbIxfaevlW/B5jGgxd8+HgdvOTOIskgTs3rRa3 brqg== 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:content-transfer-encoding; bh=bW7LOeAqMstRCdeVjA+LJkqrg+SEKtpBJ0ToPscGCms=; b=PG2n4Hn8wdY0M/8pPFjIVKzkJ3MezawjjZg9CxuJiWN0V+iILjLX1KdiEghb6iAEtX /U8Lnkc4xrJcXdJS8hE4z3jvysMCYcYCXvpUejB11fW/hEIp+Ivkmmc8MfKKxCqUee5C h8nEViqm8IEZYKedM7yYZTo0TYI0LSlDa+iuz0m4w5gHnPZR3bKEAPN2gtON99HceYb4 7pN0VHVw4qxepL0PxfusghQ9pS8tbaJwFMmKIUD00NpLifQpbzjfPiNu8Ab1pfkYJH9c G3ZVL92mxXHUWVkMWaM4n/CCjZUVTXfjVhN6zT65UxHMN0sE7vKiJBXz6uEb08aZ1+rg EoKQ== X-Gm-Message-State: APjAAAU9lyub/QVLRvwBaW0XJk/UKa5pMZn7w5hAROB1+3HjjU3TLKZq MU3GJUgZnkGHH4w9MkMhu61IHYzSI7VHjlMCOmrasw== X-Google-Smtp-Source: APXvYqyCklGlEdMoVcXCU6+AS42ftWsHQ58hFP9cqD+vmOLBHusLlc2Ltr77h7d8cAmFyEynmcaoI8Qmt/CROpubKDI= X-Received: by 2002:a1c:3d08:: with SMTP id k8mr3754011wma.119.1574769737604; Tue, 26 Nov 2019 04:02:17 -0800 (PST) MIME-Version: 1.0 References: <20191125231242.12193-1-ard.biesheuvel@linaro.org> <20191125231242.12193-2-ard.biesheuvel@linaro.org> <08876497-a2ce-4b55-6f50-f332ffd45ddd@redhat.com> In-Reply-To: <08876497-a2ce-4b55-6f50-f332ffd45ddd@redhat.com> From: "Ard Biesheuvel" Date: Tue, 26 Nov 2019 13:02:19 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits To: =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= Cc: edk2-devel-groups-io , Leif Lindholm Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 26 Nov 2019 at 12:59, Philippe Mathieu-Daud=C3=A9 wrote: > > On 11/26/19 11:44 AM, Philippe Mathieu-Daud=C3=A9 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 > >> --- > >> 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 PcdDmaDeviceOffse= t. > >> + # > >> + > >> gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xFFFFFFFFFFFFFFFF|UINT64|0x= 000005A > >> > >> + > >> # > >> # 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 fai= ls. > >> + > >> +**/ > >> +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)) =3D=3D 0); > >> + > >> + if (Pages =3D=3D 0) { > >> + return NULL; > >> + } > >> + if (Alignment > EFI_PAGE_SIZE) { > >> + // > >> + // Calculate the total number of pages since alignment is larger > >> than page > >> + // size. > >> + // > >> + AlignmentMask =3D Alignment - 1; > >> + RealPages =3D Pages + EFI_SIZE_TO_PAGES (Alignment); > >> + // > >> + // Make sure that Pages plus EFI_SIZE_TO_PAGES (Alignment) does n= ot > >> + // overflow. > >> + // > >> + ASSERT (RealPages > Pages); > >> + > >> + Memory =3D mDmaHostAddressLimit; > >> + Status =3D gBS->AllocatePages (AllocateMaxAddress, MemoryType, > >> RealPages, > >> + &Memory); > >> + if (EFI_ERROR (Status)) { > >> + return NULL; > >> + } > >> + AlignedMemory =3D ((UINTN)Memory + AlignmentMask) & ~AlignmentMa= sk; > >> + UnalignedPages =3D EFI_SIZE_TO_PAGES (AlignedMemory - (UINTN)Memo= ry); > > 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 =3D gBS->FreePages (Memory, UnalignedPages); > >> + ASSERT_EFI_ERROR (Status); > >> + } > >> + Memory =3D AlignedMemory + EFI_PAGES_TO_SIZE (Pages); > >> + UnalignedPages =3D RealPages - Pages - UnalignedPages; > >> + if (UnalignedPages > 0) { > >> + // > >> + // Free last unaligned page(s). > >> + // > >> + Status =3D gBS->FreePages (Memory, UnalignedPages); > >> + ASSERT_EFI_ERROR (Status); > >> + } > >> + } else { > >> + // > >> + // Do not over-allocate pages in this case. > >> + // > >> + Memory =3D mDmaHostAddressLimit; > >> + Status =3D gBS->AllocatePages (AllocateMaxAddress, MemoryType, Pa= ges, > >> + &Memory); > >> + if (EFI_ERROR (Status)) { > >> + return NULL; > >> + } > >> + AlignedMemory =3D (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 !=3D MapOperationBusMasterRead && > >> + if (((UINTN)HostAddress + *NumberOfBytes) > mDmaHostAddressLimit) { > >> + > >> + if (Operation =3D=3D MapOperationBusMasterCommonBuffer) { > >> + goto CommonBufferError; > >> + } > >> + > >> + AllocSize =3D ALIGN_VALUE (*NumberOfBytes, mCpu->DmaBufferAlignme= nt); > >> + Map->BufferAddress =3D InternalAllocateAlignedPages > >> (EfiBootServicesData, > >> + EFI_SIZE_TO_PAGES (AllocSize), > >> + mCpu->DmaBufferAlignment); > >> + if (Map->BufferAddress =3D=3D NULL) { > >> + Status =3D EFI_OUT_OF_RESOURCES; > >> + goto FreeMapInfo; > >> + } > >> + > >> + if (Map->Operation =3D=3D MapOperationBusMasterRead) { > >> + CopyMem (Map->BufferAddress, (VOID *)(UINTN)Map->HostAddress, > >> + *NumberOfBytes); > >> + } > >> + mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, AllocSize, > >> + EfiCpuFlushTypeWriteBack); > >> + > >> + *DeviceAddress =3D HostToDeviceAddress (Map->BufferAddress); > >> + } else if (Operation !=3D MapOperationBusMasterRead && > >> ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) !=3D 0= ) || > >> ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) !=3D 0))) = { > >> @@ -128,12 +249,7 @@ DmaMap ( > >> // on uncached buffers. > >> // > >> if (Operation =3D=3D MapOperationBusMasterCommonBuffer) { > >> - DEBUG ((DEBUG_ERROR, > >> - "%a: Operation type 'MapOperationBusMasterCommonBuffer' is > >> only " > >> - "supported\non memory regions that were allocated using " > >> - "DmaAllocateBuffer ()\n", __FUNCTION__)); > >> - Status =3D 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 =3D EFI_UNSUPPORTED; > >> FreeMapInfo: > >> FreePool (Map); > >> @@ -229,6 +351,7 @@ DmaUnmap ( > >> MAP_INFO_INSTANCE *Map; > >> EFI_STATUS Status; > >> VOID *Buffer; > >> + UINTN AllocSize; > >> if (Mapping =3D=3D NULL) { > >> ASSERT (FALSE); > >> @@ -238,7 +361,17 @@ DmaUnmap ( > >> Map =3D (MAP_INFO_INSTANCE *)Mapping; > >> Status =3D EFI_SUCCESS; > >> - if (Map->DoubleBuffer) { > >> + if (((UINTN)Map->HostAddress + Map->NumberOfBytes) > > >> mDmaHostAddressLimit) { > >> + AllocSize =3D ALIGN_VALUE (Map->NumberOfBytes, > >> mCpu->DmaBufferAlignment); > >> + if (Map->Operation =3D=3D MapOperationBusMasterWrite) { > >> + mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, AllocSiz= e, > >> + 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 =3D=3D MapOperationBusMasterWrite); > >> if (Map->Operation !=3D MapOperationBusMasterWrite) { > >> @@ -335,10 +468,9 @@ DmaAllocateAlignedBuffer ( > >> return EFI_INVALID_PARAMETER; > >> } > >> - if (MemoryType =3D=3D EfiBootServicesData) { > >> - Allocation =3D AllocateAlignedPages (Pages, Alignment); > >> - } else if (MemoryType =3D=3D EfiRuntimeServicesData) { > >> - Allocation =3D AllocateAlignedRuntimePages (Pages, Alignment); > >> + if (MemoryType =3D=3D EfiBootServicesData || > >> + MemoryType =3D=3D EfiRuntimeServicesData) { > >> + Allocation =3D 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 =3D 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 > > 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 > Thanks!