Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751953AbdF0OxG (ORCPT ); Tue, 27 Jun 2017 10:53:06 -0400 Received: from foss.arm.com ([217.140.101.70]:57940 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751541AbdF0Ow6 (ORCPT ); Tue, 27 Jun 2017 10:52:58 -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> <0a6dceb1-ffe6-b213-eb98-fd897b62febd@arm.com> From: Robin Murphy Message-ID: Date: Tue, 27 Jun 2017 15:52:55 +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: 4195 Lines: 111 On 27/06/17 13:44, Tomasz Figa wrote: > On Tue, Jun 27, 2017 at 9:26 PM, Robin Murphy wrote: >> 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. > > Right. Especially with various user pointer sources it doesn't sound > very realistic to enforce that all memory comes from __GFP_DMA(32) > zone. > > On the other hand, support for user pointer is optional in subsystems > such as V4L2 and drivers for such disabled hardware could opt out. > Then at least dma_alloc can be made working, giving some level of > usability, IMHO higher than no IOMMU and contiguous memory allocated > from CMA. > >> >>> 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. > > Just to make sure, should I resent with the commit updated? If so, > what would be your preference on the wording? Yes please; how about this? /* * Unless we have some addressing limitation implied by GFP_DMA flags, * assume the IOMMU can map all of RAM and we can allocate anywhere. */ if (!(gfp & (__GFP_DMA | __GFP_DMA32))) ... Feel free to reword it if you like, but those are the general lines I was thinking along. Robin. > > Best regards, > Tomasz > >> >> 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; >>>>> >>>> >>