Re: [PATCH v3 2/4] x86/mm: skip super-page alignment checks for non-present entries

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Nov 15, 2024 at 10:09:39AM +0100, Jan Beulich wrote:
> On 14.11.2024 15:57, Roger Pau Monne wrote:
> > @@ -5517,7 +5524,8 @@ int map_pages_to_xen(
> >          L3T_LOCK(current_l3page);
> >          ol3e = *pl3e;
> >  
> > -        if ( cpu_has_page1gb && IS_L3E_ALIGNED(virt, mfn) &&
> > +        if ( cpu_has_page1gb &&
> > +             (!(flags & _PAGE_PRESENT) || IS_L3E_ALIGNED(virt, mfn)) &&
> >               nr_mfns >= (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) &&
> >               !(flags & (_PAGE_PAT | MAP_SMALL_PAGES)) )
> >          {
> > @@ -5636,7 +5644,7 @@ int map_pages_to_xen(
> >          if ( !pl2e )
> >              goto out;
> >  
> > -        if ( IS_L2E_ALIGNED(virt, mfn) &&
> > +        if ( (!(flags & _PAGE_PRESENT) || IS_L2E_ALIGNED(virt, mfn)) &&
> >               (nr_mfns >= (1u << PAGETABLE_ORDER)) &&
> >               !(flags & (_PAGE_PAT|MAP_SMALL_PAGES)) )
> >          {
> 
> Upon inspecting Andrew's report of crashes I noticed that this can't be quite
> right. We can't entirely skip the alignment check when non-present mappings
> are requested; we merely need to limit the check to the VA. In a reply to
> the 1st v2 I actually had it arranged to match that requirement:
> 
>         if ( (cpu_has_page1gb || !(flags & _PAGE_PRESENT)) &&
>              IS_L3E_ALIGNED(virt, flags & _PAGE_PRESENT ? mfn : _mfn(0)) &&
>              nr_mfns >= (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) &&
>              !(flags & (_PAGE_PAT | MAP_SMALL_PAGES)) )
> 
> Yet then I didn't pay attention to the difference when reviewing the 2nd v2
> (that versioning issue of course isn't helping here either).
> 
> I'm afraid I can't (yet) connect the observed bad behavior with this issue,
> though.

I think this might be caused by map_pages_to_xen() now freeing vmap
regions still under use, and that seems to manifest with the
memnodemap array being unintentionally unmapped (because it's
allocated with vmap_contig()).  See the usage of mfn_to_nid() in
free_heap_pages().

Will prepare a patch, sorry.

Thanks, Roger.