Re: [PATCH v2 3/4] x86/setup: remove bootstrap_map_addr() usage of destroy_xen_mappings()

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

On Fri, Nov 08, 2024 at 09:41:35AM +0000, Andrew Cooper wrote:
> On 06/11/2024 12:29 pm, Roger Pau Monne wrote:
> > bootstrap_map_addr() top level comment states that it doesn't indented to
> > remove the L2 tables, as the same L2 will be re-used to create further 2MB
> > mappings.  It's incorrect for the function to use destroy_xen_mappings() which
> > will free empty L2 tables.
> >
> > Fix this by using map_pages_to_xen(), which does zap the page-table entries,
> > but does not free page-table structures even when empty.
> >
> > Fixes: 4376c05c3113 ('x86-64: use 1GB pages in 1:1 mapping if available')
> > Signed-off-by: Roger Pau Monné <roger.pau@ctrix.com>
> > ---
> > The fixes tag reflects the fact that if 4376c05c3113 freed the L2 correctly
> > when empty, it would have become obvious that bootstrap_map_addr() shouldn't be
> > using it if it wants to keep the L2.  4376c05c3113 should have switched
> > bootstrap_map_addr() to not use destroy_xen_mappings().
> > ---
> >  xen/arch/x86/setup.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 177f4024abca..815b8651ba79 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -456,7 +456,9 @@ static void *__init bootstrap_map_addr(paddr_t start, paddr_t end)
> >  
> >      if ( !end )
> >      {
> > -        destroy_xen_mappings(BOOTSTRAP_MAP_BASE, BOOTSTRAP_MAP_LIMIT);
> > +        map_pages_to_xen(BOOTSTRAP_MAP_BASE, INVALID_MFN,
> > +                         PFN_DOWN(map_cur - BOOTSTRAP_MAP_BASE),
> > +                         _PAGE_NONE);
> >          map_cur = BOOTSTRAP_MAP_BASE;
> 
> One option to consider is this.
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index eac8488c4ca5..b317a4d12f55 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -461,8 +461,13 @@ static void *__init bootstrap_map_addr(paddr_t
> start, paddr_t end)
>  
>      if ( !end )
>      {
> -        destroy_xen_mappings(BOOTSTRAP_MAP_BASE, BOOTSTRAP_MAP_LIMIT);
> -        map_cur = BOOTSTRAP_MAP_BASE;
> +        if ( map_cur > BOOTSTRAP_MAP_BASE )
> +        {
> +            memset(&l2_bootmap[l2_table_offset(BOOTSTRAP_MAP_BASE)],
> +                   0, (map_cur - BOOTSTRAP_MAP_BASE) >>
> L2_PAGETABLE_SHIFT);
> +            flush_tlb_local();
> +            map_cur = BOOTSTRAP_MAP_BASE;
> +        }
>          return NULL;
>      }
>  
> We know for certain there's nothing to free, and and this far less logic
> than either destroy_xen_mappings() or map_pages_to_xen().

Should we then also consider using l2_bootmap directly when creating
the mappings?  So that we have symmetry between the map and unmap
logic.

I think it might be better do to this change as a followup patch, as I
would like to change both the map and unmap paths at the same time.

Thanks, Roger.