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

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

On Thu, Nov 07, 2024 at 12:23:51PM +0100, Jan Beulich wrote:
> On 06.11.2024 13:29, 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.
> 
> With that I assume you refer to the 2nd sentence in the comment immediately
> ahead of the function. According to my reading, it may imply what you say,
> but it certainly doesn't "state" this. Imo the problem was latent already
> before, if BOOTSTRAP_MAP_{BASE,LIMIT} were changed to cover at least one
> full L3E range. Which isn't to say that ...

Hm, maybe I've implied too much from that comment.  What about
replacing the first paragraph with:

"bootstrap_map_addr() needs to be careful to not remove existing
page-table structures when tearing down mappings, as such pagetable
structures might be needed to fulfill subsequent mappings requests.
The comment ahead of the function already notes that pagetable memory
shouldn't be allocated."

> >  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().
> 
> ... I mind the commit you name to be blamed. It was clearly something I
> missed back then.
> 
> With the 1st sentence of the description re-worded some:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks, Roger.