Messages in this thread | | | Date | Sat, 28 Jun 2008 10:38:04 -0700 | From | Jeremy Fitzhardinge <> | Subject | Re: [PATCH] x86: fix init_memory_mapping over boundary v2 |
| |
Yinghai Lu wrote: > some end boundary is only page alignment, instead of 2M alignment, > so call ker_phycial_mapping_init three times. > then don't overmap above the max_low_pfn > > v2: make init_memory_mapping more solid: start could be any value other than 0 > > Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com> > > --- > arch/x86/mm/init_32.c | 86 ++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 62 insertions(+), 24 deletions(-) > > Index: linux-2.6/arch/x86/mm/init_32.c > =================================================================== > --- linux-2.6.orig/arch/x86/mm/init_32.c > +++ linux-2.6/arch/x86/mm/init_32.c > @@ -184,8 +184,9 @@ static inline int is_kernel_text(unsigne > * PAGE_OFFSET: > */ > static void __init kernel_physical_mapping_init(pgd_t *pgd_base, > - unsigned long start, > - unsigned long end) > + unsigned long start_pfn, > + unsigned long end_pfn, > + int use_pse) >
Better to use bool for use_pse.
> { > int pgd_idx, pmd_idx, pte_ofs; > unsigned long pfn; > @@ -193,32 +194,30 @@ static void __init kernel_physical_mappi > pmd_t *pmd; > pte_t *pte; > unsigned pages_2m = 0, pages_4k = 0; > - unsigned limit_pfn = end >> PAGE_SHIFT; > > - pgd_idx = pgd_index(PAGE_OFFSET); > - pgd = pgd_base + pgd_idx; > - pfn = start >> PAGE_SHIFT; > + if (!cpu_has_pse) > + use_pse = 0; >
What are the semantics of the use_pse argument? Is it just a hint? If so, then the caller below should just pass a constant 1/true.
> > + pfn = start_pfn; > + pgd_idx = pgd_index((pfn<<PAGE_SHIFT) + PAGE_OFFSET); > + pgd = pgd_base + pgd_idx; > for (; pgd_idx < PTRS_PER_PGD; pgd++, pgd_idx++) { > pmd = one_md_table_init(pgd); > - if (pfn >= limit_pfn) > + > + if (pfn >= end_pfn) > continue; > > - for (pmd_idx = 0; > - pmd_idx < PTRS_PER_PMD && pfn < limit_pfn; > + pmd_idx = pmd_index((pfn<<PAGE_SHIFT) + PAGE_OFFSET); > + pmd += pmd_idx; > + for (; pmd_idx < PTRS_PER_PMD && pfn < end_pfn; > pmd++, pmd_idx++) { > unsigned int addr = pfn * PAGE_SIZE + PAGE_OFFSET; > > /* > * Map with big pages if possible, otherwise > * create normal page tables: > - * > - * Don't use a large page for the first 2/4MB of memory > - * because there are often fixed size MTRRs in there > - * and overlapping MTRRs into large pages can cause > - * slowdowns. > */ > - if (cpu_has_pse && !(pgd_idx == 0 && pmd_idx == 0)) { > + if (use_pse) { > unsigned int addr2; > pgprot_t prot = PAGE_KERNEL_LARGE; > > @@ -233,13 +232,12 @@ static void __init kernel_physical_mappi > set_pmd(pmd, pfn_pmd(pfn, prot)); > > pfn += PTRS_PER_PTE; > - max_pfn_mapped = pfn; > continue; > } > pte = one_page_table_init(pmd); > > for (pte_ofs = 0; > - pte_ofs < PTRS_PER_PTE && pfn < max_low_pfn; > + pte_ofs < PTRS_PER_PTE && pfn < end_pfn; > pte++, pfn++, pte_ofs++, addr += PAGE_SIZE) { > pgprot_t prot = PAGE_KERNEL; > > @@ -249,7 +247,6 @@ static void __init kernel_physical_mappi > pages_4k++; > set_pte(pte, pfn_pte(pfn, prot)); > } > - max_pfn_mapped = pfn; > } > } > update_page_count(PG_LEVEL_2M, pages_2m); > @@ -729,7 +726,7 @@ void __init setup_bootmem_allocator(void > > static void __init find_early_table_space(unsigned long end) > { > - unsigned long puds, pmds, tables, start; > + unsigned long puds, pmds, ptes, tables, start; > > puds = (end + PUD_SIZE - 1) >> PUD_SHIFT; > tables = PAGE_ALIGN(puds * sizeof(pud_t)); > @@ -737,10 +734,15 @@ static void __init find_early_table_spac > pmds = (end + PMD_SIZE - 1) >> PMD_SHIFT; > tables += PAGE_ALIGN(pmds * sizeof(pmd_t)); > > - if (!cpu_has_pse) { > - int ptes = (end + PAGE_SIZE - 1) >> PAGE_SHIFT; > - tables += PAGE_ALIGN(ptes * sizeof(pte_t)); > - } > + if (cpu_has_pse) { > + int extra; > + extra = end - ((end>>21) << 21); >
That's a bit magic. Isn't that just "extra = end & ((1 << 21) - 1)"? And what are you actually computing here anyway?
> + extra += (2<<20); >
PMD_SIZE? The comment below says "2/4MB", which corresponds to a PAE/non-PAE large PMD.
> + ptes = (extra + PAGE_SIZE - 1) >> PAGE_SHIFT; > + } else > + ptes = (end + PAGE_SIZE - 1) >> PAGE_SHIFT; > + > + tables += PAGE_ALIGN(ptes * sizeof(pte_t)); > > /* > * RED-PEN putting page tables only on node 0 could > @@ -766,6 +768,8 @@ unsigned long __init_refok init_memory_m > unsigned long end) > { > pgd_t *pgd_base = swapper_pg_dir; > + unsigned long start_pfn, end_pfn; > + unsigned long big_page_start; > > /* > * Find space for the kernel direct mapping tables. > @@ -790,7 +794,41 @@ unsigned long __init_refok init_memory_m > __PAGE_KERNEL_EXEC |= _PAGE_GLOBAL; > } > > - kernel_physical_mapping_init(pgd_base, start, end); > + /* > + * Don't use a large page for the first 2/4MB of memory > + * because there are often fixed size MTRRs in there > + * and overlapping MTRRs into large pages can cause > + * slowdowns. > + */ > + big_page_start = 2UL<<20; >
PMD_SIZE again? Non-PAE big pages are 4MB.
> + if (start < big_page_start) { > + start_pfn = start >> PAGE_SHIFT; > + end_pfn = min(big_page_start>>PAGE_SHIFT, end>>PAGE_SHIFT); > + } else { > + /* head is not 2M alignment ? */ > + start_pfn = (start >> 21) << (21 - PAGE_SHIFT); > + end_pfn = start >> PAGE_SHIFT; > + } > + if (start_pfn < end_pfn) > + kernel_physical_mapping_init(pgd_base, start_pfn, end_pfn, 0); > + > + /* big page range */ > + start_pfn = ((start + ((2UL<<20) - 1)) >> 21) << (21 - PAGE_SHIFT); >
More magic.
> + if (start_pfn < (big_page_start >> PAGE_SHIFT)) > + start_pfn = big_page_start >> PAGE_SHIFT; > + end_pfn = (end >> 21) << (21 - PAGE_SHIFT); > + if (start_pfn < end_pfn) > + kernel_physical_mapping_init(pgd_base, start_pfn, end_pfn, > + cpu_has_pse); > + > + /* tail is not 2M alignment ? */ > + start_pfn = end_pfn; > + if (start_pfn > (big_page_start>>PAGE_SHIFT)) { > + end_pfn = end >> PAGE_SHIFT; > + if (start_pfn < end_pfn) > + kernel_physical_mapping_init(pgd_base, start_pfn, > + end_pfn, 0); > + }
I think this code could do with a dose of comments and some symbolic constants. And perhaps the tactical use of round_up/down.
But it's a nice simplification of kernel_physical_mapping_init().
J
| |