On 19.03.2025 15:20, Andrew Cooper wrote:
> The current CI failures happen to be a latent bug triggered by a narrow set of
> properties of the initrd, which CI encountered by chance.
Plus properties of the host memory map.
> One step during boot involves constructing directmap mappings for modules.
> With some probing at the point of creation, it is observed that there's a 4k
> mapping missing towards the end of the initrd.
>
> (XEN) === Mapped Mod1 [0000000394001000, 00000003be1ff6dc] to Directmap
> (XEN) Probing paddr 394001000, va ffff830394001000
> (XEN) Probing paddr 3be1ff6db, va ffff8303be1ff6db
> (XEN) Probing paddr 3bdffffff, va ffff8303bdffffff
> (XEN) Probing paddr 3be001000, va ffff8303be001000
> (XEN) Probing paddr 3be000000, va ffff8303be000000
> (XEN) Early fatal page fault at e008:ffff82d04032014c
> (cr2=ffff8303be000000, ec=0000)
>
> The conditions for this bug appear to be map_pages_to_xen() call with a non-2M
> aligned start address, some number of full 2M pages, then a tail needing 4k
> pages.
>
> Anyway, the condition for spotting superpage boundaries in map_pages_to_xen()
> is wrong. The IS_ALIGNED() macro expects a power of two for the alignment
> argument, and subtracts 1 itself.
>
> Fixing this causes the failing case to now boot.
>
> Fixes: 97fb6fcf26e8 ("x86/mm: introduce helpers to detect super page
> alignment")
> Debugged-by: Marek Marczykowski-Górecki <marma...@invisiblethingslab.com>
> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> Tested-by: Marek Marczykowski-Górecki <marma...@invisiblethingslab.com>
Reviewed-by: Jan Beulich <jbeul...@suse.com>
> Judging by how IS_ALIGNED() is wrong, I think the pre-condition might be
> exactly 4k past a 2M boundary, not just simply a non-2M boundary.
That's the understanding I have gained, yes.
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5505,7 +5505,7 @@ int map_pages_to_xen(
> \
> ASSERT(!mfn_eq(m_, INVALID_MFN)); \
> IS_ALIGNED(PFN_DOWN(v) | mfn_x(m_), \
> - (1UL << (PAGETABLE_ORDER * ((n) - 1))) - 1); \
> + (1UL << (PAGETABLE_ORDER * ((n) - 1)))); \
Can we also get rid of the now unnecessary outermost pair of parentheses
of that operand, please? Imo any reduction in parentheses in constructs
like this helps readability.
Jan
