Re: [PATCH] x86: allow non-BIGMEM configs to boot on >= 16Tb systems

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Jun 07, 2023 at 08:17:30AM +0200, Jan Beulich wrote:
> While frame table setup, directmap init, and boot allocator population
> respect all intended bounds, the logic passing memory to the heap
> allocator which wasn't passed to the boot allocator fails to respect
> max_{pdx,pfn}. This then typically triggers the BUG() in
> free_heap_pages() after checking page state, because of hitting a struct
> page_info instance which was set to all ~0.
> 
> Of course all the memory above the 16Tb boundary is still going to
> remain unused; using it requires BIGMEM=y. And of course this fix
> similarly ought to help BIGMEM=y configurations on >= 123Tb systems
> (where all the memory beyond that boundary continues to be unused).
> 
> Fixes: bac2000063ba ("x86-64: reduce range spanned by 1:1 mapping and frame table indexes")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> Sadly the people reporting the issue have decided to go with the 16Tb
> limit, and hence the patch wasn't tested by them. I thought that I'd
> still post it, though.

We should see about finding a way to test BIGMEM, maybe with PoD.

> The "must not be passed to the boot allocator" for the range in question
> may already not be applicable anymore, with all page tables now being
> mapped via map_domain_page() (iirc this work has been completed). But of
> course there would be a risk that something else is/was overlooked, and
> hence the offending code is being fixed rather than purged (and the
> purging should occur once the directmap is properly gone). (This also
> seems preferable for potential backports of this change.)
> 
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1722,15 +1722,16 @@ void __init noreturn __start_xen(unsigne
>  
>      if ( max_page - 1 > virt_to_mfn(HYPERVISOR_VIRT_END - 1) )
>      {
> -        unsigned long limit = virt_to_mfn(HYPERVISOR_VIRT_END - 1);
> +        unsigned long lo = virt_to_mfn(HYPERVISOR_VIRT_END - 1);
> +        unsigned long hi = pdx_to_pfn(max_pdx - 1) + 1;

Maybe use max_page to avoid the pdx_to_pfn() call?  (And is also more
in context with the condition on the outside if).

Thanks, Roger.