[PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Jan Kiszka]

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

Release and re-acquire preemption and IRQ lock in the same order as
vcpu_enter_guest does.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

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

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index d75c271..4f914c3 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3324,8 +3324,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu,
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	enum emulation_result err = EMULATE_DONE;
 
-	preempt_enable();
 	local_irq_enable();
+	preempt_enable();
 
 	while (!guest_state_valid(vcpu)) {
 		err = emulate_instruction(vcpu, kvm_run, 0, 0, 0);
@@ -3344,8 +3344,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu,
 			schedule();
 	}
 
-	local_irq_disable();
 	preempt_disable();
+	local_irq_disable();
 
 	vmx->invalid_state_emulation_result = err;
 }


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

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Marcelo Tosatti]

On Wed, Jul 22, 2009 at 11:53:26PM +0200, Jan Kiszka wrote:
> Release and re-acquire preemption and IRQ lock in the same order as
> vcpu_enter_guest does.

This should happen in vcpu_enter_guest, before it decides to disable
preemption/irqs (so you consolidate the control there).

Maybe add a new member to x86_ops?

> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
>  arch/x86/kvm/vmx.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index d75c271..4f914c3 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3324,8 +3324,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu,
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  	enum emulation_result err = EMULATE_DONE;
>  
> -	preempt_enable();
>  	local_irq_enable();
> +	preempt_enable();
>  
>  	while (!guest_state_valid(vcpu)) {
>  		err = emulate_instruction(vcpu, kvm_run, 0, 0, 0);
> @@ -3344,8 +3344,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu,
>  			schedule();
>  	}
>  
> -	local_irq_disable();
>  	preempt_disable();
> +	local_irq_disable();
>  
>  	vmx->invalid_state_emulation_result = err;
>  }
> 

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Jan Kiszka]

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

Marcelo Tosatti wrote:
> On Wed, Jul 22, 2009 at 11:53:26PM +0200, Jan Kiszka wrote:
>> Release and re-acquire preemption and IRQ lock in the same order as
>> vcpu_enter_guest does.
> 
> This should happen in vcpu_enter_guest, before it decides to disable
> preemption/irqs (so you consolidate the control there).

Maybe, maybe not. handle_invalid_guest_state is an alternative way of
"executing" guest code, and it currently shares the setup and tear-down
with vmx_vcpu_run. If it has to share parts that actually require
preemption and IRQ lock, then moving makes not much sense. Can anyone
comment on what the requirements for handle_invalid_guest_state are?

I would suggest to merge this fix first and then decide about and
potentially merge a refactoring patch.

Jan

> 
> Maybe add a new member to x86_ops?
> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>>  arch/x86/kvm/vmx.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index d75c271..4f914c3 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -3324,8 +3324,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu,
>>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>>  	enum emulation_result err = EMULATE_DONE;
>>  
>> -	preempt_enable();
>>  	local_irq_enable();
>> +	preempt_enable();
>>  
>>  	while (!guest_state_valid(vcpu)) {
>>  		err = emulate_instruction(vcpu, kvm_run, 0, 0, 0);
>> @@ -3344,8 +3344,8 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu,
>>  			schedule();
>>  	}
>>  
>> -	local_irq_disable();
>>  	preempt_disable();
>> +	local_irq_disable();
>>  
>>  	vmx->invalid_state_emulation_result = err;
>>  }
>>
> 
> 



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

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Avi Kivity]

On 07/24/2009 10:00 AM, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
>    
>> On Wed, Jul 22, 2009 at 11:53:26PM +0200, Jan Kiszka wrote:
>>      
>>> Release and re-acquire preemption and IRQ lock in the same order as
>>> vcpu_enter_guest does.
>>>        
>> This should happen in vcpu_enter_guest, before it decides to disable
>> preemption/irqs (so you consolidate the control there).
>>      
>
> Maybe, maybe not. handle_invalid_guest_state is an alternative way of
> "executing" guest code, and it currently shares the setup and tear-down
> with vmx_vcpu_run. If it has to share parts that actually require
> preemption and IRQ lock, then moving makes not much sense. Can anyone
> comment on what the requirements for handle_invalid_guest_state are?
>    

Like you said, it's an alternative to vmx entry/exit, so it shares the 
same requirements.  It must run with interrupts and preemption enabled, 
but any code that normally runs in the entry critical section (like 
interrupt injection) must continue to run in a critical section.


> I would suggest to merge this fix first and then decide about and
> potentially merge a refactoring patch.
>    

btw, what does it fix?  a debug warning?

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

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Jan Kiszka]

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

Avi Kivity wrote:
> On 07/24/2009 10:00 AM, Jan Kiszka wrote:
>> Marcelo Tosatti wrote:
>>   
>>> On Wed, Jul 22, 2009 at 11:53:26PM +0200, Jan Kiszka wrote:
>>>     
>>>> Release and re-acquire preemption and IRQ lock in the same order as
>>>> vcpu_enter_guest does.
>>>>        
>>> This should happen in vcpu_enter_guest, before it decides to disable
>>> preemption/irqs (so you consolidate the control there).
>>>      
>>
>> Maybe, maybe not. handle_invalid_guest_state is an alternative way of
>> "executing" guest code, and it currently shares the setup and tear-down
>> with vmx_vcpu_run. If it has to share parts that actually require
>> preemption and IRQ lock, then moving makes not much sense. Can anyone
>> comment on what the requirements for handle_invalid_guest_state are?
>>    
> 
> Like you said, it's an alternative to vmx entry/exit, so it shares the
> same requirements.  It must run with interrupts and preemption enabled,
> but any code that normally runs in the entry critical section (like
> interrupt injection) must continue to run in a critical section.
> 
> 
>> I would suggest to merge this fix first and then decide about and
>> potentially merge a refactoring patch.
>>    
> 
> btw, what does it fix?  a debug warning?
> 

I haven't seen anything in the wild, and I don't think it would raise a
warning. All it should cause is a potential delay of some pending
reschedule as preempt_enable will not fire under local_irq_disable.

Jan


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

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Avi Kivity]

On 07/26/2009 05:23 PM, Jan Kiszka wrote:
>> btw, what does it fix?  a debug warning?
>>
>>      
>
> I haven't seen anything in the wild, and I don't think it would raise a
> warning. All it should cause is a potential delay of some pending
> reschedule as preempt_enable will not fire under local_irq_disable.
>    

Ah, okay, then it is a real fix.  Preempt-correctness is important.

(but won't local_irq_enable() reschedule?)

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

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Jan Kiszka]

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

Avi Kivity wrote:
> On 07/26/2009 05:23 PM, Jan Kiszka wrote:
>>> btw, what does it fix?  a debug warning?
>>>
>>>      
>>
>> I haven't seen anything in the wild, and I don't think it would raise a
>> warning. All it should cause is a potential delay of some pending
>> reschedule as preempt_enable will not fire under local_irq_disable.
>>    
> 
> Ah, okay, then it is a real fix.  Preempt-correctness is important.
> 
> (but won't local_irq_enable() reschedule?)

The last time I checked it was essentially a plain 'sti'.

Jan


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

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Avi Kivity]

On 07/26/2009 05:38 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 07/26/2009 05:23 PM, Jan Kiszka wrote:
>>      
>>>> btw, what does it fix?  a debug warning?
>>>>
>>>>
>>>>          
>>> I haven't seen anything in the wild, and I don't think it would raise a
>>> warning. All it should cause is a potential delay of some pending
>>> reschedule as preempt_enable will not fire under local_irq_disable.
>>>
>>>        
>> Ah, okay, then it is a real fix.  Preempt-correctness is important.
>>
>> (but won't local_irq_enable() reschedule?)
>>      
>
> The last time I checked it was essentially a plain 'sti'.
>
>    

Presumably there's a reschedule interrupt queued; I think if you set the 
reschedule bit you have to IPI the cpu running the task.

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

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Jan Kiszka]

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

Avi Kivity wrote:
> On 07/26/2009 05:38 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>   
>>> On 07/26/2009 05:23 PM, Jan Kiszka wrote:
>>>     
>>>>> btw, what does it fix?  a debug warning?
>>>>>
>>>>>
>>>>>          
>>>> I haven't seen anything in the wild, and I don't think it would raise a
>>>> warning. All it should cause is a potential delay of some pending
>>>> reschedule as preempt_enable will not fire under local_irq_disable.
>>>>
>>>>        
>>> Ah, okay, then it is a real fix.  Preempt-correctness is important.
>>>
>>> (but won't local_irq_enable() reschedule?)
>>>      
>>
>> The last time I checked it was essentially a plain 'sti'.
>>
>>    
> 
> Presumably there's a reschedule interrupt queued; I think if you set the
> reschedule bit you have to IPI the cpu running the task.
> 

Yeah. But as we preempt_disable first, that one might have been
processed already.

Jan


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

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Avi Kivity]

On 07/26/2009 05:55 PM, Jan Kiszka wrote:
>> Presumably there's a reschedule interrupt queued; I think if you set the
>> reschedule bit you have to IPI the cpu running the task.
>>      
>
> Yeah. But as we preempt_disable first, that one might have been
> processed already.
>    

Ah, yes.  Thanks.

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

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Marcelo Tosatti]

On Thu, Jul 23, 2009 at 06:45:53PM -0300, Marcelo Tosatti wrote:
> On Wed, Jul 22, 2009 at 11:53:26PM +0200, Jan Kiszka wrote:
> > Release and re-acquire preemption and IRQ lock in the same order as
> > vcpu_enter_guest does.
> 
> This should happen in vcpu_enter_guest, before it decides to disable
> preemption/irqs (so you consolidate the control there).
> 
> Maybe add a new member to x86_ops?

Why don't do something like this ?


Index: kvm-requests/arch/x86/include/asm/kvm_host.h
===================================================================
--- kvm-requests.orig/arch/x86/include/asm/kvm_host.h
+++ kvm-requests/arch/x86/include/asm/kvm_host.h
@@ -529,6 +529,7 @@ struct kvm_x86_ops {
 	int (*set_tss_addr)(struct kvm *kvm, unsigned int addr);
 	int (*get_tdp_level)(void);
 	u64 (*get_mt_mask)(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio);
+	int (*emulate_guest_state)(struct kvm_vcpu *vcpu, struct kvm_run *run);
 	const struct trace_print_flags *exit_reasons_str;
 };
 
Index: kvm-requests/arch/x86/kvm/vmx.c
===================================================================
--- kvm-requests.orig/arch/x86/kvm/vmx.c
+++ kvm-requests/arch/x86/kvm/vmx.c
@@ -106,8 +106,6 @@ struct vcpu_vmx {
 		} irq;
 	} rmode;
 	int vpid;
-	bool emulation_required;
-	enum emulation_result invalid_state_emulation_result;
 
 	/* Support for vnmi-less CPUs */
 	int soft_vnmi_blocked;
@@ -1416,7 +1414,6 @@ static void enter_pmode(struct kvm_vcpu 
 	unsigned long flags;
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
-	vmx->emulation_required = 1;
 	vmx->rmode.vm86_active = 0;
 
 	vmcs_writel(GUEST_TR_BASE, vmx->rmode.tr.base);
@@ -1433,8 +1430,10 @@ static void enter_pmode(struct kvm_vcpu 
 
 	update_exception_bitmap(vcpu);
 
-	if (emulate_invalid_guest_state)
+	if (emulate_invalid_guest_state) {
+		set_bit(KVM_REQ_EMULATE, &vcpu->requests);
 		return;
+	}
 
 	fix_pmode_dataseg(VCPU_SREG_ES, &vmx->rmode.es);
 	fix_pmode_dataseg(VCPU_SREG_DS, &vmx->rmode.ds);
@@ -1481,7 +1480,6 @@ static void enter_rmode(struct kvm_vcpu 
 	if (enable_unrestricted_guest)
 		return;
 
-	vmx->emulation_required = 1;
 	vmx->rmode.vm86_active = 1;
 
 	vmx->rmode.tr.base = vmcs_readl(GUEST_TR_BASE);
@@ -1503,8 +1501,10 @@ static void enter_rmode(struct kvm_vcpu 
 	vmcs_writel(GUEST_CR4, vmcs_readl(GUEST_CR4) | X86_CR4_VME);
 	update_exception_bitmap(vcpu);
 
-	if (emulate_invalid_guest_state)
+	if (emulate_invalid_guest_state) {
+		set_bit(KVM_REQ_EMULATE, &vcpu->requests);
 		goto continue_rmode;
+	}
 
 	vmcs_write16(GUEST_SS_SELECTOR, vmcs_readl(GUEST_SS_BASE) >> 4);
 	vmcs_write32(GUEST_SS_LIMIT, 0xffff);
@@ -2517,9 +2517,6 @@ static int vmx_vcpu_reset(struct kvm_vcp
 
 	ret = 0;
 
-	/* HACK: Don't enable emulation on guest boot/reset */
-	vmx->emulation_required = 0;
-
 out:
 	up_read(&vcpu->kvm->slots_lock);
 	return ret;
@@ -3319,15 +3316,11 @@ static int handle_nmi_window(struct kvm_
 	return 1;
 }
 
-static void handle_invalid_guest_state(struct kvm_vcpu *vcpu,
+static int handle_invalid_guest_state(struct kvm_vcpu *vcpu,
 				struct kvm_run *kvm_run)
 {
-	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	enum emulation_result err = EMULATE_DONE;
 
-	local_irq_enable();
-	preempt_enable();
-
 	while (!guest_state_valid(vcpu)) {
 		err = emulate_instruction(vcpu, kvm_run, 0, 0, 0);
 
@@ -3345,10 +3338,10 @@ static void handle_invalid_guest_state(s
 			schedule();
 	}
 
-	preempt_disable();
-	local_irq_disable();
+	if (!guest_state_valid(vcpu))
+		set_bit(KVM_REQ_EMULATE, &vcpu->requests);
 
-	vmx->invalid_state_emulation_result = err;
+	return (err != EMULATE_DO_MMIO);
 }
 
 /*
@@ -3405,14 +3398,6 @@ static int vmx_handle_exit(struct kvm_ru
 
 	trace_kvm_exit(exit_reason, kvm_rip_read(vcpu));
 
-	/* If we need to emulate an MMIO from handle_invalid_guest_state
-	 * we just return 0 */
-	if (vmx->emulation_required && emulate_invalid_guest_state) {
-		if (guest_state_valid(vcpu))
-			vmx->emulation_required = 0;
-		return vmx->invalid_state_emulation_result != EMULATE_DO_MMIO;
-	}
-
 	/* Access CR3 don't cause VMExit in paging mode, so we need
 	 * to sync with guest real CR3. */
 	if (enable_ept && is_paging(vcpu))
@@ -3606,12 +3591,6 @@ static void vmx_vcpu_run(struct kvm_vcpu
 	if (unlikely(!cpu_has_virtual_nmis() && vmx->soft_vnmi_blocked))
 		vmx->entry_time = ktime_get();
 
-	/* Handle invalid guest state instead of entering VMX */
-	if (vmx->emulation_required && emulate_invalid_guest_state) {
-		handle_invalid_guest_state(vcpu, kvm_run);
-		return;
-	}
-
 	if (test_bit(VCPU_REGS_RSP, (unsigned long *)&vcpu->arch.regs_dirty))
 		vmcs_writel(GUEST_RSP, vcpu->arch.regs[VCPU_REGS_RSP]);
 	if (test_bit(VCPU_REGS_RIP, (unsigned long *)&vcpu->arch.regs_dirty))
@@ -3967,6 +3946,7 @@ static struct kvm_x86_ops vmx_x86_ops = 
 	.set_tss_addr = vmx_set_tss_addr,
 	.get_tdp_level = get_ept_level,
 	.get_mt_mask = vmx_get_mt_mask,
+	.emulate_guest_state = handle_invalid_guest_state,
 
 	.exit_reasons_str = vmx_exit_reasons_str,
 };
Index: kvm-requests/arch/x86/kvm/x86.c
===================================================================
--- kvm-requests.orig/arch/x86/kvm/x86.c
+++ kvm-requests/arch/x86/kvm/x86.c
@@ -3556,6 +3556,11 @@ static int vcpu_enter_guest(struct kvm_v
 			kvm_mmu_sync_roots(vcpu);
 		if (test_and_clear_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests))
 			kvm_x86_ops->tlb_flush(vcpu);
+		if (test_and_clear_bit(KVM_REQ_EMULATE, &vcpu->requests)) {
+			r = kvm_x86_ops->emulate_guest_state(vcpu, kvm_run);
+			goto out;
+		}
+
 		if (test_and_clear_bit(KVM_REQ_REPORT_TPR_ACCESS,
 				       &vcpu->requests)) {
 			kvm_run->exit_reason = KVM_EXIT_TPR_ACCESS;
Index: kvm-requests/include/linux/kvm_host.h
===================================================================
--- kvm-requests.orig/include/linux/kvm_host.h
+++ kvm-requests/include/linux/kvm_host.h
@@ -39,6 +39,7 @@
 #define KVM_REQ_MMU_SYNC           7
 #define KVM_REQ_KVMCLOCK_UPDATE    8
 #define KVM_REQ_KICK               9
+#define KVM_REQ_EMULATE		   10
 
 #define KVM_USERSPACE_IRQ_SOURCE_ID	0
 

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Avi Kivity]

On 07/29/2009 03:24 PM, Marcelo Tosatti wrote:
> On Thu, Jul 23, 2009 at 06:45:53PM -0300, Marcelo Tosatti wrote:
>    
>> On Wed, Jul 22, 2009 at 11:53:26PM +0200, Jan Kiszka wrote:
>>      
>>> Release and re-acquire preemption and IRQ lock in the same order as
>>> vcpu_enter_guest does.
>>>        
>> This should happen in vcpu_enter_guest, before it decides to disable
>> preemption/irqs (so you consolidate the control there).
>>
>> Maybe add a new member to x86_ops?
>>      
>
> Why don't do something like this ?
>    

The downside is that we're moving a vmx specific hack to common code.

I think this could be simplified if interrupt injection happened outside 
the critical section.  This is needed anyway because emulated interrupt 
injection needs to access guest memory (IVT and the stack).

Something else I noticed, handle_invalid_guest_state() doesn't check 
vcpu->requests; normal execution will exit due to the interrupt while 
emulated execution will not.

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

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Marcelo Tosatti]

On Wed, Jul 29, 2009 at 03:44:20PM +0300, Avi Kivity wrote:
> On 07/29/2009 03:24 PM, Marcelo Tosatti wrote:
>> On Thu, Jul 23, 2009 at 06:45:53PM -0300, Marcelo Tosatti wrote:
>>    
>>> On Wed, Jul 22, 2009 at 11:53:26PM +0200, Jan Kiszka wrote:
>>>      
>>>> Release and re-acquire preemption and IRQ lock in the same order as
>>>> vcpu_enter_guest does.
>>>>        
>>> This should happen in vcpu_enter_guest, before it decides to disable
>>> preemption/irqs (so you consolidate the control there).
>>>
>>> Maybe add a new member to x86_ops?
>>>      
>>
>> Why don't do something like this ?
>>    
>
> The downside is that we're moving a vmx specific hack to common code.
>
> I think this could be simplified if interrupt injection happened outside  
> the critical section.  This is needed anyway because emulated interrupt  
> injection needs to access guest memory (IVT and the stack).

Why can't it happen now (outside of the critical section), other than
the kvm_vcpu_kick thing?

>
> Something else I noticed, handle_invalid_guest_state() doesn't check  
> vcpu->requests; normal execution will exit due to the interrupt while  
> emulated execution will not.
>
> -- 
> error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Gleb Natapov]

On Wed, Jul 29, 2009 at 11:07:16AM -0300, Marcelo Tosatti wrote:
> On Wed, Jul 29, 2009 at 03:44:20PM +0300, Avi Kivity wrote:
> > On 07/29/2009 03:24 PM, Marcelo Tosatti wrote:
> >> On Thu, Jul 23, 2009 at 06:45:53PM -0300, Marcelo Tosatti wrote:
> >>    
> >>> On Wed, Jul 22, 2009 at 11:53:26PM +0200, Jan Kiszka wrote:
> >>>      
> >>>> Release and re-acquire preemption and IRQ lock in the same order as
> >>>> vcpu_enter_guest does.
> >>>>        
> >>> This should happen in vcpu_enter_guest, before it decides to disable
> >>> preemption/irqs (so you consolidate the control there).
> >>>
> >>> Maybe add a new member to x86_ops?
> >>>      
> >>
> >> Why don't do something like this ?
> >>    
> >
> > The downside is that we're moving a vmx specific hack to common code.
> >
> > I think this could be simplified if interrupt injection happened outside  
> > the critical section.  This is needed anyway because emulated interrupt  
> > injection needs to access guest memory (IVT and the stack).
> 
> Why can't it happen now (outside of the critical section), other than
> the kvm_vcpu_kick thing?
> 
Depend what part of irq injection we want out of critical section. I
guess inject_pending_event() can be called outside of it now. Need to
think about others.

--
			Gleb.

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Gleb Natapov]

On Thu, Jul 30, 2009 at 02:16:30PM +0300, Avi Kivity wrote:
> On 07/29/2009 05:07 PM, Marcelo Tosatti wrote:
>>> The downside is that we're moving a vmx specific hack to common code.
>>>
>>> I think this could be simplified if interrupt injection happened outside
>>> the critical section.  This is needed anyway because emulated interrupt
>>> injection needs to access guest memory (IVT and the stack).
>>>      
>>
>> Why can't it happen now (outside of the critical section), other than
>> the kvm_vcpu_kick thing?
>>    
>
> I think there's little reason now.  One thing we need to do is make it  
> possible to call the injection code twice without entering the guest.  I  
> think right now it assumes nothing has been injected.
>
I Looked at this and it seems the current code handle this case.
Injection puts an event on a queue and if we haven't entered a guest
after this point on the next entry event is injected from the queue,
just like if injection failed due to IDT access.

What may happen is that at the time of irq injection there will be irq
with higher irr pending in APIC, But it looks like this already may
happen if injection failed on IDT access, but this is rare.

> We need either to cancel a previous injection (a variant of  
> vmx_complete_interrupts()), or avoid reinjecting  if we already did.
>
> -- 
> error compiling committee.c: too many arguments to function

--
			Gleb.

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Avi Kivity]

On 07/29/2009 05:07 PM, Marcelo Tosatti wrote:
>> The downside is that we're moving a vmx specific hack to common code.
>>
>> I think this could be simplified if interrupt injection happened outside
>> the critical section.  This is needed anyway because emulated interrupt
>> injection needs to access guest memory (IVT and the stack).
>>      
>
> Why can't it happen now (outside of the critical section), other than
> the kvm_vcpu_kick thing?
>    

I think there's little reason now.  One thing we need to do is make it 
possible to call the injection code twice without entering the guest.  I 
think right now it assumes nothing has been injected.

We need either to cancel a previous injection (a variant of 
vmx_complete_interrupts()), or avoid reinjecting  if we already did.

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

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Gleb Natapov]

On Thu, Jul 30, 2009 at 02:26:24PM +0300, Avi Kivity wrote:
> On 07/30/2009 02:16 PM, Gleb Natapov wrote:
>>> I think there's little reason now.  One thing we need to do is make it
>>> possible to call the injection code twice without entering the guest.  I
>>> think right now it assumes nothing has been injected.
>>>
>>>      
>> I Looked at this and it seems the current code handle this case.
>> Injection puts an event on a queue and if we haven't entered a guest
>> after this point on the next entry event is injected from the queue,
>> just like if injection failed due to IDT access.
>>
>>    
>
> Good (it was one of the goals of the original interrupt rework, ~2 years  
> ago)
>
But if we emulate an injection by playing with guest memory and
registers we have to be sure we do it only once.

--
			Gleb.

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Avi Kivity]

On 07/30/2009 02:16 PM, Gleb Natapov wrote:
>> I think there's little reason now.  One thing we need to do is make it
>> possible to call the injection code twice without entering the guest.  I
>> think right now it assumes nothing has been injected.
>>
>>      
> I Looked at this and it seems the current code handle this case.
> Injection puts an event on a queue and if we haven't entered a guest
> after this point on the next entry event is injected from the queue,
> just like if injection failed due to IDT access.
>
>    

Good (it was one of the goals of the original interrupt rework, ~2 years 
ago)

> What may happen is that at the time of irq injection there will be irq
> with higher irr pending in APIC, But it looks like this already may
> happen if injection failed on IDT access, but this is rare.
>    

Right, we don't guarantee the precise time the APIC selects the vector 
to inject, as long as it's after the last vmexit.

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

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Avi Kivity]

On 07/30/2009 02:24 PM, Gleb Natapov wrote:
>> Good (it was one of the goals of the original interrupt rework, ~2 years
>> ago)
>>
>>      
> But if we emulate an injection by playing with guest memory and
> registers we have to be sure we do it only once.
>    

Once we've successfully updated the stack, inject is complete and we can 
remove the interrupt from the queue.

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

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Gleb Natapov]

On Thu, Jul 30, 2009 at 02:46:59PM +0300, Avi Kivity wrote:
> On 07/30/2009 02:24 PM, Gleb Natapov wrote:
>>> Good (it was one of the goals of the original interrupt rework, ~2 years
>>> ago)
>>>
>>>      
>> But if we emulate an injection by playing with guest memory and
>> registers we have to be sure we do it only once.
>>    
>
> Once we've successfully updated the stack, inject is complete and we can  
> remove the interrupt from the queue.
>
Then, it we skip actual guest entry for some reason, we will not be
able to tell if we can inject next irq or not.

--
			Gleb.

Re: [PATCH] KVM: VMX: Fix locking order in handle_invalid_guest_state

[Gleb Natapov]

On Thu, Jul 30, 2009 at 02:47:30PM +0300, Gleb Natapov wrote:
> On Thu, Jul 30, 2009 at 02:46:59PM +0300, Avi Kivity wrote:
> > On 07/30/2009 02:24 PM, Gleb Natapov wrote:
> >>> Good (it was one of the goals of the original interrupt rework, ~2 years
> >>> ago)
> >>>
> >>>      
> >> But if we emulate an injection by playing with guest memory and
> >> registers we have to be sure we do it only once.
> >>    
> >
> > Once we've successfully updated the stack, inject is complete and we can  
> > remove the interrupt from the queue.
> >
> Then, it we skip actual guest entry for some reason, we will not be
> able to tell if we can inject next irq or not.
> 
Actually thinking about it when first irq is injected we should clear IF
flag, so the second one will not happen.

--
			Gleb.