[RFC -tip] x86: do_IRQ - send APIC EOI for x86-32 on irq without handler v3

[RFC -tip] x86: do_IRQ - send APIC EOI for x86-32 on irq without handler v3

[Cyrill Gorcunov]

Impact: bugfix, cleanup

We should send APIC EOI if it's enabled only.
Since the same is needed for ack_bad_irq
we just introduce ack_APIC_irq_safe which in
turn check if we have APIC properly installed
and initialized and do EOI then.

Also a tiny cleanup: use pr_... macros and
add printk_ratelimit for ack_bad_irq to eliminate
possible storm on screwed hardware.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---

Ingo, I've checked the sources and as far as I see
we could NOP'ify apic->write indeed but I have
an internal feeling that this will bring us more problem
in future (for example it could be the following scenario:
some screwed APIC would require cleaning of LVT's or
IRR after resume regardless if it was initialized
or not at all). Mostly I mean that the idea of making
apic->write NOP'ified is quite elegant indeed but
cut off the subset of apic operations (we need
apic->read anyway) somehow bothering me from inside :)

CC'ed a number of people I know were involved in
this area.

On the other hand I could make a testing patch for
nop'fied ->write operation so we could check if
it bring any problems in real tests. Thoughts?

 arch/x86/include/asm/apic.h |   10 +++++++++-
 arch/x86/kernel/irq.c       |   18 ++++++------------
 2 files changed, 15 insertions(+), 13 deletions(-)

Index: linux-2.6.git/arch/x86/include/asm/apic.h
=====================================================================
--- linux-2.6.git.orig/arch/x86/include/asm/apic.h
+++ linux-2.6.git/arch/x86/include/asm/apic.h
@@ -392,7 +392,6 @@ static inline u32 safe_apic_wait_icr_idl
 	return apic->safe_wait_icr_idle();
 }
 
-
 static inline void ack_APIC_irq(void)
 {
 #ifdef CONFIG_X86_LOCAL_APIC
@@ -406,6 +405,15 @@ static inline void ack_APIC_irq(void)
 #endif
 }
 
+/* Ack APIC irq if it's enabled only */
+static inline void ack_APIC_irq_safe(void)
+{
+#ifdef CONFIG_X86_LOCAL_APIC
+	if (cpu_has_apic)
+		ack_APIC_irq();
+#endif
+}
+
 static inline unsigned default_get_apic_id(unsigned long x)
 {
 	unsigned int ver = GET_APIC_VERSION(apic_read(APIC_LVR));
Index: linux-2.6.git/arch/x86/kernel/irq.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/irq.c
+++ linux-2.6.git/arch/x86/kernel/irq.c
@@ -24,9 +24,9 @@ void (*generic_interrupt_extension)(void
  */
 void ack_bad_irq(unsigned int irq)
 {
-	printk(KERN_ERR "unexpected IRQ trap at vector %02x\n", irq);
+	if (printk_ratelimit())
+		pr_err("unexpected IRQ trap at vector %02x\n", irq);
 
-#ifdef CONFIG_X86_LOCAL_APIC
 	/*
 	 * Currently unexpected vectors happen only on SMP and APIC.
 	 * We _must_ ack these because every local APIC has only N
@@ -36,9 +36,7 @@ void ack_bad_irq(unsigned int irq)
 	 * completely.
 	 * But only ack when the APIC is enabled -AK
 	 */
-	if (cpu_has_apic)
-		ack_APIC_irq();
-#endif
+	ack_APIC_irq_safe();
 }
 
 #define irq_stats(x)		(&per_cpu(irq_stat, x))
@@ -223,14 +221,10 @@ unsigned int __irq_entry do_IRQ(struct p
 	irq = __get_cpu_var(vector_irq)[vector];
 
 	if (!handle_irq(irq, regs)) {
-#ifdef CONFIG_X86_64
-		if (!disable_apic)
-			ack_APIC_irq();
-#endif
-
+		ack_APIC_irq_safe();
 		if (printk_ratelimit())
-			printk(KERN_EMERG "%s: %d.%d No irq handler for vector (irq %d)\n",
-			       __func__, smp_processor_id(), vector, irq);
+			pr_emerg("%s: %d.%d No irq handler for vector (irq %d)\n",
+				__func__, smp_processor_id(), vector, irq);
 	}
 
 	irq_exit();

Re: [RFC -tip] x86: do_IRQ - send APIC EOI for x86-32 on irq without handler v3

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> Ingo, I've checked the sources and as far as I see
> we could NOP'ify apic->write indeed but I have
> an internal feeling that this will bring us more problem
> in future (for example it could be the following scenario:
> some screwed APIC would require cleaning of LVT's or
> IRR after resume regardless if it was initialized
> or not at all). Mostly I mean that the idea of making
> apic->write NOP'ified is quite elegant indeed but
> cut off the subset of apic operations (we need
> apic->read anyway) somehow bothering me from inside :)

it's as if assigned a special type of 'dummy apic' struct apic. It 
wont cause problems down the line: we use the new APIC driver 
infrastructure to abstract out quirks.

one small detail:

> +/* Ack APIC irq if it's enabled only */
> +static inline void ack_APIC_irq_safe(void)
> +{
> +#ifdef CONFIG_X86_LOCAL_APIC
> +	if (cpu_has_apic)
> +		ack_APIC_irq();
> +#endif

we dont need the cpu_has_apic check there, do we? In the 
!cpu_has_apic the ->write method should be a dummy.

> -#ifdef CONFIG_X86_64
> -		if (!disable_apic)
> -			ack_APIC_irq();
> -#endif
> -
> +		ack_APIC_irq_safe();

Please keep the ack_APIC_irq() name - it is inherently safe to call 
it if we always give it a meaningful ->write method.

	Ingo

Re: [RFC -tip] x86: do_IRQ - send APIC EOI for x86-32 on irq without handler v3

[Cyrill Gorcunov]

[Ingo Molnar - Fri, Apr 10, 2009 at 02:27:50PM +0200]
| 
| * Cyrill Gorcunov <gorcunov@openvz.org> wrote:
| 
| > Ingo, I've checked the sources and as far as I see
| > we could NOP'ify apic->write indeed but I have
| > an internal feeling that this will bring us more problem
| > in future (for example it could be the following scenario:
| > some screwed APIC would require cleaning of LVT's or
| > IRR after resume regardless if it was initialized
| > or not at all). Mostly I mean that the idea of making
| > apic->write NOP'ified is quite elegant indeed but
| > cut off the subset of apic operations (we need
| > apic->read anyway) somehow bothering me from inside :)
| 
| it's as if assigned a special type of 'dummy apic' struct apic. It 
| wont cause problems down the line: we use the new APIC driver 
| infrastructure to abstract out quirks.

Well, it's not that new actually :-)

| 
| one small detail:
| 
| > +/* Ack APIC irq if it's enabled only */
| > +static inline void ack_APIC_irq_safe(void)
| > +{
| > +#ifdef CONFIG_X86_LOCAL_APIC
| > +	if (cpu_has_apic)
| > +		ack_APIC_irq();
| > +#endif
| 
| we dont need the cpu_has_apic check there, do we? In the 
| !cpu_has_apic the ->write method should be a dummy.

Yes. In case you're talking about it'll not be needed
(we will find earlier whether cpu_has_apic or not).

| 
| > -#ifdef CONFIG_X86_64
| > -		if (!disable_apic)
| > -			ack_APIC_irq();
| > -#endif
| > -
| > +		ack_APIC_irq_safe();
| 
| Please keep the ack_APIC_irq() name - it is inherently safe to call 
| it if we always give it a meaningful ->write method.
| 
| 	Ingo
| 

Ok, I think we eventually try to NOP'ify apic->write method
so this patch is plainly not needed (thanks for comments!).

Will back with new patch.

        Cyrill

Re: [RFC -tip] x86: do_IRQ - send APIC EOI for x86-32 on irq without handler v3

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> [Ingo Molnar - Fri, Apr 10, 2009 at 02:27:50PM +0200]
> | 
> | * Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> | 
> | > Ingo, I've checked the sources and as far as I see
> | > we could NOP'ify apic->write indeed but I have
> | > an internal feeling that this will bring us more problem
> | > in future (for example it could be the following scenario:
> | > some screwed APIC would require cleaning of LVT's or
> | > IRR after resume regardless if it was initialized
> | > or not at all). Mostly I mean that the idea of making
> | > apic->write NOP'ified is quite elegant indeed but
> | > cut off the subset of apic operations (we need
> | > apic->read anyway) somehow bothering me from inside :)
> | 
> | it's as if assigned a special type of 'dummy apic' struct apic. It 
> | wont cause problems down the line: we use the new APIC driver 
> | infrastructure to abstract out quirks.
> 
> Well, it's not that new actually :-)

Yeah, i mean the new unified/modernized code in 2.6.30-to-be.

> | 
> | one small detail:
> | 
> | > +/* Ack APIC irq if it's enabled only */
> | > +static inline void ack_APIC_irq_safe(void)
> | > +{
> | > +#ifdef CONFIG_X86_LOCAL_APIC
> | > +	if (cpu_has_apic)
> | > +		ack_APIC_irq();
> | > +#endif
> | 
> | we dont need the cpu_has_apic check there, do we? In the 
> | !cpu_has_apic the ->write method should be a dummy.
> 
> Yes. In case you're talking about it'll not be needed
> (we will find earlier whether cpu_has_apic or not).

yeah.

	Ingo

Re: [RFC -tip] x86: do_IRQ - send APIC EOI for x86-32 on irq without handler v3

[Cyrill Gorcunov]

[Ingo Molnar - Fri, Apr 10, 2009 at 04:00:23PM +0200]
| 
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > [Ingo Molnar - Fri, Apr 10, 2009 at 02:27:50PM +0200]
| > | 
| > | * Cyrill Gorcunov <gorcunov@openvz.org> wrote:
| > | 
| > | > Ingo, I've checked the sources and as far as I see
| > | > we could NOP'ify apic->write indeed but I have
| > | > an internal feeling that this will bring us more problem
| > | > in future (for example it could be the following scenario:
| > | > some screwed APIC would require cleaning of LVT's or
| > | > IRR after resume regardless if it was initialized
| > | > or not at all). Mostly I mean that the idea of making
| > | > apic->write NOP'ified is quite elegant indeed but
| > | > cut off the subset of apic operations (we need
| > | > apic->read anyway) somehow bothering me from inside :)
| > | 
| > | it's as if assigned a special type of 'dummy apic' struct apic. It 
| > | wont cause problems down the line: we use the new APIC driver 
| > | infrastructure to abstract out quirks.
| > 
| > Well, it's not that new actually :-)
| 
| Yeah, i mean the new unified/modernized code in 2.6.30-to-be.
| 
| > | 
| > | one small detail:
| > | 
| > | > +/* Ack APIC irq if it's enabled only */
| > | > +static inline void ack_APIC_irq_safe(void)
| > | > +{
| > | > +#ifdef CONFIG_X86_LOCAL_APIC
| > | > +	if (cpu_has_apic)
| > | > +		ack_APIC_irq();
| > | > +#endif
| > | 
| > | we dont need the cpu_has_apic check there, do we? In the 
| > | !cpu_has_apic the ->write method should be a dummy.
| > 
| > Yes. In case you're talking about it'll not be needed
| > (we will find earlier whether cpu_has_apic or not).
| 
| yeah.
| 
| 	Ingo
| 

Ingo,

I think you meant something like the patch below. It's
not finished yet -- I need to find out right place for
calling freshly introduced apic_disable_write_op.
Will continue tomorrow.

But even having it not completed yet I would like to
get some feedbackabout code structure in general.

        Cyrill
---
 arch/x86/include/asm/apic.h |   10 ++++++++++
 arch/x86/kernel/apic/apic.c |    6 ++++++
 arch/x86/kernel/irq.c       |   10 ++--------
 3 files changed, 18 insertions(+), 8 deletions(-)

Index: linux-2.6.git/arch/x86/include/asm/apic.h
=====================================================================
--- linux-2.6.git.orig/arch/x86/include/asm/apic.h
+++ linux-2.6.git/arch/x86/include/asm/apic.h
@@ -106,6 +106,7 @@ extern void native_apic_wait_icr_idle(vo
 extern u32 native_safe_apic_wait_icr_idle(void);
 extern void native_apic_icr_write(u32 low, u32 id);
 extern u64 native_apic_icr_read(void);
+extern void native_apic_write_dummy(u32 reg, u32 v);
 
 #define EIM_8BIT_APIC_ID	0
 #define EIM_32BIT_APIC_ID	1
@@ -372,6 +373,15 @@ static inline void apic_write(u32 reg, u
 	apic->write(reg, val);
 }
 
+/*
+ * right after this call apic->write doesn't do anything
+ * note that there is no restore operation it works one way
+ */
+static inline void apic_disable_write_op(void)
+{
+	apic->write = native_apic_write_dummy;
+}
+
 static inline u64 apic_icr_read(void)
 {
 	return apic->icr_read();
Index: linux-2.6.git/arch/x86/kernel/apic/apic.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic/apic.c
+++ linux-2.6.git/arch/x86/kernel/apic/apic.c
@@ -210,6 +210,12 @@ static int modern_apic(void)
 	return lapic_get_version() >= 0x14;
 }
 
+/*
+ * bare function to substitute write operation
+ * and it's _that_ fast :)
+ */
+void native_apic_write_dummy(u32 reg, u32 v) { }
+
 void native_apic_wait_icr_idle(void)
 {
 	while (apic_read(APIC_ICR) & APIC_ICR_BUSY)
Index: linux-2.6.git/arch/x86/kernel/irq.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/irq.c
+++ linux-2.6.git/arch/x86/kernel/irq.c
@@ -27,7 +27,6 @@ void ack_bad_irq(unsigned int irq)
 	if (printk_ratelimit())
 		pr_err("unexpected IRQ trap at vector %02x\n", irq);
 
-#ifdef CONFIG_X86_LOCAL_APIC
 	/*
 	 * Currently unexpected vectors happen only on SMP and APIC.
 	 * We _must_ ack these because every local APIC has only N
@@ -37,9 +36,7 @@ void ack_bad_irq(unsigned int irq)
 	 * completely.
 	 * But only ack when the APIC is enabled -AK
 	 */
-	if (cpu_has_apic)
-		ack_APIC_irq();
-#endif
+	ack_APIC_irq();
 }
 
 #define irq_stats(x)		(&per_cpu(irq_stat, x))
@@ -224,10 +221,7 @@ unsigned int __irq_entry do_IRQ(struct p
 	irq = __get_cpu_var(vector_irq)[vector];
 
 	if (!handle_irq(irq, regs)) {
-#ifdef CONFIG_X86_64
-		if (!disable_apic)
-			ack_APIC_irq();
-#endif
+		ack_APIC_irq();
 
 		if (printk_ratelimit())
 			pr_emerg("%s: %d.%d No irq handler for vector (irq %d)\n",

Re: [RFC -tip] x86: do_IRQ - send APIC EOI for x86-32 on irq without handler v3

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> [Ingo Molnar - Fri, Apr 10, 2009 at 04:00:23PM +0200]
> | 
> | * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> | 
> | > [Ingo Molnar - Fri, Apr 10, 2009 at 02:27:50PM +0200]
> | > | 
> | > | * Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> | > | 
> | > | > Ingo, I've checked the sources and as far as I see
> | > | > we could NOP'ify apic->write indeed but I have
> | > | > an internal feeling that this will bring us more problem
> | > | > in future (for example it could be the following scenario:
> | > | > some screwed APIC would require cleaning of LVT's or
> | > | > IRR after resume regardless if it was initialized
> | > | > or not at all). Mostly I mean that the idea of making
> | > | > apic->write NOP'ified is quite elegant indeed but
> | > | > cut off the subset of apic operations (we need
> | > | > apic->read anyway) somehow bothering me from inside :)
> | > | 
> | > | it's as if assigned a special type of 'dummy apic' struct apic. It 
> | > | wont cause problems down the line: we use the new APIC driver 
> | > | infrastructure to abstract out quirks.
> | > 
> | > Well, it's not that new actually :-)
> | 
> | Yeah, i mean the new unified/modernized code in 2.6.30-to-be.
> | 
> | > | 
> | > | one small detail:
> | > | 
> | > | > +/* Ack APIC irq if it's enabled only */
> | > | > +static inline void ack_APIC_irq_safe(void)
> | > | > +{
> | > | > +#ifdef CONFIG_X86_LOCAL_APIC
> | > | > +	if (cpu_has_apic)
> | > | > +		ack_APIC_irq();
> | > | > +#endif
> | > | 
> | > | we dont need the cpu_has_apic check there, do we? In the 
> | > | !cpu_has_apic the ->write method should be a dummy.
> | > 
> | > Yes. In case you're talking about it'll not be needed
> | > (we will find earlier whether cpu_has_apic or not).
> | 
> | yeah.
> | 
> | 	Ingo
> | 
> 
> Ingo,
> 
> I think you meant something like the patch below. It's
> not finished yet -- I need to find out right place for
> calling freshly introduced apic_disable_write_op.
> Will continue tomorrow.
> 
> But even having it not completed yet I would like to
> get some feedbackabout code structure in general.

Yeah, the goal now looks good.

Note, i'd suggest to not expose it like this:

>  extern u64 native_apic_icr_read(void);
> +extern void native_apic_write_dummy(u32 reg, u32 v);
>  
>  #define EIM_8BIT_APIC_ID	0
>  #define EIM_32BIT_APIC_ID	1
> @@ -372,6 +373,15 @@ static inline void apic_write(u32 reg, u
>  	apic->write(reg, val);
>  }
>  
> +/*
> + * right after this call apic->write doesn't do anything
> + * note that there is no restore operation it works one way
> + */
> +static inline void apic_disable_write_op(void)
> +{
> +	apic->write = native_apic_write_dummy;
> +}
> +

But have a central and opaque:

	extern void apic_disable(void);

function, defined in apic.c - which does all the internal details 
(like installing a dummy ->write) entry.

	Ingo