Received-SPF: pass (google.com: domain of linux-kernel+bounces-159504-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 26 Apr 2024 13:40:18 +0900
From: Byungchul Park <byungchul@sk.com>
To: tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
	dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com,
	jbohac@suse.cz, dyoung@redhat.com
Cc: kernel_team@skhynix.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] x86/e820: apply 'mem=' boot command while reserving
 memory using boot_params
Message-ID: <20240426044018.GA24557@system.software.com>
References: <20240423102320.GA47818@system.software.com>
 <20240424010313.25264-1-byungchul@sk.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240424010313.25264-1-byungchul@sk.com>
User-Agent: Mutt/1.9.4 (2018-02-28)

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@sk.com>
> 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@sk.com>
> ---
>  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