Re: [PATCH 07/15] x86/PVH: lift E820 alignment requirement

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

On Mon, Oct 12, 2026 at 10:22:28AM +0200, Jan Beulich wrote:
> Before reordering things (to move in particular pvh_steal_ram()
> invocations ahead of the pvh_populate_p2m() one) drop an undue (and as it
> turns out unnecessary) assumption: find_memory() doesn't really depend on
> E820 regions to end on page boundaries.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Really the assumption towards the start of regions doesn't look to be
> relevant either, yet I thought I'd limit the change to the minimum needed.
> (In fact the statement immediately ahead of the assertion arranges for
> page alignment, so the check is dubious altogether.)
> 
> We also could consider whether kernel_end really needs rounding up to a
> page boundary: Technically there's no need, but the question is whether it
> would be a good idea to have the tail of the kernel region share a page
> with metadata (or, as things still are at this point of the series, the
> initrd).

I think it would a bad idea to have this kind of page-sharing.  The
guest might use NX for metadata page-tables (ie: initrd), so it
sharing a page with the kernel could be problematic.

> 
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -365,6 +365,9 @@ static __init void pvh_setup_e820(struct
>           * that's the minimum granularity of the 2nd stage translation. Since
>           * the p2m code uses PAGE_ORDER_4K internally, also use it here in
>           * order to prevent this code from getting out of sync.
> +         *
> +         * Note that this is only for pvh_populate_p2m().  Subsequent uses of
> +         * pvh_steal_ram() will break the length part.

Maybe because I'm too familiar with this code, but the previous
comment mentioning the 4K alignment being required for the 2nd stage
translation kind of implies this is the reason for it?  I guess you
want to make it clear there is no further logic depending on that
alignment.

>           */
>          start = ROUNDUP(entry->addr, PAGE_SIZE << PAGE_ORDER_4K);
>          end = (entry->addr + entry->size) &
> @@ -506,8 +509,8 @@ static paddr_t __init find_memory(
>      unsigned int i;
>  
>      /*
> -     * The memory map is sorted and all RAM regions starts and sizes are
> -     * aligned to page boundaries.
> +     * The memory map is sorted and all RAM region starts are aligned to
> +     * page boundaries.
>       */
>      for ( i = 0; i < d->arch.nr_e820; i++ )
>      {
> @@ -519,7 +522,7 @@ static paddr_t __init find_memory(
>  
>          start = MAX(ROUNDUP(d->arch.e820[i].addr, PAGE_SIZE), MB(1));
>  
> -        ASSERT(IS_ALIGNED(start, PAGE_SIZE) && IS_ALIGNED(end, PAGE_SIZE));
> +        ASSERT(IS_ALIGNED(start, PAGE_SIZE));

I would remove this altogether - as you noticed start is clearly
aligned by the code in the line just above.

Thanks, Roger.