asm/config.h is included in every translation unit (via xen/config.h), while
only a handful of functions actually interact with the trampoline.

Move the infrastructure into its own header, and take the opportunity to
document everything.

Also change trampoline_realmode_entry() and wakeup_start() to be nocall
functions, rather than char arrays.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Frediano Ziglio <frediano.ziglio@cloud.com>
CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/arch/x86/acpi/power.c             |  2 +
 xen/arch/x86/cpu/intel.c              |  2 +
 xen/arch/x86/efi/efi-boot.h           |  1 +
 xen/arch/x86/guest/xen/pvh-boot.c     |  1 +
 xen/arch/x86/include/asm/config.h     | 19 ------
 xen/arch/x86/include/asm/trampoline.h | 98 +++++++++++++++++++++++++++
 xen/arch/x86/mm.c                     |  1 +
 xen/arch/x86/platform_hypercall.c     |  2 +
 xen/arch/x86/setup.c                  |  1 +
 xen/arch/x86/smpboot.c                |  1 +
 xen/arch/x86/tboot.c                  |  2 +
 xen/arch/x86/x86_64/mm.c              |  2 +
 12 files changed, 113 insertions(+), 19 deletions(-)
 create mode 100644 xen/arch/x86/include/asm/trampoline.h

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 557faf312b09..08a7fc250800 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -31,6 +31,8 @@
 #include <asm/microcode.h>
 #include <asm/prot-key.h>
 #include <asm/spec_ctrl.h>
+#include <asm/trampoline.h>
+
 #include <acpi/cpufreq/cpufreq.h>
 
 uint32_t system_reset_counter = 1;
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index af56e57bd8ab..807b708217e9 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -12,6 +12,8 @@
 #include <asm/mpspec.h>
 #include <asm/apic.h>
 #include <asm/i387.h>
+#include <asm/trampoline.h>
+
 #include <mach_apic.h>
 
 #include "cpu.h"
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index f282358435f1..23e510c77e2e 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -9,6 +9,7 @@
 #include <asm/microcode.h>
 #include <asm/msr.h>
 #include <asm/setup.h>
+#include <asm/trampoline.h>
 
 static struct file __initdata ucode;
 static multiboot_info_t __initdata mbi = {
diff --git a/xen/arch/x86/guest/xen/pvh-boot.c b/xen/arch/x86/guest/xen/pvh-boot.c
index cc57ab2cbcde..e14d7e20e942 100644
--- a/xen/arch/x86/guest/xen/pvh-boot.c
+++ b/xen/arch/x86/guest/xen/pvh-boot.c
@@ -12,6 +12,7 @@
 
 #include <asm/e820.h>
 #include <asm/guest.h>
+#include <asm/trampoline.h>
 
 #include <public/arch-x86/hvm/start_info.h>
 
diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h
index 2a260a2581fd..1f828bfd52f4 100644
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -83,25 +83,6 @@
 #define LIST_POISON1  ((void *)0x0100100100100100UL)
 #define LIST_POISON2  ((void *)0x0200200200200200UL)
 
-#ifndef __ASSEMBLY__
-extern unsigned long trampoline_phys;
-#define bootsym_phys(sym)                                 \
-    (((unsigned long)&(sym)-(unsigned long)&trampoline_start)+trampoline_phys)
-#define bootsym(sym)                                      \
-    (*((typeof(sym) *)__va(bootsym_phys(sym))))
-
-extern char trampoline_start[], trampoline_end[];
-extern char trampoline_realmode_entry[];
-extern unsigned int trampoline_xen_phys_start;
-extern unsigned char trampoline_cpu_started;
-extern char wakeup_start[];
-
-extern unsigned char video_flags;
-
-extern unsigned short boot_edid_caps;
-extern unsigned char boot_edid_info[128];
-#endif
-
 #include <xen/const.h>
 
 #define PML4_ENTRY_BITS  39
diff --git a/xen/arch/x86/include/asm/trampoline.h b/xen/arch/x86/include/asm/trampoline.h
new file mode 100644
index 000000000000..cc3420ba3530
--- /dev/null
+++ b/xen/arch/x86/include/asm/trampoline.h
@@ -0,0 +1,98 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef XEN_ASM_TRAMPOLINE_H
+#define XEN_ASM_TRAMPOLINE_H
+
+/*
+ * Data in or about the low memory trampoline.
+ *
+ * x86 systems software typically needs a block of logic below the 1M
+ * boundary, commonly called the trampoline, containing 16-bit logic.  Xen has
+ * a combined trampoline of all necessary 16-bit logic, formed of two parts.
+ *
+ * 1) The permanent trampoline; a single 4k page containing:
+ *
+ *    - The INIT-SIPI-SIPI entrypoint for APs, and
+ *    - The S3 wakeup vector.
+ *
+ *    Both of these are 16-bit entrypoints, responsible for activating paging
+ *    and getting into 64-bit mode.  This requires the permanent trampoline to
+ *    be identity mapped in idle_pg_table[].
+ *
+ *    The SIPI64 spec deprecates the 16-bit AP entrypoint, while S0ix (also
+ *    called Low Power Idle or Connected Standby) deprecates S3.
+ *
+ * 2) The boot trampoline:
+ *
+ *    This is used by the BSP to drop into 16-bit mode, make various BIOS
+ *    calls to obtain E820/EDID/etc.  It follows the permanent and exceeds 4k,
+ *    but is only used in 16-bit and 32-bit unpaged mode so does not need
+ *    mapping in pagetables.
+ *
+ *    When the BIOS calls are complete, execution does join back with the AP
+ *    path, and becomes subject to the same paging requirements.  This path is
+ *    not needed for non-BIOS boots.
+ *
+ * The location of trampoline is not fixed.  The layout of low memory varies
+ * greatly from platform to platform.  Therefore, the trampoline is relocated
+ * manually as part of placement.
+ */
+
+#ifndef __ASSEMBLY__
+
+#include <xen/compiler.h>
+#include <xen/types.h>
+
+/*
+ * Start and end of the trampoline section, as linked into Xen.  It is within
+ * the .init section and reclaimed after boot.
+ */
+/* SAF-0-safe */
+extern char trampoline_start[], trampoline_end[];
+
+/*
+ * The physical address of trampoline_start[] in low memory.  It must be below
+ * the 1M boundary (as the trampoline contains 16-bit code), and must be 4k
+ * aligned (SIPI requirement for APs).
+ */
+extern unsigned long trampoline_phys;
+
+/*
+ * Calculate the physical address of a symbol in the trampoline.
+ *
+ * Should only be used on symbols declared later in this header.  Specifying
+ * other symbols will compile but malfunction when used, as will using this
+ * helper before the trampoline is placed.
+ */
+#define bootsym_phys(sym)                                       \
+    (trampoline_phys + ((unsigned long)&(sym) -                 \
+                        (unsigned long)trampoline_start))
+
+/* Given a trampoline symbol, construct a pointer to it in the directmap. */
+#define bootsym(sym) (*((typeof(sym) *)__va(bootsym_phys(sym))))
+
+/* The INIT-SIPI-SIPI entrypoint.  16-bit code. */
+void nocall trampoline_realmode_entry(void);
+
+/* The S3 wakeup vector.  16-bit code. */
+void nocall wakeup_start(void);
+
+/*
+ * A variable in the trampoline, containing Xen's physical address.  Amongst
+ * other things, it is used to find idle_pg_table[] in order to enable paging
+ * and activate 64-bit mode.  This variable needs keeping in sync with
+ * xen_phys_start.
+ */
+extern uint32_t trampoline_xen_phys_start;
+
+/* A semaphore to indicate signs-of-life at the start of the AP boot path. */
+extern uint8_t trampoline_cpu_started;
+
+/* Quirks about video mode-setting on S3 resume. */
+extern uint8_t video_flags;
+
+/* Extended Display Identification Data, gathered from the BIOS. */
+extern uint16_t boot_edid_caps;
+extern uint8_t boot_edid_info[128];
+
+#endif /* !__ASSEMBLY__ */
+#endif /* XEN_ASM_TRAMPOLINE_H */
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 608583a1134e..c735aaf0e823 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -131,6 +131,7 @@
 #include <asm/guest.h>
 #include <asm/pv/domain.h>
 #include <asm/pv/mm.h>
+#include <asm/trampoline.h>
 
 #ifdef CONFIG_PV
 #include "pv/mm.h"
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 7e3278109300..67f851237def 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -32,6 +32,8 @@
 #include <asm/mtrr.h>
 #include <asm/io_apic.h>
 #include <asm/setup.h>
+#include <asm/trampoline.h>
+
 #include "cpu/mcheck/mce.h"
 #include "cpu/mtrr/mtrr.h"
 #include <xsm/xsm.h>
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index cd69198326da..a6e77c9ed9fc 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -59,6 +59,7 @@
 #include <asm/microcode.h>
 #include <asm/prot-key.h>
 #include <asm/pv/domain.h>
+#include <asm/trampoline.h>
 
 /* opt_nosmp: If true, secondary processors are ignored. */
 static bool __initdata opt_nosmp;
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 0a89f22a3980..9e79c1a6d6e6 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -35,6 +35,7 @@
 #include <asm/spec_ctrl.h>
 #include <asm/time.h>
 #include <asm/tboot.h>
+#include <asm/trampoline.h>
 #include <irq_vectors.h>
 #include <mach_apic.h>
 
diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
index ba0700d2d5da..d5db60d335e3 100644
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -14,6 +14,8 @@
 #include <asm/e820.h>
 #include <asm/tboot.h>
 #include <asm/setup.h>
+#include <asm/trampoline.h>
+
 #include <crypto/vmac.h>
 
 /* tboot=<physical address of shared page> */
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index b2a280fba369..0d8e60252962 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -37,6 +37,8 @@ EMIT_FILE;
 #include <asm/numa.h>
 #include <asm/mem_paging.h>
 #include <asm/mem_sharing.h>
+#include <asm/trampoline.h>
+
 #include <public/memory.h>
 
 #ifdef CONFIG_PV32
-- 
2.39.2

On 05.09.2024 15:06, Andrew Cooper wrote:
> asm/config.h is included in every translation unit (via xen/config.h), while
> only a handful of functions actually interact with the trampoline.
> 
> Move the infrastructure into its own header, and take the opportunity to
> document everything.
> 
> Also change trampoline_realmode_entry() and wakeup_start() to be nocall
> functions, rather than char arrays.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> --- /dev/null
> +++ b/xen/arch/x86/include/asm/trampoline.h
> @@ -0,0 +1,98 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef XEN_ASM_TRAMPOLINE_H
> +#define XEN_ASM_TRAMPOLINE_H

Not exactly usual a guard name, but once the new naming scheme is finalized
most will need renaming anyway.

> +/*
> + * Calculate the physical address of a symbol in the trampoline.
> + *
> + * Should only be used on symbols declared later in this header.  Specifying
> + * other symbols will compile but malfunction when used, as will using this
> + * helper before the trampoline is placed.
> + */

As to the last point made - we could of course overcome this by setting
trampoline_phys to a suitable value initially, and only to its low-mem
value once the trampoline was moved there.

As to compiling but malfunctioning - I'd kind of expect relocation
overflows to be flagged by the linker.

Jan

On 05/09/2024 4:35 pm, Jan Beulich wrote:
> On 05.09.2024 15:06, Andrew Cooper wrote:
>> asm/config.h is included in every translation unit (via xen/config.h), while
>> only a handful of functions actually interact with the trampoline.
>>
>> Move the infrastructure into its own header, and take the opportunity to
>> document everything.
>>
>> Also change trampoline_realmode_entry() and wakeup_start() to be nocall
>> functions, rather than char arrays.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/trampoline.h
>> @@ -0,0 +1,98 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +#ifndef XEN_ASM_TRAMPOLINE_H
>> +#define XEN_ASM_TRAMPOLINE_H
> Not exactly usual a guard name, but once the new naming scheme is finalized
> most will need renaming anyway.

What would you prefer?

>
>> +/*
>> + * Calculate the physical address of a symbol in the trampoline.
>> + *
>> + * Should only be used on symbols declared later in this header.  Specifying
>> + * other symbols will compile but malfunction when used, as will using this
>> + * helper before the trampoline is placed.
>> + */
> As to the last point made - we could of course overcome this by setting
> trampoline_phys to a suitable value initially, and only to its low-mem
> value once the trampoline was moved there.

Yes, but then we've got yet another variable needing to stay in sync
with xen_phys_start (for a while at least).

> As to compiling but malfunctioning - I'd kind of expect relocation
> overflows to be flagged by the linker.

What I meant by this is that things like bootsym(opt_cpu_info) will
compile but may #PF when used or corrupt data if not.

I couldn't think of any good way to bounds check sym between
trampoline_{start,end}[] at build time.

Doing it at runtime is possible, but some usage sites aren't
printk/panic friendly.

~Andrew

On 05.09.2024 17:45, Andrew Cooper wrote:
> On 05/09/2024 4:35 pm, Jan Beulich wrote:
>> On 05.09.2024 15:06, Andrew Cooper wrote:
>>> asm/config.h is included in every translation unit (via xen/config.h), while
>>> only a handful of functions actually interact with the trampoline.
>>>
>>> Move the infrastructure into its own header, and take the opportunity to
>>> document everything.
>>>
>>> Also change trampoline_realmode_entry() and wakeup_start() to be nocall
>>> functions, rather than char arrays.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Thanks.
> 
>>
>>> --- /dev/null
>>> +++ b/xen/arch/x86/include/asm/trampoline.h
>>> @@ -0,0 +1,98 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +#ifndef XEN_ASM_TRAMPOLINE_H
>>> +#define XEN_ASM_TRAMPOLINE_H
>> Not exactly usual a guard name, but once the new naming scheme is finalized
>> most will need renaming anyway.
> 
> What would you prefer?

X86_ASM_TRAMPOLINE_H would likely be closer to what we use elsewhere.

Jan

On 05/09/2024 4:47 pm, Jan Beulich wrote:
> On 05.09.2024 17:45, Andrew Cooper wrote:
>> On 05/09/2024 4:35 pm, Jan Beulich wrote:
>>> On 05.09.2024 15:06, Andrew Cooper wrote:
>>>> asm/config.h is included in every translation unit (via xen/config.h), while
>>>> only a handful of functions actually interact with the trampoline.
>>>>
>>>> Move the infrastructure into its own header, and take the opportunity to
>>>> document everything.
>>>>
>>>> Also change trampoline_realmode_entry() and wakeup_start() to be nocall
>>>> functions, rather than char arrays.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> Thanks.
>>
>>>> --- /dev/null
>>>> +++ b/xen/arch/x86/include/asm/trampoline.h
>>>> @@ -0,0 +1,98 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +#ifndef XEN_ASM_TRAMPOLINE_H
>>>> +#define XEN_ASM_TRAMPOLINE_H
>>> Not exactly usual a guard name, but once the new naming scheme is finalized
>>> most will need renaming anyway.
>> What would you prefer?
> X86_ASM_TRAMPOLINE_H would likely be closer to what we use elsewhere.

Fine.  I'll adjust.

~Andrew