Message ID | 1556803627-22855-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86/boot: Annotate the Real Mode entry points | expand |
>>> 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
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
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().
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",
... 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(-)