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 <[email protected]>
---
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
Memory allocation routines are expected to report allocation errors to
kernel log. However, current implementation of __iommu_dma_alloc_pages()
adds __GFP_NOWARN for all calls to alloc_pages(), which completely
disables any logging.
Fix it by adding __GFP_NOWARN only to high order allocation attempts,
which are not critical.
Signed-off-by: Tomasz Figa <[email protected]>
---
drivers/iommu/dma-iommu.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 29965a092a69..8507987eed90 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -433,6 +433,7 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
{
struct page **pages;
unsigned int i = 0, array_size = count * sizeof(*pages);
+ const gfp_t high_order_gfp = __GFP_NOWARN | __GFP_NORETRY;
order_mask &= (2U << MAX_ORDER) - 1;
if (!order_mask)
@@ -452,8 +453,6 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
if (!(gfp & (__GFP_DMA | __GFP_DMA32)))
gfp |= __GFP_HIGHMEM;
- gfp |= __GFP_NOWARN;
-
while (count) {
struct page *page = NULL;
unsigned int order_size;
@@ -469,7 +468,7 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
order_size = 1U << order;
page = alloc_pages((order_mask - order_size) ?
- gfp | __GFP_NORETRY : gfp, order);
+ gfp | high_order_gfp : gfp, order);
if (!page)
continue;
if (!order)
--
2.13.1.611.g7e3b11ae1-goog
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 <[email protected]>
> ---
> 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;
>
On 27/06/17 08:28, Tomasz Figa wrote:
> Memory allocation routines are expected to report allocation errors to
> kernel log. However, current implementation of __iommu_dma_alloc_pages()
> adds __GFP_NOWARN for all calls to alloc_pages(), which completely
> disables any logging.
>
> Fix it by adding __GFP_NOWARN only to high order allocation attempts,
> which are not critical.
Makes sense to me.
Reviewed-by: Robin Murphy <[email protected]>
> Signed-off-by: Tomasz Figa <[email protected]>
> ---
> drivers/iommu/dma-iommu.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 29965a092a69..8507987eed90 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -433,6 +433,7 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
> {
> struct page **pages;
> unsigned int i = 0, array_size = count * sizeof(*pages);
> + const gfp_t high_order_gfp = __GFP_NOWARN | __GFP_NORETRY;
>
> order_mask &= (2U << MAX_ORDER) - 1;
> if (!order_mask)
> @@ -452,8 +453,6 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
> if (!(gfp & (__GFP_DMA | __GFP_DMA32)))
> gfp |= __GFP_HIGHMEM;
>
> - gfp |= __GFP_NOWARN;
> -
> while (count) {
> struct page *page = NULL;
> unsigned int order_size;
> @@ -469,7 +468,7 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
>
> order_size = 1U << order;
> page = alloc_pages((order_mask - order_size) ?
> - gfp | __GFP_NORETRY : gfp, order);
> + gfp | high_order_gfp : gfp, order);
> if (!page)
> continue;
> if (!order)
>
On Tue, Jun 27, 2017 at 8:01 PM, Robin Murphy <[email protected]> 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 <[email protected]>
>> ---
>> 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;
>>
>
On 27/06/17 12:17, Tomasz Figa wrote:
> On Tue, Jun 27, 2017 at 8:01 PM, Robin Murphy <[email protected]> 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 <[email protected]>
>>> ---
>>> 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;
>>>
>>
On Tue, Jun 27, 2017 at 9:26 PM, Robin Murphy <[email protected]> wrote:
> On 27/06/17 12:17, Tomasz Figa wrote:
>> On Tue, Jun 27, 2017 at 8:01 PM, Robin Murphy <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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;
>>>>
>>>
>
On 27/06/17 13:44, Tomasz Figa wrote:
> On Tue, Jun 27, 2017 at 9:26 PM, Robin Murphy <[email protected]> wrote:
>> On 27/06/17 12:17, Tomasz Figa wrote:
>>> On Tue, Jun 27, 2017 at 8:01 PM, Robin Murphy <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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;
>>>>>
>>>>
>>