Re: [PATCH] KVM: nVMX: Fix direct injection of interrupts from L0 to L2

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

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

On 2013-02-17 17:26, Gleb Natapov wrote:
> On Sun, Feb 17, 2013 at 04:31:26PM +0100, Jan Kiszka wrote:
>> On 2013-02-17 16:07, Gleb Natapov wrote:
>>> On Sat, Feb 16, 2013 at 06:10:14PM +0100, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> If L1 does not set PIN_BASED_EXT_INTR_MASK, we incorrectly skipped
>>>> vmx_complete_interrupts on L2 exits. This is required because, with
>>>> direct interrupt injection from L0 to L2, L0 has to update its pending
>>>> events.
>>>>
>>>> Also, we need to allow vmx_cancel_injection when entering L2 in we left
>>>> to L0. This condition is indirectly derived from the absence of valid
>>>> vectoring info in vmcs12. We no explicitly clear it if we find out that
>>>> the L2 exit is not targeting L1 but L0.
>>>>
>>> We really need to overhaul how interrupt injection is emulated in nested
>>> VMX. Why not put pending events into event queue instead of
>>> get_vmcs12(vcpu)->idt_vectoring_info_field and inject them in usual way.
>>
>> I was thinking about the same step but felt unsure so far if
>> vmx_complete_interrupts & Co. do not include any assumptions about the
>> vmcs configuration that won't match what L1 does. So I went for a
>> different path first, specifically to avoid impact on these hairy bits
>> for non-nested mode.
>>
> Assumption made by those functions should be still correct since guest
> VMCS configuration is not applied directly to real HW, but we should be
> careful of course. For instance interrupt queues should be cleared
> during nested vmexit and event transfered back to idt_vectoring_info_field.
> IIRC this is how nested SVM works BTW.

Checking __vmx_complete_interrupts, the first issue I find is that type
5 (privileged software exception) is not decoded, thus will be lost if
L2 leaves this way. That's a reason why it might be better to re-inject
the content of vmcs12 if it is valid. VMX is a bit more hairy than SVM,
I guess.

> 
> And with you patch you did a half of the job already :) When exiting to
> L0 you transfer event information from get_vmcs12(vcpu)->idt_vectoring_info_field
> to our internal event queues anyway. Hmm, but you do not clear the queue
> during nested vmexit. So what happens if L2 exits to L0 with an exception
> in idt_vectoring_info_field. Now interrupt is delivered so nested vm exit
> is done, but exception is left in internal queue. I think it will be
> delivered into L1 during next vmentry.

Indeed. The queue is only cleared on L2->L0 exits (via the late
vmx_complete_interrupts). It should be cleared on L2->L1 exists as well.
Will fix.

Jan



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