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

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3375 bytes --]

On 05.05.26 10:43, Jan Beulich wrote:
> 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@suse.com>
> 
> 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)?

Yes, you are right.

> 
> 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?

This is subtle. :-)

We are converting to RAM (usable), so the type value is 1. e820__update_table()
will handle overlaps just fine, with higher type values "winning" against lower
ones. So any other region overlapping with the new RAM region will result in
another conflict in the next loop iteration.

Using the page-aligned subset would result in possible memory holes, which would
be problematic (the kernel or page tables shouldn't have holes, after all).

> 
> 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?

How would you want to remap a sub-page physical memory area to another location
without affecting the rest of the page? We are reworking the final p2m map here.

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

Thanks,


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]