Re: [PATCH v8 33/33] KVM: x86/vmx: refactor VMX_DO_EVENT_IRQOFF to generate FRED stack frames

From: Sean Christopherson <seanjc@google.com>
To: Xin3 Li <xin3.li@intel.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"x86@kernel.org" <x86@kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"bp@alien8.de" <bp@alien8.de>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	Ravi V Shankar <ravi.v.shankar@intel.com>,
	"jiangshanlai@gmail.com" <jiangshanlai@gmail.com>,
	Shan Kang <shan.kang@intel.com>
Subject: Re: [PATCH v8 33/33] KVM: x86/vmx: refactor VMX_DO_EVENT_IRQOFF to generate FRED stack frames
Date: Wed, 12 Apr 2023 12:37:48 -0700	[thread overview]
Message-ID: <ZDcIjF/QnCZNkXJ8@google.com> (raw)
In-Reply-To: <SA1PR11MB6734897C0BC0FC7481333E29A89B9@SA1PR11MB6734.namprd11.prod.outlook.com>

On Wed, Apr 12, 2023, Xin3 Li wrote:
> 
> > And then this is equally gross.  Rather than funnel FRED+legacy into a single
> > function only to split them back out, just route FRED into its own asm subroutine.
> > The common bits are basically the creation/destruction of the stack frame and
> > the CALL itself, i.e. the truly interesting bits are what's different.
> 
> I try to catch up with you but am still confused.
> 
> Because a FRED stack frame always contains an error code pushed after RIP,
> the FRED entry code doesn't push any error code.
> 
> Thus I introduced a trampoline code, which is called to have the return
> instruction address pushed first. Then the trampoline code pushes an error
> code (0 for both IRQ and NMI) and jumps to fred_entrypoint_kernel() for NMI
> handling or calls external_interrupt() for IRQ handling.
> 
> The return RIP is used to return from fred_entrypoint_kernel(), but not
> external_interrupt().

...

> > +	/*
> > +	* A FRED stack frame has extra 16 bytes of information pushed at the
> > +	* regular stack top compared to an IDT stack frame.
> > +	*/
> > +	push $0         /* Reserved by FRED, must be 0 */
> > +	push $0         /* FRED event data, 0 for NMI and external interrupts */
> > +	shl $32, %rax
> > +	orq $__KERNEL_DS | $FRED_64_BIT_MODE, %ax
> > +	push %rax	/* Vector (from the "caller") and DS */
> > +
> > +	push %rbp
> > +	pushf
> > +	push \cs_val
> 
> We need to push the RIP of the next instruction here. Or are you suggesting
> we don't need to care about it because it may not be used to return from the
> callee?

...

> > +	push $0 /* FRED error code, 0 for NMI and external interrupts */
> > +	PUSH_REGS
> > +
> > +	/* Load @pt_regs */
> > +	movq    %rsp, %_ASM_ARG1
> > +
> > +	call \call_target

The CALL here would push RIP, I missed/forgot the detail that the error code needs
to be pushed _after_ RIP, not before.

Unless CET complains, there's no need for a trampoline, just LEA+PUSH the return
RIP, PUSH the error code, and JMP to the handler.  IMO, that isn't any weirder than
a trampoline, and it's a bit more obviously weird, e.g. the LEA+PUSH can have a nice
big comment.