Skip to contentSkip to conversation

x86: Don't write io_apic ID if it is not changed

PATCH In review Yinghai Lu 4 participants 6 messages Aug 14, 2010 – Aug 16, 2010 x86

lore
Share & tools
lore

Yinghai Lu

author
#0 lore raw

PATCH 1/1 x86: Don't write io_apic ID if it is not changed

For 32bit mptable path, setup_ids_from_mpc() always write io apic id register, even there is no change needed.

So try to do that when they are different bewteen reading out and mptable

Signed-off-by Yinghai Lu <[email protected]>
---
 arch/x86/kernel/apic/io_apic.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
arch/x86/kernel/apic/io_apic.c +5 -3
Index: linux-2.6/arch/x86/kernel/apic/io_apic.c
===================================================================
@@ -2125,7 +2125,6 @@ void __init setup_ioapic_ids_from_mpc(vo
 			physids_or(phys_id_present_map, phys_id_present_map, tmp);
 		}
 
-
 		/*
 		 * We need to adjust the IRQ routing table
 		 * if the ID changed.

@@ -2137,9 +2136,12 @@ void __init setup_ioapic_ids_from_mpc(vo
 						= mp_ioapics[apic_id].apicid;
 
 		/*
-		 * Read the right value from the MPC table and
-		 * write it into the ID register.
+		 * Update the ID register according to the right value from
+		 *  the MPC table if they are different.
 		 */
+		if (mp_ioapics[apic_id].apicid == reg_00.bits.ID)
+			continue;
+
 		apic_printk(APIC_VERBOSE, KERN_INFO
 			"...changing IO-APIC physical APIC ID to %d ...",
 			mp_ioapics[apic_id].apicid);
#1 lore raw
Quoted 29 lines · Yinghai Lu

For 32bit mptable path, setup_ids_from_mpc() always write io apic id register, even there is no change needed.

So try to do that when they are different bewteen reading out and mptable

Signed-off-by Yinghai Lu <[email protected]>
---
 arch/x86/kernel/apic/io_apic.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Index: linux-2.6/arch/x86/kernel/apic/io_apic.c
===================================================================

arch/x86/kernel/apic/io_apic.c +0 -2
@@ -2125,7 +2125,6 @@ void __init setup_ioapic_ids_from_mpc(vo
 			physids_or(phys_id_present_map, phys_id_present_map, tmp);
 		}
 
-
 		/*
 		 * We need to adjust the IRQ routing table
 		 * if the ID changed.

@@ -2137,9 +2136,12 @@ void __init setup_ioapic_ids_from_mpc(vo
 						= mp_ioapics[apic_id].apicid;
 
 		/*
-		 * Read the right value from the MPC table and

arch/x86/kernel/apic/io_apic.c void __init setup_ioapic_ids_from_mpc(vo · line 2147

Quoted 10 lines
arch/x86/kernel/apic/io_apic.c +5 -1
-		 * write it into the ID register.
+		 * Update the ID register according to the right value from
+		 *  the MPC table if they are different.
 		 */
+		if (mp_ioapics[apic_id].apicid == reg_00.bits.ID)
+			continue;
+
 		apic_printk(APIC_VERBOSE, KERN_INFO
 			"...changing IO-APIC physical APIC ID to %d ...",
 			mp_ioapics[apic_id].apicid);

As far as I see, this was done by a purpose in former code. Consider the situation when mp_ioapics[apic_id].apicid >= get_physical_broadcast(). By code flow (io_apic.c:2099) this set

		if (mp_ioapics[apic_id].apicid >= get_physical_broadcast()) {
			printk(KERN_ERR "BIOS bug, IO-APIC#%d ID is %d in the MPC table!...\n",
				apic_id, mp_ioapics[apic_id].apicid);
			printk(KERN_ERR "... fixing up to %d. (tell your hw vendor)\n",
				reg_00.bits.ID);
--->			mp_ioapics[apic_id].apicid = reg_00.bits.ID;
		}

So with your patch we always hit "continue" without real changing of ID which is not correct.

But perhaps I miss something?

-- Cyrill

#2 lore raw

On Sun, Aug 15, 2010 at 12:24:46PM +0400, Cyrill Gorcunov wrote:
...

Quoted 12 lines

As far as I see, this was done by a purpose in former code. Consider the situation when mp_ioapics[apic_id].apicid >= get_physical_broadcast(). By code flow (io_apic.c:2099) this set

		if (mp_ioapics[apic_id].apicid >= get_physical_broadcast()) {
			printk(KERN_ERR "BIOS bug, IO-APIC#%d ID is %d in the MPC table!...\n",
				apic_id, mp_ioapics[apic_id].apicid);
			printk(KERN_ERR "... fixing up to %d. (tell your hw vendor)\n",
				reg_00.bits.ID);
--->			mp_ioapics[apic_id].apicid = reg_00.bits.ID;
		}
Quoted 6 lines

So with your patch we always hit "continue" without real changing of ID which is not correct.

But perhaps I miss something?

-- Cyrill

False alarm Yinghai, of course there is no need to write same value back, sorry for noise.

-- Cyrill

#3 lore raw
Quoted 17 lines · Cyrill Gorcunov

On Sun, Aug 15, 2010 at 12:24:46PM +0400, Cyrill Gorcunov wrote:
...

Quoted 15 lines

As far as I see, this was done by a purpose in former code. Consider the situation when mp_ioapics[apic_id].apicid >= get_physical_broadcast(). By code flow (io_apic.c:2099) this set

		if (mp_ioapics[apic_id].apicid >= get_physical_broadcast()) {
			printk(KERN_ERR "BIOS bug, IO-APIC#%d ID is %d in the MPC table!...\n",
				apic_id, mp_ioapics[apic_id].apicid);
			printk(KERN_ERR "... fixing up to %d. (tell your hw vendor)\n",
				reg_00.bits.ID);
--->			mp_ioapics[apic_id].apicid = reg_00.bits.ID;
		}

So with your patch we always hit "continue" without real changing of ID which is not correct.

Quoted 8 lines
Quoted 3 lines

But perhaps I miss something?

-- Cyrill

False alarm Yinghai, of course there is no need to write same value back, sorry for noise.

-- Cyrill

Thinking a bit more I guess there might be a sence to write io-apic id unconditionally, io-apic id is not latched on reset but set to 0 (at least for old apics) so if we hit id match -- this will be once, but the same time we will have additional if() for every cycle iteration, not sure it worth it. Right?

-- Cyrill

#4 lore raw
In reply to Yinghai Lu
Quoted 5 lines · Yinghai Lu

For 32bit mptable path, setup_ids_from_mpc() always write io apic id register, even there is no change needed.

So try to do that when they are different bewteen reading out and mptable

What is the advantage of not writing the value?

Eric

#5 lore raw
Quoted 9 lines · Eric W. Biederman
Quoted 5 lines · Yinghai Lu

For 32bit mptable path, setup_ids_from_mpc() always write io apic id register, even there is no change needed.

So try to do that when they are different bewteen reading out and mptable

What is the advantage of not writing the value?

Just guessing, but if other OSs dont do it we might surprise buggy hardware [and trigger erratas in them], so this would be a standard 'dont touch the hw unless necessary' defensive move.

Similar bugs are not unprecedented.

Thanks,

Ingo