Re: [PATCH 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags
On 27/06/17 13:44, Tomasz Figa wrote: > On Tue, Jun 27, 2017 at 9:26 PM, Robin Murphywrote: >> 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; > >>
Re: [PATCH 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags
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; > >>
Re: [PATCH 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags
On Tue, Jun 27, 2017 at 9:26 PM, Robin Murphywrote: > 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? 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; >>> >
Re: [PATCH 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags
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? 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; >>> >
Re: [PATCH 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags
On 27/06/17 12:17, Tomasz Figa wrote: > On Tue, Jun 27, 2017 at 8:01 PM, Robin Murphywrote: >> 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; >>> >>
Re: [PATCH 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags
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; >>> >>
Re: [PATCH 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags
On Tue, Jun 27, 2017 at 8:01 PM, Robin Murphywrote: > 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). 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? 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; >> >
Re: [PATCH 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags
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). 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? 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; >> >
Re: [PATCH 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags
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? 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; >
Re: [PATCH 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags
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? 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; >
[PATCH 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags
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. 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; -- 2.13.1.611.g7e3b11ae1-goog
[PATCH 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags
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. 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; -- 2.13.1.611.g7e3b11ae1-goog