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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxx>
> ---
> 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().

~Andrew