Re: Getting rid of invalid SYSCALL RSP under Xen?

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 26/07/2015 23:08, Andy Lutomirski wrote:
>
>>>   If so, can we just
>>> enter later on:
>>>
>>>     pushq    %r11                /* pt_regs->flags */
>>>     pushq    $__USER_CS            /* pt_regs->cs */
>>>     pushq    %rcx                /* pt_regs->ip */
>>>
>>> <-- Xen enters here
>>>
>>>     pushq    %rax                /* pt_regs->orig_ax */
>>>     pushq    %rdi                /* pt_regs->di */
>>>     pushq    %rsi                /* pt_regs->si */
>>>     pushq    %rdx                /* pt_regs->dx */
>> This looks plausible, and indeed preferable to the current doublestep
>> with undo_xen_syscall.
>>
>> One slight complication is that xen_enable_syscall() will want to
>> special case register_callback() to not set CALLBACKF_mask_events, as
>> the entry point is now after re-enabling interrupts.
> I wouldn't do that.  Let's just move the ENABLE_INTERRUPTS a few
> instructions later even on native -- I want to do that anyway.

That would also work.

>
>>> For SYSRET, I think the way to go is to force Xen to always use the
>>> syscall slow path.  Instead, Xen could hook into
>>> syscall_return_via_sysret or even right before the opportunistic
>>> sysret stuff.  Then we could remove the USERGS_SYSRET hooks entirely.
>>>
>>> Would this work?
>> None of the opportunistic sysret stuff makes sense under Xen.  The path
>> will inevitably end up in xen_iret making a hypercall.  Short circuiting
>> all of this seems like a good idea, especially if it allows for the
>> removal of the UERGS_SYSRET.
> Doesn't Xen decide what to do based on VGCF_IN_SYSCALL?  Maybe Xen
> should have its own opportunistic VGCF_IN_SYSCALL logic.

VGCF_in_syscall affects whether the extra r11/rcx get restored or not,
as the hypercall itself is implemented using syscall.  As the extra
r11/rcx (and rax for that matter) are unconditionally saved in the
hypercall stub, I can't see anything Linux could usefully do,
opportunistically speaking.

>
> Hmm, maybe some of this would be easier to think about if, rather than
> having a paravirt op, we could have:
>
> ALTERNATIVE "", "jmp xen_pop_things_and_iret", X86_FEATURE_XEN
>
> Or just IF_XEN("jmp ...");
>
> As a practical matter, x86_64 has native and Xen -- I don't think
> there's any other paravirt platform that needs the asm hooks.

It would certainly seem so.  A careful use of IF_XEN() or two would make
the code far clearer to read, and to drop the hooks.

~Andrew