Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752875AbdF0M0X (ORCPT ); Tue, 27 Jun 2017 08:26:23 -0400 Received: from foss.arm.com ([217.140.101.70]:56086 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751453AbdF0M0Q (ORCPT ); Tue, 27 Jun 2017 08:26:16 -0400 Subject: Re: [PATCH 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags To: Tomasz Figa Cc: "open list:IOMMU DRIVERS" , "linux-kernel@vger.kernel.org" , Joerg Roedel References: <20170627072812.15316-1-tfiga@chromium.org> From: Robin Murphy Message-ID: <0a6dceb1-ffe6-b213-eb98-fd897b62febd@arm.com> Date: Tue, 27 Jun 2017 13:26:12 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2991 Lines: 75 On 27/06/17 12:17, Tomasz Figa wrote: > On Tue, Jun 27, 2017 at 8:01 PM, Robin Murphy wrote: >> On 27/06/17 08:28, Tomasz Figa wrote: >>> Current implementation of __iommu_dma_alloc_pages() keeps adding >>> __GFP_HIGHMEM to GFP flags regardless of whether other zone flags are >>> already included in the incoming flags. If __GFP_DMA or __GFP_DMA32 is >>> set at the same time as __GFP_HIGHMEM, the allocation fails due to >>> invalid zone flag combination. >>> >>> Fix this by checking for __GFP_DMA and __GFP_DMA32 in incoming GFP flags >>> and adding __GFP_HIGHMEM only if they are not present. >> >> Wouldn't it make more sense to strip off the ZONE_DMA* related flags, >> since the whole point here is that we don't care where the pages come from? > > I guess for my use case it wouldn't break things, as I strip them in > my DMA mapping implementation right now (+/- one minor detail below). > > However I recall existing IOMMUs that could only use pages from within > the 32-bit address space (e.g. Tegra X1). In general, iommu-dma can't really support IOMMUs which can't reach the entirety of kernel memory - there's no easy way to determine what such a limit is if it exists, nor necessarily enforce it, and either way the streaming API callbacks are pretty much dead in the water. > Also the IOMMU I'm working > on is a part of a PCI device and it might actually prefer 32-bit > addressable memory as well (to avoid DAC addressing; I still need to > evaluate this). With this said, maybe it could actually make sense to > leave the choice to the DMA mapping implementation? I think you're right - we're just not in a position to make any decision at this level, so we probably do have to do this for robustness. I would like to fix the longstanding dodgy comment, though, to clarify that "IOMMU can map any pages" is only an assumption, and particularly one which is invalidated by the presence of GFP_DMA flags. Robin. > > Best regards, > Tomasz > >> >> Robin. >> >>> Signed-off-by: Tomasz Figa >>> --- >>> drivers/iommu/dma-iommu.c | 10 ++++++++-- >>> 1 file changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c >>> index 9d1cebe7f6cb..29965a092a69 100644 >>> --- a/drivers/iommu/dma-iommu.c >>> +++ b/drivers/iommu/dma-iommu.c >>> @@ -445,8 +445,14 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count, >>> if (!pages) >>> return NULL; >>> >>> - /* IOMMU can map any pages, so himem can also be used here */ >>> - gfp |= __GFP_NOWARN | __GFP_HIGHMEM; >>> + /* >>> + * IOMMU can map any pages, so himem can also be used here, >>> + * unless another DMA zone is explicitly requested. >>> + */ >>> + if (!(gfp & (__GFP_DMA | __GFP_DMA32))) >>> + gfp |= __GFP_HIGHMEM; >>> + >>> + gfp |= __GFP_NOWARN; >>> >>> while (count) { >>> struct page *page = NULL; >>> >>