Re: [PATCH] KVM: VMX: remove LFENCE in vmx_spec_ctrl_restore_host()

[Sean Christopherson <seanjc@google.com>]

On Mon, Jun 05, 2023, Pawan Gupta wrote:
> On Mon, Jun 05, 2023 at 04:39:02PM +0000, Jon Kohler wrote:
> > >>> Yes.  Though in practice it might not make much of a difference.  With
> > >>> wrmsr+lfence, the lfence has nothing to do so it might be almost
> > >>> instantaneous anyway.
> > >>> 
> > >>> -- 
> > >>> Josh
> > >> 
> > >> Coming back to this, what if we hoisted call vmx_spec_ctrl_restore_host above
> > >> FILL_RETURN_BUFFER, and dropped this LFENCE as I did here?
> > >> 
> > >> That way, we wouldn’t have to mess with the internal LFENCE in nospec-branch.h,
> > >> and that would act as the “final line of defense” LFENCE.
> > >> 
> > >> Would that be acceptable? Or does FILL_RETURN_BUFFER *need* to occur
> > >> before any sort of calls no matter what?
> > > 
> > > If we go by Intel's statement that only unbalanced RETs are a concern,
> > > that *might* be ok as long as there's a nice comment above the
> > > FILL_RETURN_BUFFER usage site describing the two purposes for the
> > > LFENCE.
> 
> We would then need FILL_RETURN_BUFFER to unconditionally execute LFENCE
> to account for wrmsr branch misprediction. Currently LFENCE is not
> executed for !X86_BUG_EIBRS_PBRSB.
> 
> > > However, based on Andy's concerns, which I've discussed with him
> > > privately (but I'm not qualified to agree or disagree with), we may want
> > > to just convert vmx_spec_ctrl_restore_host() to asm.  Better safe than
> > > sorry.  My original implementation of that function was actually asm.  I
> > > can try to dig up that code.
> 
> Note:
> 
>   VMexit
>   CALL
>     RET
>   RET    <---- This is also a problem if the first call hasn't retired yet.
>   LFENCE
> 
> Converting vmx_spec_ctrl_restore_host() to ASM should be able to take care
> of this.

Is there an actual bug here, or are we just micro-optimizing something that may or
may not need additional optimization?  Unless there's a bug to be fixed, moving
code into ASM and increasing complexity doesn't seem worthwhile.