On 21/11/17 00:58, Paolo Bonzini wrote:
> On 20/11/2017 23:46, Liran Alon wrote:
>>>>
>>>
>>> But this is buggy as well, because the #GP is lost, isn't it?
>>
>> I don't think so.
>>
>> Since commit 664f8e26b00c ("KVM: X86: Fix loss of exception which has
>> not yet been injected"), there is a fundamental difference between a
>> pending exception and an injected exception.
>> A pending exception means that no side-effects of the exception have
>> been applied yet. Including incrementing the RIP after the instruction
>> which cause exception. In our case for example, handle_wrmsr() calls
>> kvm_inject_gp() and returns without calling
>> kvm_skip_emulated_instruction() which increments the RIP.
>
> Ok, I was almost sure this was going to be the case if the #GP wasn't
> lost (but couldn't convince myself 100%).
>
>> Therefore, when we exit from L2 to L1 on vmx-preemption-timer, we can
>> safely clear exception.pending because when L1 will resume L2, the
>> exception will be raised again (the same WRMSR instruction will be run
>> again which will raise #GP again).
>> This is also why vmcs12_save_pending_event() only makes sure to save in
>> VMCS12 idt-vectoring-info the "injected" events and not the "pending"
>> events (interrupt.pending is misleading name and I would rename it in
>> upcoming patch to interrupt.injected. See explanation below).
>
> Indeed. And then kvm_event_needs_reinjection starts making sense.
>
>> I can confirm this patch works because I have wrote a kvm-unit-test
>> which reproduce this issue. And after the fix the #GP is not lost and
>> raised to L2 directly correctly.
>> (I haven't posted the unit-test yet because it is very dependent on
>> correct vmx-preemption-timer timer config that varies between
>> environments).
>
> Can you post it anyway? Tests always help understanding the code.
>
> Paolo
>
Kindly pinging about this patch to understand if there is any work left to be done.
(kvm-unit-test reproducing issue was sent privately to Paolo a couple of weeks ago).
Thanks,
-Liran
