linux-kernel - Re: [PATCH 27/29] x86/boot/e820: Simplify the e820__range

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aCXU0x6lqJ0J7W5D@gmail.com>
Date: Thu, 15 May 2025 13:49:39 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Andy Shevchenko <andy@...nel.org>
Cc: linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...nel.org>,
	Borislav Petkov <bp@...en8.de>, Juergen Gross <jgross@...e.com>,
	"H . Peter Anvin" <hpa@...or.com>,
	Kees Cook <keescook@...omium.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Mike Rapoport <rppt@...nel.org>,
	Paul Menzel <pmenzel@...gen.mpg.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	David Woodhouse <dwmw@...zon.co.uk>
Subject: Re: [PATCH 27/29] x86/boot/e820: Simplify the e820__range_remove()
 API


* Andy Shevchenko <andy@...nel.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