4/5 in support of hot-add memory x86_64 fix kernel mapping code
PATCH 1/1 4/5 in support of hot-add memory x86_64 fix kernel mapping code
Hello All,
phys_pud_init is broken when using it at runtime with some offsets.
It currently only maps one pud entry worth of pages while trampling any mappings that may have existed on the pmd_page :(
This his patch make it safe to map addresses that are not pmd_page aligned and generally more robust.
This is a critical patch for hot-add and x86_64.
Thanks to Dave Hansen for input on look and feel of this patch and it's constructs.
This was built against 2.6.18-rc2.
arch/x86_64/mm/init.c +26 -22
@@ -252,9 +252,11 @@static void __meminit phys_pmd_init (pmd_t *pmd,unsigned long address,unsigned long end) { -int i; +int i =pmd_index (address); -for (i =0 ; i <PTRS_PER_PMD ; pmd++, i++, address +=PMD_SIZE ) { + pmd = pmd + i; + +for (; i <PTRS_PER_PMD ; pmd++, i++, address +=PMD_SIZE ) {unsigned long entry;if (address >= end) {
@@ -263,6 +265,11 @@set_pmd (pmd,__pmd (0 ));break ; } + +if (pmd_val (*pmd)) { +printk (KERN_ERR "%s trying to trample pte entry \ +%08lx@%08lx\n" ,__func__,pmd_val (*pmd),address); + } entry = _PAGE_NX|_PAGE_PSE|_KERNPG_TABLE|_PAGE_GLOBAL|address; entry &= __supported_pte_mask;set_pmd (pmd,__pmd (entry));
@@ -272,45 +279,42 @@static void __meminit phys_pmd_update (pud_t *pud,unsigned long address,unsigned long end) { -pmd_t *pmd =pmd_offset (pud, (unsigned long )__va (address)); - -if (pmd_none (*pmd)) { -spin_lock (&init_mm.page_table_lock); -phys_pmd_init (pmd, address, end); -spin_unlock (&init_mm.page_table_lock); -__flush_tlb_all (); - } +pmd_t *pmd =pmd_offset (pud,0 ); +spin_lock (&init_mm.page_table_lock); +phys_pmd_init (pmd, address, end); +spin_unlock (&init_mm.page_table_lock); +__flush_tlb_all (); }static void __meminit phys_pud_init (pud_t *pud,unsigned long address,unsigned long end) { -long i =pud_index (address); +int i =pud_index (address); pud = pud + i; -if (after_bootmem &&pud_val (*pud)) { -phys_pmd_update (pud, address, end); -return ; - } - -for (; i <PTRS_PER_PUD ; pud++, i++) { +for (; i <PTRS_PER_PUD ; pud++, i++, \ + address = (address &PUD_MASK ) +PUD_SIZE ) {int map; -unsigned long paddr, pmd_phys; +unsigned long pmd_phys;pmd_t *pmd; - paddr = (address &PGDIR_MASK ) + i*PUD_SIZE ; -if (paddr >= end) +if (address >= end)break ; -if (!after_bootmem && !e820_any_mapped (paddr, paddr+PUD_SIZE ,0 )) { +if (!after_bootmem && !e820_any_mapped (address, address+PUD_SIZE ,0 )) {set_pud (pud,__pud (0 ));continue ; } +if (pud_val (*pud)) { +phys_pmd_update (pud, address, end); +continue ; + } + pmd =alloc_low_page (&map, &pmd_phys);spin_lock (&init_mm.page_table_lock);set_pud (pud,__pud (pmd_phys | _KERNPG_TABLE)); -phys_pmd_init (pmd, paddr, end); +phys_pmd_init (pmd, address, end);spin_unlock (&init_mm.page_table_lock);unmap_low_page (map); }
Quoted 5 lines · keith mannthey
Hello All,
phys_pud_initis broken when using it at runtime with some offsets.
It currently only maps one pud entry worth of pages while trampling any mappings that may have existed on thepmd_page:(
To print x86-64 ptes you need a %016lx (or just %lx)
it would be cleaner to recompute pmd inside the loop based on i and use a standard for()
It is unclear why you hardcode 0 as address in phys_pmd_update when a real address is passed in?
-Andi
Quoted 8 lines · Andi Kleen
Quoted 5 lines · keith mannthey
Hello All,
phys_pud_initis broken when using it at runtime with some offsets.
It currently only maps one pud entry worth of pages while trampling any mappings that may have existed on thepmd_page:(To print x86-64 ptes you need a %016lx (or just %lx)
Thanks.
Quoted 2 lines
it would be cleaner to recompute pmd inside the loop based on i and use a standard
for()
sure I can do away with the pmd++ pud++ in the for loops.
Quoted 3 lines
It is unclear why you hardcode 0 as address in
phys_pmd_updatewhen a real address is passed in
When the pud is already set there a 2 options.
1. You need to initialize pmds at the start of the pmd_page.
2. You need to initialize pmds starting at some offset of the pmd_page.
When calling phys_pmd_init you need to pass it the start of the pmd_page not some random pmd with in the page.
pmd = alloc_low_page(&map, &pmd_phys);
always gives us the start of the pmd_page. I keep this idea for the update path as well.pmd_offset(pud,0) does just this. Maybe there is a better macro to use?
Things could be different is terms of flexibility of what you pass in (more changes are involved) but this set of changes seemed to be min- tampering of the current code.
Also in general is there some reason kernel mapping code is arch specific? This all seems to be pretty generic.
Thanks,
Keith