[Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[Mike Travis]

Currently, the e820_reserve_resources() function does not add entries
obtained via the "add_efi_memmap" kernel cmdline option.  This causes
/sys/firmware/memmap/... to be incomplete (stops after 128 entries).
Utilities that examine these entries then do not get the complete
picture of system memory.

This patch causes the above function to use the e820 memmap instead
of the e820_saved memmap if "add_efi_memmap" cmdline option is
specified.

Signed-off-by: Mike Travis <travis@sgi.com>
Signed-off-by: Jack Steiner <steiner@sgi.com>
---
 arch/x86/include/asm/e820.h |    1 +
 arch/x86/kernel/e820.c      |    8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

--- linux.orig/arch/x86/include/asm/e820.h
+++ linux/arch/x86/include/asm/e820.h
@@ -89,6 +89,7 @@ extern int e820_search_gap(unsigned long
 			unsigned long start_addr, unsigned long long end_addr);
 struct setup_data;
 extern void parse_e820_ext(struct setup_data *data, unsigned long pa_data);
+extern int add_efi_memmap;
 
 #if defined(CONFIG_X86_64) || \
 	(defined(CONFIG_X86_32) && defined(CONFIG_HIBERNATION))
--- linux.orig/arch/x86/kernel/e820.c
+++ linux/arch/x86/kernel/e820.c
@@ -1074,6 +1074,7 @@ void __init e820_reserve_resources(void)
 	int i;
 	struct resource *res;
 	u64 end;
+	struct e820map *e_map = &e820_saved;
 
 	res = alloc_bootmem(sizeof(struct resource) * e820.nr_map);
 	e820_res = res;
@@ -1101,8 +1102,11 @@ void __init e820_reserve_resources(void)
 		res++;
 	}
 
-	for (i = 0; i < e820_saved.nr_map; i++) {
-		struct e820entry *entry = &e820_saved.map[i];
+	if (add_efi_memmap)
+		e_map = &e820;
+
+	for (i = 0; i < e_map->nr_map; i++) {
+		struct e820entry *entry = &e_map->map[i];
 		firmware_map_add_early(entry->addr,
 			entry->addr + entry->size - 1,
 			e820_type_to_string(entry->type));

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[H. Peter Anvin]

On 05/12/2010 11:10 AM, Mike Travis wrote:
> 
> Currently, the e820_reserve_resources() function does not add entries
> obtained via the "add_efi_memmap" kernel cmdline option.  This causes
> /sys/firmware/memmap/... to be incomplete (stops after 128 entries).
> Utilities that examine these entries then do not get the complete
> picture of system memory.
> 
> This patch causes the above function to use the e820 memmap instead
> of the e820_saved memmap if "add_efi_memmap" cmdline option is
> specified.
> 
> Signed-off-by: Mike Travis <travis@sgi.com>
> Signed-off-by: Jack Steiner <steiner@sgi.com>

If I'm not mistaken, the very reason for the e820 vs e820_saved map is
that the latter is supposed to reflect the firmware report, whereas the
former is subject to be modified by the kernel.  As this is actually a
reflection of the firmware (although it would be better if you could fix
the bootloader instead of adding hacks in the kernel...) it really
should go into e820_saved as well as e820.  Displaying the adjusted e820
map doesn't seem appropriate under any circumstances.

	-hpa

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[Yinghai]

On 05/12/2010 11:55 AM, H. Peter Anvin wrote:
> On 05/12/2010 11:10 AM, Mike Travis wrote:
>>
>> Currently, the e820_reserve_resources() function does not add entries
>> obtained via the "add_efi_memmap" kernel cmdline option.  This causes
>> /sys/firmware/memmap/... to be incomplete (stops after 128 entries).
>> Utilities that examine these entries then do not get the complete
>> picture of system memory.
>>
>> This patch causes the above function to use the e820 memmap instead
>> of the e820_saved memmap if "add_efi_memmap" cmdline option is
>> specified.
>>
>> Signed-off-by: Mike Travis <travis@sgi.com>
>> Signed-off-by: Jack Steiner <steiner@sgi.com>
> 
> If I'm not mistaken, the very reason for the e820 vs e820_saved map is
> that the latter is supposed to reflect the firmware report, whereas the
> former is subject to be modified by the kernel.  As this is actually a
> reflection of the firmware (although it would be better if you could fix
> the bootloader instead of adding hacks in the kernel...) it really
> should go into e820_saved as well as e820.  Displaying the adjusted e820
> map doesn't seem appropriate under any circumstances.

Yes, you are right.

We should not touch e820_saved and keep /sys/firmware/memmap to be consistent with it.

YH

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[Mike Travis]

Yinghai wrote:
> On 05/12/2010 11:55 AM, H. Peter Anvin wrote:
>> On 05/12/2010 11:10 AM, Mike Travis wrote:
>>> Currently, the e820_reserve_resources() function does not add entries
>>> obtained via the "add_efi_memmap" kernel cmdline option.  This causes
>>> /sys/firmware/memmap/... to be incomplete (stops after 128 entries).
>>> Utilities that examine these entries then do not get the complete
>>> picture of system memory.
>>>
>>> This patch causes the above function to use the e820 memmap instead
>>> of the e820_saved memmap if "add_efi_memmap" cmdline option is
>>> specified.
>>>
>>> Signed-off-by: Mike Travis <travis@sgi.com>
>>> Signed-off-by: Jack Steiner <steiner@sgi.com>
>> If I'm not mistaken, the very reason for the e820 vs e820_saved map is
>> that the latter is supposed to reflect the firmware report, whereas the
>> former is subject to be modified by the kernel.  As this is actually a
>> reflection of the firmware (although it would be better if you could fix
>> the bootloader instead of adding hacks in the kernel...) it really
>> should go into e820_saved as well as e820.  Displaying the adjusted e820
>> map doesn't seem appropriate under any circumstances.
> 
> Yes, you are right.
> 
> We should not touch e820_saved and keep /sys/firmware/memmap to be consistent with it.
> 
> YH


I'm confused.  Should I:

 - copy the extra memmap entries into e820_saved and fill early_memmap from that?
or
 - don't touch e820_saved and use e820 to fill early_memmap (which is how it is now).

Thanks,
Mike

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[Mike Travis]

Mike Travis wrote:
> 
> 
> Yinghai wrote:
>> On 05/12/2010 11:55 AM, H. Peter Anvin wrote:
>>> On 05/12/2010 11:10 AM, Mike Travis wrote:
>>>> Currently, the e820_reserve_resources() function does not add entries
>>>> obtained via the "add_efi_memmap" kernel cmdline option.  This causes
>>>> /sys/firmware/memmap/... to be incomplete (stops after 128 entries).
>>>> Utilities that examine these entries then do not get the complete
>>>> picture of system memory.
>>>>
>>>> This patch causes the above function to use the e820 memmap instead
>>>> of the e820_saved memmap if "add_efi_memmap" cmdline option is
>>>> specified.
>>>>
>>>> Signed-off-by: Mike Travis <travis@sgi.com>
>>>> Signed-off-by: Jack Steiner <steiner@sgi.com>
>>> If I'm not mistaken, the very reason for the e820 vs e820_saved map is
>>> that the latter is supposed to reflect the firmware report, whereas the
>>> former is subject to be modified by the kernel.  As this is actually a
>>> reflection of the firmware (although it would be better if you could fix
>>> the bootloader instead of adding hacks in the kernel...) it really
>>> should go into e820_saved as well as e820.  Displaying the adjusted e820
>>> map doesn't seem appropriate under any circumstances.
>>
>> Yes, you are right.
>>
>> We should not touch e820_saved and keep /sys/firmware/memmap to be 
>> consistent with it.
>>
>> YH
> 

Here's where it gets confusing:

/*
 * The e820 map is the map that gets modified e.g. with command line parameters
 * and that is also registered with modifications in the kernel resource tree
 * with the iomem_resource as parent.
 *
 * The e820_saved is directly saved after the BIOS-provided memory map is
 * copied. It doesn't get modified afterwards. It's registered for the
 * /sys/firmware/memmap interface.
 *
 * That memory map is not modified and is used as base for kexec. The kexec'd
 * kernel should get the same memory map as the firmware provides. Then the
 * user can e.g. boot the original kernel with mem=1G while still booting the
 * next kernel with full memory.
 */
struct e820map e820;
struct e820map e820_saved;


It specifically mentions that kexec needs the unmodified address map.  But
we know it does not work if it does not have the "extra memmap entries"
from BIOS (added with option "add_efi_memmap".)

So in essence I see it as a lie that e820_saved contains the memmap
provided by the firmware.  It clearly does not.  It is missing all
the entries greater than 128.

So the question remains, should /sys/firmware/memmap be provisioned
from e820_saved with changes to add to that map the extra memmap
entries?  Or should it be provisioned from the updated e820 map
that is also printed by e820_print_map()?  

The output to the console and the contents of /sys/firmware/memmap
should match, and this is what this patch was intending to do.
/sys/firmware/memmap should not be truncated due to legacy BIOS
limitations.

If the e820 memmap changes further due to other circumstances,
that should not change the mappings under /sys/firmware/memmap?
Unless I'm missing something here, I don't see the downside.

So which should it be?

Thanks,
Mike

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[Yinghai]

On 05/13/2010 02:18 PM, Mike Travis wrote:
> 
> 
> Mike Travis wrote:
>>
>>
>> Yinghai wrote:
>>> On 05/12/2010 11:55 AM, H. Peter Anvin wrote:
>>>> On 05/12/2010 11:10 AM, Mike Travis wrote:
>>>>> Currently, the e820_reserve_resources() function does not add entries
>>>>> obtained via the "add_efi_memmap" kernel cmdline option.  This causes
>>>>> /sys/firmware/memmap/... to be incomplete (stops after 128 entries).
>>>>> Utilities that examine these entries then do not get the complete
>>>>> picture of system memory.
>>>>>
>>>>> This patch causes the above function to use the e820 memmap instead
>>>>> of the e820_saved memmap if "add_efi_memmap" cmdline option is
>>>>> specified.
>>>>>
>>>>> Signed-off-by: Mike Travis <travis@sgi.com>
>>>>> Signed-off-by: Jack Steiner <steiner@sgi.com>
>>>> If I'm not mistaken, the very reason for the e820 vs e820_saved map is
>>>> that the latter is supposed to reflect the firmware report, whereas the
>>>> former is subject to be modified by the kernel.  As this is actually a
>>>> reflection of the firmware (although it would be better if you could
>>>> fix
>>>> the bootloader instead of adding hacks in the kernel...) it really
>>>> should go into e820_saved as well as e820.  Displaying the adjusted
>>>> e820
>>>> map doesn't seem appropriate under any circumstances.
>>>
>>> Yes, you are right.
>>>
>>> We should not touch e820_saved and keep /sys/firmware/memmap to be
>>> consistent with it.
>>>
>>> YH
>>
> 
> Here's where it gets confusing:
> 
> /*
> * The e820 map is the map that gets modified e.g. with command line
> parameters
> * and that is also registered with modifications in the kernel resource
> tree
> * with the iomem_resource as parent.
> *
> * The e820_saved is directly saved after the BIOS-provided memory map is
> * copied. It doesn't get modified afterwards. It's registered for the
> * /sys/firmware/memmap interface.
> *
> * That memory map is not modified and is used as base for kexec. The
> kexec'd
> * kernel should get the same memory map as the firmware provides. Then the
> * user can e.g. boot the original kernel with mem=1G while still booting
> the
> * next kernel with full memory.
> */
> struct e820map e820;
> struct e820map e820_saved;
> 
> 
> It specifically mentions that kexec needs the unmodified address map.  But
> we know it does not work if it does not have the "extra memmap entries"
> from BIOS (added with option "add_efi_memmap".)
> 
> So in essence I see it as a lie that e820_saved contains the memmap
> provided by the firmware.  It clearly does not.  It is missing all
> the entries greater than 128.
> 
> So the question remains, should /sys/firmware/memmap be provisioned
> from e820_saved with changes to add to that map the extra memmap
> entries?  Or should it be provisioned from the updated e820 map
> that is also printed by e820_print_map()? 
> The output to the console and the contents of /sys/firmware/memmap
> should match, and this is what this patch was intending to do.
> /sys/firmware/memmap should not be truncated due to legacy BIOS
> limitations.
> 
> If the e820 memmap changes further due to other circumstances,
> that should not change the mappings under /sys/firmware/memmap?
> Unless I'm missing something here, I don't see the downside.
> 
> So which should it be?

in setup.c::setup_arch()

        setup_memory_map();
        parse_setup_data();
        /* update the e820_saved too */
        e820_reserve_setup_data();
	...
	parse_early_param();
	...
	finish_e820_parsing();



efi memmap is appended to e820 by parse_setup_data
e820_reserve_setup_data() will copy e820 to e820_saved.


or do you have old boot loader 

        if (boot_params.hdr.version < 0x0209)
                return;

YH

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[Mike Travis]

...
> in setup.c::setup_arch()
> 
>         setup_memory_map();
>         parse_setup_data();
>         /* update the e820_saved too */
>         e820_reserve_setup_data();
> 	...
> 	parse_early_param();
> 	...
> 	finish_e820_parsing();
> 
> 
> 
> efi memmap is appended to e820 by parse_setup_data
> e820_reserve_setup_data() will copy e820 to e820_saved.
> 
> 
> or do you have old boot loader 
> 
>         if (boot_params.hdr.version < 0x0209)
>                 return;
> 
> YH

I saw that too, and wondered why e820_saved did not
have the extra entries.   The comment indicates it
should.

I'm on the system tonight and will investigate this
further.

Thanks!
Mike

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[H. Peter Anvin]

On 05/13/2010 02:55 PM, Mike Travis wrote:
> 
> I saw that too, and wondered why e820_saved did not
> have the extra entries.   The comment indicates it
> should.
> 
> I'm on the system tonight and will investigate this
> further.
> 

e820_saved lacks the extra entries because they aren't being passed in
from the bootloader, as they should, and instead you're using
add_efi_memmap which is, as far as the kernel is concerned, a post-boot
modification.

That being said, add_efi_memmap does come from the firmware, and as such
it would be legitimate for it to add them to e820_saved.

	-hpa

[Patch 1/1] x86 efi: insert add_efi_memmap entries into both e820 maps

[Mike Travis]

Currently, the e820_reserve_resources() function does not add entries
obtained via the "add_efi_memmap" kernel cmdline option.  This causes
/sys/firmware/memmap/... to be incomplete (stops after 128 entries).
Utilities that examine these entries then do not get the complete
picture of system memory.

This patch causes the add_efi_memmap function to add the memmap entries
to both the e820 map and the e820_saved map.

Signed-off-by: Mike Travis <travis@sgi.com>
Signed-off-by: Jack Steiner <steiner@sgi.com>
---
 arch/x86/include/asm/e820.h |    1 +
 arch/x86/kernel/e820-xen.c  |    5 +++++
 arch/x86/kernel/e820.c      |   11 ++++++++---
 arch/x86/kernel/efi.c       |    3 +++
 4 files changed, 17 insertions(+), 3 deletions(-)

--- linux-2.6.32.orig/arch/x86/include/asm/e820.h
+++ linux-2.6.32/arch/x86/include/asm/e820.h
@@ -70,6 +70,7 @@ extern unsigned long pci_mem_start;
 extern int e820_any_mapped(u64 start, u64 end, unsigned type);
 extern int e820_all_mapped(u64 start, u64 end, unsigned type);
 extern void e820_add_region(u64 start, u64 size, int type);
+extern void e820_saved_add_region(u64 start, u64 size, int type);
 extern void e820_print_map(char *who);
 extern int
 sanitize_e820_map(struct e820entry *biosmap, int max_nr_map, u32 *pnr_map);
--- linux-2.6.32.orig/arch/x86/kernel/e820-xen.c
+++ linux-2.6.32/arch/x86/kernel/e820-xen.c
@@ -150,6 +150,11 @@ void __init e820_add_region(u64 start, u
 	__e820_add_region(&e820, start, size, type);
 }
 
+void __init e820_saved_add_region(u64 start, u64 size, int type)
+{
+	__e820_add_region(&e820_saved, start, size, type);
+}
+
 static void __init e820_print_type(u32 type)
 {
 	switch (type) {
--- linux-2.6.32.orig/arch/x86/kernel/e820.c
+++ linux-2.6.32/arch/x86/kernel/e820.c
@@ -33,9 +33,9 @@
  * and that is also registered with modifications in the kernel resource tree
  * with the iomem_resource as parent.
  *
- * The e820_saved is directly saved after the BIOS-provided memory map is
- * copied. It doesn't get modified afterwards. It's registered for the
- * /sys/firmware/memmap interface.
+ * The e820_saved is saved after the BIOS-provided memory map is copied as
+ * well as the optional add_efi_memmap entries are processed.  It doesn't get
+ * modified afterwards. It's registered for the /sys/firmware/memmap interface.
  *
  * That memory map is not modified and is used as base for kexec. The kexec'd
  * kernel should get the same memory map as the firmware provides. Then the
@@ -132,6 +132,11 @@ void __init e820_add_region(u64 start, u
 	__e820_add_region(&e820, start, size, type);
 }
 
+void __init e820_saved_add_region(u64 start, u64 size, int type)
+{
+	__e820_add_region(&e820_saved, start, size, type);
+}
+
 static void __init e820_print_type(u32 type)
 {
 	switch (type) {
--- linux-2.6.32.orig/arch/x86/kernel/efi.c
+++ linux-2.6.32/arch/x86/kernel/efi.c
@@ -271,8 +271,11 @@ static void __init do_add_efi_memmap(voi
 			break;
 		}
 		e820_add_region(start, size, e820_type);
+		e820_saved_add_region(start, size, e820_type);
 	}
 	sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
+	sanitize_e820_map(e820_saved.map, ARRAY_SIZE(e820_saved.map),
+							&e820_saved.nr_map);
 }
 
 void __init efi_reserve_early(void)

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[Mike Travis]

H. Peter Anvin wrote:
> On 05/13/2010 02:55 PM, Mike Travis wrote:
>> I saw that too, and wondered why e820_saved did not
>> have the extra entries.   The comment indicates it
>> should.
>>
>> I'm on the system tonight and will investigate this
>> further.
>>
> 
> e820_saved lacks the extra entries because they aren't being passed in
> from the bootloader, as they should, and instead you're using
> add_efi_memmap which is, as far as the kernel is concerned, a post-boot
> modification.
> 
> That being said, add_efi_memmap does come from the firmware, and as such
> it would be legitimate for it to add them to e820_saved.
> 
> 	-hpa

Did this last patch meet expectations?

http://marc.info/?l=linux-kernel&m=127474230623061&w=4

Thanks,
Mike

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[H. Peter Anvin]

On 05/25/2010 03:34 PM, Mike Travis wrote:
> 
> 
> H. Peter Anvin wrote:
>> On 05/13/2010 02:55 PM, Mike Travis wrote:
>>> I saw that too, and wondered why e820_saved did not
>>> have the extra entries.   The comment indicates it
>>> should.
>>>
>>> I'm on the system tonight and will investigate this
>>> further.
>>>
>>
>> e820_saved lacks the extra entries because they aren't being passed in
>> from the bootloader, as they should, and instead you're using
>> add_efi_memmap which is, as far as the kernel is concerned, a post-boot
>> modification.
>>
>> That being said, add_efi_memmap does come from the firmware, and as such
>> it would be legitimate for it to add them to e820_saved.
>>
>> 	-hpa
> 
> Did this last patch meet expectations?
> 
> http://marc.info/?l=linux-kernel&m=127474230623061&w=4
> 

I'm concerned about calling sanitize_e820_map() on e820_saved; it is
supposed to reflect the raw data as reported by the source, and
sanitizing it would corrupt that.

	-hpa

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[Mike Travis]

H. Peter Anvin wrote:
> On 05/25/2010 03:34 PM, Mike Travis wrote:
>>
>> H. Peter Anvin wrote:
>>> On 05/13/2010 02:55 PM, Mike Travis wrote:
>>>> I saw that too, and wondered why e820_saved did not
>>>> have the extra entries.   The comment indicates it
>>>> should.
>>>>
>>>> I'm on the system tonight and will investigate this
>>>> further.
>>>>
>>> e820_saved lacks the extra entries because they aren't being passed in
>>> from the bootloader, as they should, and instead you're using
>>> add_efi_memmap which is, as far as the kernel is concerned, a post-boot
>>> modification.
>>>
>>> That being said, add_efi_memmap does come from the firmware, and as such
>>> it would be legitimate for it to add them to e820_saved.
>>>
>>> 	-hpa
>> Did this last patch meet expectations?
>>
>> http://marc.info/?l=linux-kernel&m=127474230623061&w=4
>>
> 
> I'm concerned about calling sanitize_e820_map() on e820_saved; it is
> supposed to reflect the raw data as reported by the source, and
> sanitizing it would corrupt that.
> 
> 	-hpa

I wondered about that. Sanitize seems to remove adjacent
entries, etc. making the map smaller, but I couldn't detect
any real differences (though admittedly I didn't do a byte
by byte comparison.)

But I'll submit another with that call removed.

Thanks,
Mike

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[H. Peter Anvin]

On 05/25/2010 03:46 PM, Mike Travis wrote:
> 
> I wondered about that. Sanitize seems to remove adjacent
> entries, etc. making the map smaller, but I couldn't detect
> any real differences (though admittedly I didn't do a byte
> by byte comparison.)
> 
> But I'll submit another with that call removed.
> 

Sanitize in particular tries to make sense of an otherwise-messed-up
map.  This means that if we find a system where it does something
broken, we don't have a way to get the raw data to check what it did!

	-hpa

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[Yinghai Lu]

On Tue, May 25, 2010 at 3:46 PM, Mike Travis <travis@sgi.com> wrote:
>
>
> H. Peter Anvin wrote:
>>
>> On 05/25/2010 03:34 PM, Mike Travis wrote:
>>>
>>> H. Peter Anvin wrote:
>>>>
>>>> On 05/13/2010 02:55 PM, Mike Travis wrote:
>>>>>
>>>>> I saw that too, and wondered why e820_saved did not
>>>>> have the extra entries.   The comment indicates it
>>>>> should.
>>>>>
>>>>> I'm on the system tonight and will investigate this
>>>>> further.
>>>>>
>>>> e820_saved lacks the extra entries because they aren't being passed in
>>>> from the bootloader, as they should, and instead you're using
>>>> add_efi_memmap which is, as far as the kernel is concerned, a post-boot
>>>> modification.
>>>>
>>>> That being said, add_efi_memmap does come from the firmware, and as such
>>>> it would be legitimate for it to add them to e820_saved.
>>>>
>>>>        -hpa
>>>
>>> Did this last patch meet expectations?
>>>
>>> http://marc.info/?l=linux-kernel&m=127474230623061&w=4
>>>
>>
>> I'm concerned about calling sanitize_e820_map() on e820_saved; it is
>> supposed to reflect the raw data as reported by the source, and
>> sanitizing it would corrupt that.
>>
>>        -hpa
>
> I wondered about that. Sanitize seems to remove adjacent
> entries, etc. making the map smaller, but I couldn't detect
> any real differences (though admittedly I didn't do a byte
> by byte comparison.)
>
> But I'll submit another with that call removed.

can you use updated boot loader instead?

Also we should drop add_efi_memmap if possible.

YH

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[Mike Travis]

Yinghai Lu wrote:
> On Tue, May 25, 2010 at 3:46 PM, Mike Travis <travis@sgi.com> wrote:
>>
>> H. Peter Anvin wrote:
>>> On 05/25/2010 03:34 PM, Mike Travis wrote:
>>>> H. Peter Anvin wrote:
>>>>> On 05/13/2010 02:55 PM, Mike Travis wrote:
>>>>>> I saw that too, and wondered why e820_saved did not
>>>>>> have the extra entries.   The comment indicates it
>>>>>> should.
>>>>>>
>>>>>> I'm on the system tonight and will investigate this
>>>>>> further.
>>>>>>
>>>>> e820_saved lacks the extra entries because they aren't being passed in
>>>>> from the bootloader, as they should, and instead you're using
>>>>> add_efi_memmap which is, as far as the kernel is concerned, a post-boot
>>>>> modification.
>>>>>
>>>>> That being said, add_efi_memmap does come from the firmware, and as such
>>>>> it would be legitimate for it to add them to e820_saved.
>>>>>
>>>>>        -hpa
>>>> Did this last patch meet expectations?
>>>>
>>>> http://marc.info/?l=linux-kernel&m=127474230623061&w=4
>>>>
>>> I'm concerned about calling sanitize_e820_map() on e820_saved; it is
>>> supposed to reflect the raw data as reported by the source, and
>>> sanitizing it would corrupt that.
>>>
>>>        -hpa
>> I wondered about that. Sanitize seems to remove adjacent
>> entries, etc. making the map smaller, but I couldn't detect
>> any real differences (though admittedly I didn't do a byte
>> by byte comparison.)
>>
>> But I'll submit another with that call removed.
> 
> can you use updated boot loader instead?
> 
> Also we should drop add_efi_memmap if possible.
> 
> YH

I'm open for suggestions on how to improve this, but we are shipping
systems very soon and I don't think we'll get any other change into
the system until the next update.

Thanks,
Mike

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[H. Peter Anvin]

On 05/26/2010 10:42 AM, Mike Travis wrote:
> 
> I'm open for suggestions on how to improve this, but we are shipping
> systems very soon and I don't think we'll get any other change into
> the system until the next update.
> 

You know you've been using that excuse for two years when it comes to
not fixing the bootloader, and I'm starting to be a bit frustrated with
that.

	-hpa

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[Mike Travis]

H. Peter Anvin wrote:
> On 05/26/2010 10:42 AM, Mike Travis wrote:
>> I'm open for suggestions on how to improve this, but we are shipping
>> systems very soon and I don't think we'll get any other change into
>> the system until the next update.
>>
> 
> You know you've been using that excuse for two years when it comes to
> not fixing the bootloader, and I'm starting to be a bit frustrated with
> that.
> 
> 	-hpa

I didn't know bootloader had a problem two years ago  And as I mentioned,
I'm open to suggestions but what I've heard is "fix the bootloader". What
I don't know is how to do that.

I see that BIOS sets up an e820 compatible memory map and extra memmap
entries "somewhere".  EFI seems to pick this up via ELILO and somehow
the kernel finds these extra memmap entries.

So my question is what is broken?  Should the bootloader not use the
standard e820 memmap at all, and invent another way of passing the
memmap entries?  And what about EFI grub, shouldn't that also change?
And how does this affect compatibility?

I'm in the dark here as to how the bootloader manages to pass these
entries from BIOS to the kernel, and how far I can go modifying these
things when I don't completely understand them.  The last problem I
worked on in ELILO I couldn't even print messages as that invalidated
the "magic key" and it would loop endlessly between ELILO and BIOS.

The other thing is priorities.  If something is working and not
causing undue problems, it does not get higher priority than issues
that cause complete system failure.  I've suggested a workable
(maybe short term solution), that gets us past the boot, and on to
the other potentially far more important problems.

And p.s. it's been way longer than two years... ;-)  [the other thing
to consider is we haven't had real hardware that long, and many, many,
many problems have surfaced since then.  The memmap problem only
showed up when we ran over 128 entries.  When has that happened before?
And how was it handled then?] 

Thanks,
Mike

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[H. Peter Anvin]

On 05/26/2010 11:47 AM, Mike Travis wrote:
> 
> I didn't know bootloader had a problem two years ago  And as I mentioned,
> I'm open to suggestions but what I've heard is "fix the bootloader". What
> I don't know is how to do that.
> 
> I see that BIOS sets up an e820 compatible memory map and extra memmap
> entries "somewhere".  EFI seems to pick this up via ELILO and somehow
> the kernel finds these extra memmap entries.
> 
> So my question is what is broken?  Should the bootloader not use the
> standard e820 memmap at all, and invent another way of passing the
> memmap entries?  And what about EFI grub, shouldn't that also change?
> And how does this affect compatibility?
> 
> I'm in the dark here as to how the bootloader manages to pass these
> entries from BIOS to the kernel, and how far I can go modifying these
> things when I don't completely understand them.  The last problem I
> worked on in ELILO I couldn't even print messages as that invalidated
> the "magic key" and it would loop endlessly between ELILO and BIOS.
> 
> The other thing is priorities.  If something is working and not
> causing undue problems, it does not get higher priority than issues
> that cause complete system failure.  I've suggested a workable
> (maybe short term solution), that gets us past the boot, and on to
> the other potentially far more important problems.
> 
> And p.s. it's been way longer than two years... ;-)  [the other thing
> to consider is we haven't had real hardware that long, and many, many,
> many problems have surfaced since then.  The memmap problem only
> showed up when we ran over 128 entries.  When has that happened before?
> And how was it handled then?] 
> 

The 128-entry limit (caused by the fixed array in struct setup_info) was
brought to our attention in 2007.  At that time, a number of
alternatives were discussed; Linus in particular was adamant that this
should be handled in the bootloader, and not by the kernel.

The result was that we added a facility to the kernel to supply
arbitrary information via a linked list.  That has been in there since
at least early 2008.  However, noone who cares about this issue has
bothered stepping up and put the support into the bootloaders --
instead, we got suckered into accepting a "temporary hack" which only
has had the effect of giving you guys yet another excuse not to do what
you signed off to do in the first place.

Accordingly, the right answer for me to do is to veto this and any
further patches which enable you to skirt your responsibility, and I'm
getting damned close to doing so.

	-hpa

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[Mike Travis]

H. Peter Anvin wrote:
> On 05/26/2010 11:47 AM, Mike Travis wrote:
>> I didn't know bootloader had a problem two years ago  And as I mentioned,
>> I'm open to suggestions but what I've heard is "fix the bootloader". What
>> I don't know is how to do that.
>>
>> I see that BIOS sets up an e820 compatible memory map and extra memmap
>> entries "somewhere".  EFI seems to pick this up via ELILO and somehow
>> the kernel finds these extra memmap entries.
>>
>> So my question is what is broken?  Should the bootloader not use the
>> standard e820 memmap at all, and invent another way of passing the
>> memmap entries?  And what about EFI grub, shouldn't that also change?
>> And how does this affect compatibility?
>>
>> I'm in the dark here as to how the bootloader manages to pass these
>> entries from BIOS to the kernel, and how far I can go modifying these
>> things when I don't completely understand them.  The last problem I
>> worked on in ELILO I couldn't even print messages as that invalidated
>> the "magic key" and it would loop endlessly between ELILO and BIOS.
>>
>> The other thing is priorities.  If something is working and not
>> causing undue problems, it does not get higher priority than issues
>> that cause complete system failure.  I've suggested a workable
>> (maybe short term solution), that gets us past the boot, and on to
>> the other potentially far more important problems.
>>
>> And p.s. it's been way longer than two years... ;-)  [the other thing
>> to consider is we haven't had real hardware that long, and many, many,
>> many problems have surfaced since then.  The memmap problem only
>> showed up when we ran over 128 entries.  When has that happened before?
>> And how was it handled then?] 
>>
> 
> The 128-entry limit (caused by the fixed array in struct setup_info) was
> brought to our attention in 2007.  At that time, a number of
> alternatives were discussed; Linus in particular was adamant that this
> should be handled in the bootloader, and not by the kernel.
> 
> The result was that we added a facility to the kernel to supply
> arbitrary information via a linked list.  That has been in there since
> at least early 2008.  However, noone who cares about this issue has
> bothered stepping up and put the support into the bootloaders --
> instead, we got suckered into accepting a "temporary hack" which only
> has had the effect of giving you guys yet another excuse not to do what
> you signed off to do in the first place.
> 
> Accordingly, the right answer for me to do is to veto this and any
> further patches which enable you to skirt your responsibility, and I'm
> getting damned close to doing so.
> 
> 	-hpa


Ahh, so you aren't referring to me personally, but someone else?  I'll
do some digging into finding that mail thread to at least come up to
speed.  Had it been tossed to me in 2007 I would have dealt with it,
but it's on my todo list now.

Thanks,
Mike

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified.

[H. Peter Anvin]

On 05/26/2010 12:09 PM, Mike Travis wrote:
> 
> Ahh, so you aren't referring to me personally, but someone else?  I'll
> do some digging into finding that mail thread to at least come up to
> speed.  Had it been tossed to me in 2007 I would have dealt with it,
> but it's on my todo list now.
> 

I'm referring to the collective you as in "the community of
large-machine producers".

	-hpa

Re: [Patch 1/1] x86 efi: Fill all reserved memmap entries if add_efi_memmap specified v2

[Mike Travis]

Currently, the e820_reserve_resources() function does not add entries
obtained via the "add_efi_memmap" kernel cmdline option.  This causes
/sys/firmware/memmap/... to be incomplete (stops after 128 entries).
Utilities that examine these entries then do not get the complete
picture of system memory.

This patch causes the add_efi_memmap function to add the memmap entries
to both the e820 map and the e820_saved map.

Signed-off-by: Mike Travis <travis@sgi.com>
Signed-off-by: Jack Steiner <steiner@sgi.com>
---
v2: drop call to sanitize e820_saved map after adding efi_memmap
    entries.  (Note: it may still be "sanitized" via setup_arch->
    early_reserve_e820_mpc_new-> early_reserve_e820->update_e820_saved->
    sanitize_e820_map)
---
 arch/x86/include/asm/e820.h |    1 +
 arch/x86/kernel/e820-xen.c  |    5 +++++
 arch/x86/kernel/e820.c      |   11 ++++++++---
 arch/x86/kernel/efi.c       |    1 +
 4 files changed, 15 insertions(+), 3 deletions(-)

--- linux-2.6.32.orig/arch/x86/include/asm/e820.h
+++ linux-2.6.32/arch/x86/include/asm/e820.h
@@ -70,6 +70,7 @@ extern unsigned long pci_mem_start;
 extern int e820_any_mapped(u64 start, u64 end, unsigned type);
 extern int e820_all_mapped(u64 start, u64 end, unsigned type);
 extern void e820_add_region(u64 start, u64 size, int type);
+extern void e820_saved_add_region(u64 start, u64 size, int type);
 extern void e820_print_map(char *who);
 extern int
 sanitize_e820_map(struct e820entry *biosmap, int max_nr_map, u32 *pnr_map);
--- linux-2.6.32.orig/arch/x86/kernel/e820-xen.c
+++ linux-2.6.32/arch/x86/kernel/e820-xen.c
@@ -150,6 +150,11 @@ void __init e820_add_region(u64 start, u
 	__e820_add_region(&e820, start, size, type);
 }
 
+void __init e820_saved_add_region(u64 start, u64 size, int type)
+{
+	__e820_add_region(&e820_saved, start, size, type);
+}
+
 static void __init e820_print_type(u32 type)
 {
 	switch (type) {
--- linux-2.6.32.orig/arch/x86/kernel/e820.c
+++ linux-2.6.32/arch/x86/kernel/e820.c
@@ -33,9 +33,9 @@
  * and that is also registered with modifications in the kernel resource tree
  * with the iomem_resource as parent.
  *
- * The e820_saved is directly saved after the BIOS-provided memory map is
- * copied. It doesn't get modified afterwards. It's registered for the
- * /sys/firmware/memmap interface.
+ * The e820_saved is saved after the BIOS-provided memory map is copied as
+ * well as the optional add_efi_memmap entries are processed.  It doesn't get
+ * modified afterwards. It's registered for the /sys/firmware/memmap interface.
  *
  * That memory map is not modified and is used as base for kexec. The kexec'd
  * kernel should get the same memory map as the firmware provides. Then the
@@ -132,6 +132,11 @@ void __init e820_add_region(u64 start, u
 	__e820_add_region(&e820, start, size, type);
 }
 
+void __init e820_saved_add_region(u64 start, u64 size, int type)
+{
+	__e820_add_region(&e820_saved, start, size, type);
+}
+
 static void __init e820_print_type(u32 type)
 {
 	switch (type) {
--- linux-2.6.32.orig/arch/x86/kernel/efi.c
+++ linux-2.6.32/arch/x86/kernel/efi.c
@@ -271,6 +271,7 @@ static void __init do_add_efi_memmap(voi
 			break;
 		}
 		e820_add_region(start, size, e820_type);
+		e820_saved_add_region(start, size, e820_type);
 	}
 	sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
 }