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

[Andi Kleen <ak@suse.de>]

On Tue, Jan 08, 2008 at 05:35:26PM -0800, Jeremy Fitzhardinge wrote:
> Andi Kleen wrote:
>>> of the if()?  If not, can't we just remove it and avoid this present 
>>> problem?
>>>     
>>
>> That would just hide your problem.
>>   
>
> The "problem" is a BUG() in pageattr_64.c:change_page_attr(), which to me 
> looks spurious.  It arises because __PAGE_KERNEL_* doesn't contain 
> _PAGE_GLOBAL, but PAGE_KERNEL_* does.  When ioremap() change_page_attr(), 
> it does so in a way that guarentees that the test
>
> 	if (pgprot_val(prot) != pgprot_val(ref_prot)) {
>
> in __change_page_attr() always succeeds.  When I folded _PAGE_GLOBAL into 
> the __PAGE_KERNEL_* definitions, it mostly works except it causes this if() 
> to fail, falling into the otherwise dead else clause and triggers a BUG().

That means your callers pass in the wrong values.

> I'm not really sure what the logic in here is supposed to be doing, but it 
> seems to me that it isn't the difference between global and non-global 
> mappings (I guess its really testing for cached vs uncached mappings).

No it is testing for all page attributes. That can be cached vs uncached
but also rw vs ro and nx vs non nx and everything else you can set
as bits on ptes.

While changing global sounds weird the pageattr-test code I recently
submitted does just that for test purposes.


> Or to put it another way, what's the underlying rationale for making 
> __PAGE_KERNEL_* not include the GLOBAL flag, but including it in the pgprot 
> versions?  It means that code like this in ioremap_64.c:

There is none, but that is not what change_page_attr() cares about.
It just cares that you pass in the correct bits and you don't currently.

I think you're missing the forrest because of all the trees currently.

-Andi