Re: [PATCH 1/3] KVM: x86: Move kvm_(un)register_irq_mask_notifier() to generic KVM

[Sean Christopherson <seanjc@google.com>]

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?