[PATCH] x86: Don't ack_APIC_irq() if lapic is disabled in GENERIC_INTERRUPT_VECTOR handler

[PATCH] x86: Don't ack_APIC_irq() if lapic is disabled in GENERIC_INTERRUPT_VECTOR handler

[Sheng Yang]

Otherwise would cause trouble...

Cc: Dimitri Sivanich <sivanich@sgi.com>
Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/kernel/irq.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index b0cdde6..78b23d0 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -257,7 +257,8 @@ void smp_generic_interrupt(struct pt_regs *regs)
 {
 	struct pt_regs *old_regs = set_irq_regs(regs);
 
-	ack_APIC_irq();
+	if (!disable_apic)
+		ack_APIC_irq();
 
 	exit_idle();
 
-- 
1.5.4.5

Re: [PATCH] x86: Don't ack_APIC_irq() if lapic is disabled in GENERIC_INTERRUPT_VECTOR handler

[Ingo Molnar]

* Sheng Yang <sheng@linux.intel.com> wrote:

> Otherwise would cause trouble...
> 
> Cc: Dimitri Sivanich <sivanich@sgi.com>
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  arch/x86/kernel/irq.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> index b0cdde6..78b23d0 100644
> --- a/arch/x86/kernel/irq.c
> +++ b/arch/x86/kernel/irq.c
> @@ -257,7 +257,8 @@ void smp_generic_interrupt(struct pt_regs *regs)
>  {
>  	struct pt_regs *old_regs = set_irq_regs(regs);
>  
> -	ack_APIC_irq();
> +	if (!disable_apic)
> +		ack_APIC_irq();

Wont in that case the apic->ack method be a NOP?

	Ingo

Re: [PATCH] x86: Don't ack_APIC_irq() if lapic is disabled in GENERIC_INTERRUPT_VECTOR handler

[Cyrill Gorcunov]

[Ingo Molnar - Sun, Sep 20, 2009 at 08:06:32PM +0200]
| 
| * Sheng Yang <sheng@linux.intel.com> wrote:
| 
| > Otherwise would cause trouble...
| > 
| > Cc: Dimitri Sivanich <sivanich@sgi.com>
| > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
| > ---
| >  arch/x86/kernel/irq.c |    3 ++-
| >  1 files changed, 2 insertions(+), 1 deletions(-)
| > 
| > diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
| > index b0cdde6..78b23d0 100644
| > --- a/arch/x86/kernel/irq.c
| > +++ b/arch/x86/kernel/irq.c
| > @@ -257,7 +257,8 @@ void smp_generic_interrupt(struct pt_regs *regs)
| >  {
| >  	struct pt_regs *old_regs = set_irq_regs(regs);
| >  
| > -	ack_APIC_irq();
| > +	if (!disable_apic)
| > +		ack_APIC_irq();
| 
| Wont in that case the apic->ack method be a NOP?
| 
| 	Ingo
| 

iirc it was Xen related patch. So it's not that simple.

I've pointed out Sheng about disable_apic. I'm not Xen
specialist but Xen team seem to use specific apic setup
so our "dummy" operations are not involved (case they
set disable_apic=1 without "turn off" apic ops in real).
Something like that.

	-- Cyrill

Re: [PATCH] x86: Don't ack_APIC_irq() if lapic is disabled in GENERIC_INTERRUPT_VECTOR handler

[Ingo Molnar]

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

> [Ingo Molnar - Sun, Sep 20, 2009 at 08:06:32PM +0200]
> | 
> | * Sheng Yang <sheng@linux.intel.com> wrote:
> | 
> | > Otherwise would cause trouble...
> | > 
> | > Cc: Dimitri Sivanich <sivanich@sgi.com>
> | > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> | > ---
> | >  arch/x86/kernel/irq.c |    3 ++-
> | >  1 files changed, 2 insertions(+), 1 deletions(-)
> | > 
> | > diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> | > index b0cdde6..78b23d0 100644
> | > --- a/arch/x86/kernel/irq.c
> | > +++ b/arch/x86/kernel/irq.c
> | > @@ -257,7 +257,8 @@ void smp_generic_interrupt(struct pt_regs *regs)
> | >  {
> | >  	struct pt_regs *old_regs = set_irq_regs(regs);
> | >  
> | > -	ack_APIC_irq();
> | > +	if (!disable_apic)
> | > +		ack_APIC_irq();
> | 
> | Wont in that case the apic->ack method be a NOP?
> | 
> | 	Ingo
> | 
> 
> iirc it was Xen related patch. So it's not that simple.
> 
> I've pointed out Sheng about disable_apic. I'm not Xen
> specialist but Xen team seem to use specific apic setup
> so our "dummy" operations are not involved (case they
> set disable_apic=1 without "turn off" apic ops in real).
> Something like that.

They should then set a NOP function in that case. We really dont want to 
slow down hotpath functions like smp_generic_interrupt() with flaggery.

	Ingo

Re: [PATCH] x86: Don't ack_APIC_irq() if lapic is disabled in GENERIC_INTERRUPT_VECTOR handler

[Cyrill Gorcunov]

[Ingo Molnar - Sun, Sep 20, 2009 at 08:21:24PM +0200]
| 
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > [Ingo Molnar - Sun, Sep 20, 2009 at 08:06:32PM +0200]
| > | 
| > | * Sheng Yang <sheng@linux.intel.com> wrote:
| > | 
| > | > Otherwise would cause trouble...
| > | > 
| > | > Cc: Dimitri Sivanich <sivanich@sgi.com>
| > | > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
| > | > ---
| > | >  arch/x86/kernel/irq.c |    3 ++-
| > | >  1 files changed, 2 insertions(+), 1 deletions(-)
| > | > 
| > | > diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
| > | > index b0cdde6..78b23d0 100644
| > | > --- a/arch/x86/kernel/irq.c
| > | > +++ b/arch/x86/kernel/irq.c
| > | > @@ -257,7 +257,8 @@ void smp_generic_interrupt(struct pt_regs *regs)
| > | >  {
| > | >  	struct pt_regs *old_regs = set_irq_regs(regs);
| > | >  
| > | > -	ack_APIC_irq();
| > | > +	if (!disable_apic)
| > | > +		ack_APIC_irq();
| > | 
| > | Wont in that case the apic->ack method be a NOP?
| > | 
| > | 	Ingo
| > | 
| > 
| > iirc it was Xen related patch. So it's not that simple.
| > 
| > I've pointed out Sheng about disable_apic. I'm not Xen
| > specialist but Xen team seem to use specific apic setup
| > so our "dummy" operations are not involved (case they
| > set disable_apic=1 without "turn off" apic ops in real).
| > Something like that.
| 
| They should then set a NOP function in that case. We really dont want to 
| slow down hotpath functions like smp_generic_interrupt() with flaggery.
| 
| 	Ingo
| 

Well, I suppose we should wait for Sheng's comments.
I wish I would answer you but I simply don't know Xen
code :)

	-- Cyrill

Re: [PATCH] x86: Don't ack_APIC_irq() if lapic is disabled in GENERIC_INTERRUPT_VECTOR handler

[Cyrill Gorcunov]

[Cyrill Gorcunov - Sun, Sep 20, 2009 at 10:30:11PM +0400]
...
| | > 
| | > iirc it was Xen related patch. So it's not that simple.
| | > 
| | > I've pointed out Sheng about disable_apic. I'm not Xen
| | > specialist but Xen team seem to use specific apic setup
| | > so our "dummy" operations are not involved (case they
| | > set disable_apic=1 without "turn off" apic ops in real).
| | > Something like that.
| | 
| | They should then set a NOP function in that case. We really dont want to 
| | slow down hotpath functions like smp_generic_interrupt() with flaggery.
| | 
| | 	Ingo
| | 
| 
| Well, I suppose we should wait for Sheng's comments.
| I wish I would answer you but I simply don't know Xen
| code :)
| 
| 	-- Cyrill

Wait a bit Ingo, please. It seems I'm having different
patch series in mind. Need to restore mail thread.
Will back soon :)

	-- Cyrill

Re: [PATCH] x86: Don't ack_APIC_irq() if lapic is disabled in GENERIC_INTERRUPT_VECTOR handler

[Cyrill Gorcunov]

[Cyrill Gorcunov - Sun, Sep 20, 2009 at 10:42:03PM +0400]
| [Cyrill Gorcunov - Sun, Sep 20, 2009 at 10:30:11PM +0400]
| ...
| | | > 
| | | > iirc it was Xen related patch. So it's not that simple.
| | | > 
| | | > I've pointed out Sheng about disable_apic. I'm not Xen
| | | > specialist but Xen team seem to use specific apic setup
| | | > so our "dummy" operations are not involved (case they
| | | > set disable_apic=1 without "turn off" apic ops in real).
| | | > Something like that.
| | | 
| | | They should then set a NOP function in that case. We really dont want to 
| | | slow down hotpath functions like smp_generic_interrupt() with flaggery.
| | | 
| | | 	Ingo
| | | 
| | 
| | Well, I suppose we should wait for Sheng's comments.
| | I wish I would answer you but I simply don't know Xen
| | code :)
| | 
| | 	-- Cyrill
| 
| Wait a bit Ingo, please. It seems I'm having different
| patch series in mind. Need to restore mail thread.
| Will back soon :)
| 
| 	-- Cyrill

yeah, it comes from Xen RFC series. Here is a quote from
conversation.

> Sheng Yan
>
> | | is there was some problem with it? I'm asking you
> | | because if disable_apic=1 then any apic write/read
> | | operations become NOPs. So I don't see how it may
> | | hurt. But I could be missing something.
> | |
> | |   -- Cyrill
> |
> | Ah, I see -- it's due to your other patch...
> | Hmm this makes all "disable apic" idea less
> | general. And safety of ack_APIC_irq is now
> | under suspicious.
>
> Um, probably. I've seen a ack_APIC_irq() in do_IRQ when handle_irq() fail.
> Seems the assumption that ack_APIC_irq() always safe is there. I will check if
> I can make it more elegant - maybe disable the warning in the Xen code...
>

Personally, I think "out-of-xen-thread" this patch is not needed.
And if this apic-ack operation causes any kind of problems --
this problem should be fixed without disable_apic involved.

	-- Cyrill

Re: [PATCH] x86: Don't ack_APIC_irq() if lapic is disabled in GENERIC_INTERRUPT_VECTOR handler

[Sheng Yang]

On Monday 21 September 2009 02:49:06 Cyrill Gorcunov wrote:
> [Cyrill Gorcunov - Sun, Sep 20, 2009 at 10:42:03PM +0400]
>
> | [Cyrill Gorcunov - Sun, Sep 20, 2009 at 10:30:11PM +0400]
> | ...
> |
> | | | > iirc it was Xen related patch. So it's not that simple.
> | | | >
> | | | > I've pointed out Sheng about disable_apic. I'm not Xen
> | | | > specialist but Xen team seem to use specific apic setup
> | | | > so our "dummy" operations are not involved (case they
> | | | > set disable_apic=1 without "turn off" apic ops in real).
> | | | > Something like that.
> | | |
> | | | They should then set a NOP function in that case. We really dont want
> | | | to slow down hotpath functions like smp_generic_interrupt() with
> | | | flaggery.
> | | |
> | | | 	Ingo
> | |
> | | Well, I suppose we should wait for Sheng's comments.
> | | I wish I would answer you but I simply don't know Xen
> | | code :)
> | |
> | | 	-- Cyrill
> |
> | Wait a bit Ingo, please. It seems I'm having different
> | patch series in mind. Need to restore mail thread.
> | Will back soon :)
> |
> | 	-- Cyrill
>
> yeah, it comes from Xen RFC series. Here is a quote from
> conversation.
>
> > Sheng Yan
> >
> > | | is there was some problem with it? I'm asking you
> > | | because if disable_apic=1 then any apic write/read
> > | | operations become NOPs. So I don't see how it may
> > | | hurt. But I could be missing something.
> > | |
> > | |   -- Cyrill
> > |
> > | Ah, I see -- it's due to your other patch...
> > | Hmm this makes all "disable apic" idea less
> > | general. And safety of ack_APIC_irq is now
> > | under suspicious.
> >
> > Um, probably. I've seen a ack_APIC_irq() in do_IRQ when handle_irq()
> > fail. Seems the assumption that ack_APIC_irq() always safe is there. I
> > will check if I can make it more elegant - maybe disable the warning in
> > the Xen code...
>
> Personally, I think "out-of-xen-thread" this patch is not needed.
> And if this apic-ack operation causes any kind of problems --
> this problem should be fixed without disable_apic involved.

In fact Xen set it to functional nop, but print warning when processing APIC 
access. So Xen assumed that no apic access would in the Xen path. But now this 
assumption have a little trouble with current hybrid implementation. I am 
working on to fix this issue now, so please ignore this patch temporarily, I 
would try to figure out a more elegant way to handle this issue and minimal  
the native impact.

Thanks.

-- 
regards
Yang, Sheng