Chris Wright wrote:

>* Zachary Amsden (zach@vmware.com) wrote:
>  
>
>>Chris Wright wrote:
>>
>>    
>>
>>>* Zachary Amsden (zach@vmware.com) wrote:
>>>
>>>
>>>      
>>>
>>>>--- linux-2.6.13.orig/arch/i386/mm/init.c	2005-08-24 
>>>>09:31:05.000000000 -0700
>>>>+++ linux-2.6.13/arch/i386/mm/init.c	2005-08-24 09:31:31.000000000 -0700
>>>>@@ -79,6 +79,7 @@ static pte_t * __init one_page_table_ini
>>>>{
>>>>	if (pmd_none(*pmd)) {
>>>>		pte_t *page_table = (pte_t *) 
>>>>		alloc_bootmem_low_pages(PAGE_SIZE);
>>>>+		SetPagePTE(virt_to_page(page_table));
>>>>  
>>>>
>>>>        
>>>>
>>>Xen has this on one_md_table_init() as well for pmd.
>>>      
>>>
>>I'll add that in another patch.  It's easy to miss some of the init time 
>>call sites (we don't actually depend on them for correctness).
>>    
>>
>
>You could just respin this patch.
>  
>

Done.  Looks like you want empty_zero_page write protected too.  That 
seems like a fine idea to me unless something really wants to do atomic 
64-bit reads on it.

>>>>        
>>>>
>>>Why the switch of kmem_cache_free call?
>>>      
>>>
>>Because pgd_val(pgd[i])-1 is confusing.
>>    
>>
>
>Yes, I agree it is.  That's why I was wondering about the ClearPagePDE calls
>which don't use them.
>  
>

The -1 is quite useless when you're going to shift  >> 12 anyways to get 
a frame number to index into mem_map, which is why they are not there.  
Plus, it just seems scary if you got it wrong - let's say you had a not 
present page - not that you could, but now you are freeing a misaligned 
address in the _previous_ page.  I really don't like that -1 at all.  I 
will clean it up, but it does certainly deserve another patch.

>  
>
>>Using (pgd_val(pgd[i]) - 
>>_PAGE_PRESENT) would be better, but the +/- 1s all over the place here 
>>could use some general cleanup as well.  I smell a cleanup fit coming 
>>on.  Using (pgd_val(pgd[i]) & PAGE_MASK) is a less error prone way to 
>>get the physical frame bits, since it is not wrong if you turn on PCD or 
>>PWD.
>>    
>>
>
>Please save all that for later cleanup.  As it stands it's just a conversion
>of one random call site, which should be dropped from the patch.
>  
>

Working on it now.