On Mon, May 06, 2024 at 05:29:35AM +0000, Mingwei Zhang wrote:
> From: Xiong Zhang <xiong.y.zhang@xxxxxxxxxxxxxxx>
>
> KVM needs to register irq handler for POSTED_INTR_WAKEUP_VECTOR and
> KVM_GUEST_PMI_VECTOR, a common function x86_set_kvm_irq_handler() is
> extracted to reduce exports function and duplicated code.
>
> Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> Signed-off-by: Xiong Zhang <xiong.y.zhang@xxxxxxxxxxxxxxx>
> ---
> arch/x86/include/asm/irq.h | 3 +--
> arch/x86/kernel/irq.c | 27 +++++++++++----------------
> arch/x86/kvm/vmx/vmx.c | 4 ++--
> 3 files changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/arch/x86/include/asm/irq.h b/arch/x86/include/asm/irq.h
> index 2483f6ef5d4e..050a247b69b4 100644
> --- a/arch/x86/include/asm/irq.h
> +++ b/arch/x86/include/asm/irq.h
> @@ -30,8 +30,7 @@ struct irq_desc;
> extern void fixup_irqs(void);
>
> #if IS_ENABLED(CONFIG_KVM)
> -extern void kvm_set_posted_intr_wakeup_handler(void (*handler)(void));
> -void kvm_set_guest_pmi_handler(void (*handler)(void));
> +void x86_set_kvm_irq_handler(u8 vector, void (*handler)(void));
> #endif
>
> extern void (*x86_platform_ipi_callback)(void);
> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> index 22c10e5c50af..3ada69c50951 100644
> --- a/arch/x86/kernel/irq.c
> +++ b/arch/x86/kernel/irq.c
> @@ -302,27 +302,22 @@ static void dummy_handler(void) {}
> static void (*kvm_posted_intr_wakeup_handler)(void) = dummy_handler;
> static void (*kvm_guest_pmi_handler)(void) = dummy_handler;
>
> -void kvm_set_posted_intr_wakeup_handler(void (*handler)(void))
> +void x86_set_kvm_irq_handler(u8 vector, void (*handler)(void))
> {
> - if (handler)
> + if (!handler)
> + handler = dummy_handler;
> +
> + if (vector == POSTED_INTR_WAKEUP_VECTOR)
> kvm_posted_intr_wakeup_handler = handler;
> - else {
> - kvm_posted_intr_wakeup_handler = dummy_handler;
> - synchronize_rcu();
> - }
> -}
> -EXPORT_SYMBOL_GPL(kvm_set_posted_intr_wakeup_handler);
> -
> -void kvm_set_guest_pmi_handler(void (*handler)(void))
> -{
> - if (handler) {
> + else if (vector == KVM_GUEST_PMI_VECTOR)
> kvm_guest_pmi_handler = handler;
> - } else {
> - kvm_guest_pmi_handler = dummy_handler;
> + else
> + WARN_ON_ONCE(1);
> +
> + if (handler == dummy_handler)
> synchronize_rcu();
> - }
> }
> -EXPORT_SYMBOL_GPL(kvm_set_guest_pmi_handler);
> +EXPORT_SYMBOL_GPL(x86_set_kvm_irq_handler);
Can't you just squash this into the previous patch? I mean, what's the
point of this back and forth?
> +void x86_set_kvm_irq_handler(u8 vector, void (*handler)(void))
> {
> + if (!handler)
> + handler = dummy_handler;
> +
> + if (vector == POSTED_INTR_WAKEUP_VECTOR)
> kvm_posted_intr_wakeup_handler = handler;
> + else if (vector == KVM_GUEST_PMI_VECTOR)
> kvm_guest_pmi_handler = handler;
> + else
> + WARN_ON_ONCE(1);
> +
> + if (handler == dummy_handler)
> synchronize_rcu();
> }
> +EXPORT_SYMBOL_GPL(x86_set_kvm_irq_handler);
So what about:
x86_set_kvm_irq_handler(foo, handler1);
x86_set_kvm_irq_handler(foo, handler2);
?
I'm fairly sure you either want to enforce a NULL<->handler transition,
or add some additional synchronize stuff.
Hmm?
