[PATCH] Fix direct mapping correctly in ioremap

[PATCH] Fix direct mapping correctly in ioremap

[Andi Kleen]

Fix direct mapping correctly in ioremap

[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.

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,7 +157,12 @@ static void __iomem *__ioremap(unsigned 
 		return NULL;
 	}
 
-	if (ioremap_change_attr(vaddr, size, mode) < 0) {
+	/* Fix up the direct mapping for the new cache attributes */
+	err = ioremap_change_attr((unsigned long)__va(phys_addr),
+				size + offset, mode);
+	if (err == -EINVAL) {
+		/* Original address was partly unmapped. Valid condition; ignore. */
+	} else if (err < 0) {
 		vunmap(area->addr);
 		return NULL;
 	}

Re: [PATCH] Fix direct mapping correctly in ioremap

[Ingo Molnar]

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

> -	if (ioremap_change_attr(vaddr, size, mode) < 0) {
> +	/* Fix up the direct mapping for the new cache attributes */
> +	err = ioremap_change_attr((unsigned long)__va(phys_addr),
> +				size + offset, mode);

Ugh. This would break the 32-bit kernel - if any phys_addr larger than 
1GB is passed in (which is the common case on 32-bit) then we'll start 
changing the attributes of random (most likely user-space) pages!

That is because on 32-bit __va() does this:

  #define __va(x)                  ((void *)((unsigned long)(x)+PAGE_OFFSET))

where on 32-bit 3GB:1GB split PAGE_OFFSET is defined to 0xc0000000.

So if a driver passes in the physical address of a PCI device with a BAR 
at 0xe1000000 somewhere in the PCI aperture, we'll get 
0xe1000000+0xc0000000 == 0x91000000 - right in the middle of user-space.

Changing attributes there is very wrong. (it could even crash the kernel 
in certain circumstances.)

Have you tried to boot this patch on 32-bit? There are a couple of new 
safety nets in the cpa code that would/should trigger very visibly - 
such as the warning here:

                if (!pte_val(old_pte)) {
                        printk(KERN_WARNING "CPA: called for zero pte. "
                               "vaddr = %lx cpa->vaddr = %lx\n", address,
                                cpa->vaddr);
                        WARN_ON(1);
                        return -EINVAL;
                }

(these are already there in -git)

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,

	Ingo

Re: [PATCH] Fix direct mapping correctly in ioremap

[Andi Kleen]

On Thu, Feb 14, 2008 at 05:03:23PM +0100, Ingo Molnar wrote:
> 
> * Andi Kleen <andi@firstfloor.org> wrote:
> 
> > -	if (ioremap_change_attr(vaddr, size, mode) < 0) {
> > +	/* Fix up the direct mapping for the new cache attributes */
> > +	err = ioremap_change_attr((unsigned long)__va(phys_addr),
> > +				size + offset, mode);
> 
> Ugh. This would break the 32-bit kernel - if any phys_addr larger than 
> 1GB is passed in (which is the common case on 32-bit) then we'll start 
> changing the attributes of random (most likely user-space) pages!
> That is because on 32-bit __va() does this:
> 
>   #define __va(x)                  ((void *)((unsigned long)(x)+PAGE_OFFSET))
> 
> where on 32-bit 3GB:1GB split PAGE_OFFSET is defined to 0xc0000000.
> 
> So if a driver passes in the physical address of a PCI device with a BAR 
> at 0xe1000000 somewhere in the PCI aperture, we'll get 
> 0xe1000000+0xc0000000 == 0x91000000 - right in the middle of user-space.
> 
> Changing attributes there is very wrong. (it could even crash the kernel 
> in certain circumstances.)

True. It needs a end_pfn_map check.  I'll revise.

> Have you tried to boot this patch on 32-bit? There are a couple of new 

Yes, it booted on several machines.  Don't think I saw CPA messages,
but i might have missed them.

> safety nets in the cpa code that would/should trigger very visibly - 
> such as the warning here:
> 
>                 if (!pte_val(old_pte)) {
>                         printk(KERN_WARNING "CPA: called for zero pte. "
>                                "vaddr = %lx cpa->vaddr = %lx\n", address,
>                                 cpa->vaddr);
>                         WARN_ON(1);
>                         return -EINVAL;
>                 }
> 
> (these are already there in -git)
> 
> 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 want this regression fixed in .25, so #mm is not an option for this.

-Andi