Re: [PATCH] x86/boot: Fix zap_low_mappings() to map less of the trampoline

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.03.2025 21:47, Andrew Cooper wrote:
> On 06/01/2025 11:54 am, Jan Beulich wrote:
>> On 06.01.2025 12:26, Andrew Cooper wrote:
>>> Regular data access into the trampoline is via the directmap.
>>>
>>> As now discussed quite extensively in asm/trampoline.h, the trampoline is
>>> arranged so that only the AP and S3 paths need an identity mapping, and that
>>> they fit within a single page.
>>>
>>> Right now, PFN_UP(trampoline_end - trampoline_start) is 2, causing more than
>>> expected of the trampoline to be mapped.  Cut it down just the single page 
>>> it
>>> ought to be.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Thanks.  However,
> 
>> on the basis that this improves things. However, ...
>>
>>> --- a/xen/arch/x86/x86_64/mm.c
>>> +++ b/xen/arch/x86/x86_64/mm.c
>>> @@ -718,14 +718,16 @@ void __init zap_low_mappings(void)
>>>  {
>>>      BUG_ON(num_online_cpus() != 1);
>>>  
>>> -    /* Remove aliased mapping of first 1:1 PML4 entry. */
>>> +    /* Stop using l?_bootmap[] mappings. */
>>>      l4e_write(&idle_pg_table[0], l4e_empty());
>>>      flush_local(FLUSH_TLB_GLOBAL);
>>>  
>>> -    /* Replace with mapping of the boot trampoline only. */
>>> +    /*
>>> +     * Insert an identity mapping of the AP/S3 part of the trampoline, 
>>> which
>>> +     * is arranged to fit in a single page.
>>> +     */
>>>      map_pages_to_xen(trampoline_phys, maddr_to_mfn(trampoline_phys),
>>> -                     PFN_UP(trampoline_end - trampoline_start),
>>> -                     __PAGE_HYPERVISOR_RX);
>>> +                     1, __PAGE_HYPERVISOR_RX);
>> ... literal numbers like this - however well they are commented - are
>> potentially problematic to locate in case something changes significantly.
>> The 1 here really would want connecting with the .equ establishing
>> wakeup_stack.
> 
> how do you propose doing this?
> 
> PFN_UP(wakeup_stack - trampoline_start) doesn't have the same obvious
> connection, and it would involve partly undoing 7d73c6f196a5 which hid
> the symbol recently.
> 
> While 1 isn't ideal, it is next to a comment explaining what's going on,
> and it's not going to go stale in a silent way...  (It's also not liable
> to go stale either.)

If in

        .equ    wakeup_stack, trampoline_start + PAGE_SIZE

PAGE_SIZE was replaced by a new (in asm/trampoline.h) TRAMPOLINE_PERM_SIZE,
you could use PFN_UP(TRAMPOLINE_PERM_SIZE) here to establish a connection.

I have to admit I also don't really see why things going stale here would
(a) be unlikely and (b) be guaranteed to not go silently. We just don't
know what we may need to add to the trampoline, sooner or later.

Jan