From mboxrd@z Thu Jan  1 00:00:00 1970
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [RFC PATCH 3/4] KVM: x86: Add EOI exit bitmap inference
Date: Wed, 13 May 2015 10:04:53 +0200
Message-ID: <555305A5.8060601@redhat.com>
References: <1431481652-27268-1-git-send-email-srutherford@google.com> <1431481652-27268-3-git-send-email-srutherford@google.com> <5552EB35.2070806@siemens.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7BIT
Cc: ahonig@google.com
To: Jan Kiszka <jan.kiszka@siemens.com>,
	Steve Rutherford <srutherford@google.com>, kvm@vger.kernel.org
Return-path: <kvm-owner@vger.kernel.org>
Received: from mail-wg0-f44.google.com ([74.125.82.44]:36674 "EHLO
	mail-wg0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S933091AbbEMIE7 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 13 May 2015 04:04:59 -0400
Received: by wgbhc8 with SMTP id hc8so1261338wgb.3
        for <kvm@vger.kernel.org>; Wed, 13 May 2015 01:04:58 -0700 (PDT)
In-Reply-To: <5552EB35.2070806@siemens.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>



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