| Message ID | 20220715155928.26362-2-dmy@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Fix oneshot interrupts forwarding | expand |
On Fri, Jul 15, 2022, Dmytro Maluka wrote: > In preparation for implementing postponing resamplefd event until the > interrupt is unmasked, move kvm_(un)register_irq_mask_notifier() from > x86 to arch-independent code to make it usable by irqfd. This patch needs to move more than just the helpers, e.g. mask_notifier_list needs to be in "struct kvm", not "stuct kvm_arch". arch/arm64/kvm/../../../virt/kvm/eventfd.c: In function ‘kvm_register_irq_mask_notifier’: arch/arm64/kvm/../../../virt/kvm/eventfd.c:528:51: error: ‘struct kvm_arch’ has no member named ‘mask_notifier_list’ 528 | hlist_add_head_rcu(&kimn->link, &kvm->arch.mask_notifier_list); | ^ make[3]: *** [scripts/Makefile.build:249: arch/arm64/kvm/../../../virt/kvm/eventfd.o] Error 1 make[3]: *** Waiting for unfinished jobs.... AR kernel/entry/built-in.a And kvm_fire_mask_notifiers() should probably be moved as well, otherwise there's no point in moving the registration to common code. The other option would be to make the generic functions wrappers around arch-specific hooks. But IIRC won't this eventually be needed for other architectures?
On 7/28/22 20:46, Sean Christopherson wrote: > On Fri, Jul 15, 2022, Dmytro Maluka wrote: >> In preparation for implementing postponing resamplefd event until the >> interrupt is unmasked, move kvm_(un)register_irq_mask_notifier() from >> x86 to arch-independent code to make it usable by irqfd. > > This patch needs to move more than just the helpers, e.g. mask_notifier_list > needs to be in "struct kvm", not "stuct kvm_arch". > > arch/arm64/kvm/../../../virt/kvm/eventfd.c: In function ‘kvm_register_irq_mask_notifier’: > arch/arm64/kvm/../../../virt/kvm/eventfd.c:528:51: error: ‘struct kvm_arch’ has no member named ‘mask_notifier_list’ > 528 | hlist_add_head_rcu(&kimn->link, &kvm->arch.mask_notifier_list); > | ^ > make[3]: *** [scripts/Makefile.build:249: arch/arm64/kvm/../../../virt/kvm/eventfd.o] Error 1 > make[3]: *** Waiting for unfinished jobs.... > AR kernel/entry/built-in.a Oops, sorry. > And kvm_fire_mask_notifiers() should probably be moved as well, otherwise there's > no point in moving the registration to common code. Good point, we can move it right away, even though it is not called on other architectures for now. > The other option would be to make the generic functions wrappers around arch-specific > hooks. But IIRC won't this eventually be needed for other architectures? Right, I assume we will eventually need it for ARM at least. Not in the near future though, and at the moment I have no non-x86 hardware on hand to implement it for other architectures. Actually I feel a bit uncomfortable with generic irqfd relying on kvm_register_irq_mask_notifier() which silently has no effect on other architectures. Maybe it's better to keep kvm_(un)register_irq_mask_notifier() in the x86 code, and for the generic code add a weak version which e.g. just prints a warning like "irq mask notifiers not implemented on this arch". (Or maybe instead of weak functions introduce arch-specific hooks as you suggested, and print such a warning if no hook is provided.) What do you think? Thanks, Dmytro
On Fri, Jul 29, 2022, Dmytro Maluka wrote: > On 7/28/22 20:46, Sean Christopherson wrote: > > On Fri, Jul 15, 2022, Dmytro Maluka wrote: > >> In preparation for implementing postponing resamplefd event until the > >> interrupt is unmasked, move kvm_(un)register_irq_mask_notifier() from > >> x86 to arch-independent code to make it usable by irqfd. > > > > This patch needs to move more than just the helpers, e.g. mask_notifier_list > > needs to be in "struct kvm", not "stuct kvm_arch". > > > > arch/arm64/kvm/../../../virt/kvm/eventfd.c: In function ‘kvm_register_irq_mask_notifier’: > > arch/arm64/kvm/../../../virt/kvm/eventfd.c:528:51: error: ‘struct kvm_arch’ has no member named ‘mask_notifier_list’ > > 528 | hlist_add_head_rcu(&kimn->link, &kvm->arch.mask_notifier_list); > > | ^ > > make[3]: *** [scripts/Makefile.build:249: arch/arm64/kvm/../../../virt/kvm/eventfd.o] Error 1 > > make[3]: *** Waiting for unfinished jobs.... > > AR kernel/entry/built-in.a > > Oops, sorry. > > > And kvm_fire_mask_notifiers() should probably be moved as well, otherwise there's > > no point in moving the registration to common code. > > Good point, we can move it right away, even though it is not called on > other architectures for now. > > > The other option would be to make the generic functions wrappers around arch-specific > > hooks. But IIRC won't this eventually be needed for other architectures? > > Right, I assume we will eventually need it for ARM at least. Not in the > near future though, and at the moment I have no non-x86 hardware on hand > to implement it for other architectures. > > Actually I feel a bit uncomfortable with generic irqfd relying on > kvm_register_irq_mask_notifier() which silently has no effect on other > architectures. Maybe it's better to keep > kvm_(un)register_irq_mask_notifier() in the x86 code, and for the > generic code add a weak version which e.g. just prints a warning like > "irq mask notifiers not implemented on this arch". (Or maybe instead of > weak functions introduce arch-specific hooks as you suggested, and print > such a warning if no hook is provided.) What do you think? If the entire concept of having mask notifiers is x86 specific, then moving it to generic code obviously doesn't make sense. But if the concept applies to other archictectures, then IMO the list belongs in "struct kvm" with generic, common helpers, even if no other arch calls kvm_fire_mask_notifiers() at this time. Paolo and/or non-x86 folks, any thoughts?
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 9217bd6cf0d1..39a867d68721 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1688,16 +1688,6 @@ int load_pdptrs(struct kvm_vcpu *vcpu, unsigned long cr3); int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa, const void *val, int bytes); -struct kvm_irq_mask_notifier { - void (*func)(struct kvm_irq_mask_notifier *kimn, bool masked); - int irq; - struct hlist_node link; -}; - -void kvm_register_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, bool mask); diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c index 0687162c4f22..43e13892ed34 100644 --- a/arch/x86/kvm/irq_comm.c +++ b/arch/x86/kvm/irq_comm.c @@ -234,24 +234,6 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id) mutex_unlock(&kvm->irq_lock); } -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->arch.mask_notifier_list); - mutex_unlock(&kvm->irq_lock); -} - -void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq, - struct kvm_irq_mask_notifier *kimn) -{ - mutex_lock(&kvm->irq_lock); - hlist_del_rcu(&kimn->link); - mutex_unlock(&kvm->irq_lock); - synchronize_srcu(&kvm->irq_srcu); -} - void kvm_fire_mask_notifiers(struct kvm *kvm, unsigned irqchip, unsigned pin, bool mask) { diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 90a45ef7203b..9e12ef503157 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1581,6 +1581,12 @@ struct kvm_irq_ack_notifier { void (*irq_acked)(struct kvm_irq_ack_notifier *kian); }; +struct kvm_irq_mask_notifier { + void (*func)(struct kvm_irq_mask_notifier *kimn, bool masked); + int irq; + struct hlist_node link; +}; + int kvm_irq_map_gsi(struct kvm *kvm, struct kvm_kernel_irq_routing_entry *entries, int gsi); int kvm_irq_map_chip_pin(struct kvm *kvm, unsigned irqchip, unsigned pin); @@ -1599,6 +1605,10 @@ 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_unregister_irq_mask_notifier(struct kvm *kvm, int irq, + struct kvm_irq_mask_notifier *kimn); int kvm_request_irq_source_id(struct kvm *kvm); void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id); bool kvm_arch_irqfd_allowed(struct kvm *kvm, struct kvm_irqfd *args); diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c index 2a3ed401ce46..50ddb1d1a7f0 100644 --- a/virt/kvm/eventfd.c +++ b/virt/kvm/eventfd.c @@ -520,6 +520,24 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm, } #endif +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->arch.mask_notifier_list); + mutex_unlock(&kvm->irq_lock); +} + +void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq, + struct kvm_irq_mask_notifier *kimn) +{ + mutex_lock(&kvm->irq_lock); + hlist_del_rcu(&kimn->link); + mutex_unlock(&kvm->irq_lock); + synchronize_srcu(&kvm->irq_srcu); +} + void kvm_eventfd_init(struct kvm *kvm) {
In preparation for implementing postponing resamplefd event until the interrupt is unmasked, move kvm_(un)register_irq_mask_notifier() from x86 to arch-independent code to make it usable by irqfd. Note that calling mask notifiers is still implemented for x86 only, so registering mask notifiers on non-x86 will have no effect. Link: https://lore.kernel.org/kvm/31420943-8c5f-125c-a5ee-d2fde2700083@semihalf.com/ Signed-off-by: Dmytro Maluka <dmy@semihalf.com> --- arch/x86/include/asm/kvm_host.h | 10 ---------- arch/x86/kvm/irq_comm.c | 18 ------------------ include/linux/kvm_host.h | 10 ++++++++++ virt/kvm/eventfd.c | 18 ++++++++++++++++++ 4 files changed, 28 insertions(+), 28 deletions(-)