Re: [PATCH 3/3] KVM: x86: move kvm_inject_gp up from kvm_set_dr to callers

From: Sean Christopherson <seanjc@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH 3/3] KVM: x86: move kvm_inject_gp up from kvm_set_dr to callers
Date: Tue, 2 Feb 2021 10:17:16 -0800	[thread overview]
Message-ID: <YBmXLJPPTS7yzClF@google.com> (raw)
In-Reply-To: <20210202165141.88275-4-pbonzini@redhat.com>

On Tue, Feb 02, 2021, Paolo Bonzini wrote:
> Push the injection of #GP up to the callers, so that they can just use
> kvm_complete_insn_gp. __kvm_set_dr is pretty much what the callers can use
> together with kvm_complete_insn_gp, so rename it to kvm_set_dr and drop
> the old kvm_set_dr wrapper.
> 
> This allows nested VMX code, which really wanted to use __kvm_set_dr, to
> use the right function.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/svm/svm.c | 14 +++++++-------
>  arch/x86/kvm/vmx/vmx.c | 19 ++++++++++---------
>  arch/x86/kvm/x86.c     | 19 +++++--------------
>  3 files changed, 22 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index c0d41a6920f0..818cf3babef2 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2599,6 +2599,7 @@ static int dr_interception(struct vcpu_svm *svm)
>  {
>  	int reg, dr;
>  	unsigned long val;
> +	int err;
>  
>  	if (svm->vcpu.guest_debug == 0) {
>  		/*
> @@ -2617,19 +2618,18 @@ static int dr_interception(struct vcpu_svm *svm)
>  	reg = svm->vmcb->control.exit_info_1 & SVM_EXITINFO_REG_MASK;
>  	dr = svm->vmcb->control.exit_code - SVM_EXIT_READ_DR0;
>  
> +	if (!kvm_require_dr(&svm->vcpu, dr & 15))

Purely because I suck at reading base-10 bitwise operations, can we do "dr & 0xf"?
This also creates separate logic for writes (AND versus SUB), can you also
convert the other 'dr - 16'?

Alternatively, grab the "mov to DR" flag early on, but that feels less
readable, e.g.

	mov_to_dr = dr & 0x10;
	dr &= 0xf;

> +		return 1;
> +
>  	if (dr >= 16) { /* mov to DRn */
> -		if (!kvm_require_dr(&svm->vcpu, dr - 16))
> -			return 1;
>  		val = kvm_register_read(&svm->vcpu, reg);
> -		kvm_set_dr(&svm->vcpu, dr - 16, val);
> +		err = kvm_set_dr(&svm->vcpu, dr - 16, val);
>  	} else {
> -		if (!kvm_require_dr(&svm->vcpu, dr))
> -			return 1;
> -		kvm_get_dr(&svm->vcpu, dr, &val);
> +		err = kvm_get_dr(&svm->vcpu, dr, &val);

Technically, 'err' needs to be checked, else 'reg' will theoretically be
clobbered with garbage.  I say "theoretically", because kvm_get_dr() always
returns '0'; the CR4.DE=1 behavior is handled by kvm_require_dr(), presumably
due to it being a #UD instead of #GP.  AFAICT, you can simply add a prep patch
to change the return type to void.

Side topic, is the kvm_require_dr() check needed on SVM interception?  The APM
states:

  All normal exception checks take precedence over the by implicit DR6/DR7 writes.)

I can't find anything that would suggest the CR4.DE=1 #UD isn't a "normal"
exception.

>  		kvm_register_write(&svm->vcpu, reg, val);
>  	}
>  
> -	return kvm_skip_emulated_instruction(&svm->vcpu);
> +	return kvm_complete_insn_gp(&svm->vcpu, err);
>  }
>  
>  static int cr8_write_interception(struct vcpu_svm *svm)