x86/boot: Fix zap_low_mappings() to map less of the trampoline

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

Posted by Andrew Cooper 10 months ago

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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

There's not an obvious candidate for a Fixes tag.
---
 xen/arch/x86/x86_64/mm.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 389d813ebe63..d4e6a9c0a2e0 100644
--- 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);
 }
 
 int setup_compat_arg_xlat(struct vcpu *v)
-- 
2.39.5

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

Posted by Jan Beulich 10 months ago

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@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
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.

Jan

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

Posted by Andrew Cooper 7 months, 3 weeks ago

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@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

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.)

~Andrew

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

Posted by Jan Beulich 7 months, 3 weeks ago

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@citrix.com>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> 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

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

Posted by Roger Pau Monné 7 months, 3 weeks ago

On Wed, Mar 12, 2025 at 09:31:37AM +0100, Jan Beulich wrote:
> 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@citrix.com>
> >> Acked-by: Jan Beulich <jbeulich@suse.com>
> > 
> > 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.

Maybe we could introduce trampoline_{ap?,runtime?}_{start,end} that
covers this more narrow section of the trampoline code?

Thanks, Roger.

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

Posted by Andrew Cooper 7 months, 3 weeks ago

On 12/03/2025 9:57 am, Roger Pau Monné wrote:
> On Wed, Mar 12, 2025 at 09:31:37AM +0100, Jan Beulich wrote:
>> 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@citrix.com>
>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>> 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.

The size can't go to 0 or everything will break, and if it goes larger
than 1 (which it almost certainly never will), then APs and/or S3 will
break, and we've got both of these in CI.

Furthermore, the actual thing which matters is:

> /* Map the permanent trampoline page into l1_bootmap[]. */
> mov     sym_esi(trampoline_phys), %ecx
> lea     __PAGE_HYPERVISOR_RX(%ecx), %edx /* %edx = PTE to write  */
> shr     $PAGE_SHIFT, %ecx                /* %ecx = Slot to write */
> mov     %edx, sym_offs(l1_bootmap)(%esi, %ecx, 8)

which hardcodes 1 page, because there's almost certainly no chance this
will ever change.

>>  We just don't
>> know what we may need to add to the trampoline, sooner or later.
> Maybe we could introduce trampoline_{ap?,runtime?}_{start,end} that
> covers this more narrow section of the trampoline code?

We already have one of those, and a linker assertion that it stays below
1k, so wakeup_stack is at least 3k.

The complexity is that the wakeup_stack overlays some init-only logic in
the placed trampoline.  It's not something that exists concretely in the
Xen image.

~Andrew

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

Posted by Jan Beulich 7 months, 2 weeks ago

On 14.03.2025 20:00, Andrew Cooper wrote:
> On 12/03/2025 9:57 am, Roger Pau Monné wrote:
>> On Wed, Mar 12, 2025 at 09:31:37AM +0100, Jan Beulich wrote:
>>> 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@citrix.com>
>>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>> 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.
> 
> The size can't go to 0 or everything will break, and if it goes larger
> than 1 (which it almost certainly never will), then APs and/or S3 will
> break, and we've got both of these in CI.
> 
> Furthermore, the actual thing which matters is:
> 
>> /* Map the permanent trampoline page into l1_bootmap[]. */
>> mov     sym_esi(trampoline_phys), %ecx
>> lea     __PAGE_HYPERVISOR_RX(%ecx), %edx /* %edx = PTE to write  */
>> shr     $PAGE_SHIFT, %ecx                /* %ecx = Slot to write */
>> mov     %edx, sym_offs(l1_bootmap)(%esi, %ecx, 8)
> 
> which hardcodes 1 page, because there's almost certainly no chance this
> will ever change.
> 
>>>  We just don't
>>> know what we may need to add to the trampoline, sooner or later.
>> Maybe we could introduce trampoline_{ap?,runtime?}_{start,end} that
>> covers this more narrow section of the trampoline code?
> 
> We already have one of those, and a linker assertion that it stays below
> 1k, so wakeup_stack is at least 3k.
> 
> The complexity is that the wakeup_stack overlays some init-only logic in
> the placed trampoline.  It's not something that exists concretely in the
> Xen image.

Well - you've got an ack; while I'd prefer if connections were properly
made, I agree it's unlikely the size will grow enough for it to matter. So
I think you should feel free to put this in as is.

Jan

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

Posted by Andrew Cooper 7 months, 2 weeks ago

On 17/03/2025 7:56 am, Jan Beulich wrote:
> On 14.03.2025 20:00, Andrew Cooper wrote:
>> On 12/03/2025 9:57 am, Roger Pau Monné wrote:
>>> On Wed, Mar 12, 2025 at 09:31:37AM +0100, Jan Beulich wrote:
>>>> 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@citrix.com>
>>>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>> 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.
>> The size can't go to 0 or everything will break, and if it goes larger
>> than 1 (which it almost certainly never will), then APs and/or S3 will
>> break, and we've got both of these in CI.
>>
>> Furthermore, the actual thing which matters is:
>>
>>> /* Map the permanent trampoline page into l1_bootmap[]. */
>>> mov     sym_esi(trampoline_phys), %ecx
>>> lea     __PAGE_HYPERVISOR_RX(%ecx), %edx /* %edx = PTE to write  */
>>> shr     $PAGE_SHIFT, %ecx                /* %ecx = Slot to write */
>>> mov     %edx, sym_offs(l1_bootmap)(%esi, %ecx, 8)
>> which hardcodes 1 page, because there's almost certainly no chance this
>> will ever change.
>>
>>>>  We just don't
>>>> know what we may need to add to the trampoline, sooner or later.
>>> Maybe we could introduce trampoline_{ap?,runtime?}_{start,end} that
>>> covers this more narrow section of the trampoline code?
>> We already have one of those, and a linker assertion that it stays below
>> 1k, so wakeup_stack is at least 3k.
>>
>> The complexity is that the wakeup_stack overlays some init-only logic in
>> the placed trampoline.  It's not something that exists concretely in the
>> Xen image.
> Well - you've got an ack; while I'd prefer if connections were properly
> made, I agree it's unlikely the size will grow enough for it to matter. So
> I think you should feel free to put this in as is.

Thankyou.

~Andrew