[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v2 2/6] x86/boot: Only jump into low trampoline code for real-mode boot
On 21.08.2019 16:04, David Woodhouse wrote:
On Mon, 2019-08-12 at 11:10 +0200, Jan Beulich wrote:
On 09.08.2019 17:01, David Woodhouse wrote:
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -735,7 +735,17 @@ trampoline_setup:
/* Switch to low-memory stack which lives at the end of trampoline
region. */
mov sym_fs(trampoline_phys),%edi
lea TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE(%edi),%esp
+ cmpb $0, sym_fs(skip_realmode)
+ jz 1f
+ /* If no-real-mode, jump straight to trampoline_protmode_entry */
+ lea trampoline_protmode_entry-trampoline_start(%edi),%eax
+ /* EBX == 0 indicates we are the BP (Boot Processor). */
+ xor %ebx,%ebx
+ jmp 2f
+1:
+ /* Go via 16-bit code in trampoline_boot_cpu_entry */
lea trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
+2:
pushl $BOOT_CS32
push %eax
May I suggest to slightly streamline this into
/* Switch to low-memory stack which lives at the end of trampoline
region. */
mov sym_fs(trampoline_phys),%edi
lea TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE(%edi),%esp
/* Go via 16-bit code in trampoline_boot_cpu_entry */
lea trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
cmpb $0,sym_fs(skip_realmode)
je 1f
/* If no-real-mode, jump straight to trampoline_protmode_entry */
lea trampoline_protmode_entry-trampoline_start(%edi),%eax
/* EBX == 0 indicates we are the BP (Boot Processor). */
xor %ebx,%ebx
1:
pushl $BOOT_CS32
push %eax
perhaps with the second slightly adapted to it now being an override
rather than an alternative path?
It's a *temporary* alternative path, and it's gone away by the end of
the series.
Indeed I did notice it's temporary when making it to the later patches
of the series. If all parts go in at about the same time, I think I'm
okay with leaving the code as is.
Additionally I think it would be nice if the clearing of %ebx wasn't
replicated here and ...
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -194,9 +194,6 @@ gdt_48: .word 6*8-1
.code32
trampoline_boot_cpu_entry:
- cmpb $0,bootsym_rel(skip_realmode,5)
- jnz .Lskip_realmode
-
/* Load pseudo-real-mode segments. */
mov $BOOT_PSEUDORM_DS,%eax
mov %eax,%ds
@@ -276,7 +273,6 @@ trampoline_boot_cpu_entry:
mov %eax,%gs
mov %eax,%ss
-.Lskip_realmode:
/* EBX == 0 indicates we are the BP (Boot Processor). */
xor %ebx,%ebx
... here. Why don't you further do
.code32
trampoline_protmode_entry_bsp:
/* EBX == 0 indicates we are the BSP (Boot Strap Processor).
*/
xor %ebx,%ebx
trampoline_protmode_entry:
directing the BSP paths to the new label?
Yeah, I kind of see your point. But that gives us one entry point which
clears %ebx... and one which doesn't, so you still have to make sure
it's not already zero for the AP startup.
If we ended up with two simple entry points that didn't care about %ebx
for trampoline_protmode_entry_bsp and trampoline_protmode_entry_ap then
that'd be nice and simple — but I don't like the inconsistency.
I think I prefer having to set %ebx explicitly in all three separate
callers, over having one entry point that requires it and another that
doesn't.
I think differently, but I'm not going to insist if Andrew agrees with
your preference.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
