[PATCH 2/2] x86/IO-APIC: drop io_apic_get_unique_id()

Jan Beulich posted 2 patches 1 month, 3 weeks ago
[PATCH 2/2] x86/IO-APIC: drop io_apic_get_unique_id()
Posted by Jan Beulich 1 month, 3 weeks ago
Along the lines of what b89f8f054f96 ("x86/apic: Drop sync_Arb_IDs()")
said, the function is dead logic as well: All 64-bit capable Intel systems
have (at least) xAPIC (if not x2APIC).

Even if Eclair can't know it, such code is violating Misra rule 2.2 (dead
code; we didn't accept that yet, but - where possible - we probably would
better follow it). Depending on one's reading, this code may actually be a
violation of rule 2.1 (unreachable), which we did accept:

"Code is unreachable if, ..., there is no combination of program inputs
 that can cause it to be executed."

Otoh it's "only" __init code.

As this removes the last user of APIC_XAPIC(), remove the macro as well.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/include/asm/apicdef.h
+++ b/xen/arch/x86/include/asm/apicdef.h
@@ -19,7 +19,6 @@
 #define			APIC_LVR_DIRECTED_EOI	(1 << 24)
 #define			GET_APIC_VERSION(x)	((x)&0xFF)
 #define			GET_APIC_MAXLVT(x)	(((x)>>16)&0xFF)
-#define			APIC_XAPIC(x)		((x) >= 0x14)
 #define		APIC_TASKPRI	0x80
 #define			APIC_TPRI_MASK		0xFF
 #define		APIC_ARBPRI	0x90
--- a/xen/arch/x86/include/asm/io_apic.h
+++ b/xen/arch/x86/include/asm/io_apic.h
@@ -184,7 +184,6 @@ extern bool skip_ioapic_setup;
 extern bool ioapic_ack_new;
 extern bool ioapic_ack_forced;
 
-extern int io_apic_get_unique_id (int ioapic, int apic_id);
 extern int io_apic_get_version (int ioapic);
 extern int io_apic_get_redir_entries (int ioapic);
 extern int io_apic_set_pci_routing (int ioapic, int pin, int irq, int edge_level, int active_high_low);
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2092,86 +2092,6 @@ void ioapic_resume(void)
     spin_unlock_irqrestore(&ioapic_lock, flags);
 }
 
-/* --------------------------------------------------------------------------
-                          ACPI-based IOAPIC Configuration
-   -------------------------------------------------------------------------- */
-
-
-int __init io_apic_get_unique_id (int ioapic, int apic_id)
-{
-    union IO_APIC_reg_00 reg_00;
-    static physid_mask_t __initdata apic_id_map = PHYSID_MASK_NONE;
-    unsigned long flags;
-    int i = 0;
-    const uint32_t broadcast_id = 0xf;
-
-    /*
-     * The P4 platform supports up to 256 APIC IDs on two separate APIC 
-     * buses (one for LAPICs, one for IOAPICs), where predecessors only 
-     * supports up to 16 on one shared APIC bus.
-     * 
-     * TBD: Expand LAPIC/IOAPIC support on P4-class systems to take full
-     *      advantage of new APIC bus architecture.
-     */
-
-    if (physids_empty(apic_id_map))
-        apic_id_map = phys_cpu_present_map;
-
-    spin_lock_irqsave(&ioapic_lock, flags);
-    reg_00.raw = io_apic_read(ioapic, 0);
-    spin_unlock_irqrestore(&ioapic_lock, flags);
-
-    if (apic_id >= broadcast_id) {
-        printk(KERN_WARNING "IOAPIC[%d]: Invalid apic_id %d, trying "
-               "%d\n", ioapic, apic_id, reg_00.bits.ID);
-        apic_id = reg_00.bits.ID;
-    }
-
-    /*
-     * Every APIC in a system must have a unique ID or we get lots of nice 
-     * 'stuck on smp_invalidate_needed IPI wait' messages.
-     */
-    if ( physid_isset(apic_id, apic_id_map) )
-    {
-
-        for (i = 0; i < broadcast_id; i++) {
-            if ( !physid_isset(i, apic_id_map) )
-                break;
-        }
-
-        if (i == broadcast_id)
-            panic("Max apic_id exceeded\n");
-
-        printk(KERN_WARNING "IOAPIC[%d]: apic_id %d already used, "
-               "trying %d\n", ioapic, apic_id, i);
-
-        apic_id = i;
-    } 
-
-    physid_set(apic_id, apic_id_map);
-
-    if (reg_00.bits.ID != apic_id) {
-        reg_00.bits.ID = apic_id;
-
-        spin_lock_irqsave(&ioapic_lock, flags);
-        io_apic_write(ioapic, 0, reg_00.raw);
-        reg_00.raw = io_apic_read(ioapic, 0);
-        spin_unlock_irqrestore(&ioapic_lock, flags);
-
-        /* Sanity check */
-        if (reg_00.bits.ID != apic_id) {
-            printk("IOAPIC[%d]: Unable to change apic_id!\n", ioapic);
-            return -1;
-        }
-    }
-
-    apic_printk(APIC_VERBOSE, KERN_INFO
-                "IOAPIC[%d]: Assigned apic_id %d\n", ioapic, apic_id);
-
-    return apic_id;
-}
-
-
 int __init io_apic_get_version (int ioapic)
 {
     union IO_APIC_reg_01	reg_01;
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -871,7 +871,6 @@ void __init mp_register_ioapic (
 	u32			gsi_base)
 {
 	int			idx = 0;
-	int			tmpid;
 
 	if (nr_ioapics >= MAX_IO_APICS) {
 		printk(KERN_ERR "ERROR: Max # of I/O APICs (%d) exceeded "
@@ -891,16 +890,7 @@ void __init mp_register_ioapic (
 	mp_ioapics[idx].mpc_apicaddr = address;
 
 	set_fixmap_nocache(FIX_IO_APIC_BASE_0 + idx, address);
-	if ((boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
-		&& !APIC_XAPIC(apic_version[boot_cpu_physical_apicid]))
-		tmpid = io_apic_get_unique_id(idx, id);
-	else
-		tmpid = id;
-	if (tmpid == -1) {
-		nr_ioapics--;
-		return;
-	}
-	mp_ioapics[idx].mpc_apicid = tmpid;
+	mp_ioapics[idx].mpc_apicid = id;
 	mp_ioapics[idx].mpc_apicver = io_apic_get_version(idx);
 	
 	/*
Re: [PATCH 2/2] x86/IO-APIC: drop io_apic_get_unique_id()
Posted by Andrew Cooper 1 month, 2 weeks ago
On 03/09/2025 8:56 am, Jan Beulich wrote:
> Along the lines of what b89f8f054f96 ("x86/apic: Drop sync_Arb_IDs()")
> said, the function is dead logic as well: All 64-bit capable Intel systems
> have (at least) xAPIC (if not x2APIC).
>
> Even if Eclair can't know it, such code is violating Misra rule 2.2 (dead
> code; we didn't accept that yet, but - where possible - we probably would
> better follow it). Depending on one's reading, this code may actually be a
> violation of rule 2.1 (unreachable), which we did accept:
>
> "Code is unreachable if, ..., there is no combination of program inputs
>  that can cause it to be executed."
>
> Otoh it's "only" __init code.
>
> As this removes the last user of APIC_XAPIC(), remove the macro as well.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Same feedback as with the previous patch, albeit about the middle 3
paragraphs.  Again with a strong preference for those to be removed,
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I'd not even spotted that APIC_XAPIC() existed.  This being explicit
conformation of where XAPIC starts would have been helpful when doing
archaeology.

Re: [PATCH 2/2] x86/IO-APIC: drop io_apic_get_unique_id()
Posted by Jason Andryuk 1 month, 3 weeks ago
On 2025-09-03 03:56, Jan Beulich wrote:
> Along the lines of what b89f8f054f96 ("x86/apic: Drop sync_Arb_IDs()")
> said, the function is dead logic as well: All 64-bit capable Intel systems
> have (at least) xAPIC (if not x2APIC).
> 
> Even if Eclair can't know it, such code is violating Misra rule 2.2 (dead
> code; we didn't accept that yet, but - where possible - we probably would
> better follow it). Depending on one's reading, this code may actually be a
> violation of rule 2.1 (unreachable), which we did accept:
> 
> "Code is unreachable if, ..., there is no combination of program inputs
>   that can cause it to be executed."
> 
> Otoh it's "only" __init code.
> 
> As this removes the last user of APIC_XAPIC(), remove the macro as well.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>