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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.08.2019 12:33, Andrew Cooper wrote:
> 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".)
Not a rhetorical question at all. As said - the pre-OS environment
is a physical one, hence relocating binaries to their preferred
addresses may (and with the addresses we use definitely is) not
possible. Hence them relocating images by default.

> > 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 I'd be okay (but not enthusiastic) to see the other CPU0 use
disappear at the same time (same series at least, not necessarily
same patch).

> > > 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...
Still, I don't see what Pascal has got to do with it other than it
being (having been) one of the users of it. I don't think insn use
in our code base should be influenced by what languages or other
environments use them. If they're suitable for the purpose, they're
fine to use.

Jan

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