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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.10.2026 16:59, Roger Pau Monné wrote:
> 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.

Okay, will not pursue this then.

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

To me it seemed as if the other comments / assumptions were (at least in
part) building on what's said here. I.e. yes, I meant to verbally break
any such connections people may have memorized. If you think I'm going
too far with this comment addition - getting rid of it again is easy.

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

Will do. I kept this part because it wasn't really the first time round
that I had noticed the check being pointless. I.e. there still was that
chance that I might have been missing something.

Alternatively I could drop the ROUNDUP() from the assignment of start.
Else I think the comment ahead of the loop may want altering further.

Jan