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

[Paolo Bonzini <pbonzini@redhat.com>]

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).

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.

>> +	table = kvm->irq_routing;
>> +	nr_rt_entries = min_t(u32, table->nr_rt_entries, IOAPIC_NUM_PINS);
>> +	for (i = 0; i < nr_rt_entries; ++i) {
>> +		hlist_for_each_entry(entry, &table->map[i], link) {
>> +			u32 dest_id, dest_mode;
>> +
>> +			if (entry->type != KVM_IRQ_ROUTING_MSI)
>> +				continue;
>> +			dest_id = (entry->msi.address_lo >> 12) & 0xff;
>> +			dest_mode = (entry->msi.address_lo >> 2) & 0x1;
>> +			if (kvm_apic_match_dest(vcpu, NULL, 0, dest_id,
>> +						dest_mode)) {
>> +				u32 vector = entry->msi.data & 0xff;
>> +
>> +				__set_bit(vector,
>> +					  (unsigned long *) eoi_exit_bitmap);
>> +			}
>> +		}
>> +	}
>> +	mutex_unlock(&kvm->irq_lock);
>> +}
>>
> 
> This looks a bit frightening regarding the lookup costs. Do we really
> have to run through the complete routing table to find the needed
> information? There can be way more "real" MSI entries than IOAPIC pins.

It does at most IOAPIC_NUM_PINS iterations however.

> There can even be multiple IOAPICs (thanks to your patches overcoming
> the single in-kernel instance).

With multiple IOAPICs you have more than 24 GSIs per IOAPIC.  That means
that the above loop is broken for multiple IOAPICs.

But perhaps when enabling KVM_SPLIT_IRQCHIP we can use args[0] to pass
the number of IOAPIC routes that will cause EOI exits?

Paolo