Re: [PATCH v2] x86/e820: apply 'mem=' boot command while reserving memory using boot_params

[Byungchul Park]

On Fri, Apr 26, 2024 at 09:52:30AM -0700, H. Peter Anvin wrote:
> On April 25, 2024 9:40:18 PM PDT, Byungchul Park <byungchul@xxxxxx> wrote:
> >On Wed, Apr 24, 2024 at 10:03:13AM +0900, Byungchul Park wrote:
> >> I might miss something.  Please lemme know if I go wrong.  Thanks.
> >
> >I started to work on it since I wanted to limit memory boundary using
> >'mem=' boot command but it doesn't work.  However, while looking around
> >the code in more detail, I found the issue is about which one should
> >have higher priority between:
> >
> >   1. boot command limiting memory boundary e.g. 'mem=',
> >   2. setup data of memory map from bootloader, boot_params.
> >
> >Based on the current code, setup data from bootloader has higher
> >priority than boot command so the setup data can overwrite the user
> >defined limit specified in boot command.  Is it inteded?
> >
> >   If yes, I should stop posting.
> >   If not, I will keep posting with the following - v3.
> >
> >	Byungchul
> >
> >---
> >
> >diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> >index 6f1b379e3b38..3bc593235b76 100644
> >--- a/arch/x86/kernel/e820.c
> >+++ b/arch/x86/kernel/e820.c
> >@@ -879,6 +879,7 @@ static void __init early_panic(char *msg)
> > }
> > 
> > static int userdef __initdata;
> >+static u64 userdef_mem_limit;
> > 
> > /* The "mem=nopentium" boot option disables 4MB page tables on 32-bit kernels: */
> > static int __init parse_memopt(char *p)
> >@@ -905,7 +906,10 @@ static int __init parse_memopt(char *p)
> > 	if (mem_size == 0)
> > 		return -EINVAL;
> > 
> >-	e820__range_remove(mem_size, ULLONG_MAX - mem_size, E820_TYPE_RAM, 1);
> >+	if (userdef_mem_limit)
> >+		userdef_mem_limit = min(userdef_mem_limit, mem_size);
> >+	else
> >+		userdef_mem_limit = mem_size;
> > 
> > #ifdef CONFIG_MEMORY_HOTPLUG
> > 	max_mem_size = mem_size;
> >@@ -966,7 +970,10 @@ static int __init parse_memmap_one(char *p)
> > 		else
> > 			e820__range_remove(start_at, mem_size, 0, 0);
> > 	} else {
> >-		e820__range_remove(mem_size, ULLONG_MAX - mem_size, E820_TYPE_RAM, 1);
> >+		if (userdef_mem_limit)
> >+			userdef_mem_limit = min(userdef_mem_limit, mem_size);
> >+		else
> >+			userdef_mem_limit = mem_size;
> > 	}
> > 
> > 	return *p == '\0' ? 0 : -EINVAL;
> >@@ -1050,6 +1057,11 @@ void __init e820__reserve_setup_data(void)
> > void __init e820__finish_early_params(void)
> > {
> > 	if (userdef) {
> >+		if (userdef_mem_limit)
> >+			e820__range_remove(userdef_mem_limit,
> >+					ULLONG_MAX - userdef_mem_limit,
> >+					E820_TYPE_RAM, 1);
> >+
> > 		if (e820__update_table(e820_table) < 0)
> > 			early_panic("Invalid user supplied memory map");
> > 
> >---
> >> 	Byungchul
> >> 
> >> Changes from v1
> >> 	1. before - handle boot_mem_limit assuming the default is U64_MAX.
> >> 	   after  - handle boot_mem_limit assuming the default is 0.
> >> 
> >> --->8---
> >> >From e8bf247d6024b35af5300914dcff9135df9c1d66 Mon Sep 17 00:00:00 2001
> >> From: Byungchul Park <byungchul@xxxxxx>
> >> Date: Wed, 24 Apr 2024 09:55:25 +0900
> >> Subject: [PATCH v2] x86/e820: apply 'mem=' boot command while reserving memory using boot_params
> >> 
> >> When a user specifies 'mem=' boot command, it's expected to limit the
> >> maximum address of usable memory for the kernel no matter what the
> >> memory map source is.  However, 'mem=' boot command doesn't work since
> >> it doesn't respect it when reserving memory using boot_params.
> >> 
> >> Applied the restriction when reserving memory using boot_params.  While
> >> at it, renamed mem_size to a more specific name, boot_mem_limit.
> >> 
> >> Signed-off-by: Byungchul Park <byungchul@xxxxxx>
> >> ---
> >>  arch/x86/kernel/e820.c | 15 +++++++++------
> >>  1 file changed, 9 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> >> index 6f1b379e3b38..e3f716128caf 100644
> >> --- a/arch/x86/kernel/e820.c
> >> +++ b/arch/x86/kernel/e820.c
> >> @@ -880,11 +880,11 @@ static void __init early_panic(char *msg)
> >>  
> >>  static int userdef __initdata;
> >>  
> >> +static u64 boot_mem_limit;
> >> +
> >>  /* The "mem=nopentium" boot option disables 4MB page tables on 32-bit kernels: */
> >>  static int __init parse_memopt(char *p)
> >>  {
> >> -	u64 mem_size;
> >> -
> >>  	if (!p)
> >>  		return -EINVAL;
> >>  
> >> @@ -899,16 +899,16 @@ static int __init parse_memopt(char *p)
> >>  	}
> >>  
> >>  	userdef = 1;
> >> -	mem_size = memparse(p, &p);
> >> +	boot_mem_limit = memparse(p, &p);
> >>  
> >>  	/* Don't remove all memory when getting "mem={invalid}" parameter: */
> >> -	if (mem_size == 0)
> >> +	if (boot_mem_limit == 0)
> >>  		return -EINVAL;
> >>  
> >> -	e820__range_remove(mem_size, ULLONG_MAX - mem_size, E820_TYPE_RAM, 1);
> >> +	e820__range_remove(boot_mem_limit, ULLONG_MAX - boot_mem_limit, E820_TYPE_RAM, 1);
> >>  
> >>  #ifdef CONFIG_MEMORY_HOTPLUG
> >> -	max_mem_size = mem_size;
> >> +	max_mem_size = boot_mem_limit;
> >>  #endif
> >>  
> >>  	return 0;
> >> @@ -1036,6 +1036,9 @@ void __init e820__reserve_setup_data(void)
> >>  		early_memunmap(data, len);
> >>  	}
> >>  
> >> +	if (boot_mem_limit)
> >> +		e820__range_remove(boot_mem_limit, ULLONG_MAX - boot_mem_limit,
> >> +				E820_TYPE_RESERVED_KERN, 1);
> >>  	e820__update_table(e820_table);
> >>  
> >>  	pr_info("extended physical RAM map:\n");
> >> -- 
> >> 2.17.1
> 
> mem= typically should cap the usable memory at that address. At one
> point in history we also allowed it to add memory at the top, but

Ah.. I just needed to cap for a test.

> modern systems have too complex memory maps; the memmap= option can be
> used for that, however.

Even with memmap=, I still cannot cap the usable memory.  However, with
a tricky way like by adding a dummy reserve memory, I could manage to
make it.

So.. Is the following approach not acceptable then?

   https://lore.kernel.org/lkml/20240510074714.73177-1-byungchul@xxxxxx/

	Byungchul