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

[Jon Kohler]

> On Jun 5, 2023, at 2:31 PM, Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> 
> 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.

The (slight) bug here is that on systems where the host != guest spec ctrl, they get
hit with LFENCE + WRMSR to SPEC_CTRL + LFENCE, when in reality they should
just get LFENCE + WRMSR to SPEC_CTRL and thats it. 

That would be satisfied with Pawan’s suggestion to move the LFENCE into the else
condition in the last branch of vmx_spec_ctrl_restore_host().

The optimization on top of that would be to see if we could whack that 2x LFENCE
down to 1x LFENCE. Just feels wrong to have 2x LFENCE’s in the critical path,
even if the second one ends up being fairly snappy because of the first one (and/or
the WRMSR).

So to recap, fixing “the bug” does not require moving to ASM. Optimizing the 2x LFENCE
into 1x LFENCE (probably) requires going to ASM from the sounds of it?