Hi Eric, On 8/9/22 10:43 PM, Eric Auger wrote: > 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? That would be quite different: it would be "masked on at least one of both", while I want "masked on both (if both are used)". > >> + 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() Actually I think it would still be ok to call it after kvm_register_irq_ack_notifier(), not necessarily before. We could then miss a mask notification between kvm_register_irq_ack_notifier() and kvm_register_and_fire_irq_mask_notifier(), but it's ok since kvm_register_and_fire_irq_mask_notifier() would then immediately send a new notification with the up-to-date mask value. > >> + * >> + * 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). Ok. > > 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) >> { >
