Re: [PATCH] x86/xen: Fix a potential problem in xen_e820_resolve_conflicts()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.05.2026 10:06, Juergen Gross wrote:
> When fixing a conflict in xen_e820_resolve_conflicts(), the loop over
> the E820 map entries needs to be restarted, as the E820 map will have
> been modified by the fix. Otherwise entries might be skipped by
> accident.
> 
> Fixes: be35d91c8880 ("xen: tolerate ACPI NVS memory overlapping with Xen 
> allocated memory")
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

First, while trying to review this, isn't there another issue in
xen_e820_swap_entry_with_ram(), in that

                        entry->addr = entry_end - swap_size +
                                      swap_addr - swap_entry->addr;


really means to be

                        entry->addr = entry_end - swap_size +
                                      swap_entry->addr - swap_addr;

(affecting non-page-aligned E820 entries)?

Further, that function converts swap_entry to the page-aligned superset
of the passed in range. How is it guaranteed that this new range won't
overlap with the predecessor and/or successor one? Wouldn't that need
to be conversion to the page-aligned subset instead?

And then, is passing the page-aligned superset to xen_add_remap_nonram()
really appropriate? Why would any leading or trailing space there be
subject to remapping?

> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -695,17 +695,22 @@ static void __init 
> xen_e820_resolve_conflicts(phys_addr_t start,
>               return;
>  
>       end = start + size;
> -     entry = xen_e820_table.entries;
> +     mapcnt = 0;
>  
> -     for (mapcnt = 0; mapcnt < xen_e820_table.nr_entries; mapcnt++) {
> +     while (mapcnt < xen_e820_table.nr_entries) {
> +             entry = xen_e820_table.entries + mapcnt;
>               if (entry->addr >= end)
>                       return;
>  
>               if (entry->addr + entry->size > start &&
> -                 entry->type == E820_TYPE_NVS)
> +                 entry->type == E820_TYPE_NVS) {
>                       xen_e820_swap_entry_with_ram(entry);
> +                     /* E820 map has been changed, restart loop! */
> +                     mapcnt = 0;
> +                     continue;
> +             }
>  
> -             entry++;
> +             mapcnt++;
>       }
>  }

Given what exactly xen_e820_swap_entry_with_ram() does, restarting from
entry 0 looks to be needed only if the non-RAM entry ended up moving down
(strictly speaking even there it wouldn't need to be entry 0). If it
moved up, simply not incrementing mapcnt would look to suffice. Since the
extra overhead is likely tolerable here (with simplicity of the code
being more important), this may want mentioning in a code comment (or at
least the description). Preferably with that:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan