Hi Dmytro, On 8/5/22 21:39, Dmytro Maluka wrote: > In order to implement postponing resamplefd notification until an > interrupt is unmasked, we need not only to track changes of the > interrupt mask state (which is already possible with > kvm_register_irq_mask_notifier()) but also to know its initial > mask state before any mask notifier has fired. > > Moreover, we need to do this initial check of the IRQ mask state in a > race-free way, to ensure that we will not miss any further mask or > unmask events after we check the initial mask state. > > So implement kvm_register_and_fire_irq_mask_notifier() which atomically > registers an IRQ mask notifier and calls it with the current mask value > of the IRQ. It does that using the same locking order as when calling > notifier normally via kvm_fire_mask_notifiers(), to prevent deadlocks. > > Its implementation needs to be arch-specific since it relies on > arch-specific synchronization (e.g. ioapic->lock and pic->lock on x86, > or a per-IRQ lock on ARM vGIC) for serializing our initial reading of > the IRQ mask state with a pending change of this mask state. > > For now implement it for x86 only, and for other archs add a weak dummy > implementation which doesn't really call the notifier (as other archs > don't currently implement calling notifiers normally via > kvm_fire_mask_notifiers() either, i.e. registering mask notifiers has no > effect on those archs anyway). > > Signed-off-by: Dmytro Maluka <dmy@xxxxxxxxxxxx> > Link: https://lore.kernel.org/lkml/c7b7860e-ae3a-7b98-e97e-28a62470c470@xxxxxxxxxxxx/ > --- > arch/x86/include/asm/kvm_host.h | 1 + > arch/x86/kvm/i8259.c | 6 ++++ > arch/x86/kvm/ioapic.c | 6 ++++ > arch/x86/kvm/ioapic.h | 1 + > arch/x86/kvm/irq_comm.c | 57 +++++++++++++++++++++++++++++++++ > include/linux/kvm_host.h | 4 +++ > virt/kvm/eventfd.c | 31 ++++++++++++++++-- > 7 files changed, 104 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index dc76617f11c1..cf0571ed2968 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1834,6 +1834,7 @@ static inline int __kvm_irq_line_state(unsigned long *irq_state, > > int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level); > void kvm_pic_clear_all(struct kvm_pic *pic, int irq_source_id); > +bool kvm_pic_irq_is_masked(struct kvm_pic *s, int irq); > > void kvm_inject_nmi(struct kvm_vcpu *vcpu); > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c > index e1bb6218bb96..1eb3127f6047 100644 > --- a/arch/x86/kvm/i8259.c > +++ b/arch/x86/kvm/i8259.c > @@ -211,6 +211,12 @@ void kvm_pic_clear_all(struct kvm_pic *s, int irq_source_id) > pic_unlock(s); > } > > +/* Called with s->lock held. */ > +bool kvm_pic_irq_is_masked(struct kvm_pic *s, int irq) > +{ > + return !!(s->pics[irq >> 3].imr & (1 << irq)); > +} > + > /* > * acknowledge interrupt 'irq' > */ > diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c > index 765943d7cfa5..fab11de1f885 100644 > --- a/arch/x86/kvm/ioapic.c > +++ b/arch/x86/kvm/ioapic.c > @@ -478,6 +478,12 @@ void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id) > spin_unlock(&ioapic->lock); > } > > +/* Called with ioapic->lock held. */ > +bool kvm_ioapic_irq_is_masked(struct kvm_ioapic *ioapic, int irq) > +{ > + return !!ioapic->redirtbl[irq].fields.mask; > +} > + > static void kvm_ioapic_eoi_inject_work(struct work_struct *work) > { > int i; > diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h > index 539333ac4b38..fe1f51319992 100644 > --- a/arch/x86/kvm/ioapic.h > +++ b/arch/x86/kvm/ioapic.h > @@ -114,6 +114,7 @@ void kvm_ioapic_destroy(struct kvm *kvm); > int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id, > int level, bool line_status); > void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id); > +bool kvm_ioapic_irq_is_masked(struct kvm_ioapic *ioapic, int irq); > void kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state); > void kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state); > void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, > diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c > index f27e4c9c403e..4bd4218821a2 100644 > --- a/arch/x86/kvm/irq_comm.c > +++ b/arch/x86/kvm/irq_comm.c > @@ -234,6 +234,63 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id) > mutex_unlock(&kvm->irq_lock); > } > > +void kvm_register_and_fire_irq_mask_notifier(struct kvm *kvm, int irq, > + struct kvm_irq_mask_notifier *kimn) > +{ > + struct kvm_pic *pic = kvm->arch.vpic; > + struct kvm_ioapic *ioapic = kvm->arch.vioapic; > + struct kvm_kernel_irq_routing_entry entries[KVM_NR_IRQCHIPS]; > + struct kvm_kernel_irq_routing_entry *pic_e = NULL, *ioapic_e = NULL; > + int idx, i, n; > + bool masked; > + > + mutex_lock(&kvm->irq_lock); > + > + /* > + * Not possible to detect if the guest uses the PIC or the > + * IOAPIC. So assume the interrupt to be unmasked iff it is > + * unmasked in at least one of both. > + */ > + idx = srcu_read_lock(&kvm->irq_srcu); > + n = kvm_irq_map_gsi(kvm, entries, irq); > + srcu_read_unlock(&kvm->irq_srcu, idx); > + > + for (i = 0; i < n; i++) { > + if (entries[i].type != KVM_IRQ_ROUTING_IRQCHIP) > + continue; > + > + switch (entries[i].irqchip.irqchip) { > + case KVM_IRQCHIP_PIC_MASTER: > + case KVM_IRQCHIP_PIC_SLAVE: > + pic_e = &entries[i]; > + break; > + case KVM_IRQCHIP_IOAPIC: > + ioapic_e = &entries[i]; > + break; > + default: > + break; > + } > + } > + > + if (pic_e) > + spin_lock(&pic->lock); > + if (ioapic_e) > + spin_lock(&ioapic->lock); > + > + __kvm_register_irq_mask_notifier(kvm, irq, kimn); > + > + masked = (!pic_e || kvm_pic_irq_is_masked(pic, pic_e->irqchip.pin)) && > + (!ioapic_e || kvm_ioapic_irq_is_masked(ioapic, ioapic_e->irqchip.pin)); Looks a bit cryptic to me. Don't you want pic_e && masked on pic || ioapic_e && masked on ioapic? > + kimn->func(kimn, masked); > + > + if (ioapic_e) > + spin_unlock(&ioapic->lock); > + if (pic_e) > + spin_unlock(&pic->lock); > + > + mutex_unlock(&kvm->irq_lock); > +} > + > bool kvm_arch_can_set_irq_routing(struct kvm *kvm) > { > return irqchip_in_kernel(kvm); > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index dd5f14e31996..55233eb18eb4 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -1608,8 +1608,12 @@ void kvm_register_irq_ack_notifier(struct kvm *kvm, > struct kvm_irq_ack_notifier *kian); > void kvm_unregister_irq_ack_notifier(struct kvm *kvm, > struct kvm_irq_ack_notifier *kian); > +void __kvm_register_irq_mask_notifier(struct kvm *kvm, int irq, > + struct kvm_irq_mask_notifier *kimn); > void kvm_register_irq_mask_notifier(struct kvm *kvm, int irq, > struct kvm_irq_mask_notifier *kimn); > +void kvm_register_and_fire_irq_mask_notifier(struct kvm *kvm, int irq, > + struct kvm_irq_mask_notifier *kimn); > void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq, > struct kvm_irq_mask_notifier *kimn); > void kvm_fire_mask_notifiers(struct kvm *kvm, unsigned irqchip, unsigned pin, > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c > index 39403d9fbdcc..3007d956b626 100644 > --- a/virt/kvm/eventfd.c > +++ b/virt/kvm/eventfd.c > @@ -519,15 +519,42 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm, > kvm_arch_post_irq_ack_notifier_list_update(kvm); > } > > +void __kvm_register_irq_mask_notifier(struct kvm *kvm, int irq, > + struct kvm_irq_mask_notifier *kimn) > +{ > + kimn->irq = irq; > + hlist_add_head_rcu(&kimn->link, &kvm->irq_mask_notifier_list); > +} > + > void kvm_register_irq_mask_notifier(struct kvm *kvm, int irq, > struct kvm_irq_mask_notifier *kimn) > { > mutex_lock(&kvm->irq_lock); > - kimn->irq = irq; > - hlist_add_head_rcu(&kimn->link, &kvm->irq_mask_notifier_list); > + __kvm_register_irq_mask_notifier(kvm, irq, kimn); > mutex_unlock(&kvm->irq_lock); > } > > +/* > + * kvm_register_and_fire_irq_mask_notifier() registers the notifier and > + * immediately calls it with the current mask value of the IRQ. It does > + * that atomically, so that we will find out the initial mask state of > + * the IRQ and will not miss any further mask or unmask events. It does > + * that using the same locking order as when calling notifier normally > + * via kvm_fire_mask_notifiers(), to prevent deadlocks. you may document somewhere that it must be called before kvm_register_irq_ack_notifier() > + * > + * Implementation is arch-specific since it relies on arch-specific > + * (irqchip-specific) synchronization. Below is a weak dummy > + * implementation for archs not implementing it yet, as those archs > + * don't implement calling notifiers normally via > + * kvm_fire_mask_notifiers() either, i.e. registering mask notifiers > + * has no effect on those archs anyway. I would advise you to put Marc in the loop for the whole series (adding him in CC). Thanks Eric > + */ > +void __weak kvm_register_and_fire_irq_mask_notifier(struct kvm *kvm, int irq, > + struct kvm_irq_mask_notifier *kimn) > +{ > + kvm_register_irq_mask_notifier(kvm, irq, kimn); > +} > + > void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq, > struct kvm_irq_mask_notifier *kimn) > {
