[PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Shaohui Xie]

PowerPC platform only supports ZONE_DMA zone for 64bit kernel, so all the
memory will be put into this zone. If the memory size is greater than
the device's DMA capability and device uses dma_alloc_coherent to allocate
memory, it will get an address which is over the device's DMA addressing,
the device will fail.

So we split the memory to two zones by adding a zone ZONE_NORMAL, since
we already allocate PCICSRBAR/PEXCSRBAR right below the 4G boundary (if the
lowest PCI address is above 4G), so we constrain the DMA zone ZONE_DMA
to 2GB, also, we clear the flag __GFP_DMA and set it only if the device's
dma_mask < total memory size. By doing this, devices which cannot DMA all
the memory will be limited to ZONE_DMA, but devices which can DMA all the
memory will not be affected by this limitation.

Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
Signed-off-by: Chen Yuanquan <B41889@freescale.com>
---
 arch/powerpc/kernel/dma.c |   13 ++++++++++++-
 arch/powerpc/mm/mem.c     |    4 +++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/dma.c b/arch/powerpc/kernel/dma.c
index b1ec983..8029295 100644
--- a/arch/powerpc/kernel/dma.c
+++ b/arch/powerpc/kernel/dma.c
@@ -30,6 +30,7 @@ void *dma_direct_alloc_coherent(struct device *dev, size_t size,
 				struct dma_attrs *attrs)
 {
 	void *ret;
+	phys_addr_t top_ram_pfn = memblock_end_of_DRAM();
 #ifdef CONFIG_NOT_COHERENT_CACHE
 	ret = __dma_alloc_coherent(dev, size, dma_handle, flag);
 	if (ret == NULL)
@@ -40,8 +41,18 @@ void *dma_direct_alloc_coherent(struct device *dev, size_t size,
 	struct page *page;
 	int node = dev_to_node(dev);
 
+	/*
+	 * check for crappy device which has dma_mask < ZONE_DMA, and
+	 * we are not going to support it, just warn and fail.
+	 */
+	if (*dev->dma_mask < DMA_BIT_MASK(31)) {
+		dev_err(dev, "Unsupported dma_mask 0x%llx\n", *dev->dma_mask);
+		return NULL;
+	}
 	/* ignore region specifiers */
-	flag  &= ~(__GFP_HIGHMEM);
+	flag  &= ~(__GFP_HIGHMEM | __GFP_DMA);
+	if (*dev->dma_mask < top_ram_pfn - 1)
+		flag |= GFP_DMA;
 
 	page = alloc_pages_node(node, flag, get_order(size));
 	if (page == NULL)
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index baaafde..a494555 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -281,7 +281,9 @@ void __init paging_init(void)
 	max_zone_pfns[ZONE_DMA] = lowmem_end_addr >> PAGE_SHIFT;
 	max_zone_pfns[ZONE_HIGHMEM] = top_of_ram >> PAGE_SHIFT;
 #else
-	max_zone_pfns[ZONE_DMA] = top_of_ram >> PAGE_SHIFT;
+	max_zone_pfns[ZONE_DMA] = min_t(phys_addr_t, top_of_ram,
+					1ull << 31) >> PAGE_SHIFT;
+	max_zone_pfns[ZONE_NORMAL] = top_of_ram >> PAGE_SHIFT;
 #endif
 	free_area_init_nodes(max_zone_pfns);
 
-- 
1.6.4

Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Benjamin Herrenschmidt]

On Fri, 2012-07-20 at 20:21 +0800, Shaohui Xie wrote:
> PowerPC platform only supports ZONE_DMA zone for 64bit kernel, so all the
> memory will be put into this zone. If the memory size is greater than
> the device's DMA capability and device uses dma_alloc_coherent to allocate
> memory, it will get an address which is over the device's DMA addressing,
> the device will fail.
> 
> So we split the memory to two zones by adding a zone ZONE_NORMAL, since
> we already allocate PCICSRBAR/PEXCSRBAR right below the 4G boundary (if the
> lowest PCI address is above 4G), so we constrain the DMA zone ZONE_DMA
> to 2GB, also, we clear the flag __GFP_DMA and set it only if the device's
> dma_mask < total memory size. By doing this, devices which cannot DMA all
> the memory will be limited to ZONE_DMA, but devices which can DMA all the
> memory will not be affected by this limitation.

This is wrong. Don't you have an iommu do deal with those devices
anyway ? What about swiotlb ?

If you *really* need to honor 32 (or 31 even) bit DMAs, what you -may-
want to do is create a ZONE_DMA32 like other architectures, do not
hijack the historical ZONE_DMA.

But even then, I'm dubious this is really needed.

Cheers,
Ben.

Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Scott Wood]

On 07/23/2012 01:06 AM, Benjamin Herrenschmidt wrote:
> On Fri, 2012-07-20 at 20:21 +0800, Shaohui Xie wrote:
>> PowerPC platform only supports ZONE_DMA zone for 64bit kernel, so all the
>> memory will be put into this zone. If the memory size is greater than
>> the device's DMA capability and device uses dma_alloc_coherent to allocate
>> memory, it will get an address which is over the device's DMA addressing,
>> the device will fail.
>>
>> So we split the memory to two zones by adding a zone ZONE_NORMAL, since
>> we already allocate PCICSRBAR/PEXCSRBAR right below the 4G boundary (if the
>> lowest PCI address is above 4G), so we constrain the DMA zone ZONE_DMA
>> to 2GB, also, we clear the flag __GFP_DMA and set it only if the device's
>> dma_mask < total memory size. By doing this, devices which cannot DMA all
>> the memory will be limited to ZONE_DMA, but devices which can DMA all the
>> memory will not be affected by this limitation.
> 
> This is wrong.

How so?

> Don't you have an iommu do deal with those devices anyway ?

Yes, but we don't yet have DMA API support for it, it would lower
performance because we'd have to use a lot of subwindows which are
poorly cached (and even then we wouldn't be able to map more than 256
pages at once on a given device), and the IOMMU may not be available at
all if we're being virtualized.

> What about swiotlb ?

That doesn't help with alloc_coherent().

> If you *really* need to honor 32 (or 31 even) bit DMAs,

31-bit is to accommodate PCI, which has PEXCSRBAR that must live under 4
GiB and can't be disabled.

> what you -may- want to do is create a ZONE_DMA32 like other architectures, do not
> hijack the historical ZONE_DMA.

Could you point me to somewhere that clearly defines what ZONE_DMA is to
be used for, such that this counts as hijacking (but using ZONE_DMA32 to
mean 31-bit wouldn't)?  The only arches I see using ZONE_DMA32 (x86 and
mips) also have a separate, more restrictive ZONE_DMA.  PowerPC doesn't.
 It uses ZONE_DMA to point to all of memory (except highmem on 32-bit)
-- how is that not hijacking, if this is?  We can't have ZONE_DMA be
less restrictive than ZONE_DMA32, because the fallback rules are
hardcoded the other way around in generic code.

The exact threshold for ZONE_DMA could be made platform-configurable.

> But even then, I'm dubious this is really needed.

We'd like our drivers to stop crashing with more than 4GiB of RAM on 64-bit.

-Scott

Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Benjamin Herrenschmidt]

On Mon, 2012-07-23 at 11:17 -0500, Scott Wood wrote:
> > This is wrong.
> 
> How so?
> 
> > Don't you have an iommu do deal with those devices anyway ?
> 
> Yes, but we don't yet have DMA API support for it, it would lower
> performance because we'd have to use a lot of subwindows which are
> poorly cached (and even then we wouldn't be able to map more than 256
> pages at once on a given device), and the IOMMU may not be available at
> all if we're being virtualized.

Ugh ? You mean some designers need to be fired urgently and wasted
everybody's time implementing an unusable iommu ? Nice one ...

> > What about swiotlb ?
> 
> That doesn't help with alloc_coherent().

Somewhat... you can use the pool, but it sucks.

> > If you *really* need to honor 32 (or 31 even) bit DMAs,
> 
> 31-bit is to accommodate PCI, which has PEXCSRBAR that must live under 4
> GiB and can't be disabled.
> 
> > what you -may- want to do is create a ZONE_DMA32 like other architectures, do not
> > hijack the historical ZONE_DMA.
> 
> Could you point me to somewhere that clearly defines what ZONE_DMA is to
> be used for, such that this counts as hijacking (but using ZONE_DMA32 to
> mean 31-bit wouldn't)?

Habit and history. ZONE_DMA used to be about ISA DMA, doesn't apply to
us, and in general ZONE_NORMAL alias to it iirc. It's old stuff I
haven't looked for a long time.

However, my understanding is that what you are trying to solve is
exactly what ZONE_DMA32 was created for.

>   The only arches I see using ZONE_DMA32 (x86 and
> mips) also have a separate, more restrictive ZONE_DMA.

And ? Who cares ? Drivers who know about a 32-bit limitations use
GFP_DMA32, that's what is expected, don't mess around with ZONE_DMA.

>   PowerPC doesn't.
>  It uses ZONE_DMA to point to all of memory (except highmem on 32-bit)
> -- how is that not hijacking, if this is?  We can't have ZONE_DMA be
> less restrictive than ZONE_DMA32, because the fallback rules are
> hardcoded the other way around in generic code.
> 
> The exact threshold for ZONE_DMA could be made platform-configurable.
> 
> > But even then, I'm dubious this is really needed.
> 
> We'd like our drivers to stop crashing with more than 4GiB of RAM on 64-bit.

Fix your HW :-)

Cheers,
Ben.

Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Tabi Timur-B04825]

On Mon, Jul 23, 2012 at 5:20 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:

> And ? Who cares ? Drivers who know about a 32-bit limitations use
> GFP_DMA32, that's what is expected, don't mess around with ZONE_DMA.

I thought drivers are supposed to set a dma_mask, and
dma_alloc_coherent() is supposed to use that to figure out how to
honor that mask.

--=20
Timur Tabi
Linux kernel developer at Freescale=

Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Benjamin Herrenschmidt]

On Mon, 2012-07-23 at 23:08 +0000, Tabi Timur-B04825 wrote:
> 
> > And ? Who cares ? Drivers who know about a 32-bit limitations use
> > GFP_DMA32, that's what is expected, don't mess around with ZONE_DMA.
> 
> I thought drivers are supposed to set a dma_mask, and
> dma_alloc_coherent() is supposed to use that to figure out how to
> honor that mask.

Sure, that's the right way to go, I meant bits of pieces of the
infrastructure in between. Why diverge from other archs gratuituously
here ?

Cheers,
Ben.

Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Timur Tabi]

Benjamin Herrenschmidt wrote:
> Sure, that's the right way to go, I meant bits of pieces of the
> infrastructure in between. Why diverge from other archs gratuituously
> here ?

Ok, I'm confused.  Are you suggesting that drivers do this:

u64 fsl_dma_dmamask = DMA_BIT_MASK(36);
dev->dma_mask = &fsl_dma_dmamask;
v = dma_alloc_coherent(dev, ..., GFP_DMA32);

That is, set the DMA mask *and* set GFP_DMA32?  That seems redundant.

I don't understand why a driver would set GFP_DMA32 if it has already set
the mask.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Benjamin Herrenschmidt]

On Mon, 2012-07-23 at 18:15 -0500, Timur Tabi wrote:
> Benjamin Herrenschmidt wrote:
> > Sure, that's the right way to go, I meant bits of pieces of the
> > infrastructure in between. Why diverge from other archs gratuituously
> > here ?
> 
> Ok, I'm confused.  Are you suggesting that drivers do this:
> 
> u64 fsl_dma_dmamask = DMA_BIT_MASK(36);
> dev->dma_mask = &fsl_dma_dmamask;
> v = dma_alloc_coherent(dev, ..., GFP_DMA32);
> 
> That is, set the DMA mask *and* set GFP_DMA32?  That seems redundant.

No, but dma_alloc_coherent would under the hood.

> I don't understand why a driver would set GFP_DMA32 if it has already set
> the mask.

The layers in between, not the well behaved drivers. Again, we have
ZONE_DMA32 specifically for the purpose, why use something else ?

In any case, make the whole thing at the very least a config option, I
don't want sane HW to have to deal with split zones.

Cheers,
Ben.

Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Benjamin Herrenschmidt]

On Tue, 2012-07-24 at 09:29 +1000, Benjamin Herrenschmidt wrote:

> The layers in between, not the well behaved drivers. Again, we have
> ZONE_DMA32 specifically for the purpose, why use something else ?
> 
> In any case, make the whole thing at the very least a config option, I
> don't want sane HW to have to deal with split zones.

Or if possible a flag set by machine probe()

Cheers,
Ben.

Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Scott Wood]

On 07/23/2012 06:30 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2012-07-24 at 09:29 +1000, Benjamin Herrenschmidt wrote:
> 
>> The layers in between, not the well behaved drivers. Again, we have
>> ZONE_DMA32 specifically for the purpose, why use something else ?
>>
>> In any case, make the whole thing at the very least a config option, I
>> don't want sane HW to have to deal with split zones.
> 
> Or if possible a flag set by machine probe()

I suggested making the threshold configurable by platform code.  Sane
hardware would leave it at its default of infinity (~0ULL), and you
wouldn't have a split zone.  Our hardware would set it at 31-bit.

It looks like this is already sort-of done for ISA_DMA_THRESHOLD for
32-bit non-coherent-DMA platforms (amigaone sets a 24-bit threshold),
though I don't see where ZONE_DMA is limited accordingly.

We'd add an additional threshold for ZONE_DMA32, make it actually affect
the zone definition, and make the standard alloc_coherent() honor it.

-Scott

Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Timur Tabi]

Benjamin Herrenschmidt wrote:

> No, but dma_alloc_coherent would under the hood.

Which is what Shaohui's patch does.  Well, it does it for GFP_DMA instead
of GFP_DMA32, but still.

When you said, "Drivers who know about a 32-bit limitations use
GFP_DMA32", I thought you meant that drivers should *set* GFP_DMA32.

>> I don't understand why a driver would set GFP_DMA32 if it has already set
>> the mask.
> 
> The layers in between, not the well behaved drivers. Again, we have
> ZONE_DMA32 specifically for the purpose, why use something else ?
> 
> In any case, make the whole thing at the very least a config option, I
> don't want sane HW to have to deal with split zones.

The DMA zone only kicks in if the DMA mask is set to a size smaller that
available physical memory.  Sane HW should set the DMA mask to
DMA_BIT_MASK(36).  And we have plenty of sane HW on our SOCs, but not
every device is like that.

The whole point behind this patch is that some drivers are setting a DMA
mask of 32, but still getting memory above 4GB.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Benjamin Herrenschmidt]

On Mon, 2012-07-23 at 18:36 -0500, Timur Tabi wrote:
> The DMA zone only kicks in if the DMA mask is set to a size smaller
> that
> available physical memory.  Sane HW should set the DMA mask to
> DMA_BIT_MASK(36).  And we have plenty of sane HW on our SOCs, but not
> every device is like that.
> 
> The whole point behind this patch is that some drivers are setting a
> DMA
> mask of 32, but still getting memory above 4GB.

Sure but I don't want to create the zones in the first place (and thus
introduce the added pressure on the memory management) on machines that
don't need it.

Cheers,
Ben.

Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Tabi Timur-B04825]

Benjamin Herrenschmidt wrote:
> Sure but I don't want to create the zones in the first place (and thus
> introduce the added pressure on the memory management) on machines that
> don't need it.

Ah yes, I forgot about memory pressure.

--=20
Timur Tabi
Linux kernel developer at Freescale=

RE: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Zang Roy-R61911]

> -----Original Message-----
> From: Linuxppc-dev [mailto:linuxppc-dev-bounces+tie-
> fei.zang=3Dfreescale.com@lists.ozlabs.org] On Behalf Of Benjamin
> Herrenschmidt
> Sent: Tuesday, July 24, 2012 11:09 AM
> To: Tabi Timur-B04825
> Cc: Wood Scott-B07421; Hu Mingkai-B21284; linuxppc-dev@lists.ozlabs.org;
> Xie Shaohui-B21989; Chen Yuanquan-B41889
> Subject: Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel
>=20
> On Mon, 2012-07-23 at 18:36 -0500, Timur Tabi wrote:
> > The DMA zone only kicks in if the DMA mask is set to a size smaller
> > that
> > available physical memory.  Sane HW should set the DMA mask to
> > DMA_BIT_MASK(36).  And we have plenty of sane HW on our SOCs, but not
> > every device is like that.
> >
> > The whole point behind this patch is that some drivers are setting a
> > DMA
> > mask of 32, but still getting memory above 4GB.
>=20
> Sure but I don't want to create the zones in the first place (and thus
> introduce the added pressure on the memory management) on machines that
> don't need it.
OK, then how do you think Scott's suggestion to add a configurable threshol=
d?
Roy

Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Tabi Timur-B04825]

Benjamin Herrenschmidt wrote:
> Sure but I don't want to create the zones in the first place (and thus
> introduce the added pressure on the memory management) on machines that
> don't need it.

One thing that does confuse me -- by default, we don't create a=20
ZONE_NORMAL.  We only create a ZONE_DMA.  Why is that?  Shouldn't it be=20
the other way around?

--=20
Timur Tabi
Linux kernel developer at Freescale=

Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Benjamin Herrenschmidt]

On Tue, 2012-07-24 at 04:04 +0000, Tabi Timur-B04825 wrote:
> Benjamin Herrenschmidt wrote:
> > Sure but I don't want to create the zones in the first place (and thus
> > introduce the added pressure on the memory management) on machines that
> > don't need it.
> 
> One thing that does confuse me -- by default, we don't create a 
> ZONE_NORMAL.  We only create a ZONE_DMA.  Why is that?  Shouldn't it be 
> the other way around?

Because ZONE_NORMAL allocations can be serviced from the ZONE_DMA while
the other way isn't possible.

Especially in the old days, there were quite a few cases of drivers
and/or subsystems who were a bit heavy handed at using ZONE_DMA, so not
having one would essentially make them not work at all.

Cheers,
Ben.

RE: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+bharat.bhushan=3Dfreescale.com@lists.ozlabs.org] On Behalf Of Ben=
jamin
> Herrenschmidt
> Sent: Tuesday, July 24, 2012 10:16 AM
> To: Tabi Timur-B04825
> Cc: Wood Scott-B07421; Hu Mingkai-B21284; linuxppc-dev@lists.ozlabs.org; =
Xie
> Shaohui-B21989; Chen Yuanquan-B41889
> Subject: Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel
>=20
> On Tue, 2012-07-24 at 04:04 +0000, Tabi Timur-B04825 wrote:
> > Benjamin Herrenschmidt wrote:
> > > Sure but I don't want to create the zones in the first place (and
> > > thus introduce the added pressure on the memory management) on
> > > machines that don't need it.
> >
> > One thing that does confuse me -- by default, we don't create a
> > ZONE_NORMAL.  We only create a ZONE_DMA.  Why is that?  Shouldn't it
> > be the other way around?
>=20
> Because ZONE_NORMAL allocations can be serviced from the ZONE_DMA while t=
he
> other way isn't possible.

Say, if we have defined only one zone (ZONE_DMA) to which we give all memor=
y ( > 4G).
Device set the DMA_MASK to 4G or less.

dma_alloc_coherent() will set GFP_DMA flag, But that is of no use, because =
the memory allocator have only one zone which have all memory (which assume=
s all dma-able). And can return memory at address at > 4G. which will crash=
 !!

I think we have to have at least one zone which gives memory to be dma-able=
 for all devices (memory limit should be set by platform, because different=
  platform have different devices with different limits.). And another ( 1 =
or more) will cover rest of memory.

Thanks
-Bharat

>=20
> Especially in the old days, there were quite a few cases of drivers and/o=
r
> subsystems who were a bit heavy handed at using ZONE_DMA, so not having o=
ne
> would essentially make them not work at all.
>=20
> Cheers,
> Ben.
>=20
>=20
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] powerpc/mm: add ZONE_NORMAL zone for 64 bit kernel

[Scott Wood]

On 07/23/2012 05:20 PM, Benjamin Herrenschmidt wrote:
> On Mon, 2012-07-23 at 11:17 -0500, Scott Wood wrote:
>>> This is wrong.
>>
>> How so?
>>
>>> Don't you have an iommu do deal with those devices anyway ?
>>
>> Yes, but we don't yet have DMA API support for it, it would lower
>> performance because we'd have to use a lot of subwindows which are
>> poorly cached (and even then we wouldn't be able to map more than 256
>> pages at once on a given device), and the IOMMU may not be available at
>> all if we're being virtualized.
> 
> Ugh ? You mean some designers need to be fired urgently and wasted
> everybody's time implementing an unusable iommu ? Nice one ...

Yeah, that's old news.

It's somewhat usable for specific purposes, using very large pages, but
not for arbitrary use.

>>> But even then, I'm dubious this is really needed.
>>
>> We'd like our drivers to stop crashing with more than 4GiB of RAM on 64-bit.
> 
> Fix your HW :-)

I wish...  Some hardware people may solicit input from us every now and
again, but ultimately they do what they want.

Returning addresses in excess of a device's declared DMA mask is
something that needs fixing too, though.

-Scott