Re: [RFT/PATCH] x86: Unify kernel_physical_mapping_init() API

[Pekka Enberg]

Hi Hiroyuki-san,

KAMEZAWA Hiroyuki kirjoitti:
> On Wed, 24 Feb 2010 17:04:47 +0200 (EET)
> Pekka J Enberg <penberg@xxxxxxxxxxxxxx> wrote:
>
> > From: Pekka Enberg <penberg@xxxxxxxxxxxxxx>
> >
> > This patch changes the 32-bit version of kernel_physical_mapping_init() to
> > return the last mapped address like the 64-bit one so that we can unify the
> > call-site in init_memory_mapping().
>
> I'm sorry if I can't track the logic correctly..

OK. I tried to make it match what x86-64 does.

> > Cc: Yinghai Lu <yinghai@xxxxxxxxxx>
> > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> > Signed-off-by: Pekka Enberg <penberg@xxxxxxxxxxxxxx>
> > ---
> > Note: I have only tested this on VirtualBox which is why I tagged the patch as
> > RFT.
> >
> >   arch/x86/mm/init.c    |    7 -------
> >   arch/x86/mm/init_32.c |    8 +++++---
> >   2 files changed, 5 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> > index d406c52..e71c5cb 100644
> > --- a/arch/x86/mm/init.c
> > +++ b/arch/x86/mm/init.c
> > @@ -266,16 +266,9 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
> >   	if (!after_bootmem)
> >   		find_early_table_space(end, use_pse, use_gbpages);
> >
> > -#ifdef CONFIG_X86_32
> > -	for (i = 0; i < nr_range; i++)
> > -		kernel_physical_mapping_init(mr[i].start, mr[i].end,
> > -					     mr[i].page_size_mask);
> > -	ret = end;
> > -#else /* CONFIG_X86_64 */
> >   	for (i = 0; i < nr_range; i++)
> >   		ret = kernel_physical_mapping_init(mr[i].start, mr[i].end,
> >   						   mr[i].page_size_mask);
> > -#endif
> >
> >   #ifdef CONFIG_X86_32
> >   	early_ioremap_page_table_range_init();
> > diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
> > index 9a0c258..2226f2c 100644
> > --- a/arch/x86/mm/init_32.c
> > +++ b/arch/x86/mm/init_32.c
> > @@ -241,6 +241,7 @@ kernel_physical_mapping_init(unsigned long start,
> >   			     unsigned long page_size_mask)
> >   {
> >   	int use_pse = page_size_mask == (1<<PG_LEVEL_2M);
> > +	unsigned long last_map_addr = end;
> >   	unsigned long start_pfn, end_pfn;
> >   	pgd_t *pgd_base = swapper_pg_dir;
> >   	int pgd_idx, pmd_idx, pte_ofs;
> > @@ -341,9 +342,10 @@ repeat:
> >   					prot = PAGE_KERNEL_EXEC;
> >
> >   				pages_4k++;
> > -				if (mapping_iter == 1)
> > +				if (mapping_iter == 1) {
> >   					set_pte(pte, pfn_pte(pfn, init_prot));
> > -				else
> > +					last_map_addr = (pfn << PAGE_SHIFT) + PAGE_SIZE;
> > +				} else
> >   					set_pte(pte, pfn_pte(pfn, prot));
> >   			}
> >   		}
>
> Don't we need to update last_map_addr after pages_2m++; ?

Yeah, I missed that part. It probably works fine because we return "end" 
for 2M pages. However, to fix that, I'm not completely sure what 
"last_map_addr" would be. I am thinking

  last_map_addr = (addr & PMD_MASK) + PMD_SIZE;

might do the trick but I'm not that familiar with the code. Ideas?

> And...from the logic,
>   unsigned long last_map_addr = end;
> seems to be always true.
>
> So, just returning "end" is not good ?

Might be but like I said, I tried to make it match x86-64 so that we 
could eventually try to unify those bits as well.

			Pekka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/