On 14.11.2024 19:22, Andrew Cooper wrote: > It may have taken a while, but it occurs to me that the mentioned commit fixed > a second problem too. > > On entering trampoline_boot_cpu_entry(), %esp points at the trampoline stack, > but in a 32bit flat segment. It happens to be page aligned. > > When dropping into 16bit mode, the stack segment operates on %sp, preserving > the upper bits. Prior to 1ed76797439e, the top nibble of %sp would depend on > where the trampoline was placed in low memory, and only had a 1/16 chance of > being 0 and therefore operating on the intended stack. > > There was a 15/16 chance of using a different page in the trampoline as if it > were the stack. Therefore, zeroing %esp was correct, but for more reasons > than realised at the time.
I'm afraid I don't follow this analysis. Said commit replaced clearing of %sp by clearing of %esp. That made no difference for anything using the 16-bit register. I don't see how the top nibble of %sp could have been non-zero prior to that change. > Update the comment to explain why zeroing %esp is correct in all cases. Move > it marginally earlier to when a 16bit stack segment is first loaded. The movement is fine, and the comment is fine by itself, too. It doesn't cover the significance of using 32-bit operand size, though (which may or may not be relevant, to a fair degree depending on the above). Jan > --- a/xen/arch/x86/boot/trampoline.S > +++ b/xen/arch/x86/boot/trampoline.S > @@ -176,6 +176,12 @@ trampoline_boot_cpu_entry: > mov %eax,%gs > mov %eax,%ss > > + /* > + * The stack is at trampoline_phys + 64k, which for a 16bit stack > + * segment wants %sp starting at 0. > + */ > + xor %esp, %esp > + > /* Switch to pseudo-rm CS, enter real mode, and flush insn queue. */ > mov %cr0,%eax > dec %eax > @@ -190,8 +196,6 @@ trampoline_boot_cpu_entry: > mov %ax,%es > mov %ax,%ss > > - /* Initialise stack pointer and IDT, and enable irqs. */ > - xor %esp,%esp > lidt bootsym(rm_idt) > sti >
