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@xxxxxxxxxx>
> ---
> 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)
