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

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

[Jan Kiszka]

We intercept #BP while in guest debugging mode. As VM exits 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>
---

Changes in v2:
 - added comments

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

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index f82b072..14873b9 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2775,6 +2775,12 @@ static int handle_rmode_exception(struct kvm_vcpu *vcpu,
 		kvm_queue_exception(vcpu, vec);
 		return 1;
 	case BP_VECTOR:
+		/*
+		 * Update instruction length as we may reinject the exception
+		 * from user space while in guest debugging mode.
+		 */
+		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 +2903,13 @@ static int handle_exception(struct kvm_vcpu *vcpu)
 		kvm_run->debug.arch.dr7 = vmcs_readl(GUEST_DR7);
 		/* fall through */
 	case BP_VECTOR:
+		/*
+		 * Update instruction length as we may reinject #BP from
+		 * user space while in guest debugging mode. Reading it for
+		 * #DB as well causes no harm, it is not used in that case.
+		 */
+		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;

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

[Gleb Natapov]

On Mon, Feb 15, 2010 at 03:53:04PM +0100, Jan Kiszka wrote:
> We intercept #BP while in guest debugging mode. As VM exits 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.
> 
Thinking about it some more. Why do we exit to userspace at all if we
intercept wrong #DB? It seams to me not wise to have ability to inject
exceptions from userspace. Exceptions generation mechanism is a part of
CPU and we shouldn't outsource part of CPU functionality to userspace.

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Changes in v2:
>  - added comments
> 
>  arch/x86/kvm/vmx.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index f82b072..14873b9 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2775,6 +2775,12 @@ static int handle_rmode_exception(struct kvm_vcpu *vcpu,
>  		kvm_queue_exception(vcpu, vec);
>  		return 1;
>  	case BP_VECTOR:
> +		/*
> +		 * Update instruction length as we may reinject the exception
> +		 * from user space while in guest debugging mode.
> +		 */
> +		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 +2903,13 @@ static int handle_exception(struct kvm_vcpu *vcpu)
>  		kvm_run->debug.arch.dr7 = vmcs_readl(GUEST_DR7);
>  		/* fall through */
>  	case BP_VECTOR:
> +		/*
> +		 * Update instruction length as we may reinject #BP from
> +		 * user space while in guest debugging mode. Reading it for
> +		 * #DB as well causes no harm, it is not used in that case.
> +		 */
> +		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 v2] KVM: VMX: Update instruction length on intercepted BP

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Mon, Feb 15, 2010 at 03:53:04PM +0100, Jan Kiszka wrote:
>> We intercept #BP while in guest debugging mode. As VM exits 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.
>>
> Thinking about it some more. Why do we exit to userspace at all if we
> intercept wrong #DB? It seams to me not wise to have ability to inject
> exceptions from userspace. Exceptions generation mechanism is a part of
> CPU and we shouldn't outsource part of CPU functionality to userspace.

The guest debugging API was design to avoid maintaining a "countless"
number of breakpoints in kernel space and instead chose to loop over
user space to decide about #DB & #BP. So this part is required even if
we start thinking about an alternative interface in the future.

Jan

> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> Changes in v2:
>>  - added comments
>>
>>  arch/x86/kvm/vmx.c |   13 +++++++++++++
>>  1 files changed, 13 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index f82b072..14873b9 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2775,6 +2775,12 @@ static int handle_rmode_exception(struct kvm_vcpu *vcpu,
>>  		kvm_queue_exception(vcpu, vec);
>>  		return 1;
>>  	case BP_VECTOR:
>> +		/*
>> +		 * Update instruction length as we may reinject the exception
>> +		 * from user space while in guest debugging mode.
>> +		 */
>> +		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 +2903,13 @@ static int handle_exception(struct kvm_vcpu *vcpu)
>>  		kvm_run->debug.arch.dr7 = vmcs_readl(GUEST_DR7);
>>  		/* fall through */
>>  	case BP_VECTOR:
>> +		/*
>> +		 * Update instruction length as we may reinject #BP from
>> +		 * user space while in guest debugging mode. Reading it for
>> +		 * #DB as well causes no harm, it is not used in that case.
>> +		 */
>> +		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 v2] KVM: VMX: Update instruction length on intercepted BP

[Gleb Natapov]

On Tue, Feb 16, 2010 at 09:05:40AM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Mon, Feb 15, 2010 at 03:53:04PM +0100, Jan Kiszka wrote:
> >> We intercept #BP while in guest debugging mode. As VM exits 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.
> >>
> > Thinking about it some more. Why do we exit to userspace at all if we
> > intercept wrong #DB? It seams to me not wise to have ability to inject
> > exceptions from userspace. Exceptions generation mechanism is a part of
> > CPU and we shouldn't outsource part of CPU functionality to userspace.
> 
> The guest debugging API was design to avoid maintaining a "countless"
> number of breakpoints in kernel space and instead chose to loop over
> user space to decide about #DB & #BP. So this part is required even if
> we start thinking about an alternative interface in the future.
> 
How much is "countless"? 10000? I am sure we can handle this.

> Jan
> 
> > 
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> >>
> >> Changes in v2:
> >>  - added comments
> >>
> >>  arch/x86/kvm/vmx.c |   13 +++++++++++++
> >>  1 files changed, 13 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> index f82b072..14873b9 100644
> >> --- a/arch/x86/kvm/vmx.c
> >> +++ b/arch/x86/kvm/vmx.c
> >> @@ -2775,6 +2775,12 @@ static int handle_rmode_exception(struct kvm_vcpu *vcpu,
> >>  		kvm_queue_exception(vcpu, vec);
> >>  		return 1;
> >>  	case BP_VECTOR:
> >> +		/*
> >> +		 * Update instruction length as we may reinject the exception
> >> +		 * from user space while in guest debugging mode.
> >> +		 */
> >> +		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 +2903,13 @@ static int handle_exception(struct kvm_vcpu *vcpu)
> >>  		kvm_run->debug.arch.dr7 = vmcs_readl(GUEST_DR7);
> >>  		/* fall through */
> >>  	case BP_VECTOR:
> >> +		/*
> >> +		 * Update instruction length as we may reinject #BP from
> >> +		 * user space while in guest debugging mode. Reading it for
> >> +		 * #DB as well causes no harm, it is not used in that case.
> >> +		 */
> >> +		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.

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

[Jan Kiszka]

Gleb Natapov wrote:
> On Tue, Feb 16, 2010 at 09:05:40AM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Mon, Feb 15, 2010 at 03:53:04PM +0100, Jan Kiszka wrote:
>>>> We intercept #BP while in guest debugging mode. As VM exits 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.
>>>>
>>> Thinking about it some more. Why do we exit to userspace at all if we
>>> intercept wrong #DB? It seams to me not wise to have ability to inject
>>> exceptions from userspace. Exceptions generation mechanism is a part of
>>> CPU and we shouldn't outsource part of CPU functionality to userspace.
>> The guest debugging API was design to avoid maintaining a "countless"
>> number of breakpoints in kernel space and instead chose to loop over
>> user space to decide about #DB & #BP. So this part is required even if
>> we start thinking about an alternative interface in the future.
>>
> How much is "countless"? 10000? I am sure we can handle this.

We could even handle more. But would have to
 - handle INT3 injection in kernel space, including step-over on resume
 - fully parse HW breakpoints in kernel space
 - probably deal with some more complications that are now handled in
   user space, part of them even in gdb

And, again: This is an _existing_ user space ABI. We could only provide
an alternative, but we have to maintain what is there at least for some
longer grace period.

Jan

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

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

[Gleb Natapov]

On Tue, Feb 16, 2010 at 10:11:06AM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Tue, Feb 16, 2010 at 09:05:40AM +0100, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Mon, Feb 15, 2010 at 03:53:04PM +0100, Jan Kiszka wrote:
> >>>> We intercept #BP while in guest debugging mode. As VM exits 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.
> >>>>
> >>> Thinking about it some more. Why do we exit to userspace at all if we
> >>> intercept wrong #DB? It seams to me not wise to have ability to inject
> >>> exceptions from userspace. Exceptions generation mechanism is a part of
> >>> CPU and we shouldn't outsource part of CPU functionality to userspace.
> >> The guest debugging API was design to avoid maintaining a "countless"
> >> number of breakpoints in kernel space and instead chose to loop over
> >> user space to decide about #DB & #BP. So this part is required even if
> >> we start thinking about an alternative interface in the future.
> >>
> > How much is "countless"? 10000? I am sure we can handle this.
> 
> We could even handle more. But would have to
>  - handle INT3 injection in kernel space, including step-over on resume
>  - fully parse HW breakpoints in kernel space
>  - probably deal with some more complications that are now handled in
>    user space, part of them even in gdb
> 
The first point in this list is needed no anyway, no matter who reinjects
#BP event.  About point three what are those complications? As far as
I see all we need to know in kernel is a list of cr3:address pairs that
have breakpoint set. If #BP intercept happens we scan this list and if
match is not found reinject event to the guest otherwise exit to
userspace.

> And, again: This is an _existing_ user space ABI. We could only provide
> an alternative, but we have to maintain what is there at least for some
> longer grace period.
> 
But it was always broken for SVM and was broken for VMX for a year and
nobody noticed, so may be instead of reintroducing old interface we should
do it right this time?

--
			Gleb.

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

[Avi Kivity]

On 02/17/2010 12:43 PM, Gleb Natapov wrote:
>> And, again: This is an _existing_ user space ABI. We could only provide
>> an alternative, but we have to maintain what is there at least for some
>> longer grace period.
>>
>>      
> But it was always broken for SVM and was broken for VMX for a year and
> nobody noticed, so may be instead of reintroducing old interface we should
> do it right this time?
>    

We need to fix the existing interface first, and then think long and 
hard if we want yet another interface, since we're likely to screw it up 
as well.

The more interfaces we introduce, the harder maintenance becomes.

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

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

[Gleb Natapov]

On Wed, Feb 17, 2010 at 01:13:29PM +0200, Avi Kivity wrote:
> On 02/17/2010 12:43 PM, Gleb Natapov wrote:
> >>And, again: This is an _existing_ user space ABI. We could only provide
> >>an alternative, but we have to maintain what is there at least for some
> >>longer grace period.
> >>
> >But it was always broken for SVM and was broken for VMX for a year and
> >nobody noticed, so may be instead of reintroducing old interface we should
> >do it right this time?
> 
> We need to fix the existing interface first, and then think long and
> hard if we want yet another interface, since we're likely to screw
> it up as well.
> 
> The more interfaces we introduce, the harder maintenance becomes.
> 
We are in a sad state if we cannot improve interface. The current one
outsource part of CPU functionality into userspace. This should be a big
no-no.

--
			Gleb.

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

[Jan Kiszka]

Gleb Natapov wrote:
> On Wed, Feb 17, 2010 at 01:13:29PM +0200, Avi Kivity wrote:
>> On 02/17/2010 12:43 PM, Gleb Natapov wrote:
>>>> And, again: This is an _existing_ user space ABI. We could only provide
>>>> an alternative, but we have to maintain what is there at least for some
>>>> longer grace period.
>>>>
>>> But it was always broken for SVM and was broken for VMX for a year and
>>> nobody noticed, so may be instead of reintroducing old interface we should
>>> do it right this time?
>> We need to fix the existing interface first, and then think long and
>> hard if we want yet another interface, since we're likely to screw
>> it up as well.
>>
>> The more interfaces we introduce, the harder maintenance becomes.
>>
> We are in a sad state if we cannot improve interface. The current one
> outsource part of CPU functionality into userspace. This should be a big
> no-no.

I still disagree on this. Moving the decision logic to user space
prevented to re-implement a gdbstub in kernel space. I oversaw that
re-injecting #BP over older SVM was broken, but it is now fixed for all
vendors. So moving it back to kernel has actually no long-term reason.

Jan

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

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

[Gleb Natapov]

On Wed, Feb 17, 2010 at 12:23:39PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Feb 17, 2010 at 01:13:29PM +0200, Avi Kivity wrote:
> >> On 02/17/2010 12:43 PM, Gleb Natapov wrote:
> >>>> And, again: This is an _existing_ user space ABI. We could only provide
> >>>> an alternative, but we have to maintain what is there at least for some
> >>>> longer grace period.
> >>>>
> >>> But it was always broken for SVM and was broken for VMX for a year and
> >>> nobody noticed, so may be instead of reintroducing old interface we should
> >>> do it right this time?
> >> We need to fix the existing interface first, and then think long and
> >> hard if we want yet another interface, since we're likely to screw
> >> it up as well.
> >>
> >> The more interfaces we introduce, the harder maintenance becomes.
> >>
> > We are in a sad state if we cannot improve interface. The current one
> > outsource part of CPU functionality into userspace. This should be a big
> > no-no.
> 
> I still disagree on this. Moving the decision logic to user space
> prevented to re-implement a gdbstub in kernel space. I oversaw that
> re-injecting #BP over older SVM was broken, but it is now fixed for all
> vendors. So moving it back to kernel has actually no long-term reason.
> 
There were patches to implement gdbstub in kernel space! And not so long
time ago :) But I want to move only a tiny bit of logic into the kernel space.
And #BP reinjection brokenness is a different issue. It should be fixed
anyway no matter where decision about reinfection happens.

If maintainers think that we should not have improved interface and we
should support reinjection of #DB from userspace then this patch should
be applied. I don't have other objections to it. But I, at least, would
prefer the old interface for #DB reinjection (KVM_GUESTDBG_INJECT_DB)
and not the new one. The old one makes it explicit what we are doing,
the new one allows injection of any event and should be used only during
migration or CPU reset. It would be event good idea to fail setting
events if CPU is running.

--
			Gleb.

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

[Jan Kiszka]

Gleb Natapov wrote:
> On Wed, Feb 17, 2010 at 12:23:39PM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Wed, Feb 17, 2010 at 01:13:29PM +0200, Avi Kivity wrote:
>>>> On 02/17/2010 12:43 PM, Gleb Natapov wrote:
>>>>>> And, again: This is an _existing_ user space ABI. We could only provide
>>>>>> an alternative, but we have to maintain what is there at least for some
>>>>>> longer grace period.
>>>>>>
>>>>> But it was always broken for SVM and was broken for VMX for a year and
>>>>> nobody noticed, so may be instead of reintroducing old interface we should
>>>>> do it right this time?
>>>> We need to fix the existing interface first, and then think long and
>>>> hard if we want yet another interface, since we're likely to screw
>>>> it up as well.
>>>>
>>>> The more interfaces we introduce, the harder maintenance becomes.
>>>>
>>> We are in a sad state if we cannot improve interface. The current one
>>> outsource part of CPU functionality into userspace. This should be a big
>>> no-no.
>> I still disagree on this. Moving the decision logic to user space
>> prevented to re-implement a gdbstub in kernel space. I oversaw that
>> re-injecting #BP over older SVM was broken, but it is now fixed for all
>> vendors. So moving it back to kernel has actually no long-term reason.
>>
> There were patches to implement gdbstub in kernel space! And not so long
> time ago :)

Yes, a good reason to implement yet another one. :)

> But I want to move only a tiny bit of logic into the kernel space.
> And #BP reinjection brokenness is a different issue. It should be fixed
> anyway no matter where decision about reinfection happens.
> 
> If maintainers think that we should not have improved interface and we
> should support reinjection of #DB from userspace then this patch should
> be applied. I don't have other objections to it. But I, at least, would
> prefer the old interface for #DB reinjection (KVM_GUESTDBG_INJECT_DB)
> and not the new one. The old one makes it explicit what we are doing,
> the new one allows injection of any event and should be used only during
> migration or CPU reset. It would be event good idea to fail setting
> events if CPU is running.

Event injection is well supported by both vendors (except for those
software-triggered events). Just because QEMU mostly uses it for reset
and migration doesn't mean we have to restrict other users to only those
cases as well.

And as we have true event injection now, and as it naturally conflicts
with the special KVM_SET_GUEST_DEBUG interface, I have a patch that
consolidates this usage for QEMU: use the old interface of
SET_GUEST_DEBUG for pre-2.6.33 kernels, switch to SET_VCPU_EVENTS on
recent ones.

Jan

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

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

[Gleb Natapov]

On Wed, Feb 17, 2010 at 08:17:28PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Feb 17, 2010 at 12:23:39PM +0100, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Wed, Feb 17, 2010 at 01:13:29PM +0200, Avi Kivity wrote:
> >>>> On 02/17/2010 12:43 PM, Gleb Natapov wrote:
> >>>>>> And, again: This is an _existing_ user space ABI. We could only provide
> >>>>>> an alternative, but we have to maintain what is there at least for some
> >>>>>> longer grace period.
> >>>>>>
> >>>>> But it was always broken for SVM and was broken for VMX for a year and
> >>>>> nobody noticed, so may be instead of reintroducing old interface we should
> >>>>> do it right this time?
> >>>> We need to fix the existing interface first, and then think long and
> >>>> hard if we want yet another interface, since we're likely to screw
> >>>> it up as well.
> >>>>
> >>>> The more interfaces we introduce, the harder maintenance becomes.
> >>>>
> >>> We are in a sad state if we cannot improve interface. The current one
> >>> outsource part of CPU functionality into userspace. This should be a big
> >>> no-no.
> >> I still disagree on this. Moving the decision logic to user space
> >> prevented to re-implement a gdbstub in kernel space. I oversaw that
> >> re-injecting #BP over older SVM was broken, but it is now fixed for all
> >> vendors. So moving it back to kernel has actually no long-term reason.
> >>
> > There were patches to implement gdbstub in kernel space! And not so long
> > time ago :)
> 
> Yes, a good reason to implement yet another one. :)
> 
We can you unify them later :). But seriously I am not proposing
anything like gdbstub in kernel, just track inserted breakpoints in
kernel.

> > But I want to move only a tiny bit of logic into the kernel space.
> > And #BP reinjection brokenness is a different issue. It should be fixed
> > anyway no matter where decision about reinfection happens.
> > 
> > If maintainers think that we should not have improved interface and we
> > should support reinjection of #DB from userspace then this patch should
> > be applied. I don't have other objections to it. But I, at least, would
> > prefer the old interface for #DB reinjection (KVM_GUESTDBG_INJECT_DB)
> > and not the new one. The old one makes it explicit what we are doing,
> > the new one allows injection of any event and should be used only during
> > migration or CPU reset. It would be event good idea to fail setting
> > events if CPU is running.
> 
> Event injection is well supported by both vendors (except for those
> software-triggered events). Just because QEMU mostly uses it for reset
> and migration doesn't mean we have to restrict other users to only those
> cases as well.
Yes we have too! Qemu implements device model and the way devices
communicates with CPU is well defined and called interrupts, so we have
a way to inject interrupts (KVM_IRQ_LINE/KVM_INTERRUPT). Input is
validated and passed into VCPU in the right time, we do not inject
interrupts directly into VCPU using event injection. Exceptions, on the
other hand, is completely internal CPU thing. QEMU shouldn't be a part
of CPU emulation.

> 
> And as we have true event injection now, and as it naturally conflicts
Now we have a bug that should be fixed ASAP. We should allow setting of
some VCPU state only when VCPU is stopped and only for migration/reset
purposes.

> with the special KVM_SET_GUEST_DEBUG interface, I have a patch that
> consolidates this usage for QEMU: use the old interface of
> SET_GUEST_DEBUG for pre-2.6.33 kernels, switch to SET_VCPU_EVENTS on
> recent ones.
Don't do that please, this will encourage use of SET_VCPU_EVENTS for
something it shouldn't be used for.

--
			Gleb.