Skip to contentSkip to conversation

x86, mm: add generic kernel/ident mapping helper

PATCH In review Borislav Petkov 2 participants 9 messages Dec 27, 2012 – Dec 28, 2012

lore
Share & tools
lore

Borislav Petkov

author
#0 lore raw

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

Quoted 23 lines

commit bb3cda32f9586e13c5d3dde6c83a914bb2d7f87f

Author: Yinghai Lu <[email protected]>
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 <[email protected]>
arch/x86/include/asm/init.h +0 -0
index bac770b7cbc4..66a368190139 100644

arch/x86/include/asm/init.h line 5

Quoted 5 lines
arch/x86/include/asm/init.h +2 -0
@@ -1,5 +1,17 @@
 #ifndef _ASM_X86_INIT_H
 #define _ASM_X86_INIT_H
 
+struct mapping_info {
+	void *(*alloc)(void *);

alloc_page

arch/x86/include/asm/init.h line 7

Quoted 2 lines
arch/x86/include/asm/init.h +2 -0
+	void *data;
+	unsigned long flag;

		page_flags;

arch/x86/include/asm/init.h line 8

Quoted 1 line
arch/x86/include/asm/init.h +1 -0
+	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?

Quoted 49 lines
arch/x86/include/asm/init.h +7 -0
+};
+
+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 */
arch/x86/mm/init_64.c +32 -0
index b1178eb7af22..6e1c063c49d2 100644
@@ -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)

arch/x86/mm/init_64.c line 100

Quoted 10 lines
arch/x86/mm/init_64.c +10 -0
+			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.

Quoted 30 lines
arch/x86/mm/init_64.c +30 -0
+{
+	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));
+	}
+

arch/x86/mm/init_64.c line 140

Quoted 10 lines
arch/x86/mm/init_64.c +10 -0
+	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.

Quoted 14 lines
arch/x86/mm/init_64.c +11 -0
+{
+	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;
Signature · 2 lines
Regards/Gruss,
Boris.
author
#1 lore raw
Quoted 8 lines · Borislav Petkov
Quoted 8 lines

commit bb3cda32f9586e13c5d3dde6c83a914bb2d7f87f

Author: Yinghai Lu <[email protected]>
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.

Signature · 2 lines
Regards/Gruss,
Boris.
#2 lore raw
Quoted 11 lines · Borislav Petkov
Quoted 8 lines · Borislav Petkov
Quoted 8 lines

commit bb3cda32f9586e13c5d3dde6c83a914bb2d7f87f

Author: Yinghai Lu <[email protected]>
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

#3 lore raw
In reply to Borislav Petkov
Quoted 4 lines · Borislav Petkov

Review

Quoted 2 lines
(quoted diff) +2 -0
+struct mapping_info {
+     void *(*alloc)(void *);

alloc_page

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

Quoted 5 lines

Review

Quoted 3 lines
(quoted diff) +2 -0
+     void *data;
+     unsigned long flag;

                page_flags;

will change to pmd_flags

Quoted 4 lines
Quoted 2 lines

+ 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

Quoted 7 lines

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.

#4 lore raw
Quoted 7 lines · Yinghai Lu
Quoted 4 lines · Borislav Petkov

Review

Quoted 2 lines
(quoted diff) +2 -0
+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

author
#5 lore raw
Quoted 1 line · Yinghai Lu

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.

Signature · 2 lines
Regards/Gruss,
Boris.
#6 lore raw
Quoted 7 lines · Borislav Petkov
Quoted 1 line · Yinghai Lu

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.

author
#7 lore raw
In reply to Yinghai Lu
Quoted 1 line · Yinghai Lu

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.

Signature · 2 lines
Regards/Gruss,
Boris.
#8 lore raw
Quoted 7 lines · Borislav Petkov
Quoted 1 line · Yinghai Lu

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