iommu/io-pgtable-arm-v7s: About pagetable 33bit PA

iommu/io-pgtable-arm-v7s: About pagetable 33bit PA

[Yong Wu]

Hi Robin,

After the commit ad67f5a6545f ("arm64: replace ZONE_DMA with
ZONE_DMA32"), we don't have ZONE_DMA in aarch64, but
__arm_v7s_alloc_table[1] use the GFP_DMA to expect the physical address
of pagetable should be 32bit.

Right now we meet a issue that the lvl1 pagetable PA is 0x1_3ab6_0000 in
the 4GB broad. then the IOMMU initialize failed.(This issue can be fixed
if we revert ad67f5a6545f.)

I planed to add GFP_DMA32 for pagetable allocation. the level1 was
ok but level2 was failed. It looks that slab don't like GFP_DMA32[2].
this is the warning log:
=====
Unexpected gfp: 0x4 (GFP_DMA32). Fixing up to gfp: 0x488020 (GFP_ATOMIC|
__GFP_ZERO). Fix your code!
=====
I don't know why slab don't allow GFP_DMA32, the gfp flags seems only
be bypassed to alloc_page. Is it possible that let slab allow GFP_DMA32
for aarch64?

We notice that this has been discussed[3]. but if we use alloc_page for
the level2 pagetable, It may waste lots of memory.

Any suggestion is appreciated.


Reported-by: Nicolas Boichat <drinkcat-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

[1]
https://elixir.bootlin.com/linux/v4.20-rc1/source/drivers/iommu/io-pgtable-arm-v7s.c#L200
[2] https://elixir.bootlin.com/linux/v4.20-rc1/source/mm/internal.h#L37
[3] https://patchwork.kernel.org/patch/10474289/

Re: iommu/io-pgtable-arm-v7s: About pagetable 33bit PA

[Robin Murphy]

On 08/11/2018 07:31, Yong Wu wrote:
> Hi Robin,
> 
> After the commit ad67f5a6545f ("arm64: replace ZONE_DMA with
> ZONE_DMA32"), we don't have ZONE_DMA in aarch64, but
> __arm_v7s_alloc_table[1] use the GFP_DMA to expect the physical address
> of pagetable should be 32bit.
> 
> Right now we meet a issue that the lvl1 pagetable PA is 0x1_3ab6_0000 in
> the 4GB broad. then the IOMMU initialize failed.(This issue can be fixed
> if we revert ad67f5a6545f.)

But that shouldn't actually be failing on MTK platforms anyway, right, 
since your IOMMU is still capable of addressing that? Seems like we 
overlooked that check in __arm_v7s_alloc_table(), where the conversion 
by casting will want updating to something like 
"iopte_to_paddr(paddr_to_iopte(...))" in patch #4 of your series.

> I planed to add GFP_DMA32 for pagetable allocation. the level1 was
> ok but level2 was failed. It looks that slab don't like GFP_DMA32[2].
> this is the warning log:
> =====
> Unexpected gfp: 0x4 (GFP_DMA32). Fixing up to gfp: 0x488020 (GFP_ATOMIC|
> __GFP_ZERO). Fix your code!
> =====
> I don't know why slab don't allow GFP_DMA32, the gfp flags seems only
> be bypassed to alloc_page. Is it possible that let slab allow GFP_DMA32
> for aarch64?
I had a bit of a look into it some time ago, and I don't recall seeing 
any obvious reason that the kmem_cache API couldn't support ZONE_DMA32 
in general (either via a separate SLAB_CACHE_DMA32, or just overloading 
SLAB_CACHE_DMA depending on which of CONFIG_ZONE_DMA/CONFIG_ZONE_DMA32 
are enabled) - I guess that's just never been implemented since nobody 
needed it before.

Robin.

> We notice that this has been discussed[3]. but if we use alloc_page for
> the level2 pagetable, It may waste lots of memory.
> 
> Any suggestion is appreciated.
> 
> 
> Reported-by: Nicolas Boichat <drinkcat-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> [1]
> https://elixir.bootlin.com/linux/v4.20-rc1/source/drivers/iommu/io-pgtable-arm-v7s.c#L200
> [2] https://elixir.bootlin.com/linux/v4.20-rc1/source/mm/internal.h#L37
> [3] https://patchwork.kernel.org/patch/10474289/
> 

Re: iommu/io-pgtable-arm-v7s: About pagetable 33bit PA

[Yong Wu]

On Thu, 2018-11-08 at 13:49 +0000, Robin Murphy wrote:
> On 08/11/2018 07:31, Yong Wu wrote:
> > Hi Robin,
> > 
> > After the commit ad67f5a6545f ("arm64: replace ZONE_DMA with
> > ZONE_DMA32"), we don't have ZONE_DMA in aarch64, but
> > __arm_v7s_alloc_table[1] use the GFP_DMA to expect the physical address
> > of pagetable should be 32bit.
> > 
> > Right now we meet a issue that the lvl1 pagetable PA is 0x1_3ab6_0000 in
> > the 4GB broad. then the IOMMU initialize failed.(This issue can be fixed
> > if we revert ad67f5a6545f.)
> 
> But that shouldn't actually be failing on MTK platforms anyway, right, 
> since your IOMMU is still capable of addressing that? Seems like we 
> overlooked that check in __arm_v7s_alloc_table(), where the conversion 
> by casting will want updating to something like 
> "iopte_to_paddr(paddr_to_iopte(...))" in patch #4 of your series.

The current mt8183 IOMMU support the pagetable address over 4GB.
Unfortunately the previous mt8173 don't support.(mt8173 IOMMU support
mapping for the dram over 4GB while its pagetable still need locate
below 4GB.)

In order to unify the code, we still expect lvl2 pagetable base don't
over 4GB. thus I didn't change it in the current patchset.

> 
> > I planed to add GFP_DMA32 for pagetable allocation. the level1 was
> > ok but level2 was failed. It looks that slab don't like GFP_DMA32[2].
> > this is the warning log:
> > =====
> > Unexpected gfp: 0x4 (GFP_DMA32). Fixing up to gfp: 0x488020 (GFP_ATOMIC|
> > __GFP_ZERO). Fix your code!
> > =====
> > I don't know why slab don't allow GFP_DMA32, the gfp flags seems only
> > be bypassed to alloc_page. Is it possible that let slab allow GFP_DMA32
> > for aarch64?
> I had a bit of a look into it some time ago, and I don't recall seeing 
> any obvious reason that the kmem_cache API couldn't support ZONE_DMA32 
> in general (either via a separate SLAB_CACHE_DMA32, or just overloading 
> SLAB_CACHE_DMA depending on which of CONFIG_ZONE_DMA/CONFIG_ZONE_DMA32 
> are enabled) - I guess that's just never been implemented since nobody 
> needed it before.

Thanks for the comment. We could try a patch for this.

> 
> Robin.
> 
> > We notice that this has been discussed[3]. but if we use alloc_page for
> > the level2 pagetable, It may waste lots of memory.
> > 
> > Any suggestion is appreciated.
> > 
> > 
> > Reported-by: Nicolas Boichat <drinkcat-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > 
> > [1]
> > https://elixir.bootlin.com/linux/v4.20-rc1/source/drivers/iommu/io-pgtable-arm-v7s.c#L200
> > [2] https://elixir.bootlin.com/linux/v4.20-rc1/source/mm/internal.h#L37
> > [3] https://patchwork.kernel.org/patch/10474289/
> > 

Re: iommu/io-pgtable-arm-v7s: About pagetable 33bit PA

[Nicolas Boichat]

Hi Robin/Yong,

On Fri, Nov 9, 2018 at 3:51 PM Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>
> On Thu, 2018-11-08 at 13:49 +0000, Robin Murphy wrote:
> > On 08/11/2018 07:31, Yong Wu wrote:
> > > Hi Robin,
> > >
> > > After the commit ad67f5a6545f ("arm64: replace ZONE_DMA with
> > > ZONE_DMA32"), we don't have ZONE_DMA in aarch64, but
> > > __arm_v7s_alloc_table[1] use the GFP_DMA to expect the physical address
> > > of pagetable should be 32bit.
> > >
> > > Right now we meet a issue that the lvl1 pagetable PA is 0x1_3ab6_0000 in
> > > the 4GB broad. then the IOMMU initialize failed.(This issue can be fixed
> > > if we revert ad67f5a6545f.)
> >
> > But that shouldn't actually be failing on MTK platforms anyway, right,
> > since your IOMMU is still capable of addressing that? Seems like we
> > overlooked that check in __arm_v7s_alloc_table(), where the conversion
> > by casting will want updating to something like
> > "iopte_to_paddr(paddr_to_iopte(...))" in patch #4 of your series.
>
> The current mt8183 IOMMU support the pagetable address over 4GB.
> Unfortunately the previous mt8173 don't support.(mt8173 IOMMU support
> mapping for the dram over 4GB while its pagetable still need locate
> below 4GB.)
>
> In order to unify the code, we still expect lvl2 pagetable base don't
> over 4GB. thus I didn't change it in the current patchset.
>
> >
> > > I planed to add GFP_DMA32 for pagetable allocation. the level1 was
> > > ok but level2 was failed. It looks that slab don't like GFP_DMA32[2].
> > > this is the warning log:
> > > =====
> > > Unexpected gfp: 0x4 (GFP_DMA32). Fixing up to gfp: 0x488020 (GFP_ATOMIC|
> > > __GFP_ZERO). Fix your code!
> > > =====
> > > I don't know why slab don't allow GFP_DMA32, the gfp flags seems only
> > > be bypassed to alloc_page. Is it possible that let slab allow GFP_DMA32
> > > for aarch64?
> > I had a bit of a look into it some time ago, and I don't recall seeing
> > any obvious reason that the kmem_cache API couldn't support ZONE_DMA32
> > in general (either via a separate SLAB_CACHE_DMA32, or just overloading
> > SLAB_CACHE_DMA depending on which of CONFIG_ZONE_DMA/CONFIG_ZONE_DMA32
> > are enabled) - I guess that's just never been implemented since nobody
> > needed it before.
>
> Thanks for the comment. We could try a patch for this.

I made an attempt here, if that's helpful:
https://lore.kernel.org/patchwork/cover/1009116/ .

I'm still a bit uncomfortable with GFP_DMA passed to kmem_cache_alloc
suddenly becoming GFP_DMA32 in the underlying call, but I'm not sure
if there is a better way (apart from duplicating the caches, and a lot
of ifdefs in callers).

Thanks,

> >
> > Robin.
> >
> > > We notice that this has been discussed[3]. but if we use alloc_page for
> > > the level2 pagetable, It may waste lots of memory.
> > >
> > > Any suggestion is appreciated.
> > >
> > >
> > > Reported-by: Nicolas Boichat <drinkcat-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > >
> > > [1]
> > > https://elixir.bootlin.com/linux/v4.20-rc1/source/drivers/iommu/io-pgtable-arm-v7s.c#L200
> > > [2] https://elixir.bootlin.com/linux/v4.20-rc1/source/mm/internal.h#L37
> > > [3] https://patchwork.kernel.org/patch/10474289/
> > >
>
>