Re: [PATCH v2 5/5] hvf: save away type as well as vector so we can reinject them

[Paolo Bonzini]

On 30/11/19 09:31, Cameron Esfahani wrote:
> So far so good.  Without any workaround, I could get it to fail
> within a few seconds.  With your change, I've been running for a few
> minutes without a problem.  But, this is on my laptop, so I'll wait
> until I can test it on a wider-range of machines at work next week.
> If it continues to work, I'll update my patch to include this fix.

Great!  Note that there was a follow-up hunk in another email.

> Now, can you help me understand why this approach is better than what
> I had written?  When we're in hvf_store_events(), we have vector type
> and number.  All the information we need to reinject later.  So why
> not save vector type away, instead of attempting to reconstruct it
> from other information (env->ins_len) in hvf_inject_interrupts()?

No huge reason, I agree.  However, the event injection code in the
Android emulator was broken for pretty much every corner case, so when
we rewrote it in QEMU for Summer of Code we took KVM as a model (commit
b7394c8394, "i386: hvf: refactor event injection code for hvf",
2017-12-22).  Keeping the code similar can help with debugging, so I
prefer having the same meaning for env->interrupt_injected and
env->exception_nr across HVF and KVM.

I didn't write the code for KVM, so I cannot really say why they chose
to not save the event type.

Thanks,

Paolo

> 
> Cameron Esfahani
> address@hidden
> 
> "There are times in the life of a nation when the only place a decent man can 
> find himself is in prison."
> 
> 
> 
>> On Nov 28, 2019, at 5:56 AM, Paolo Bonzini <address@hidden> wrote:
>>
>> On 26/11/19 21:04, Cameron Esfahani wrote:
>>> Our test case was booting many concurrent macOS VMs under heavy
>>> system load.  I don't know if I could create one to replicate that.
>>
>> Does this work?
>>
>> diff --git a/target/i386/hvf/x86hvf.c b/target/i386/hvf/x86hvf.c
>> index 1485b95776..26c6c3a49f 100644
>> --- a/target/i386/hvf/x86hvf.c
>> +++ b/target/i386/hvf/x86hvf.c
>> @@ -357,7 +357,11 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
>>     bool have_event = true;
>>     if (env->interrupt_injected != -1) {
>>         vector = env->interrupt_injected;
>> -        intr_type = VMCS_INTR_T_SWINTR;
>> +        if (env->ins_len) {
>> +            intr_type = VMCS_INTR_T_SWINTR;
>> +        } else {
>> +            intr_type = VMCS_INTR_T_HWINTR;
>> +        }
>>     } else if (env->exception_nr != -1) {
>>         vector = env->exception_nr;
>>         if (vector == EXCP03_INT3 || vector == EXCP04_INTO) {
>>
>> Thanks,
>>
>> Paolo
>>
>