Limit E820 map when specifying mem parameter

Limit E820 map when specifying mem parameter

[Bernhard Walle]

This patch modifies the E820 map when specifying the mem kernel command line
parameter. That's the behaviour i386 had before the merging work in the
current "tip" tree.

As Yinghai Lu pointed out in email discussion, e820_update_range() should be
used for the updating instead of an own function. Two modifications in
e820_update_range() are necessary:

 1. Fix a small bug that prevented the partically covered entry from
    being stripped (size is not updated).
 
 2. Small API extension to be able to specify size == ULLONG_MAX to 
    update the whole map from size to the end.

The modification is necessary that kexec can build the ELF core headers only
for the used memory. Once the exporting of the real, unmodified memory map is
in the kernel, kexec can use the raw map and still reboot with full memory
size.

The patch is against 2.6.26-rc7-tip and has been successfully tested on i386
and x86-64, with and without "mem" parameter.


Signed-off-by: Bernhard Walle <bwalle@suse.de>

[PATCH 1/3] e820_update_range(): Strip size of original region

[Bernhard Walle]

This patch fixes a bug in e820_update_range(): Previously, if a region was
partially covered, then e820_update_range() only added a new E820 range but
didn't update the end/size of the previous range. That lead to duplicate
covering of a region.

Patch tested on i386 and x86-64 with patch that uses e820_update_range()
to limit the E820 map when "mem" parameter is specified on the command line.


Signed-off-by: Bernhard Walle <bwalle@suse.de>
---
 arch/x86/kernel/e820.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index e285ea3..e466073 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -422,6 +422,7 @@ u64 __init e820_update_range(u64 start, u64 size, unsigned old_type,
 		final_end = min(start + size, ei->addr + ei->size);
 		if (final_start >= final_end)
 			continue;
+		ei->size -= final_end - final_start;
 		e820_add_region(final_start, final_end - final_start,
 					 new_type);
 		real_updated_size += final_end - final_start;
-- 
1.5.4.5

Re: [PATCH 1/3] e820_update_range(): Strip size of original region

[Yinghai Lu]

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

On Tue, Jun 24, 2008 at 7:35 AM, Bernhard Walle <bwalle@suse.de> wrote:
> This patch fixes a bug in e820_update_range(): Previously, if a region was
> partially covered, then e820_update_range() only added a new E820 range but
> didn't update the end/size of the previous range. That lead to duplicate
> covering of a region.
>
> Patch tested on i386 and x86-64 with patch that uses e820_update_range()
> to limit the E820 map when "mem" parameter is specified on the command line.
>
>
> Signed-off-by: Bernhard Walle <bwalle@suse.de>
> ---
>  arch/x86/kernel/e820.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index e285ea3..e466073 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -422,6 +422,7 @@ u64 __init e820_update_range(u64 start, u64 size, unsigned old_type,
>                final_end = min(start + size, ei->addr + ei->size);
>                if (final_start >= final_end)
>                        continue;
> +               ei->size -= final_end - final_start;
>                e820_add_region(final_start, final_end - final_start,
>                                         new_type);
>                real_updated_size += final_end - final_start;
> --

thanks for finding it, but fix is not complete. could have problem
with fore boundary overlapping. need to update the ei->addr

please check attached patch

YH

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

[PATCH] x86: fix e820_update_range size when overlapping

before that we relay on sanitize_e820_map to remove the overlap.

but e820_update_range(,,E820_RESERVED, E820_RAM) will not work

this patch fix that

who is going to use this?

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

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 512f779..15b4393 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -425,6 +425,11 @@ u64 __init e820_update_range(u64 start, u64 size, unsigned old_type,
 		e820_add_region(final_start, final_end - final_start,
 					 new_type);
 		real_updated_size += final_end - final_start;
+
+		ei->size -= final_end - final_start;
+		if (ei->addr < final_start)
+			continue;
+		ei->addr = final_end;
 	}
 	return real_updated_size;
 }

Re: [PATCH 1/3] e820_update_range(): Strip size of original region

[Bernhard Walle]

* Yinghai Lu [2008-06-24 12:57]:
>
> On Tue, Jun 24, 2008 at 7:35 AM, Bernhard Walle <bwalle@suse.de> wrote:
> > This patch fixes a bug in e820_update_range(): Previously, if a region was
> > partially covered, then e820_update_range() only added a new E820 range but
> > didn't update the end/size of the previous range. That lead to duplicate
> > covering of a region.
> >
> > Patch tested on i386 and x86-64 with patch that uses e820_update_range()
> > to limit the E820 map when "mem" parameter is specified on the command line.
> >
> >
> > Signed-off-by: Bernhard Walle <bwalle@suse.de>
> > ---
> >  arch/x86/kernel/e820.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> > index e285ea3..e466073 100644
> > --- a/arch/x86/kernel/e820.c
> > +++ b/arch/x86/kernel/e820.c
> > @@ -422,6 +422,7 @@ u64 __init e820_update_range(u64 start, u64 size, unsigned old_type,
> >                final_end = min(start + size, ei->addr + ei->size);
> >                if (final_start >= final_end)
> >                        continue;
> > +               ei->size -= final_end - final_start;
> >                e820_add_region(final_start, final_end - final_start,
> >                                         new_type);
> >                real_updated_size += final_end - final_start;
> > --
> 
> thanks for finding it, but fix is not complete. could have problem
> with fore boundary overlapping. need to update the ei->addr
> 
> please check attached patch

Thanks, works. I added this to my patch series and reposted them.


Bernhard
-- 
Bernhard Walle, SUSE LINUX Products GmbH, Architecture Development

[PATCH 2/3] e820_update_range(): Allow specifying ULLONG_MAX

[Bernhard Walle]

Allow the specifying of ULLONG_MAX to limit the whole E820 map from the
specified start to the end. Without the patch, there would be integer
overflows.


Signed-off-by: Bernhard Walle <bwalle@suse.de>
---
 arch/x86/kernel/e820.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index e466073..7d1109b 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -397,6 +397,9 @@ int __init copy_e820_map(struct e820entry *biosmap, int nr_map)
 	return __copy_e820_map(biosmap, nr_map);
 }
 
+/*
+ * Pass size == ULLONG_MAX to update until the end.
+ */
 u64 __init e820_update_range(u64 start, u64 size, unsigned old_type,
 				unsigned new_type)
 {
@@ -412,14 +415,18 @@ u64 __init e820_update_range(u64 start, u64 size, unsigned old_type,
 			continue;
 		/* totally covered? */
 		if (ei->addr >= start &&
-		    (ei->addr + ei->size) <= (start + size)) {
+		    (((ei->addr + ei->size) <= (start + size)) ||
+			    (size == ULLONG_MAX))) {
 			ei->type = new_type;
 			real_updated_size += ei->size;
 			continue;
 		}
 		/* partially covered */
 		final_start = max(start, ei->addr);
-		final_end = min(start + size, ei->addr + ei->size);
+		if (size == ULLONG_MAX)
+			final_end = ei->addr + ei->size;
+		else
+			final_end = min(start + size, ei->addr + ei->size);
 		if (final_start >= final_end)
 			continue;
 		ei->size -= final_end - final_start;
-- 
1.5.4.5

Re: [PATCH 2/3] e820_update_range(): Allow specifying ULLONG_MAX

[Yinghai Lu]

On Tue, Jun 24, 2008 at 7:35 AM, Bernhard Walle <bwalle@suse.de> wrote:
> Allow the specifying of ULLONG_MAX to limit the whole E820 map from the
> specified start to the end. Without the patch, there would be integer
> overflows.
>
>
> Signed-off-by: Bernhard Walle <bwalle@suse.de>
> ---
>  arch/x86/kernel/e820.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index e466073..7d1109b 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -397,6 +397,9 @@ int __init copy_e820_map(struct e820entry *biosmap, int nr_map)
>        return __copy_e820_map(biosmap, nr_map);
>  }
>
> +/*
> + * Pass size == ULLONG_MAX to update until the end.
> + */
>  u64 __init e820_update_range(u64 start, u64 size, unsigned old_type,
>                                unsigned new_type)
>  {
> @@ -412,14 +415,18 @@ u64 __init e820_update_range(u64 start, u64 size, unsigned old_type,
>                        continue;
>                /* totally covered? */
>                if (ei->addr >= start &&
> -                   (ei->addr + ei->size) <= (start + size)) {
> +                   (((ei->addr + ei->size) <= (start + size)) ||
> +                           (size == ULLONG_MAX))) {
>                        ei->type = new_type;
>                        real_updated_size += ei->size;
>                        continue;
>                }
>                /* partially covered */
>                final_start = max(start, ei->addr);
> -               final_end = min(start + size, ei->addr + ei->size);
> +               if (size == ULLONG_MAX)
> +                       final_end = ei->addr + ei->size;
> +               else
> +                       final_end = min(start + size, ei->addr + ei->size);
>                if (final_start >= final_end)
>                        continue;
>                ei->size -= final_end - final_start;
> --


it seems we should let the caller to use
e820_update_range(start, ULLONG_MAX - size,....)

so you don't need to touch this func.

YH

Re: [PATCH 2/3] e820_update_range(): Allow specifying ULLONG_MAX

[Yinghai Lu]

On Tue, Jun 24, 2008 at 1:01 PM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> On Tue, Jun 24, 2008 at 7:35 AM, Bernhard Walle <bwalle@suse.de> wrote:
>> Allow the specifying of ULLONG_MAX to limit the whole E820 map from the
>> specified start to the end. Without the patch, there would be integer
>> overflows.
>>
>>
>> Signed-off-by: Bernhard Walle <bwalle@suse.de>
>> ---
>>  arch/x86/kernel/e820.c |   11 +++++++++--
>>  1 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>> index e466073..7d1109b 100644
>> --- a/arch/x86/kernel/e820.c
>> +++ b/arch/x86/kernel/e820.c
>> @@ -397,6 +397,9 @@ int __init copy_e820_map(struct e820entry *biosmap, int nr_map)
>>        return __copy_e820_map(biosmap, nr_map);
>>  }
>>
>> +/*
>> + * Pass size == ULLONG_MAX to update until the end.
>> + */
>>  u64 __init e820_update_range(u64 start, u64 size, unsigned old_type,
>>                                unsigned new_type)
>>  {
>> @@ -412,14 +415,18 @@ u64 __init e820_update_range(u64 start, u64 size, unsigned old_type,
>>                        continue;
>>                /* totally covered? */
>>                if (ei->addr >= start &&
>> -                   (ei->addr + ei->size) <= (start + size)) {
>> +                   (((ei->addr + ei->size) <= (start + size)) ||
>> +                           (size == ULLONG_MAX))) {
>>                        ei->type = new_type;
>>                        real_updated_size += ei->size;
>>                        continue;
>>                }
>>                /* partially covered */
>>                final_start = max(start, ei->addr);
>> -               final_end = min(start + size, ei->addr + ei->size);
>> +               if (size == ULLONG_MAX)
>> +                       final_end = ei->addr + ei->size;
>> +               else
>> +                       final_end = min(start + size, ei->addr + ei->size);
>>                if (final_start >= final_end)
>>                        continue;
>>                ei->size -= final_end - final_start;
>> --
>
>
> it seems we should let the caller to use
> e820_update_range(start, ULLONG_MAX - size,....)
>
> so you don't need to touch this func.

or add sanitary check before using size in this func like
if (size > ULLONG_MAX - start)
     size = ULLONG_MAX - start;

e820_remove_range need to the same thing

YH

Re: [PATCH 2/3] e820_update_range(): Allow specifying ULLONG_MAX

[Bernhard Walle]

* Yinghai Lu [2008-06-24 13:21]:
>
> > it seems we should let the caller to use
> > e820_update_range(start, ULLONG_MAX - size,....)
> >
> > so you don't need to touch this func.
> 
> or add sanitary check before using size in this func like
> if (size > ULLONG_MAX - start)
>      size = ULLONG_MAX - start;
> 
> e820_remove_range need to the same thing

I like that. I think the complexity should be in the function, and not
in the caller's function.



Bernhard
-- 
Bernhard Walle, SUSE LINUX Products GmbH, Architecture Development

Re: [PATCH 2/3] e820_update_range(): Allow specifying ULLONG_MAX

[Ingo Molnar]

* Bernhard Walle <bwalle@suse.de> wrote:

> * Yinghai Lu [2008-06-24 13:21]:
> >
> > > it seems we should let the caller to use
> > > e820_update_range(start, ULLONG_MAX - size,....)
> > >
> > > so you don't need to touch this func.
> > 
> > or add sanitary check before using size in this func like
> > if (size > ULLONG_MAX - start)
> >      size = ULLONG_MAX - start;
> > 
> > e820_remove_range need to the same thing
> 
> I like that. I think the complexity should be in the function, and not 
> in the caller's function.

ok - i'll wait for v2 of these patches, which will have the feedback 
from Yinghai incorporated - agreed? (Please Cc: Yinghai on the next 
submission of this series) Thanks,

	Ingo

[PATCH 3/3] Limit E820 map when a user-defined memory map is specified

[Bernhard Walle]

This patch brings back limiting of the E820 map when a user-defined
E820 map is specified. While the behaviour of i386 (32 bit) was to limit
the E820 map (and /proc/iomem), the behaviour of x86-64 (64 bit) was not to
limit.

That patch limits the E820 map again for both x86 architectures.

Code was tested for compilation and booting on a 32 bit and 64 bit system.


Signed-off-by: Bernhard Walle <bwalle@suse.de>
---
 arch/x86/kernel/e820.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 7d1109b..19b7f05 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -979,6 +979,8 @@ static int __init parse_memopt(char *p)
 
 	mem_size = memparse(p, &p);
 	end_user_pfn = mem_size>>PAGE_SHIFT;
+	e820_update_range(mem_size, ULLONG_MAX, E820_RAM, E820_RESERVED);
+
 	return 0;
 }
 early_param("mem", parse_memopt);
@@ -1023,6 +1025,7 @@ static int __init parse_memmap_opt(char *p)
 		e820_add_region(start_at, mem_size, E820_RESERVED);
 	} else {
 		end_user_pfn = (mem_size >> PAGE_SHIFT);
+		e820_update_range(mem_size, ULLONG_MAX, E820_RAM, E820_RESERVED);
 	}
 	return *p == '\0' ? 0 : -EINVAL;
 }
-- 
1.5.4.5

Re: [PATCH 3/3] Limit E820 map when a user-defined memory map is specified

[Yinghai Lu]

On Tue, Jun 24, 2008 at 7:35 AM, Bernhard Walle <bwalle@suse.de> wrote:
> This patch brings back limiting of the E820 map when a user-defined
> E820 map is specified. While the behaviour of i386 (32 bit) was to limit
> the E820 map (and /proc/iomem), the behaviour of x86-64 (64 bit) was not to
> limit.
>
> That patch limits the E820 map again for both x86 architectures.
>
> Code was tested for compilation and booting on a 32 bit and 64 bit system.
>
>
> Signed-off-by: Bernhard Walle <bwalle@suse.de>
> ---
>  arch/x86/kernel/e820.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 7d1109b..19b7f05 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -979,6 +979,8 @@ static int __init parse_memopt(char *p)
>
>        mem_size = memparse(p, &p);
>        end_user_pfn = mem_size>>PAGE_SHIFT;
> +       e820_update_range(mem_size, ULLONG_MAX, E820_RAM, E820_RESERVED);
==>
+       e820_update_range(mem_size, ULLONG_MAX - mem_size, E820_RAM,
E820_RESERVED);
> +
>        return 0;
>  }
>  early_param("mem", parse_memopt);
> @@ -1023,6 +1025,7 @@ static int __init parse_memmap_opt(char *p)
>                e820_add_region(start_at, mem_size, E820_RESERVED);
>        } else {
>                end_user_pfn = (mem_size >> PAGE_SHIFT);
> +               e820_update_range(mem_size, ULLONG_MAX, E820_RAM, E820_RESERVED);
==>
+       e820_update_range(mem_size, ULLONG_MAX - mem_size, E820_RAM,
E820_RESERVED);

YH

Limit E820 map when specifying mem parameter

[Bernhard Walle]

This patch modifies the E820 map when specifying the mem kernel command line
parameter. That's the behaviour i386 had before the merging work in the
current "tip" tree.

As Yinghai Lu pointed out in email discussion, e820_update_range() should be
used for the updating instead of an own function. Two modifications in
e820_update_range() are necessary:

 1. Fix a small bug that prevented the partically covered entry from
    being stripped (size is not updated).
 
 2. Small API extension to be able to specify size == ULLONG_MAX to 
    update the whole map from size to the end.

The modification is necessary that kexec can build the ELF core headers only
for the used memory. Once the exporting of the real, unmodified memory map is
in the kernel, kexec can use the raw map and still reboot with full memory
size.

The patch is against 2.6.26-rc7-tip and has been successfully tested on i386
and x86-64, with and without "mem" parameter.

[PATCH 2/3] e820_update_range(): Allow specifying ULLONG_MAX

[Bernhard Walle]

Allow the specifying of ULLONG_MAX to limit the whole E820 map from the
specified start to the end. Without the patch, there would be integer
overflows.


Signed-off-by: Bernhard Walle <bwalle@suse.de>
---
 arch/x86/kernel/e820.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index e466073..7d1109b 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -397,6 +397,9 @@ int __init copy_e820_map(struct e820entry *biosmap, int nr_map)
 	return __copy_e820_map(biosmap, nr_map);
 }
 
+/*
+ * Pass size == ULLONG_MAX to update until the end.
+ */
 u64 __init e820_update_range(u64 start, u64 size, unsigned old_type,
 				unsigned new_type)
 {
@@ -412,14 +415,18 @@ u64 __init e820_update_range(u64 start, u64 size, unsigned old_type,
 			continue;
 		/* totally covered? */
 		if (ei->addr >= start &&
-		    (ei->addr + ei->size) <= (start + size)) {
+		    (((ei->addr + ei->size) <= (start + size)) ||
+			    (size == ULLONG_MAX))) {
 			ei->type = new_type;
 			real_updated_size += ei->size;
 			continue;
 		}
 		/* partially covered */
 		final_start = max(start, ei->addr);
-		final_end = min(start + size, ei->addr + ei->size);
+		if (size == ULLONG_MAX)
+			final_end = ei->addr + ei->size;
+		else
+			final_end = min(start + size, ei->addr + ei->size);
 		if (final_start >= final_end)
 			continue;
 		ei->size -= final_end - final_start;
-- 
1.5.4.5