On 27/06/17 13:44, Tomasz Figa wrote: > On Tue, Jun 27, 2017 at 9:26 PM, Robin Murphy <robin.mur...@arm.com> wrote: >> On 27/06/17 12:17, Tomasz Figa wrote: >>> On Tue, Jun 27, 2017 at 8:01 PM, Robin Murphy <robin.mur...@arm.com> 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 <tf...@chromium.org> >>>>> --- >>>>> 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; >>>>> >>>> >>