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

[Jeremy Fitzhardinge]

Ingo Molnar wrote:
> * Ingo Molnar <mingo@xxxxxxx> wrote:
>
>   
> > > > > >  #define __PAGE_KERNEL_EXEC						\
> > > > > > -	(_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_GLOBAL)
> > > > > > +	(_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED)
> > > > > >     
> > > > > >             
> > > > This shouldn't be necessary.  The old 64-bit code defined everything 
> > > > without _PAGE_GLOBAL, but then used a MAKE_GLOBAL() macro to OR it 
> > > > in later.  This seemed a bit roundabout to me, so I just put it in 
> > > > from the outset.
> > > >         
> > actually, this is wrong.
> >
> > a couple of places use __PAGE_* values, which you've now changed to 
> > include the _PAGE_GLOBAL flag.
> >     
>
> yep, fixing this resolves the crash.
>   

Bugger.  OK.

And I don't see quite how the global flag is causing the BUG bug in 
change_page_attr().  The logic is:

	if (pgprot_val(prot) != pgprot_val(ref_prot)) {
		...
	} else {
		if (!pte_huge(*kpte)) {
			...
		} else
			BUG();
	}

Is _PAGE_GLOBAL causing the first if() to fall through to the second 
clause?  Because otherwise it shouldn't have any effect on the 
pte_huge() test.

But given that ref_prot is set to PAGE_KERNEL or PAGE_KERNEL_EXEC, which 
will have _PAGE_GLOBAL in it either way, I don't see where the problem 
is coming from.

Gah!  This can't be right!  I think the original change_page_attr() code 
is plain buggy.

The crash call chain is:

 [<ffffffff8021db68>] change_page_attr_addr+0x9e/0x119
 [<ffffffff8021d44f>] ioremap_change_attr+0x49/0x58
 [<ffffffff8021d626>] iounmap+0xbe/0xe0
...


ioremap_change_attr does:

		err = change_page_attr_addr(vaddr,npages,__pgprot(__PAGE_KERNEL|flags));

Now, in the current code (ie, before my patch), __PAGE_KERNEL doesn't 
have _PAGE_GLOBAL set, but PAGE_KERNEL does.  Therefore, 
change_page_attr_addr calls

	__change_page_attr(address, pfn, prot, PAGE_KERNEL);

which means:

	__change_page_attr(address, pfn, pgprot(__PAGE_KERNEL), PAGE_KERNEL);

(iounmap always passes flags of 0) which just happens to fail the test:

	if (pgprot_val(prot) != pgprot_val(ref_prot)) {


because prot doesn't contain _PAGE_GLOBAL and ref_prot does.

In other words, prot and ref_prot can never be equal, so this path is 
always taken, and the other branch which tests pte_huge() is never run.

Andi?  Jan?  Is this code just buggy, or is there something else going 
on here?

   J
--
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/