Re: [Qemu-devel] [PATCH] kvm: First step to push iothread lock out of inner run loop

[Avi Kivity]

On 07/06/2012 09:06 PM, Jan Kiszka wrote:
> On 2012-07-06 19:16, Jan Kiszka wrote:
>> On 2012-06-24 16:08, Jan Kiszka wrote:
>>> On 2012-06-24 10:49, Avi Kivity wrote:
>>>> On 06/23/2012 02:45 PM, Jan Kiszka wrote:
>>>>>
>>>>> Hmm, we may need the iothread lock around cpu_set_apic_tpr for
>>>>> !kvm_irqchip_in_kernel(). And as we are at it, apic_base manipulation
>>>>> can be but there as well.
>>>>>
>>>>> With in-kernel irqchip, there is no such need. Also, no one accesses
>>>>> eflags outside of the vcpu thread, independent of the irqchip mode.
>>>>
>>>> In fact !kvm_irqchip_in_kernel() is broken wrt the tpr.  Interrupt
>>>> injection needs to be done atomically, but currently we check the tpr
>>>> from the injecting thread, which means the cpu thread can race with it.
>>>>  We need to move the check to the vcpu thread so that the guest vcpu is
>>>> halted.
>>>
>>> So apic_set_irq basically needs to be deferred to vcpu context, right?
>>> Will have a look.
>> 
>> Tried to wrap my head around this, but only found different issues
>> (patches under construction).
>> 
>> First of all, a simple run_on_cpu doesn't work as it may drops the BQL
>> at unexpected points inside device models.
>> 
>> Then I thought about what could actually race here: The testing of the
>> userspace TPR value under BQL vs. some modification by the CPU while in
>> KVM mode. So we may either inject while the CPU is trying to prevent
>> this - harmless as it happens on real hw as well - or not inject while
> 
> Hmm, this could actually be a problem as the race window is extended
> beyond the instruction that raises TPR. So we may generate unexpected
> spurious interrupts for the guest. And that's because the userspace APIC
> fails to update the CPU interrupt pin properly when the TPR is modified.
> Here is the bug...

Exactly.

To avoid dropping the lock we can do something similar to kvm - queue a
request to reevaluate IRR, then signal that vcpu to exit.  No need to
drop the lock.  Possibly a check in apic_update_irq() whether we're
running in the vcpu thread; if not, signal and return.  That function's
effects are asynchronous if run outside the vcpu, so it's a safe change.


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