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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/08/2019 16:20, Jan Beulich wrote:
> 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:

So I did specifically test it, but it now occurs to me that the test I
did was via the MB2 64-bit EFI path, which isn't this path.  /sigh

> 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.

In which case, what is the point of using a file format which does
identify the virtual address it would prefer to run at...

(This is a rhetorical question.  The answer is "because EFI seems to
always do the unhelpful thing, given the choice".)

> 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.

That doesn't mean that a) its conceptually the right thing to do ...

> 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.

... or b) that I have any intention of letting stack_start survive. 
Specifically, it is an unnecessary point of serialisation for APs, which
needs to disappear.

cpu0_stack is where cpu0 should have its stack, and this path is
exclusive to cpu0.

>
>> 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'm afraid that this statement clearly highlights the problem I'm trying
to solve.

> 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").

It really doesn't matter if you can follow the code, or whether I can
follow it when I've double checked the instruction behaviour because,
while I'm aware this form exists, frankly I'm a little rusty on Pascal
it having ceased to be a dominant programming language before I was born...

The easier code is to follow, the easier time other people will have to
debug and develop on it, and the healthier the Xen community will be as
result.

~Andrew

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