Re: [PATCH 25/32] x86/boot/e820: Simplify append_e820_table() and remove restriction on single-entry tables

[Nikolay Borisov]

On 5/15/25 15:05, Ingo Molnar wrote:
> So append_e820_table() begins with this weird condition that checks 'nr_entries':
>
>      static int __init append_e820_table(struct boot_e820_entry *entries, u32 nr_entries)
>      {
>              /* Only one memory region (or negative)? Ignore it */
>              if (nr_entries < 2)
>                      return -1;
>
> Firstly, 'nr_entries' has been an u32 since 2017 and cannot be negative.
>
> Secondly, there's nothing inherently wrong with single-entry E820 maps,
> especially in virtualized environments.
>
> So remove this restriction and remove the __append_e820_table()
> indirection.
>
> Also:
>
>   - fix/update comments
>   - remove obsolete comments
>
> This shrinks the generated code a bit as well:
>
>     text       data        bss        dec        hex    filename
>     7549      44072          0      51621       c9a5    e820.o.before
>     7533      44072          0      51605       c995    e820.o.after
>
> Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Andy Shevchenko <andy@xxxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxxxx>
> Cc: David Woodhouse <dwmw@xxxxxxxxxxxx>
> Cc: H. Peter Anvin <hpa@xxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>
> ---
>   arch/x86/kernel/e820.c | 35 +++++++++++------------------------
>   1 file changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 447f8bbb77b8..22bfcad7b723 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -483,17 +483,22 @@ __init int e820__update_table(struct e820_table *table)
>   	return 0;
>   }
>   
> -__init static int __append_e820_table(struct boot_e820_entry *entries, u32 nr_entries)
> +/*
> + * Copy the BIOS E820 map into the kernel's e820_table.
> + *
> + * Sanity-check it while we're at it..
> + */
> +__init static int append_e820_table(struct boot_e820_entry *entries, u32 nr_entries)
>   {
>   	struct boot_e820_entry *entry = entries;
>   
>   	while (nr_entries) {
>   		u64 start = entry->addr;
> -		u64 size = entry->size;
> -		u64 end = start + size - 1;
> -		u32 type = entry->type;
> +		u64 size  = entry->size;
> +		u64 end   = start + size-1;
> +		u32 type  = entry->type;
>   
> -		/* Ignore the entry on 64-bit overflow: */
> +		/* Ignore the remaining entries on 64-bit overflow: */
>   		if (start > end && likely(size))
>   			return -1;

nit: All this function wants to do is simply iterate an array, I'd say
the standard way to iterate an array is to use a 'for' loop.

How about doing this instead (on top of this patch):

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 22bfcad7b723..41aa26eae594 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -490,9 +490,9 @@ __init int e820__update_table(struct e820_table *table)
  */
 __init static int append_e820_table(struct boot_e820_entry *entries, u32 nr_entries)
 {
-       struct boot_e820_entry *entry = entries;
 
-       while (nr_entries) {
+       for (int i = 0; i < nr_entries; i++) {
+               struct boot_e820_entry *entry = entries + i;
                u64 start = entry->addr;
                u64 size  = entry->size;
                u64 end   = start + size-1;
@@ -503,9 +503,6 @@ __init static int append_e820_table(struct boot_e820_entry *entries, u32 nr_entr
                        return -1;
 
                e820__range_add(start, size, type);
-
-               entry++;
-               nr_entries--;
        }
        return 0;
 }




>   
> @@ -505,24 +510,6 @@ __init static int __append_e820_table(struct boot_e820_entry *entries, u32 nr_en
>   	return 0;
>   }
>   
> -/*
> - * Copy the BIOS E820 map into a safe place.
> - *
> - * Sanity-check it while we're at it..
> - *
> - * If we're lucky and live on a modern system, the setup code
> - * will have given us a memory map that we can use to properly
> - * set up memory.  If we aren't, we'll fake a memory map.
> - */
> -__init static int append_e820_table(struct boot_e820_entry *entries, u32 nr_entries)
> -{
> -	/* Only one memory region (or negative)? Ignore it */
> -	if (nr_entries < 2)
> -		return -1;
> -
> -	return __append_e820_table(entries, nr_entries);
> -}
> -
>   __init static u64
>   __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type)
>   {
> @@ -796,7 +783,7 @@ __init void e820__memory_setup_extended(u64 phys_addr, u32 data_len)
>   	entries = sdata->len / sizeof(*extmap);
>   	extmap = (struct boot_e820_entry *)(sdata->data);
>   
> -	__append_e820_table(extmap, entries);
> +	append_e820_table(extmap, entries);
>   	e820__update_table(e820_table);
>   
>   	memcpy(e820_table_kexec, e820_table, sizeof(*e820_table_kexec));