diff mbox

[05/15] Coalesce userspace/kernel irqchip interrupt injection logic.

Message ID 20090419135745.GO10126@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Gleb Natapov April 19, 2009, 1:57 p.m. UTC
On Sat, Apr 18, 2009 at 07:28:20PM +0300, Gleb Natapov wrote:
> > 
> > So this patch may either expose a bug in the svm emulation of qemu or
> > comes with a subtle regression that only triggers due to qemu's timing.
> > This needs to be understood. Gleb, any progress on reproducing it on
> > your side?
> > 
> I reproduced it and I am debugging it. In my case the boot hangs on sti;hlt
> sequence. Instrumentation thus far shows that at this point interrupts no longer
> injected because ppr value is too big. Need to see why, but tpr handling
> is not complete in qemu svm. May be this is the reason. Will know more
> tomorrow.
> 
I've looked into this and my conclusion is that if you are not going to
develop SVM in qemu don't use it just yet. QEMU doesn't handle exceptions
during event injection properly. Actually it does not handle it at all,
so if PF happens during interrupt injection interrupt is lost and, what
worse, is never acked. If interrupt was high prio it blocks all other
interrupts.

The patch below adds exception handling during event injection. Valid
flag removed from EVENTINJ only after successful injection and EVENTINJ
is copied to EXITINTINFO on exit. Can you give it a try?

And this is not the only problem I saw, but the one that caused my guest
to hang.

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

Comments

Jan Kiszka April 19, 2009, 2:05 p.m. UTC | #1
Gleb Natapov wrote:
> On Sat, Apr 18, 2009 at 07:28:20PM +0300, Gleb Natapov wrote:
>>> So this patch may either expose a bug in the svm emulation of qemu or
>>> comes with a subtle regression that only triggers due to qemu's timing.
>>> This needs to be understood. Gleb, any progress on reproducing it on
>>> your side?
>>>
>> I reproduced it and I am debugging it. In my case the boot hangs on sti;hlt
>> sequence. Instrumentation thus far shows that at this point interrupts no longer
>> injected because ppr value is too big. Need to see why, but tpr handling
>> is not complete in qemu svm. May be this is the reason. Will know more
>> tomorrow.
>>
> I've looked into this and my conclusion is that if you are not going to
> develop SVM in qemu don't use it just yet.

We had a resource conflict regarding SVM capable AMD boxes and a tight
schedule, so we decided to pick qemu as initial development platform.
Turns out that this has was a bit too optimistic. :)

> QEMU doesn't handle exceptions
> during event injection properly. Actually it does not handle it at all,
> so if PF happens during interrupt injection interrupt is lost and, what
> worse, is never acked. If interrupt was high prio it blocks all other
> interrupts.
> 
> The patch below adds exception handling during event injection. Valid
> flag removed from EVENTINJ only after successful injection and EVENTINJ
> is copied to EXITINTINFO on exit. Can you give it a try?

Ah, great, thanks. Will test.

> 
> And this is not the only problem I saw, but the one that caused my guest
> to hang.

OK, good to know. I added Alex (though he's said to be on vacation ATM)
and qemu to CC. Maybe you can quickly list the other issues you've
stumbled over, for the records and for motivating contributors...

> 
> diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
> index be09263..9264afd 100644
> --- a/target-i386/op_helper.c
> +++ b/target-i386/op_helper.c
> @@ -1249,6 +1249,10 @@ void do_interrupt(int intno, int is_int, int error_code,
>      } else {
>          do_interrupt_real(intno, is_int, error_code, next_eip);
>      }
> +    if (env->hflags & HF_SVMI_MASK) {
> +	    uint32_t event_inj = ldl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj));
> +	    stl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj), event_inj & ~SVM_EVTINJ_VALID);
> +    }
>  }
>  
>  /* This should come from sysemu.h - if we could include it here... */
> @@ -4994,7 +4998,6 @@ void helper_vmrun(int aflag, int next_eip_addend)
>          uint8_t vector = event_inj & SVM_EVTINJ_VEC_MASK;
>          uint16_t valid_err = event_inj & SVM_EVTINJ_VALID_ERR;
>          uint32_t event_inj_err = ldl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj_err));
> -        stl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj), event_inj & ~SVM_EVTINJ_VALID);
>  
>          qemu_log_mask(CPU_LOG_TB_IN_ASM, "Injecting(%#hx): ", valid_err);
>          /* FIXME: need to implement valid_err */
> @@ -5331,6 +5334,8 @@ void helper_vmexit(uint32_t exit_code, uint64_t exit_info_1)
>      cpu_x86_set_cpl(env, 0);
>      stq_phys(env->vm_vmcb + offsetof(struct vmcb, control.exit_code), exit_code);
>      stq_phys(env->vm_vmcb + offsetof(struct vmcb, control.exit_info_1), exit_info_1);
> +    stl_phys(env->vm_vmcb + offsetof(struct vmcb, control.exit_int_info), ldl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj)));
> +    stl_phys(env->vm_vmcb + offsetof(struct vmcb, control.exit_int_info_err), ldl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj_err)));
>  
>      env->hflags2 &= ~HF2_GIF_MASK;
>      /* FIXME: Resets the current ASID register to zero (host ASID). */
> --
> 			Gleb.

Thanks again,
Jan
Jan Kiszka April 19, 2009, 3:06 p.m. UTC | #2
Jan Kiszka wrote:
> Gleb Natapov wrote:
>> On Sat, Apr 18, 2009 at 07:28:20PM +0300, Gleb Natapov wrote:
>>>> So this patch may either expose a bug in the svm emulation of qemu or
>>>> comes with a subtle regression that only triggers due to qemu's timing.
>>>> This needs to be understood. Gleb, any progress on reproducing it on
>>>> your side?
>>>>
>>> I reproduced it and I am debugging it. In my case the boot hangs on sti;hlt
>>> sequence. Instrumentation thus far shows that at this point interrupts no longer
>>> injected because ppr value is too big. Need to see why, but tpr handling
>>> is not complete in qemu svm. May be this is the reason. Will know more
>>> tomorrow.
>>>
>> I've looked into this and my conclusion is that if you are not going to
>> develop SVM in qemu don't use it just yet.
> 
> We had a resource conflict regarding SVM capable AMD boxes and a tight
> schedule, so we decided to pick qemu as initial development platform.
> Turns out that this has was a bit too optimistic. :)
> 
>> QEMU doesn't handle exceptions
>> during event injection properly. Actually it does not handle it at all,
>> so if PF happens during interrupt injection interrupt is lost and, what
>> worse, is never acked. If interrupt was high prio it blocks all other
>> interrupts.
>>
>> The patch below adds exception handling during event injection. Valid
>> flag removed from EVENTINJ only after successful injection and EVENTINJ
>> is copied to EXITINTINFO on exit. Can you give it a try?
> 
> Ah, great, thanks. Will test.

I can confirm: patch below makes my kvm-in-qemu test case happy, too.
Maybe you want to post this with changelog and signed-off to qemu-devel.

Jan

>> diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
>> index be09263..9264afd 100644
>> --- a/target-i386/op_helper.c
>> +++ b/target-i386/op_helper.c
>> @@ -1249,6 +1249,10 @@ void do_interrupt(int intno, int is_int, int error_code,
>>      } else {
>>          do_interrupt_real(intno, is_int, error_code, next_eip);
>>      }
>> +    if (env->hflags & HF_SVMI_MASK) {
>> +	    uint32_t event_inj = ldl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj));
>> +	    stl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj), event_inj & ~SVM_EVTINJ_VALID);
>> +    }
>>  }
>>  
>>  /* This should come from sysemu.h - if we could include it here... */
>> @@ -4994,7 +4998,6 @@ void helper_vmrun(int aflag, int next_eip_addend)
>>          uint8_t vector = event_inj & SVM_EVTINJ_VEC_MASK;
>>          uint16_t valid_err = event_inj & SVM_EVTINJ_VALID_ERR;
>>          uint32_t event_inj_err = ldl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj_err));
>> -        stl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj), event_inj & ~SVM_EVTINJ_VALID);
>>  
>>          qemu_log_mask(CPU_LOG_TB_IN_ASM, "Injecting(%#hx): ", valid_err);
>>          /* FIXME: need to implement valid_err */
>> @@ -5331,6 +5334,8 @@ void helper_vmexit(uint32_t exit_code, uint64_t exit_info_1)
>>      cpu_x86_set_cpl(env, 0);
>>      stq_phys(env->vm_vmcb + offsetof(struct vmcb, control.exit_code), exit_code);
>>      stq_phys(env->vm_vmcb + offsetof(struct vmcb, control.exit_info_1), exit_info_1);
>> +    stl_phys(env->vm_vmcb + offsetof(struct vmcb, control.exit_int_info), ldl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj)));
>> +    stl_phys(env->vm_vmcb + offsetof(struct vmcb, control.exit_int_info_err), ldl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj_err)));
>>  
>>      env->hflags2 &= ~HF2_GIF_MASK;
>>      /* FIXME: Resets the current ASID register to zero (host ASID). */
>> --
>> 			Gleb.
Gleb Natapov April 19, 2009, 3:20 p.m. UTC | #3
On Sun, Apr 19, 2009 at 05:06:29PM +0200, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Gleb Natapov wrote:
> >> On Sat, Apr 18, 2009 at 07:28:20PM +0300, Gleb Natapov wrote:
> >>>> So this patch may either expose a bug in the svm emulation of qemu or
> >>>> comes with a subtle regression that only triggers due to qemu's timing.
> >>>> This needs to be understood. Gleb, any progress on reproducing it on
> >>>> your side?
> >>>>
> >>> I reproduced it and I am debugging it. In my case the boot hangs on sti;hlt
> >>> sequence. Instrumentation thus far shows that at this point interrupts no longer
> >>> injected because ppr value is too big. Need to see why, but tpr handling
> >>> is not complete in qemu svm. May be this is the reason. Will know more
> >>> tomorrow.
> >>>
> >> I've looked into this and my conclusion is that if you are not going to
> >> develop SVM in qemu don't use it just yet.
> > 
> > We had a resource conflict regarding SVM capable AMD boxes and a tight
> > schedule, so we decided to pick qemu as initial development platform.
> > Turns out that this has was a bit too optimistic. :)
> > 
> >> QEMU doesn't handle exceptions
> >> during event injection properly. Actually it does not handle it at all,
> >> so if PF happens during interrupt injection interrupt is lost and, what
> >> worse, is never acked. If interrupt was high prio it blocks all other
> >> interrupts.
> >>
> >> The patch below adds exception handling during event injection. Valid
> >> flag removed from EVENTINJ only after successful injection and EVENTINJ
> >> is copied to EXITINTINFO on exit. Can you give it a try?
> > 
> > Ah, great, thanks. Will test.
> 
> I can confirm: patch below makes my kvm-in-qemu test case happy, too.
> Maybe you want to post this with changelog and signed-off to qemu-devel.
> 
Yeah, I'll reformat and submit.

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

Patch

diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
index be09263..9264afd 100644
--- a/target-i386/op_helper.c
+++ b/target-i386/op_helper.c
@@ -1249,6 +1249,10 @@  void do_interrupt(int intno, int is_int, int error_code,
     } else {
         do_interrupt_real(intno, is_int, error_code, next_eip);
     }
+    if (env->hflags & HF_SVMI_MASK) {
+	    uint32_t event_inj = ldl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj));
+	    stl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj), event_inj & ~SVM_EVTINJ_VALID);
+    }
 }
 
 /* This should come from sysemu.h - if we could include it here... */
@@ -4994,7 +4998,6 @@  void helper_vmrun(int aflag, int next_eip_addend)
         uint8_t vector = event_inj & SVM_EVTINJ_VEC_MASK;
         uint16_t valid_err = event_inj & SVM_EVTINJ_VALID_ERR;
         uint32_t event_inj_err = ldl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj_err));
-        stl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj), event_inj & ~SVM_EVTINJ_VALID);
 
         qemu_log_mask(CPU_LOG_TB_IN_ASM, "Injecting(%#hx): ", valid_err);
         /* FIXME: need to implement valid_err */
@@ -5331,6 +5334,8 @@  void helper_vmexit(uint32_t exit_code, uint64_t exit_info_1)
     cpu_x86_set_cpl(env, 0);
     stq_phys(env->vm_vmcb + offsetof(struct vmcb, control.exit_code), exit_code);
     stq_phys(env->vm_vmcb + offsetof(struct vmcb, control.exit_info_1), exit_info_1);
+    stl_phys(env->vm_vmcb + offsetof(struct vmcb, control.exit_int_info), ldl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj)));
+    stl_phys(env->vm_vmcb + offsetof(struct vmcb, control.exit_int_info_err), ldl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj_err)));
 
     env->hflags2 &= ~HF2_GIF_MASK;
     /* FIXME: Resets the current ASID register to zero (host ASID). */