Re: [Xen-devel] [PATCH 2/4] x86/boot: Minor improvements to efi_arch_post_exit_boot()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.08.2019 14:42, Andrew Cooper wrote:
> Split up the long asm block by commenting the logical subsections.
>
> The movabs for obtaining __start_xen can be a rip-relative lea instead.  This
> has the added advantage that objdump can now cross reference it during
> disassembly.
I'm surprised this works, but I take it that you've tested it: At
the time the asm() executes, I thought we're still in (what EFI
calls) physical mode, i.e. %rip holding a value less than 4Gb.
Accessing memory using %rip-relative addressing is fine, since
the Xen image is mapped in both places, but obtaining the new
linear address for %rip this way via lea should not be, as this
wouldn't move us to the XEN_VIRT_{START,END} range. I'm curious
to learn which part of my understanding is wrong here.

> The stack handing is confusing to follow.  %rsp is set up by reading
> stack_start which is a pointer to cpu0_stack, then constructing an lret frame
> under %rsp (to avoid clobbering whatever is adjacent to cpu0_stack), and uses
> the Pascal form of lret to move %rsp to the base of cpu0_stack.
>
> Remove stack_start from the mix and use a single lea to load cpu0_stack base
> directly,
I disagree with this change, at least as long as
xen/arch/x86/boot/x86_64.S also reads from stack_start, rather than
accessing cpu0_stack directly. The code here is intended to mirror
what's being done on the non-EFI path. And it was always my
understanding that it's done this way such that one would need to go
hunt for uses if one wanted to change what (right now) stack_start
points at during pre-SMP boot. Otherwise stack_start wouldn't need
an initializer anymore, and hence could move to .bss.

> and use the more common push/push/lretq sequence for reloading %cs.
I don't see what's wrong with what you call "Pascal form" of lret
(C's __stdcall uses this as well, for example). I don't heavily
mind this transformation, but (I'm sorry to say that) it looks to
me as if this was a change for the sake of changing the code, not
for making it any "better" (for whatever definition of "better").

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel