2008-06-24 14:35:18

by Bernhard Walle

[permalink] [raw]
Subject: Limit E820 map when specifying mem parameter

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 <[email protected]>


2008-06-24 14:34:57

by Bernhard Walle

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

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 <[email protected]>
---
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

2008-06-24 14:35:38

by Bernhard Walle

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

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 <[email protected]>
---
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

2008-06-24 14:35:51

by Bernhard Walle

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

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 <[email protected]>
---
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

2008-06-24 20:03:47

by Yinghai Lu

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

On Tue, Jun 24, 2008 at 7:35 AM, Bernhard Walle <[email protected]> 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 <[email protected]>
> ---
> 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

2008-06-24 20:21:57

by Yinghai Lu

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

On Tue, Jun 24, 2008 at 1:01 PM, Yinghai Lu <[email protected]> wrote:
> On Tue, Jun 24, 2008 at 7:35 AM, Bernhard Walle <[email protected]> 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 <[email protected]>
>> ---
>> 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

2008-06-24 19:57:47

by Yinghai Lu

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

On Tue, Jun 24, 2008 at 7:35 AM, Bernhard Walle <[email protected]> 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 <[email protected]>
> ---
> 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


Attachments:
(No filename) (1.40 kB)
e820_update_range_fix.patch (797.00 B)
Download all attachments

2008-06-24 20:01:37

by Yinghai Lu

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

On Tue, Jun 24, 2008 at 7:35 AM, Bernhard Walle <[email protected]> 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 <[email protected]>
> ---
> 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

2008-06-25 12:04:20

by Bernhard Walle

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

* Yinghai Lu [2008-06-24 12:57]:
>
> On Tue, Jun 24, 2008 at 7:35 AM, Bernhard Walle <[email protected]> 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 <[email protected]>
> > ---
> > 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

2008-06-25 12:04:48

by Bernhard Walle

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

* 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

2008-06-25 15:45:36

by Ingo Molnar

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


* Bernhard Walle <[email protected]> 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