2006-08-03 00:26:19

by Jeremy Fitzhardinge

[permalink] [raw]
Subject: [patch 7/8] Add a bootparameter to reserve high linear address space.

Add a bootparameter to reserve high linear address space for hypervisors.
This is necessary to allow dynamically loaded hypervisor modules, which
might not happen until userspace is already running, and also provides a
useful tool to benchmark the performance impact of reduced lowmem address
space.

Signed-off-by: Zachary Amsden <[email protected]>
Signed-off-by: Chris Wright <[email protected]>

---
Documentation/kernel-parameters.txt | 5 +++++
arch/i386/kernel/setup.c | 19 +++++++++++++++++++
2 files changed, 24 insertions(+)

===================================================================
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1357,6 +1357,11 @@ running once the system is up.

reserve= [KNL,BUGS] Force the kernel to ignore some iomem area

+ reservetop= [IA-32]
+ Format: nn[KMG]
+ Reserves a hole at the top of the kernel virtual
+ address space.
+
resume= [SWSUSP]
Specify the partition device for software suspend

===================================================================
--- a/arch/i386/kernel/setup.c
+++ b/arch/i386/kernel/setup.c
@@ -160,6 +160,12 @@ static char command_line[COMMAND_LINE_SI
static char command_line[COMMAND_LINE_SIZE];

unsigned char __initdata boot_params[PARAM_SIZE];
+
+static int __init setup_reservetop(char *s)
+{
+ return 1;
+}
+__setup("reservetop", setup_reservetop);

static struct resource data_resource = {
.name = "Kernel data",
@@ -917,6 +923,19 @@ static void __init parse_cmdline_early (
else if (!memcmp(from, "vmalloc=", 8))
__VMALLOC_RESERVE = memparse(from+8, &from);

+ /*
+ * reservetop=size reserves a hole at the top of the kernel
+ * address space which a hypervisor can load into later.
+ * Needed for dynamically loaded hypervisors, so relocating
+ * the fixmap can be done before paging initialization.
+ * This hole must be a multiple of 4M.
+ */
+ else if (!memcmp(from, "reservetop=", 11)) {
+ unsigned long reserve = memparse(from+11, &from);
+ reserve &= ~0x3fffff;
+ reserve_top_address(reserve);
+ }
+
next_char:
c = *(from++);
if (!c)

--


2006-08-03 06:19:21

by Andrew Morton

[permalink] [raw]
Subject: Re: [patch 7/8] Add a bootparameter to reserve high linear address space.

On Wed, 02 Aug 2006 17:25:17 -0700
Jeremy Fitzhardinge <[email protected]> wrote:

> + /*
> + * reservetop=size reserves a hole at the top of the kernel
> + * address space which a hypervisor can load into later.
> + * Needed for dynamically loaded hypervisors, so relocating
> + * the fixmap can be done before paging initialization.
> + * This hole must be a multiple of 4M.
> + */
> + else if (!memcmp(from, "reservetop=", 11)) {
> + unsigned long reserve = memparse(from+11, &from);
> + reserve &= ~0x3fffff;
> + reserve_top_address(reserve);
> + }

I assume that this argument will normally be passed in via the hypervisor
rather than by human-entered information?

In which case, perhaps a panic would be a more appropriate response to a
non-multiple-of-4M.

Either way, rounding the number down rather than up seems wrong...

2006-08-03 07:33:38

by Zachary Amsden

[permalink] [raw]
Subject: Re: [patch 7/8] Add a bootparameter to reserve high linear address space.

Andrew Morton wrote:
> On Wed, 02 Aug 2006 17:25:17 -0700
> Jeremy Fitzhardinge <[email protected]> wrote:
>
>
>> + /*
>> + * reservetop=size reserves a hole at the top of the kernel
>> + * address space which a hypervisor can load into later.
>> + * Needed for dynamically loaded hypervisors, so relocating
>> + * the fixmap can be done before paging initialization.
>> + * This hole must be a multiple of 4M.
>> + */
>> + else if (!memcmp(from, "reservetop=", 11)) {
>> + unsigned long reserve = memparse(from+11, &from);
>> + reserve &= ~0x3fffff;
>> + reserve_top_address(reserve);
>> + }
>>
>
> I assume that this argument will normally be passed in via the hypervisor
> rather than by human-entered information?
>
> In which case, perhaps a panic would be a more appropriate response to a
> non-multiple-of-4M.
>
> Either way, rounding the number down rather than up seems wrong...
>

Agree on the rounding issue - but is a panic really correct? Perhaps we
should not round at all.

The presumption is actually that this is human or script entered
information. A runtime loaded hypervisor module has no way to tweak or
toggle the boot parameters, as it hasn't yet been loaded. It could be
that a human operator wants to make room for it. Giving the operator a
panic is not the most friendly thing to do - logging the failure on
module load is much nicer. And such a runtime loaded hypervisor must be
fully virtualizing anyway, so even if the argument is wrong and doesn't
give the hypervisor enough space to load, no damage is done - the
operator just resets the parameter and reboots.

Zach

2006-08-03 07:41:52

by Andrew Morton

[permalink] [raw]
Subject: Re: [patch 7/8] Add a bootparameter to reserve high linear address space.

On Thu, 03 Aug 2006 00:33:10 -0700
Zachary Amsden <[email protected]> wrote:

> >> + /*
> >> + * reservetop=size reserves a hole at the top of the kernel
> >> + * address space which a hypervisor can load into later.
> >> + * Needed for dynamically loaded hypervisors, so relocating
> >> + * the fixmap can be done before paging initialization.
> >> + * This hole must be a multiple of 4M.
> >> + */
> >> + else if (!memcmp(from, "reservetop=", 11)) {
> >> + unsigned long reserve = memparse(from+11, &from);
> >> + reserve &= ~0x3fffff;
> >> + reserve_top_address(reserve);
> >> + }
> >>
> >
> > I assume that this argument will normally be passed in via the hypervisor
> > rather than by human-entered information?
> >
> > In which case, perhaps a panic would be a more appropriate response to a
> > non-multiple-of-4M.
> >
> > Either way, rounding the number down rather than up seems wrong...
> >
>
> Agree on the rounding issue - but is a panic really correct? Perhaps we
> should not round at all.
>
> The presumption is actually that this is human or script entered
> information. A runtime loaded hypervisor module has no way to tweak or
> toggle the boot parameters, as it hasn't yet been loaded. It could be
> that a human operator wants to make room for it. Giving the operator a
> panic is not the most friendly thing to do - logging the failure on
> module load is much nicer. And such a runtime loaded hypervisor must be
> fully virtualizing anyway, so even if the argument is wrong and doesn't
> give the hypervisor enough space to load, no damage is done - the
> operator just resets the parameter and reboots.

The comment says "must". If that's true then printing a what-you-did-wrong
message and halting is appropriate.

But whatever. The issue is flagged and I'm happy to leave it in Jeremy's
lap.

2006-08-03 08:58:34

by Zachary Amsden

[permalink] [raw]
Subject: Re: [patch 7/8] Add a bootparameter to reserve high linear address space.

Subject: Add a bootparameter to reserve high linear address space.

Add a bootparameter to reserve high linear address space for hypervisors.
This is necessary to allow dynamically loaded hypervisor modules, which
might not happen until userspace is already running, and also provides a
useful tool to benchmark the performance impact of reduced lowmem address
space.

Signed-off-by: Zachary Amsden <[email protected]>
Signed-off-by: Chris Wright <[email protected]>

---
Documentation/kernel-parameters.txt | 5 +++++
arch/i386/kernel/setup.c | 19 +++++++++++++++++++
2 files changed, 24 insertions(+)

diff -r 5bb2fc59943d Documentation/kernel-parameters.txt
--- a/Documentation/kernel-parameters.txt Thu Aug 03 01:36:13 2006 -0700
+++ b/Documentation/kernel-parameters.txt Thu Aug 03 01:36:13 2006 -0700
@@ -1357,6 +1357,11 @@ running once the system is up.

reserve= [KNL,BUGS] Force the kernel to ignore some iomem area

+ reservetop= [IA-32]
+ Format: nn[KMG]
+ Reserves a hole at the top of the kernel virtual
+ address space.
+
resume= [SWSUSP]
Specify the partition device for software suspend

diff -r 5bb2fc59943d arch/i386/kernel/setup.c
--- a/arch/i386/kernel/setup.c Thu Aug 03 01:36:13 2006 -0700
+++ b/arch/i386/kernel/setup.c Thu Aug 03 01:36:52 2006 -0700
@@ -160,6 +160,12 @@ static char command_line[COMMAND_LINE_SI
static char command_line[COMMAND_LINE_SIZE];

unsigned char __initdata boot_params[PARAM_SIZE];
+
+static int __init setup_reservetop(char *s)
+{
+ return 1;
+}
+__setup("reservetop", setup_reservetop);

static struct resource data_resource = {
.name = "Kernel data",
@@ -917,6 +923,17 @@ static void __init parse_cmdline_early (
else if (!memcmp(from, "vmalloc=", 8))
__VMALLOC_RESERVE = memparse(from+8, &from);

+ /*
+ * reservetop=size reserves a hole at the top of the kernel
+ * address space which a hypervisor can load into later.
+ * Needed for dynamically loaded hypervisors, so relocating
+ * the fixmap can be done before paging initialization.
+ */
+ else if (!memcmp(from, "reservetop=", 11)) {
+ unsigned long reserve = memparse(from+11, &from);
+ reserve_top_address(reserve);
+ }
+
next_char:
c = *(from++);
if (!c)


Attachments:
fixmap-bootparam.patch (2.19 kB)

2006-08-05 21:58:58

by Andrew Morton

[permalink] [raw]
Subject: Re: [patch 7/8] Add a bootparameter to reserve high linear address space.

On Thu, 03 Aug 2006 01:58:32 -0700
Zachary Amsden <[email protected]> wrote:

> Add a bootparameter to reserve high linear address space for hypervisors.
> This is necessary to allow dynamically loaded hypervisor modules, which
> might not happen until userspace is already running, and also provides a
> useful tool to benchmark the performance impact of reduced lowmem address
> space.

Andi has gone and rotorooted the x86 boot parameter handling in there.
This patch now looks like this:


From: Zachary Amsden <[email protected]>

Add a boot parameter to reserve high linear address space for hypervisors.
This is necessary to allow dynamically loaded hypervisor modules, which might
not happen until userspace is already running, and also provides a useful tool
to benchmark the performance impact of reduced lowmem address space.

Signed-off-by: Zachary Amsden <[email protected]>
Signed-off-by: Chris Wright <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---

Documentation/kernel-parameters.txt | 5 +++++
arch/i386/kernel/setup.c | 24 ++++++++++++++++++++++++
2 files changed, 29 insertions(+)

diff -puN arch/i386/kernel/setup.c~x86-add-a-bootparameter-to-reserve-high-linear-address-space arch/i386/kernel/setup.c
--- a/arch/i386/kernel/setup.c~x86-add-a-bootparameter-to-reserve-high-linear-address-space
+++ a/arch/i386/kernel/setup.c
@@ -149,6 +149,12 @@ static char command_line[COMMAND_LINE_SI

unsigned char __initdata boot_params[PARAM_SIZE];

+static int __init setup_reservetop(char *s)
+{
+ return 1;
+}
+__setup("reservetop", setup_reservetop);
+
static struct resource data_resource = {
.name = "Kernel data",
.start = 0,
@@ -814,6 +820,24 @@ static int __init parse_vmalloc(char *ar
early_param("vmalloc", parse_vmalloc);

/*
+ * reservetop=size reserves a hole at the top of the kernel address space which
+ * a hypervisor can load into later. Needed for dynamically loaded hypervisors,
+ * so relocating the fixmap can be done before paging initialization.
+ */
+static int __init parse_reservetop(char *arg)
+{
+ unsigned long address;
+
+ if (!arg)
+ return -EINVAL;
+
+ address = memparse(arg, &arg);
+ reserve_top_address(address);
+ return 0;
+}
+early_param("reservetop", parse_reservetop);
+
+/*
* Callback for efi_memory_walk.
*/
static int __init
diff -puN Documentation/kernel-parameters.txt~x86-add-a-bootparameter-to-reserve-high-linear-address-space Documentation/kernel-parameters.txt
--- a/Documentation/kernel-parameters.txt~x86-add-a-bootparameter-to-reserve-high-linear-address-space
+++ a/Documentation/kernel-parameters.txt
@@ -1357,6 +1357,11 @@ running once the system is up.

reserve= [KNL,BUGS] Force the kernel to ignore some iomem area

+ reservetop= [IA-32]
+ Format: nn[KMG]
+ Reserves a hole at the top of the kernel virtual
+ address space.
+
resume= [SWSUSP]
Specify the partition device for software suspend

_

2006-08-05 22:52:48

by Zachary Amsden

[permalink] [raw]
Subject: Re: [patch 7/8] Add a bootparameter to reserve high linear address space.

Andrew Morton wrote:
> On Thu, 03 Aug 2006 01:58:32 -0700
> Zachary Amsden <[email protected]> wrote:
>
>
>> Add a bootparameter to reserve high linear address space for hypervisors.
>> This is necessary to allow dynamically loaded hypervisor modules, which
>> might not happen until userspace is already running, and also provides a
>> useful tool to benchmark the performance impact of reduced lowmem address
>> space.
>>
>
> Andi has gone and rotorooted the x86 boot parameter handling in there.
> This patch now looks like this:
>

The rototilled patch looks great. Acked.

Zach

2006-08-05 23:17:33

by Rusty Russell

[permalink] [raw]
Subject: Re: [patch 7/8] Add a bootparameter to reserve high linear address space.

On Sat, 2006-08-05 at 14:58 -0700, Andrew Morton wrote:
> On Thu, 03 Aug 2006 01:58:32 -0700
> Zachary Amsden <[email protected]> wrote:
>
> > Add a bootparameter to reserve high linear address space for hypervisors.
> > This is necessary to allow dynamically loaded hypervisor modules, which
> > might not happen until userspace is already running, and also provides a
> > useful tool to benchmark the performance impact of reduced lowmem address
> > space.
>
> Andi has gone and rotorooted the x86 boot parameter handling in there.

That was me, via Andi, but yep:

> diff -puN arch/i386/kernel/setup.c~x86-add-a-bootparameter-to-reserve-high-linear-address-space arch/i386/kernel/setup.c
> --- a/arch/i386/kernel/setup.c~x86-add-a-bootparameter-to-reserve-high-linear-address-space
> +++ a/arch/i386/kernel/setup.c
> @@ -149,6 +149,12 @@ static char command_line[COMMAND_LINE_SI
>
> unsigned char __initdata boot_params[PARAM_SIZE];
>
> +static int __init setup_reservetop(char *s)
> +{
> + return 1;
> +}
> +__setup("reservetop", setup_reservetop);
> +
> static struct resource data_resource = {
> .name = "Kernel data",
> .start = 0,

Please remove this hunk: it's now junk.

Cheers,
Rusty.
--
Help! Save Australia from the worst of the DMCA: http://linux.org.au/law

2006-08-06 22:00:53

by Pavel Machek

[permalink] [raw]
Subject: Re: [patch 7/8] Add a bootparameter to reserve high linear address space.

Hi!

> Add a bootparameter to reserve high linear address space for hypervisors.
> This is necessary to allow dynamically loaded hypervisor modules, which

Dynamically loaded hypervisor? Do we really want to support that?

Pavel
--
Thanks for all the (sleeping) penguins.

2006-08-07 02:10:35

by Andi Kleen

[permalink] [raw]
Subject: Re: [patch 7/8] Add a bootparameter to reserve high linear address space.

On Thursday 01 January 1970 01:15, Pavel Machek wrote:
> Hi!
>
> > Add a bootparameter to reserve high linear address space for hypervisors.
> > This is necessary to allow dynamically loaded hypervisor modules, which
>
> Dynamically loaded hypervisor? Do we really want to support that?

I hope so. IMHO letting Linux boot first and then the Hypervisor is
a better design than the other way round which requires a lot of duplicated code.

-Andi