KVM: VMX: Update instruction length on intercepted BP

PATCH In review Jan Kiszka 4 participants 39 messages Feb 13, 2010 – Feb 17, 2010

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Jan Kiszka

author

2010-02-13 09:31 UTC #0 lore raw

PATCH 1/1 KVM: VMX: Update instruction length on intercepted BP

From: Jan Kiszka <[email protected]>

We intercept #BP while in guest debugging mode. As VM exists due to intercepted exceptions do not necessarily come with valid idt_vectoring, we have to update event_exit_inst_len explicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.

Signed-off-by Jan Kiszka <[email protected]>

---

Please consider for stable, this fixes guest debugging scenarios.

 arch/x86/kvm/vmx.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

arch/x86/kvm/vmx.c +4 -0

index f82b072..e9f64e8 100644
@@ -2775,6 +2775,8 @@ static int handle_rmode_exception(struct kvm_vcpu *vcpu,
 		kvm_queue_exception(vcpu, vec);
 		return 1;
 	case BP_VECTOR:
+		to_vmx(vcpu)->vcpu.arch.event_exit_inst_len =
+			vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 		if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
 			return 0;
 		/* fall through */

@@ -2897,6 +2899,8 @@ static int handle_exception(struct kvm_vcpu *vcpu)
 		kvm_run->debug.arch.dr7 = vmcs_readl(GUEST_DR7);
 		/* fall through */
 	case BP_VECTOR:
+		vmx->vcpu.arch.event_exit_inst_len =
+			vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 		kvm_run->exit_reason = KVM_EXIT_DEBUG;
 		kvm_run->debug.arch.pc = vmcs_readl(GUEST_CS_BASE) + rip;
 		kvm_run->debug.arch.exception = ex_no;

Gleb Natapov

2010-02-14 07:53 UTC #1 lore raw

Quoted 8 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>
We intercept #BP while in guest debugging mode. As VM exists due to intercepted exceptions do not necessarily come with valid idt_vectoring, we have to update event_exit_inst_len explicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.

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?

Quoted 23 lines

Signed-off-by Jan Kiszka <[email protected]>

---

Please consider for stable, this fixes guest debugging scenarios.

 arch/x86/kvm/vmx.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

arch/x86/kvm/vmx.c +2 -0

index f82b072..e9f64e8 100644
@@ -2775,6 +2775,8 @@ static int handle_rmode_exception(struct kvm_vcpu *vcpu,
 		kvm_queue_exception(vcpu, vec);
 		return 1;
 	case BP_VECTOR:
+		to_vmx(vcpu)->vcpu.arch.event_exit_inst_len =
+			vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 		if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
 			return 0;
 		/* fall through */

arch/x86/kvm/vmx.c static int handle_exception(struct kvm_vcpu *vcpu) · line 2906

Quoted 8 lines

arch/x86/kvm/vmx.c +2 -0

@@ -2897,6 +2899,8 @@ static int handle_exception(struct kvm_vcpu *vcpu)
 		kvm_run->debug.arch.dr7 = vmcs_readl(GUEST_DR7);
 		/* fall through */
 	case BP_VECTOR:
+		vmx->vcpu.arch.event_exit_inst_len =
+			vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 		kvm_run->exit_reason = KVM_EXIT_DEBUG;
 		kvm_run->debug.arch.pc = vmcs_readl(GUEST_CS_BASE) + rip;
 		kvm_run->debug.arch.exception = ex_no;

--
Gleb.

Jan Kiszka

author

2010-02-14 10:26 UTC #2 lore raw

Quoted 9 lines · Gleb Natapov

Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
We intercept #BP while in guest debugging mode. As VM exists due to intercepted exceptions do not necessarily come with valid idt_vectoring, we have to update event_exit_inst_len explicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.

Quoted 3 lines

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.

Jan

Quoted 37 lines

Quoted 24 lines

Signed-off-by Jan Kiszka <[email protected]>

---

Please consider for stable, this fixes guest debugging scenarios.

 arch/x86/kvm/vmx.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

arch/x86/kvm/vmx.c +2 -0

index f82b072..e9f64e8 100644
@@ -2775,6 +2775,8 @@ static int handle_rmode_exception(struct kvm_vcpu *vcpu,
 		kvm_queue_exception(vcpu, vec);
 		return 1;
 	case BP_VECTOR:
+		to_vmx(vcpu)->vcpu.arch.event_exit_inst_len =
+			vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 		if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
 			return 0;
 		/* fall through */

arch/x86/kvm/vmx.c static int handle_exception(struct kvm_vcpu *vcpu) · line 2906

Quoted 8 lines

arch/x86/kvm/vmx.c +2 -0

@@ -2897,6 +2899,8 @@ static int handle_exception(struct kvm_vcpu *vcpu)
 		kvm_run->debug.arch.dr7 = vmcs_readl(GUEST_DR7);
 		/* fall through */
 	case BP_VECTOR:
+		vmx->vcpu.arch.event_exit_inst_len =
+			vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 		kvm_run->exit_reason = KVM_EXIT_DEBUG;
 		kvm_run->debug.arch.pc = vmcs_readl(GUEST_CS_BASE) + rip;
 		kvm_run->debug.arch.exception = ex_no;

--
Gleb.

Gleb Natapov

2010-02-14 10:34 UTC #3 lore raw

Quoted 20 lines · Jan Kiszka

Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
We intercept #BP while in guest debugging mode. As VM exists due to intercepted exceptions do not necessarily come with valid idt_vectoring, we have to update event_exit_inst_len explicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.
Quoted 3 lines
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).

Quoted 4 lines

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?

--
Gleb.

Jan Kiszka

author

2010-02-14 10:48 UTC #4 lore raw

Quoted 20 lines · Gleb Natapov

Quoted 19 lines · Jan Kiszka
Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
We intercept #BP while in guest debugging mode. As VM exists due to intercepted exceptions do not necessarily come with valid idt_vectoring, we have to update event_exit_inst_len explicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.
Quoted 3 lines
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).

Quoted 8 lines

Quoted 4 lines
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?

Maybe not at all. I don't think I've tested this scenario on amd so far. Guess it needs some special handling in svm to move rip after the int3 when requesting to inject #BP.

Jan

Gleb Natapov

2010-02-14 11:15 UTC #5 lore raw

Quoted 25 lines · Jan Kiszka

Quoted 24 lines · Gleb Natapov
Quoted 23 lines · Jan Kiszka
Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
We intercept #BP while in guest debugging mode. As VM exists due to intercepted exceptions do not necessarily come with valid idt_vectoring, we have to update event_exit_inst_len explicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.
Quoted 3 lines
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.

Quoted 9 lines

Quoted 4 lines
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?
Maybe not at all. I don't think I've tested this scenario on amd so far. Guess it needs some special handling in svm to move rip after the int3 when requesting to inject #BP.

This will work for VMX too, no? So may be we should design something that will work for both VMX and SVM before applying patches that make oly VMX work? How about disabling #BP intercept and reenter guest (may be single step it at this point to reenable #BP interception ASAP)?

PS: moving rip after int3 may lead to wrong rip to be pushed on exception stack if exception happens during event delivery.

--
Gleb.

Jan Kiszka

author

2010-02-14 11:40 UTC #6 lore raw

Quoted 30 lines · Gleb Natapov

Quoted 29 lines · Jan Kiszka
Quoted 28 lines · Gleb Natapov
Quoted 19 lines · Jan Kiszka
Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
We intercept #BP while in guest debugging mode. As VM exists due to intercepted exceptions do not necessarily come with valid idt_vectoring, we have to update event_exit_inst_len explicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.
Quoted 3 lines
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).
Quoted 4 lines
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?

Quoted 7 lines

Quoted 4 lines
Maybe not at all. I don't think I've tested this scenario on amd so far. Guess it needs some special handling in svm to move rip after the int3 when requesting to inject #BP.
This will work for VMX too, no? So may be we should design something that will work for both VMX and SVM before applying patches that make oly VMX work?

VMX used to work, so my patch is actually a regression fix. I bet this was accidentally broken while cleaning up the interrupt handling of VMX.

Quoted 2 lines

How about disabling #BP intercept and reenter guest (may be single step it at this point to reenable #BP interception ASAP)?

If there were be some "sticky" single-step that is pushed into exception handlers as well...

Quoted 4 lines

PS: moving rip after int3 may lead to wrong rip to be pushed on exception stack if exception happens during event delivery.

Hmm, would have been too easy.

Jan

Avi Kivity

2010-02-14 14:16 UTC #7 lore raw

Quoted 7 lines · Jan Kiszka

Quoted 3 lines
that will work for both VMX and SVM before applying patches that make oly VMX work?
VMX used to work, so my patch is actually a regression fix. I bet this was accidentally broken while cleaning up the interrupt handling of VMX.

Care to post a test case?

I can't say we run the internal test suite at the moment, but it should really be integrated into autotest.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2010-02-14 16:38 UTC #8 lore raw

Quoted 9 lines · Avi Kivity

Quoted 7 lines · Jan Kiszka
Quoted 3 lines
that will work for both VMX and SVM before applying patches that make oly VMX work?
VMX used to work, so my patch is actually a regression fix. I bet this was accidentally broken while cleaning up the interrupt handling of VMX.

Quoted 5 lines

Care to post a test case?
I can't say we run the internal test suite at the moment, but it should really be integrated into autotest.

I know this should have been done long before. I just haven't worked on autotest so far (a colleague did, though), so there is some reluctance to get wet feet. But maybe I find some rubber boots.

Jan

Avi Kivity

2010-02-14 16:44 UTC #9 lore raw

Quoted 15 lines · Jan Kiszka

Quoted 13 lines · Avi Kivity
Quoted 9 lines · Jan Kiszka
Quoted 4 lines
that will work for both VMX and SVM before applying patches that make oly VMX work?
VMX used to work, so my patch is actually a regression fix. I bet this was accidentally broken while cleaning up the interrupt handling of VMX.
Care to post a test case?

Quoted 8 lines

Quoted 4 lines
I can't say we run the internal test suite at the moment, but it should really be integrated into autotest.
I know this should have been done long before. I just haven't worked on autotest so far (a colleague did, though), so there is some reluctance to get wet feet. But maybe I find some rubber boots.

The autotest people (Lucas copied) are generally good at adding tests, so all we need to do is (a) add a test case to qemu-kvm.git:kvm/user/test (b) indicate to the autotest people how to build and run this thing.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2010-02-14 17:07 UTC #10 lore raw

Quoted 26 lines · Avi Kivity

Quoted 16 lines · Jan Kiszka
Quoted 14 lines · Avi Kivity
Quoted 10 lines · Jan Kiszka
Quoted 4 lines
that will work for both VMX and SVM before applying patches that make oly VMX work?
VMX used to work, so my patch is actually a regression fix. I bet this was accidentally broken while cleaning up the interrupt handling of VMX.
Care to post a test case?
Quoted 8 lines
Quoted 4 lines
I can't say we run the internal test suite at the moment, but it should really be integrated into autotest.
I know this should have been done long before. I just haven't worked on autotest so far (a colleague did, though), so there is some reluctance to get wet feet. But maybe I find some rubber boots.

Quoted 4 lines

The autotest people (Lucas copied) are generally good at adding tests, so all we need to do is (a) add a test case to qemu-kvm.git:kvm/user/test (b) indicate to the autotest people how to build and run this thing.

I wonder if it requires a stand-alone test. So far I'm testing the guest side via gdb installed in some Linux image. We need to interact with gdb anyway to test guest debugging, so this would just add another instance and some indirection (via serial console or ssh into the guest).

Jan

Avi Kivity

2010-02-15 06:48 UTC #11 lore raw

Quoted 11 lines · Jan Kiszka

Quoted 6 lines
The autotest people (Lucas copied) are generally good at adding tests, so all we need to do is (a) add a test case to qemu-kvm.git:kvm/user/test (b) indicate to the autotest people how to build and run this thing.
I wonder if it requires a stand-alone test. So far I'm testing the guest side via gdb installed in some Linux image. We need to interact with gdb anyway to test guest debugging, so this would just add another instance and some indirection (via serial console or ssh into the guest).

Scripting gdb is perfectly fine, don't know why I thought of the unit tests. These are more suitable to test the guest debugging itself.

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Gleb Natapov

2010-02-14 14:45 UTC #12 lore raw

In reply to Jan Kiszka

Quoted 35 lines · Jan Kiszka

Quoted 34 lines · Gleb Natapov
Quoted 33 lines · Jan Kiszka
Quoted 20 lines · Gleb Natapov
Quoted 19 lines · Jan Kiszka
Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
We intercept #BP while in guest debugging mode. As VM exists due to intercepted exceptions do not necessarily come with valid idt_vectoring, we have to update event_exit_inst_len explicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.
Quoted 3 lines
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).
Quoted 8 lines
Quoted 4 lines
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?
Maybe not at all. I don't think I've tested this scenario on amd so far. Guess it needs some special handling in svm to move rip after the int3 when requesting to inject #BP.

Quoted 7 lines

Quoted 3 lines
This will work for VMX too, no? So may be we should design something that will work for both VMX and SVM before applying patches that make oly VMX work?
VMX used to work, so my patch is actually a regression fix. I bet this was accidentally broken while cleaning up the interrupt handling of VMX.

VMX used to always reexecute instruction.

--
Gleb.

Jan Kiszka

author

2010-02-14 16:37 UTC #13 lore raw

Quoted 36 lines · Gleb Natapov

Quoted 35 lines · Jan Kiszka
Quoted 34 lines · Gleb Natapov
Quoted 33 lines · Jan Kiszka
Quoted 20 lines · Gleb Natapov
Quoted 19 lines · Jan Kiszka
Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
We intercept #BP while in guest debugging mode. As VM exists due to intercepted exceptions do not necessarily come with valid idt_vectoring, we have to update event_exit_inst_len explicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.
Quoted 3 lines
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).
Quoted 8 lines
Quoted 4 lines
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?
Maybe not at all. I don't think I've tested this scenario on amd so far. Guess it needs some special handling in svm to move rip after the int3 when requesting to inject #BP.

Quoted 7 lines

Quoted 6 lines
Quoted 3 lines
This will work for VMX too, no? So may be we should design something that will work for both VMX and SVM before applying patches that make oly VMX work?
VMX used to work, so my patch is actually a regression fix. I bet this was accidentally broken while cleaning up the interrupt handling of VMX.
VMX used to always reexecute instruction.

...since 66fd3f7f90. And that was what broke this guest debugging corner case.

Jan

Gleb Natapov

2010-02-14 16:53 UTC #14 lore raw

Quoted 40 lines · Jan Kiszka

Quoted 39 lines · Gleb Natapov
Quoted 38 lines · Jan Kiszka
Quoted 37 lines · Gleb Natapov
Quoted 25 lines · Jan Kiszka
Quoted 24 lines · Gleb Natapov
Quoted 23 lines · Jan Kiszka
Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
We intercept #BP while in guest debugging mode. As VM exists due to intercepted exceptions do not necessarily come with valid idt_vectoring, we have to update event_exit_inst_len explicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.
Quoted 3 lines
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.
Quoted 8 lines
Quoted 4 lines
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?
Maybe not at all. I don't think I've tested this scenario on amd so far. Guess it needs some special handling in svm to move rip after the int3 when requesting to inject #BP.
This will work for VMX too, no? So may be we should design something that will work for both VMX and SVM before applying patches that make oly VMX work?

Quoted 8 lines

Quoted 4 lines
Quoted 3 lines
VMX used to work, so my patch is actually a regression fix. I bet this was accidentally broken while cleaning up the interrupt handling of VMX.
VMX used to always reexecute instruction.
...since 66fd3f7f90. And that was what broke this guest debugging corner case.

I see. And I see why it worked, but it shouldn't have been working for SVM. I prefer to look for general solution here that works for SVM/VMX.

--
Gleb.

Jan Kiszka

author

2010-02-14 17:12 UTC #15 lore raw

Quoted 44 lines · Gleb Natapov

Quoted 43 lines · Jan Kiszka
Quoted 42 lines · Gleb Natapov
Quoted 41 lines · Jan Kiszka
Quoted 30 lines · Gleb Natapov
Quoted 29 lines · Jan Kiszka
Quoted 28 lines · Gleb Natapov
Quoted 19 lines · Jan Kiszka
Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
We intercept #BP while in guest debugging mode. As VM exists due to intercepted exceptions do not necessarily come with valid idt_vectoring, we have to update event_exit_inst_len explicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.
Quoted 3 lines
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).
Quoted 4 lines
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?
Quoted 7 lines
Quoted 4 lines
Maybe not at all. I don't think I've tested this scenario on amd so far. Guess it needs some special handling in svm to move rip after the int3 when requesting to inject #BP.
This will work for VMX too, no? So may be we should design something that will work for both VMX and SVM before applying patches that make oly VMX work?
VMX used to work, so my patch is actually a regression fix. I bet this was accidentally broken while cleaning up the interrupt handling of VMX.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
VMX used to always reexecute instruction.
...since 66fd3f7f90. And that was what broke this guest debugging corner case.
I see. And I see why it worked, but it shouldn't have been working for SVM. I prefer to look for general solution here that works for SVM/VMX.

I don't see the need to emulate INT3 for the sake of unification. VMX works today (with this patch), and SVM might work without further efforts, at least on modern hosts:

"Software interrupts cannot be properly injected if the processor does not support the NextRIP field, indicated by EDX[3] = 1 as returned by CPUID function 8000_000A. Hypervisor software should emulate the event injection of software interrupts if NextRIP is not supported."

(right below the paragraph I cited before)

I assume, INT3 can be considered as software interrupt as well in this context.

Jan

Gleb Natapov

2010-02-14 17:26 UTC #16 lore raw

Quoted 61 lines · Jan Kiszka

Quoted 44 lines · Gleb Natapov
Quoted 43 lines · Jan Kiszka
Quoted 42 lines · Gleb Natapov
Quoted 41 lines · Jan Kiszka
Quoted 30 lines · Gleb Natapov
Quoted 29 lines · Jan Kiszka
Quoted 28 lines · Gleb Natapov
Quoted 19 lines · Jan Kiszka
Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
We intercept #BP while in guest debugging mode. As VM exists due to intercepted exceptions do not necessarily come with valid idt_vectoring, we have to update event_exit_inst_len explicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.
Quoted 3 lines
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).
Quoted 4 lines
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?
Quoted 7 lines
Quoted 4 lines
Maybe not at all. I don't think I've tested this scenario on amd so far. Guess it needs some special handling in svm to move rip after the int3 when requesting to inject #BP.
This will work for VMX too, no? So may be we should design something that will work for both VMX and SVM before applying patches that make oly VMX work?
VMX used to work, so my patch is actually a regression fix. I bet this was accidentally broken while cleaning up the interrupt handling of VMX.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
VMX used to always reexecute instruction.
...since 66fd3f7f90. And that was what broke this guest debugging corner case.
I see. And I see why it worked, but it shouldn't have been working for SVM. I prefer to look for general solution here that works for SVM/VMX.
I don't see the need to emulate INT3 for the sake of unification. VMX works today (with this patch), and SVM might work without further efforts, at least on modern hosts:
"Software interrupts cannot be properly injected if the processor does not support the NextRIP field, indicated by EDX[3] = 1 as returned by CPUID function 8000_000A. Hypervisor software should emulate the event injection of software interrupts if NextRIP is not supported."

Quoted 5 lines

(right below the paragraph I cited before)
I assume, INT3 can be considered as software interrupt as well in this context.

Lets check if SVM works. I can do that if you tell me how. My concern is that if SVM doesn't work we will write another way to handle this situation that will work on SVM and VMX and then we will have code in VMX that is not needed, but we have to support it.

--
Gleb.

Jan Kiszka

author

2010-02-14 17:50 UTC #17 lore raw

Quoted 61 lines · Gleb Natapov

Quoted 60 lines · Jan Kiszka
Quoted 44 lines · Gleb Natapov
Quoted 43 lines · Jan Kiszka
Quoted 42 lines · Gleb Natapov
Quoted 41 lines · Jan Kiszka
Quoted 30 lines · Gleb Natapov
Quoted 29 lines · Jan Kiszka
Quoted 28 lines · Gleb Natapov
Quoted 19 lines · Jan Kiszka
Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
We intercept #BP while in guest debugging mode. As VM exists due to intercepted exceptions do not necessarily come with valid idt_vectoring, we have to update event_exit_inst_len explicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.
Quoted 3 lines
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).
Quoted 4 lines
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?
Quoted 7 lines
Quoted 4 lines
Maybe not at all. I don't think I've tested this scenario on amd so far. Guess it needs some special handling in svm to move rip after the int3 when requesting to inject #BP.
This will work for VMX too, no? So may be we should design something that will work for both VMX and SVM before applying patches that make oly VMX work?
VMX used to work, so my patch is actually a regression fix. I bet this was accidentally broken while cleaning up the interrupt handling of VMX.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
VMX used to always reexecute instruction.
...since 66fd3f7f90. And that was what broke this guest debugging corner case.
I see. And I see why it worked, but it shouldn't have been working for SVM. I prefer to look for general solution here that works for SVM/VMX.
I don't see the need to emulate INT3 for the sake of unification. VMX works today (with this patch), and SVM might work without further efforts, at least on modern hosts:
"Software interrupts cannot be properly injected if the processor does not support the NextRIP field, indicated by EDX[3] = 1 as returned by CPUID function 8000_000A. Hypervisor software should emulate the event injection of software interrupts if NextRIP is not supported."

Quoted 6 lines

Quoted 5 lines
(right below the paragraph I cited before)
I assume, INT3 can be considered as software interrupt as well in this context.
Lets check if SVM works. I can do that if you tell me how.

- Fire up some Linux guest with gdb installed
- Attach gdb to gdbstub of the VM
- Set a soft breakpoint in guest kernel, ideally where it does not immediately trigger, e.g. on sys_reboot (use grep sys_reboot /proc/kallsyms if you don't have symbols for the guest kernel)
- Start gdb /bin/true in the guest
- run

As gdb sets some automatic breakpoints, this already exercises the reinjection of #BP.

Quoted 4 lines

My concern is
that if SVM doesn't work we will write another way to handle this situation that will work on SVM and VMX and then we will have code in VMX that is not needed, but we have to support it.

I understand your concerns, but I do not share them ATM. I bet we will need some workaround for older SVM, but it will be some SVM-only thing. Anyway, we can wait with this patch a few days until we know more.

Jan

Jan Kiszka

2010-02-15 13:20 UTC #18 lore raw

Quoted 11 lines · Jan Kiszka

Quoted 1 line · Gleb Natapov
Lets check if SVM works. I can do that if you tell me how.
- Fire up some Linux guest with gdb installed
- Attach gdb to gdbstub of the VM
- Set a soft breakpoint in guest kernel, ideally where it does not immediately trigger, e.g. on sys_reboot (use grep sys_reboot /proc/kallsyms if you don't have symbols for the guest kernel)
- Start gdb /bin/true in the guest
- run

Quoted 2 lines

As gdb sets some automatic breakpoints, this already exercises the reinjection of #BP.

I just did this on our primary AMD platform (Embedded Opteron, 13KS EE), and it just worked.

But this is a fairly new processor. Consequently, it reports NextRIP support via cpuid function 0x8000000A. Looking for an older one too.

In the meantime I also browsed a bit more in the manuals, and I don't think stepping over or (what is actually required) into an INT3 will work. We can't step into as the processor clears TF on any event handler entry. And stepping over would cause troubles

a) as an unknown amount of code may run without #DB interception
b) we would fiddle with TF in code that is already under debugger control, thus we would very likely run into conflicts.

Leaves us with tricky INT3 emulation. Sigh.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Gleb Natapov

2010-02-15 13:30 UTC #19 lore raw

Quoted 26 lines · Jan Kiszka

Quoted 11 lines · Jan Kiszka
Quoted 1 line · Gleb Natapov
Lets check if SVM works. I can do that if you tell me how.
- Fire up some Linux guest with gdb installed
- Attach gdb to gdbstub of the VM
- Set a soft breakpoint in guest kernel, ideally where it does not immediately trigger, e.g. on sys_reboot (use grep sys_reboot /proc/kallsyms if you don't have symbols for the guest kernel)
- Start gdb /bin/true in the guest
- run
Quoted 2 lines
As gdb sets some automatic breakpoints, this already exercises the reinjection of #BP.
I just did this on our primary AMD platform (Embedded Opteron, 13KS EE), and it just worked.
But this is a fairly new processor. Consequently, it reports NextRIP support via cpuid function 0x8000000A. Looking for an older one too.
In the meantime I also browsed a bit more in the manuals, and I don't think stepping over or (what is actually required) into an INT3 will work. We can't step into as the processor clears TF on any event handler entry. And stepping over would cause troubles

Quoted 6 lines

a) as an unknown amount of code may run without #DB interception
b) we would fiddle with TF in code that is already under debugger control, thus we would very likely run into conflicts.
Leaves us with tricky INT3 emulation. Sigh.

So the question is do we want to support this kind of debugging on older AMDs. May we don't. Then lets apply your patch for VMX with a comment that explains why we need to save instruction length here (int3 will be reinjected from userspace).

--
Gleb.

Jan Kiszka

2010-02-15 14:25 UTC #20 lore raw

Quoted 25 lines · Gleb Natapov

Quoted 24 lines · Jan Kiszka
Quoted 10 lines · Jan Kiszka
Quoted 1 line · Gleb Natapov
Lets check if SVM works. I can do that if you tell me how.
- Fire up some Linux guest with gdb installed
- Attach gdb to gdbstub of the VM
- Set a soft breakpoint in guest kernel, ideally where it does not immediately trigger, e.g. on sys_reboot (use grep sys_reboot /proc/kallsyms if you don't have symbols for the guest kernel)
- Start gdb /bin/true in the guest
- run
Quoted 2 lines
As gdb sets some automatic breakpoints, this already exercises the reinjection of #BP.
I just did this on our primary AMD platform (Embedded Opteron, 13KS EE), and it just worked.
But this is a fairly new processor. Consequently, it reports NextRIP support via cpuid function 0x8000000A. Looking for an older one too.
In the meantime I also browsed a bit more in the manuals, and I don't think stepping over or (what is actually required) into an INT3 will work. We can't step into as the processor clears TF on any event handler entry. And stepping over would cause troubles

Quoted 8 lines

Quoted 6 lines
a) as an unknown amount of code may run without #DB interception
b) we would fiddle with TF in code that is already under debugger control, thus we would very likely run into conflicts.
Leaves us with tricky INT3 emulation. Sigh.
So the question is do we want to support this kind of debugging on older AMDs. May we don't.

So guests in this special scenario on older AMD hosts remain broken. OK, but then I think we could at least reduce the breakage a bit by emulating typical #DB re-injections:

arch/x86/kvm/svm.c +30 ~~-17~~

index 52f78dd..5f35aaf 100644
@@ -46,6 +46,7 @@ MODULE_LICENSE("GPL");
 #define SVM_FEATURE_NPT  (1 << 0)
 #define SVM_FEATURE_LBRV (1 << 1)
 #define SVM_FEATURE_SVML (1 << 2)
+#define SVM_FEATURE_NRIP (1 << 3)
 #define SVM_FEATURE_PAUSE_FILTER (1 << 10)
 
 #define NESTED_EXIT_HOST	0	/* Exit handled on host level */

@@ -109,6 +110,8 @@ struct vcpu_svm {
 	struct nested_state nested;
 
 	bool nmi_singlestep;
+
+	bool int3_injected;
 };
 
 /* enable NPT for AMD64 and X86 with PAE */

@@ -234,23 +237,6 @@ static void svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
 	vcpu->arch.efer = efer;
 }
 
-static void svm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
-				bool has_error_code, u32 error_code)
-{
-	struct vcpu_svm *svm = to_svm(vcpu);
-
-	/* If we are within a nested VM we'd better #VMEXIT and let the
-	   guest handle the exception */
-	if (nested_svm_check_exception(svm, nr, has_error_code, error_code))
-		return;
-
-	svm->vmcb->control.event_inj = nr
-		| SVM_EVTINJ_VALID
-		| (has_error_code ? SVM_EVTINJ_VALID_ERR : 0)
-		| SVM_EVTINJ_TYPE_EXEPT;
-	svm->vmcb->control.event_inj_err = error_code;
-}
-
 static int is_external_interrupt(u32 info)
 {
 	info &= SVM_EVTINJ_TYPE_MASK | SVM_EVTINJ_VALID;

@@ -296,6 +282,29 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	svm_set_interrupt_shadow(vcpu, 0);
 }
 
+static void svm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
+				bool has_error_code, u32 error_code)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	/* If we are within a nested VM we'd better #VMEXIT and let the
+	   guest handle the exception */
+	if (nested_svm_check_exception(svm, nr, has_error_code, error_code))
+		return;
+
+	if (nr == BP_VECTOR && !svm_has(SVM_FEATURE_NRIP)) {
+		svm->next_rip = kvm_rip_read(&svm->vcpu) + 1;
+		skip_emulated_instruction(&svm->vcpu);
+		svm->int3_injected = true;
+	}
+
+	svm->vmcb->control.event_inj = nr
+		| SVM_EVTINJ_VALID
+		| (has_error_code ? SVM_EVTINJ_VALID_ERR : 0)
+		| SVM_EVTINJ_TYPE_EXEPT;
+	svm->vmcb->control.event_inj_err = error_code;
+}
+
 static int has_svm(void)
 {
 	const char *msg;

@@ -2653,6 +2662,9 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
 		if (is_nested(svm))
 			break;
 		if (kvm_exception_is_soft(vector))
+			if (vector == BP_VECTOR && svm->int3_injected)
+				kvm_rip_write(&svm->vcpu,
+					      kvm_rip_read(&svm->vcpu) - 1);
 			break;
 		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
 			u32 err = svm->vmcb->control.exit_int_info_err;

@@ -2667,6 +2679,7 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
 	default:
 		break;
 	}
+	svm->int3_injected =false;
 }
 
 #ifdef CONFIG_X86_64

i.e. move RIP by one when injecting INT3 and move it back in case we catch a fault. It does not work for unintercepted faults or when int is differently encoded - but it won't crash the guest more frequently, rather less. What do you think?

Quoted 3 lines

Then lets apply your patch for VMX with a comment that explains why we need to save instruction length here (int3 will be reinjected from userspace).

Will do this.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Avi Kivity

2010-02-17 11:11 UTC #21 lore raw

In reply to Gleb Natapov

Quoted 16 lines · Gleb Natapov

Quoted 16 lines
I just did this on our primary AMD platform (Embedded Opteron, 13KS EE), and it just worked.
But this is a fairly new processor. Consequently, it reports NextRIP support via cpuid function 0x8000000A. Looking for an older one too.
In the meantime I also browsed a bit more in the manuals, and I don't think stepping over or (what is actually required) into an INT3 will work. We can't step into as the processor clears TF on any event handler entry. And stepping over would cause troubles
a) as an unknown amount of code may run without #DB interception
b) we would fiddle with TF in code that is already under debugger control, thus we would very likely run into conflicts.

Quoted 6 lines

Quoted 3 lines
Leaves us with tricky INT3 emulation. Sigh.
So the question is do we want to support this kind of debugging on older AMDs. May we don't.

How much older are they?

Signature · 1 line

error compiling committee.c: too many arguments to function

Gleb Natapov

2010-02-17 11:13 UTC #22 lore raw

Quoted 17 lines · Avi Kivity

Quoted 16 lines · Gleb Natapov
Quoted 16 lines
I just did this on our primary AMD platform (Embedded Opteron, 13KS EE), and it just worked.
But this is a fairly new processor. Consequently, it reports NextRIP support via cpuid function 0x8000000A. Looking for an older one too.
In the meantime I also browsed a bit more in the manuals, and I don't think stepping over or (what is actually required) into an INT3 will work. We can't step into as the processor clears TF on any event handler entry. And stepping over would cause troubles
a) as an unknown amount of code may run without #DB interception
b) we would fiddle with TF in code that is already under debugger control, thus we would very likely run into conflicts.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Leaves us with tricky INT3 emulation. Sigh.
So the question is do we want to support this kind of debugging on older AMDs. May we don't.
How much older are they?

Actually I am not sure new AMDs support this correctly. Need one to run tests. GDB is not a good test case, it is too smart.

--
Gleb.

Jan Kiszka

2010-02-17 11:24 UTC #23 lore raw

Quoted 17 lines · Gleb Natapov

Quoted 16 lines · Avi Kivity
Quoted 15 lines · Gleb Natapov
Quoted 15 lines
I just did this on our primary AMD platform (Embedded Opteron, 13KS EE), and it just worked.
But this is a fairly new processor. Consequently, it reports NextRIP support via cpuid function 0x8000000A. Looking for an older one too.
In the meantime I also browsed a bit more in the manuals, and I don't think stepping over or (what is actually required) into an INT3 will work. We can't step into as the processor clears TF on any event handler entry. And stepping over would cause troubles
a) as an unknown amount of code may run without #DB interception
b) we would fiddle with TF in code that is already under debugger control, thus we would very likely run into conflicts.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Leaves us with tricky INT3 emulation. Sigh.
So the question is do we want to support this kind of debugging on older AMDs. May we don't.
How much older are they?
Actually I am not sure new AMDs support this correctly. Need one to run tests. GDB is not a good test case, it is too smart.

It works well - and gdb is far from being "smart": one byte off the expected INT3 address, and everything falls apart. That's what the VMX bug demonstrated.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Gleb Natapov

2010-02-17 12:39 UTC #24 lore raw

Quoted 22 lines · Jan Kiszka

Quoted 21 lines · Gleb Natapov
Quoted 20 lines · Avi Kivity
Quoted 19 lines · Gleb Natapov
Quoted 11 lines
I just did this on our primary AMD platform (Embedded Opteron, 13KS EE), and it just worked.
But this is a fairly new processor. Consequently, it reports NextRIP support via cpuid function 0x8000000A. Looking for an older one too.
In the meantime I also browsed a bit more in the manuals, and I don't think stepping over or (what is actually required) into an INT3 will work. We can't step into as the processor clears TF on any event handler entry. And stepping over would cause troubles
Quoted 6 lines
a) as an unknown amount of code may run without #DB interception
b) we would fiddle with TF in code that is already under debugger control, thus we would very likely run into conflicts.
Leaves us with tricky INT3 emulation. Sigh.
So the question is do we want to support this kind of debugging on older AMDs. May we don't.

Quoted 9 lines

Quoted 4 lines
Quoted 2 lines
How much older are they?
Actually I am not sure new AMDs support this correctly. Need one to run tests. GDB is not a good test case, it is too smart.
It works well - and gdb is far from being "smart": one byte off the expected INT3 address, and everything falls apart. That's what the VMX bug demonstrated.

Simple test on AMD shows the one byte off doesn't matter for GDB, at least as long as this byte still belong to the same instruction or may be same line of source code. On VMX something else happens. I can't reproduce problem on master with VMX since event_exit_inst_len is always 1 when #DB is reinjected. May be in your test we are much more then 1 byte off on VMX?

--
Gleb.

Gleb Natapov

2010-02-17 10:55 UTC #25 lore raw

In reply to Jan Kiszka

Quoted 12 lines · Jan Kiszka

Quoted 11 lines · Jan Kiszka
Quoted 1 line · Gleb Natapov
Lets check if SVM works. I can do that if you tell me how.
- Fire up some Linux guest with gdb installed
- Attach gdb to gdbstub of the VM
- Set a soft breakpoint in guest kernel, ideally where it does not immediately trigger, e.g. on sys_reboot (use grep sys_reboot /proc/kallsyms if you don't have symbols for the guest kernel)
- Start gdb /bin/true in the guest
- run

Quoted 6 lines

Quoted 2 lines
As gdb sets some automatic breakpoints, this already exercises the reinjection of #BP.
I just did this on our primary AMD platform (Embedded Opteron, 13KS EE), and it just worked.

I tested it on processor without NextRIP and your test case works there too, but it shouldn't have, so I looked deeper into that and what I see is that GDB outsmart us. It doesn't matter if we inject event before int3 inserted by GDB or after it GDB correctly finds breakpoint that triggered and restart instruction correctly. I assume it doesn't use exact match between rip where int3 was inserted and where exceptions triggers. But if I run program below on latest kernel which prints rip where #DB was delivered in dmesg I get different results with and without external breakpoint inserted.

int main(int argc, char **argv)

{
        asm("int3");
        return 0;
}

Quoted 8 lines

But this is a fairly new processor. Consequently, it reports NextRIP support via cpuid function 0x8000000A. Looking for an older one too.
In the meantime I also browsed a bit more in the manuals, and I don't think stepping over or (what is actually required) into an INT3 will work. We can't step into as the processor clears TF on any event handler entry. And stepping over would cause troubles

Quoted 11 lines

a) as an unknown amount of code may run without #DB interception
b) we would fiddle with TF in code that is already under debugger control, thus we would very likely run into conflicts.
Leaves us with tricky INT3 emulation. Sigh.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

--
Gleb.

Jan Kiszka

2010-02-17 11:32 UTC #26 lore raw

Quoted 17 lines · Gleb Natapov

Quoted 11 lines · Jan Kiszka
Quoted 10 lines · Jan Kiszka
Quoted 1 line · Gleb Natapov
Lets check if SVM works. I can do that if you tell me how.
- Fire up some Linux guest with gdb installed
- Attach gdb to gdbstub of the VM
- Set a soft breakpoint in guest kernel, ideally where it does not immediately trigger, e.g. on sys_reboot (use grep sys_reboot /proc/kallsyms if you don't have symbols for the guest kernel)
- Start gdb /bin/true in the guest
- run
Quoted 5 lines
Quoted 2 lines
As gdb sets some automatic breakpoints, this already exercises the reinjection of #BP.
I just did this on our primary AMD platform (Embedded Opteron, 13KS EE), and it just worked.

Quoted 7 lines

I tested it on processor without NextRIP and your test case works there too, but it shouldn't have, so I looked deeper into that and what I see is that GDB outsmart us. It doesn't matter if we inject event before int3 inserted by GDB or after it GDB correctly finds breakpoint that triggered and restart instruction correctly. I assume it doesn't use exact match between rip where int3 was inserted and where exceptions triggers.

At latest when you have two successive breakpoints on single-byte instructions, gdb will reach its limits (for it failed earlier, BTW). And other debuggers under other OSes may become unhappy as well.

Quoted 3 lines

But if I run program below on latest kernel which prints rip where #DB was delivered in dmesg I get different results with and without external breakpoint inserted.

Does applying v2 of my patch corrects the picture?

Quoted 7 lines

int main(int argc, char **argv)
{
        asm("int3");
        return 0;
}

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Gleb Natapov

2010-02-17 13:03 UTC #27 lore raw

Quoted 26 lines · Jan Kiszka

Quoted 17 lines · Gleb Natapov
Quoted 11 lines · Jan Kiszka
Quoted 10 lines · Jan Kiszka
Quoted 1 line · Gleb Natapov
Lets check if SVM works. I can do that if you tell me how.
- Fire up some Linux guest with gdb installed
- Attach gdb to gdbstub of the VM
- Set a soft breakpoint in guest kernel, ideally where it does not immediately trigger, e.g. on sys_reboot (use grep sys_reboot /proc/kallsyms if you don't have symbols for the guest kernel)
- Start gdb /bin/true in the guest
- run
Quoted 5 lines
Quoted 2 lines
As gdb sets some automatic breakpoints, this already exercises the reinjection of #BP.
I just did this on our primary AMD platform (Embedded Opteron, 13KS EE), and it just worked.
Quoted 7 lines
I tested it on processor without NextRIP and your test case works there too, but it shouldn't have, so I looked deeper into that and what I see is that GDB outsmart us. It doesn't matter if we inject event before int3 inserted by GDB or after it GDB correctly finds breakpoint that triggered and restart instruction correctly. I assume it doesn't use exact match between rip where int3 was inserted and where exceptions triggers.

Quoted 3 lines

At latest when you have two successive breakpoints on single-byte instructions, gdb will reach its limits (for it failed earlier, BTW). And other debuggers under other OSes may become unhappy as well.

Yes, and that is why I am saying checking with GDB is not a good test. GDB may work, but it doesn't mean injection works correctly. It took me some time to write test that finally confused gdb. It was like this:

1: int main(int argc, char **argv)
2: {
3: if (argc == 1)
4: goto a;
5: asm("cmc");
6: a:
7: asm("cmc");
8: return 0;
9: }

If you set breakpoint on lines 5 and 7 when breakpoint triggers GDB thinks it is on line 5.

So can you run int3 test below on master on AMD with NextRIP support? I doubt the result will be correct.

Quoted 7 lines

Quoted 4 lines
But if I run program below on latest kernel which prints rip where #DB was delivered in dmesg I get different results with and without external breakpoint inserted.
Does applying v2 of my patch corrects the picture?

Of course, since it now injects #DB at correct address. If exception will happen during #DB processing thins will go wrong, but we can do only so much on broken SVM without emulating int3 in software.

Quoted 7 lines

Quoted 7 lines
int main(int argc, char **argv)
{
        asm("int3");
        return 0;
}

--
Gleb.

Jan Kiszka

2010-02-17 15:13 UTC #28 lore raw

Quoted 43 lines · Gleb Natapov

Quoted 25 lines · Jan Kiszka
Quoted 17 lines · Gleb Natapov
Quoted 11 lines · Jan Kiszka
Quoted 10 lines · Jan Kiszka
Quoted 1 line · Gleb Natapov
Lets check if SVM works. I can do that if you tell me how.
- Fire up some Linux guest with gdb installed
- Attach gdb to gdbstub of the VM
- Set a soft breakpoint in guest kernel, ideally where it does not immediately trigger, e.g. on sys_reboot (use grep sys_reboot /proc/kallsyms if you don't have symbols for the guest kernel)
- Start gdb /bin/true in the guest
- run
Quoted 5 lines
Quoted 2 lines
As gdb sets some automatic breakpoints, this already exercises the reinjection of #BP.
I just did this on our primary AMD platform (Embedded Opteron, 13KS EE), and it just worked.
Quoted 7 lines
I tested it on processor without NextRIP and your test case works there too, but it shouldn't have, so I looked deeper into that and what I see is that GDB outsmart us. It doesn't matter if we inject event before int3 inserted by GDB or after it GDB correctly finds breakpoint that triggered and restart instruction correctly. I assume it doesn't use exact match between rip where int3 was inserted and where exceptions triggers.
Quoted 3 lines
At latest when you have two successive breakpoints on single-byte instructions, gdb will reach its limits (for it failed earlier, BTW). And other debuggers under other OSes may become unhappy as well.
Yes, and that is why I am saying checking with GDB is not a good test. GDB may work, but it doesn't mean injection works correctly. It took me some time to write test that finally confused gdb. It was like this:
1: int main(int argc, char **argv)
2: {
3: if (argc == 1)
4: goto a;
5: asm("cmc");
6: a:
7: asm("cmc");
8: return 0;
9: }

Quoted 5 lines

If you set breakpoint on lines 5 and 7 when breakpoint triggers GDB thinks it is on line 5.
So can you run int3 test below on master on AMD with NextRIP support? I doubt the result will be correct.

If you meant your test above: Works out of the box with unpatched kvm on modern AMD CPUs, ie. gdb always stops at line 7 even if host debugging is active.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Gleb Natapov

2010-02-17 16:12 UTC #29 lore raw

Quoted 47 lines · Jan Kiszka

Quoted 46 lines · Gleb Natapov
Quoted 25 lines · Jan Kiszka
Quoted 17 lines · Gleb Natapov
Quoted 11 lines · Jan Kiszka
Quoted 10 lines · Jan Kiszka
Quoted 1 line · Gleb Natapov
Lets check if SVM works. I can do that if you tell me how.
- Fire up some Linux guest with gdb installed
- Attach gdb to gdbstub of the VM
- Set a soft breakpoint in guest kernel, ideally where it does not immediately trigger, e.g. on sys_reboot (use grep sys_reboot /proc/kallsyms if you don't have symbols for the guest kernel)
- Start gdb /bin/true in the guest
- run
Quoted 5 lines
Quoted 2 lines
As gdb sets some automatic breakpoints, this already exercises the reinjection of #BP.
I just did this on our primary AMD platform (Embedded Opteron, 13KS EE), and it just worked.
Quoted 7 lines
I tested it on processor without NextRIP and your test case works there too, but it shouldn't have, so I looked deeper into that and what I see is that GDB outsmart us. It doesn't matter if we inject event before int3 inserted by GDB or after it GDB correctly finds breakpoint that triggered and restart instruction correctly. I assume it doesn't use exact match between rip where int3 was inserted and where exceptions triggers.
Quoted 3 lines
At latest when you have two successive breakpoints on single-byte instructions, gdb will reach its limits (for it failed earlier, BTW). And other debuggers under other OSes may become unhappy as well.
Yes, and that is why I am saying checking with GDB is not a good test. GDB may work, but it doesn't mean injection works correctly. It took me some time to write test that finally confused gdb. It was like this:
1: int main(int argc, char **argv)
2: {
3: if (argc == 1)
4: goto a;
5: asm("cmc");
6: a:
7: asm("cmc");
8: return 0;
9: }
If you set breakpoint on lines 5 and 7 when breakpoint triggers GDB thinks it is on line 5.

Quoted 7 lines

Quoted 2 lines
So can you run int3 test below on master on AMD with NextRIP support? I doubt the result will be correct.
If you meant your test above: Works out of the box with unpatched kvm on modern AMD CPUs, ie. gdb always stops at line 7 even if host debugging is active.

I meant test that does asm("int3") and see that rip it reports with and without host debugging active is the same and points after int3. But I guess if program above works correctly int3 test should work too. Thanks.

--
Gleb.

Gleb Natapov

2010-02-16 11:20 UTC #30 lore raw

In reply to Jan Kiszka

Quoted 4 lines · Jan Kiszka

Quoted 1 line
Lets check if SVM works. I can do that if you tell me how.
- Fire up some Linux guest with gdb installed
- Attach gdb to gdbstub of the VM

I get "Remote 'g' packet reply is too long". My guest and host are 64 bit.

Quoted 14 lines

- Set a soft breakpoint in guest kernel, ideally where it does not immediately trigger, e.g. on sys_reboot (use grep sys_reboot /proc/kallsyms if you don't have symbols for the guest kernel)
- Start gdb /bin/true in the guest
- run
As gdb sets some automatic breakpoints, this already exercises the reinjection of #BP.
Quoted 4 lines
My concern is
that if SVM doesn't work we will write another way to handle this situation that will work on SVM and VMX and then we will have code in VMX that is not needed, but we have to support it.

Quoted 6 lines

I understand your concerns, but I do not share them ATM. I bet we will need some workaround for older SVM, but it will be some SVM-only thing. Anyway, we can wait with this patch a few days until we know more.
Jan

--
Gleb.

Gleb Natapov

2010-02-16 11:26 UTC #31 lore raw

Quoted 8 lines · Gleb Natapov

Quoted 4 lines · Jan Kiszka
Quoted 1 line
Lets check if SVM works. I can do that if you tell me how.
- Fire up some Linux guest with gdb installed
- Attach gdb to gdbstub of the VM
I get "Remote 'g' packet reply is too long". My guest and host are 64 bit.

May fault. Provided wrong file to gdb.

Quoted 23 lines

Quoted 14 lines
- Set a soft breakpoint in guest kernel, ideally where it does not immediately trigger, e.g. on sys_reboot (use grep sys_reboot /proc/kallsyms if you don't have symbols for the guest kernel)
- Start gdb /bin/true in the guest
- run
As gdb sets some automatic breakpoints, this already exercises the reinjection of #BP.
Quoted 4 lines
My concern is
that if SVM doesn't work we will write another way to handle this situation that will work on SVM and VMX and then we will have code in VMX that is not needed, but we have to support it.
Quoted 6 lines
I understand your concerns, but I do not share them ATM. I bet we will need some workaround for older SVM, but it will be some SVM-only thing. Anyway, we can wait with this patch a few days until we know more.
Jan

Quoted 6 lines

--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Gleb.

Avi Kivity

2010-02-14 12:27 UTC #32 lore raw

In reply to Gleb Natapov

Quoted 11 lines · Gleb Natapov

Quoted 11 lines
Quoted 5 lines
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).

Quoted 10 lines

Quoted 5 lines
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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2010-02-14 12:39 UTC #33 lore raw

Quoted 18 lines · Avi Kivity

Quoted 17 lines · Gleb Natapov
Quoted 17 lines
Quoted 6 lines
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.

Quoted 9 lines

Quoted 5 lines
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):

arch/x86/kvm/svm.c +25 -2

index 52f78dd..450c1e7 100644
@@ -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

Gleb Natapov

2010-02-14 12:43 UTC #34 lore raw

Quoted 25 lines · Jan Kiszka

Quoted 24 lines · Avi Kivity
Quoted 22 lines · Gleb Natapov
Quoted 12 lines
Quoted 6 lines
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).
Quoted 5 lines
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?

Quoted 8 lines

Quoted 3 lines
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... ]

I can't find nothing newer then that. What is the link?

--
Gleb.

Avi Kivity

2010-02-14 12:47 UTC #35 lore raw

Quoted 8 lines · Gleb Natapov

Quoted 6 lines
Nice.
[ /me goes updating his manual - September 07... ]
I can't find nothing newer then that. What is the link?

http://www.amd.com/us-en/Processors/DevelopWithAMD/0,,30_2252_875_7044,00.html

Signature · 1 line

error compiling committee.c: too many arguments to function

Gleb Natapov

2010-02-14 12:53 UTC #36 lore raw

Quoted 10 lines · Avi Kivity

Quoted 6 lines · Gleb Natapov
Quoted 5 lines
Nice.
[ /me goes updating his manual - September 07... ]
I can't find nothing newer then that. What is the link?
http://www.amd.com/us-en/Processors/DevelopWithAMD/0,,30_2252_875_7044,00.html

It looks like SVM's nRIP is not the same as VMX's instruction length that is used during soft event injection. nRIP is VMEXIT things and we need VMENTRY thing.

--
Gleb.

Jan Kiszka

author

2010-02-14 13:24 UTC #37 lore raw

In reply to Avi Kivity

Quoted 4 lines · Avi Kivity

Quoted 3 lines · Gleb Natapov
Quoted 3 lines
Nice.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
[ /me goes updating his manual - September 07... ]
I can't find nothing newer then that. What is the link?
http://www.amd.com/us-en/Processors/DevelopWithAMD/0,,30_2252_875_7044,00.html

For the records, it's

http://developer.amd.com/documentation/guides/Pages/default.aspx

and then

http://support.amd.com/us/Processor_TechDocs/24593.pdf

(Your link still points to the previous revision.)

nRIP is actually useless for our problem. Either SVM has built-in magic to push the RIP after the INT3 on the stack or we need a workaround. I bet on the latter as our use case may not have been exercised that often before (if at all).

Jan

Jan Kiszka

author

2010-02-14 13:29 UTC #38 lore raw

Quoted 18 lines · Jan Kiszka

Quoted 10 lines · Avi Kivity
Quoted 7 lines · Gleb Natapov
Quoted 5 lines
Nice.
[ /me goes updating his manual - September 07... ]
I can't find nothing newer then that. What is the link?
http://www.amd.com/us-en/Processors/DevelopWithAMD/0,,30_2252_875_7044,00.html
For the records, it's
http://developer.amd.com/documentation/guides/Pages/default.aspx
and then

Quoted 9 lines

http://support.amd.com/us/Processor_TechDocs/24593.pdf
(Your link still points to the previous revision.)
nRIP is actually useless for our problem. Either SVM has built-in magic to push the RIP after the INT3 on the stack or we need a workaround. I bet on the latter as our use case may not have been exercised that often before (if at all).

On the other hand:

"Injecting an exception (TYPE = 3) with vectors 3 or 4 behaves like a trap raised by INT3 and INTO instructions, respectively, in which case the processor checks the DPL of the IDT descriptor before dispatching to the handler."

Which /might/ also be read that not only the privilege checks are applied, but also the original trap characteristics. And that case I would send kudos to AMD. Will test tomorrow.

Jan