[PATCH] x86, mm: add generic kernel/ident mapping helper

[PATCH] x86, mm: add generic kernel/ident mapping helper

[Borislav Petkov]

> commit bb3cda32f9586e13c5d3dde6c83a914bb2d7f87f
> Author: Yinghai Lu <yinghai@kernel.org>
> Date:   Mon Dec 24 18:00:21 2012 -0800
> 
>     x86, mm: add generic kernel/ident mapping helper
>     
>     It is simple version for kernel_physical_mapping_init.
>     it will work to build one page table that will be put effectiv later.
>     
>     Use mapping_info to control
>             1. alloc method
>             2. if PMD is EXEC,
>             3. if pgd is with kernel low mapping or ident mapping.
>     
>     Will use to replace some local versions in kexec, hibernation and etc.
>     
>     Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> diff --git a/arch/x86/include/asm/init.h b/arch/x86/include/asm/init.h
> index bac770b7cbc4..66a368190139 100644
> --- a/arch/x86/include/asm/init.h
> +++ b/arch/x86/include/asm/init.h
> @@ -1,5 +1,17 @@
>  #ifndef _ASM_X86_INIT_H
>  #define _ASM_X86_INIT_H
>  
> +struct mapping_info {
> +	void *(*alloc)(void *);

		alloc_page

> +	void *data;
> +	unsigned long flag;

		page_flags;

> +	bool kernel;

	kernel_space?

In general, all those members could use more meaningful names and some
commenting explaining what they are, instead of people having to deduce
what they mean from their usage in the code.

Also, struct name 'mapping_info' is too generic. Maybe
ident_mapping_info?

> +};
> +
> +int kernel_ident_mapping_init(struct mapping_info *info, pgd_t *pgd_page,
> +				unsigned long addr, unsigned long end);
> +
> +int kernel_mapping_init(pgd_t *pgd_page,
> +				unsigned long addr, unsigned long end);
>  
>  #endif /* _ASM_X86_INIT_H */
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index b1178eb7af22..6e1c063c49d2 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -56,6 +56,99 @@
>  
>  #include "mm_internal.h"
>  
> +static void ident_pmd_init(unsigned long flag, pmd_t *pmd_page,
> +			  unsigned long addr, unsigned long end)
> +{
> +	addr &= PMD_MASK;
> +	for (; addr < end; addr += PMD_SIZE) {
> +		pmd_t *pmd = pmd_page + pmd_index(addr);
> +
> +		if (!pmd_present(*pmd))
> +			set_pmd(pmd, __pmd(addr | flag));
> +	}
> +}
> +static int ident_pud_init(struct mapping_info *info,
> +			  pud_t *pud_page,
> +			  unsigned long addr, unsigned long end)
> +{
> +	unsigned long next;
> +
> +	for (; addr < end; addr = next) {
> +		pud_t *pud = pud_page + pud_index(addr);
> +		pmd_t *pmd;
> +
> +		next = (addr & PUD_MASK) + PUD_SIZE;
> +		if (next > end)
> +			next = end;
> +
> +		if (pud_present(*pud)) {
> +			pmd = pmd_offset(pud, 0);
> +			ident_pmd_init(info->flag, pmd, addr, next);
> +			continue;
> +		}
> +		pmd = (pmd_t *)info->alloc(info->data);
> +		if (!pmd)
> +			return -ENOMEM;
> +		ident_pmd_init(info->flag, pmd, addr, next);
> +		set_pud(pud, __pud(__pa(pmd) | _KERNPG_TABLE));
> +	}
> +
> +	return 0;
> +}
> +
> +int kernel_ident_mapping_init(struct mapping_info *info,
> +		       pgd_t *pgd_page, unsigned long addr, unsigned long end)

that's some funny argument alignment.

> +{
> +	unsigned long next;
> +	int result;
> +	int off = info->kernel ? pgd_index(__PAGE_OFFSET) : 0;
> +
> +	for (; addr < end; addr = next) {
> +		pgd_t *pgd = pgd_page + pgd_index(addr) + off;
> +		pud_t *pud;
> +
> +		next = (addr & PGDIR_MASK) + PGDIR_SIZE;
> +		if (next > end)
> +			next = end;
> +
> +		if (pgd_present(*pgd)) {
> +			pud = pud_offset(pgd, 0);
> +			result = ident_pud_init(info, pud, addr, next);
> +			if (result)
> +				return result;
> +			continue;
> +		}
> +
> +		pud = (pud_t *)info->alloc(info->data);
> +		if (!pud)
> +			return -ENOMEM;
> +		result = ident_pud_init(info, pud, addr, next);
> +		if (result)
> +			return result;
> +		set_pgd(pgd, __pgd(__pa(pud) | _KERNPG_TABLE));
> +	}
> +
> +	return 0;
> +}
> +
> +static void *alloc_pgt_page(void *data)
> +{
> +	return alloc_low_page();
> +}
> +
> +int kernel_mapping_init(pgd_t *pgd_page,
> +				unsigned long addr, unsigned long end)

ditto.

> +{
> +	struct mapping_info info = {
> +		.alloc = alloc_pgt_page,
> +		.data = NULL,
> +		.flag = __PAGE_KERNEL_LARGE,
> +		.kernel = true,
> +	};
> +
> +	return kernel_ident_mapping_init(&info, pgd_page, addr, end);
> +}
> +
>  static int __init parse_direct_gbpages_off(char *arg)
>  {
>  	direct_gbpages = 0;

-- 
Regards/Gruss,
Boris.

Re: [PATCH] x86, mm: add generic kernel/ident mapping helper

[Borislav Petkov]

On Thu, Dec 27, 2012 at 07:51:26PM +0100, Borislav Petkov wrote:
> > commit bb3cda32f9586e13c5d3dde6c83a914bb2d7f87f
> > Author: Yinghai Lu <yinghai@kernel.org>
> > Date:   Mon Dec 24 18:00:21 2012 -0800
> > 
> >     x86, mm: add generic kernel/ident mapping helper
> >     
> >     It is simple version for kernel_physical_mapping_init.
> >     it will work to build one page table that will be put effectiv later.

Forgot to ask: what does "put effectiv later" mean?

* will be effectively put to use later

* will effectively be put, i.e. dropped later

* we will switch to using it later

* ...

Thanks.

-- 
Regards/Gruss,
Boris.

Re: [PATCH] x86, mm: add generic kernel/ident mapping helper

[Yinghai Lu]

On Thu, Dec 27, 2012 at 11:03 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Thu, Dec 27, 2012 at 07:51:26PM +0100, Borislav Petkov wrote:
>> > commit bb3cda32f9586e13c5d3dde6c83a914bb2d7f87f
>> > Author: Yinghai Lu <yinghai@kernel.org>
>> > Date:   Mon Dec 24 18:00:21 2012 -0800
>> >
>> >     x86, mm: add generic kernel/ident mapping helper
>> >
>> >     It is simple version for kernel_physical_mapping_init.
>> >     it will work to build one page table that will be put effectiv later.
>
> Forgot to ask: what does "put effectiv later" mean?

 * we will switch to using it later

Re: [PATCH] x86, mm: add generic kernel/ident mapping helper

[Yinghai Lu]

On Thu, Dec 27, 2012 at 10:51 AM, Borislav Petkov <bp@alien8.de> wrote:
>> +struct mapping_info {
>> +     void *(*alloc)(void *);
>
>                 alloc_page

alloc_page make me feel that it will return struct page *.

>
>> +     void *data;
>> +     unsigned long flag;
>
>                 page_flags;

will change to pmd_flags

>
>> +     bool kernel;
>
>         kernel_space?

that is used to tell: if it is kernel mapping or ident mapping.

will change to is_kernel_mapping or kernel_mapping instead

>
> In general, all those members could use more meaningful names and some
> commenting explaining what they are, instead of people having to deduce
> what they mean from their usage in the code.
>
> Also, struct name 'mapping_info' is too generic. Maybe
> ident_mapping_info?

do you like to name it with kernel_ident_mapping_info ?

looks too long.

Re: [PATCH] x86, mm: add generic kernel/ident mapping helper

[Yinghai Lu]

On Thu, Dec 27, 2012 at 4:34 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Dec 27, 2012 at 10:51 AM, Borislav Petkov <bp@alien8.de> wrote:
>>> +struct mapping_info {
>>> +     void *(*alloc)(void *);
>>
>>                 alloc_page
>
> alloc_page make me feel that it will return struct page *.

also will get error

arch/x86/mm/init_64.c: In function ‘ident_pud_init’:
arch/x86/mm/init_64.c:88:22: error: ‘struct mapping_info’ has no
member named ‘alloc_pages’
arch/x86/mm/init_64.c: In function ‘kernel_ident_mapping_init’:
arch/x86/mm/init_64.c:121:22: error: ‘struct mapping_info’ has no
member named ‘alloc_pages’

because include/linux/gfp.h has

#define alloc_page(gfp_mask) alloc_pages(gfp_mask, 0)

so will stay with alloc.

Yinghai

Re: [PATCH] x86, mm: add generic kernel/ident mapping helper

[Borislav Petkov]

On Fri, Dec 28, 2012 at 12:13:06AM -0800, Yinghai Lu wrote:
> so will stay with alloc.

"alloc" is too generic. So call it 'get_one_pgt_page' or
'alloc_pgt_page' or something descriptive to say what it does than
simply 'alloc'. 'alloc' can be anything - it could mean allocation hints
or flags or whatever you feel like at that particular time of day :-).

Thanks.

-- 
Regards/Gruss,
Boris.

Re: [PATCH] x86, mm: add generic kernel/ident mapping helper

[Borislav Petkov]

On Thu, Dec 27, 2012 at 04:34:03PM -0800, Yinghai Lu wrote:
> do you like to name it with kernel_ident_mapping_info ?

We have already kernel_ident_mapping_init so the first three words are
already the same. Besides, you're differentiating between kernel mapping
or ident mapping so ident_mapping_info is not that suitable too. Maybe
x86_mapping_info since it is x86-specific?

Thanks.

-- 
Regards/Gruss,
Boris.

Re: [PATCH] x86, mm: add generic kernel/ident mapping helper

[Yinghai Lu]

On Fri, Dec 28, 2012 at 3:39 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Dec 28, 2012 at 12:13:06AM -0800, Yinghai Lu wrote:
>> so will stay with alloc.
>
> "alloc" is too generic. So call it 'get_one_pgt_page' or
> 'alloc_pgt_page' or something descriptive to say what it does than
> simply 'alloc'. 'alloc' can be anything - it could mean allocation hints
> or flags or whatever you feel like at that particular time of day :-).

alloc_pgt_page should be ok.

Re: [PATCH] x86, mm: add generic kernel/ident mapping helper

[Yinghai Lu]

On Fri, Dec 28, 2012 at 3:47 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Thu, Dec 27, 2012 at 04:34:03PM -0800, Yinghai Lu wrote:
>> do you like to name it with kernel_ident_mapping_info ?
>
> We have already kernel_ident_mapping_init so the first three words are
> already the same. Besides, you're differentiating between kernel mapping
> or ident mapping so ident_mapping_info is not that suitable too. Maybe
> x86_mapping_info since it is x86-specific?

ok, will use x86_mapping_info.

Thanks a lot.

Yinghai