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

[Jeremy Fitzhardinge <jeremy@goop.org>]

Ingo Molnar wrote:
> * Ingo Molnar <mingo@elte.hu> 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