[PATCH] ACPI / APEI: Fix NMI notification handling

[PATCH] ACPI / APEI: Fix NMI notification handling

[Prarit Bhargava]

When removing and adding cpu 0 on a system with GHES NMI the following stack
trace is seen when re-adding the cpu:

WARNING: CPU: 0 PID: 0 at arch/x86/kernel/apic/apic.c:1349 setup_local_APIC+
Modules linked in: nfsv3 rpcsec_gss_krb5 nfsv4 nfs fscache coretemp intel_ra
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.9.0-rc5+ #59
Hardware name: Intel Corporation S2600WFT/S2600WFT, BIOS SE5C620.86B.01.00.0
ffffffff81c03e78 ffffffff81337905 0000000000000000 0000000000000000
ffffffff81c03eb8 ffffffff8107d9c1 00000545810aac4a 0000000000000000
00000000000000f0 0000000000000000 000081cb6440f1d0 0000000000000001
Call Trace:
[<ffffffff81337905>] dump_stack+0x63/0x8e
[<ffffffff8107d9c1>] __warn+0xd1/0xf0
[<ffffffff8107daad>] warn_slowpath_null+0x1d/0x20
[<ffffffff810522b5>] setup_local_APIC+0x275/0x370
[<ffffffff810523be>] apic_ap_setup+0xe/0x20
[<ffffffff8104f5a8>] start_secondary+0x48/0x180
[<ffffffff81d89aa0>] ? set_init_arg+0x55/0x55
[<ffffffff81d89120>] ? early_idt_handler_array+0x120/0x120
[<ffffffff81d895d6>] ? x86_64_start_reservations+0x2a/0x2c
[<ffffffff81d89715>] ? x86_64_start_kernel+0x13d/0x14c
---[ end trace 7b6555b6343ef9ee ]---

During the cpu bringup, wakeup_cpu_via_init_nmi() is called and issues an
NMI on CPU 0.  The GHES NMI handler, ghes_notify_nmi() runs the
ghes_proc_irq_work work queue which ends up setting IRQ_WORK_VECTOR
(0xf6).  The "faulty" IR line set at arch/x86/kernel/apic/apic.c:1349 is  also
0xf6 (specifically APIC IRR for irqs 255 to 224 is 0x400000) which confirms
that something has set the IRQ_WORK_VECTOR line prior to the APIC being
initialized.

Commit 2383844d4850 ("GHES: Elliminate double-loop in the NMI handler")
incorrectly modified the behavior such that the handler returns
NMI_HANDLED only if an error was processed, and incorrectly runs the ghes
work queue for every NMI.

This patch modifies the ghes_proc_irq_work() to run as it did prior to
2383844d4850 ("GHES: Elliminate double-loop in the NMI handler") by
properly returning NMI_HANDLED and only calling the work queue if
NMI_HANDLED has been set.

Fixes: 2383844d4850 ("GHES: Elliminate double-loop in the NMI handler")
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Len Brown <lenb@kernel.org>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: Tyler Baicar <tbaicar@codeaurora.org>
Cc: Punit Agrawal <punit.agrawal@arm.com>
Cc: Don Zickus <dzickus@redhat.com>
Cc: linux-acpi@vger.kernel.org
---
 drivers/acpi/apei/ghes.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 0d099a24f776..39c45efbcb3d 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -858,17 +858,18 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
 		if (sev >= GHES_SEV_PANIC)
 			__ghes_panic(ghes);
 
+		ret = NMI_HANDLED;
+
 		if (!(ghes->flags & GHES_TO_CLEAR))
 			continue;
 
 		__process_error(ghes);
 		ghes_clear_estatus(ghes);
-
-		ret = NMI_HANDLED;
 	}
 
 #ifdef CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG
-	irq_work_queue(&ghes_proc_irq_work);
+	if (ret == NMI_HANDLED)
+		irq_work_queue(&ghes_proc_irq_work);
 #endif
 	atomic_dec(&ghes_in_nmi);
 	return ret;
-- 
1.7.9.3

Re: [PATCH] ACPI / APEI: Fix NMI notification handling

[Borislav Petkov]

On Tue, Nov 29, 2016 at 01:43:59PM -0500, Prarit Bhargava wrote:
> When removing and adding cpu 0 on a system with GHES NMI the following stack
> trace is seen when re-adding the cpu:
> 
> WARNING: CPU: 0 PID: 0 at arch/x86/kernel/apic/apic.c:1349 setup_local_APIC+
> Modules linked in: nfsv3 rpcsec_gss_krb5 nfsv4 nfs fscache coretemp intel_ra
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.9.0-rc5+ #59
> Hardware name: Intel Corporation S2600WFT/S2600WFT, BIOS SE5C620.86B.01.00.0
> ffffffff81c03e78 ffffffff81337905 0000000000000000 0000000000000000
> ffffffff81c03eb8 ffffffff8107d9c1 00000545810aac4a 0000000000000000
> 00000000000000f0 0000000000000000 000081cb6440f1d0 0000000000000001
> Call Trace:
> [<ffffffff81337905>] dump_stack+0x63/0x8e
> [<ffffffff8107d9c1>] __warn+0xd1/0xf0
> [<ffffffff8107daad>] warn_slowpath_null+0x1d/0x20
> [<ffffffff810522b5>] setup_local_APIC+0x275/0x370
> [<ffffffff810523be>] apic_ap_setup+0xe/0x20
> [<ffffffff8104f5a8>] start_secondary+0x48/0x180
> [<ffffffff81d89aa0>] ? set_init_arg+0x55/0x55
> [<ffffffff81d89120>] ? early_idt_handler_array+0x120/0x120
> [<ffffffff81d895d6>] ? x86_64_start_reservations+0x2a/0x2c
> [<ffffffff81d89715>] ? x86_64_start_kernel+0x13d/0x14c
> ---[ end trace 7b6555b6343ef9ee ]---

Please remove all hex numbers from the splat - they're useless in the
commit message.

> During the cpu bringup, wakeup_cpu_via_init_nmi() is called and issues an
> NMI on CPU 0.  The GHES NMI handler, ghes_notify_nmi() runs the
> ghes_proc_irq_work work queue which ends up setting IRQ_WORK_VECTOR
> (0xf6).  The "faulty" IR line set at arch/x86/kernel/apic/apic.c:1349 is  also
> 0xf6 (specifically APIC IRR for irqs 255 to 224 is 0x400000) which confirms
> that something has set the IRQ_WORK_VECTOR line prior to the APIC being
> initialized.
>
> Commit 2383844d4850 ("GHES: Elliminate double-loop in the NMI handler")
> incorrectly modified the behavior such that the handler returns
> NMI_HANDLED only if an error was processed, and incorrectly runs the ghes
> work queue for every NMI.
> 
> This patch modifies the ghes_proc_irq_work() to run as it did prior to
> 2383844d4850 ("GHES: Elliminate double-loop in the NMI handler") by
> properly returning NMI_HANDLED and only calling the work queue if
> NMI_HANDLED has been set.
> 
> Fixes: 2383844d4850 ("GHES: Elliminate double-loop in the NMI handler")
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: Tyler Baicar <tbaicar@codeaurora.org>
> Cc: Punit Agrawal <punit.agrawal@arm.com>
> Cc: Don Zickus <dzickus@redhat.com>
> Cc: linux-acpi@vger.kernel.org
> ---
>  drivers/acpi/apei/ghes.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 0d099a24f776..39c45efbcb3d 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -858,17 +858,18 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
>  		if (sev >= GHES_SEV_PANIC)
>  			__ghes_panic(ghes);
>  
> +		ret = NMI_HANDLED;
> +

Make that more explicit:

                if (ghes_read_estatus(ghes, 1)) {
                        ghes_clear_estatus(ghes);
                        continue;
                } else {
			ret = NMI_HANDLED;
		}


>  		if (!(ghes->flags & GHES_TO_CLEAR))
>  			continue;
>  
>  		__process_error(ghes);
>  		ghes_clear_estatus(ghes);
> -
> -		ret = NMI_HANDLED;
>  	}
>  
>  #ifdef CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG
> -	irq_work_queue(&ghes_proc_irq_work);
> +	if (ret == NMI_HANDLED)
> +		irq_work_queue(&ghes_proc_irq_work);
>  #endif
>  	atomic_dec(&ghes_in_nmi);
>  	return ret;
> -- 

Otherwise looks ok,
thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH v2] ACPI / APEI: Fix NMI notification handling

[Rafael J. Wysocki]

On Thursday, December 01, 2016 09:07:39 PM Borislav Petkov wrote:
> On Wed, Nov 30, 2016 at 08:19:39AM -0500, Prarit Bhargava wrote:
> > When removing and adding cpu 0 on a system with GHES NMI the following stack
> > trace is seen when re-adding the cpu:
> > 
> > WARNING: CPU: 0 PID: 0 at arch/x86/kernel/apic/apic.c:1349 setup_local_APIC+
> > Modules linked in: nfsv3 rpcsec_gss_krb5 nfsv4 nfs fscache coretemp intel_ra
> > CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.9.0-rc6+ #2
> > Call Trace:
> >  dump_stack+0x63/0x8e
> >  __warn+0xd1/0xf0
> >  warn_slowpath_null+0x1d/0x20
> >  setup_local_APIC+0x275/0x370
> >  apic_ap_setup+0xe/0x20
> >  start_secondary+0x48/0x180
> >  set_init_arg+0x55/0x55
> >  early_idt_handler_array+0x120/0x120
> >  x86_64_start_reservations+0x2a/0x2c
> >  x86_64_start_kernel+0x13d/0x14c
> > 
> > During the cpu bringup, wakeup_cpu_via_init_nmi() is called and issues an
> > NMI on CPU 0.  The GHES NMI handler, ghes_notify_nmi() runs the
> > ghes_proc_irq_work work queue which ends up setting IRQ_WORK_VECTOR
> > (0xf6).  The "faulty" IR line set at arch/x86/kernel/apic/apic.c:1349 is  also
> > 0xf6 (specifically APIC IRR for irqs 255 to 224 is 0x400000) which confirms
> > that something has set the IRQ_WORK_VECTOR line prior to the APIC being
> > initialized.
> > 
> > Commit 2383844d4850 ("GHES: Elliminate double-loop in the NMI handler")
> > incorrectly modified the behavior such that the handler returns
> > NMI_HANDLED only if an error was processed, and incorrectly runs the ghes
> > work queue for every NMI.
> > 
> > This patch modifies the ghes_proc_irq_work() to run as it did prior to
> > 2383844d4850 ("GHES: Elliminate double-loop in the NMI handler") by
> > properly returning NMI_HANDLED and only calling the work queue if
> > NMI_HANDLED has been set.
> > 
> > v2: Borislav, setting of NMI_HANDLED moved & cleaned up changelog.
> > 
> > Fixes: 2383844d4850 ("GHES: Elliminate double-loop in the NMI handler")
> > Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: Len Brown <lenb@kernel.org>
> > Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> > Cc: Tyler Baicar <tbaicar@codeaurora.org>
> > Cc: Punit Agrawal <punit.agrawal@arm.com>
> > Cc: Don Zickus <dzickus@redhat.com>
> > ---
> >  drivers/acpi/apei/ghes.c |    7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Borislav Petkov <bp@suse.de>

I guess I should pick up this one, then?

Thanks,
Rafael

Re: [PATCH v2] ACPI / APEI: Fix NMI notification handling

[Rafael J. Wysocki]

On Thursday, December 01, 2016 10:51:49 PM Borislav Petkov wrote:
> On Thu, Dec 01, 2016 at 10:17:54PM +0100, Rafael J. Wysocki wrote:
> > I guess I should pick up this one, then?
> 
> If you already have stuff touching this area, it probably would be more
> prudent if you took it. If not, I can take it through tip's ras branch,
> if you prefer.

Well, there's another ARM-related patch touching APEI.

I guess whoever takes this one should also take the other one and
honestly they can go in via any tree as far as I'm concerned, I'm just trying to
avoid merge clashes here. :-)

Thanks,
Rafael

Re: [PATCH v2] ACPI / APEI: Fix NMI notification handling

[Borislav Petkov]

On Thu, Dec 01, 2016 at 11:29:45PM +0100, Rafael J. Wysocki wrote:
> Well, there's another ARM-related patch touching APEI.
> 
> I guess whoever takes this one should also take the other one and
> honestly they can go in via any tree as far as I'm concerned, I'm just trying to
> avoid merge clashes here. :-)

Maybe have ARM-folk ACK the other one and take both through your ACPI
tree? They both do have ACPI in common :-)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v2] ACPI / APEI: Fix NMI notification handling

[Rafael J. Wysocki]

On Thursday, December 01, 2016 11:47:17 PM Borislav Petkov wrote:
> On Thu, Dec 01, 2016 at 11:29:45PM +0100, Rafael J. Wysocki wrote:
> > Well, there's another ARM-related patch touching APEI.
> > 
> > I guess whoever takes this one should also take the other one and
> > honestly they can go in via any tree as far as I'm concerned, I'm just trying to
> > avoid merge clashes here. :-)
> 
> Maybe have ARM-folk ACK the other one and take both through your ACPI
> tree? They both do have ACPI in common :-)

That one have been ACKed already.

OK, I'll take them both.

Thanks,
Rafael

Re: [PATCH v2] ACPI / APEI: Fix NMI notification handling

[Hanjun Guo]

On 2016/12/2 7:12, Rafael J. Wysocki wrote:
> On Thursday, December 01, 2016 11:47:17 PM Borislav Petkov wrote:
>> On Thu, Dec 01, 2016 at 11:29:45PM +0100, Rafael J. Wysocki wrote:
>>> Well, there's another ARM-related patch touching APEI.
>>>
>>> I guess whoever takes this one should also take the other one and
>>> honestly they can go in via any tree as far as I'm concerned, I'm just trying to
>>> avoid merge clashes here. :-)
>> Maybe have ARM-folk ACK the other one and take both through your ACPI
>> tree? They both do have ACPI in common :-)
> That one have been ACKed already.
>
> OK, I'll take them both.

Thank you very much :)

Hanjun

Re: [PATCH v2] ACPI / APEI: Fix NMI notification handling

[Fu Wei]

Hi Rafael,

On 2 December 2016 at 07:12, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, December 01, 2016 11:47:17 PM Borislav Petkov wrote:
>> On Thu, Dec 01, 2016 at 11:29:45PM +0100, Rafael J. Wysocki wrote:
>> > Well, there's another ARM-related patch touching APEI.
>> >
>> > I guess whoever takes this one should also take the other one and
>> > honestly they can go in via any tree as far as I'm concerned, I'm just trying to
>> > avoid merge clashes here. :-)
>>
>> Maybe have ARM-folk ACK the other one and take both through your ACPI
>> tree? They both do have ACPI in common :-)
>
> That one have been ACKed already.

I guess that is "[PATCH v15] acpi, apei, arm64: APEI initial support
for aarch64."

>
> OK, I'll take them both.

Great thanks, Rafael :-)

>
> Thanks,
> Rafael
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat