[PATCH 0/3] x86/trampoline: Header cleanup

[PATCH 0/3] x86/trampoline: Header cleanup

[Andrew Cooper]

This started while inspecting a preprossed file for bitops, but it turns out
is relevant for Frediano's 32bit boot code changes too.

Its header file juggling, and documentation with observations relevant to both
the ASI and Host UEFI Secureboot work, hence the extended CC list.

Andrew Cooper (3):
  x86/acpi: Drop acpi_video_flags and use bootsym(video_flags) directly
  x86/trampoline: Move the trampoline declarations out of <asm/config.h>
  x86/trampoline: Collect other scattered trampoline symbols

 xen/arch/x86/acpi/power.c             |   4 +-
 xen/arch/x86/cpu/intel.c              |   2 +
 xen/arch/x86/dmi_scan.c               |  12 ---
 xen/arch/x86/efi/efi-boot.h           |   4 +-
 xen/arch/x86/guest/xen/pvh-boot.c     |   1 +
 xen/arch/x86/include/asm/acpi.h       |   1 -
 xen/arch/x86/include/asm/config.h     |  19 ----
 xen/arch/x86/include/asm/processor.h  |   2 -
 xen/arch/x86/include/asm/setup.h      |   2 -
 xen/arch/x86/include/asm/trampoline.h | 120 ++++++++++++++++++++++++++
 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 +
 16 files changed, 136 insertions(+), 40 deletions(-)
 create mode 100644 xen/arch/x86/include/asm/trampoline.h


base-commit: a2de7dc4d845738e734b10fce6550c89c6b1092c
-- 
2.39.2

[PATCH 1/3] x86/acpi: Drop acpi_video_flags and use bootsym(video_flags) directly

[Andrew Cooper]

This removes a level of indirection, as well as removing a somewhat misleading
name; the variable is really "S3 video quirks".

More importantly however it makes it very clear that, right now, parsing the
cmdline and quirks depends on having already placed the trampoline; a
dependency which is going to be gnarly to untangle.

That said, fixing the quirk is easy.  The Toshiba Satellite 4030CDT has an
Intel Celeron 300Mhz CPU (Pentium 2 era) from 1998 when MMX was the headline
feature, sporting 64M of RAM.  Being a 32-bit processor, it hasn't been able
to run Xen for about a decade now, so drop the quirk entirely.

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/dmi_scan.c         | 12 ------------
 xen/arch/x86/include/asm/acpi.h |  1 -
 3 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 610937f42e95..557faf312b09 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -56,7 +56,7 @@ static int __init cf_check parse_acpi_sleep(const char *s)
         s = ss + 1;
     } while ( *ss );
 
-    acpi_video_flags |= flag;
+    bootsym(video_flags) |= flag;
 
     return rc;
 }
diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
index 81f80c053a7a..9257aee2ab97 100644
--- a/xen/arch/x86/dmi_scan.c
+++ b/xen/arch/x86/dmi_scan.c
@@ -499,13 +499,6 @@ static int __init cf_check ich10_bios_quirk(const struct dmi_system_id *d)
     return 0;
 }
 
-static __init int cf_check reset_videomode_after_s3(const struct dmi_blacklist *d)
-{
-	/* See wakeup.S */
-	acpi_video_flags |= 2;
-	return 0;
-}
-
 static __init int cf_check dmi_disable_acpi(const struct dmi_blacklist *d)
 { 
 	if (!acpi_force) { 
@@ -546,11 +539,6 @@ static __init int cf_check force_acpi_ht(const struct dmi_blacklist *d)
  
 static const struct dmi_blacklist __initconstrel dmi_blacklist[] = {
 
-	{ reset_videomode_after_s3, "Toshiba Satellite 4030cdt", { /* Reset video mode after returning from ACPI S3 sleep */
-			MATCH(DMI_PRODUCT_NAME, "S4030CDT/4.3"),
-			NO_MATCH, NO_MATCH, NO_MATCH
-			} },
-
 	{ ich10_bios_quirk, "Intel board & BIOS",
 		/*
 		 * BIOS leaves legacy USB emulation enabled while
diff --git a/xen/arch/x86/include/asm/acpi.h b/xen/arch/x86/include/asm/acpi.h
index 3c47b216d0e0..217819dd619c 100644
--- a/xen/arch/x86/include/asm/acpi.h
+++ b/xen/arch/x86/include/asm/acpi.h
@@ -103,7 +103,6 @@ extern unsigned long acpi_wakeup_address;
 extern int8_t acpi_numa;
 
 extern struct acpi_sleep_info acpi_sinfo;
-#define acpi_video_flags bootsym(video_flags)
 struct xenpf_enter_acpi_sleep;
 extern int acpi_enter_sleep(const struct xenpf_enter_acpi_sleep *sleep);
 extern int acpi_enter_state(u32 state);
-- 
2.39.2

[PATCH 2/3] x86/trampoline: Move the trampoline declarations out of <asm/config.h>

[Andrew Cooper]

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

[PATCH 3/3] x86/trampoline: Collect other scattered trampoline symbols

[Andrew Cooper]

... and document them too.

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>

video.h, edd.h and e820.h also contain trampoline symbols, but they're pretty
well contained headers already.

kbd_shift_flags seems especially dubious.  It's a snapshot of the keyboard
state when Xen happened to pass through the trampoline, and surely cannot be
useful for dom0 in the slightest...
---
 xen/arch/x86/efi/efi-boot.h           |  3 ---
 xen/arch/x86/include/asm/processor.h  |  2 --
 xen/arch/x86/include/asm/setup.h      |  2 --
 xen/arch/x86/include/asm/trampoline.h | 22 ++++++++++++++++++++++
 4 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 23e510c77e2e..833e343a475e 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -102,9 +102,6 @@ static void __init efi_arch_relocate_image(unsigned long delta)
     }
 }
 
-extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
-extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];
-
 static void __init relocate_trampoline(unsigned long phys)
 {
     const s32 *trampoline_ptr;
diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
index e71dbb8d3fbf..b8d8127e2dc3 100644
--- a/xen/arch/x86/include/asm/processor.h
+++ b/xen/arch/x86/include/asm/processor.h
@@ -97,8 +97,6 @@ extern void ctxt_switch_levelling(const struct vcpu *next);
 extern void (*ctxt_switch_masking)(const struct vcpu *next);
 
 extern bool opt_cpu_info;
-extern u32 trampoline_efer;
-extern u64 trampoline_misc_enable_off;
 
 /* Maximum width of physical addresses supported by the hardware. */
 extern unsigned int paddr_bits;
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index d75589178b91..4d88503fd2e6 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -40,8 +40,6 @@ int remove_xen_ranges(struct rangeset *r);
 
 int cf_check stub_selftest(void);
 
-extern uint8_t kbd_shift_flags;
-
 #ifdef NDEBUG
 # define highmem_start 0
 #else
diff --git a/xen/arch/x86/include/asm/trampoline.h b/xen/arch/x86/include/asm/trampoline.h
index cc3420ba3530..dc2c47946be4 100644
--- a/xen/arch/x86/include/asm/trampoline.h
+++ b/xen/arch/x86/include/asm/trampoline.h
@@ -49,6 +49,13 @@
 /* SAF-0-safe */
 extern char trampoline_start[], trampoline_end[];
 
+/*
+ * Relocations for the trampoline.  Generated by the bootsym_{seg,}rel()
+ * macros, and collected by the linker.
+ */
+extern const int32_t __trampoline_rel_start[], __trampoline_rel_stop[];
+extern const int32_t __trampoline_seg_start[], __trampoline_seg_stop[];
+
 /*
  * 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
@@ -87,9 +94,24 @@ 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;
 
+/*
+ * Extra MSR_EFER settings when activating Long Mode.  EFER_NXE is necessary
+ * for APs to boot if the BSP found and activated support.
+ */
+extern uint32_t trampoline_efer;
+
+/*
+ * When nonzero, clear the specified bits in MSR_MISC_ENABLE.  This is
+ * necessary to clobber XD_DISABLE before trying to set MSR_EFER.NXE.
+ */
+extern uint64_t trampoline_misc_enable_off;
+
 /* Quirks about video mode-setting on S3 resume. */
 extern uint8_t video_flags;
 
+/* BIOS Int 16h, Fn 02h.  The keyboard shift status. */
+extern uint8_t kbd_shift_flags;
+
 /* Extended Display Identification Data, gathered from the BIOS. */
 extern uint16_t boot_edid_caps;
 extern uint8_t boot_edid_info[128];
-- 
2.39.2

Re: [PATCH 0/3] x86/trampoline: Header cleanup

[Frediano Ziglio]

[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]

On Thu, Sep 5, 2024 at 2:07 PM Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

> This started while inspecting a preprossed file for bitops, but it turns
> out
> is relevant for Frediano's 32bit boot code changes too.
>
> Its header file juggling, and documentation with observations relevant to
> both
> the ASI and Host UEFI Secureboot work, hence the extended CC list.
>
> Andrew Cooper (3):
>   x86/acpi: Drop acpi_video_flags and use bootsym(video_flags) directly
>   x86/trampoline: Move the trampoline declarations out of <asm/config.h>
>   x86/trampoline: Collect other scattered trampoline symbols
>
>  xen/arch/x86/acpi/power.c             |   4 +-
>  xen/arch/x86/cpu/intel.c              |   2 +
>  xen/arch/x86/dmi_scan.c               |  12 ---
>  xen/arch/x86/efi/efi-boot.h           |   4 +-
>  xen/arch/x86/guest/xen/pvh-boot.c     |   1 +
>  xen/arch/x86/include/asm/acpi.h       |   1 -
>  xen/arch/x86/include/asm/config.h     |  19 ----
>  xen/arch/x86/include/asm/processor.h  |   2 -
>  xen/arch/x86/include/asm/setup.h      |   2 -
>  xen/arch/x86/include/asm/trampoline.h | 120 ++++++++++++++++++++++++++
>  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 +
>  16 files changed, 136 insertions(+), 40 deletions(-)
>  create mode 100644 xen/arch/x86/include/asm/trampoline.h
>
>
> base-commit: a2de7dc4d845738e734b10fce6550c89c6b1092c
>
>
Hi,
   the series looks good to me.

Frediano

[-- Attachment #2: Type: text/html, Size: 2259 bytes --]

Re: [PATCH 1/3] x86/acpi: Drop acpi_video_flags and use bootsym(video_flags) directly

[Jan Beulich]

On 05.09.2024 15:06, Andrew Cooper wrote:
> This removes a level of indirection, as well as removing a somewhat misleading
> name; the variable is really "S3 video quirks".
> 
> More importantly however it makes it very clear that, right now, parsing the
> cmdline and quirks depends on having already placed the trampoline; a
> dependency which is going to be gnarly to untangle.
> 
> That said, fixing the quirk is easy.  The Toshiba Satellite 4030CDT has an
> Intel Celeron 300Mhz CPU (Pentium 2 era) from 1998 when MMX was the headline
> feature, sporting 64M of RAM.  Being a 32-bit processor, it hasn't been able
> to run Xen for about a decade now, so drop the quirk entirely.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 1/3] x86/acpi: Drop acpi_video_flags and use bootsym(video_flags) directly

[Alejandro Vallejo]

On Thu Sep 5, 2024 at 2:06 PM BST, Andrew Cooper wrote:
> This removes a level of indirection, as well as removing a somewhat misleading
> name; the variable is really "S3 video quirks".

nit: Would it be beneficial to rename video_flags to s3_video_flags?

>
> More importantly however it makes it very clear that, right now, parsing the
> cmdline and quirks depends on having already placed the trampoline; a
> dependency which is going to be gnarly to untangle.
>
> That said, fixing the quirk is easy.  The Toshiba Satellite 4030CDT has an
> Intel Celeron 300Mhz CPU (Pentium 2 era) from 1998 when MMX was the headline
> feature, sporting 64M of RAM.  Being a 32-bit processor, it hasn't been able
> to run Xen for about a decade now, so drop the quirk entirely.
>
> 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/dmi_scan.c         | 12 ------------
>  xen/arch/x86/include/asm/acpi.h |  1 -
>  3 files changed, 1 insertion(+), 14 deletions(-)

Always nice to see old hacks disappear.

  Reviewed-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Cheers,
Alejandro

Re: [PATCH 2/3] x86/trampoline: Move the trampoline declarations out of <asm/config.h>

[Jan Beulich]

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

Re: [PATCH 1/3] x86/acpi: Drop acpi_video_flags and use bootsym(video_flags) directly

[Andrew Cooper]

On 05/09/2024 4:05 pm, Alejandro Vallejo wrote:
> On Thu Sep 5, 2024 at 2:06 PM BST, Andrew Cooper wrote:
>> This removes a level of indirection, as well as removing a somewhat misleading
>> name; the variable is really "S3 video quirks".
> nit: Would it be beneficial to rename video_flags to s3_video_flags?

Probably.  Also to give it some named constants rather than magic numbers.

But I think I'll leave that as an exercise to someone with more time.

>
>> More importantly however it makes it very clear that, right now, parsing the
>> cmdline and quirks depends on having already placed the trampoline; a
>> dependency which is going to be gnarly to untangle.
>>
>> That said, fixing the quirk is easy.  The Toshiba Satellite 4030CDT has an
>> Intel Celeron 300Mhz CPU (Pentium 2 era) from 1998 when MMX was the headline
>> feature, sporting 64M of RAM.  Being a 32-bit processor, it hasn't been able
>> to run Xen for about a decade now, so drop the quirk entirely.
>>
>> 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/dmi_scan.c         | 12 ------------
>>  xen/arch/x86/include/asm/acpi.h |  1 -
>>  3 files changed, 1 insertion(+), 14 deletions(-)
> Always nice to see old hacks disappear.
>
>   Reviewed-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Thanks.

~Andrew

Re: [PATCH 3/3] x86/trampoline: Collect other scattered trampoline symbols

[Jan Beulich]

On 05.09.2024 15:06, Andrew Cooper wrote:
> kbd_shift_flags seems especially dubious.  It's a snapshot of the keyboard
> state when Xen happened to pass through the trampoline, and surely cannot be
> useful for dom0 in the slightest...

No more or less than if the kernel takes such a snapshot while booting natively.
Whatever this is used for there (it's been too long - I don't recall).

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -102,9 +102,6 @@ static void __init efi_arch_relocate_image(unsigned long delta)
>      }
>  }
>  
> -extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
> -extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];

I'd prefer if these stayed here, leaving the 4 symbols as minimally exposed as
possible. Recall that efi-boot.h isn't really a header in that sense, but
rather a .c file. Elsewhere we keep decls in .c files when they're used in just
one CU.

Jan

Re: [PATCH 2/3] x86/trampoline: Move the trampoline declarations out of <asm/config.h>

[Andrew Cooper]

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

Re: [PATCH 2/3] x86/trampoline: Move the trampoline declarations out of <asm/config.h>

[Jan Beulich]

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

Re: [PATCH 2/3] x86/trampoline: Move the trampoline declarations out of <asm/config.h>

[Andrew Cooper]

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

Re: [PATCH 3/3] x86/trampoline: Collect other scattered trampoline symbols

[Andrew Cooper]

On 05/09/2024 4:42 pm, Jan Beulich wrote:
> On 05.09.2024 15:06, Andrew Cooper wrote:
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -102,9 +102,6 @@ static void __init efi_arch_relocate_image(unsigned long delta)
>>      }
>>  }
>>  
>> -extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
>> -extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];
> I'd prefer if these stayed here, leaving the 4 symbols as minimally exposed as
> possible. Recall that efi-boot.h isn't really a header in that sense, but
> rather a .c file. Elsewhere we keep decls in .c files when they're used in just
> one CU.

See Frediano's RFC series, which needs to change this in order to move
the 32bit relocation logic from asm to C.

The only reason efi-boot.h can get away with this right now is because
the other logic is written entirely in asm.


Scope-limiting linker section boundaries more than regular variables is
weird to me.  It's not as if they magically take more care to use than
regular variables, and trampoline.h is not a wide scope by any means.

~Andrew

Re: [PATCH 3/3] x86/trampoline: Collect other scattered trampoline symbols

[Frediano Ziglio]

[-- Attachment #1: Type: text/plain, Size: 1912 bytes --]

On Thu, Sep 5, 2024 at 5:10 PM Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

> On 05/09/2024 4:42 pm, Jan Beulich wrote:
> > On 05.09.2024 15:06, Andrew Cooper wrote:
> >> --- a/xen/arch/x86/efi/efi-boot.h
> >> +++ b/xen/arch/x86/efi/efi-boot.h
> >> @@ -102,9 +102,6 @@ static void __init efi_arch_relocate_image(unsigned
> long delta)
> >>      }
> >>  }
> >>
> >> -extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
> >> -extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];
> > I'd prefer if these stayed here, leaving the 4 symbols as minimally
> exposed as
> > possible. Recall that efi-boot.h isn't really a header in that sense, but
> > rather a .c file. Elsewhere we keep decls in .c files when they're used
> in just
> > one CU.
>
> See Frediano's RFC series, which needs to change this in order to move
> the 32bit relocation logic from asm to C.
>
> Not strictly necessary, I can declare in the C file as they were declared
in efi-boot.h (which is more a .c file as an header as Jan said).
I think the idea of declaring into a source file is that if another file
wants to use it has to declare it again, so a bit more friction.
But any access to trampoline variables should be considered as something to
limit in any case, so having in a separate header helps (this does not mean
that removing from the header is still increasing the friction).

Personally, I'm not strong about the 2 options here. Slightly prefer having
all variable declared in a single header.

The only reason efi-boot.h can get away with this right now is because
> the other logic is written entirely in asm.
>
>
> Scope-limiting linker section boundaries more than regular variables is
> weird to me.  It's not as if they magically take more care to use than
> regular variables, and trampoline.h is not a wide scope by any means.
>
>
Frediano

[-- Attachment #2: Type: text/html, Size: 2564 bytes --]

Re: [PATCH 3/3] x86/trampoline: Collect other scattered trampoline symbols

[Andrew Cooper]

On 05/09/2024 5:34 pm, Frediano Ziglio wrote:
> On Thu, Sep 5, 2024 at 5:10 PM Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>
>     On 05/09/2024 4:42 pm, Jan Beulich wrote:
>     > On 05.09.2024 15:06, Andrew Cooper wrote:
>     >> --- a/xen/arch/x86/efi/efi-boot.h
>     >> +++ b/xen/arch/x86/efi/efi-boot.h
>     >> @@ -102,9 +102,6 @@ static void __init
>     efi_arch_relocate_image(unsigned long delta)
>     >>      }
>     >>  }
>     >> 
>     >> -extern const s32 __trampoline_rel_start[],
>     __trampoline_rel_stop[];
>     >> -extern const s32 __trampoline_seg_start[],
>     __trampoline_seg_stop[];
>     > I'd prefer if these stayed here, leaving the 4 symbols as
>     minimally exposed as
>     > possible. Recall that efi-boot.h isn't really a header in that
>     sense, but
>     > rather a .c file. Elsewhere we keep decls in .c files when
>     they're used in just
>     > one CU.
>
>     See Frediano's RFC series, which needs to change this in order to move
>     the 32bit relocation logic from asm to C.
>
> Not strictly necessary, I can declare in the C file as they were
> declared in efi-boot.h (which is more a .c file as an header as Jan said).
> I think the idea of declaring into a source file is that if another
> file wants to use it has to declare it again, so a bit more friction.
> But any access to trampoline variables should be considered as
> something to limit in any case, so having in a separate header helps
> (this does not mean that removing from the header is still increasing
> the friction).
>
> Personally, I'm not strong about the 2 options here. Slightly prefer
> having all variable declared in a single header.

Declaring the same symbol in multiple places is a hard MISRA failure now.

Hence why I was trying to clean this up to simplify your patches.

~Andrew

Re: [PATCH 3/3] x86/trampoline: Collect other scattered trampoline symbols

[Jan Beulich]

On 05.09.2024 18:10, Andrew Cooper wrote:
> On 05/09/2024 4:42 pm, Jan Beulich wrote:
>> On 05.09.2024 15:06, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/efi/efi-boot.h
>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>> @@ -102,9 +102,6 @@ static void __init efi_arch_relocate_image(unsigned long delta)
>>>      }
>>>  }
>>>  
>>> -extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
>>> -extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];
>> I'd prefer if these stayed here, leaving the 4 symbols as minimally exposed as
>> possible. Recall that efi-boot.h isn't really a header in that sense, but
>> rather a .c file. Elsewhere we keep decls in .c files when they're used in just
>> one CU.
> 
> See Frediano's RFC series, which needs to change this in order to move
> the 32bit relocation logic from asm to C.

And it moves the declarations to the new .c file. Visibility still limited
to that one file. And (afaics) no Misra violation, contrary to what your
later reply to Frediano suggests.

> The only reason efi-boot.h can get away with this right now is because
> the other logic is written entirely in asm.
> 
> 
> Scope-limiting linker section boundaries more than regular variables is
> weird to me.  It's not as if they magically take more care to use than
> regular variables, and trampoline.h is not a wide scope by any means.

It's not "more than", it's "like" (i.e. no matter whether a linker script
or assembly is the origin of the symbol).

Jan

Re: [PATCH 3/3] x86/trampoline: Collect other scattered trampoline symbols

[Andrew Cooper]

On 06/09/2024 6:58 am, Jan Beulich wrote:
> On 05.09.2024 18:10, Andrew Cooper wrote:
>> On 05/09/2024 4:42 pm, Jan Beulich wrote:
>>> On 05.09.2024 15:06, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/efi/efi-boot.h
>>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>>> @@ -102,9 +102,6 @@ static void __init efi_arch_relocate_image(unsigned long delta)
>>>>      }
>>>>  }
>>>>  
>>>> -extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
>>>> -extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];
>>> I'd prefer if these stayed here, leaving the 4 symbols as minimally exposed as
>>> possible. Recall that efi-boot.h isn't really a header in that sense, but
>>> rather a .c file. Elsewhere we keep decls in .c files when they're used in just
>>> one CU.
>> See Frediano's RFC series, which needs to change this in order to move
>> the 32bit relocation logic from asm to C.
> And it moves the declarations to the new .c file. Visibility still limited
> to that one file. And (afaics) no Misra violation, contrary to what your
> later reply to Frediano suggests.

If only there were an easy way to answer the question.

https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/7766305370

Failure: 4 regressions found for clean guidelines
  service MC3R1.R8.5: (required) An external object or function shall be
declared once in one and only one file:
   violation: 4
>> The only reason efi-boot.h can get away with this right now is because
>> the other logic is written entirely in asm.
>>
>>
>> Scope-limiting linker section boundaries more than regular variables is
>> weird to me.  It's not as if they magically take more care to use than
>> regular variables, and trampoline.h is not a wide scope by any means.
> It's not "more than", it's "like" (i.e. no matter whether a linker script
> or assembly is the origin of the symbol).

I'm asking why linker-section-boundary symbols should be treated
specially, and not seeing an answer.

I assert they do not warrant special treatment, and should live in
header files like every other extern symbol we use.

~Andrew

Re: [PATCH 3/3] x86/trampoline: Collect other scattered trampoline symbols

[Jan Beulich]

On 06.09.2024 21:46, Andrew Cooper wrote:
> On 06/09/2024 6:58 am, Jan Beulich wrote:
>> On 05.09.2024 18:10, Andrew Cooper wrote:
>>> On 05/09/2024 4:42 pm, Jan Beulich wrote:
>>>> On 05.09.2024 15:06, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/efi/efi-boot.h
>>>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>>>> @@ -102,9 +102,6 @@ static void __init efi_arch_relocate_image(unsigned long delta)
>>>>>      }
>>>>>  }
>>>>>  
>>>>> -extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
>>>>> -extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];
>>>> I'd prefer if these stayed here, leaving the 4 symbols as minimally exposed as
>>>> possible. Recall that efi-boot.h isn't really a header in that sense, but
>>>> rather a .c file. Elsewhere we keep decls in .c files when they're used in just
>>>> one CU.
>>> See Frediano's RFC series, which needs to change this in order to move
>>> the 32bit relocation logic from asm to C.
>> And it moves the declarations to the new .c file. Visibility still limited
>> to that one file. And (afaics) no Misra violation, contrary to what your
>> later reply to Frediano suggests.
> 
> If only there were an easy way to answer the question.
> 
> https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/7766305370
> 
> Failure: 4 regressions found for clean guidelines
>   service MC3R1.R8.5: (required) An external object or function shall be
> declared once in one and only one file:
>    violation: 4

I'm afraid I'm having trouble locating, in that .log file, where the actual
regressions are pointed out. I guess I'm simply not used to reading such
logs yet, and hence I just don't know what to search for. In any event, I
think there's a set of issues here:
- Eclair apparently considered efi-boot.h a header file, which (as said
  earlier) isn't quite right.
- Declarations there are thus deemed okay (when they shouldn't, unless
  deviated).
- Movement to a proper .c file points out that those decls may have been
  missing "asmlinkage" already before.

>>> The only reason efi-boot.h can get away with this right now is because
>>> the other logic is written entirely in asm.
>>>
>>>
>>> Scope-limiting linker section boundaries more than regular variables is
>>> weird to me.  It's not as if they magically take more care to use than
>>> regular variables, and trampoline.h is not a wide scope by any means.
>> It's not "more than", it's "like" (i.e. no matter whether a linker script
>> or assembly is the origin of the symbol).
> 
> I'm asking why linker-section-boundary symbols should be treated
> specially, and not seeing an answer.

IOW you're not taking "they're no different from symbols defined in .S
files, and hence shouldn't be treated differently" as a possible position
to take? See __page_tables_{start,end}, cpu0_stack[], or multiboot_ptr as
examples.

> I assert they do not warrant special treatment, and should live in
> header files like every other extern symbol we use.

Then the same would also apply to symbols coming from .S files. Which in
turn were deliberately deviated (rather than moved) in the course of
dealing with the Misra rule relevant here.

Jan