[PATCH .36-rc8] arm: mm: allow, but warn, when issuing ioremap() on RAM

[PATCH .36-rc8] arm: mm: allow, but warn, when issuing ioremap() on RAM

[Felipe Contreras]

From: Catalin Marinas <catalin.marinas@arm.com>

Drivers have been relying on this behavior, but done so wrongly.
However, rather than breaking drivers from .35 to .36, we should warn on
.36 and only break on .37. This way we give a chance for contributors to
fix the issues.

According to ARM, the behavior of having multiple mappings is
unspecified from ARMv6+. This causes real issues specially on modern
hardware, and specially with speculative prefetching. So drivers need to
be fixed.

Also, since current hardware has palliative meassures to deal with
multiple mappings with the same memory type but diferent cacheability
attributes, ensure that such restriction is taking place.

Cc: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Richard Woodruff <r-woodruff2@ti.com>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 arch/arm/mm/ioremap.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index ab50627..7dfd6dd 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -202,10 +202,14 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
 		return NULL;
 
 	/*
-	 * Don't allow RAM to be mapped - this causes problems with ARMv6+
+	 * This causes problems with ARMv6+. Will be disallowed soon.
+	 * Also avoid a second mapping with different shareability, which is
+	 * not supposed to work anyway.
 	 */
 	if (WARN_ON(pfn_valid(pfn)))
-		return NULL;
+		if (__LINUX_ARM_ARCH__ >= 6 &&
+		    (mtype != MT_DEVICE_CACHED && mtype != MT_DEVICE_WC))
+			mtype = MT_DEVICE_WC;
 
 	type = get_mem_type(mtype);
 	if (!type)
-- 
1.7.3.1.2.g7fe2b

[PATCH .36-rc8] arm: mm: allow, but warn, when issuing ioremap() on RAM

[Uwe Kleine-König]

On Fri, Oct 15, 2010 at 05:15:20PM +0300, Felipe Contreras wrote:
> From: Catalin Marinas <catalin.marinas@arm.com>
> 
> Drivers have been relying on this behavior, but done so wrongly.
> However, rather than breaking drivers from .35 to .36, we should warn on
> .36 and only break on .37. This way we give a chance for contributors to
> fix the issues.
> 
> According to ARM, the behavior of having multiple mappings is
> unspecified from ARMv6+. This causes real issues specially on modern
> hardware, and specially with speculative prefetching. So drivers need to
> be fixed.
> 
> Also, since current hardware has palliative meassures to deal with
> multiple mappings with the same memory type but diferent cacheability
> attributes, ensure that such restriction is taking place.
> 
> Cc: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Richard Woodruff <r-woodruff2@ti.com>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  arch/arm/mm/ioremap.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
> index ab50627..7dfd6dd 100644
> --- a/arch/arm/mm/ioremap.c
> +++ b/arch/arm/mm/ioremap.c
> @@ -202,10 +202,14 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
>  		return NULL;
>  
>  	/*
> -	 * Don't allow RAM to be mapped - this causes problems with ARMv6+
> +	 * This causes problems with ARMv6+. Will be disallowed soon.
s/soon/for 2.6.37/ ?

> +	 * Also avoid a second mapping with different shareability, which is
> +	 * not supposed to work anyway.
>  	 */
>  	if (WARN_ON(pfn_valid(pfn)))
> -		return NULL;
> +		if (__LINUX_ARM_ARCH__ >= 6 &&
> +		    (mtype != MT_DEVICE_CACHED && mtype != MT_DEVICE_WC))
> +			mtype = MT_DEVICE_WC;
>  
>  	type = get_mem_type(mtype);
>  	if (!type)
> -- 
> 1.7.3.1.2.g7fe2b
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH .36-rc8] arm: mm: allow, but warn, when issuing ioremap() on RAM

[Felipe Contreras]

On Fri, Oct 15, 2010 at 5:15 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> From: Catalin Marinas <catalin.marinas@arm.com>
>
> Drivers have been relying on this behavior, but done so wrongly.
> However, rather than breaking drivers from .35 to .36, we should warn on
> .36 and only break on .37. This way we give a chance for contributors to
> fix the issues.
>
> According to ARM, the behavior of having multiple mappings is
> unspecified from ARMv6+. This causes real issues specially on modern
> hardware, and specially with speculative prefetching. So drivers need to
> be fixed.
>
> Also, since current hardware has palliative meassures to deal with
> multiple mappings with the same memory type but diferent cacheability
> attributes, ensure that such restriction is taking place.

Ah, disregard this, a similar one is on .36-rc8 already (06c1088), I
wasn't CC'ed.

-- 
Felipe Contreras

[PATCH .36-rc8] arm: mm: allow, but warn, when issuing ioremap() on RAM

[Catalin Marinas]

Hi Felipe,

On Fri, 2010-10-15 at 15:15 +0100, Felipe Contreras wrote:
> From: Catalin Marinas <catalin.marinas@arm.com>
> 
> Drivers have been relying on this behavior, but done so wrongly.
> However, rather than breaking drivers from .35 to .36, we should warn on
> .36 and only break on .37. This way we give a chance for contributors to
> fix the issues.
> 
> According to ARM, the behavior of having multiple mappings is
> unspecified from ARMv6+. This causes real issues specially on modern
> hardware, and specially with speculative prefetching. So drivers need to
> be fixed.
> 
> Also, since current hardware has palliative meassures to deal with
> multiple mappings with the same memory type but diferent cacheability
> attributes, ensure that such restriction is taking place.
> 
> Cc: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Richard Woodruff <r-woodruff2@ti.com>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

I just showed a partial workaround but I did not sign off my patch. Not
sure if just posting a code snippet counts as an explicit signed-off-by.

> --- a/arch/arm/mm/ioremap.c
> +++ b/arch/arm/mm/ioremap.c
> @@ -202,10 +202,14 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
>                 return NULL;
> 
>         /*
> -        * Don't allow RAM to be mapped - this causes problems with ARMv6+
> +        * This causes problems with ARMv6+. Will be disallowed soon.
> +        * Also avoid a second mapping with different shareability, which is
> +        * not supposed to work anyway.
>          */
>         if (WARN_ON(pfn_valid(pfn)))
> -               return NULL;
> +               if (__LINUX_ARM_ARCH__ >= 6 &&
> +                   (mtype != MT_DEVICE_CACHED && mtype != MT_DEVICE_WC))
> +                       mtype = MT_DEVICE_WC;
> 
>         type = get_mem_type(mtype);
>         if (!type)

And one of the reasons is that this patch is incomplete. You still
create a mapping alias (though now with the same memory type and
different cacheability attributes) but there isn't anything to take care
of the potential dirty cache lines in the original alias (e.g. kernel
linear mapping). You can easily end up with data corruption.

-- 
Catalin

[PATCH .36-rc8] arm: mm: allow, but warn, when issuing ioremap() on RAM

[Russell King - ARM Linux]

On Fri, Oct 15, 2010 at 03:30:21PM +0100, Catalin Marinas wrote:
> Hi Felipe,
> 
> On Fri, 2010-10-15 at 15:15 +0100, Felipe Contreras wrote:
> > From: Catalin Marinas <catalin.marinas@arm.com>
> > 
> > Drivers have been relying on this behavior, but done so wrongly.
> > However, rather than breaking drivers from .35 to .36, we should warn on
> > .36 and only break on .37. This way we give a chance for contributors to
> > fix the issues.
> > 
> > According to ARM, the behavior of having multiple mappings is
> > unspecified from ARMv6+. This causes real issues specially on modern
> > hardware, and specially with speculative prefetching. So drivers need to
> > be fixed.
> > 
> > Also, since current hardware has palliative meassures to deal with
> > multiple mappings with the same memory type but diferent cacheability
> > attributes, ensure that such restriction is taking place.
> > 
> > Cc: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Richard Woodruff <r-woodruff2@ti.com>
> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> 
> I just showed a partial workaround but I did not sign off my patch. Not
> sure if just posting a code snippet counts as an explicit signed-off-by.

It doesn't, and that's a sign of bad practice.  Given the legal implications
of Signed-off-by, this is not something _anyone_ should be adding for
addresses other than their own - otherwise it's a plain and simple lie.

> And one of the reasons is that this patch is incomplete. You still
> create a mapping alias (though now with the same memory type and
> different cacheability attributes) but there isn't anything to take care
> of the potential dirty cache lines in the original alias (e.g. kernel
> linear mapping). You can easily end up with data corruption.

It seems that the overall feeling from those in the know (iow, those
who work for companies who know the processor IP) is that the behaviour
we _had_ until -rc8 was the right one.

It's now been relaxed in -rc8 against those people's better judgement,
and issues a big fat textual three line warning about it which is probably
going to make users panic about it.  (Any warn-on dump also makes users
panic, but the three lines explaining it will probably give them a heart
attack too!)

As soon as the first ARM merge hits during the merge window, I'll be
restoring the 'always fail' behaviour.

[PATCH .36-rc8] arm: mm: allow, but warn, when issuing ioremap() on RAM

[Felipe Contreras]

On Fri, Oct 15, 2010 at 10:42 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> As soon as the first ARM merge hits during the merge window, I'll be
> restoring the 'always fail' behaviour.

Ok, are you going to merge also your patch to use memblock for the
initialization? Many drivers could certainly use that to fix the
issue.

-- 
Felipe Contreras

[PATCH .36-rc8] arm: mm: allow, but warn, when issuing ioremap() on RAM

[Russell King - ARM Linux]

On Fri, Oct 15, 2010 at 11:00:40PM +0300, Felipe Contreras wrote:
> On Fri, Oct 15, 2010 at 10:42 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > As soon as the first ARM merge hits during the merge window, I'll be
> > restoring the 'always fail' behaviour.
> 
> Ok, are you going to merge also your patch to use memblock for the
> initialization? Many drivers could certainly use that to fix the
> issue.

There's a lot of work going on with memblock in x86-land which impacts
ARM and means major conflicts for that patch - I think first we'll have
to deal with the fallout from that (iow, finding out what's been broken
by this activity and fix that), and then I'll have to re-do my patch from
the beginning.

Technically, following the rules, that means it has to miss this merge
window - so it looks like it's going to take another six months to solve
this issue (three months to get the memblock patch in, and another three
months for driver fixes to find their way through.)

I find these timescales (12 months) rather unacceptable to fix a problem
such as this.  Makes me wonder why I even bother trying.

[PATCH .36-rc8] arm: mm: allow, but warn, when issuing ioremap() on RAM

[Felipe Contreras]

On Fri, Oct 15, 2010 at 11:14 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Oct 15, 2010 at 11:00:40PM +0300, Felipe Contreras wrote:
>> On Fri, Oct 15, 2010 at 10:42 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > As soon as the first ARM merge hits during the merge window, I'll be
>> > restoring the 'always fail' behaviour.
>>
>> Ok, are you going to merge also your patch to use memblock for the
>> initialization? Many drivers could certainly use that to fix the
>> issue.
>
> There's a lot of work going on with memblock in x86-land which impacts
> ARM and means major conflicts for that patch - I think first we'll have
> to deal with the fallout from that (iow, finding out what's been broken
> by this activity and fix that), and then I'll have to re-do my patch from
> the beginning.
>
> Technically, following the rules, that means it has to miss this merge
> window - so it looks like it's going to take another six months to solve
> this issue (three months to get the memblock patch in, and another three
> months for driver fixes to find their way through.)

I don't think that's a good idea. Drivers would not have a way to get
fixed, even for .37.

Are these x86 changes in some branch? Would it help if I try to port
your patch on top of that? IOW; is there any chance of getting these
drivers fixed on .37?

If not, I guess people would have to revert the patch that disables
ioremap() on RAM on their trees.

> I find these timescales (12 months) rather unacceptable to fix a problem
> such as this. ?Makes me wonder why I even bother trying.

As I said, if the warning was merged on .35 (or even before) I think
things would have gone smoother, but I guess we'll see on .36 what
happens.

-- 
Felipe Contreras

[PATCH .36-rc8] arm: mm: allow, but warn, when issuing ioremap() on RAM

[Russell King - ARM Linux]

On Sun, Oct 17, 2010 at 03:06:44PM +0300, Felipe Contreras wrote:
> On Fri, Oct 15, 2010 at 11:14 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Fri, Oct 15, 2010 at 11:00:40PM +0300, Felipe Contreras wrote:
> >> On Fri, Oct 15, 2010 at 10:42 PM, Russell King - ARM Linux
> >> <linux@arm.linux.org.uk> wrote:
> >> > As soon as the first ARM merge hits during the merge window, I'll be
> >> > restoring the 'always fail' behaviour.
> >>
> >> Ok, are you going to merge also your patch to use memblock for the
> >> initialization? Many drivers could certainly use that to fix the
> >> issue.
> >
> > There's a lot of work going on with memblock in x86-land which impacts
> > ARM and means major conflicts for that patch - I think first we'll have
> > to deal with the fallout from that (iow, finding out what's been broken
> > by this activity and fix that), and then I'll have to re-do my patch from
> > the beginning.
> >
> > Technically, following the rules, that means it has to miss this merge
> > window - so it looks like it's going to take another six months to solve
> > this issue (three months to get the memblock patch in, and another three
> > months for driver fixes to find their way through.)
> 
> I don't think that's a good idea. Drivers would not have a way to get
> fixed, even for .37.
> 
> Are these x86 changes in some branch?

Yes but I've no idea where they are, and neither do I know exactly what
they are - they've never been copied to me or the ARM lists.  I'll just
wait for them to appear via Linus' tree in this merge window, and as I
said above, then fix the resulting breakage from them.