On Wed, May 13, 2015 at 10:04:53AM +0200, Paolo Bonzini wrote:
>
>
> On 13/05/2015 08:12, Jan Kiszka wrote:
> >> +void kvm_scan_ioapic_routes(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
> >> +{
> >> + struct kvm *kvm = vcpu->kvm;
> >> + struct kvm_kernel_irq_routing_entry *entry;
> >> + struct kvm_irq_routing_table *table;
> >> + u32 i, nr_rt_entries;
> >> +
> >> + mutex_lock(&kvm->irq_lock);
>
> This only needs irq_srcu protection, not irq_lock, so the lookup cost
> becomes much smaller (all CPUs can proceed in parallel).
I've updated the next iteration to include this.
>
> You would need to put an smp_mb here, to ensure that irq_routing is read
> after KVM_SCAN_IOAPIC is cleared. You can introduce
> smb_mb__after_srcu_read_lock in order to elide it.
>
> The matching memory barrier would be a smp_mb__before_atomic in
> kvm_make_scan_ioapic_request.
Wait, what could happen if KVM_SCAN_IOAPIC is cleared after irq_routing is read? I'm imagining the following, but I'm not sure it makes sense.
After reading irq_routing, another routing update could roll through (which would be followed the setting of KVM_SCAN_IOAPIC), both of which could be followed by the reordered clearing of KVM_SCAN_IOAPIC. This would lead to only one scan, which would use the stale value for kvm->irq_routing.
The reading of kvm->irq_routing (and the reads in srcu_read_lock) should be able to be reordered back to before the clearing of the bit in vcpu->requests (but after the read of vcpu->requests), because reads can be reordered with unrelated writes, but not with other reads. If a routing update came through on another cpu during this window, the issue above could happen.
Adding a memory barrier (but not an wmb or an rmb) before reading irq_routing (ideally not in the srcu critical section) should prevent this from happening?
Sorry if this is long winded. Memory ordering always feels subtle.
Steve
--
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
