xen/arch/x86/boot/trampoline.S | 6 ++++++ xen/arch/x86/boot/wakeup.S | 10 +++++++++- xen/arch/x86/smpboot.c | 5 ++++- 3 files changed, 19 insertions(+), 2 deletions(-)
... because its already hard enough to follow. Cross reference the locations
in C which set the entrypoints up, and state the alignment requirements and
entry conditions.
Drop a redundant .align 16, and panic() in do_boot_cpu() if the AP trampoline
isn't set up properly rather than blindly continuing an letting the APs
execute junk, or shifting part of the address into unrelated fields in ICR.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: David Woodhouse <dwmw2@infradead.org>
---
xen/arch/x86/boot/trampoline.S | 6 ++++++
xen/arch/x86/boot/wakeup.S | 10 +++++++++-
xen/arch/x86/smpboot.c | 5 ++++-
3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 125bdb5..cac0f3e1 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -38,6 +38,12 @@
.code16
+/*
+ * do_boot_cpu() programs the Startup-IPI to point here. Due to the SIPI
+ * format, the relocated entrypoint must be 4k aligned.
+ *
+ * It is entered in Real Mode, with %cs = wakeup_start >> 4 and %ip = 0.
+ */
GLOBAL(trampoline_realmode_entry)
mov %cs,%ax
mov %ax,%ds
diff --git a/xen/arch/x86/boot/wakeup.S b/xen/arch/x86/boot/wakeup.S
index 89df261..e3cb9e0 100644
--- a/xen/arch/x86/boot/wakeup.S
+++ b/xen/arch/x86/boot/wakeup.S
@@ -2,7 +2,15 @@
#define wakesym(sym) (sym - wakeup_start)
- .align 16
+/*
+ * acpi_sleep_prepare() programs the S3 wakeup vector to point here.
+ *
+ * The ACPI spec says that we shall be entered in Real Mode with:
+ * %cs = wakeup_start >> 4
+ * %ip = wakeup_start & 0xf
+ *
+ * As wakeup_start is 16-byte aligned, %ip is 0 in practice.
+ */
ENTRY(wakeup_start)
cli
cld
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index b7a0a4a..4f65c8d 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -548,9 +548,12 @@ static int do_boot_cpu(int apicid, int cpu)
booting_cpu = cpu;
- /* start_eip had better be page-aligned! */
start_eip = setup_trampoline();
+ /* start_eip needs be page aligned, and below the 1M boundary. */
+ if ( start_eip & ~0xff000 )
+ panic("AP trampoline %#lx not suitably positioned\n", start_eip);
+
/* So we see what's up */
if ( opt_cpu_info )
printk("Booting processor %d/%d eip %lx\n",
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> On 02.05.19 at 15:27, <andrew.cooper3@citrix.com> wrote: > --- a/xen/arch/x86/boot/trampoline.S > +++ b/xen/arch/x86/boot/trampoline.S > @@ -38,6 +38,12 @@ > > .code16 > > +/* > + * do_boot_cpu() programs the Startup-IPI to point here. Due to the SIPI > + * format, the relocated entrypoint must be 4k aligned. > + * > + * It is entered in Real Mode, with %cs = wakeup_start >> 4 and %ip = 0. > + */ > GLOBAL(trampoline_realmode_entry) The reference to wakeup_start looks to be a copy-and-paste (or alike) mistake here. > --- a/xen/arch/x86/smpboot.c > +++ b/xen/arch/x86/smpboot.c > @@ -548,9 +548,12 @@ static int do_boot_cpu(int apicid, int cpu) > > booting_cpu = cpu; > > - /* start_eip had better be page-aligned! */ > start_eip = setup_trampoline(); > > + /* start_eip needs be page aligned, and below the 1M boundary. */ > + if ( start_eip & ~0xff000 ) > + panic("AP trampoline %#lx not suitably positioned\n", start_eip); Seeing what setup_trampoline() really does, I'm not convinced a panic() is of much value. The page-alignment should be possible to check at build time, and the below-1M requirement should be guaranteed by us allocating low memory space in the first place. Nevertheless I won't insist, so with the earlier comment corrected Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 02/05/2019 14:50, Jan Beulich wrote: >>>> On 02.05.19 at 15:27, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/arch/x86/boot/trampoline.S >> +++ b/xen/arch/x86/boot/trampoline.S >> @@ -38,6 +38,12 @@ >> >> .code16 >> >> +/* >> + * do_boot_cpu() programs the Startup-IPI to point here. Due to the SIPI >> + * format, the relocated entrypoint must be 4k aligned. >> + * >> + * It is entered in Real Mode, with %cs = wakeup_start >> 4 and %ip = 0. >> + */ >> GLOBAL(trampoline_realmode_entry) > The reference to wakeup_start looks to be a copy-and-paste > (or alike) mistake here. Oops, indeed. Fixed. > >> --- a/xen/arch/x86/smpboot.c >> +++ b/xen/arch/x86/smpboot.c >> @@ -548,9 +548,12 @@ static int do_boot_cpu(int apicid, int cpu) >> >> booting_cpu = cpu; >> >> - /* start_eip had better be page-aligned! */ >> start_eip = setup_trampoline(); >> >> + /* start_eip needs be page aligned, and below the 1M boundary. */ >> + if ( start_eip & ~0xff000 ) >> + panic("AP trampoline %#lx not suitably positioned\n", start_eip); > Seeing what setup_trampoline() really does, I'm not > convinced a panic() is of much value. The page-alignment > should be possible to check at build time, and the below-1M > requirement should be guaranteed by us allocating low > memory space in the first place. Sadly it cant. We have a number of alignment issues (well - confusions at least), most obviously that trampoline_{start,end} in the linked Xen image has no particular alignment, but the relocated trampoline_start has a 4k requirement as a consequence of its alias with trampoline_realmode_entry. All it takes is one error in the 32bit asm which relocates the trampoline and this ends up exploding in a way which can only be detected at runtime. > Nevertheless I won't insist, > so with the earlier comment corrected > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks, ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Thu, 2019-05-02 at 15:08 +0100, Andrew Cooper wrote: > Sadly it cant. > > We have a number of alignment issues (well - confusions at least), most > obviously that trampoline_{start,end} in the linked Xen image has no > particular alignment, but the relocated trampoline_start has a 4k > requirement as a consequence of its alias with trampoline_realmode_entry. > > All it takes is one error in the 32bit asm which relocates the > trampoline and this ends up exploding in a way which can only be > detected at runtime. I'm relocating the permanent trampoline from 64-bit C code now, not in assembly. In the no-real-mode case (ignoring reloc(), qv) nothing is put into low memory until __start_xen() calls relocate_trampoline(). _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.