Re: [PATCH 00 of 10] x86: unify asm/pgtable.h

[Andi Kleen]

> Yeah, that may be true, but this particular tree is weird, and I'm trying 
> to understand what's going on here.  Specifically, 64-bit ioremap()s 
> *don't* set _PAGE_GLOBAL, which appears to be an accident resulting from 
> the strange definitions of __PAGE_KERNEL_* vs PAGE_KERNEL_*. 

ioremap() should set G agreed.

> For example, ioremap_64.c:__ioremap() creates a vma for the io mapping, and 
> explicitly sets _PAGE_GLOBAL in the vma's version of pgprot - but then it 
> calls ioremap_page_range() to actually create the mapping, which ends up 
> making a non-global mapping, because its rolling its own version of 
> PAGE_KERNEL by using pgprot(__PAGE_KERNEL) - which is not the actual 
> definition of PAGE_KERNEL.

That should not really matter because ioremap_change_attr()->c_p_a is only called
when flags is != 0 and that means it is already different from PAGE_KERNEL.

>
> I think there's a bug around here, but I think its currently being hidden 

There's one Jan pointed out: iounmap does not subtract the guard page size
so it ends up resetting one page too much. That is probably what causes your
problem. But again you should be passing in G in the first place.

-Andi

Here was Jan's patch; it incidently fixes the G problem too

snip

Additionally I found it necessary to fix ioremap_64.c's use of
change_page_attr_addr():

--- a/arch/x86/mm/ioremap_64.c
+++ b/arch/x86/mm/ioremap_64.c
@@ -48,7 +48,7 @@ ioremap_change_attr(unsigned long phys_a
  		 * Must use a address here and not struct page because the phys addr
 		 * can be a in hole between nodes and not have an memmap entry.
 		 */
-		err = change_page_attr_addr(vaddr,npages,__pgprot(__PAGE_KERNEL|flags));
+		err = change_page_attr_addr(vaddr,npages,MAKE_GLOBAL(__PAGE_KERNEL|flags));
 		if (!err)
 			global_flush_tlb();
 	}
@@ -199,7 +199,7 @@ void iounmap(volatile void __iomem *addr
 
 	/* Reset the direct mapping. Can block */
 	if (p->flags >> 20)
-		ioremap_change_attr(p->phys_addr, p->size, 0);
+		ioremap_change_attr(p->phys_addr, get_vm_area_size(p), 0);
 
 	/* Finally remove it */
 	o = remove_vm_area((void *)addr);

Other extra changes I had in my version could possibly be counted as enhancements...

Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/