acpi: apei: Improve error handling with firmware-first

BIOSes like to send NMIs for a number of silly reasons often deemed to be "fatal". For example pin bounce during a PCIE hotplug/unplug might cause the link to go down and retrain, with fatal PCI errors being generated while the link is retraining.

Instead of panic()ing in NMI context, pass fatal errors down to IRQ context to see if they can be resolved.

With these change, PCIe error are handled by AER. Other far less common errors, such as machine check exceptions, still cause a panic() in their respective handlers.

drivers/acpi/apei/ghes.c +1 -1

index 2c998125b1d5..7243a99ea57e 100644
@@ -955,7 +962,7 @@ static void __process_error(struct ghes *ghes)
 static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
 {
 	struct ghes *ghes;
-	int sev, ret = NMI_DONE;
+	int ret = NMI_DONE;
 
 	if (!atomic_add_unless(&ghes_in_nmi, 1, 1))
 		return ret;

@@ -968,13 +975,6 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
 			ret = NMI_HANDLED;
 		}

Hi Alexandru,

Quoted 4 lines · Alexandru Gagniuc

BIOSes like to send NMIs for a number of silly reasons often deemed to be "fatal". For example pin bounce during a PCIE hotplug/unplug might cause the link to go down and retrain, with fatal PCI errors being generated while the link is retraining.

Sounds fun!

Quoted 2 lines

Instead of panic()ing in NMI context, pass fatal errors down to IRQ context to see if they can be resolved.

How do we know we will survive this trip?

On arm64 systems it may not be possible to return to the context we took the NMI notification from: we may bounce back into firmware with the same "world is on fire" error. Firmware can rightly assume the OS has made no attempt to handle the error.

Your 'not re-arming the error' example makes this worrying.

Quoted 3 lines

With these change, PCIe error are handled by AER. Other far less common errors, such as machine check exceptions, still cause a panic() in their respective handlers.

I agree AER is always going to be different. Could we take a look at the CPER records while still in_nmi() to decide whether linux knows better than firmware?

For non-standard or processor-errors I think we should always panic() if they're marked as fatal.
For memory-errors we could split memory_failure() up to have {NMI,IRQ,process}-context helpers, all we need to know at NMI-time is whether the affected memory is kernel memory.

For the PCI*/AER errors we should be able to try and handle it ... if we can get back to process/IRQ context:
What happens if a PCIe driver has interrupts masked and is doing something to cause this error? We can take the NMI and setup the irq-work, but it may never run as we return to interrupts-masked context.

Quoted 18 lines

drivers/acpi/apei/ghes.c +1 -1

index 2c998125b1d5..7243a99ea57e 100644
@@ -955,7 +962,7 @@ static void __process_error(struct ghes *ghes)
 static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
 {
 	struct ghes *ghes;
-	int sev, ret = NMI_DONE;
+	int ret = NMI_DONE;
 
 	if (!atomic_add_unless(&ghes_in_nmi, 1, 1))
 		return ret;

@@ -968,13 +975,6 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
 			ret = NMI_HANDLED;
 		}
 

drivers/acpi/apei/ghes.c static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) · line 979

Quoted 9 lines

drivers/acpi/apei/ghes.c +0 -7

-		sev = ghes_severity(ghes->estatus->error_severity);
-		if (sev >= GHES_SEV_PANIC) {
-			oops_begin();
-			ghes_print_queued_estatus();
-			__ghes_panic(ghes);
-		}
-
 		if (!(ghes->flags & GHES_TO_CLEAR))
 			continue;

For Processor-errors I think this is the wrong thing to do, but we should be able to poke around in the CPER records and find out what we are dealing with.

Thanks,

James

Quoted 5 lines · James Morse

Quoted 4 lines · Alexandru Gagniuc

BIOSes like to send NMIs for a number of silly reasons often deemed to be "fatal". For example pin bounce during a PCIE hotplug/unplug might cause the link to go down and retrain, with fatal PCI errors being generated while the link is retraining.

Quoted 4 lines

Quoted 2 lines

Instead of panic()ing in NMI context, pass fatal errors down to IRQ context to see if they can be resolved.

How do we know we will survive this trip?

We don't.

Quoted 4 lines

On arm64 systems it may not be possible to return to the context we took the NMI notification from: we may bounce back into firmware with the same "world is on fire" error. Firmware can rightly assume the OS has made no attempt to handle the error.

I'm not aware of the NMI path being used on arm64:

$ git grep 'select HAVE_ACPI_APEI_NMI'
arch/x86/Kconfig:       select HAVE_ACPI_APEI_NMI               if ACPI
$

A far as jumping back into firmware, the OS should clear the GHES block status before exiting the NMI. THis tells the firmware that the OS got the message [1]
I'm not seeing any mechanism to say "handled/not handled", so I'm not sure how firmware can make such assumptions.

Quoted 1 line

Your 'not re-arming the error' example makes this worrying.

Those are one class of bugs you get when you let firmware run the show. It's been an issue since day one of EFI. The best you can do in such cases is accept you may later lose the link to a device.
Side-note: We have a BIOS defect filed with against this particular issue, and it only seems to affect PCIe SMIs.

Quoted 7 lines

Quoted 3 lines

With these change, PCIe error are handled by AER. Other far less common errors, such as machine check exceptions, still cause a panic() in their respective handlers.

I agree AER is always going to be different. Could we take a look at the CPER records while still in_nmi() to decide whether linux knows better than firmware?

Quoted 5 lines

For non-standard or processor-errors I think we should always panic() if they're marked as fatal.
For memory-errors we could split memory_failure() up to have {NMI,IRQ,process}-context helpers, all we need to know at NMI-time is whether the affected memory is kernel memory.

I'm trying to avoid splitting up the recovery logic based on _how_ we were notified of the error.

The only sure way to know if you've handled
an error is to try to handle it. Not handling _any_ fatal error still results in a panic(), and I was very careful to ensure that.

The alternative is to keep ghes_do_proc() and ghes_notify_nmi() in sync, duplicate code between the two. But we can also get ghes_proc_irq_work() as an entry point, and this easily turns in a maintenance nightmare.

Quoted 5 lines

For the PCI*/AER errors we should be able to try and handle it ... if we can get back to process/IRQ context:
What happens if a PCIe driver has interrupts masked and is doing something to cause this error? We can take the NMI and setup the irq-work, but it may never run as we return to interrupts-masked context.

The way the recovery from NMI is set up is confusing. On NMIs, ghes_proc_in_irq() is queued up to do further work. Despite the name, in this case it runs as a task. That further queues up AER work, Either way, PCIe endpoint interrupts are irrelevant here. AER recovery runs independent of drivers or interrupts:

- Drivers that implement AER recovery services are expected to do the
right thing and shut down IO. To not do so, is a bug in the driver.
- Drivers that do not implement AER have their devices taken offline.

It is not uncommon for PCIe errors to amplify. Imagine losing the link right after you've fired off several DMA queues. You'd get a stream of Unsupported Request errors. AER handles this fairly well.

Quoted 17 lines

Quoted 17 lines

drivers/acpi/apei/ghes.c +1 -1

index 2c998125b1d5..7243a99ea57e 100644
@@ -955,7 +962,7 @@ static void __process_error(struct ghes *ghes)
 static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
 {
 	struct ghes *ghes;
-	int sev, ret = NMI_DONE;
+	int ret = NMI_DONE;
 
 	if (!atomic_add_unless(&ghes_in_nmi, 1, 1))
 		return ret;

@@ -968,13 +975,6 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
 			ret = NMI_HANDLED;
 		}
 

Quoted 12 lines

drivers/acpi/apei/ghes.c static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) · line 979

Quoted 9 lines

drivers/acpi/apei/ghes.c +0 -7

-		sev = ghes_severity(ghes->estatus->error_severity);
-		if (sev >= GHES_SEV_PANIC) {
-			oops_begin();
-			ghes_print_queued_estatus();
-			__ghes_panic(ghes);
-		}
-
 		if (!(ghes->flags & GHES_TO_CLEAR))
 			continue;

For Processor-errors I think this is the wrong thing to do, but we should be able to poke around in the CPER records and find out what we are dealing with.

I don't think being trigger-happy with the panic button is ever a good thing. The logic to stop the system exists downstream, and is being called.

I can see an issue if we somehow lose the ability to run queued tasks, but in that case, we've already lost control of the machine.

Hi Alex,

Quoted 3 lines

Quoted 1 line

How do we know we will survive this trip?

We don't.

Isn't that even worse than panic()ing? (No output, followed by a watchdog reboot if we're lucky)

Quoted 9 lines

Quoted 4 lines

On arm64 systems it may not be possible to return to the context we took the NMI notification from: we may bounce back into firmware with the same "world is on fire" error. Firmware can rightly assume the OS has made no attempt to handle the error.

I'm not aware of the NMI path being used on arm64:

$ git grep 'select HAVE_ACPI_APEI_NMI'
arch/x86/Kconfig:       select HAVE_ACPI_APEI_NMI               if ACPI
$

(I agree its not obvious!), arm64 has NOTIFY_SEA, NOTIFY_SEI and NOTIFY_SDEI that all behave like NOTIFY_NMI (in that they can interrupt IRQ-masked code).

CONFIG_HAVE_ACPI_APEI_NMI has come to mean NOTIFY_NMI, which requires the arch code to implement register_nmi_handler() and (from memory) behave as a notifier chain, which doesn't fit with how these things behave.

NOTIFY_SEA lives behind CONFIG_ACPI_APEI_SEA in driver/acpi/apei/Kconfig. The series to add SDEI support is on the list here:
https://www.spinics.net/lists/arm-kernel/msg642946.html
(NOTIFY_SEI is very tricky for firmware to get right, I don't think we're there yet.)

From my understanding, your patch series tries to use ghes_notify_nmi()

Wouldn't reasonably-intelligent firmware be able to spot the same fault repeatedly in a loop? (last-fault-address == this-fault-address, last-fault-time==now)

Quoted 7 lines

Quoted 1 line

Your 'not re-arming the error' example makes this worrying.

Those are one class of bugs you get when you let firmware run the show. It's been an issue since day one of EFI. The best you can do in such cases is accept you may later lose the link to a device.
Side-note: We have a BIOS defect filed with against this particular issue, and it only seems to affect PCIe SMIs.

This sounds like policy applied too early

fixing the firmware to at least make this configurable would be preferable.

My point was there is a class of reported-as-fatal error that really is fatal, and for these trying to defer the work to irq_work is worse than panic()ing as we get less information about what happened.

Are all AER errors recoverable? (is an OS:'fatal' AER error ever valid?)

From ACPI, FFS should only send a "fatal" error if a machine needs reset

Quoted 3 lines

The alternative is to keep ghes_do_proc() and ghes_notify_nmi() in sync, duplicate code between the two. But we can also get ghes_proc_irq_work() as an entry point, and this easily turns in a maintenance nightmare.

We can't call all the recovery-helpers from ghes_notify_nmi() as they sleep and allocate memory. I think the trick would be to make the CPER parsing routines NMI-safe so that we can check these 'fatal' records for cases where we're pretty sure linux can do a better job.

Quoted 11 lines

Quoted 5 lines

For the PCI*/AER errors we should be able to try and handle it ... if we can get back to process/IRQ context:
What happens if a PCIe driver has interrupts masked and is doing something to cause this error? We can take the NMI and setup the irq-work, but it may never run as we return to interrupts-masked context.

The way the recovery from NMI is set up is confusing. On NMIs, ghes_proc_in_irq() is queued up to do further work. Despite the name, in this case it runs as a task. That further queues up AER work, Either way, PCIe endpoint interrupts are irrelevant here. AER recovery runs independent of drivers or interrupts:

Hmm, we may be looking at different things, (and I'm not familiar with x86):
ghes_notify_nmi() uses irq_work_queue() to add the work to a percpu llist and call arch_irq_work_raise(). arch_irq_work_raise() sends a self IPI, which, once taken causes the arch code to call irq_work_run() to consume the llist.

My badly made point is we could take the NMI that triggers all this from a context that has interrupts masked. We can't take the self-IPI until we return from the NMI, and we return to a context that still has interrupts masked. The original context may cause the NMI to trigger again before it unmasks interrupts. In this case we've livelocked, the recovery work never runs.

I think something like polling a status register over a broken-link would do this. (has my config write been acknowledge?)

Quoted 2 lines

I can see an issue if we somehow lose the ability to run queued tasks, but in that case, we've already lost control of the machine.

about to loose control of the machine: firmware has saved the day giving us a clean slate and some standardised messages that describe the fault.

I agree we can do better for AER errors, but I don't think we should make the handling of other classes of error worse. These classes match nicely to the CPER record types, which we already have mapped in ghes_notify_nmi().

Quoted 6 lines · James Morse

Quoted 3 lines

Quoted 1 line

How do we know we will survive this trip?

We don't.

Isn't that even worse than panic()ing? (No output, followed by a watchdog reboot if we're lucky)

On the contrary. No output, followed by a watchdog reboot is usually what happens when the panic() is in NMI. One of the very few ways that can be debugged is over a NULL modem cable.

Quoted 17 lines

Quoted 9 lines

Quoted 4 lines

On arm64 systems it may not be possible to return to the context we took the NMI notification from: we may bounce back into firmware with the same "world is on fire" error. Firmware can rightly assume the OS has made no attempt to handle the error.

I'm not aware of the NMI path being used on arm64:

$ git grep 'select HAVE_ACPI_APEI_NMI'
arch/x86/Kconfig:       select HAVE_ACPI_APEI_NMI               if ACPI
$

(I agree its not obvious!), arm64 has NOTIFY_SEA, NOTIFY_SEI and NOTIFY_SDEI that all behave like NOTIFY_NMI (in that they can interrupt IRQ-masked code).

CONFIG_HAVE_ACPI_APEI_NMI has come to mean NOTIFY_NMI, which requires the arch code to implement register_nmi_handler() and (from memory) behave as a notifier chain, which doesn't fit with how these things behave.

Quoted 5 lines

NOTIFY_SEA lives behind CONFIG_ACPI_APEI_SEA in driver/acpi/apei/Kconfig. The series to add SDEI support is on the list here:
https://www.spinics.net/lists/arm-kernel/msg642946.html
(NOTIFY_SEI is very tricky for firmware to get right, I don't think we're there yet.)

From my understanding, your patch series tries to use ghes_notify_nmi() as a common entry point.

But if on arm64, you can return to firmware,
then we have wildly different constraints.

On x86, you can't return to SMM. You can have a new SMI triggered while servicing the NMI, but you can't re-enter an SMI that you've returned from. SMM holds all the cores, and they all leave SMM at roughly the same time, so you don't deal with coexistence issues. This probably also avoids the multiple notifications that you are trying to implement on arm.

On quick thought, I think the best way to go for both series is to leave the entry points arch-specific, and prevent hax86 and harm64 from stepping on each other's toes. Thoughts?

Quoted 3 lines

Wouldn't reasonably-intelligent firmware be able to spot the same fault repeatedly in a loop? (last-fault-address == this-fault-address, last-fault-time==now)

I'm told that the BIOS on Dell machines will detect interrupt storms, and disable the specific error source in such cases. On an x86 server. 50us of SMM eats up several milliseconds-core of CPU time, so they try to be a little careful.

Quoted 9 lines

Quoted 7 lines

Quoted 1 line

Your 'not re-arming the error' example makes this worrying.

Those are one class of bugs you get when you let firmware run the show. It's been an issue since day one of EFI. The best you can do in such cases is accept you may later lose the link to a device.
Side-note: We have a BIOS defect filed with against this particular issue, and it only seems to affect PCIe SMIs.

This sounds like policy applied too early

That's firmware-first in one sentence.

Quoted 1 line

fixing the firmware to at least make this configurable would be preferable.

I can ask for "fixing the firmware" and we might even get the fix, but the fact remains that there are machines in the field with the buggy BIOSes, and there will continue to be such machines even after a fix is released.

Quoted 3 lines

My point was there is a class of reported-as-fatal error that really is fatal, and for these trying to defer the work to irq_work is worse than panic()ing as we get less information about what happened.

How do we get less information? We save the GHES structure on the estatus_queue, and use it as the error source in either case. Is there a class of errors where we can reasonable expect things to be okay in NMI, but go horribly wrong on return from NMI?

(snip)

Quoted 1 line

Are all AER errors recoverable? (is an OS:'fatal' AER error ever valid?)

PCIe "fatal" means that your link is gone and you might not get it back. You lose all the downstream devices. You may be able to reset the link and get it back. This sort of "fatal" has no impact on a machine's ability to continue operation.

From ACPI, FFS should only send a "fatal" error if a machine needs reset [1], so it's clearly a firmware bug to mark any PCIe error "fatal". This won't stop BIOS vendors from taking shortcuts and deciding to crash an OS by sending the error as "fatal".

[1]ACPI 6.2: 18.1Hardware Errors and Error Sources

Quoted 8 lines

Quoted 3 lines

The alternative is to keep ghes_do_proc() and ghes_notify_nmi() in sync, duplicate code between the two. But we can also get ghes_proc_irq_work() as an entry point, and this easily turns in a maintenance nightmare.

We can't call all the recovery-helpers from ghes_notify_nmi() as they sleep and allocate memory. I think the trick would be to make the CPER parsing routines NMI-safe so that we can check these 'fatal' records for cases where we're pretty sure linux can do a better job.

can_i_handle_this_nonblocking(struct four_letter_acronum_err *err) ?

That's the approach I originally tried, but I didn't like it. The parsing gets deeper and deeper, and, by the time you realize if the error can be handled or not, you're already halfway through recovery.

This is a perfectly viable approach. However, is there a palpable (non-theoretical) concern that warrants complicating things this way?

Quoted 12 lines

Quoted 11 lines

Quoted 5 lines

For the PCI*/AER errors we should be able to try and handle it ... if we can get back to process/IRQ context:
What happens if a PCIe driver has interrupts masked and is doing something to cause this error? We can take the NMI and setup the irq-work, but it may never run as we return to interrupts-masked context.

The way the recovery from NMI is set up is confusing. On NMIs, ghes_proc_in_irq() is queued up to do further work. Despite the name, in this case it runs as a task. That further queues up AER work, Either way, PCIe endpoint interrupts are irrelevant here. AER recovery runs independent of drivers or interrupts:

Quoted 4 lines

Hmm, we may be looking at different things, (and I'm not familiar with x86):
ghes_notify_nmi() uses irq_work_queue() to add the work to a percpu llist and call arch_irq_work_raise(). arch_irq_work_raise() sends a self IPI, which, once taken causes the arch code to call irq_work_run() to consume the llist.

I abstract things at irq_work_queue(): "queue this work and let me return from this interrupt"

Quoted 5 lines

My badly made point is we could take the NMI that triggers all this from a context that has interrupts masked. We can't take the self-IPI until we return from the NMI, and we return to a context that still has interrupts masked. The original context may cause the NMI to trigger again before it unmasks interrupts. In this case we've livelocked, the recovery work never runs.

In order to get another NMI from firmware, you need SMM to be triggered again. Remember, SMM has to exit before the NMI can be serviced.

I doubt the livelock is a concern in this place, because we're dealing with NMIs invoked by firmware(SMM). NMIs for other reasons would be handled elsewhere.

In the event that FFS fails to prevent and SMI storm, and keeps spamming the CPU with NMIs, that is a clear firmware bug. It's also the sort of bug I haven't observed in the wild.

Quoted 2 lines

I think something like polling a status register over a broken-link would do this. (has my config write been acknowledge?)

That would not be a valid reason to disable interrupts. Non-posted PCIe requests can take hundreds of milliseconds (memory-mapped or otherwise). I can't think of any sort of code that can cause errors to come in via NMI which would be a suitable atomic context for disabling interrupts.

Maybe on arm it's normal to stuff in anything error-related via NMI? On x86, most error sources have their own (maskable) interrupt vector.

I am not concert about livelocking the system.

Quoted 3 lines

On the contrary. No output, followed by a watchdog reboot is usually what happens when the panic() is in NMI. One of the very few ways that can be debugged is over a NULL modem cable.

(IPMI-SOL?)

Really? printk has NMI support, and panic()s call to bust_spinlocks() should unblank the screen before printk_safe_flush_on_panic() dumps the messages. (I'm assuming some BMC-VGA exists to capture the 'screen').

Quoted 2 lines

But if on arm64, you can return to firmware,
then we have wildly different constraints.

We can't return to firmware, but we can take the error again, causing another trap to firmware.

e.g.: If a program accesses a value in the cache and the cache location is discovered to be corrupt/marked-as-poisoned. This causes an 'external abort' exception, which firmware has configured to trap to firmware. Once there, it generates CPER records and sets-up an external-abort-exception for Linux. If linux returns from this external-abort, we return to whatever program accessed the bad-cache-value in the first case, re-run the instruction that generates the external abort, and the same thing happens again, but to firmware it looks like a second fault at the same address.

Quoted 5 lines

On x86, you can't return to SMM. You can have a new SMI triggered while servicing the NMI, but you can't re-enter an SMI that you've returned from. SMM holds all the cores, and they all leave SMM at roughly the same time, so you don't deal with coexistence issues. This probably also avoids the multiple notifications that you are trying to implement on arm.

its the new SMI case.

(We don't necessarily have all cores pulled into firmware, (although firmware could do that in software), so different CPUs taking NMI-like notifications is one of the things we have to handle.)

Quoted 3 lines

On quick thought, I think the best way to go for both series is to leave the entry points arch-specific, and prevent hax86 and harm64 from stepping on each other's toes. Thoughts?

The behaviour should be driven from the standard. I agree there is some leeway, which linux should handle on all architectures that support GHES.

The entry points from the arch code are already separate, as these have different notification types in the HEST->GHES entries and different registration requirements.
Once in ghes.c all the NMI-like notifications get merged together as the only relevant thing about them is we have to use the estatus-queue, and can't call any of the sleepy recovery helpers.

I don't think this is the issue though: An NMI notification could by any of the eleven CPER notification record types.
Your argument is Linux probably can handle PCIe-AER errors,

even if broken firmware thinks they are fatal.

This is only one of the eleven notification record types. Returning to handle the error definitely doesn't work for some classes of fatal error, especially when interrupts are masked.

I think its straightforward to test whether the CPER records contain only errors where we can do better: (not tested/built, I haven't checked whether this is nmi safe):

---------------------%<---------------------
/*
* Return true if only PCIe-AER CPER sections are present. We ignore the fatal
* severity in this case as linux knows better. Called in_nmi(). */
static bool ghes_is_deferrable(struct ghes *ghes,
const struct acpi_hest_generic_status *estatus)

{
       guid_t *sec_type;
       struct acpi_hest_generic_data *gdata;

       apei_estatus_for_each_section(estatus, gdata) {
               sec_type = (guid_t *)gdata->section_type;
               if (!guid_equal(sec_type, &CPER_SEC_PCIE))

Quoted 4 lines

Quoted 3 lines

My point was there is a class of reported-as-fatal error that really is fatal, and for these trying to defer the work to irq_work is worse than panic()ing as we get less information about what happened.

Quoted 4 lines

How do we get less information? We save the GHES structure on the estatus_queue, and use it as the error source in either case. Is there a class of errors where we can reasonable expect things to be okay in NMI, but go horribly wrong on return from NMI?

Yes, (described above). This could happen if we see Arm64's 'SEA' CPER records or x86's 'MCE' CPER records. I'm pretty sure it will happen on both architectures if you return to a context with interrupts masked.

Quoted 13 lines

(snip)

Quoted 1 line

Are all AER errors recoverable? (is an OS:'fatal' AER error ever valid?)

PCIe "fatal" means that your link is gone and you might not get it back. You lose all the downstream devices. You may be able to reset the link and get it back. This sort of "fatal" has no impact on a machine's ability to continue operation.

From ACPI, FFS should only send a "fatal" error if a machine needs reset [1], so it's clearly a firmware bug to mark any PCIe error "fatal". This won't stop BIOS vendors from taking shortcuts and deciding to crash an OS by sending the error as "fatal".

Quoted 1 line

[1]ACPI 6.2: 18.1Hardware Errors and Error Sources

Great! Its never valid. So we can check the CPER records in the NMI handler, and for AER errors, clear the fatal bit printing a nasty message about the firmware.

Quoted 6 lines

That's the approach I originally tried, but I didn't like it. The parsing gets deeper and deeper, and, by the time you realize if the error can be handled or not, you're already halfway through recovery.

This is a perfectly viable approach. However, is there a palpable (non-theoretical) concern that warrants complicating things this way?

For AER your text above says no, we can always handle an error, they are never fatal.

For memory-errors, yes. An ECC error for a location on the kernel stack while the affected thread was preempt_disable()d. We can't kick the thread off the CPU, and we can't return from the handler as we can't stop accesses to the stack when we return.

We can avoid it being complicated by just separating AER errors out when they come in. You've said they never mean the OS has to reboot.

Quoted 2 lines

I abstract things at irq_work_queue(): "queue this work and let me return from this interrupt"

('from this NMI', to come back later as an IRQ, from where we can call schedule_work_on(). This chaining is because the scheduler takes irqsave spin-locks to schedule the work.)

Quoted 3 lines

In the event that FFS fails to prevent and SMI storm, and keeps spamming the CPU with NMIs, that is a clear firmware bug. It's also the sort of bug I haven't observed in the wild.

Do these SMI ever occur synchronously with the CPUs execution? i.e. if I execute this instruction, I will get an SMI. (or are these your handled-elsewhere-NMI case?)

On arm64 poisoned-cache locations triggering an external abort (leading to a NOTIFY_SEA NMI-like notification to APEI) in response to a load is synchronous, hence the live-lock problem.

Maybe. On arm systems error sources might have their own interrupts that are taken to firmware,

Quoted 1 line

I am not concert about livelocking the system.

This is what keeps me awake at night.

I think your platform has NMI handled as MCE/kernel-first for synchronous CPU errors. SMI triggered by PCI-AER are notified by NMI too, but handled by firmware-first.

This hybrid kernel/firmware first depending on the error-source isn't the only way of building things. Appendix-N of the UEFI spec lists CPER records for firmware-first handling of types of error that your platform must 'handle elsewhere'.

We have arm systems where kernel-first isn't viable as the equivalent of the MCA registers is platform/integration/silicon-revision specific. These systems handle all their errors using firmware-first. These systems will generate CPER records where fatal means fatal, and returning from the NMI-like notification will either send us into the weeds, or trap us back into firmware.

On 04/06/2018 01:24 PM, James Morse wrote:
Do you have any ETA on when your SEA patches are going to make it upstream? There's not much point in updating my patchset if it's going to conflict with your work.

Quoted 6 lines

Quoted 2 lines

But if on arm64, you can return to firmware,
then we have wildly different constraints.

We can't return to firmware, but we can take the error again, causing another trap to firmware.

Quoted 8 lines

e.g.: If a program accesses a value in the cache and the cache location is discovered to be corrupt/marked-as-poisoned. This causes an 'external abort' exception, which firmware has configured to trap to firmware. Once there, it generates CPER records and sets-up an external-abort-exception for Linux. If linux returns from this external-abort, we return to whatever program accessed the bad-cache-value in the first case, re-run the instruction that generates the external abort, and the same thing happens again, but to firmware it looks like a second fault at the same address.

This is a very good example of why we should _not_ panic in NMI. Invalidate the cache-line, next load causes a memory round-trip, and everyone's happy. Of course, FFS can't do that because it doesn't know if it's valid to invalidate (pun intended). But the OS does. So you _want_ to reach the error handler downstream of NMI context, and you do _not_ want to crash.

Quoted 8 lines

Quoted 5 lines

On x86, you can't return to SMM. You can have a new SMI triggered while servicing the NMI, but you can't re-enter an SMI that you've returned from. SMM holds all the cores, and they all leave SMM at roughly the same time, so you don't deal with coexistence issues. This probably also avoids the multiple notifications that you are trying to implement on arm.

its the new SMI case.

Quoted 3 lines

(We don't necessarily have all cores pulled into firmware, (although firmware could do that in software), so different CPUs taking NMI-like notifications is one of the things we have to handle.)

How does FFS handle race conditions that can occur when accessing HW concurrently with the OS? I'm told it's the main reasons why BIOS doesn't release unused cores from SMM early.

Quoted 6 lines

Quoted 3 lines

On quick thought, I think the best way to go for both series is to leave the entry points arch-specific, and prevent hax86 and harm64 from stepping on each other's toes. Thoughts?

The behaviour should be driven from the standard. I agree there is some leeway, which linux should handle on all architectures that support GHES.

"The standard" deals mostly with firmware behavior. While I'm all for following specifications in order to ensure smooth interaction and integration, Linux is an OS and it deals with a plethora of "standards". Its job is to serve user requests. When a "standard" deviates from this, such as mandating a crash when one is not required, it's the OS's job to _not_ follow this requirement.

Quoted 7 lines

Once in ghes.c all the NMI-like notifications get merged together as the only relevant thing about them is we have to use the estatus-queue, and can't call any of the sleepy recovery helpers.

I don't think this is the issue though: An NMI notification could by any of the eleven CPER notification record types.
Your argument is Linux probably can handle PCIe-AER errors,

My argument is that Linux should do a best effort to recover from hardware (or FFS) errors. In support of that argument, I presented evidence that Linux already can handle a variety of errors. How those errors are reported determines whether the OS crashes or not, and that's not right.

When errors are reported natively, we make it to the error handlers before crashing. And the essence of my argument is that there is no reason to have a different behavior with FFS.

Quoted 1 line

even if broken firmware thinks they are fatal.

I think the idea of firmware-first is broken. But it's there, it's shipping in FW, so we have to accommodate it in SW.

Quoted 7 lines

This is only one of the eleven notification record types. Returning to handle the error definitely doesn't work for some classes of fatal error, especially when interrupts are masked.

I think its straightforward to test whether the CPER records contain only errors where we can do better: (not tested/built, I haven't checked whether this is nmi safe):

That looks very similar to what I had originally implemented.

Quoted 11 lines

---------------------%<---------------------
/*
* Return true if only PCIe-AER CPER sections are present. We ignore the fatal
* severity in this case as linux knows better. Called in_nmi(). */
static bool ghes_is_deferrable(struct ghes *ghes,
const struct acpi_hest_generic_status *estatus)

{
       guid_t *sec_type;
       struct acpi_hest_generic_data *gdata;

Quoted 3 lines

       apei_estatus_for_each_section(estatus, gdata) {
               sec_type = (guid_t *)gdata->section_type;
               if (!guid_equal(sec_type, &CPER_SEC_PCIE))

Also need to check if the CPER record contains enough information to identify the cause of the error. There's no guarantee the record even has a valid PCI BDF, let alone error information. Possible? yes. Easy? yes. Advisable? maybe.

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

My point was there is a class of reported-as-fatal error that really is fatal, and for these trying to defer the work to irq_work is worse than panic()ing as we get less information about what happened.

How do we get less information? We save the GHES structure on the estatus_queue, and use it as the error source in either case. Is there a class of errors where we can reasonable expect things to be okay in NMI, but go horribly wrong on return from NMI?

Quoted 3 lines

Yes, (described above). This could happen if we see Arm64's 'SEA' CPER records or x86's 'MCE' CPER records. I'm pretty sure it will happen on both architectures if you return to a context with interrupts masked.

And linux can handle a wide subset of MCEs just fine, so the ghes_is_deferrable() logic would, under my argument, agree to pass execution to the actual handlers.

I do agree that this is one case where
splitting the handler in a top half and bottom half would make sense.

Though in that case, the problem is generalized to "how to best handle error Y", rather than "how to handle error Y in FFS".

Quoted 12 lines

Quoted 12 lines

Quoted 1 line

Are all AER errors recoverable? (is an OS:'fatal' AER error ever valid?)

PCIe "fatal" means that your link is gone and you might not get it back. You lose all the downstream devices. You may be able to reset the link and get it back. This sort of "fatal" has no impact on a machine's ability to continue operation.

From ACPI, FFS should only send a "fatal" error if a machine needs reset [1], so it's clearly a firmware bug to mark any PCIe error "fatal". This won't stop BIOS vendors from taking shortcuts and deciding to crash an OS by sending the error as "fatal".

Quoted 4 lines

Quoted 1 line

[1]ACPI 6.2: 18.1Hardware Errors and Error Sources

Great! Its never valid. So we can check the CPER records in the NMI handler, and for AER errors, clear the fatal bit printing a nasty message about the firmware.

There's the counter-argument that the machine is a "XYZ" appliance, and losing the PCIe link to the "XYZ" device is essentially "fatal" to the (purpose of the) machine. As you noted, there is some leeway ;)

Sarcasm aside, I do like the idea of a message complaining about the firmware.

Quoted 15 lines

Quoted 6 lines

That's the approach I originally tried, but I didn't like it. The parsing gets deeper and deeper, and, by the time you realize if the error can be handled or not, you're already halfway through recovery.

This is a perfectly viable approach. However, is there a palpable (non-theoretical) concern that warrants complicating things this way?

For AER your text above says no, we can always handle an error, they are never fatal.

For memory-errors, yes. An ECC error for a location on the kernel stack while the affected thread was preempt_disable()d. We can't kick the thread off the CPU, and we can't return from the handler as we can't stop accesses to the stack when we return.

Quoted 2 lines

We can avoid it being complicated by just separating AER errors out when they come in. You've said they never mean the OS has to reboot.

If we're going to split recovery paths, then it makes better sense to have a system where handleable errors are passed down to a lesser context. Or even run non-blocking handlers in whatever context they are received. Much more than having a special case for a specific error.

Quoted 6 lines

Quoted 2 lines

I abstract things at irq_work_queue(): "queue this work and let me return from this interrupt"

('from this NMI', to come back later as an IRQ, from where we can call schedule_work_on(). This chaining is because the scheduler takes irqsave spin-locks to schedule the work.)

Hopefully, the handling is set up in such a way that I don't have to worry about these details on a per-error basis. If I do have to worry, them I must be doing something wrong.

Quoted 6 lines

Quoted 3 lines

In the event that FFS fails to prevent and SMI storm, and keeps spamming the CPU with NMIs, that is a clear firmware bug. It's also the sort of bug I haven't observed in the wild.

Do these SMI ever occur synchronously with the CPUs execution? i.e. if I execute this instruction, I will get an SMI. (or are these your handled-elsewhere-NMI case?)

I'm not aware of synchronous exceptions that end up as SMIs, though I imagine it is possible on x86. An IO write to an SMI trap is equivalent to that.

Quoted 3 lines

On arm64 poisoned-cache locations triggering an external abort (leading to a NOTIFY_SEA NMI-like notification to APEI) in response to a load is synchronous, hence the live-lock problem.

How is this currently handled in the kernel-first case?

Quoted 3 lines

Quoted 1 line

I am not concert about livelocking the system.

This is what keeps me awake at night.

Nothing keeps me awake at night. I keep others awake at night.

Quoted 3 lines

I think your platform has NMI handled as MCE/kernel-first for synchronous CPU errors. SMI triggered by PCI-AER are notified by NMI too, but handled by firmware-first.

MCEs come in via FFS.

There's a school of thought that sees a certain
value in logging ECC errors to the BMC. It's arguable whether the ECC errors are synchronous in 100% of cases.

Quoted 5 lines

This hybrid kernel/firmware first depending on the error-source isn't the only way of building things. Appendix-N of the UEFI spec lists CPER records for firmware-first handling of types of error that your platform must 'handle elsewhere'.

Quoted 5 lines

We have arm systems where kernel-first isn't viable as the equivalent of the MCA registers is platform/integration/silicon-revision specific. These systems handle all their errors using firmware-first. These systems will generate CPER records where fatal means fatal, and returning from the NMI-like notification will either send us into the weeds, or trap us back into firmware.

Without getting into details, the non-viablity of kernel-first is subjective.

Hi Alex,

Quoted 4 lines · Alex G.

On 04/06/2018 01:24 PM, James Morse wrote:
Do you have any ETA on when your SEA patches are going to make it upstream? There's not much point in updating my patchset if it's going to conflict with your work.

The SEA stuff went in with 7edda0886bc3 ("acpi: apei: handle SEA notification type for ARMv8"). My series is moving it to use the estatus-queue in the same way as x86's NOTIFY_NMI does. This lets us safely add the other two NMI-like notifications. I have no idea on the ETA, it depends on review feedback!

This assumes a cache-invalidate will clear the error, which I don't think we're guaranteed on arm.
It also destroys any adjacent data, "everyone's happy" includes the thread that got a chunk of someone-else's stack frame, I don't think it will be happy for very long!

(this is a side issue for AER though)

Quoted 3 lines

How does FFS handle race conditions that can occur when accessing HW concurrently with the OS? I'm told it's the main reasons why BIOS doesn't release unused cores from SMM early.

This is firmware's problem, it depends on whether there is any hardware that is shared with the OS. Some hardware can be marked 'secure' in which case only firmware can access it, alternatively firmware can trap or just disable the OS's access to the shared hardware.

For example, with the v8.2 RAS Extensions, there are some per-cpu error registers. Firmware can disable these for the OS, so that it always reads 0 from them. Instead firmware takes the error via FF, reads the registers from firmware, and dumps CPER records into the OS's memory.

If there is a shared hardware resource that both the OS and firmware may be accessing, yes firmware needs to pull the other CPUs in, but this depends on the SoC design, it doesn't necessarily happen.

Sure, we're quirking our behaviour based on a high level of mistrust for the firmware. My point here was we shouldn't duplicate the implementation because we want x86:{AER,CPU,MEM} to behave differently to arm64:{AER,CPU,MEM}. I'd rather the quirked-behaviour was along the *:{AER} versus *:{CPU,MEM} line. If we have extra code to spot deferrable errors, we should use it on both architectures.

For AER we agree, these never mean 'the CPU is on fire'.

Quoted 4 lines

Quoted 1 line

even if broken firmware thinks they are fatal.

I think the idea of firmware-first is broken. But it's there, it's shipping in FW, so we have to accommodate it in SW.

Part of our different-views here is firmware-first is taking something away from you, whereas for me its giving me information that would otherwise be in secret-soc-specific registers.

Quoted 3 lines

And linux can handle a wide subset of MCEs just fine, so the ghes_is_deferrable() logic would, under my argument, agree to pass execution to the actual handlers.

For some classes of error we can't safely get there.

Quoted 2 lines

Though in that case, the problem is generalized to "how to best handle error Y", rather than "how to handle error Y in FFS".

(that's a good thing yes?) I assume x86 can take MCE errors out of IRQ-masked code. Sharing the handle_foo_error_nmi() code between the two paths would be a good thing.

Quoted 2 lines

Sarcasm aside, I do like the idea of a message complaining about the firmware.

It wouldn't be appropriate in all cases, but for AER it looks like 'fatal' is always an over-reaction.
For example, given a memory error firmware can't know whether we can re-read some page of kernel memory from disk and then reschedule the thread.

Quoted 4 lines

If we're going to split recovery paths, then it makes better sense to have a system where handleable errors are passed down to a lesser context. Or even run non-blocking handlers in whatever context they are received. Much more than having a special case for a specific error.

Yes. I think running the NMI-safe parts of the handler first, then the IRQ-parts when we drop to IRQ context is the way to do this.

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

I abstract things at irq_work_queue(): "queue this work and let me return from this interrupt"

('from this NMI', to come back later as an IRQ, from where we can call schedule_work_on(). This chaining is because the scheduler takes irqsave spin-locks to schedule the work.)

Quoted 3 lines

Hopefully, the handling is set up in such a way that I don't have to worry about these details on a per-error basis. If I do have to worry, them I must be doing something wrong.

I agree, but something has to know. Lets tackle AER first,

where we have none of theses concerns

Quoted 5 lines

Quoted 3 lines

On arm64 poisoned-cache locations triggering an external abort (leading to a NOTIFY_SEA NMI-like notification to APEI) in response to a load is synchronous, hence the live-lock problem.

How is this currently handled in the kernel-first case?

We don't have any kernel-first support today

it would need the Arm version of
MCA (v8.2 RAS Extensions) which are relatively new. Everything we've seen so far is using firmware-first as it has to know a little about the SoC topology.

How would it be handled? For the kernel-first version of NOTIFY_SEA the code would get the interrupted pt_regs and can probe the RAS Error registers (snip)

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

I am not concert about livelocking the system.

This is what keeps me awake at night.

Nothing keeps me awake at night. I keep others awake at night.

(Ha! I owe you a beer!)

Quoted 5 lines

Quoted 3 lines

I think your platform has NMI handled as MCE/kernel-first for synchronous CPU errors. SMI triggered by PCI-AER are notified by NMI too, but handled by firmware-first.

MCEs come in via FFS.

Oh? I thought those were to separate worlds. I assumed MCE-NMIs didn't go via SMM.

I think the way forward here is to spot defer-able errors, even if they are marked fatal. Initially this will just be AER, but if we can split up the handlers to have NMI/IRQ/Process context counterparts, we may be able to do enough work early-on to defer other types of error.

Quoted 5 lines · James Morse

This assumes a cache-invalidate will clear the error, which I don't think we're guaranteed on arm.
It also destroys any adjacent data, "everyone's happy" includes the thread that got a chunk of someone-else's stack frame, I don't think it will be happy for very long!

Hmm, no cache-line (or page) invalidation on arm64? How does dma_map/unmap_*() work then? You may not guarantee to fix the error, but

I don't buy into the "let's crash without trying" argument.

Quoted 1 line

(this is a side issue for AER though)

Somebody muddled up AER with these tables, so we now have to worry about it. :)

Quoted 8 lines

Quoted 3 lines

How does FFS handle race conditions that can occur when accessing HW concurrently with the OS? I'm told it's the main reasons why BIOS doesn't release unused cores from SMM early.

This is firmware's problem, it depends on whether there is any hardware that is shared with the OS. Some hardware can be marked 'secure' in which case only firmware can access it, alternatively firmware can trap or just disable the OS's access to the shared hardware.

It's everyone's problem. It's the firmware's responsibility.

Quoted 5 lines

For example, with the v8.2 RAS Extensions, there are some per-cpu error registers. Firmware can disable these for the OS, so that it always reads 0 from them. Instead firmware takes the error via FF, reads the registers from firmware, and dumps CPER records into the OS's memory.

Quoted 3 lines

If there is a shared hardware resource that both the OS and firmware may be accessing, yes firmware needs to pull the other CPUs in, but this depends on the SoC design, it doesn't necessarily happen.

The problem with shared resources is just a problem. I've seen systems where all 100 cores are held up for 300+ ms. In latency-critical applications reliability drops exponentially. Am I correct in assuming your answer would be to "hide" more stuff from the OS?

Quoted 6 lines

Quoted 2 lines

I think the idea of firmware-first is broken. But it's there, it's shipping in FW, so we have to accommodate it in SW.

Part of our different-views here is firmware-first is taking something away from you, whereas for me its giving me information that would otherwise be in secret-soc-specific registers.

Under this interpretation, FFS is a band-aid to the problem of "secret" registers. "Secret" hardware doesn't really fit well into the idea of an OS [1].

Quoted 5 lines

Quoted 3 lines

And linux can handle a wide subset of MCEs just fine, so the ghes_is_deferrable() logic would, under my argument, agree to pass execution to the actual handlers.

For some classes of error we can't safely get there.

Optimize for the common case.

Hi Alex,

Quoted 2 lines · Alex G.

Quoted 1 line · James Morse

This assumes a cache-invalidate will clear the error, which I don't

think we're

Quoted 2 lines

Quoted 2 lines

guaranteed on arm.
It also destroys any adjacent data, "everyone's happy" includes the

thread that

Quoted 1 line

Quoted 1 line

got a chunk of someone-else's stack frame, I don't think it will be

happy for

Quoted 4 lines

Quoted 1 line

very long!

Hmm, no cache-line (or page) invalidation on arm64? How does dma_map/unmap_*() work then? You may not guarantee to fix the error, but

There are cache-invalidate instructions, but I don't think 'solving' a RAS error with them is the right thing to do.

Quoted 1 line

I don't buy into the "let's crash without trying" argument.

Our 'cache writeback granule' may be as large as 2K, so we may have to invalidate up to 2K of data to convince the hardware this address is okay again.

All we've done here is differently-corrupt the data so that it no longer generates a RAS fault, it just gives you the wrong data instead. Cache-invalidation is destructive.

I don't think there is a one-size-fits-all solution here.

Quoted 4 lines

Quoted 1 line

(this is a side issue for AER though)

Somebody muddled up AER with these tables, so we now have to worry about it. :)

Eh? I see there is a v2, maybe I'll understand this comment once I read it.

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

How does FFS handle race conditions that can occur when accessing HW concurrently with the OS? I'm told it's the main reasons why BIOS doesn't release unused cores from SMM early.

This is firmware's problem, it depends on whether there is any

hardware that is

Quoted 1 line

Quoted 1 line

shared with the OS. Some hardware can be marked 'secure' in which

case only

Quoted 1 line

Quoted 1 line

firmware can access it, alternatively firmware can trap or just

disable the OS's

Quoted 3 lines

Quoted 1 line

access to the shared hardware.

It's everyone's problem. It's the firmware's responsibility.

It depends on the SoC design. If there is no hardware that the OS and firmware both need to access to handle an error then I don't think firmware needs to do this.

Quoted 2 lines

Quoted 2 lines

For example, with the v8.2 RAS Extensions, there are some per-cpu error registers. Firmware can disable these for the OS, so that it always

reads 0 from

Quoted 4 lines

Quoted 4 lines

them. Instead firmware takes the error via FF, reads the registers from firmware, and dumps CPER records into the OS's memory.

If there is a shared hardware resource that both the OS and firmware

may be

Quoted 1 line

Quoted 1 line

accessing, yes firmware needs to pull the other CPUs in, but this

depends on the

Quoted 6 lines

Quoted 1 line

SoC design, it doesn't necessarily happen.

The problem with shared resources is just a problem. I've seen systems where all 100 cores are held up for 300+ ms. In latency-critical applications reliability drops exponentially. Am I correct in assuming your answer would be to "hide" more stuff from the OS?

No, I'm not a fan of firmware cycle stealing. If you can design the SoC or firmware so that the 'all CPUs' stuff doesn't need to happen, then you won't get
these issues. (I don't design these things, I'm sure they're much more complicated
than I think!)

Because the firmware is SoC-specific, so it only needs to do exactly what is necessary.

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

I think the idea of firmware-first is broken. But it's there, it's shipping in FW, so we have to accommodate it in SW.

Part of our different-views here is firmware-first is taking

something away from

Quoted 3 lines

Quoted 2 lines

you, whereas for me its giving me information that would otherwise be in secret-soc-specific registers.

Quoted 3 lines

Under this interpretation, FFS is a band-aid to the problem of "secret" registers. "Secret" hardware doesn't really fit well into the idea of an OS [1].

Sorry, I'm being sloppy with my terminology, by secret-soc-specific I mean either Linux can't access them (firmware privilege-level only) or Linux can't reasonably know where these registers are, as they're soc-specific and vary by manufacture.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

And linux can handle a wide subset of MCEs just fine, so the ghes_is_deferrable() logic would, under my argument, agree to pass execution to the actual handlers.

For some classes of error we can't safely get there.

Optimize for the common case.

At the expense of reliability?