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 the CPU is enabling this. But the latter is quickly resolved because all such TPR changes in userspace APIC mode are trapped and then processed under BQL. At that point we will also reevaluate the pending interrupts and inject what was deferred before (kvm_arch_post_run -> cpu_set_apic_tpr -> apic_set_tpr -> apic_update_irq, or via apic_mem_writel). So where is a race? Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux -- 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
