| Message ID | 20240506053020.3911940-11-mizhang@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Mediated Passthrough vPMU 2.0 for x86 | expand |
On Mon, May 06, 2024 at 05:29:35AM +0000, Mingwei Zhang wrote: > From: Xiong Zhang <xiong.y.zhang@linux.intel.com> > > 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@google.com> > Signed-off-by: Xiong Zhang <xiong.y.zhang@linux.intel.com> > --- > 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?
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@linux.intel.com> >> >> 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@google.com> >> Signed-off-by: Xiong Zhang <xiong.y.zhang@linux.intel.com> >> --- >> 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 > >
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); /* * Handler for POSTED_INTERRUPT_VECTOR. diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index c37a89eda90f..c2dc68a25a53 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -8214,7 +8214,7 @@ static void vmx_migrate_timers(struct kvm_vcpu *vcpu) static void vmx_hardware_unsetup(void) { - kvm_set_posted_intr_wakeup_handler(NULL); + x86_set_kvm_irq_handler(POSTED_INTR_WAKEUP_VECTOR, NULL); if (nested) nested_vmx_hardware_unsetup(); @@ -8679,7 +8679,7 @@ static __init int hardware_setup(void) if (r && nested) nested_vmx_hardware_unsetup(); - kvm_set_posted_intr_wakeup_handler(pi_wakeup_handler); + x86_set_kvm_irq_handler(POSTED_INTR_WAKEUP_VECTOR, pi_wakeup_handler); return r; }