Re: [RFC PATCH 3/4] KVM: x86: Add EOI exit bitmap inference

[Steve Rutherford <srutherford@google.com>]

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