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

Jeremy Fitzhardinge
2008-01-09T01:03:55Z

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