Re: [PATCH] KVM: VMX: Fix race between pending IRQ and NMI

[Avi Kivity <avi@redhat.com>]

Jan Kiszka wrote:
>> The complexity, as shown by the decision tree, is exploding, however.
>>     
>
> Yes, I had the same impression while gluing the fix together. And I also
> wondered how things may look like for SVM once we support NMIs there as
> well. However, I didn't want to add a new regression source by
> refactoring this sensitive code at this point.
>   

Right.  We want a short-term fix (your patch, or perhaps some simplified 
version thereof) and a long-term fix which will be simpler to understand.


>> I wonder whether this patch can be simplified, and what to do in the
>> long term.
>>
>> First, this clearly has to be in subarch independent code, it's too
>> complex to live in vmx.c.  Second, how about this:
>>
>> - create an 'event' object, that can be an exception, interrupt, or
>> nmi.  An event can be injected or pending; its other attributes are
>> vector number, has_error, and exception code.
>> - an event has 'inject' and 'check_injected' methods
>> - events live in a queue, either a real array or simply three entries
>> for the three types which are checked in order of precedence, like now
>> - the pre-entry logic looks like
>>
>> if (!injected_event && queue_len > 0)
>>    injected_event = queue_head()
>> if (injected_event)
>>    injected_event.inject()
>>
>> - the post-exit logic looks like
>>
>> if (injected_event)
>>    if (injected_event.check_injected())
>>        injected_event = null
>>        queue_pop()
>>
>> The queue is resorted only when injected_event = null.
>>     
>
> Don't understand if I got the last sentence correctly, 

So long as an event is being injected, we don't want to mess with the 
priority queue.

> but generally
> this sounds nice. Let's try to express my understanding:
>
> We have an event class of which at most one object is instantiated per
> type (which are: exception, NMI, IRQ), ie. we will never have more than
> 3 instances at the same time. These objects are kept in queue, _always_
> sorted according to their priority (in descending order: exception, NMI,
> IRQ). On pre-entry, the topmost object is taken and injected to the
> guest. The queue may only change after the post-exit check (and before
> the pre-entry injection, of course) so that we can track if some object
> finally has been successfully injected and can be deleted from the queue
> (or replaced with a new one, provided by its backing source). Did I get
> your idea correctly?
>   

Yes.

Reflecting some more, there is some talk in SDM 3A 5.9 about handling 
simultaneous exceptions, and MAY want to use the queue to resolve this 
according to the rules provided there.  On the other hand, we MAY want 
to tear 5.9 our of the manual and live with the existing complexity.

Even more, we want to be able to remove an event that is in the queue 
but has not been injected yet.  This is useful for svm, where you can 
queue an interrupt when it is masked by eflags.if or apic.tpr, and svm 
will inject it when it becomes unmasked.  In other words, svm implements 
part of our queue in hardware.

So when an irq line is asserted, the ioapic locates the victim lapic, 
sets the IRR bit, and IPIs it.  This forces an exit; the uninjected 
interrupt is placed back in the queue.  The lapic code sees a higher 
priority interrupt, yanks the lower interrupt from the queue, replaces 
it with the new interrupt, and we go back to the guest.

>> Thoughts?  I think this will close the race as well as simplify things. 
>> It's similar to the current logic, except that the if ()s are taken out
>> of the context of individual events and placed into a generic context,
>> once.
>>
>> I also want to think about the short term solution.  If you can think of
>> a simpler way to close the race, I'd like to hear of it.
>>     
>
> Frankly, not at the moment. This patch tries to extend the existing
> logic only as far as required to fix the issue, while keeping the rest
> as is to avoid breaking some other corner case. In short: conservative
> fixing. Unless someone (/me is busy the next weeks) pulls a patch to
> implement your refactoring idea out of some hat, I would suggest to
> apply my patch for now, but keep the rework on the todo list.
>   

As mentioned above, that's my plan.  I'll see if I can simplify it a 
bit.  Is there an easy way to test it?

-- 
error compiling committee.c: too many arguments to function