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

[Jürgen Groß <jgross@xxxxxxxx>]

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@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)?
> > 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: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature