| Message ID | 20250318201033.60634-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/boot: Untangle the trampoline copying/entry logic | expand |
On Tue, Mar 18, 2025 at 08:10:33PM +0000, Andrew Cooper wrote: > The LRET is detached from the PUSHes which set it up, and this is about to get > worse with the changes to trampoline relocation. For the sake of one variable > read, the complexity is not worth it. > > Reorder the logic to copy the trampoline into place, then switch stack and > enter the trampoline. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > > Roger: I'd like this to be a prerequisite to your "[PATCH 4/7] x86/boot: apply > trampoline relocations at destination position" to avoid the movement of > reloc_trampoline32() making things worse. I think you could commit this now-ish, and I can rebase on top? Thanks, Roger.
On 19/03/2025 9:05 am, Roger Pau Monné wrote: > On Tue, Mar 18, 2025 at 08:10:33PM +0000, Andrew Cooper wrote: >> The LRET is detached from the PUSHes which set it up, and this is about to get >> worse with the changes to trampoline relocation. For the sake of one variable >> read, the complexity is not worth it. >> >> Reorder the logic to copy the trampoline into place, then switch stack and >> enter the trampoline. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> >> Roger: I'd like this to be a prerequisite to your "[PATCH 4/7] x86/boot: apply >> trampoline relocations at destination position" to avoid the movement of >> reloc_trampoline32() making things worse. > I think you could commit this now-ish, and I can rebase on top? CI said no, and the bug is hiding in plain sight. The setup for the rep movs: lea sym_esi(trampoline_start), %esi mov sym_esi(trampoline_phys), %edi is buggy. I'll try and find a nicer way to do this. ~Andrew
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 59a2b5005cf6..4082d7c6c0a0 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -697,19 +697,20 @@ trampoline_setup: call cmdline_parse_early 1: - /* Switch to low-memory stack which lives at the end of trampoline region. */ - mov sym_esi(trampoline_phys), %edi - lea TRAMPOLINE_SIZE(%edi), %esp - lea trampoline_boot_cpu_entry-trampoline_start(%edi),%eax - pushl $BOOT_CS32 - push %eax - /* Copy bootstrap trampoline to low memory, below 1MB. */ lea sym_esi(trampoline_start), %esi + mov sym_esi(trampoline_phys), %edi mov $((trampoline_end - trampoline_start) / 4),%ecx rep movsl - /* Jump into the relocated trampoline. */ + /* Switch to low-memory stack which lives at the end of trampoline. */ + mov sym_esi(trampoline_phys), %edi + lea TRAMPOLINE_SIZE(%edi), %esp + + /* Enter the trampoline at trampoline_boot_cpu_entry(). */ + lea trampoline_boot_cpu_entry - trampoline_start(%edi), %eax + pushl $BOOT_CS32 + push %eax lret ENTRY(trampoline_start)
The LRET is detached from the PUSHes which set it up, and this is about to get worse with the changes to trampoline relocation. For the sake of one variable read, the complexity is not worth it. Reorder the logic to copy the trampoline into place, then switch stack and enter the trampoline. 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> Roger: I'd like this to be a prerequisite to your "[PATCH 4/7] x86/boot: apply trampoline relocations at destination position" to avoid the movement of reloc_trampoline32() making things worse. --- xen/arch/x86/boot/head.S | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)