Re: [PATCH v2 3/5] KVM: x86: Deliver exception payload on KVM_GET_VCPU_EVENTS

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Tue, Jan 28, 2020 at 01:27:13AM -0800, Oliver Upton wrote:
> KVM doesn't utilize exception payloads by default, as this behavior
> diverges from the expectations of the userspace API. However, this
> constraint only applies if KVM is servicing a KVM_GET_VCPU_EVENTS ioctl
> before delivering the exception.
> 
> Use exception payloads unconditionally if the vcpu is in guest mode.

This sentence is super confusing.  It doesn't align with the code, which
is clearly handling "not is in guest mode".  And KVM already uses payloads
unconditionally, it's only the deferring behavior that is changing.

> Deliver the exception payload before completing a KVM_GET_VCPU_EVENTS
> to ensure API compatibility.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/x86/kvm/x86.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7a341c0c978a..9f080101618c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -497,19 +497,7 @@ static void kvm_multiple_exception(struct kvm_vcpu *vcpu,
>  		vcpu->arch.exception.error_code = error_code;
>  		vcpu->arch.exception.has_payload = has_payload;
>  		vcpu->arch.exception.payload = payload;
> -		/*
> -		 * In guest mode, payload delivery should be deferred,
> -		 * so that the L1 hypervisor can intercept #PF before
> -		 * CR2 is modified (or intercept #DB before DR6 is
> -		 * modified under nVMX).  However, for ABI
> -		 * compatibility with KVM_GET_VCPU_EVENTS and
> -		 * KVM_SET_VCPU_EVENTS, we can't delay payload
> -		 * delivery unless userspace has enabled this
> -		 * functionality via the per-VM capability,
> -		 * KVM_CAP_EXCEPTION_PAYLOAD.
> -		 */
> -		if (!vcpu->kvm->arch.exception_payload_enabled ||
> -		    !is_guest_mode(vcpu))
> +		if (!is_guest_mode(vcpu))
>  			kvm_deliver_exception_payload(vcpu);
>  		return;
>  	}
> @@ -3790,6 +3778,21 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
>  {
>  	process_nmi(vcpu);
>  
> +	/*
> +	 * In guest mode, payload delivery should be deferred,
> +	 * so that the L1 hypervisor can intercept #PF before
> +	 * CR2 is modified (or intercept #DB before DR6 is
> +	 * modified under nVMX).  However, for ABI
> +	 * compatibility with KVM_GET_VCPU_EVENTS and
> +	 * KVM_SET_VCPU_EVENTS, we can't delay payload
> +	 * delivery unless userspace has enabled this
> +	 * functionality via the per-VM capability,
> +	 * KVM_CAP_EXCEPTION_PAYLOAD.
> +	 */

This comment needs to be rewritten.  It makes sense in the context of
kvm_multiple_exception(), here it's just confusing.

> +	if (!vcpu->kvm->arch.exception_payload_enabled &&
> +	    vcpu->arch.exception.pending && vcpu->arch.exception.has_payload)
> +		kvm_deliver_exception_payload(vcpu);

Crushing CR2/DR6 just because userspace is retrieving info can't possibly
be correct.  If it somehow is correct then this needs big fat comment.

What is the ABI compatibility issue?  E.g. can KVM simply hide the payload
info on KVM_GET_VCPU_EVENT and then drop it on KVM_SET_VCPU_EVENTS?

> +
>  	/*
>  	 * The API doesn't provide the instruction length for software
>  	 * exceptions, so don't report them. As long as the guest RIP
> -- 
> 2.25.0.341.g760bfbb309-goog
>