| Message ID | 20240905130657.4075906-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/trampoline: Header cleanup | expand |
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
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
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