Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754207Ab2HOM0G (ORCPT ); Wed, 15 Aug 2012 08:26:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:64211 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751691Ab2HOM0E (ORCPT ); Wed, 15 Aug 2012 08:26:04 -0400 Date: Wed, 15 Aug 2012 15:27:08 +0300 From: "Michael S. Tsirkin" To: Alex Williamson Cc: avi@redhat.com, gleb@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v8 1/6] kvm: Allow filtering of acked irqs Message-ID: <20120815122708.GA3068@redhat.com> References: <20120810223633.809.44188.stgit@bling.home> <20120810223714.809.25474.stgit@bling.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120810223714.809.25474.stgit@bling.home> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6687 Lines: 172 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. 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. > 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(); > } -- MST -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/