On 5/7/2024 5:18 PM, Peter Zijlstra wrote:
> 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?
Ok, I will put this before the previous patch, and let previous patch use
this directly.
>
>> +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?
yes, x86_set_kvm_irq_handler() is called once for each vector at
kvm/kvm_intel module_init() and module_exit(). so we should enforce a
NULL<->handler transition.
thanks
>
>
