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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
