Re: [PATCH 1/2] KVM: VMX: Move IRQ invocation to assembly subroutine

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Mon, Sep 14, 2020 at 02:07:19PM -0700, Sean Christopherson wrote:
> > RSP needs to be aligned to what?  How would this align the stack, other
> > than by accident?
> 
> Ah, yeah, that's lacking info.
> 
> 16-byte aligned to correctly mimic CPU behavior when vectoring an IRQ/NMI.
> When not changing stack, the CPU aligns RSP before pushing the frame.
> 
> The above shenanigans work because the x86-64 ABI also requires RSP to be
> 16-byte aligned prior to CALL.  RSP is thus 8-byte aligned due to CALL
> pushing the return IP, and so creating the stack frame by pushing RBP makes
> it 16-byte aliagned again.

As Uros mentioned, the kernel doesn't do this.

> > > +
> > > +#ifdef CONFIG_X86_64
> > > +	push $__KERNEL_DS
> > > +	push %_ASM_BP
> > > +#endif
> > > +	pushf
> > > +	push $__KERNEL_CS
> > > +	CALL_NOSPEC _ASM_ARG1
> > > +
> > > +	/*
> > > +	 * "Restore" RSP from RBP, even though IRET has already unwound RSP to
> > > +	 * the correct value.  objtool doesn't know the target will IRET and so
> > > +	 * thinks the stack is getting walloped (without the explicit restore).
> > > +	 */
> > > +	mov %_ASM_BP, %rsp
> > > +	pop %_ASM_BP
> > > +	ret
> > 
> > BTW, there *is* actually an unwind hint for this situation:
> > UNWIND_HINT_RET_OFFSET.
> 
> I played with that one, but for the life of me couldn't figure out how to
> satisfy both the "stack size" and "cfa.offset" checks.  In the code below,
> cfa.offset will be 8, stack_size will be 40 and initial_func_cfi.cfa.offset
> will be 8.  But rereading this, I assume I missed something that would allow
> maniuplating cfa.offset?  Or maybe I botched my debugging?
> 
> static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
> {
> 	...
> 
>         if (cfi->cfa.offset != initial_func_cfi.cfa.offset + ret_offset)
>                 return true;
> 
>         if (cfi->stack_size != initial_func_cfi.cfa.offset + ret_offset)
>                 return true;
> 
> 	...
> }

It only works without the frame pointer, in which case stack size and
cfa.offset will be the same (see below code).  With the frame pointer,
it probably wouldn't work.

But if you're going to be aligning the stack in the next patch version,
your frame pointer approach works better anyway, because the stack size
will be variable depending on the stack alignment of the callee.  So
forget I said anything :-)

> > So you might be able to do something like the following (depending on
> > what your alignment requirements actually are):
> > 
> > SYM_FUNC_START(vmx_do_interrupt_nmi_irqoff)
> > #ifdef CONFIG_X86_64
> > 	push $__KERNEL_DS
> > 	push %_ASM_BP
> > #endif
> > 	pushf
> > 	push $__KERNEL_CS
> > 	CALL_NOSPEC _ASM_ARG1
> > 
> > 	/* The call popped the pushes */
> > 	UNWIND_HINT_RET_OFFSET sp_offset=32
> > 
> > 	ret
> > SYM_FUNC_END(vmx_do_interrupt_nmi_irqoff)

-- 
Josh