Re: [PATCH] KVM: VMX: Update instruction length on intercepted BP

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3811 bytes --]

Avi Kivity wrote:
> On 02/14/2010 12:34 PM, Gleb Natapov wrote:
>>>>
>>>> event_exit_inst_len is only used for event reinjection. Since event
>>>> intercepted here will not be reinjected why updating
>>>> event_exit_inst_len
>>>> is needed here?
>>>>        
>>> In guest debugging mode a #BP exception is always reported to user space
>>> to find out what caused it. If it was the guest itself, the exception is
>>> reinjected, on older kernels via KVM_SET_GUEST_DEBUG and since 2.6.33
>>> via KVM_SET_VCPU_EVENTS (the latter requires some qemu patch that I will
>>> post later).
>>>
>>> As we currently do not update event_exit_inst_len on #BP exits,
>>> reinjecting fails unless event_exit_inst_len happens to be 1 from some
>>> other exit.
>>>
>>>      
>> Hmm, how does it work on SVM then where we do not have
>> event_exit_inst_len so execution will resume on the same rip that caused
>> #BP after event reinjection?
>>
>>    
> 
> Note, newer AMDs do have such a field (nRIP, 0xC8).  We need to support
> older machines, though.
> 

Nice.

[ /me goes updating his manual - September 07... ]

Actually, emulation via single-stepping should work as only #DB clears
the TF on entry. So this draft may fly (nothing to test on at hand):

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 52f78dd..450c1e7 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -109,6 +109,7 @@ struct vcpu_svm {
 	struct nested_state nested;
 
 	bool nmi_singlestep;
+	bool bp_singlestep;
 };
 
 /* enable NPT for AMD64 and X86 with PAE */
@@ -244,6 +245,19 @@ static void svm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
 	if (nested_svm_check_exception(svm, nr, has_error_code, error_code))
 		return;
 
+	if (nr == BP_VECTOR && vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) {
+		/*
+		 * Temporarily disable BP interception so that the trap is
+		 * properly delivered to the guest (if we left it on, SVM
+		 * would push the wrong IP on the stack).
+		 * We single-step into theexception handler in order to
+		 * reenble interception ASAP.
+		 */
+		svm->vmcb->control.intercept_exceptions &= ~(1 << BP_VECTOR);
+		svm->bp_singlestep = true;
+		svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
+	}
+
 	svm->vmcb->control.event_inj = nr
 		| SVM_EVTINJ_VALID
 		| (has_error_code ? SVM_EVTINJ_VALID_ERR : 0)
@@ -1083,7 +1097,8 @@ static void update_db_intercept(struct kvm_vcpu *vcpu)
 		    (KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP))
 			svm->vmcb->control.intercept_exceptions |=
 				1 << DB_VECTOR;
-		if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
+		if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP &&
+		    !svm->bp_singlestep)
 			svm->vmcb->control.intercept_exceptions |=
 				1 << BP_VECTOR;
 	} else
@@ -1214,7 +1229,7 @@ static int db_interception(struct vcpu_svm *svm)
 
 	if (!(svm->vcpu.guest_debug &
 	      (KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP)) &&
-		!svm->nmi_singlestep) {
+		!svm->nmi_singlestep && !svm->bp_singlestep) {
 		kvm_queue_exception(&svm->vcpu, DB_VECTOR);
 		return 1;
 	}
@@ -1227,6 +1242,14 @@ static int db_interception(struct vcpu_svm *svm)
 		update_db_intercept(&svm->vcpu);
 	}
 
+	if (svm->bp_singlestep) {
+		svm->bp_singlestep = false;
+		if (!(svm->vcpu.guest_debug & KVM_GUESTDBG_SINGLESTEP))
+			svm->vmcb->save.rflags &=
+				~(X86_EFLAGS_TF | X86_EFLAGS_RF);
+		update_db_intercept(&svm->vcpu);
+	}
+
 	if (svm->vcpu.guest_debug &
 	    (KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP)){
 		kvm_run->exit_reason = KVM_EXIT_DEBUG;


But it requires a bit more work as I just realized that already
nmi_singlestep could cause spurious #DB reports to user space that may
propagate to the guest in the end.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]