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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.11.2024 10:41, 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@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().

Yet then such open-coding can badly bite us at other times.

Jan