Re: [PATCH] KVM: nVMX: Emulate NOPs in L2, and PAUSE if it's not intercepted

[Paolo Bonzini]

On 4/5/23 02:23, Sean Christopherson wrote:
> Extend VMX's nested intercept logic for emulated instructions to handle
> "pause" interception, in quotes because KVM's emulator doesn't filter out
> NOPs when checking for nested intercepts.  Failure to allow emulation of
> NOPs results in KVM injecting a #UD into L2 on any NOP that collides with
> the emulator's definition of PAUSE, i.e. on all single-byte NOPs.
>
> For PAUSE itself, honor L1's PAUSE-exiting control, but ignore PLE to
> avoid unnecessarily injecting a #UD into L2.  Per the SDM, the first
> execution of PAUSE after VM-Entry is treated as the beginning of a new
> loop, i.e. will never trigger a PLE VM-Exit, and so L1 can't expect any
> given execution of PAUSE to deterministically exit.
>
>    ... the processor considers this execution to be the first execution of
>    PAUSE in a loop. (It also does so for the first execution of PAUSE at
>    CPL 0 after VM entry.)
>
> All that said, the PLE side of things is currently a moot point, as KVM
> doesn't expose PLE to L1.
>
> Note, vmx_check_intercept() is still wildly broken when L1 wants to
> intercept an instruction, as KVM injects a #UD instead of synthesizing a
> nested VM-Exit.  That issue extends far beyond NOP/PAUSE and needs far
> more effort to fix, i.e. is a problem for the future.
>
> Fixes: 07721feee46b ("KVM: nVMX: Don't emulate instructions in guest mode")
> Cc: Mathias Krause <minipli@xxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>   arch/x86/kvm/vmx/vmx.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 9ae4044f076f..1e560457bf9a 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7898,6 +7898,21 @@ static int vmx_check_intercept(struct kvm_vcpu *vcpu,
>   		/* FIXME: produce nested vmexit and return X86EMUL_INTERCEPTED.  */
>   		break;
>   
> +	case x86_intercept_pause:
> +		/*
> +		 * PAUSE is a single-byte NOP with a REPE prefix, i.e. collides
> +		 * with vanilla NOPs in the emulator.  Apply the interception
> +		 * check only to actual PAUSE instructions.  Don't check
> +		 * PAUSE-loop-exiting, software can't expect a given PAUSE to
> +		 * exit, i.e. KVM is within its rights to allow L2 to execute
> +		 * the PAUSE.
> +		 */
> +		if ((info->rep_prefix != REPE_PREFIX) ||
> +		    !nested_cpu_has2(vmcs12, CPU_BASED_PAUSE_EXITING))
> +			return X86EMUL_CONTINUE;
> +
> +		break;
> +
>   	/* TODO: check more intercepts... */
>   	default:
>   		break;
>
> base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393

Reviewed-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>

Would you like me to apply this for 6.3?

Paolo