[PATCH] x86 boot: Pass E820 memory map entries more than 128 via linked list of setup data

[PATCH] x86 boot: Pass E820 memory map entries more than 128 via linked list of setup data

[Huang, Ying]

Because of the size limits of struct boot_params (zero page), the
maximum number of E820 memory map entries can be passed to kernel is
128. As pointed by Paul Jackson, there is some machine produced by SGI
with so many nodes that the number of E820 memory map entries is more
than 128. To enabling Linux kernel on these system, a new setup data
type named SETUP_E820_EXT is defined to pass additional memory map
entries to Linux kernel.

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

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

---
 arch/x86/kernel/e820.c      |   59 ++++++++++++++++++++++++++++++++++----------
 arch/x86/kernel/setup.c     |    3 ++
 include/asm-x86/bootparam.h |    1 
 include/asm-x86/e820.h      |    2 +
 4 files changed, 52 insertions(+), 13 deletions(-)

--- a/include/asm-x86/bootparam.h
+++ b/include/asm-x86/bootparam.h
@@ -11,6 +11,7 @@
 
 /* setup data types */
 #define SETUP_NONE			0
+#define SETUP_E820_EXT			1
 
 /* extensible setup data list node */
 struct setup_data {
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -359,6 +359,26 @@ static struct e820entry new_bios[E820_X_
 	return 0;
 }
 
+static int __init __copy_e820_map(struct e820entry *biosmap, int nr_map)
+{
+	while (nr_map) {
+		u64 start = biosmap->addr;
+		u64 size = biosmap->size;
+		u64 end = start + size;
+		u32 type = biosmap->type;
+
+		/* Overflow in 64 bits? Ignore the memory map. */
+		if (start > end)
+			return -1;
+
+		add_memory_region(start, size, type);
+
+		biosmap++;
+		nr_map--;
+	}
+	return 0;
+}
+
 /*
  * Copy the BIOS e820 map into a safe place.
  *
@@ -374,19 +394,7 @@ int __init copy_e820_map(struct e820entr
 	if (nr_map < 2)
 		return -1;
 
-	do {
-		u64 start = biosmap->addr;
-		u64 size = biosmap->size;
-		u64 end = start + size;
-		u32 type = biosmap->type;
-
-		/* Overflow in 64 bits? Ignore the memory map. */
-		if (start > end)
-			return -1;
-
-		add_memory_region(start, size, type);
-	} while (biosmap++, --nr_map);
-	return 0;
+	return __copy_e820_map(biosmap, nr_map);
 }
 
 u64 __init update_memory_range(u64 start, u64 size, unsigned old_type,
@@ -496,6 +504,31 @@ __init void e820_setup_gap(void)
 	       pci_mem_start, gapstart, gapsize);
 }
 
+/**
+ * Because of the size limitation of struct boot_params, only first
+ * 128 E820 memory entries are passed to kernel via
+ * boot_params.e820_map, others are passed via SETUP_E820_EXT node of
+ * linked list of struct setup_data, which is parsed here.
+ */
+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);
+	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");
+}
+
 #if defined(CONFIG_X86_64) || \
 	(defined(CONFIG_X86_32) && defined(CONFIG_HIBERNATION))
 /**
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -231,6 +231,9 @@ void __init parse_setup_data(void)
 	while (pa_data) {
 		data = early_ioremap(pa_data, PAGE_SIZE);
 		switch (data->type) {
+		case SETUP_E820_EXT:
+			parse_e820_ext(data, pa_data);
+			break;
 		default:
 			break;
 		}
--- a/include/asm-x86/e820.h
+++ b/include/asm-x86/e820.h
@@ -69,6 +69,8 @@ extern u64 update_memory_range(u64 start
 			       unsigned new_type);
 extern void update_e820(void);
 extern void e820_setup_gap(void);
+struct setup_data;
+extern void parse_e820_ext(struct setup_data *data, unsigned long pa_data);
 
 #if defined(CONFIG_X86_64) || \
 	(defined(CONFIG_X86_32) && defined(CONFIG_HIBERNATION))

Re: [PATCH] x86 boot: Pass E820 memory map entries more than 128 via linked list of setup data

[Ingo Molnar]

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

> Because of the size limits of struct boot_params (zero page), the 
> maximum number of E820 memory map entries can be passed to kernel is 
> 128. As pointed by Paul Jackson, there is some machine produced by SGI 
> with so many nodes that the number of E820 memory map entries is more 
> than 128. To enabling Linux kernel on these system, a new setup data 
> type named SETUP_E820_EXT is defined to pass additional memory map 
> entries to Linux kernel.
> 
> This patch is based on x86/auto-latest branch of git-x86 tree and has 
> been tested on x86_64 and i386 platform.

applied to tip/x86/mpparse, thanks.

this bit collided with a recent change:

> -	do {
> -		u64 start = biosmap->addr;
> -		u64 size = biosmap->size;
> -		u64 end = start + size;
> -		u32 type = biosmap->type;
> -
> -		/* Overflow in 64 bits? Ignore the memory map. */
> -		if (start > end)
> -			return -1;
> -
> -		add_memory_region(start, size, type);
> -	} while (biosmap++, --nr_map);
> -	return 0;
> +	return __copy_e820_map(biosmap, nr_map);

because add_memory_region() got renamed to e820_add_region(). I did the 
obvious fixup.

	Ingo

Re: [PATCH] x86 boot: Pass E820 memory map entries more than 128 via linked list of setup data

[Huang, Ying]

Hi, Paul,

This patch can be used for machine whose E820 memory map has more than
128 entries. So it is not necessary to use EFI memory map for this (EFI
memory map is used for EFI runtime services only).

With this patch, your previous patch:

x86 boot: add code to add BIOS provided EFI memory entries to kernel

is not necessary, so can be reversed.

What do you think about that?

Best Regards,
Huang Ying

On Wed, 2008-06-18 at 13:45 +0200, Ingo Molnar wrote:
> * Huang, Ying <ying.huang@intel.com> wrote:
> 
> > Because of the size limits of struct boot_params (zero page), the 
> > maximum number of E820 memory map entries can be passed to kernel is 
> > 128. As pointed by Paul Jackson, there is some machine produced by SGI 
> > with so many nodes that the number of E820 memory map entries is more 
> > than 128. To enabling Linux kernel on these system, a new setup data 
> > type named SETUP_E820_EXT is defined to pass additional memory map 
> > entries to Linux kernel.
> > 
> > This patch is based on x86/auto-latest branch of git-x86 tree and has 
> > been tested on x86_64 and i386 platform.
> 
> applied to tip/x86/mpparse, thanks.
> 
> this bit collided with a recent change:
> 
> > -	do {
> > -		u64 start = biosmap->addr;
> > -		u64 size = biosmap->size;
> > -		u64 end = start + size;
> > -		u32 type = biosmap->type;
> > -
> > -		/* Overflow in 64 bits? Ignore the memory map. */
> > -		if (start > end)
> > -			return -1;
> > -
> > -		add_memory_region(start, size, type);
> > -	} while (biosmap++, --nr_map);
> > -	return 0;
> > +	return __copy_e820_map(biosmap, nr_map);
> 
> because add_memory_region() got renamed to e820_add_region(). I did the 
> obvious fixup.
> 
> 	Ingo

Re: [PATCH] x86 boot: Pass E820 memory map entries more than 128 via linked list of setup data

[Paul Jackson]

Huang Ying wrote:
> With this patch, your previous patch:
> 
> x86 boot: add code to add BIOS provided EFI memory entries to kernel
> 
> is not necessary, so can be reversed.
> 
> What do you think about that?

... the same thing I thought about it when you asked this question
last month, in post http://lkml.org/lkml/2008/5/26/307.

Please see my reply in http://lkml.org/lkml/2008/5/28/59.
It was lengthy and carefully researched, so I won't repeat
it here.

In short, we still need it.  The key kernel routine that adds
additional EFI entries to the existing E820 map is just 20 or so
fairly easy lines.  We agree that it doesn't handle some of what your
more extended work with a linked list of setup data handles (such as
additional EDD entries), but it does handle additional memory map
(node) entries using the existing data structure interface between
the firmware and kernel.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.940.382.4214

Re: [PATCH] x86 boot: Pass E820 memory map entries more than 128 via linked list of setup data

[Huang, Ying]

On Mon, 2008-06-23 at 01:53 -0500, Paul Jackson wrote:
> Huang Ying wrote:
> > With this patch, your previous patch:
> > 
> > x86 boot: add code to add BIOS provided EFI memory entries to kernel
> > 
> > is not necessary, so can be reversed.
> > 
> > What do you think about that?
> 
> ... the same thing I thought about it when you asked this question
> last month, in post http://lkml.org/lkml/2008/5/26/307.
> 
> Please see my reply in http://lkml.org/lkml/2008/5/28/59.
> It was lengthy and carefully researched, so I won't repeat
> it here.
> 
> In short, we still need it.  The key kernel routine that adds
> additional EFI entries to the existing E820 map is just 20 or so
> fairly easy lines.  We agree that it doesn't handle some of what your
> more extended work with a linked list of setup data handles (such as
> additional EDD entries), but it does handle additional memory map
> (node) entries using the existing data structure interface between
> the firmware and kernel.

EFI memmap based code versus E820 EXT based code:

1. Copying from EFI memory map to E820 map in kernel is not necessary,
because now it can be done in boot-loader with extended E820 memory map
via linked list of setup data.

2. EFI memmap based code only works when CONFIG_EFI is defined, that is
EFI runtime service is enabled. If you do not want to enable EFI runtime
services, you lose some memory map entries. Considering that kexec does
not support EFI runtime service so far.

3. EFI memmap based code is EFI specific, while E820 EXT based code is
general. It can be used for such as legacy BIOS, LinuxBIOS, kexec, etc.

4. Current EFI memmap based code does not work properly in all
situation, for example it can not works with kernel parameter:
"memmap=exactmap, memmap=<xxx>, ...", "mem=<xxx>" or "noefi".

So, I think it is better to remove "EFI memmap based code".

Best Regards,
Huang Ying

Re: [PATCH] x86 boot: Pass E820 memory map entries more than 128 via linked list of setup data

[Paul Jackson]

Huang Ying wrote:
> So, I think it is better to remove "EFI memmap based code".

You give good reasons for -adding- E820 EXT code.  Fine.

You give no reason for -removing- the EFI memmap based code,
except the implicit (unstated) reason that we should only
support a single mechanism.

However the kernel routinely supports a variety of mechanisms
for various BIOS firmware, as it should.

Internally, within the kernel, when it is entirely within the
kernels control and when there is no externally visible kernel
interface affected, we routinely strive to minimize redundant
mechanisms, as we should.

But externally, such as in supporting various boot firmware
protocols, we routinely support multiple useful interfaces.

If that EFI memmap based code for > 128 nodes is causing you
no problem, then please leave it be.  It is providing us good
benefit.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.940.382.4214

Re: [PATCH] x86 boot: Pass E820 memory map entries more than 128 via linked list of setup data

[Huang, Ying]

On Mon, 2008-06-23 at 03:47 -0500, Paul Jackson wrote:
> Huang Ying wrote:
> > So, I think it is better to remove "EFI memmap based code".
> 
> You give good reasons for -adding- E820 EXT code.  Fine.
> 
> You give no reason for -removing- the EFI memmap based code,
> except the implicit (unstated) reason that we should only
> support a single mechanism.
> 
> However the kernel routinely supports a variety of mechanisms
> for various BIOS firmware, as it should.
> 
> Internally, within the kernel, when it is entirely within the
> kernels control and when there is no externally visible kernel
> interface affected, we routinely strive to minimize redundant
> mechanisms, as we should.
> 
> But externally, such as in supporting various boot firmware
> protocols, we routinely support multiple useful interfaces.
> 
> If that EFI memmap based code for > 128 nodes is causing you
> no problem, then please leave it be.  It is providing us good
> benefit.

Please fix the following issue, if it is agreed to keep this redundant
code in kernel:

4. Current EFI memmap based code does not work properly in all
situation, for example it can not works with kernel parameter:
"memmap=exactmap, memmap=<xxx>, ...", "mem=<xxx>" or "noefi".

Best Regards,
Huang Ying

Re: [PATCH] x86 boot: Pass E820 memory map entries more than 128 via linked list of setup data

[Paul Jackson]

Huang wrote:
> 4. Current EFI memmap based code does not work properly in all
> situation, for example it can not works with kernel parameter:
> "memmap=exactmap, memmap=<xxx>, ...", "mem=<xxx>" or "noefi".

With "noefi" parameter, my EFI memmap based code is not supposed
to do anything.  The "noefi" parameter asks the kernel to ignore
any EFI support in the firmware it is booting from.

Could you tell me more what you mean by "does not work properly?"

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.940.382.4214

Re: [PATCH] x86 boot: Pass E820 memory map entries more than 128 via linked list of setup data

[Huang, Ying]

On Mon, 2008-06-23 at 04:48 -0500, Paul Jackson wrote:
> Huang wrote:
> > 4. Current EFI memmap based code does not work properly in all
> > situation, for example it can not works with kernel parameter:
> > "memmap=exactmap, memmap=<xxx>, ...", "mem=<xxx>" or "noefi".
> 
> With "noefi" parameter, my EFI memmap based code is not supposed
> to do anything.  The "noefi" parameter asks the kernel to ignore
> any EFI support in the firmware it is booting from.

"noefi" is used to specify that the EFI runtime services should be
disabled in kernel. But the memmap should be complete.

> Could you tell me more what you mean by "does not work properly?"

OK. It is OK for your code with "noefi". The remaining issues:

If "memmap=exactmap memmap=<xxx>" is specified in kernel command line.
The user defined memmap should override that from firmware. But your
code is executed after the user defined memmap is parsed, so the memmap
from firmware will override that from user. This does not conform the
semantics of "memmap=exactmap ...". Same issue for "mem=<xxx>".

Another issue is that the size of E820 memmap required on EFI system
must be two times bigger than that really needed. Because at first the
E820 memmap is filled with the entries from E820 and E820_EXT, then that
from EFI memmap is appended.

Best Regards,
Huang Ying

Re: [PATCH] x86 boot: Pass E820 memory map entries more than 128 via linked list of setup data

[H. Peter Anvin]

Huang, Ying wrote:
> On Mon, 2008-06-23 at 04:48 -0500, Paul Jackson wrote:
>> Huang wrote:
>>> 4. Current EFI memmap based code does not work properly in all
>>> situation, for example it can not works with kernel parameter:
>>> "memmap=exactmap, memmap=<xxx>, ...", "mem=<xxx>" or "noefi".
>> With "noefi" parameter, my EFI memmap based code is not supposed
>> to do anything.  The "noefi" parameter asks the kernel to ignore
>> any EFI support in the firmware it is booting from.
> 
> "noefi" is used to specify that the EFI runtime services should be
> disabled in kernel. But the memmap should be complete.
> 
>> Could you tell me more what you mean by "does not work properly?"
> 
> OK. It is OK for your code with "noefi". The remaining issues:
> 
> If "memmap=exactmap memmap=<xxx>" is specified in kernel command line.
> The user defined memmap should override that from firmware. But your
> code is executed after the user defined memmap is parsed, so the memmap
> from firmware will override that from user. This does not conform the
> semantics of "memmap=exactmap ...". Same issue for "mem=<xxx>".
> 
> Another issue is that the size of E820 memmap required on EFI system
> must be two times bigger than that really needed. Because at first the
> E820 memmap is filled with the entries from E820 and E820_EXT, then that
> from EFI memmap is appended.
> 

Hello,

I discussed this with Ingo earlier today, and we came to the following 
conclusion:

1. The EFI memmap code as a backup to the bootloader is fine.
2. Ying's memmap= objection needs to be addressed.  Violating user 
overrides is not appropriate.
3. It is important that we don't override the bootloader when the 
bootloader really does know best.  For example, kexec may want to 
control exactly what memory the target kernel uses.  As a result, we 
need a flag somewhere to disable *any* attempts at obtaining memory 
information from the environment, be it EFI, OpenFirmware or what have 
you.  The easiest way to do this is probably via a command-line flag, 
e.g. "noauxmem".

What do you guys think?

	-hpa

Re: [PATCH] x86 boot: Pass E820 memory map entries more than 128 via linked list of setup data

[Huang, Ying]

On Mon, 2008-06-23 at 22:45 -0700, H. Peter Anvin wrote:
> Huang, Ying wrote:
> > On Mon, 2008-06-23 at 04:48 -0500, Paul Jackson wrote:
> >> Huang wrote:
> >>> 4. Current EFI memmap based code does not work properly in all
> >>> situation, for example it can not works with kernel parameter:
> >>> "memmap=exactmap, memmap=<xxx>, ...", "mem=<xxx>" or "noefi".
> >> With "noefi" parameter, my EFI memmap based code is not supposed
> >> to do anything.  The "noefi" parameter asks the kernel to ignore
> >> any EFI support in the firmware it is booting from.
> > 
> > "noefi" is used to specify that the EFI runtime services should be
> > disabled in kernel. But the memmap should be complete.
> > 
> >> Could you tell me more what you mean by "does not work properly?"
> > 
> > OK. It is OK for your code with "noefi". The remaining issues:
> > 
> > If "memmap=exactmap memmap=<xxx>" is specified in kernel command line.
> > The user defined memmap should override that from firmware. But your
> > code is executed after the user defined memmap is parsed, so the memmap
> > from firmware will override that from user. This does not conform the
> > semantics of "memmap=exactmap ...". Same issue for "mem=<xxx>".
> > 
> > Another issue is that the size of E820 memmap required on EFI system
> > must be two times bigger than that really needed. Because at first the
> > E820 memmap is filled with the entries from E820 and E820_EXT, then that
> > from EFI memmap is appended.
> > 
> 
> Hello,
> 
> I discussed this with Ingo earlier today, and we came to the following 
> conclusion:
> 
> 1. The EFI memmap code as a backup to the bootloader is fine.
> 2. Ying's memmap= objection needs to be addressed.  Violating user 
> overrides is not appropriate.
> 3. It is important that we don't override the bootloader when the 
> bootloader really does know best.  For example, kexec may want to 
> control exactly what memory the target kernel uses.  As a result, we 
> need a flag somewhere to disable *any* attempts at obtaining memory 
> information from the environment, be it EFI, OpenFirmware or what have 
> you.  The easiest way to do this is probably via a command-line flag, 
> e.g. "noauxmem".
> 
> What do you guys think?

I think it is better to add a command-line flag "auxmem", the EFI memmap
is only used when this flag is set. This has better back-compatibility. 

Best Regards,
Huang Ying