x86/APIC: make a few interrupt handler functions static

[PATCH] x86/APIC: make a few interrupt handler functions static

Posted by Jan Beulich 3 years, 2 months ago

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);

Re: [PATCH] x86/APIC: make a few interrupt handler functions static

Posted by Roger Pau Monné 3 years, 2 months ago

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.

Re: [PATCH] x86/APIC: make a few interrupt handler functions static

Posted by Andrew Cooper 3 years, 2 months ago

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

Re: [PATCH] x86/APIC: make a few interrupt handler functions static

Posted by Jan Beulich 3 years, 2 months ago

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