Re: [PATCH 3/4] x86/IRQ: direct-APIC-vector setting is now init-only

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/11/2025 10:50 am, Jan Beulich wrote:
> With all callers of alloc_direct_apic_vector() now being limited to BSP
> setup, it and its helpers (whose other callers have already been init-
> only) can become __init. As a result data items can be adjusted, too.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Question is whether with this the "if (*vector == 0)" in
> alloc_direct_apic_vector() is of much use anymore.
>
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -970,22 +970,22 @@ void pirq_set_affinity(struct domain *d,
>  DEFINE_PER_CPU(unsigned int, irq_count);
>  static DEFINE_PER_CPU(bool, check_eoi_deferral);
>  
> -uint8_t alloc_hipriority_vector(void)
> +uint8_t __init alloc_hipriority_vector(void)
>  {
> -    static uint8_t next = FIRST_HIPRIORITY_VECTOR;
> +    static uint8_t __initdata next = FIRST_HIPRIORITY_VECTOR;
>      BUG_ON(next < FIRST_HIPRIORITY_VECTOR);
>      BUG_ON(next > LAST_HIPRIORITY_VECTOR);
>      return next++;
>  }
>  
> -static void (*direct_apic_vector[X86_IDT_VECTORS])(void);
> -void set_direct_apic_vector(uint8_t vector, void (*handler)(void))
> +static void (*__ro_after_init direct_apic_vector[X86_IDT_VECTORS])(void);
> +void __init set_direct_apic_vector(uint8_t vector, void (*handler)(void))
>  {
>      BUG_ON(direct_apic_vector[vector] != NULL);
>      direct_apic_vector[vector] = handler;
>  }
>  
> -void alloc_direct_apic_vector(uint8_t *vector, void (*handler)(void))
> +void __init alloc_direct_apic_vector(uint8_t *vector, void (*handler)(void))
>  {
>      static DEFINE_SPINLOCK(lock);
>  
>

This function, being __init now, cannot possibly need the spinlock.

To your question checking vector for 0, that always wrong in my opinion,
stemming from this being a bad API.

It should become:

diff --git a/xen/arch/x86/cpu/mcheck/mce_intel.c 
b/xen/arch/x86/cpu/mcheck/mce_intel.c
index bb3c9f9e5234..9ae0bece1af7 100644
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -161,7 +161,7 @@ static void intel_init_thermal(const struct cpuinfo_x86 *c, 
bool bsp)
     }
 
     if ( bsp )
-        alloc_direct_apic_vector(&thermal_apic_vector, 
intel_thermal_interrupt);
+        thermal_apic_vector = 
alloc_direct_apic_vector(intel_thermal_interrupt);
 
     /* The temperature transition interrupt handler setup */
     val = thermal_apic_vector;    /* our delivery vector */
@@ -689,7 +689,7 @@ static void intel_init_cmci(struct cpuinfo_x86 *c, bool bsp)
     }
 
     if ( bsp )
-        alloc_direct_apic_vector(&cmci_apic_vector, cmci_interrupt);
+        cmci_apic_vector = alloc_direct_apic_vector(cmci_interrupt);
 
     apic = cmci_apic_vector;
     apic |= (APIC_DM_FIXED | APIC_LVT_MASKED);
diff --git a/xen/arch/x86/guest/xen/xen.c b/xen/arch/x86/guest/xen/xen.c
index 640872efdd2a..a473296ba94f 100644
--- a/xen/arch/x86/guest/xen/xen.c
+++ b/xen/arch/x86/guest/xen/xen.c
@@ -289,7 +289,7 @@ static void __init cf_check setup(void)
                XEN_LEGACY_MAX_VCPUS);
     }
 
-    alloc_direct_apic_vector(&evtchn_upcall_vector, xen_evtchn_upcall);
+    evtchn_upcall_vector = alloc_direct_apic_vector(xen_evtchn_upcall);
 
     BUG_ON(init_evtchn());
 }
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index e2b5077654ef..0919edd73312 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3118,10 +3118,10 @@ const struct hvm_function_table * __init start_vmx(void)
 
     if ( cpu_has_vmx_posted_intr_processing )
     {
-        alloc_direct_apic_vector(&posted_intr_vector, 
pi_notification_interrupt);
+        posted_intr_vector = 
alloc_direct_apic_vector(pi_notification_interrupt);
         if ( iommu_intpost )
         {
-            alloc_direct_apic_vector(&pi_wakeup_vector, pi_wakeup_interrupt);
+            pi_wakeup_vector = alloc_direct_apic_vector(pi_wakeup_interrupt);
 
             vmx_function_table.pi_update_irte = vmx_pi_update_irte;
         }
diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
index 7315150b66b4..0cf47a883722 100644
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -119,7 +119,7 @@ uint8_t alloc_hipriority_vector(void);
 void free_lopriority_vector(uint8_t vector);
 
 void set_direct_apic_vector(uint8_t vector, void (*handler)(void));
-void alloc_direct_apic_vector(uint8_t *vector, void (*handler)(void));
+uint8_t alloc_direct_apic_vector(void (*handler)(void));
 
 void do_IRQ(struct cpu_user_regs *regs);
 
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index c740035927c2..f50b5b5d5037 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -985,16 +985,13 @@ void __init set_direct_apic_vector(uint8_t vector, void 
(*handler)(void))
     direct_apic_vector[vector] = handler;
 }
 
-void __init alloc_direct_apic_vector(uint8_t *vector, void (*handler)(void))
+uint8_t __init alloc_direct_apic_vector(void (*handler)(void))
 {
-    static DEFINE_SPINLOCK(lock);
+    uint8_t vec = alloc_hipriority_vector();
 
-    spin_lock(&lock);
-    if (*vector == 0) {
-        *vector = alloc_hipriority_vector();
-        set_direct_apic_vector(*vector, handler);
-    }
-    spin_unlock(&lock);
+    set_direct_apic_vector(vec, handler);
+
+    return vec;
 }
 
 /* This could free any vectors, but is needed only for low-prio ones. */


which makes it a far more normal looking API, improves code generation,
and gets rid of the pointless lock.

Feel free to fold this in, or I can submit it separately, but I think
both parts want to go in together.

~Andrew