From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755930Ab2HOQrl (ORCPT ); Wed, 15 Aug 2012 12:47:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:21718 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755868Ab2HOQrj (ORCPT ); Wed, 15 Aug 2012 12:47:39 -0400 Message-ID: <1345049258.4683.394.camel@ul30vt.home> Subject: Re: [PATCH v8 1/6] kvm: Allow filtering of acked irqs From: Alex Williamson To: "Michael S. Tsirkin" Cc: avi@redhat.com, gleb@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Date: Wed, 15 Aug 2012 10:47:38 -0600 In-Reply-To: <20120815122708.GA3068@redhat.com> References: <20120810223633.809.44188.stgit@bling.home> <20120810223714.809.25474.stgit@bling.home> <20120815122708.GA3068@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2012-08-15 at 15:27 +0300, Michael S. Tsirkin wrote: > On Fri, Aug 10, 2012 at 04:37:17PM -0600, Alex Williamson wrote: > > Registering an kvm_irq_ack_notifier with kian.irq_source_id < 0 > > retains existing behavior, filling in the actual irq_source_id results > > in the callback only being called when the specified irq_source_id is > > asserting the given gsi. > > > > The i8254 PIT remains unfiltered because it de-asserts it's irq source > > id, so it's notifier would never get called otherwise. KVM device > > assignment gets filtering as it de-asserts the GSI in it's notifier. > > > > Signed-off-by: Alex Williamson > > Looks good to me. For the record, I expect this to help if > - an assigned device interrupt is shared in host > so we use slow config cycles in the ack notifier > - said device is sharing interrupt with another device in guest > - said another device is actually driving most interrupts > For example, I think this could be tested > by booting guest with pci=nomsi. Yes, that's how I do most of my testing of legacy interrupts with vfio-pci. KVM assignment is smart enough not to do config access unless the irq is marked as disabled, but it does eliminate an unnecessary de-assert and a couple spinlocks in assignment code. > A minor suggestions below but > nothing that needs to block this patch. > > > --- > > > > arch/x86/kvm/i8254.c | 1 + > > arch/x86/kvm/i8259.c | 8 +++++++- > > include/linux/kvm_host.h | 4 +++- > > virt/kvm/assigned-dev.c | 1 + > > virt/kvm/ioapic.c | 5 ++++- > > virt/kvm/irq_comm.c | 6 ++++-- > > 6 files changed, 20 insertions(+), 5 deletions(-) > > > > diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c > > index adba28f..2355d19 100644 > > --- a/arch/x86/kvm/i8254.c > > +++ b/arch/x86/kvm/i8254.c > > @@ -709,6 +709,7 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags) > > hrtimer_init(&pit_state->pit_timer.timer, > > CLOCK_MONOTONIC, HRTIMER_MODE_ABS); > > pit_state->irq_ack_notifier.gsi = 0; > > + pit_state->irq_ack_notifier.irq_source_id = -1; /* No filter */ > > A bit prettier would be to > #define KVM_NO_IRQ_SOURCE_ID (-1) > and test for it explicitly. Ok. I'll add a define and resend this one. Thanks, Alex > > pit_state->irq_ack_notifier.irq_acked = kvm_pit_ack_irq; > > kvm_register_irq_ack_notifier(kvm, &pit_state->irq_ack_notifier); > > pit_state->pit_timer.reinject = true; > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c > > index e498b18..d2175a9 100644 > > --- a/arch/x86/kvm/i8259.c > > +++ b/arch/x86/kvm/i8259.c > > @@ -74,9 +74,14 @@ static void pic_unlock(struct kvm_pic *s) > > > > static void pic_clear_isr(struct kvm_kpic_state *s, int irq) > > { > > + unsigned long irq_source_ids; > > + > > s->isr &= ~(1 << irq); > > if (s != &s->pics_state->pics[0]) > > irq += 8; > > + > > + irq_source_ids = s->pics_state->irq_states[irq]; > > + > > /* > > * We are dropping lock while calling ack notifiers since ack > > * notifier callbacks for assigned devices call into PIC recursively. > > @@ -84,7 +89,8 @@ static void pic_clear_isr(struct kvm_kpic_state *s, int irq) > > * it should be safe since PIC state is already updated at this stage. > > */ > > pic_unlock(s->pics_state); > > - kvm_notify_acked_irq(s->pics_state->kvm, SELECT_PIC(irq), irq); > > + kvm_notify_acked_irq(s->pics_state->kvm, SELECT_PIC(irq), irq, > > + irq_source_ids); > > pic_lock(s->pics_state); > > } > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > index b70b48b..2ad3e4a 100644 > > --- a/include/linux/kvm_host.h > > +++ b/include/linux/kvm_host.h > > @@ -577,6 +577,7 @@ int kvm_is_mmio_pfn(pfn_t pfn); > > > > struct kvm_irq_ack_notifier { > > struct hlist_node link; > > + int irq_source_id; > > unsigned gsi; > > void (*irq_acked)(struct kvm_irq_ack_notifier *kian); > > }; > > @@ -627,7 +628,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic, > > int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level); > > int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm, > > int irq_source_id, int level); > > -void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin); > > +void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin, > > + unsigned long irq_source_ids); > > void kvm_register_irq_ack_notifier(struct kvm *kvm, > > struct kvm_irq_ack_notifier *kian); > > void kvm_unregister_irq_ack_notifier(struct kvm *kvm, > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c > > index 23a41a9..a08c9c1 100644 > > --- a/virt/kvm/assigned-dev.c > > +++ b/virt/kvm/assigned-dev.c > > @@ -407,6 +407,7 @@ static int assigned_device_enable_guest_intx(struct kvm *kvm, > > { > > dev->guest_irq = irq->guest_irq; > > dev->ack_notifier.gsi = irq->guest_irq; > > + dev->ack_notifier.irq_source_id = dev->irq_source_id; > > return 0; > > } > > > > diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c > > index ef61d52..1a9f445 100644 > > --- a/virt/kvm/ioapic.c > > +++ b/virt/kvm/ioapic.c > > @@ -241,10 +241,12 @@ static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector, > > > > for (i = 0; i < IOAPIC_NUM_PINS; i++) { > > union kvm_ioapic_redirect_entry *ent = &ioapic->redirtbl[i]; > > + unsigned long irq_source_ids; > > > > if (ent->fields.vector != vector) > > continue; > > > > + irq_source_ids = ioapic->irq_states[i]; > > /* > > * We are dropping lock while calling ack notifiers because ack > > * notifier callbacks for assigned devices call into IOAPIC > > @@ -254,7 +256,8 @@ static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector, > > * after ack notifier returns. > > */ > > spin_unlock(&ioapic->lock); > > - kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i); > > + kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i, > > + irq_source_ids); > > spin_lock(&ioapic->lock); > > > > if (trigger_mode != IOAPIC_LEVEL_TRIG) > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c > > index 83402d7..7d75126 100644 > > --- a/virt/kvm/irq_comm.c > > +++ b/virt/kvm/irq_comm.c > > @@ -173,7 +173,8 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level) > > return ret; > > } > > > > -void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin) > > +void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin, > > + unsigned long irq_source_ids) > > { > > struct kvm_irq_ack_notifier *kian; > > struct hlist_node *n; > > @@ -186,7 +187,8 @@ void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin) > > if (gsi != -1) > > hlist_for_each_entry_rcu(kian, n, &kvm->irq_ack_notifier_list, > > link) > > - if (kian->gsi == gsi) > > + if (kian->gsi == gsi && (kian->irq_source_id < 0 || > > + test_bit(kian->irq_source_id, &irq_source_ids))) > > kian->irq_acked(kian); > > rcu_read_unlock(); > > } >