Re: [PATCH v7u1 20/31] x86, kexec: replace ident_mapping_init and init_level4_page
On Wed, Jan 09, 2013 at 05:26:18PM -0800, Yinghai Lu wrote: > I should say: > > that *is* initialized to false by default. > > please check > > http://stackoverflow.com/questions/10828294/c-and-c-partial-initialization-of-automatic-structure Ok, I didn't know that, thanks for pointing it out. And yet, this is not the point - the point is that this code is complicated enough as it is so why not make the easy things trivial so that people looking at it months or even years from now can still try to understand it. So what it is defined by the standard?! Just add that line anyway! Then there's no need to go check what was meant. This way it is *there*, *explicit* and everyone *knows* what is meant - even people who don't sleep with C99std under their pillow. It is not like we're saving code since the mov $0 gets issued by the compiler anyway when it is on the stack: movq$0, -48(%rbp) #, info movq$0, -40(%rbp) #, info movq$0, -32(%rbp) #, info movq$0, -24(%rbp) #, info Thanks. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7u1 20/31] x86, kexec: replace ident_mapping_init and init_level4_page
On Wed, Jan 09, 2013 at 05:26:18PM -0800, Yinghai Lu wrote: I should say: that *is* initialized to false by default. please check http://stackoverflow.com/questions/10828294/c-and-c-partial-initialization-of-automatic-structure Ok, I didn't know that, thanks for pointing it out. And yet, this is not the point - the point is that this code is complicated enough as it is so why not make the easy things trivial so that people looking at it months or even years from now can still try to understand it. So what it is defined by the standard?! Just add that line anyway! Then there's no need to go check what was meant. This way it is *there*, *explicit* and everyone *knows* what is meant - even people who don't sleep with C99std under their pillow. It is not like we're saving code since the mov $0 gets issued by the compiler anyway when it is on the stack: movq$0, -48(%rbp) #, info movq$0, -40(%rbp) #, info movq$0, -32(%rbp) #, info movq$0, -24(%rbp) #, info Thanks. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7u1 20/31] x86, kexec: replace ident_mapping_init and init_level4_page
On Sat, Jan 5, 2013 at 5:24 AM, Borislav Petkov wrote:
> On Fri, Jan 04, 2013 at 02:04:05PM -0800, Yinghai Lu wrote:
>> On Fri, Jan 4, 2013 at 1:01 PM, Borislav Petkov wrote:
>> > On Thu, Jan 03, 2013 at 04:48:40PM -0800, Yinghai Lu wrote:
>> >> static int init_pgtable(struct kimage *image, unsigned long
>> >> start_pgtable)
>> >> {
>> >> + struct x86_mapping_info info = {
>> >> + .alloc_pgt_page = alloc_pgt_page,
>> >> + .context= image,
>> >> + .pmd_flag = __PAGE_KERNEL_LARGE_EXEC,
>> >> + };
>> >
>> > This is leaving ->kernel_mapping uninitialized to contain a random,
>> > previous stack value. I don't think we want that.
>>
>> that should be initialized to false by default.
>
> So make it explicit. You can't possibly rely on what the stack contains
> when you allocate that struct there.
I should say:
that *is* initialized to false by default.
please check
http://stackoverflow.com/questions/10828294/c-and-c-partial-initialization-of-automatic-structure
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7u1 20/31] x86, kexec: replace ident_mapping_init and init_level4_page
On Sat, Jan 5, 2013 at 5:24 AM, Borislav Petkov b...@alien8.de wrote:
On Fri, Jan 04, 2013 at 02:04:05PM -0800, Yinghai Lu wrote:
On Fri, Jan 4, 2013 at 1:01 PM, Borislav Petkov b...@alien8.de wrote:
On Thu, Jan 03, 2013 at 04:48:40PM -0800, Yinghai Lu wrote:
static int init_pgtable(struct kimage *image, unsigned long
start_pgtable)
{
+ struct x86_mapping_info info = {
+ .alloc_pgt_page = alloc_pgt_page,
+ .context= image,
+ .pmd_flag = __PAGE_KERNEL_LARGE_EXEC,
+ };
This is leaving -kernel_mapping uninitialized to contain a random,
previous stack value. I don't think we want that.
that should be initialized to false by default.
So make it explicit. You can't possibly rely on what the stack contains
when you allocate that struct there.
I should say:
that *is* initialized to false by default.
please check
http://stackoverflow.com/questions/10828294/c-and-c-partial-initialization-of-automatic-structure
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7u1 20/31] x86, kexec: replace ident_mapping_init and init_level4_page
On Fri, Jan 04, 2013 at 02:04:05PM -0800, Yinghai Lu wrote:
> On Fri, Jan 4, 2013 at 1:01 PM, Borislav Petkov wrote:
> > On Thu, Jan 03, 2013 at 04:48:40PM -0800, Yinghai Lu wrote:
> >> static int init_pgtable(struct kimage *image, unsigned long start_pgtable)
> >> {
> >> + struct x86_mapping_info info = {
> >> + .alloc_pgt_page = alloc_pgt_page,
> >> + .context= image,
> >> + .pmd_flag = __PAGE_KERNEL_LARGE_EXEC,
> >> + };
> >
> > This is leaving ->kernel_mapping uninitialized to contain a random,
> > previous stack value. I don't think we want that.
>
> that should be initialized to false by default.
So make it explicit. You can't possibly rely on what the stack contains
when you allocate that struct there.
--
Regards/Gruss,
Boris.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7u1 20/31] x86, kexec: replace ident_mapping_init and init_level4_page
On Fri, Jan 04, 2013 at 02:04:05PM -0800, Yinghai Lu wrote:
On Fri, Jan 4, 2013 at 1:01 PM, Borislav Petkov b...@alien8.de wrote:
On Thu, Jan 03, 2013 at 04:48:40PM -0800, Yinghai Lu wrote:
static int init_pgtable(struct kimage *image, unsigned long start_pgtable)
{
+ struct x86_mapping_info info = {
+ .alloc_pgt_page = alloc_pgt_page,
+ .context= image,
+ .pmd_flag = __PAGE_KERNEL_LARGE_EXEC,
+ };
This is leaving -kernel_mapping uninitialized to contain a random,
previous stack value. I don't think we want that.
that should be initialized to false by default.
So make it explicit. You can't possibly rely on what the stack contains
when you allocate that struct there.
--
Regards/Gruss,
Boris.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7u1 20/31] x86, kexec: replace ident_mapping_init and init_level4_page
On Fri, Jan 4, 2013 at 1:01 PM, Borislav Petkov wrote:
> On Thu, Jan 03, 2013 at 04:48:40PM -0800, Yinghai Lu wrote:
>> static int init_pgtable(struct kimage *image, unsigned long start_pgtable)
>> {
>> + struct x86_mapping_info info = {
>> + .alloc_pgt_page = alloc_pgt_page,
>> + .context= image,
>> + .pmd_flag = __PAGE_KERNEL_LARGE_EXEC,
>> + };
>
> This is leaving ->kernel_mapping uninitialized to contain a random,
> previous stack value. I don't think we want that.
that should be initialized to false by default.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7u1 20/31] x86, kexec: replace ident_mapping_init and init_level4_page
On Thu, Jan 03, 2013 at 04:48:40PM -0800, Yinghai Lu wrote:
> static int init_pgtable(struct kimage *image, unsigned long start_pgtable)
> {
> + struct x86_mapping_info info = {
> + .alloc_pgt_page = alloc_pgt_page,
> + .context= image,
> + .pmd_flag = __PAGE_KERNEL_LARGE_EXEC,
> + };
This is leaving ->kernel_mapping uninitialized to contain a random,
previous stack value. I don't think we want that.
> unsigned long mstart, mend;
> pgd_t *level4p;
> int result;
> int i;
>
> level4p = (pgd_t *)__va(start_pgtable);
> - result = init_level4_page(image, level4p, 0, max_pfn << PAGE_SHIFT);
> + clear_page(level4p);
> + result = kernel_ident_mapping_init(, level4p,
> + 0, max_pfn << PAGE_SHIFT);
> if (result)
> return result;
>
> @@ -225,7 +115,8 @@ static int init_pgtable(struct kimage *image, unsigned
> long start_pgtable)
> mstart = image->segment[i].mem;
> mend = mstart + image->segment[i].memsz;
>
> - result = ident_mapping_init(image, level4p, mstart, mend);
> + result = kernel_ident_mapping_init(,
> + level4p, mstart, mend);
>
> if (result)
> return result;
> --
> 1.7.10.4
>
>
--
Regards/Gruss,
Boris.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7u1 20/31] x86, kexec: replace ident_mapping_init and init_level4_page
On Thu, Jan 03, 2013 at 04:48:40PM -0800, Yinghai Lu wrote:
static int init_pgtable(struct kimage *image, unsigned long start_pgtable)
{
+ struct x86_mapping_info info = {
+ .alloc_pgt_page = alloc_pgt_page,
+ .context= image,
+ .pmd_flag = __PAGE_KERNEL_LARGE_EXEC,
+ };
This is leaving -kernel_mapping uninitialized to contain a random,
previous stack value. I don't think we want that.
unsigned long mstart, mend;
pgd_t *level4p;
int result;
int i;
level4p = (pgd_t *)__va(start_pgtable);
- result = init_level4_page(image, level4p, 0, max_pfn PAGE_SHIFT);
+ clear_page(level4p);
+ result = kernel_ident_mapping_init(info, level4p,
+ 0, max_pfn PAGE_SHIFT);
if (result)
return result;
@@ -225,7 +115,8 @@ static int init_pgtable(struct kimage *image, unsigned
long start_pgtable)
mstart = image-segment[i].mem;
mend = mstart + image-segment[i].memsz;
- result = ident_mapping_init(image, level4p, mstart, mend);
+ result = kernel_ident_mapping_init(info,
+ level4p, mstart, mend);
if (result)
return result;
--
1.7.10.4
--
Regards/Gruss,
Boris.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7u1 20/31] x86, kexec: replace ident_mapping_init and init_level4_page
On Fri, Jan 4, 2013 at 1:01 PM, Borislav Petkov b...@alien8.de wrote:
On Thu, Jan 03, 2013 at 04:48:40PM -0800, Yinghai Lu wrote:
static int init_pgtable(struct kimage *image, unsigned long start_pgtable)
{
+ struct x86_mapping_info info = {
+ .alloc_pgt_page = alloc_pgt_page,
+ .context= image,
+ .pmd_flag = __PAGE_KERNEL_LARGE_EXEC,
+ };
This is leaving -kernel_mapping uninitialized to contain a random,
previous stack value. I don't think we want that.
that should be initialized to false by default.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v7u1 20/31] x86, kexec: replace ident_mapping_init and init_level4_page
Now ident_mapping_init is checking if pgd/pud is present for every 2M,
so several 2Ms are in same PUD, it will keep checking if pud is there.
init_level4_page does not check existing pgd/pud.
We could use generic mapping_init to replace them all.
Signed-off-by: Yinghai Lu
---
arch/x86/kernel/machine_kexec_64.c | 161 ++--
1 file changed, 26 insertions(+), 135 deletions(-)
diff --git a/arch/x86/kernel/machine_kexec_64.c
b/arch/x86/kernel/machine_kexec_64.c
index be14ee1..d2d7e02 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -16,144 +16,12 @@
#include
#include
+#include
#include
#include
#include
#include
-static int init_one_level2_page(struct kimage *image, pgd_t *pgd,
- unsigned long addr)
-{
- pud_t *pud;
- pmd_t *pmd;
- struct page *page;
- int result = -ENOMEM;
-
- addr &= PMD_MASK;
- pgd += pgd_index(addr);
- if (!pgd_present(*pgd)) {
- page = kimage_alloc_control_pages(image, 0);
- if (!page)
- goto out;
- pud = (pud_t *)page_address(page);
- clear_page(pud);
- set_pgd(pgd, __pgd(__pa(pud) | _KERNPG_TABLE));
- }
- pud = pud_offset(pgd, addr);
- if (!pud_present(*pud)) {
- page = kimage_alloc_control_pages(image, 0);
- if (!page)
- goto out;
- pmd = (pmd_t *)page_address(page);
- clear_page(pmd);
- set_pud(pud, __pud(__pa(pmd) | _KERNPG_TABLE));
- }
- pmd = pmd_offset(pud, addr);
- if (!pmd_present(*pmd))
- set_pmd(pmd, __pmd(addr | __PAGE_KERNEL_LARGE_EXEC));
- result = 0;
-out:
- return result;
-}
-
-static int ident_mapping_init(struct kimage *image, pgd_t *level4p,
- unsigned long mstart, unsigned long mend)
-{
- int result;
-
- mstart = round_down(mstart, PMD_SIZE);
- mend = round_up(mend - 1, PMD_SIZE);
-
- while (mstart < mend) {
- result = init_one_level2_page(image, level4p, mstart);
- if (result)
- return result;
-
- mstart += PMD_SIZE;
- }
-
- return 0;
-}
-
-static void init_level2_page(pmd_t *level2p, unsigned long addr)
-{
- unsigned long end_addr;
-
- addr &= PAGE_MASK;
- end_addr = addr + PUD_SIZE;
- while (addr < end_addr) {
- set_pmd(level2p++, __pmd(addr | __PAGE_KERNEL_LARGE_EXEC));
- addr += PMD_SIZE;
- }
-}
-
-static int init_level3_page(struct kimage *image, pud_t *level3p,
- unsigned long addr, unsigned long last_addr)
-{
- unsigned long end_addr;
- int result;
-
- result = 0;
- addr &= PAGE_MASK;
- end_addr = addr + PGDIR_SIZE;
- while ((addr < last_addr) && (addr < end_addr)) {
- struct page *page;
- pmd_t *level2p;
-
- page = kimage_alloc_control_pages(image, 0);
- if (!page) {
- result = -ENOMEM;
- goto out;
- }
- level2p = (pmd_t *)page_address(page);
- init_level2_page(level2p, addr);
- set_pud(level3p++, __pud(__pa(level2p) | _KERNPG_TABLE));
- addr += PUD_SIZE;
- }
- /* clear the unused entries */
- while (addr < end_addr) {
- pud_clear(level3p++);
- addr += PUD_SIZE;
- }
-out:
- return result;
-}
-
-
-static int init_level4_page(struct kimage *image, pgd_t *level4p,
- unsigned long addr, unsigned long last_addr)
-{
- unsigned long end_addr;
- int result;
-
- result = 0;
- addr &= PAGE_MASK;
- end_addr = addr + (PTRS_PER_PGD * PGDIR_SIZE);
- while ((addr < last_addr) && (addr < end_addr)) {
- struct page *page;
- pud_t *level3p;
-
- page = kimage_alloc_control_pages(image, 0);
- if (!page) {
- result = -ENOMEM;
- goto out;
- }
- level3p = (pud_t *)page_address(page);
- result = init_level3_page(image, level3p, addr, last_addr);
- if (result)
- goto out;
- set_pgd(level4p++, __pgd(__pa(level3p) | _KERNPG_TABLE));
- addr += PGDIR_SIZE;
- }
- /* clear the unused entries */
- while (addr < end_addr) {
- pgd_clear(level4p++);
- addr += PGDIR_SIZE;
- }
-out:
- return result;
-}
-
static void free_transition_pgtable(struct kimage *image)
{
free_page((unsigned long)image->arch.pud);
@@ -203,15 +71,37 @@ err:
return result;
}
+static void
[PATCH v7u1 20/31] x86, kexec: replace ident_mapping_init and init_level4_page
Now ident_mapping_init is checking if pgd/pud is present for every 2M,
so several 2Ms are in same PUD, it will keep checking if pud is there.
init_level4_page does not check existing pgd/pud.
We could use generic mapping_init to replace them all.
Signed-off-by: Yinghai Lu ying...@kernel.org
---
arch/x86/kernel/machine_kexec_64.c | 161 ++--
1 file changed, 26 insertions(+), 135 deletions(-)
diff --git a/arch/x86/kernel/machine_kexec_64.c
b/arch/x86/kernel/machine_kexec_64.c
index be14ee1..d2d7e02 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -16,144 +16,12 @@
#include linux/io.h
#include linux/suspend.h
+#include asm/init.h
#include asm/pgtable.h
#include asm/tlbflush.h
#include asm/mmu_context.h
#include asm/debugreg.h
-static int init_one_level2_page(struct kimage *image, pgd_t *pgd,
- unsigned long addr)
-{
- pud_t *pud;
- pmd_t *pmd;
- struct page *page;
- int result = -ENOMEM;
-
- addr = PMD_MASK;
- pgd += pgd_index(addr);
- if (!pgd_present(*pgd)) {
- page = kimage_alloc_control_pages(image, 0);
- if (!page)
- goto out;
- pud = (pud_t *)page_address(page);
- clear_page(pud);
- set_pgd(pgd, __pgd(__pa(pud) | _KERNPG_TABLE));
- }
- pud = pud_offset(pgd, addr);
- if (!pud_present(*pud)) {
- page = kimage_alloc_control_pages(image, 0);
- if (!page)
- goto out;
- pmd = (pmd_t *)page_address(page);
- clear_page(pmd);
- set_pud(pud, __pud(__pa(pmd) | _KERNPG_TABLE));
- }
- pmd = pmd_offset(pud, addr);
- if (!pmd_present(*pmd))
- set_pmd(pmd, __pmd(addr | __PAGE_KERNEL_LARGE_EXEC));
- result = 0;
-out:
- return result;
-}
-
-static int ident_mapping_init(struct kimage *image, pgd_t *level4p,
- unsigned long mstart, unsigned long mend)
-{
- int result;
-
- mstart = round_down(mstart, PMD_SIZE);
- mend = round_up(mend - 1, PMD_SIZE);
-
- while (mstart mend) {
- result = init_one_level2_page(image, level4p, mstart);
- if (result)
- return result;
-
- mstart += PMD_SIZE;
- }
-
- return 0;
-}
-
-static void init_level2_page(pmd_t *level2p, unsigned long addr)
-{
- unsigned long end_addr;
-
- addr = PAGE_MASK;
- end_addr = addr + PUD_SIZE;
- while (addr end_addr) {
- set_pmd(level2p++, __pmd(addr | __PAGE_KERNEL_LARGE_EXEC));
- addr += PMD_SIZE;
- }
-}
-
-static int init_level3_page(struct kimage *image, pud_t *level3p,
- unsigned long addr, unsigned long last_addr)
-{
- unsigned long end_addr;
- int result;
-
- result = 0;
- addr = PAGE_MASK;
- end_addr = addr + PGDIR_SIZE;
- while ((addr last_addr) (addr end_addr)) {
- struct page *page;
- pmd_t *level2p;
-
- page = kimage_alloc_control_pages(image, 0);
- if (!page) {
- result = -ENOMEM;
- goto out;
- }
- level2p = (pmd_t *)page_address(page);
- init_level2_page(level2p, addr);
- set_pud(level3p++, __pud(__pa(level2p) | _KERNPG_TABLE));
- addr += PUD_SIZE;
- }
- /* clear the unused entries */
- while (addr end_addr) {
- pud_clear(level3p++);
- addr += PUD_SIZE;
- }
-out:
- return result;
-}
-
-
-static int init_level4_page(struct kimage *image, pgd_t *level4p,
- unsigned long addr, unsigned long last_addr)
-{
- unsigned long end_addr;
- int result;
-
- result = 0;
- addr = PAGE_MASK;
- end_addr = addr + (PTRS_PER_PGD * PGDIR_SIZE);
- while ((addr last_addr) (addr end_addr)) {
- struct page *page;
- pud_t *level3p;
-
- page = kimage_alloc_control_pages(image, 0);
- if (!page) {
- result = -ENOMEM;
- goto out;
- }
- level3p = (pud_t *)page_address(page);
- result = init_level3_page(image, level3p, addr, last_addr);
- if (result)
- goto out;
- set_pgd(level4p++, __pgd(__pa(level3p) | _KERNPG_TABLE));
- addr += PGDIR_SIZE;
- }
- /* clear the unused entries */
- while (addr end_addr) {
- pgd_clear(level4p++);
- addr += PGDIR_SIZE;
- }
-out:
- return result;
-}
-
static void free_transition_pgtable(struct kimage *image)
{
free_page((unsigned
