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.

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

--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1459,119 +1459,6 @@ void disable_IO_APIC(void)
 }
 
 /*
- * function to set the IO-APIC physical IDs based on the
- * values stored in the MPC table.
- *
- * by Matt Domsch <Matt_Domsch@dell.com>  Tue Dec 21 12:25:05 CST 1999
- */
-
-static void __init setup_ioapic_ids_from_mpc(void)
-{
-    union IO_APIC_reg_00 reg_00;
-    static physid_mask_t __initdata phys_id_present_map;
-    int apic;
-    int i;
-    unsigned char old_id;
-    unsigned long flags;
-    const uint32_t broadcast_id = 0xf;
-
-    /*
-     * Don't check I/O APIC IDs for xAPIC systems. They have
-     * no meaning without the serial APIC bus.
-     */
-    if (!(boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
-        || APIC_XAPIC(apic_version[boot_cpu_physical_apicid]))
-        return;
-
-    /*
-     * This is broken; anything with a real cpu count has to
-     * circumvent this idiocy regardless.
-     */
-    phys_id_present_map = phys_cpu_present_map;
-
-    /*
-     * Set the IOAPIC ID to the value stored in the MPC table.
-     */
-    for (apic = 0; apic < nr_ioapics; apic++) {
-        if (!nr_ioapic_entries[apic])
-            continue;
-
-        /* Read the register 0 value */
-        spin_lock_irqsave(&ioapic_lock, flags);
-        reg_00.raw = io_apic_read(apic, 0);
-        spin_unlock_irqrestore(&ioapic_lock, flags);
-		
-        old_id = mp_ioapics[apic].mpc_apicid;
-
-        if (mp_ioapics[apic].mpc_apicid >= broadcast_id) {
-            printk(KERN_ERR "BIOS bug, IO-APIC#%d ID is %d in the MPC table!...\n",
-                   apic, mp_ioapics[apic].mpc_apicid);
-            printk(KERN_ERR "... fixing up to %d. (tell your hw vendor)\n",
-                   reg_00.bits.ID);
-            mp_ioapics[apic].mpc_apicid = reg_00.bits.ID;
-        }
-
-        /*
-         * Sanity check, is the ID really free? 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(mp_ioapics[apic].mpc_apicid, phys_id_present_map) )
-        {
-            printk(KERN_ERR "BIOS bug, IO-APIC#%d ID %d is already used!...\n",
-                   apic, mp_ioapics[apic].mpc_apicid);
-            for (i = 0; i < broadcast_id; i++)
-                if (!physid_isset(i, phys_id_present_map))
-                    break;
-            if (i >= broadcast_id)
-                panic("Max APIC ID exceeded\n");
-            printk(KERN_ERR "... fixing up to %d. (tell your hw vendor)\n",
-                   i);
-            mp_ioapics[apic].mpc_apicid = i;
-        } else {
-            apic_printk(APIC_VERBOSE, "Setting %d in the "
-                        "phys_id_present_map\n",
-                        mp_ioapics[apic].mpc_apicid);
-        }
-        physid_set(mp_ioapics[apic].mpc_apicid, phys_id_present_map);
-
-        /*
-         * We need to adjust the IRQ routing table
-         * if the ID changed.
-         */
-        if (old_id != mp_ioapics[apic].mpc_apicid)
-            for (i = 0; i < mp_irq_entries; i++)
-                if (mp_irqs[i].mpc_dstapic == old_id)
-                    mp_irqs[i].mpc_dstapic
-                        = mp_ioapics[apic].mpc_apicid;
-
-        /*
-         * Read the right value from the MPC table and
-         * write it into the ID register.
-         */
-        apic_printk(APIC_VERBOSE, KERN_INFO
-                    "...changing IO-APIC physical APIC ID to %d ...",
-                    mp_ioapics[apic].mpc_apicid);
-
-        reg_00.bits.ID = mp_ioapics[apic].mpc_apicid;
-        spin_lock_irqsave(&ioapic_lock, flags);
-        io_apic_write(apic, 0, reg_00.raw);
-        spin_unlock_irqrestore(&ioapic_lock, flags);
-
-        /*
-         * Sanity check
-         */
-        spin_lock_irqsave(&ioapic_lock, flags);
-        reg_00.raw = io_apic_read(apic, 0);
-        spin_unlock_irqrestore(&ioapic_lock, flags);
-        if (reg_00.bits.ID != mp_ioapics[apic].mpc_apicid)
-            printk("could not set ID!\n");
-        else
-            apic_printk(APIC_VERBOSE, " ok.\n");
-    }
-}
-
-/*
  * There is a nasty bug in some older SMP boards, their mptable lies
  * about the timer IRQ. We do the following to work around the situation:
  *
@@ -2158,12 +2045,6 @@ void __init setup_IO_APIC(void)
         ioapic_level_type.end = end_level_ioapic_irq_new;
     }
 
-    /*
-     * Set up IO-APIC IRQ routing.
-     */
-    if (!acpi_ioapic)
-        setup_ioapic_ids_from_mpc();
-
     setup_IO_APIC_irqs();
     init_IO_APIC_traps();
     check_timer();

On 03/09/2025 8:55 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.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

The code change is fine, but the commit message should be first
paragraph only.

The first paragraph is plenty of justification to make the change,
irrespective of anything else.

The other 3 paragraphs are musings on an area of MISRA where which is
unclear, or even disputed.  The code here is statically reachable,
dynamically unreachable, and trying to argue this in terms of dead or
unreachability detracts from an otherwise clear patch.

With a very strong preference to have the commit message be only the
first paragraph, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

On 10.09.2025 15:26, Andrew Cooper wrote:
> On 03/09/2025 8:55 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.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> The code change is fine, but the commit message should be first
> paragraph only.
> 
> The first paragraph is plenty of justification to make the change,
> irrespective of anything else.

Well. I wouldn't have added the other parts if we weren't where we are in
the release cycle. Strictly speaking, with them dropped I can't put these
two patches in right now. Oleksii, may I ask for your view please (on
both of the patches, as they're both similar in this regard)?

> The other 3 paragraphs are musings on an area of MISRA where which is
> unclear, or even disputed.  The code here is statically reachable,
> dynamically unreachable, and trying to argue this in terms of dead or
> unreachability detracts from an otherwise clear patch.
> 
> With a very strong preference to have the commit message be only the
> first paragraph, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks (also for the one for patch 2).

Jan

On 9/10/25 3:36 PM, Jan Beulich wrote:
> On 10.09.2025 15:26, Andrew Cooper wrote:
>> On 03/09/2025 8:55 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.
>>>
>>> Signed-off-by: Jan Beulich<jbeulich@suse.com>
>> The code change is fine, but the commit message should be first
>> paragraph only.
>>
>> The first paragraph is plenty of justification to make the change,
>> irrespective of anything else.
> Well. I wouldn't have added the other parts if we weren't where we are in
> the release cycle. Strictly speaking, with them dropped I can't put these
> two patches in right now. Oleksii, may I ask for your view please (on
> both of the patches, as they're both similar in this regard)?

For both patches, I think it would still be useful to retain the explanation
about the MISRA Rule 2.2 violation in the commit message. While I agree that
the first paragraph provides sufficient justification on its own, the additional
context helps clarify why we're committing this change now, rather than waiting
until after the release. It also highlights the additional benefit of improving
MISRA compliance by removing this dead code.

Anyway, I am okay with having these patches merged now:
  Release-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>

~ Oleksii

>
>> The other 3 paragraphs are musings on an area of MISRA where which is
>> unclear, or even disputed.  The code here is statically reachable,
>> dynamically unreachable, and trying to argue this in terms of dead or
>> unreachability detracts from an otherwise clear patch.
>>
>> With a very strong preference to have the commit message be only the
>> first paragraph, Reviewed-by: Andrew Cooper<andrew.cooper3@citrix.com>
> Thanks (also for the one for patch 2).
>
> Jan

On 2025-09-03 03:55, 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.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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