[PATCH v8 1/6] kvm: Allow filtering of acked irqs
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 --- 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 */ 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, +
Re: [PATCH v8 1/6] kvm: Allow filtering of acked irqs
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->r
Re: [PATCH v8 1/6] kvm: Allow filtering of acked irqs
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 >
Re: [PATCH v8 1/6] kvm: Allow filtering of acked irqs
On Wed, Aug 15, 2012 at 10:47:38AM -0600, Alex Williamson wrote: > 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. Hmm this is less than I thought. Any performance numbers? > > 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); > >