[PATCH 1/2] x86 boot: add E820_RESVD_KERN

[PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Huang, Ying]

This patch add a new type to E820 table named E820_RESVD_KERN, which
is used for memory area reserved for kernel itself instead of BIOS.

This patch is based on latest x86/master branch of git-x86 tree and
has been tested on i386 and x86_64 platform.

Signed-off-by: Huang Ying <ying.huang@intel.com>

---
 arch/x86/kernel/e820.c |    6 ++++--
 include/asm-x86/e820.h |    3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

--- a/include/asm-x86/e820.h
+++ b/include/asm-x86/e820.h
@@ -44,6 +44,9 @@
 #define E820_ACPI	3
 #define E820_NVS	4
 
+/* reserved RAM used by kernel itself */
+#define E820_RESVD_KERN	128
+
 #ifndef __ASSEMBLY__
 struct e820entry {
 	__u64 addr;	/* start of memory segment */
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -120,6 +120,7 @@ void __init e820_print_map(char *who)
 		       (e820.map[i].addr + e820.map[i].size));
 		switch (e820.map[i].type) {
 		case E820_RAM:
+		case E820_RESVD_KERN:
 			printk(KERN_CONT "(usable)\n");
 			break;
 		case E820_RESERVED:
@@ -611,7 +612,7 @@ void __init e820_mark_nosave_regions(uns
 			register_nosave_region(pfn, PFN_UP(ei->addr));
 
 		pfn = PFN_DOWN(ei->addr + ei->size);
-		if (ei->type != E820_RAM)
+		if (ei->type != E820_RAM && ei->type != E820_RESVD_KERN)
 			register_nosave_region(PFN_UP(ei->addr), pfn);
 
 		if (pfn >= limit_pfn)
@@ -977,7 +978,7 @@ u64 __init early_reserve_e820(u64 startt
 		return 0;
 
 	addr = round_down(start + size - sizet, align);
-	e820_update_range(addr, sizet, E820_RAM, E820_RESERVED);
+	e820_update_range(addr, sizet, E820_RAM, E820_RESVD_KERN);
 	printk(KERN_INFO "update e820 for early_reserve_e820\n");
 	update_e820();
 
@@ -1191,6 +1192,7 @@ void __init e820_reserve_resources(void)
 	res = alloc_bootmem_low(sizeof(struct resource) * e820.nr_map);
 	for (i = 0; i < e820.nr_map; i++) {
 		switch (e820.map[i].type) {
+		case E820_RESVD_KERN:
 		case E820_RAM:	res->name = "System RAM"; break;
 		case E820_ACPI:	res->name = "ACPI Tables"; break;
 		case E820_NVS:	res->name = "ACPI Non-volatile Storage"; break;

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Yinghai Lu]

On Wed, Jun 25, 2008 at 11:32 PM, Huang, Ying <ying.huang@intel.com> wrote:
> This patch add a new type to E820 table named E820_RESVD_KERN, which
> is used for memory area reserved for kernel itself instead of BIOS.
>
> This patch is based on latest x86/master branch of git-x86 tree and
> has been tested on i386 and x86_64 platform.
>
> Signed-off-by: Huang Ying <ying.huang@intel.com>
>
> ---
>  arch/x86/kernel/e820.c |    6 ++++--
>  include/asm-x86/e820.h |    3 +++
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> --- a/include/asm-x86/e820.h
> +++ b/include/asm-x86/e820.h
> @@ -44,6 +44,9 @@
>  #define E820_ACPI      3
>  #define E820_NVS       4
>
> +/* reserved RAM used by kernel itself */
> +#define E820_RESVD_KERN        128
> +
>  #ifndef __ASSEMBLY__
>  struct e820entry {
>        __u64 addr;     /* start of memory segment */
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -120,6 +120,7 @@ void __init e820_print_map(char *who)
>                       (e820.map[i].addr + e820.map[i].size));
>                switch (e820.map[i].type) {
>                case E820_RAM:
> +               case E820_RESVD_KERN:
>                        printk(KERN_CONT "(usable)\n");
>                        break;
>                case E820_RESERVED:
> @@ -611,7 +612,7 @@ void __init e820_mark_nosave_regions(uns
>                        register_nosave_region(pfn, PFN_UP(ei->addr));
>
>                pfn = PFN_DOWN(ei->addr + ei->size);
> -               if (ei->type != E820_RAM)
> +               if (ei->type != E820_RAM && ei->type != E820_RESVD_KERN)
>                        register_nosave_region(PFN_UP(ei->addr), pfn);
>
>                if (pfn >= limit_pfn)
> @@ -977,7 +978,7 @@ u64 __init early_reserve_e820(u64 startt
>                return 0;
>
>        addr = round_down(start + size - sizet, align);
> -       e820_update_range(addr, sizet, E820_RAM, E820_RESERVED);
> +       e820_update_range(addr, sizet, E820_RAM, E820_RESVD_KERN);

this line is not needed.

>        printk(KERN_INFO "update e820 for early_reserve_e820\n");
>        update_e820();
>
> @@ -1191,6 +1192,7 @@ void __init e820_reserve_resources(void)
>        res = alloc_bootmem_low(sizeof(struct resource) * e820.nr_map);
>        for (i = 0; i < e820.nr_map; i++) {
>                switch (e820.map[i].type) {
> +               case E820_RESVD_KERN:
>                case E820_RAM:  res->name = "System RAM"; break;
>                case E820_ACPI: res->name = "ACPI Tables"; break;
>                case E820_NVS:  res->name = "ACPI Non-volatile Storage"; break;
>
> --

please  move reserve_setup_data() from head.c/head64.c to setup32/64.c
or merged setup.c
also need to change reserve_early in reserve_setup_data to
e820_update_range(,,E820_RAM, E820_RESEVED_EXTRA).
calling reserve_setup_data() should around early_reserve_e820_mpc_new.

we don't need early_res_to_e820.

YH

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Huang, Ying]

On Thu, 2008-06-26 at 00:25 -0700, Yinghai Lu wrote:
[...]
> >                if (pfn >= limit_pfn)
> > @@ -977,7 +978,7 @@ u64 __init early_reserve_e820(u64 startt
> >                return 0;
> >
> >        addr = round_down(start + size - sizet, align);
> > -       e820_update_range(addr, sizet, E820_RAM, E820_RESERVED);
> > +       e820_update_range(addr, sizet, E820_RAM, E820_RESVD_KERN);
> 
> this line is not needed.

Why? Memory reserved by early_rserved_e820 should not be saved during
hibernation? shoudl not be saved by kdump?

> >        printk(KERN_INFO "update e820 for early_reserve_e820\n");
> >        update_e820();
> >
> > @@ -1191,6 +1192,7 @@ void __init e820_reserve_resources(void)
> >        res = alloc_bootmem_low(sizeof(struct resource) * e820.nr_map);
> >        for (i = 0; i < e820.nr_map; i++) {
> >                switch (e820.map[i].type) {
> > +               case E820_RESVD_KERN:
> >                case E820_RAM:  res->name = "System RAM"; break;
> >                case E820_ACPI: res->name = "ACPI Tables"; break;
> >                case E820_NVS:  res->name = "ACPI Non-volatile Storage"; break;
> >
> > --
> 
> please  move reserve_setup_data() from head.c/head64.c to setup32/64.c
> or merged setup.c

OK.

> also need to change reserve_early in reserve_setup_data to
> e820_update_range(,,E820_RAM, E820_RESEVED_EXTRA).

Does this means reserve_early() should be replaced by
e820_update_range()?

> calling reserve_setup_data() should around early_reserve_e820_mpc_new.

I think reserve_xxx should be called before first find_e820_area() to
minimize the possibility of conflict.

> we don't need early_res_to_e820.

Best Regards,
Huang Ying

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Yinghai Lu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 1978 bytes --]

On Thu, Jun 26, 2008 at 12:48 AM, Huang, Ying <ying.huang@intel.com> wrote:> On Thu, 2008-06-26 at 00:25 -0700, Yinghai Lu wrote:> [...]>> >                if (pfn >= limit_pfn)>> > @@ -977,7 +978,7 @@ u64 __init early_reserve_e820(u64 startt>> >                return 0;>> >>> >        addr = round_down(start + size - sizet, align);>> > -       e820_update_range(addr, sizet, E820_RAM, E820_RESERVED);>> > +       e820_update_range(addr, sizet, E820_RAM, E820_RESVD_KERN);>>>> this line is not needed.>> Why? Memory reserved by early_rserved_e820 should not be saved during> hibernation? shoudl not be saved by kdump?>>> >        printk(KERN_INFO "update e820 for early_reserve_e820\n");>> >        update_e820();>> >>> > @@ -1191,6 +1192,7 @@ void __init e820_reserve_resources(void)>> >        res = alloc_bootmem_low(sizeof(struct resource) * e820.nr_map);>> >        for (i = 0; i < e820.nr_map; i++) {>> >                switch (e820.map[i].type) {>> > +               case E820_RESVD_KERN:>> >                case E820_RAM:  res->name = "System RAM"; break;>> >                case E820_ACPI: res->name = "ACPI Tables"; break;>> >                case E820_NVS:  res->name = "ACPI Non-volatile Storage"; break;>> >>> > -->>>> please  move reserve_setup_data() from head.c/head64.c to setup32/64.c>> or merged setup.c>> OK.>>> also need to change reserve_early in reserve_setup_data to>> e820_update_range(,,E820_RAM, E820_RESEVED_EXTRA).>> Does this means reserve_early() should be replaced by> e820_update_range()?
Yes. in your case.
>>> calling reserve_setup_data() should around early_reserve_e820_mpc_new.>> I think reserve_xxx should be called before first find_e820_area() to> minimize the possibility of conflict.You already call e820_update_range, that could prevent find_e820_areato get confused.
YHÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 2167 bytes --]

On Thu, Jun 26, 2008 at 2:47 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> On Thu, Jun 26, 2008 at 12:48 AM, Huang, Ying <ying.huang@intel.com> wrote:
>> On Thu, 2008-06-26 at 00:25 -0700, Yinghai Lu wrote:
>> [...]
>>> >                if (pfn >= limit_pfn)
>>> > @@ -977,7 +978,7 @@ u64 __init early_reserve_e820(u64 startt
>>> >                return 0;
>>> >
>>> >        addr = round_down(start + size - sizet, align);
>>> > -       e820_update_range(addr, sizet, E820_RAM, E820_RESERVED);
>>> > +       e820_update_range(addr, sizet, E820_RAM, E820_RESVD_KERN);
>>>
>>> this line is not needed.
>>
>> Why? Memory reserved by early_rserved_e820 should not be saved during
>> hibernation? shoudl not be saved by kdump?
>>
>>> >        printk(KERN_INFO "update e820 for early_reserve_e820\n");
>>> >        update_e820();
>>> >
>>> > @@ -1191,6 +1192,7 @@ void __init e820_reserve_resources(void)
>>> >        res = alloc_bootmem_low(sizeof(struct resource) * e820.nr_map);
>>> >        for (i = 0; i < e820.nr_map; i++) {
>>> >                switch (e820.map[i].type) {
>>> > +               case E820_RESVD_KERN:
>>> >                case E820_RAM:  res->name = "System RAM"; break;
>>> >                case E820_ACPI: res->name = "ACPI Tables"; break;
>>> >                case E820_NVS:  res->name = "ACPI Non-volatile Storage"; break;
>>> >
>>> > --
>>>
>>> please  move reserve_setup_data() from head.c/head64.c to setup32/64.c
>>> or merged setup.c
>>
>> OK.
>>
>>> also need to change reserve_early in reserve_setup_data to
>>> e820_update_range(,,E820_RAM, E820_RESEVED_EXTRA).
>>
>> Does this means reserve_early() should be replaced by
>> e820_update_range()?
>
> Yes. in your case.
>
>>
>>> calling reserve_setup_data() should around early_reserve_e820_mpc_new.
>>
>> I think reserve_xxx should be called before first find_e820_area() to
>> minimize the possibility of conflict.
> You already call e820_update_range, that could prevent find_e820_area
> to get confused.

some like the attach patch...

you still can merge parse_setup_data parse_e820_ext
also entries in parse_e820_ext is not initialized..., __copy_e820_map
will do nothing.

YH

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: reserve_setup_data.patch --]
[-- Type: text/x-patch; name=reserve_setup_data.patch, Size: 4372 bytes --]

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 7b7685b..b45255b 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -120,6 +120,7 @@ void __init e820_print_map(char *who)
 		       (e820.map[i].addr + e820.map[i].size));
 		switch (e820.map[i].type) {
 		case E820_RAM:
+		case E820_RESERVED_KERN:
 			printk(KERN_CONT "(usable)\n");
 			break;
 		case E820_RESERVED:
@@ -611,7 +612,7 @@ void __init e820_mark_nosave_regions(unsigned long limit_pfn)
 			register_nosave_region(pfn, PFN_UP(ei->addr));
 
 		pfn = PFN_DOWN(ei->addr + ei->size);
-		if (ei->type != E820_RAM)
+		if (ei->type != E820_RAM && ei->type != E820_RESERVED_KERN)
 			register_nosave_region(PFN_UP(ei->addr), pfn);
 
 		if (pfn >= limit_pfn)
@@ -1196,6 +1197,7 @@ void __init e820_reserve_resources(void)
 	res = alloc_bootmem_low(sizeof(struct resource) * e820.nr_map);
 	for (i = 0; i < e820.nr_map; i++) {
 		switch (e820.map[i].type) {
+		case E820_RESERVED_KERN:
 		case E820_RAM:	res->name = "System RAM"; break;
 		case E820_ACPI:	res->name = "ACPI Tables"; break;
 		case E820_NVS:	res->name = "ACPI Non-volatile Storage"; break;
diff --git a/arch/x86/kernel/head.c b/arch/x86/kernel/head.c
index a6816be..3e66bd3 100644
--- a/arch/x86/kernel/head.c
+++ b/arch/x86/kernel/head.c
@@ -53,21 +53,3 @@ void __init reserve_ebda_region(void)
 	/* reserve all memory between lowmem and the 1MB mark */
 	reserve_early_overlap_ok(lowmem, 0x100000, "BIOS reserved");
 }
-
-void __init reserve_setup_data(void)
-{
-	struct setup_data *data;
-	u64 pa_data;
-	char buf[32];
-
-	if (boot_params.hdr.version < 0x0209)
-		return;
-	pa_data = boot_params.hdr.setup_data;
-	while (pa_data) {
-		data = early_ioremap(pa_data, sizeof(*data));
-		sprintf(buf, "setup data %x", data->type);
-		reserve_early(pa_data, pa_data+sizeof(*data)+data->len, buf);
-		pa_data = data->next;
-		early_iounmap(data, sizeof(*data));
-	}
-}
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index c970929..2ff9bc6 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -123,7 +123,6 @@ void __init x86_64_start_kernel(char * real_mode_data)
 #endif
 
 	reserve_ebda_region();
-	reserve_setup_data();
 
 	/*
 	 * At this point everything still needed from the boot loader
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index bf528b2..e9ce633 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -389,14 +389,33 @@ static void __init parse_setup_data(void)
 		default:
 			break;
 		}
-#ifndef CONFIG_DEBUG_BOOT_PARAMS
-		free_early(pa_data, pa_data+sizeof(*data)+data->len);
-#endif
 		pa_data = data->next;
 		early_iounmap(data, PAGE_SIZE);
 	}
 }
 
+static void __init reserve_setup_data(void)
+{
+	struct setup_data *data;
+	u64 pa_data;
+	char buf[32];
+
+	if (boot_params.hdr.version < 0x0209)
+		return;
+	pa_data = boot_params.hdr.setup_data;
+	while (pa_data) {
+		data = early_ioremap(pa_data, sizeof(*data));
+		sprintf(buf, "setup data %x", data->type);
+		e820_update_range(pa_data, sizeof(*data)+data->len,
+			 E820_RAM, E820_RESERVED_KERN);
+		pa_data = data->next;
+		early_iounmap(data, sizeof(*data));
+	}
+	sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
+	printk(KERN_INFO "extended physical RAM map:\n");
+	e820_print_map("extended");
+}
+
 /*
  * --------- Crashkernel reservation ------------------------------
  */
@@ -640,6 +659,8 @@ void __init setup_arch(char **cmdline_p)
 	 */
 	max_pfn = e820_end_of_ram();
 
+	reserve_setup_data();
+
 	/* preallocate 4k for mptable mpc */
 	early_reserve_e820_mpc_new();
 	/* update e820 for memory not covered by WB MTRRs */
diff --git a/include/asm-x86/bootparam.h b/include/asm-x86/bootparam.h
index 6eeba3b..ae22bdf 100644
--- a/include/asm-x86/bootparam.h
+++ b/include/asm-x86/bootparam.h
@@ -108,6 +108,4 @@ struct boot_params {
 	__u8  _pad9[276];				/* 0xeec */
 } __attribute__((packed));
 
-void reserve_setup_data(void);
-
 #endif /* _ASM_BOOTPARAM_H */
diff --git a/include/asm-x86/e820.h b/include/asm-x86/e820.h
index f622685..45e904f 100644
--- a/include/asm-x86/e820.h
+++ b/include/asm-x86/e820.h
@@ -44,6 +44,9 @@
 #define E820_ACPI	3
 #define E820_NVS	4
 
+/* reserved RAM used by kernel itself */
+#define E820_RESERVED_KERN        128
+
 #ifndef __ASSEMBLY__
 struct e820entry {
 	__u64 addr;	/* start of memory segment */

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Huang, Ying]

On Thu, 2008-06-26 at 19:22 -0700, Yinghai Lu wrote:
> On Thu, Jun 26, 2008 at 2:47 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> > On Thu, Jun 26, 2008 at 12:48 AM, Huang, Ying <ying.huang@intel.com> wrote:
> >> On Thu, 2008-06-26 at 00:25 -0700, Yinghai Lu wrote:
> >> [...]
> >>> >                if (pfn >= limit_pfn)
> >>> > @@ -977,7 +978,7 @@ u64 __init early_reserve_e820(u64 startt
> >>> >                return 0;
> >>> >
> >>> >        addr = round_down(start + size - sizet, align);
> >>> > -       e820_update_range(addr, sizet, E820_RAM, E820_RESERVED);
> >>> > +       e820_update_range(addr, sizet, E820_RAM, E820_RESVD_KERN);
> >>>
> >>> this line is not needed.
> >>
> >> Why? Memory reserved by early_rserved_e820 should not be saved during
> >> hibernation? shoudl not be saved by kdump?
> >>

Can you tell me why this line is not needed?

[...]
> some like the attach patch...
> 
> you still can merge parse_setup_data parse_e820_ext
> also entries in parse_e820_ext is not initialized..., __copy_e820_map
> will do nothing.

OK. Because some E820 entries are available after parse_setup_data(),
it is better to call reserve_setup_data() after calling
parse_setup_data() if update_e820_range() is used instead of
reserve_early().

Best Regards,
Huang Ying

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Yinghai Lu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 2504 bytes --]

On Thu, Jun 26, 2008 at 7:48 PM, Huang, Ying <ying.huang@intel.com> wrote:> On Thu, 2008-06-26 at 19:22 -0700, Yinghai Lu wrote:>> On Thu, Jun 26, 2008 at 2:47 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:>> > On Thu, Jun 26, 2008 at 12:48 AM, Huang, Ying <ying.huang@intel.com> wrote:>> >> On Thu, 2008-06-26 at 00:25 -0700, Yinghai Lu wrote:>> >> [...]>> >>> >                if (pfn >= limit_pfn)>> >>> > @@ -977,7 +978,7 @@ u64 __init early_reserve_e820(u64 startt>> >>> >                return 0;>> >>> >>> >>> >        addr = round_down(start + size - sizet, align);>> >>> > -       e820_update_range(addr, sizet, E820_RAM, E820_RESERVED);>> >>> > +       e820_update_range(addr, sizet, E820_RAM, E820_RESVD_KERN);>> >>>>> >>> this line is not needed.>> >>>> >> Why? Memory reserved by early_rserved_e820 should not be saved during>> >> hibernation? shoudl not be saved by kdump?>> >>>> Can you tell me why this line is not needed?
that function only have one user. the mpc reserve_new, so we want itto be E820_RESERVED, and kexec  tools could have that range reserved,the next kernel will not overwrite the updated mptable.
>> [...]>> some like the attach patch...>>>> you still can merge parse_setup_data parse_e820_ext>> also entries in parse_e820_ext is not initialized..., __copy_e820_map>> will do nothing.>> OK. Because some E820 entries are available after parse_setup_data(),> it is better to call reserve_setup_data() after calling> parse_setup_data() if update_e820_range() is used instead of> reserve_early().
can you double check parse_e820_ext()?it seems entries is not initialized properly,
void __init parse_e820_ext(struct setup_data *sdata, unsigned long pa_data){        u32 map_len;        int entries;        struct e820entry *extmap;
        entries = sdata->len / sizeof(struct e820entry);        map_len = sdata->len + sizeof(struct setup_data);        if (map_len > PAGE_SIZE)                sdata = early_ioremap(pa_data, map_len);        extmap = (struct e820entry *)(sdata->data);        __copy_e820_map(extmap, entries);===========> what is value for entries?        sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);        if (map_len > PAGE_SIZE)                early_iounmap(sdata, map_len);        printk(KERN_INFO "extended physical RAM map:\n");        e820_print_map("extended");}

YHÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Huang, Ying]

On Thu, 2008-06-26 at 19:52 -0700, Yinghai Lu wrote:
> On Thu, Jun 26, 2008 at 7:48 PM, Huang, Ying <ying.huang@intel.com> wrote:
> > On Thu, 2008-06-26 at 19:22 -0700, Yinghai Lu wrote:
> >> On Thu, Jun 26, 2008 at 2:47 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> >> > On Thu, Jun 26, 2008 at 12:48 AM, Huang, Ying <ying.huang@intel.com> wrote:
> >> >> On Thu, 2008-06-26 at 00:25 -0700, Yinghai Lu wrote:
> >> >> [...]
> >> >>> >                if (pfn >= limit_pfn)
> >> >>> > @@ -977,7 +978,7 @@ u64 __init early_reserve_e820(u64 startt
> >> >>> >                return 0;
> >> >>> >
> >> >>> >        addr = round_down(start + size - sizet, align);
> >> >>> > -       e820_update_range(addr, sizet, E820_RAM, E820_RESERVED);
> >> >>> > +       e820_update_range(addr, sizet, E820_RAM, E820_RESVD_KERN);
> >> >>>
> >> >>> this line is not needed.
> >> >>
> >> >> Why? Memory reserved by early_rserved_e820 should not be saved during
> >> >> hibernation? shoudl not be saved by kdump?
> >> >>
> >
> > Can you tell me why this line is not needed?
> 
> that function only have one user. the mpc reserve_new, so we want it
> to be E820_RESERVED, and kexec  tools could have that range reserved,
> the next kernel will not overwrite the updated mptable.

This is OK for kexec. Does it need to be saved by kdump? Does it need to
be saved by hibernation (suspend to disk)?

> >
> > [...]
> >> some like the attach patch...
> >>
> >> you still can merge parse_setup_data parse_e820_ext
> >> also entries in parse_e820_ext is not initialized..., __copy_e820_map
> >> will do nothing.
> >
> > OK. Because some E820 entries are available after parse_setup_data(),
> > it is better to call reserve_setup_data() after calling
> > parse_setup_data() if update_e820_range() is used instead of
> > reserve_early().
> 
> can you double check parse_e820_ext()?
> it seems entries is not initialized properly,
> 
> void __init parse_e820_ext(struct setup_data *sdata, unsigned long pa_data)
> {
>         u32 map_len;
>         int entries;
>         struct e820entry *extmap;
> 
>         entries = sdata->len / sizeof(struct e820entry);
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
          here entries are initialized to be the number of entries in
          extended E820 table.
>         map_len = sdata->len + sizeof(struct setup_data);
>         if (map_len > PAGE_SIZE)
>                 sdata = early_ioremap(pa_data, map_len);
>         extmap = (struct e820entry *)(sdata->data);
>         __copy_e820_map(extmap, entries);===========> what is value for entries?
>         sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
>         if (map_len > PAGE_SIZE)
>                 early_iounmap(sdata, map_len);
>         printk(KERN_INFO "extended physical RAM map:\n");
>         e820_print_map("extended");
> }

Best Regards,
Huang Ying

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Yinghai Lu]

On Thu, Jun 26, 2008 at 8:03 PM, Huang, Ying <ying.huang@intel.com> wrote:
> On Thu, 2008-06-26 at 19:52 -0700, Yinghai Lu wrote:
>> On Thu, Jun 26, 2008 at 7:48 PM, Huang, Ying <ying.huang@intel.com> wrote:
>> > On Thu, 2008-06-26 at 19:22 -0700, Yinghai Lu wrote:
>> >> On Thu, Jun 26, 2008 at 2:47 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>> >> > On Thu, Jun 26, 2008 at 12:48 AM, Huang, Ying <ying.huang@intel.com> wrote:
>> >> >> On Thu, 2008-06-26 at 00:25 -0700, Yinghai Lu wrote:
>> >> >> [...]
>> >> >>> >                if (pfn >= limit_pfn)
>> >> >>> > @@ -977,7 +978,7 @@ u64 __init early_reserve_e820(u64 startt
>> >> >>> >                return 0;
>> >> >>> >
>> >> >>> >        addr = round_down(start + size - sizet, align);
>> >> >>> > -       e820_update_range(addr, sizet, E820_RAM, E820_RESERVED);
>> >> >>> > +       e820_update_range(addr, sizet, E820_RAM, E820_RESVD_KERN);
>> >> >>>
>> >> >>> this line is not needed.
>> >> >>
>> >> >> Why? Memory reserved by early_rserved_e820 should not be saved during
>> >> >> hibernation? shoudl not be saved by kdump?
>> >> >>
>> >
>> > Can you tell me why this line is not needed?
>>
>> that function only have one user. the mpc reserve_new, so we want it
>> to be E820_RESERVED, and kexec  tools could have that range reserved,
>> the next kernel will not overwrite the updated mptable.
>
> This is OK for kexec. Does it need to be saved by kdump? Does it need to
> be saved by hibernation (suspend to disk)?

the function is only called when alloc_mptable is specified in command line.

YH

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Yinghai Lu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 1572 bytes --]

On Thu, Jun 26, 2008 at 7:48 PM, Huang, Ying <ying.huang@intel.com> wrote:> On Thu, 2008-06-26 at 19:22 -0700, Yinghai Lu wrote:>> On Thu, Jun 26, 2008 at 2:47 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:>> > On Thu, Jun 26, 2008 at 12:48 AM, Huang, Ying <ying.huang@intel.com> wrote:>> >> On Thu, 2008-06-26 at 00:25 -0700, Yinghai Lu wrote:>> >> [...]>> >>> >                if (pfn >= limit_pfn)>> >>> > @@ -977,7 +978,7 @@ u64 __init early_reserve_e820(u64 startt>> >>> >                return 0;>> >>> >>> >>> >        addr = round_down(start + size - sizet, align);>> >>> > -       e820_update_range(addr, sizet, E820_RAM, E820_RESERVED);>> >>> > +       e820_update_range(addr, sizet, E820_RAM, E820_RESVD_KERN);>> >>>>> >>> this line is not needed.>> >>>> >> Why? Memory reserved by early_rserved_e820 should not be saved during>> >> hibernation? shoudl not be saved by kdump?>> >>>> Can you tell me why this line is not needed?>> [...]>> some like the attach patch...>>>> you still can merge parse_setup_data parse_e820_ext>> also entries in parse_e820_ext is not initialized..., __copy_e820_map>> will do nothing.>> OK. Because some E820 entries are available after parse_setup_data(),> it is better to call reserve_setup_data() after calling> parse_setup_data() if update_e820_range() is used instead of> reserve_early().
please modify it and test on your platforms then submit to Ingo..
Thanks
YHÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Huang, Ying]

On Fri, 2008-06-27 at 15:05 -0700, Yinghai Lu wrote:
> On Thu, Jun 26, 2008 at 7:48 PM, Huang, Ying <ying.huang@intel.com> wrote:
> > On Thu, 2008-06-26 at 19:22 -0700, Yinghai Lu wrote:
> >> On Thu, Jun 26, 2008 at 2:47 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> >> > On Thu, Jun 26, 2008 at 12:48 AM, Huang, Ying <ying.huang@intel.com> wrote:
> >> >> On Thu, 2008-06-26 at 00:25 -0700, Yinghai Lu wrote:
> >> >> [...]
> >> >>> >                if (pfn >= limit_pfn)
> >> >>> > @@ -977,7 +978,7 @@ u64 __init early_reserve_e820(u64 startt
> >> >>> >                return 0;
> >> >>> >
> >> >>> >        addr = round_down(start + size - sizet, align);
> >> >>> > -       e820_update_range(addr, sizet, E820_RAM, E820_RESERVED);
> >> >>> > +       e820_update_range(addr, sizet, E820_RAM, E820_RESVD_KERN);
> >> >>>
> >> >>> this line is not needed.
> >> >>
> >> >> Why? Memory reserved by early_rserved_e820 should not be saved during
> >> >> hibernation? shoudl not be saved by kdump?
> >> >>
> >
> > Can you tell me why this line is not needed?
> >
> > [...]
> >> some like the attach patch...
> >>
> >> you still can merge parse_setup_data parse_e820_ext
> >> also entries in parse_e820_ext is not initialized..., __copy_e820_map
> >> will do nothing.
> >
> > OK. Because some E820 entries are available after parse_setup_data(),
> > it is better to call reserve_setup_data() after calling
> > parse_setup_data() if update_e820_range() is used instead of
> > reserve_early().
> 
> please modify it and test on your platforms then submit to Ingo..

It seems that there is an issue:

- If parse_setup_data() is called before reserve_setup_data(), and there
is a conflict between memory area used by setup_data and other memory
area, it is possible that the contents of setup_data is changed. So that
system may panic before reporting memory area conflict. And it seems
that memory area conflict is not checked by e820_update_range().

So maybe it is better to use reserve_early() instead of
e820_update_range() for setup_data.

Best Regards,
Huang Ying

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Yinghai Lu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 2268 bytes --]

On Mon, Jun 30, 2008 at 12:03 AM, Huang, Ying <ying.huang@intel.com> wrote:> On Fri, 2008-06-27 at 15:05 -0700, Yinghai Lu wrote:>> On Thu, Jun 26, 2008 at 7:48 PM, Huang, Ying <ying.huang@intel.com> wrote:>> > On Thu, 2008-06-26 at 19:22 -0700, Yinghai Lu wrote:>> >> On Thu, Jun 26, 2008 at 2:47 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:>> >> > On Thu, Jun 26, 2008 at 12:48 AM, Huang, Ying <ying.huang@intel.com> wrote:>> >> >> On Thu, 2008-06-26 at 00:25 -0700, Yinghai Lu wrote:>> >> >> [...]>> >> >>> >                if (pfn >= limit_pfn)>> >> >>> > @@ -977,7 +978,7 @@ u64 __init early_reserve_e820(u64 startt>> >> >>> >                return 0;>> >> >>> >>> >> >>> >        addr = round_down(start + size - sizet, align);>> >> >>> > -       e820_update_range(addr, sizet, E820_RAM, E820_RESERVED);>> >> >>> > +       e820_update_range(addr, sizet, E820_RAM, E820_RESVD_KERN);>> >> >>>>> >> >>> this line is not needed.>> >> >>>> >> >> Why? Memory reserved by early_rserved_e820 should not be saved during>> >> >> hibernation? shoudl not be saved by kdump?>> >> >>>> >>> > Can you tell me why this line is not needed?>> >>> > [...]>> >> some like the attach patch...>> >>>> >> you still can merge parse_setup_data parse_e820_ext>> >> also entries in parse_e820_ext is not initialized..., __copy_e820_map>> >> will do nothing.>> >>> > OK. Because some E820 entries are available after parse_setup_data(),>> > it is better to call reserve_setup_data() after calling>> > parse_setup_data() if update_e820_range() is used instead of>> > reserve_early().>>>> please modify it and test on your platforms then submit to Ingo..>> It seems that there is an issue:>> - If parse_setup_data() is called before reserve_setup_data(), and there> is a conflict between memory area used by setup_data and other memory> area, it is possible that the contents of setup_data is changed. So that> system may panic before reporting memory area conflict. And it seems> that memory area conflict is not checked by e820_update_range().
what is "other memory area"? returned from find_e820_area? no one use that yet.
YHÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Huang, Ying]

On Mon, 2008-06-30 at 00:34 -0700, Yinghai Lu wrote:
> On Mon, Jun 30, 2008 at 12:03 AM, Huang, Ying <ying.huang@intel.com> wrote:
> > On Fri, 2008-06-27 at 15:05 -0700, Yinghai Lu wrote:
> >> On Thu, Jun 26, 2008 at 7:48 PM, Huang, Ying <ying.huang@intel.com> wrote:
> >> > On Thu, 2008-06-26 at 19:22 -0700, Yinghai Lu wrote:
> >> >> On Thu, Jun 26, 2008 at 2:47 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> >> >> > On Thu, Jun 26, 2008 at 12:48 AM, Huang, Ying <ying.huang@intel.com> wrote:
> >> >> >> On Thu, 2008-06-26 at 00:25 -0700, Yinghai Lu wrote:
> >> >> >> [...]
> >> >> >>> >                if (pfn >= limit_pfn)
> >> >> >>> > @@ -977,7 +978,7 @@ u64 __init early_reserve_e820(u64 startt
> >> >> >>> >                return 0;
> >> >> >>> >
> >> >> >>> >        addr = round_down(start + size - sizet, align);
> >> >> >>> > -       e820_update_range(addr, sizet, E820_RAM, E820_RESERVED);
> >> >> >>> > +       e820_update_range(addr, sizet, E820_RAM, E820_RESVD_KERN);
> >> >> >>>
> >> >> >>> this line is not needed.
> >> >> >>
> >> >> >> Why? Memory reserved by early_rserved_e820 should not be saved during
> >> >> >> hibernation? shoudl not be saved by kdump?
> >> >> >>
> >> >
> >> > Can you tell me why this line is not needed?
> >> >
> >> > [...]
> >> >> some like the attach patch...
> >> >>
> >> >> you still can merge parse_setup_data parse_e820_ext
> >> >> also entries in parse_e820_ext is not initialized..., __copy_e820_map
> >> >> will do nothing.
> >> >
> >> > OK. Because some E820 entries are available after parse_setup_data(),
> >> > it is better to call reserve_setup_data() after calling
> >> > parse_setup_data() if update_e820_range() is used instead of
> >> > reserve_early().
> >>
> >> please modify it and test on your platforms then submit to Ingo..
> >
> > It seems that there is an issue:
> >
> > - If parse_setup_data() is called before reserve_setup_data(), and there
> > is a conflict between memory area used by setup_data and other memory
> > area, it is possible that the contents of setup_data is changed. So that
> > system may panic before reporting memory area conflict. And it seems
> > that memory area conflict is not checked by e820_update_range().
> 
> what is "other memory area"? returned from find_e820_area? no one use that yet.

I mean memory area reserved with reserved_early() or e820_update_range()
before reserve_setup_data() is called.

And because there is no conflict check in e820_update_range(), what to
deal with potential conflict between setup_data and other memory area
regardless which one is reserved earlier?


Another issue related:

Because some memmap entries are available via extended E820 memmap
(SETUP_E820_EXT), it is not strictly safe to use e820_update_range()
between setup_memory_map() and parse_setup_data(). It may be better to
parse extended E820 memmap right after setup_memory_map().

Best Regards,
Huang Ying

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Ingo Molnar]

* Yinghai Lu <yhlu.kernel@gmail.com> wrote:

> > OK. Because some E820 entries are available after 
> > parse_setup_data(), it is better to call reserve_setup_data() after 
> > calling parse_setup_data() if update_e820_range() is used instead of 
> > reserve_early().
> 
> please modify it and test on your platforms then submit to Ingo..

btw, a small suggestion: it is a bit easier on the maintainer side if 
you also send Acked-by lines in such cases (which can be picked up for 
the next iteration of these patches) - that way we can see which patches 
have been reviewed and acked by you and which ones are still under 
discussion.

	Ingo

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Yinghai Lu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 3717 bytes --]

On Mon, Jun 30, 2008 at 12:51 AM, Huang, Ying <ying.huang@intel.com> wrote:> On Mon, 2008-06-30 at 00:34 -0700, Yinghai Lu wrote:>> On Mon, Jun 30, 2008 at 12:03 AM, Huang, Ying <ying.huang@intel.com> wrote:>> > On Fri, 2008-06-27 at 15:05 -0700, Yinghai Lu wrote:>> >> On Thu, Jun 26, 2008 at 7:48 PM, Huang, Ying <ying.huang@intel.com> wrote:>> >> > On Thu, 2008-06-26 at 19:22 -0700, Yinghai Lu wrote:>> >> >> On Thu, Jun 26, 2008 at 2:47 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:>> >> >> > On Thu, Jun 26, 2008 at 12:48 AM, Huang, Ying <ying.huang@intel.com> wrote:>> >> >> >> On Thu, 2008-06-26 at 00:25 -0700, Yinghai Lu wrote:>> >> >> >> [...]>> >> >> >>> >                if (pfn >= limit_pfn)>> >> >> >>> > @@ -977,7 +978,7 @@ u64 __init early_reserve_e820(u64 startt>> >> >> >>> >                return 0;>> >> >> >>> >>> >> >> >>> >        addr = round_down(start + size - sizet, align);>> >> >> >>> > -       e820_update_range(addr, sizet, E820_RAM, E820_RESERVED);>> >> >> >>> > +       e820_update_range(addr, sizet, E820_RAM, E820_RESVD_KERN);>> >> >> >>>>> >> >> >>> this line is not needed.>> >> >> >>>> >> >> >> Why? Memory reserved by early_rserved_e820 should not be saved during>> >> >> >> hibernation? shoudl not be saved by kdump?>> >> >> >>>> >> >>> >> > Can you tell me why this line is not needed?>> >> >>> >> > [...]>> >> >> some like the attach patch...>> >> >>>> >> >> you still can merge parse_setup_data parse_e820_ext>> >> >> also entries in parse_e820_ext is not initialized..., __copy_e820_map>> >> >> will do nothing.>> >> >>> >> > OK. Because some E820 entries are available after parse_setup_data(),>> >> > it is better to call reserve_setup_data() after calling>> >> > parse_setup_data() if update_e820_range() is used instead of>> >> > reserve_early().>> >>>> >> please modify it and test on your platforms then submit to Ingo..>> >>> > It seems that there is an issue:>> >>> > - If parse_setup_data() is called before reserve_setup_data(), and there>> > is a conflict between memory area used by setup_data and other memory>> > area, it is possible that the contents of setup_data is changed. So that>> > system may panic before reporting memory area conflict. And it seems>> > that memory area conflict is not checked by e820_update_range().>>>> what is "other memory area"? returned from find_e820_area? no one use that yet.>> I mean memory area reserved with reserved_early() or e820_update_range()> before reserve_setup_data() is called.
before parse_setup_data, reserve_early is called for1. kernel text/data/bss + initial pgt2. ramdisk3. ebdae820_update_range is not called.at this time early_res have RAM reserved by kernel.
then setup_memory_map is called, so e820 have some ranges...directlyfrom e820 table
next need to call parse_setup_datait will add some entries in e820
then reserve_setup_data is called, it will use e820_update_range toreserve setup_data itself directly in e820
...
>> And because there is no conflict check in e820_update_range(), what to> deal with potential conflict between setup_data and other memory area> regardless which one is reserved earlier?
find_e820_area will make sure it only find ram from e820 and it is notconflict with early_res

>>> Another issue related:>> Because some memmap entries are available via extended E820 memmap> (SETUP_E820_EXT), it is not strictly safe to use e820_update_range()> between setup_memory_map() and parse_setup_data(). It may be better to> parse extended E820 memmap right after setup_memory_map().
will look at the code again..
YHÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 3915 bytes --]

On Mon, Jun 30, 2008 at 2:15 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> On Mon, Jun 30, 2008 at 12:51 AM, Huang, Ying <ying.huang@intel.com> wrote:
>> On Mon, 2008-06-30 at 00:34 -0700, Yinghai Lu wrote:
>>> On Mon, Jun 30, 2008 at 12:03 AM, Huang, Ying <ying.huang@intel.com> wrote:
>>> > On Fri, 2008-06-27 at 15:05 -0700, Yinghai Lu wrote:
>>> >> On Thu, Jun 26, 2008 at 7:48 PM, Huang, Ying <ying.huang@intel.com> wrote:
>>> >> > On Thu, 2008-06-26 at 19:22 -0700, Yinghai Lu wrote:
>>> >> >> On Thu, Jun 26, 2008 at 2:47 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>>> >> >> > On Thu, Jun 26, 2008 at 12:48 AM, Huang, Ying <ying.huang@intel.com> wrote:
>>> >> >> >> On Thu, 2008-06-26 at 00:25 -0700, Yinghai Lu wrote:
>>> >> >> >> [...]
>>> >> >> >>> >                if (pfn >= limit_pfn)
>>> >> >> >>> > @@ -977,7 +978,7 @@ u64 __init early_reserve_e820(u64 startt
>>> >> >> >>> >                return 0;
>>> >> >> >>> >
>>> >> >> >>> >        addr = round_down(start + size - sizet, align);
>>> >> >> >>> > -       e820_update_range(addr, sizet, E820_RAM, E820_RESERVED);
>>> >> >> >>> > +       e820_update_range(addr, sizet, E820_RAM, E820_RESVD_KERN);
>>> >> >> >>>
>>> >> >> >>> this line is not needed.
>>> >> >> >>
>>> >> >> >> Why? Memory reserved by early_rserved_e820 should not be saved during
>>> >> >> >> hibernation? shoudl not be saved by kdump?
>>> >> >> >>
>>> >> >
>>> >> > Can you tell me why this line is not needed?
>>> >> >
>>> >> > [...]
>>> >> >> some like the attach patch...
>>> >> >>
>>> >> >> you still can merge parse_setup_data parse_e820_ext
>>> >> >> also entries in parse_e820_ext is not initialized..., __copy_e820_map
>>> >> >> will do nothing.
>>> >> >
>>> >> > OK. Because some E820 entries are available after parse_setup_data(),
>>> >> > it is better to call reserve_setup_data() after calling
>>> >> > parse_setup_data() if update_e820_range() is used instead of
>>> >> > reserve_early().
>>> >>
>>> >> please modify it and test on your platforms then submit to Ingo..
>>> >
>>> > It seems that there is an issue:
>>> >
>>> > - If parse_setup_data() is called before reserve_setup_data(), and there
>>> > is a conflict between memory area used by setup_data and other memory
>>> > area, it is possible that the contents of setup_data is changed. So that
>>> > system may panic before reporting memory area conflict. And it seems
>>> > that memory area conflict is not checked by e820_update_range().
>>>
>>> what is "other memory area"? returned from find_e820_area? no one use that yet.
>>
>> I mean memory area reserved with reserved_early() or e820_update_range()
>> before reserve_setup_data() is called.
>
> before parse_setup_data, reserve_early is called for
> 1. kernel text/data/bss + initial pgt
> 2. ramdisk
> 3. ebda
> e820_update_range is not called.
> at this time early_res have RAM reserved by kernel.
>
> then setup_memory_map is called, so e820 have some ranges...directly
> from e820 table
>
> next need to call parse_setup_data
> it will add some entries in e820
>
> then reserve_setup_data is called, it will use e820_update_range to
> reserve setup_data itself directly in e820
>
> ...
>
>>
>> And because there is no conflict check in e820_update_range(), what to
>> deal with potential conflict between setup_data and other memory area
>> regardless which one is reserved earlier?
>
> find_e820_area will make sure it only find ram from e820 and it is not
> conflict with early_res
>
>
>>
>>
>> Another issue related:
>>
>> Because some memmap entries are available via extended E820 memmap
>> (SETUP_E820_EXT), it is not strictly safe to use e820_update_range()
>> between setup_memory_map() and parse_setup_data(). It may be better to
>> parse extended E820 memmap right after setup_memory_map().
>
> will look at the code again..

Ying,

please check attach the v2 patch...
remove one left over reserve_setup_data for 32 bit

YH

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: reserve_setup_data_v2.patch --]
[-- Type: text/x-patch; name=reserve_setup_data_v2.patch, Size: 4777 bytes --]

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index ba5ac88..e03b89a 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -120,6 +120,7 @@ void __init e820_print_map(char *who)
 		       (e820.map[i].addr + e820.map[i].size));
 		switch (e820.map[i].type) {
 		case E820_RAM:
+		case E820_RESERVED_KERN:
 			printk(KERN_CONT "(usable)\n");
 			break;
 		case E820_RESERVED:
@@ -611,7 +612,7 @@ void __init e820_mark_nosave_regions(unsigned long limit_pfn)
 			register_nosave_region(pfn, PFN_UP(ei->addr));
 
 		pfn = PFN_DOWN(ei->addr + ei->size);
-		if (ei->type != E820_RAM)
+		if (ei->type != E820_RAM && ei->type != E820_RESERVED_KERN)
 			register_nosave_region(PFN_UP(ei->addr), pfn);
 
 		if (pfn >= limit_pfn)
@@ -1207,6 +1208,7 @@ void __init e820_reserve_resources(void)
 	res = alloc_bootmem_low(sizeof(struct resource) * e820.nr_map);
 	for (i = 0; i < e820.nr_map; i++) {
 		switch (e820.map[i].type) {
+		case E820_RESERVED_KERN:
 		case E820_RAM:	res->name = "System RAM"; break;
 		case E820_ACPI:	res->name = "ACPI Tables"; break;
 		case E820_NVS:	res->name = "ACPI Non-volatile Storage"; break;
diff --git a/arch/x86/kernel/head.c b/arch/x86/kernel/head.c
index a6816be..3e66bd3 100644
--- a/arch/x86/kernel/head.c
+++ b/arch/x86/kernel/head.c
@@ -53,21 +53,3 @@ void __init reserve_ebda_region(void)
 	/* reserve all memory between lowmem and the 1MB mark */
 	reserve_early_overlap_ok(lowmem, 0x100000, "BIOS reserved");
 }
-
-void __init reserve_setup_data(void)
-{
-	struct setup_data *data;
-	u64 pa_data;
-	char buf[32];
-
-	if (boot_params.hdr.version < 0x0209)
-		return;
-	pa_data = boot_params.hdr.setup_data;
-	while (pa_data) {
-		data = early_ioremap(pa_data, sizeof(*data));
-		sprintf(buf, "setup data %x", data->type);
-		reserve_early(pa_data, pa_data+sizeof(*data)+data->len, buf);
-		pa_data = data->next;
-		early_iounmap(data, sizeof(*data));
-	}
-}
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index c970929..2ff9bc6 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -123,7 +123,6 @@ void __init x86_64_start_kernel(char * real_mode_data)
 #endif
 
 	reserve_ebda_region();
-	reserve_setup_data();
 
 	/*
 	 * At this point everything still needed from the boot loader
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index fb318ed..cf03401 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -389,14 +389,33 @@ static void __init parse_setup_data(void)
 		default:
 			break;
 		}
-#ifndef CONFIG_DEBUG_BOOT_PARAMS
-		free_early(pa_data, pa_data+sizeof(*data)+data->len);
-#endif
 		pa_data = data->next;
 		early_iounmap(data, PAGE_SIZE);
 	}
 }
 
+static void __init reserve_setup_data(void)
+{
+	struct setup_data *data;
+	u64 pa_data;
+	char buf[32];
+
+	if (boot_params.hdr.version < 0x0209)
+		return;
+	pa_data = boot_params.hdr.setup_data;
+	while (pa_data) {
+		data = early_ioremap(pa_data, sizeof(*data));
+		sprintf(buf, "setup data %x", data->type);
+		e820_update_range(pa_data, sizeof(*data)+data->len,
+			 E820_RAM, E820_RESERVED_KERN);
+		pa_data = data->next;
+		early_iounmap(data, sizeof(*data));
+	}
+	sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
+	printk(KERN_INFO "extended physical RAM map:\n");
+	e820_print_map("extended");
+}
+
 /*
  * --------- Crashkernel reservation ------------------------------
  */
@@ -524,7 +543,6 @@ void __init setup_arch(char **cmdline_p)
 	pre_setup_arch_hook();
 	early_cpu_init();
 	early_ioremap_init();
-	reserve_setup_data();
 #else
 	printk(KERN_INFO "Command line: %s\n", boot_command_line);
 #endif
@@ -566,6 +584,9 @@ void __init setup_arch(char **cmdline_p)
 	ARCH_SETUP
 
 	setup_memory_map();
+	parse_setup_data();
+	reserve_setup_data();
+
 	copy_edd();
 
 	if (!boot_params.hdr.root_flags)
@@ -592,8 +613,6 @@ void __init setup_arch(char **cmdline_p)
 	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
 	*cmdline_p = command_line;
 
-	parse_setup_data();
-
 	parse_early_param();
 
 	if (acpi_mps_check()) {
diff --git a/include/asm-x86/bootparam.h b/include/asm-x86/bootparam.h
index 6eeba3b..ae22bdf 100644
--- a/include/asm-x86/bootparam.h
+++ b/include/asm-x86/bootparam.h
@@ -108,6 +108,4 @@ struct boot_params {
 	__u8  _pad9[276];				/* 0xeec */
 } __attribute__((packed));
 
-void reserve_setup_data(void);
-
 #endif /* _ASM_BOOTPARAM_H */
diff --git a/include/asm-x86/e820.h b/include/asm-x86/e820.h
index f622685..45e904f 100644
--- a/include/asm-x86/e820.h
+++ b/include/asm-x86/e820.h
@@ -44,6 +44,9 @@
 #define E820_ACPI	3
 #define E820_NVS	4
 
+/* reserved RAM used by kernel itself */
+#define E820_RESERVED_KERN        128
+
 #ifndef __ASSEMBLY__
 struct e820entry {
 	__u64 addr;	/* start of memory segment */

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Huang, Ying]

On Mon, 2008-06-30 at 02:15 -0700, Yinghai Lu wrote:
[...]
> >
> > I mean memory area reserved with reserved_early() or e820_update_range()
> > before reserve_setup_data() is called.
> 
> before parse_setup_data, reserve_early is called for
> 1. kernel text/data/bss + initial pgt
> 2. ramdisk
> 3. ebda
> e820_update_range is not called.
> at this time early_res have RAM reserved by kernel.
> 
> then setup_memory_map is called, so e820 have some ranges...directly
> from e820 table
> 
> next need to call parse_setup_data
> it will add some entries in e820
> 
> then reserve_setup_data is called, it will use e820_update_range to
> reserve setup_data itself directly in e820
> 
> ...

Yes. There is no real conflict now. I think a better rule is:

- Reserve memory area (directly from BIOS or boot-loader, not from
find_e820_area) needed as early as possible.
- Don't touch reserved area until all possible reservation is made, that
is, before conflict check is done.

> >
> > And because there is no conflict check in e820_update_range(), what to
> > deal with potential conflict between setup_data and other memory area
> > regardless which one is reserved earlier?
> 
> find_e820_area will make sure it only find ram from e820 and it is not
> conflict with early_res

For find_e820_area, this is safe enough. But what about conflict between
setup_data and ebda or ramdisk?

Best Regards,
Huang Ying

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Yinghai Lu]

On Mon, Jun 30, 2008 at 2:38 AM, Huang, Ying <ying.huang@intel.com> wrote:
> On Mon, 2008-06-30 at 02:15 -0700, Yinghai Lu wrote:
> [...]
>> >
>> > I mean memory area reserved with reserved_early() or e820_update_range()
>> > before reserve_setup_data() is called.
>>
>> before parse_setup_data, reserve_early is called for
>> 1. kernel text/data/bss + initial pgt
>> 2. ramdisk
>> 3. ebda
>> e820_update_range is not called.
>> at this time early_res have RAM reserved by kernel.
>>
>> then setup_memory_map is called, so e820 have some ranges...directly
>> from e820 table
>>
>> next need to call parse_setup_data
>> it will add some entries in e820
>>
>> then reserve_setup_data is called, it will use e820_update_range to
>> reserve setup_data itself directly in e820
>>
>> ...
>
> Yes. There is no real conflict now. I think a better rule is:
>
> - Reserve memory area (directly from BIOS or boot-loader, not from
> find_e820_area) needed as early as possible.
> - Don't touch reserved area until all possible reservation is made, that
> is, before conflict check is done.
no one touch the reserved area. before all reserved in early_res or e820.
>
>> >
>> > And because there is no conflict check in e820_update_range(), what to
>> > deal with potential conflict between setup_data and other memory area
>> > regardless which one is reserved earlier?
>>
>> find_e820_area will make sure it only find ram from e820 and it is not
>> conflict with early_res
>
> For find_e820_area, this is safe enough. But what about conflict between
> setup_data and ebda or ramdisk?

can you have setup_data and ebda at the same time?

setup_data and ramdisk should be ok, because bootloader is supposed to
make them not to be conflicts.

YH

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Ingo Molnar]

* Yinghai Lu <yhlu.kernel@gmail.com> wrote:

> > For find_e820_area, this is safe enough. But what about conflict 
> > between setup_data and ebda or ramdisk?
> 
> can you have setup_data and ebda at the same time?
> 
> setup_data and ramdisk should be ok, because bootloader is supposed to 
> make them not to be conflicts.

the more sanity checks we do before relying on some crutial data, the 
better. It's easier to panic or sanitize data in some structured way and 
complain about it in the syslog than to let things get corrupted. Boot 
loaders are ... not unknown to be have bugs too, at times.

	Ingo

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Yinghai Lu]

On Mon, Jun 30, 2008 at 12:16 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>
>> > For find_e820_area, this is safe enough. But what about conflict
>> > between setup_data and ebda or ramdisk?
>>
>> can you have setup_data and ebda at the same time?
>>
>> setup_data and ramdisk should be ok, because bootloader is supposed to
>> make them not to be conflicts.
>
> the more sanity checks we do before relying on some crutial data, the
> better. It's easier to panic or sanitize data in some structured way and
> complain about it in the syslog than to let things get corrupted. Boot
> loaders are ... not unknown to be have bugs too, at times.

to address Ying's concern, we could let reserve_setup_data
call reserve_early in addition to e820_update_range...

reserve_early will panic if RAMDISK overlap efi setup_data...

YH

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

On Mon, Jun 30, 2008 at 3:53 PM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> On Mon, Jun 30, 2008 at 12:16 PM, Ingo Molnar <mingo@elte.hu> wrote:
>>
>> * Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>>
>>> > For find_e820_area, this is safe enough. But what about conflict
>>> > between setup_data and ebda or ramdisk?
>>>
>>> can you have setup_data and ebda at the same time?
>>>
>>> setup_data and ramdisk should be ok, because bootloader is supposed to
>>> make them not to be conflicts.
>>
>> the more sanity checks we do before relying on some crutial data, the
>> better. It's easier to panic or sanitize data in some structured way and
>> complain about it in the syslog than to let things get corrupted. Boot
>> loaders are ... not unknown to be have bugs too, at times.
>
> to address Ying's concern, we could let reserve_setup_data
> call reserve_early in addition to e820_update_range...
>
> reserve_early will panic if RAMDISK overlap efi setup_data...
>

Ying,
please check the attached patch

YH

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: reserve_setup_data_v3.patch --]
[-- Type: text/x-patch; name=reserve_setup_data_v3.patch, Size: 6085 bytes --]

[PATCH] x86: move reserve_setup_data to setup.c

Ying Huang want to setup_data to be reserved, but not included in no save
range.

here try to modify e820 table to reserve that range as early.
also add that in early_res in case bootloader mess up it with ramdisk.

other solution would be
1. add early_res_to_highmem...
2. early_res_to_e820...
but they could reserve wrongly other type memory, if early_res has some resource reserved early,
and not need later, but it is not removed from early_res in time.
like the RAMDISK (already handled).

Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>

---
 arch/x86/kernel/e820.c      |    4 +++-
 arch/x86/kernel/head.c      |   18 ------------------
 arch/x86/kernel/head64.c    |    1 -
 arch/x86/kernel/setup.c     |   34 ++++++++++++++++++++++++++++------
 include/asm-x86/bootparam.h |    2 --
 include/asm-x86/e820.h      |    3 +++
 6 files changed, 34 insertions(+), 28 deletions(-)

Index: linux-2.6/arch/x86/kernel/e820.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/e820.c
+++ linux-2.6/arch/x86/kernel/e820.c
@@ -120,6 +120,7 @@ void __init e820_print_map(char *who)
 		       (e820.map[i].addr + e820.map[i].size));
 		switch (e820.map[i].type) {
 		case E820_RAM:
+		case E820_RESERVED_KERN:
 			printk(KERN_CONT "(usable)\n");
 			break;
 		case E820_RESERVED:
@@ -611,7 +612,7 @@ void __init e820_mark_nosave_regions(uns
 			register_nosave_region(pfn, PFN_UP(ei->addr));
 
 		pfn = PFN_DOWN(ei->addr + ei->size);
-		if (ei->type != E820_RAM)
+		if (ei->type != E820_RAM && ei->type != E820_RESERVED_KERN)
 			register_nosave_region(PFN_UP(ei->addr), pfn);
 
 		if (pfn >= limit_pfn)
@@ -1207,6 +1208,7 @@ void __init e820_reserve_resources(void)
 	res = alloc_bootmem_low(sizeof(struct resource) * e820.nr_map);
 	for (i = 0; i < e820.nr_map; i++) {
 		switch (e820.map[i].type) {
+		case E820_RESERVED_KERN:
 		case E820_RAM:	res->name = "System RAM"; break;
 		case E820_ACPI:	res->name = "ACPI Tables"; break;
 		case E820_NVS:	res->name = "ACPI Non-volatile Storage"; break;
Index: linux-2.6/arch/x86/kernel/head.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/head.c
+++ linux-2.6/arch/x86/kernel/head.c
@@ -53,21 +53,3 @@ void __init reserve_ebda_region(void)
 	/* reserve all memory between lowmem and the 1MB mark */
 	reserve_early_overlap_ok(lowmem, 0x100000, "BIOS reserved");
 }
-
-void __init reserve_setup_data(void)
-{
-	struct setup_data *data;
-	u64 pa_data;
-	char buf[32];
-
-	if (boot_params.hdr.version < 0x0209)
-		return;
-	pa_data = boot_params.hdr.setup_data;
-	while (pa_data) {
-		data = early_ioremap(pa_data, sizeof(*data));
-		sprintf(buf, "setup data %x", data->type);
-		reserve_early(pa_data, pa_data+sizeof(*data)+data->len, buf);
-		pa_data = data->next;
-		early_iounmap(data, sizeof(*data));
-	}
-}
Index: linux-2.6/arch/x86/kernel/head64.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/head64.c
+++ linux-2.6/arch/x86/kernel/head64.c
@@ -123,7 +123,6 @@ void __init x86_64_start_kernel(char * r
 #endif
 
 	reserve_ebda_region();
-	reserve_setup_data();
 
 	/*
 	 * At this point everything still needed from the boot loader
Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c
@@ -389,14 +389,34 @@ static void __init parse_setup_data(void
 		default:
 			break;
 		}
-#ifndef CONFIG_DEBUG_BOOT_PARAMS
-		free_early(pa_data, pa_data+sizeof(*data)+data->len);
-#endif
 		pa_data = data->next;
 		early_iounmap(data, PAGE_SIZE);
 	}
 }
 
+static void __init reserve_setup_data(void)
+{
+	struct setup_data *data;
+	u64 pa_data;
+	char buf[32];
+
+	if (boot_params.hdr.version < 0x0209)
+		return;
+	pa_data = boot_params.hdr.setup_data;
+	while (pa_data) {
+		data = early_ioremap(pa_data, sizeof(*data));
+		sprintf(buf, "setup data %x", data->type);
+		reserve_early(pa_data, pa_data+sizeof(*data)+data->len, buf);
+		e820_update_range(pa_data, sizeof(*data)+data->len,
+			 E820_RAM, E820_RESERVED_KERN);
+		pa_data = data->next;
+		early_iounmap(data, sizeof(*data));
+	}
+	sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
+	printk(KERN_INFO "extended physical RAM map:\n");
+	e820_print_map("reserve setup_data");
+}
+
 /*
  * --------- Crashkernel reservation ------------------------------
  */
@@ -524,7 +544,6 @@ void __init setup_arch(char **cmdline_p)
 	pre_setup_arch_hook();
 	early_cpu_init();
 	early_ioremap_init();
-	reserve_setup_data();
 #else
 	printk(KERN_INFO "Command line: %s\n", boot_command_line);
 #endif
@@ -566,6 +585,8 @@ void __init setup_arch(char **cmdline_p)
 	ARCH_SETUP
 
 	setup_memory_map();
+	parse_setup_data();
+
 	copy_edd();
 
 	if (!boot_params.hdr.root_flags)
@@ -592,10 +613,11 @@ void __init setup_arch(char **cmdline_p)
 	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
 	*cmdline_p = command_line;
 
-	parse_setup_data();
-
 	parse_early_param();
 
+	/* after early param, so could get panic from serial */
+	reserve_setup_data();
+
 	if (acpi_mps_check()) {
 #ifdef CONFIG_X86_LOCAL_APIC
 		disable_apic = 1;
Index: linux-2.6/include/asm-x86/bootparam.h
===================================================================
--- linux-2.6.orig/include/asm-x86/bootparam.h
+++ linux-2.6/include/asm-x86/bootparam.h
@@ -108,6 +108,4 @@ struct boot_params {
 	__u8  _pad9[276];				/* 0xeec */
 } __attribute__((packed));
 
-void reserve_setup_data(void);
-
 #endif /* _ASM_BOOTPARAM_H */
Index: linux-2.6/include/asm-x86/e820.h
===================================================================
--- linux-2.6.orig/include/asm-x86/e820.h
+++ linux-2.6/include/asm-x86/e820.h
@@ -44,6 +44,9 @@
 #define E820_ACPI	3
 #define E820_NVS	4
 
+/* reserved RAM used by kernel itself */
+#define E820_RESERVED_KERN        128
+
 #ifndef __ASSEMBLY__
 struct e820entry {
 	__u64 addr;	/* start of memory segment */

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Huang, Ying]

Hi, Yinghai,

On Mon, 2008-06-30 at 16:20 -0700, Yinghai Lu wrote:
> On Mon, Jun 30, 2008 at 3:53 PM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> > On Mon, Jun 30, 2008 at 12:16 PM, Ingo Molnar <mingo@elte.hu> wrote:
> >>
> >> * Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> >>
> >>> > For find_e820_area, this is safe enough. But what about conflict
> >>> > between setup_data and ebda or ramdisk?
> >>>
> >>> can you have setup_data and ebda at the same time?
> >>>
> >>> setup_data and ramdisk should be ok, because bootloader is supposed to
> >>> make them not to be conflicts.
> >>
> >> the more sanity checks we do before relying on some crutial data, the
> >> better. It's easier to panic or sanitize data in some structured way and
> >> complain about it in the syslog than to let things get corrupted. Boot
> >> loaders are ... not unknown to be have bugs too, at times.
> >
> > to address Ying's concern, we could let reserve_setup_data
> > call reserve_early in addition to e820_update_range...
> >
> > reserve_early will panic if RAMDISK overlap efi setup_data...
> >
> 
> Ying,
> please check the attached patch

Which git-x86 head should I use to test the patch? It can not be applied
to x86/master.

Best Regards,
Huang Ying

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Yinghai Lu]

On Mon, Jun 30, 2008 at 6:09 PM, Huang, Ying <ying.huang@intel.com> wrote:
> Hi, Yinghai,
>
> On Mon, 2008-06-30 at 16:20 -0700, Yinghai Lu wrote:
>> On Mon, Jun 30, 2008 at 3:53 PM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>> > On Mon, Jun 30, 2008 at 12:16 PM, Ingo Molnar <mingo@elte.hu> wrote:
>> >>
>> >> * Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>> >>
>> >>> > For find_e820_area, this is safe enough. But what about conflict
>> >>> > between setup_data and ebda or ramdisk?
>> >>>
>> >>> can you have setup_data and ebda at the same time?
>> >>>
>> >>> setup_data and ramdisk should be ok, because bootloader is supposed to
>> >>> make them not to be conflicts.
>> >>
>> >> the more sanity checks we do before relying on some crutial data, the
>> >> better. It's easier to panic or sanitize data in some structured way and
>> >> complain about it in the syslog than to let things get corrupted. Boot
>> >> loaders are ... not unknown to be have bugs too, at times.
>> >
>> > to address Ying's concern, we could let reserve_setup_data
>> > call reserve_early in addition to e820_update_range...
>> >
>> > reserve_early will panic if RAMDISK overlap efi setup_data...
>> >
>>
>> Ying,
>> please check the attached patch
>
> Which git-x86 head should I use to test the patch? It can not be applied
> to x86/master.
>

tip/master

YH

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Huang, Ying]

On Mon, 2008-06-30 at 16:20 -0700, Yinghai Lu wrote:
> On Mon, Jun 30, 2008 at 3:53 PM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> > On Mon, Jun 30, 2008 at 12:16 PM, Ingo Molnar <mingo@elte.hu> wrote:
> >>
> >> * Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> >>
> >>> > For find_e820_area, this is safe enough. But what about conflict
> >>> > between setup_data and ebda or ramdisk?
> >>>
> >>> can you have setup_data and ebda at the same time?
> >>>
> >>> setup_data and ramdisk should be ok, because bootloader is supposed to
> >>> make them not to be conflicts.
> >>
> >> the more sanity checks we do before relying on some crutial data, the
> >> better. It's easier to panic or sanitize data in some structured way and
> >> complain about it in the syslog than to let things get corrupted. Boot
> >> loaders are ... not unknown to be have bugs too, at times.
> >
> > to address Ying's concern, we could let reserve_setup_data
> > call reserve_early in addition to e820_update_range...
> >
> > reserve_early will panic if RAMDISK overlap efi setup_data...
> >
> 
> Ying,
> please check the attached patch
> 

Tested-by: Huang, Ying <ying.huang@intel.com>

Best Regards
Huang Ying

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Ingo Molnar]

* Yinghai Lu <yhlu.kernel@gmail.com> wrote:

> >> Ying,
> >> please check the attached patch
> >
> > Which git-x86 head should I use to test the patch? It can not be applied
> > to x86/master.
> 
> tip/master

hm, tip/master should be exactly the same thing as x86/master. (the 
x86.git repository is a symlink to tip.git on kernel.org)

	Ingo

Re: [PATCH 1/2] x86 boot: add E820_RESVD_KERN

[Ingo Molnar]

* Huang, Ying <ying.huang@intel.com> wrote:

> On Mon, 2008-06-30 at 16:20 -0700, Yinghai Lu wrote:
> > Ying,
> > please check the attached patch
> > 
> 
> Tested-by: Huang, Ying <ying.huang@intel.com>

applied to tip/x86/unify-setup - thanks!

	Ingo