[PATCH] Fix direct mapping alias regressin in ioremap v2

[PATCH] Fix direct mapping alias regressin in ioremap v2

[Andi Kleen]

Fix direct mapping alias regressin in ioremap v2

[This fixes the general ioremap bug I mentioned earlier. It was
fortunately easier to fix that I thought first.]

set_memory_*/cpa is not currently able to resolve ioremap addresses to
their direct mapping aliases.

However uncached ioremap still needs to fix up the direct mapping
to be uncached when the direct mapping happens to overlap
the remapped area. Otherwise there would be a cached mapping
to the uncached ioremap area and that is not allowed in the x86
architecture.

Do this explicitely in ioremap() by passing the direct mapping
address to cpa and ignoring the error if the address wasn't
in the direct mapping.

I also fixed a minor bug I noticed -- need to pass in size + offset,
not just size.

v2: Improve description
    Fix bug noticed by Ingo of __va() wrapping on 32bit

Signed-off-by: Andi Kleen <ak@suse.de>

Index: linux/arch/x86/mm/ioremap.c
===================================================================
--- linux.orig/arch/x86/mm/ioremap.c
+++ linux/arch/x86/mm/ioremap.c
@@ -104,6 +104,7 @@ static void __iomem *__ioremap(unsigned 
 	unsigned long pfn, offset, last_addr, vaddr;
 	struct vm_struct *area;
 	pgprot_t prot;
+	int err;
 
 	/* Don't allow wraparound or zero size */
 	last_addr = phys_addr + size - 1;
@@ -156,9 +157,16 @@ static void __iomem *__ioremap(unsigned 
 		return NULL;
 	}
 
-	if (ioremap_change_attr(vaddr, size, mode) < 0) {
-		vunmap(area->addr);
-		return NULL;
+	/* Fix up the direct mapping for the new cache attributes */
+	if ((phys_addr + size + offset) >> PAGE_SHIFT <= max_pfn_mapped) {
+		err = ioremap_change_attr((unsigned long)__va(phys_addr),
+				size + offset, mode);
+		if (err == -EINVAL) {
+			/* Original address was partly unmapped. Ignore. */
+		} else if (err < 0) {
+			vunmap(area->addr);
+			return NULL;
+		}
 	}
 
 	return (void __iomem *) (vaddr + offset);

Re: [PATCH] Fix direct mapping alias regressin in ioremap v2

[Ingo Molnar]

* Andi Kleen <andi@firstfloor.org> wrote:

>     Fix bug noticed by Ingo of __va() wrapping on 32bit

what you should realize is that had we applied your previous patch in a 
rush, it would have introduced a far more serious regression than the 
type of problem you are trying to solve. That's one of the reasons why 
we disagree with your sense of urgency.

We've got the clean fixes queued up and it's all under testing. (going 
fine so far)

Also, you only appear to have resent your fix with the obvious bug 
fixed, but it's still unclean and you do not seem to have replied to my 
main structural argument:

> > Please have a look at how we solved the "secondary alias" 64-bit 
> > problem in x86.git#mm and please resend against x86.git#mm if you 
> > still think something is missing. Thanks,

Thanks,

	Ingo

Re: [PATCH] Fix direct mapping alias regressin in ioremap v2

[Andi Kleen]

On Thu, Feb 14, 2008 at 06:42:37PM +0100, Ingo Molnar wrote:
> 
> * Andi Kleen <andi@firstfloor.org> wrote:
> 
> >     Fix bug noticed by Ingo of __va() wrapping on 32bit
> 
> what you should realize is that had we applied your previous patch in a 
> rush, it would have introduced a far more serious regression than the 
> type of problem you are trying to solve. That's one of the reasons why 
> we disagree with your sense of urgency.

Sense of urgency in this case just means for 2.6.25 which
is still a few weeks away.

Sorry for not being a perfect being. Yes I did test the patch, but
no my testing did not catch the problem unfortunately. But I'm grateful
that your review caught the problem.

> 
> We've got the clean fixes queued up and it's all under testing. (going 
> fine so far)


Ok but will you push them to .25?

> 
> Also, you only appear to have resent your fix with the obvious bug 
> fixed, but it's still unclean and you do not seem to have replied to my 

First I disagree on you describing my change as unclean. There is nothing
particularly unclean about it. It's basically back to the same
code as it was in .24 for this case.

I think fixing this regression is important for 2.6.25
and I didn't think intrusive changes to pageattr.c are a good idea anymore 
for .25 at this stage so I did a simple fix.

I chose to fix this in the simplest way I could think of by
more or less going back to the old .24 code just adapted 
for the new interfaces.

Now if you decide you want to push the big changes in #mm that
make cpa work generally for ioremaps to .25 that would probably[1] fix the 
regression too and I wouldn't complain too loudly about that. 

But at least I personally think a as simple as possible fix is better at 
this stage.

[1] I reserve judgement at this stage because I have not reviewed the latest
code there in detail yet.

> > > Please have a look at how we solved the "secondary alias" 64-bit 
> > > problem in x86.git#mm and please resend against x86.git#mm if you 
> > > still think something is missing. Thanks,

I can send it against whatever tree contains the fixes going before
2.6.25 to Linus. Is that #mm now?  I thought it was git-x86#master
so far that I is why I sent it against mainline which is virtually
identical to git-x86#master in this area.

-Andi

Re: [PATCH] Fix direct mapping alias regressin in ioremap v2

[Ingo Molnar]

* Andi Kleen <andi@firstfloor.org> wrote:

> > >     Fix bug noticed by Ingo of __va() wrapping on 32bit
> > 
> > what you should realize is that had we applied your previous patch 
> > in a rush, it would have introduced a far more serious regression 
> > than the type of problem you are trying to solve. That's one of the 
> > reasons why we disagree with your sense of urgency.
> 
> Sense of urgency in this case just means for 2.6.25 which is still a 
> few weeks away.

FYI, we had the -rc1 release 4 days ago.

> Sorry for not being a perfect being. Yes I did test the patch, but no 
> my testing did not catch the problem unfortunately. But I'm grateful 
> that your review caught the problem.

That's the task of the maintainers, to review, test and stage patches.

> > We've got the clean fixes queued up and it's all under testing. 
> > (going fine so far)
> 
> Ok but will you push them to .25?

Yes, of course, once they have been tested through - like all arch/x86 
patches/fixes. If everything goes well in overnight testing it might be 
pushed as early as tomorrow.

This is exactly how it was for 2.6.24: x86.git#mm is where we stage 
patches while they are being tested - as you probably are well aware of.

It does not mean they are .26 material (although some of them clearly 
are - such as kgdb-light) - most of the fix patches start out there, and 
once we trust them we percolate them upwards in the queue and then they 
eventually get submitted to Linus. If any bug happens to some of them 
then they bubble back to the tail of the queue. For most of the 
nontrivial patches, even if they are fixes we want to push upstream, it 
is usually at least several days of testing until we trust them.

	Ingo

Re: [PATCH] Fix direct mapping alias regressin in ioremap v2

[Andi Kleen]

> It does not mean they are .26 material (although some of them clearly 

Are the pageattr.c changes in #mm .25 or .26 material? 

-Andi