1. Patchew
  2. linux
  3. [PATCH 00/29] x86/boot/e820: Assorted E820 table handling features and cleanups
  4. View patch

[PATCH 27/29] x86/boot/e820: Simplify the e820__range_remove() API

Ingo Molnar posted 29 patches 7 months, 4 weeks ago
There is a newer version of this series
[PATCH 27/29] x86/boot/e820: Simplify the e820__range_remove() API
Posted by Ingo Molnar 7 months, 4 weeks ago 
Right now e820__range_remove() has two parameters to control the
E820 type of the range removed:

	extern void e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type);

Since E820 types start at 1, zero has a natural meaning of 'no type.

Consolidate the (old_type,check_type) parameters into a single (filter_type)
parameter:

	extern void e820__range_remove(u64 start, u64 size, enum e820_type filter_type);

Note that both e820__mapped_raw_any() and e820__mapped_any()
already have such semantics for their 'type' parameter, although
it's currently not used with '0' by in-kernel code.

Also, the __e820__mapped_all() internal helper already has such
semantics implemented as well, and the e820__get_entry_type() API
uses the '0' type to such effect.

This simplifies not just e820__range_remove(), and synchronizes its
use of type filters with other E820 API functions, but simplifies
usage sites as well, such as parse_memmap_one(), beyond the reduction
of the number of parameters:

  -               else if (from)
  -                       e820__range_remove(start_at, mem_size, from, 1);
                  else
  -                       e820__range_remove(start_at, mem_size, 0, 0);
  +                       e820__range_remove(start_at, mem_size, from);

The generated code gets smaller as well:

	add/remove: 0/0 grow/shrink: 0/5 up/down: 0/-66 (-66)

	Function                                     old     new   delta
	parse_memopt                                 112     107      -5
	efi_init                                    1048    1039      -9
	setup_arch                                  2719    2709     -10
	e820__range_remove                           283     273     -10
	parse_memmap_opt                             559     527     -32

	Total: Before=22,675,600, After=22,675,534, chg -0.00%

Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Andy Shevchenko <andy@kernel.org>
Cc: Arnd Bergmann <arnd@kernel.org>
Cc: David Woodhouse <dwmw@amazon.co.uk>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
 arch/x86/include/asm/e820/api.h |  2 +-
 arch/x86/kernel/e820.c          | 16 +++++++---------
 arch/x86/kernel/setup.c         |  4 ++--
 arch/x86/platform/efi/efi.c     |  3 +--
 4 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
index 9cf416f7a84f..bbe0c8de976c 100644
--- a/arch/x86/include/asm/e820/api.h
+++ b/arch/x86/include/asm/e820/api.h
@@ -16,7 +16,7 @@ extern bool e820__mapped_all(u64 start, u64 end, enum e820_type type);
 
 extern void e820__range_add   (u64 start, u64 size, enum e820_type type);
 extern u64  e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
-extern void e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type);
+extern void e820__range_remove(u64 start, u64 size, enum e820_type filter_type);
 extern u64  e820__range_update_table(struct e820_table *t, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
 
 extern int  e820__update_table(struct e820_table *table);
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 2f9aecb9911c..b4fe9bebdeb5 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -590,7 +590,7 @@ __init u64 e820__range_update_table(struct e820_table *t, u64 start, u64 size,
 }
 
 /* Remove a range of memory from the E820 table: */
-__init void e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type)
+__init void e820__range_remove(u64 start, u64 size, enum e820_type filter_type)
 {
 	u32 idx;
 	u64 end;
@@ -600,8 +600,8 @@ __init void e820__range_remove(u64 start, u64 size, enum e820_type old_type, boo
 
 	end = start + size;
 	printk(KERN_DEBUG "e820: remove [mem %#010Lx-%#010Lx]", start, end - 1);
-	if (check_type)
-		e820_print_type(old_type);
+	if (filter_type)
+		e820_print_type(filter_type);
 	pr_cont("\n");
 
 	for (idx = 0; idx < e820_table->nr_entries; idx++) {
@@ -609,7 +609,7 @@ __init void e820__range_remove(u64 start, u64 size, enum e820_type old_type, boo
 		u64 final_start, final_end;
 		u64 entry_end;
 
-		if (check_type && entry->type != old_type)
+		if (filter_type && entry->type != filter_type)
 			continue;
 
 		entry_end = entry->addr + entry->size;
@@ -945,7 +945,7 @@ __init static int parse_memopt(char *p)
 	if (mem_size == 0)
 		return -EINVAL;
 
-	e820__range_remove(mem_size, ULLONG_MAX - mem_size, E820_TYPE_RAM, 1);
+	e820__range_remove(mem_size, ULLONG_MAX - mem_size, E820_TYPE_RAM);
 
 #ifdef CONFIG_MEMORY_HOTPLUG
 	max_mem_size = mem_size;
@@ -1001,12 +1001,10 @@ __init static int parse_memmap_one(char *p)
 			e820__range_update(start_at, mem_size, from, to);
 		else if (to)
 			e820__range_add(start_at, mem_size, to);
-		else if (from)
-			e820__range_remove(start_at, mem_size, from, 1);
 		else
-			e820__range_remove(start_at, mem_size, 0, 0);
+			e820__range_remove(start_at, mem_size, from);
 	} else {
-		e820__range_remove(mem_size, ULLONG_MAX - mem_size, E820_TYPE_RAM, 1);
+		e820__range_remove(mem_size, ULLONG_MAX - mem_size, E820_TYPE_RAM);
 	}
 
 	return *p == '\0' ? 0 : -EINVAL;
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 83cc265140c0..0f4d78939005 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -736,7 +736,7 @@ static void __init trim_bios_range(void)
 	 * area (640Kb -> 1Mb) as RAM even though it is not.
 	 * take them out.
 	 */
-	e820__range_remove(BIOS_BEGIN, BIOS_END - BIOS_BEGIN, E820_TYPE_RAM, 1);
+	e820__range_remove(BIOS_BEGIN, BIOS_END - BIOS_BEGIN, E820_TYPE_RAM);
 
 	e820__update_table(e820_table);
 }
@@ -758,7 +758,7 @@ static void __init e820_add_kernel_range(void)
 		return;
 
 	pr_warn(".text .data .bss are not marked as E820_TYPE_RAM!\n");
-	e820__range_remove(start, size, E820_TYPE_RAM, 0);
+	e820__range_remove(start, size, 0);
 	e820__range_add(start, size, E820_TYPE_RAM);
 }
 
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 463b784499a8..d00c6de7f3b7 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -333,8 +333,7 @@ static void __init efi_remove_e820_mmio(void)
 			if (size >= 256*1024) {
 				pr_info("Remove mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluMB) from e820 map\n",
 					i, start, end, size >> 20);
-				e820__range_remove(start, size,
-						   E820_TYPE_RESERVED, 1);
+				e820__range_remove(start, size, E820_TYPE_RESERVED);
 			} else {
 				pr_info("Not removing mem%02u: MMIO range=[0x%08llx-0x%08llx] (%lluKB) from e820 map\n",
 					i, start, end, size >> 10);
-- 
2.45.2
Re: [PATCH 27/29] x86/boot/e820: Simplify the e820__range_remove() API
Posted by Andy Shevchenko 7 months, 3 weeks ago 
On Mon, Apr 21, 2025 at 08:52:07PM +0200, Ingo Molnar wrote:
> Right now e820__range_remove() has two parameters to control the
> E820 type of the range removed:
> 
> 	extern void e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type);
> 
> Since E820 types start at 1, zero has a natural meaning of 'no type.
> 
> Consolidate the (old_type,check_type) parameters into a single (filter_type)
> parameter:
> 
> 	extern void e820__range_remove(u64 start, u64 size, enum e820_type filter_type);
> 
> Note that both e820__mapped_raw_any() and e820__mapped_any()
> already have such semantics for their 'type' parameter, although
> it's currently not used with '0' by in-kernel code.
> 
> Also, the __e820__mapped_all() internal helper already has such
> semantics implemented as well, and the e820__get_entry_type() API
> uses the '0' type to such effect.
> 
> This simplifies not just e820__range_remove(), and synchronizes its
> use of type filters with other E820 API functions, but simplifies
> usage sites as well, such as parse_memmap_one(), beyond the reduction
> of the number of parameters:
> 
>   -               else if (from)
>   -                       e820__range_remove(start_at, mem_size, from, 1);
>                   else
>   -                       e820__range_remove(start_at, mem_size, 0, 0);
>   +                       e820__range_remove(start_at, mem_size, from);
> 
> The generated code gets smaller as well:
> 
> 	add/remove: 0/0 grow/shrink: 0/5 up/down: 0/-66 (-66)
> 
> 	Function                                     old     new   delta
> 	parse_memopt                                 112     107      -5
> 	efi_init                                    1048    1039      -9
> 	setup_arch                                  2719    2709     -10
> 	e820__range_remove                           283     273     -10
> 	parse_memmap_opt                             559     527     -32
> 
> 	Total: Before=22,675,600, After=22,675,534, chg -0.00%

>  extern void e820__range_add   (u64 start, u64 size, enum e820_type type);
>  extern u64  e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
> -extern void e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type);
> +extern void e820__range_remove(u64 start, u64 size, enum e820_type filter_type);
>  extern u64  e820__range_update_table(struct e820_table *t, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
>  
>  extern int  e820__update_table(struct e820_table *table);

Wondering if are going to get rid of 'extern' for the functions...

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH 27/29] x86/boot/e820: Simplify the e820__range_remove() API
Posted by Ingo Molnar 7 months ago 
* Andy Shevchenko <andy@kernel.org> wrote:

> On Mon, Apr 21, 2025 at 08:52:07PM +0200, Ingo Molnar wrote:
> > Right now e820__range_remove() has two parameters to control the
> > E820 type of the range removed:
> > 
> > 	extern void e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type);
> > 
> > Since E820 types start at 1, zero has a natural meaning of 'no type.
> > 
> > Consolidate the (old_type,check_type) parameters into a single (filter_type)
> > parameter:
> > 
> > 	extern void e820__range_remove(u64 start, u64 size, enum e820_type filter_type);
> > 
> > Note that both e820__mapped_raw_any() and e820__mapped_any()
> > already have such semantics for their 'type' parameter, although
> > it's currently not used with '0' by in-kernel code.
> > 
> > Also, the __e820__mapped_all() internal helper already has such
> > semantics implemented as well, and the e820__get_entry_type() API
> > uses the '0' type to such effect.
> > 
> > This simplifies not just e820__range_remove(), and synchronizes its
> > use of type filters with other E820 API functions, but simplifies
> > usage sites as well, such as parse_memmap_one(), beyond the reduction
> > of the number of parameters:
> > 
> >   -               else if (from)
> >   -                       e820__range_remove(start_at, mem_size, from, 1);
> >                   else
> >   -                       e820__range_remove(start_at, mem_size, 0, 0);
> >   +                       e820__range_remove(start_at, mem_size, from);
> > 
> > The generated code gets smaller as well:
> > 
> > 	add/remove: 0/0 grow/shrink: 0/5 up/down: 0/-66 (-66)
> > 
> > 	Function                                     old     new   delta
> > 	parse_memopt                                 112     107      -5
> > 	efi_init                                    1048    1039      -9
> > 	setup_arch                                  2719    2709     -10
> > 	e820__range_remove                           283     273     -10
> > 	parse_memmap_opt                             559     527     -32
> > 
> > 	Total: Before=22,675,600, After=22,675,534, chg -0.00%
> 
> >  extern void e820__range_add   (u64 start, u64 size, enum e820_type type);
> >  extern u64  e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
> > -extern void e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type);
> > +extern void e820__range_remove(u64 start, u64 size, enum e820_type filter_type);
> >  extern u64  e820__range_update_table(struct e820_table *t, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
> >  
> >  extern int  e820__update_table(struct e820_table *table);
> 
> Wondering if are going to get rid of 'extern' for the functions...

Symmetrical use of storage classes provide useful documentation:

 - I kinda like the immediate visual reminder of 'extern' that these 
   are exported API functions and not a function definition or 
   something else.

 - Just like 'static void ...' is an immediate visual reminder that the 
   following function definition is local scope, or 'static inline' in 
   a header is an immediate reminder that it's an inline API.

We use such symmetric taggint in other places: we don't write 
'unsigned' instead of 'unsigned int', just because we can.

But no strong feelings either way, as long as it's consistent within 
the subsystem. The wider kernel is certainly using both approaches.

Thanks,

	Ingo

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.