[Qemu-devel] Re: [PATCH 05/15] Coalesce userspace/kernel irqchip interrupt injection logic.

[Jan Kiszka <jan.kiszka@web.de>]

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

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.



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