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: 4234 bytes --]

On 05.05.26 12:24, Jan Beulich wrote:
> On 05.05.2026 11:13, Jürgen Groß wrote:
>> 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.
> 
> Oh, wow, and this is a property of the function that one can rely upon?

It is documented to be handled this way.

>> 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).
> 
> Aren't such holes normal to occur, e.g. on misaligned RAM/UNUSABLE
> boundaries?

This can happen, yes, but it should not be the case in the area where the
kernel is actually located.

> 
>>> 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.
> 
> Well, first and foremost: xen_add_remap_nonram() takes and stores byte-
> granular addresses / sizes, with the sole requirement being that the
> offset-into-page be identical between both addresses. That check alone
> already indicates that non-page-aligned addresses are expected to be
> passed into here.

I'd say "tolerated" instead of "expected".

> Further, xen_acpi_os_ioremap() uses the resulting remap table, and is
> byte granular. With the physical address adjustment there, both mappings
> could (theoretically) coexist. But the problem I'm trying to point out
> is that by passing the page-aligned superset into xen_add_remap_nonram()
> you misguide xen_acpi_os_ioremap() (while at the same time
> xen_do_remap_nonram() will do suitable rounding to page boundaries even
> if exact addresses were passed).

Ah, okay, now I understand your concern.

I'm on the edge whether a change is wanted or not. The current implementation
is correct, while I agree that using non-page-aligned addresses should work.

OTOH using a superset is fine, too. Especially as the remap is done based on
memory map entries, while the caller of xen_acpi_os_ioremap() will act based
on ACPI table entries. It is perfectly fine to have multiple NVS records in
an area covered by a single memory map entry, so calling xen_acpi_os_ioremap()
only for a part of a remap entry isn't weird at all.

So the implementation needs to ensure that a remap entry is allowed to be a
superset of an area mapped via xen_acpi_os_ioremap(), resulting in no need to
modify the current coding.

As this whole handling was added to support a very rare case, I'd rather not
risk to break that case by doing cosmetic changes. OTOH I wouldn't NACK a patch.


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