Four of them are used in apic.c only and hence better wouldn't be
exposed to other CUs. To avoid the need for forward declarations, move
apic_intr_init() past the four handlers.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -127,21 +127,6 @@ void ack_bad_irq(unsigned int irq)
ack_APIC_irq();
}
-void __init apic_intr_init(void)
-{
- smp_intr_init();
-
- /* self generated IPI for local APIC timer */
- set_direct_apic_vector(LOCAL_TIMER_VECTOR, apic_timer_interrupt);
-
- /* IPI vectors for APIC spurious and error interrupts */
- set_direct_apic_vector(SPURIOUS_APIC_VECTOR, spurious_interrupt);
- set_direct_apic_vector(ERROR_APIC_VECTOR, error_interrupt);
-
- /* Performance Counters Interrupt */
- set_direct_apic_vector(PMU_APIC_VECTOR, pmu_apic_interrupt);
-}
-
/* Using APIC to generate smp_local_timer_interrupt? */
static bool __read_mostly using_apic_timer;
@@ -1363,7 +1348,7 @@ int reprogram_timer(s_time_t timeout)
return apic_tmict || !timeout;
}
-void cf_check apic_timer_interrupt(struct cpu_user_regs *regs)
+static void cf_check apic_timer_interrupt(struct cpu_user_regs *regs)
{
ack_APIC_irq();
perfc_incr(apic_timer);
@@ -1382,7 +1367,7 @@ void smp_send_state_dump(unsigned int cp
/*
* Spurious interrupts should _never_ happen with our APIC/SMP architecture.
*/
-void cf_check spurious_interrupt(struct cpu_user_regs *regs)
+static void cf_check spurious_interrupt(struct cpu_user_regs *regs)
{
/*
* Check if this is a vectored interrupt (most likely, as this is probably
@@ -1413,7 +1398,7 @@ void cf_check spurious_interrupt(struct
* This interrupt should never happen with our APIC/SMP architecture
*/
-void cf_check error_interrupt(struct cpu_user_regs *regs)
+static void cf_check error_interrupt(struct cpu_user_regs *regs)
{
static const char *const esr_fields[] = {
"Send CS error",
@@ -1446,12 +1431,27 @@ void cf_check error_interrupt(struct cpu
* This interrupt handles performance counters interrupt
*/
-void cf_check pmu_apic_interrupt(struct cpu_user_regs *regs)
+static void cf_check pmu_apic_interrupt(struct cpu_user_regs *regs)
{
ack_APIC_irq();
vpmu_do_interrupt(regs);
}
+void __init apic_intr_init(void)
+{
+ smp_intr_init();
+
+ /* self generated IPI for local APIC timer */
+ set_direct_apic_vector(LOCAL_TIMER_VECTOR, apic_timer_interrupt);
+
+ /* IPI vectors for APIC spurious and error interrupts */
+ set_direct_apic_vector(SPURIOUS_APIC_VECTOR, spurious_interrupt);
+ set_direct_apic_vector(ERROR_APIC_VECTOR, error_interrupt);
+
+ /* Performance Counters Interrupt */
+ set_direct_apic_vector(PMU_APIC_VECTOR, pmu_apic_interrupt);
+}
+
/*
* This initializes the IO-APIC and APIC hardware if this is
* a UP kernel.
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -96,10 +96,6 @@ static inline struct cpu_user_regs *set_
void cf_check event_check_interrupt(struct cpu_user_regs *regs);
void cf_check invalidate_interrupt(struct cpu_user_regs *regs);
void cf_check call_function_interrupt(struct cpu_user_regs *regs);
-void cf_check apic_timer_interrupt(struct cpu_user_regs *regs);
-void cf_check error_interrupt(struct cpu_user_regs *regs);
-void cf_check pmu_apic_interrupt(struct cpu_user_regs *regs);
-void cf_check spurious_interrupt(struct cpu_user_regs *regs);
void cf_check irq_move_cleanup_interrupt(struct cpu_user_regs *regs);
uint8_t alloc_hipriority_vector(void);
On Tue, Nov 29, 2022 at 03:46:30PM +0100, Jan Beulich wrote: > Four of them are used in apic.c only and hence better wouldn't be > exposed to other CUs. To avoid the need for forward declarations, move > apic_intr_init() past the four handlers. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Roger Pau Monné <roger.pau@citrix.com> A nit below. > > --- a/xen/arch/x86/apic.c > +++ b/xen/arch/x86/apic.c > @@ -127,21 +127,6 @@ void ack_bad_irq(unsigned int irq) > ack_APIC_irq(); > } > > -void __init apic_intr_init(void) > -{ > - smp_intr_init(); > - > - /* self generated IPI for local APIC timer */ > - set_direct_apic_vector(LOCAL_TIMER_VECTOR, apic_timer_interrupt); > - > - /* IPI vectors for APIC spurious and error interrupts */ > - set_direct_apic_vector(SPURIOUS_APIC_VECTOR, spurious_interrupt); > - set_direct_apic_vector(ERROR_APIC_VECTOR, error_interrupt); > - > - /* Performance Counters Interrupt */ > - set_direct_apic_vector(PMU_APIC_VECTOR, pmu_apic_interrupt); > -} > - > /* Using APIC to generate smp_local_timer_interrupt? */ > static bool __read_mostly using_apic_timer; > > @@ -1363,7 +1348,7 @@ int reprogram_timer(s_time_t timeout) > return apic_tmict || !timeout; > } > > -void cf_check apic_timer_interrupt(struct cpu_user_regs *regs) > +static void cf_check apic_timer_interrupt(struct cpu_user_regs *regs) Given that the function is now not exported out of apic.c, wouldn't it be better to drop the apic_ prefix? The same would likely apply to pmu_apic_interrupt() then. Thanks, Roger.
On 29/11/2022 16:05, Roger Pau Monné wrote: > On Tue, Nov 29, 2022 at 03:46:30PM +0100, Jan Beulich wrote: >> Four of them are used in apic.c only and hence better wouldn't be >> exposed to other CUs. To avoid the need for forward declarations, move >> apic_intr_init() past the four handlers. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > Acked-by: Roger Pau Monné <roger.pau@citrix.com> > > A nit below. > >> --- a/xen/arch/x86/apic.c >> +++ b/xen/arch/x86/apic.c >> @@ -127,21 +127,6 @@ void ack_bad_irq(unsigned int irq) >> ack_APIC_irq(); >> } >> >> -void __init apic_intr_init(void) >> -{ >> - smp_intr_init(); >> - >> - /* self generated IPI for local APIC timer */ >> - set_direct_apic_vector(LOCAL_TIMER_VECTOR, apic_timer_interrupt); >> - >> - /* IPI vectors for APIC spurious and error interrupts */ >> - set_direct_apic_vector(SPURIOUS_APIC_VECTOR, spurious_interrupt); >> - set_direct_apic_vector(ERROR_APIC_VECTOR, error_interrupt); >> - >> - /* Performance Counters Interrupt */ >> - set_direct_apic_vector(PMU_APIC_VECTOR, pmu_apic_interrupt); >> -} >> - >> /* Using APIC to generate smp_local_timer_interrupt? */ >> static bool __read_mostly using_apic_timer; >> >> @@ -1363,7 +1348,7 @@ int reprogram_timer(s_time_t timeout) >> return apic_tmict || !timeout; >> } >> >> -void cf_check apic_timer_interrupt(struct cpu_user_regs *regs) >> +static void cf_check apic_timer_interrupt(struct cpu_user_regs *regs) > Given that the function is now not exported out of apic.c, wouldn't it > be better to drop the apic_ prefix? This is the handler for the apic timer, as opposed to the plethora of other timer interrupts we have elsewhere. Simply "timer interrupt" is too generic a name. > The same would likely apply to pmu_apic_interrupt() then. This one could lose the infix. All PMU interrupts are from an LVT vector. It may have made a different back in the days of non-integrated APICs, but those days are long gone. ~Andrew
On 29.11.2022 17:21, Andrew Cooper wrote: > On 29/11/2022 16:05, Roger Pau Monné wrote: >> On Tue, Nov 29, 2022 at 03:46:30PM +0100, Jan Beulich wrote: >>> Four of them are used in apic.c only and hence better wouldn't be >>> exposed to other CUs. To avoid the need for forward declarations, move >>> apic_intr_init() past the four handlers. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. >> A nit below. >> >>> --- a/xen/arch/x86/apic.c >>> +++ b/xen/arch/x86/apic.c >>> @@ -127,21 +127,6 @@ void ack_bad_irq(unsigned int irq) >>> ack_APIC_irq(); >>> } >>> >>> -void __init apic_intr_init(void) >>> -{ >>> - smp_intr_init(); >>> - >>> - /* self generated IPI for local APIC timer */ >>> - set_direct_apic_vector(LOCAL_TIMER_VECTOR, apic_timer_interrupt); >>> - >>> - /* IPI vectors for APIC spurious and error interrupts */ >>> - set_direct_apic_vector(SPURIOUS_APIC_VECTOR, spurious_interrupt); >>> - set_direct_apic_vector(ERROR_APIC_VECTOR, error_interrupt); >>> - >>> - /* Performance Counters Interrupt */ >>> - set_direct_apic_vector(PMU_APIC_VECTOR, pmu_apic_interrupt); >>> -} >>> - >>> /* Using APIC to generate smp_local_timer_interrupt? */ >>> static bool __read_mostly using_apic_timer; >>> >>> @@ -1363,7 +1348,7 @@ int reprogram_timer(s_time_t timeout) >>> return apic_tmict || !timeout; >>> } >>> >>> -void cf_check apic_timer_interrupt(struct cpu_user_regs *regs) >>> +static void cf_check apic_timer_interrupt(struct cpu_user_regs *regs) >> Given that the function is now not exported out of apic.c, wouldn't it >> be better to drop the apic_ prefix? > > This is the handler for the apic timer, as opposed to the plethora of > other timer interrupts we have elsewhere. > > Simply "timer interrupt" is too generic a name. I agree with Andrew here. >> The same would likely apply to pmu_apic_interrupt() then. > > This one could lose the infix. All PMU interrupts are from an LVT > vector. It may have made a different back in the days of non-integrated > APICs, but those days are long gone. I'm happy to drop the infix. Won't bother sending a v2 just for this, though - I'll simply put the adjusted patch in soon after the tree has reopened. Jan
© 2016 - 2024 Red Hat, Inc.