* [PATCH] x86: Don't write io_apic ID if it is not changed
@ 2010-08-14 20:38 Yinghai Lu
2010-08-15 8:24 ` Cyrill Gorcunov
2010-08-15 15:55 ` Eric W. Biederman
0 siblings, 2 replies; 6+ messages in thread
From: Yinghai Lu @ 2010-08-14 20:38 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Morton,
linux-kernel
Cc: Suresh Siddha, Eric W. Biederman
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 <yinghai@kernel.org>
---
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
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/io_apic.c
+++ 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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: Don't write io_apic ID if it is not changed
2010-08-14 20:38 [PATCH] x86: Don't write io_apic ID if it is not changed Yinghai Lu
@ 2010-08-15 8:24 ` Cyrill Gorcunov
2010-08-15 8:30 ` Cyrill Gorcunov
2010-08-15 15:55 ` Eric W. Biederman
1 sibling, 1 reply; 6+ messages in thread
From: Cyrill Gorcunov @ 2010-08-15 8:24 UTC (permalink / raw)
To: Yinghai Lu
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Morton,
linux-kernel, Suresh Siddha, Eric W. Biederman
On Sat, Aug 14, 2010 at 01:38:01PM -0700, Yinghai Lu wrote:
>
> 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 <yinghai@kernel.org>
>
> ---
> 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
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/apic/io_apic.c
> +++ 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);
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: Don't write io_apic ID if it is not changed
2010-08-15 8:24 ` Cyrill Gorcunov
@ 2010-08-15 8:30 ` Cyrill Gorcunov
2010-08-16 9:06 ` Cyrill Gorcunov
0 siblings, 1 reply; 6+ messages in thread
From: Cyrill Gorcunov @ 2010-08-15 8:30 UTC (permalink / raw)
To: Yinghai Lu
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Morton,
linux-kernel, Suresh Siddha, Eric W. Biederman
On Sun, Aug 15, 2010 at 12:24:46PM +0400, Cyrill Gorcunov wrote:
...
> 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
False alarm Yinghai, of course there is no need to write same value back,
sorry for noise.
-- Cyrill
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: Don't write io_apic ID if it is not changed
2010-08-14 20:38 [PATCH] x86: Don't write io_apic ID if it is not changed Yinghai Lu
2010-08-15 8:24 ` Cyrill Gorcunov
@ 2010-08-15 15:55 ` Eric W. Biederman
2010-08-16 7:28 ` Ingo Molnar
1 sibling, 1 reply; 6+ messages in thread
From: Eric W. Biederman @ 2010-08-15 15:55 UTC (permalink / raw)
To: Yinghai Lu
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Morton,
linux-kernel, Suresh Siddha
Yinghai Lu <yinghai@kernel.org> writes:
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: Don't write io_apic ID if it is not changed
2010-08-15 15:55 ` Eric W. Biederman
@ 2010-08-16 7:28 ` Ingo Molnar
0 siblings, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2010-08-16 7:28 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Yinghai Lu, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Andrew Morton, linux-kernel, Suresh Siddha
* Eric W. Biederman <ebiederm@xmission.com> wrote:
> Yinghai Lu <yinghai@kernel.org> writes:
>
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: Don't write io_apic ID if it is not changed
2010-08-15 8:30 ` Cyrill Gorcunov
@ 2010-08-16 9:06 ` Cyrill Gorcunov
0 siblings, 0 replies; 6+ messages in thread
From: Cyrill Gorcunov @ 2010-08-16 9:06 UTC (permalink / raw)
To: Yinghai Lu
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Morton,
linux-kernel, Suresh Siddha, Eric W. Biederman
On Sun, Aug 15, 2010 at 12:30:57PM +0400, Cyrill Gorcunov wrote:
> On Sun, Aug 15, 2010 at 12:24:46PM +0400, Cyrill Gorcunov wrote:
> ...
> > 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
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-08-16 9:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-14 20:38 [PATCH] x86: Don't write io_apic ID if it is not changed Yinghai Lu
2010-08-15 8:24 ` Cyrill Gorcunov
2010-08-15 8:30 ` Cyrill Gorcunov
2010-08-16 9:06 ` Cyrill Gorcunov
2010-08-15 15:55 ` Eric W. Biederman
2010-08-16 7:28 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).