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

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

[Jan Kiszka]

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

From: Jan Kiszka <jan.kiszka@siemens.com>

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 <jan.kiszka@siemens.com>
---

Please consider for stable, this fixes guest debugging scenarios.

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

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index f82b072..e9f64e8 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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;


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

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

[Gleb Natapov]

On Sat, Feb 13, 2010 at 10:31:12AM +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 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?

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Please consider for stable, this fixes guest debugging scenarios.
> 
>  arch/x86/kvm/vmx.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index f82b072..e9f64e8 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -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.

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

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Sat, Feb 13, 2010 at 10:31:12AM +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> 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?

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

> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> Please consider for stable, this fixes guest debugging scenarios.
>>
>>  arch/x86/kvm/vmx.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index f82b072..e9f64e8 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -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.



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

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

[Gleb Natapov]

On Sun, Feb 14, 2010 at 11:26:31AM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Sat, Feb 13, 2010 at 10:31:12AM +0100, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> 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?
> 
> 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?

--
			Gleb.

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

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Sun, Feb 14, 2010 at 11:26:31AM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Sat, Feb 13, 2010 at 10:31:12AM +0100, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> 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?
>> 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?
> 

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


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

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

[Gleb Natapov]

On Sun, Feb 14, 2010 at 11:47:58AM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Sun, Feb 14, 2010 at 11:26:31AM +0100, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Sat, Feb 13, 2010 at 10:31:12AM +0100, Jan Kiszka wrote:
> >>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>
> >>>> 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?
> >> 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?
> > 
> 
> 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.

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

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Sun, Feb 14, 2010 at 11:47:58AM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Sun, Feb 14, 2010 at 11:26:31AM +0100, Jan Kiszka wrote:
>>>> Gleb Natapov wrote:
>>>>> On Sat, Feb 13, 2010 at 10:31:12AM +0100, Jan Kiszka wrote:
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> 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?
>>>> 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?
>>>
>> 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.

> 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...

> 
> 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


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

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

[Avi Kivity]

On 02/14/2010 01:39 PM, Jan Kiszka wrote:
>> 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.

-- 
error compiling committee.c: too many arguments to function

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

[Jan Kiszka]

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

Avi Kivity wrote:
> On 02/14/2010 01:39 PM, Jan Kiszka wrote:
>>> 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.
> 

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


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

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

[Avi Kivity]

On 02/14/2010 06:38 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 02/14/2010 01:39 PM, Jan Kiszka wrote:
>>      
>>>> 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.
>>
>>      
> 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.

-- 
error compiling committee.c: too many arguments to function

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

[Jan Kiszka]

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

Avi Kivity wrote:
> On 02/14/2010 06:38 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>   
>>> On 02/14/2010 01:39 PM, Jan Kiszka wrote:
>>>     
>>>>> 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.
>>>
>>>      
>> 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.

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


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

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

[Avi Kivity]

On 02/14/2010 07:06 PM, Jan Kiszka wrote:
>
>> 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.

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

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

[Gleb Natapov]

On Sun, Feb 14, 2010 at 12:39:14PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Sun, Feb 14, 2010 at 11:47:58AM +0100, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Sun, Feb 14, 2010 at 11:26:31AM +0100, Jan Kiszka wrote:
> >>>> Gleb Natapov wrote:
> >>>>> On Sat, Feb 13, 2010 at 10:31:12AM +0100, Jan Kiszka wrote:
> >>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>
> >>>>>> 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?
> >>>> 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?
> >>>
> >> 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.
> 
VMX used to always reexecute instruction.

--
			Gleb.

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

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Sun, Feb 14, 2010 at 12:39:14PM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Sun, Feb 14, 2010 at 11:47:58AM +0100, Jan Kiszka wrote:
>>>> Gleb Natapov wrote:
>>>>> On Sun, Feb 14, 2010 at 11:26:31AM +0100, Jan Kiszka wrote:
>>>>>> Gleb Natapov wrote:
>>>>>>> On Sat, Feb 13, 2010 at 10:31:12AM +0100, Jan Kiszka wrote:
>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>
>>>>>>>> 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?
>>>>>> 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?
>>>>>
>>>> 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.
>>
> VMX used to always reexecute instruction.

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

Jan


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

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

[Gleb Natapov]

On Sun, Feb 14, 2010 at 05:37:39PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Sun, Feb 14, 2010 at 12:39:14PM +0100, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Sun, Feb 14, 2010 at 11:47:58AM +0100, Jan Kiszka wrote:
> >>>> Gleb Natapov wrote:
> >>>>> On Sun, Feb 14, 2010 at 11:26:31AM +0100, Jan Kiszka wrote:
> >>>>>> Gleb Natapov wrote:
> >>>>>>> On Sat, Feb 13, 2010 at 10:31:12AM +0100, Jan Kiszka wrote:
> >>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>>>
> >>>>>>>> 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?
> >>>>>> 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?
> >>>>>
> >>>> 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.
> >>
> > 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.

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

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Sun, Feb 14, 2010 at 05:37:39PM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Sun, Feb 14, 2010 at 12:39:14PM +0100, Jan Kiszka wrote:
>>>> Gleb Natapov wrote:
>>>>> On Sun, Feb 14, 2010 at 11:47:58AM +0100, Jan Kiszka wrote:
>>>>>> Gleb Natapov wrote:
>>>>>>> On Sun, Feb 14, 2010 at 11:26:31AM +0100, Jan Kiszka wrote:
>>>>>>>> Gleb Natapov wrote:
>>>>>>>>> On Sat, Feb 13, 2010 at 10:31:12AM +0100, Jan Kiszka wrote:
>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>>
>>>>>>>>>> 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?
>>>>>>>> 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?
>>>>>>>
>>>>>> 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.
>>>>
>>> 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


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

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

[Gleb Natapov]

On Sun, Feb 14, 2010 at 06:06:31PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Sun, Feb 14, 2010 at 05:37:39PM +0100, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Sun, Feb 14, 2010 at 12:39:14PM +0100, Jan Kiszka wrote:
> >>>> Gleb Natapov wrote:
> >>>>> On Sun, Feb 14, 2010 at 11:47:58AM +0100, Jan Kiszka wrote:
> >>>>>> Gleb Natapov wrote:
> >>>>>>> On Sun, Feb 14, 2010 at 11:26:31AM +0100, Jan Kiszka wrote:
> >>>>>>>> Gleb Natapov wrote:
> >>>>>>>>> On Sat, Feb 13, 2010 at 10:31:12AM +0100, Jan Kiszka wrote:
> >>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>>>>>
> >>>>>>>>>> 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?
> >>>>>>>> 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?
> >>>>>>>
> >>>>>> 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.
> >>>>
> >>> 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.
> 
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.

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

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Sun, Feb 14, 2010 at 06:06:31PM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Sun, Feb 14, 2010 at 05:37:39PM +0100, Jan Kiszka wrote:
>>>> Gleb Natapov wrote:
>>>>> On Sun, Feb 14, 2010 at 12:39:14PM +0100, Jan Kiszka wrote:
>>>>>> Gleb Natapov wrote:
>>>>>>> On Sun, Feb 14, 2010 at 11:47:58AM +0100, Jan Kiszka wrote:
>>>>>>>> Gleb Natapov wrote:
>>>>>>>>> On Sun, Feb 14, 2010 at 11:26:31AM +0100, Jan Kiszka wrote:
>>>>>>>>>> Gleb Natapov wrote:
>>>>>>>>>>> On Sat, Feb 13, 2010 at 10:31:12AM +0100, Jan Kiszka wrote:
>>>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>>>>
>>>>>>>>>>>> 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?
>>>>>>>>>> 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?
>>>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>> 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.
>>
> 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.

> 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


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

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

[Jan Kiszka]

Jan Kiszka wrote:
> Gleb Natapov wrote:
>> 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.

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

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

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

[Gleb Natapov]

On Mon, Feb 15, 2010 at 02:20:31PM +0100, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Gleb Natapov wrote:
> >> 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.
> 
> 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.
> 
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.

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

[Jan Kiszka]

Gleb Natapov wrote:
> On Mon, Feb 15, 2010 at 02:20:31PM +0100, Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Gleb Natapov wrote:
>>>> 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.
>> 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.
>>
> 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:

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 52f78dd..5f35aaf 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -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?

> 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

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

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

[Avi Kivity]

On 02/15/2010 03:30 PM, Gleb Natapov wrote:
>
>> 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.
>>
>>      
> 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?

-- 
error compiling committee.c: too many arguments to function

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

[Gleb Natapov]

On Wed, Feb 17, 2010 at 01:11:36PM +0200, Avi Kivity wrote:
> On 02/15/2010 03:30 PM, Gleb Natapov wrote:
> >
> >>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.
> >>
> >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.

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

[Jan Kiszka]

Gleb Natapov wrote:
> On Wed, Feb 17, 2010 at 01:11:36PM +0200, Avi Kivity wrote:
>> On 02/15/2010 03:30 PM, Gleb Natapov wrote:
>>>> 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.
>>>>
>>> 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

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

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

[Gleb Natapov]

On Wed, Feb 17, 2010 at 12:24:19PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Feb 17, 2010 at 01:11:36PM +0200, Avi Kivity wrote:
> >> On 02/15/2010 03:30 PM, Gleb Natapov wrote:
> >>>> 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.
> >>>>
> >>> 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.
> 
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.

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

[Gleb Natapov]

On Mon, Feb 15, 2010 at 02:20:31PM +0100, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Gleb Natapov wrote:
> >> 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.
> 
> 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;
}

> 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
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

--
			Gleb.

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

[Jan Kiszka]

Gleb Natapov wrote:
> On Mon, Feb 15, 2010 at 02:20:31PM +0100, Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Gleb Natapov wrote:
>>>> 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.
>> 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.

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.

> 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?

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

Jan

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

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

[Gleb Natapov]

On Wed, Feb 17, 2010 at 12:32:05PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Mon, Feb 15, 2010 at 02:20:31PM +0100, Jan Kiszka wrote:
> >> Jan Kiszka wrote:
> >>> Gleb Natapov wrote:
> >>>> 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.
> >> 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.
> 
> 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.

> 
> > 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.

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

--
			Gleb.

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

[Jan Kiszka]

Gleb Natapov wrote:
> On Wed, Feb 17, 2010 at 12:32:05PM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Mon, Feb 15, 2010 at 02:20:31PM +0100, Jan Kiszka wrote:
>>>> Jan Kiszka wrote:
>>>>> Gleb Natapov wrote:
>>>>>> 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.
>>>> 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.
>> 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.

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

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

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

[Gleb Natapov]

On Wed, Feb 17, 2010 at 04:13:11PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Feb 17, 2010 at 12:32:05PM +0100, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Mon, Feb 15, 2010 at 02:20:31PM +0100, Jan Kiszka wrote:
> >>>> Jan Kiszka wrote:
> >>>>> Gleb Natapov wrote:
> >>>>>> 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.
> >>>> 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.
> >> 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.
> 
> 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.

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

[Gleb Natapov]

On Sun, Feb 14, 2010 at 06:49:23PM +0100, Jan Kiszka wrote:
> > 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.

> - 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.
> 
> > 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
> 



--
			Gleb.

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

[Gleb Natapov]

On Tue, Feb 16, 2010 at 01:20:21PM +0200, Gleb Natapov wrote:
> On Sun, Feb 14, 2010 at 06:49:23PM +0100, Jan Kiszka wrote:
> > > 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.

> > - 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.
> > 
> > > 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
> > 
> 
> 
> 
> --
> 			Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

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

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

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

[Jan Kiszka]

[-- 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 --]

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

[Gleb Natapov]

On Sun, Feb 14, 2010 at 01:39:41PM +0100, Jan Kiszka wrote:
> 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... ]
> 
I can't find nothing newer then that. What is the link?

--
			Gleb.

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

[Avi Kivity]

On 02/14/2010 02:43 PM, Gleb Natapov wrote:
>>
>> 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

-- 
error compiling committee.c: too many arguments to function

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

[Gleb Natapov]

On Sun, Feb 14, 2010 at 02:47:39PM +0200, Avi Kivity wrote:
> On 02/14/2010 02:43 PM, Gleb Natapov wrote:
> >>
> >>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.

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

[Jan Kiszka]

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

Avi Kivity wrote:
> On 02/14/2010 02:43 PM, Gleb Natapov wrote:
>>>
>>> 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

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



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

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

[Jan Kiszka]

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

Jan Kiszka wrote:
> Avi Kivity wrote:
>> On 02/14/2010 02:43 PM, Gleb Natapov wrote:
>>>> 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
> 
> 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


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