acpi: apei: Improve error handling with firmware-first
Subsystems (5)
v2 cover acpi: apei: Improve error handling with firmware-first
Or "acpi: apei: Don't let puny firmware crash us with puny errors"
This is the improved implementation following feedback from James Morse (thanks James!). This implementation, I think, is more modular, and easier to follow, and just makes more sense.
I'm leaving this as RFC because the BIOS team is a bit scared of an OS that won't crash when it's told to. However, if people like the idea, then I have nothing against merging this.
Changes since v1:
- Due to popular request, the panic() is left in the NMI handler
- GHES AER handler is split into NMI and non-NMI portions
- ghes_notify_nmi() does not panic on deferrable errors
- The handlers are put in a mapping and given a common call signature
Alexandru Gagniuc (4):
EDAC, GHES: Remove unused argument to ghes_edac_report_mem_error
acpi: apei: Split GHES handlers outside of ghes_do_proc
acpi: apei: Do not panic() when correctable errors are marked as
fatal.
acpi: apei: Warn when GHES marks correctable errors as "fatal"
drivers/acpi/apei/ghes.c | 132 ++++++++++++++++++++++++++++++++++++++++------- drivers/edac/ghes_edac.c | 3 +- include/acpi/ghes.h | 5 +- 3 files changed, 117 insertions(+), 23 deletions(-)
Signature · 1 line
2.14.3
RFC PATCH v2 1/4 EDAC, GHES: Remove unused argument to ghes_edac_report_mem_error
--- drivers/acpi/apei/ghes.c | 2 +- drivers/edac/ghes_edac.c | 3 +-- include/acpi/ghes.h | 5 ++--- 3 files changed, 4 insertions(+), 6 deletions(-)
drivers/acpi/apei/ghes.c +1 -1
@@ -481,7 +481,7 @@ static void ghes_do_proc(struct ghes *ghes,if (guid_equal (sec_type, &CPER_SEC_PLATFORM_MEM )) {struct cper_sec_mem_err *mem_err =acpi_hest_get_payload (gdata); -ghes_edac_report_mem_error (ghes, sev, mem_err); +ghes_edac_report_mem_error (sev, mem_err);arch_apei_report_mem_error (sev, mem_err);ghes_handle_memory_failure (gdata, sev);
drivers/edac/ghes_edac.c +1 -2
@@ -172,8 +172,7 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg) } } -void ghes_edac_report_mem_error (struct ghes *ghes,int sev, -struct cper_sec_mem_err *mem_err) +void ghes_edac_report_mem_error (int sev,struct cper_sec_mem_err *mem_err) {enum hw_event_mc_err_type type;struct edac_raw_error_desc *e;
include/acpi/ghes.h +2 -3
@@ -55,15 +55,14 @@ enum {/* From drivers/edac/ghes_edac.c */ #ifdef CONFIG_EDAC_GHES -void ghes_edac_report_mem_error (struct ghes *ghes,int sev, -struct cper_sec_mem_err *mem_err); +void ghes_edac_report_mem_error (int sev,struct cper_sec_mem_err *mem_err);int ghes_edac_register (struct ghes *ghes,struct device *dev);void ghes_edac_unregister (struct ghes *ghes);#else -static inline void ghes_edac_report_mem_error (struct ghes *ghes,int sev, +static inline void ghes_edac_report_mem_error (int sev,struct cper_sec_mem_err *mem_err) { }
Signature · 1 line
2.14.3
Borislav Petkov
maintainerRe: [RFC PATCH v2 1/4] EDAC, GHES: Remove unused argument to ghes_edac_report_mem_error
On Mon, Apr 16, 2018 at 04:59:00PM -0500, Alexandru Gagniuc wrote:
<--- Insert commit message here.
A possible candidate would be some blurb about what commit removed the use of that first arg.
Quoted 6 lines
Signed-off-by Alexandru Gagniuc <[email protected]>--- drivers/acpi/apei/ghes.c | 2 +- drivers/edac/ghes_edac.c | 3 +-- include/acpi/ghes.h | 5 ++--- 3 files changed, 4 insertions(+), 6 deletions(-)
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [RFC PATCH v2 1/4] EDAC, GHES: Remove unused argument to ghes_edac_report_mem_error
Quoted 6 lines · Borislav Petkov
On Mon, Apr 16, 2018 at 04:59:00PM -0500, Alexandru Gagniuc wrote:
<--- Insert commit message here.
A possible candidate would be some blurb about what commit removed the use of that first arg.
I didn't consider any commit message pork to be necessary when the summary already explains the triviality of the change. I'll add it in the next rev.
Thanks,
Alex
Quoted 7 lines
Quoted 6 lines
Signed-off-by Alexandru Gagniuc <[email protected]>--- drivers/acpi/apei/ghes.c | 2 +- drivers/edac/ghes_edac.c | 3 +-- include/acpi/ghes.h | 5 ++--- 3 files changed, 4 insertions(+), 6 deletions(-)
RFC PATCH v2 2/4 acpi: apei: Split GHES handlers outside of ghes_do_proc
In reply to Alexandru GagniucUse a mapping from CPER UUID to get the correct handler for a given GHES error. This is in preparation of splitting some handlers into irq safe and regular parts.
--- drivers/acpi/apei/ghes.c | 78 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 63 insertions(+), 15 deletions(-)
drivers/acpi/apei/ghes.c +63 -15
@@ -414,6 +414,25 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int#endif } +static int ghes_handle_arm (struct acpi_hest_generic_data *gdata,int sev) +{ +struct cper_sec_proc_arm *err =acpi_hest_get_payload (gdata); + +log_arm_hw_error (err); +return ghes_severity (gdata->error_severity); +} + +static int ghes_handle_mem (struct acpi_hest_generic_data *gdata,int sev) +{ +struct cper_sec_mem_err *mem_err =acpi_hest_get_payload (gdata); + +ghes_edac_report_mem_error (sev, mem_err); +arch_apei_report_mem_error (sev, mem_err); +ghes_handle_memory_failure (gdata, sev); + +return ghes_severity (gdata->error_severity); +} +/* * PCIe AER errors need to be sent to the AER driver for reporting and * recovery. The GHES severities map to the following AER severities and
@@ -428,7 +447,7 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int* GHES_SEV_PANIC does not make it to this handling since the kernel must * panic. */ -static void ghes_handle_aer (struct acpi_hest_generic_data *gdata) +static int ghes_handle_aer (struct acpi_hest_generic_data *gdata,int sev) {#ifdef CONFIG_ACPI_APEI_PCIEAER struct cper_sec_pcie *pcie_err =acpi_hest_get_payload (gdata);
@@ -456,14 +475,54 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata) (struct aer_capability_regs *) pcie_err->aer_info); } + +return GHES_SEV_CORRECTED ;#endif +return ghes_severity (gdata->error_severity); } +/** +* ghes_handler - handler for ACPI APEI errors +* @error_uuid: UUID describing the error entry (See ACPI/EFI CPER for details) +* @handle: Handler for the GHES entry of type 'error_uuid'. The handler +* returns the severity of the error after handling. A handler is allowed +* to demote errors to correctable or corrected, as appropriate. +*/ +static const struct ghes_handler { +const guid_t *error_uuid; +int (*handle_irqsafe)(struct acpi_hest_generic_data *gdata,int sev); +int (*handle)(struct acpi_hest_generic_data *gdata,int sev); +} ghes_handlers[] = { + { + .error_uuid = &CPER_SEC_PLATFORM_MEM , + .handle = ghes_handle_mem, + }, { + .error_uuid = &CPER_SEC_PCIE , + .handle = ghes_handle_aer, + }, { + .error_uuid = &CPER_SEC_PROC_ARM , + .handle = ghes_handle_arm, + } +}; + +static const struct ghes_handler *get_handler (const guid_t *type) +{ +size_t i; + +for (i =0 ; i <ARRAY_SIZE (ghes_handlers); i++) { +if (guid_equal (type, ghes_handlers[i].error_uuid)) +return &ghes_handlers[i]; + } +return NULL ; +} + +static void ghes_do_proc (struct ghes *ghes,const struct acpi_hest_generic_status *estatus) {int sev, sec_sev;struct acpi_hest_generic_data *gdata; +const struct ghes_handler *handler;guid_t *sec_type;guid_t *fru_id = &NULL_UUID_LE ;char *fru_text ="" ;
@@ -478,21 +537,10 @@ static void ghes_do_proc(struct ghes *ghes,if (gdata->validation_bits &CPER_SEC_VALID_FRU_TEXT ) fru_text = gdata->fru_text; -if (guid_equal (sec_type, &CPER_SEC_PLATFORM_MEM )) { -struct cper_sec_mem_err *mem_err =acpi_hest_get_payload (gdata); - -ghes_edac_report_mem_error (sev, mem_err); - -arch_apei_report_mem_error (sev, mem_err); -ghes_handle_memory_failure (gdata, sev); - } -else if (guid_equal (sec_type, &CPER_SEC_PCIE )) { -ghes_handle_aer (gdata); - } -else if (guid_equal (sec_type, &CPER_SEC_PROC_ARM )) { -struct cper_sec_proc_arm *err =acpi_hest_get_payload (gdata); -log_arm_hw_error (err); + handler =get_handler (sec_type); +if (handler) { + sec_sev = handler->handle (gdata, sev); }else {void *err =acpi_hest_get_payload (gdata);
Signature · 1 line
2.14.3
Borislav Petkov
maintainerRe: [RFC PATCH v2 2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc
Quoted 19 lines · Alexandru Gagniuc
static void ghes_do_proc(struct ghes *ghes,
drivers/acpi/apei/ghes.c +1
-6const struct acpi_hest_generic_status *estatus) {int sev, sec_sev;struct acpi_hest_generic_data *gdata; +const struct ghes_handler *handler;guid_t *sec_type;guid_t *fru_id = &NULL_UUID_LE ;char *fru_text ="" ;@@ -478,21 +537,10 @@ static void ghes_do_proc(struct ghes *ghes,if (gdata->validation_bits &CPER_SEC_VALID_FRU_TEXT ) fru_text = gdata->fru_text; -if (guid_equal (sec_type, &CPER_SEC_PLATFORM_MEM )) { -struct cper_sec_mem_err *mem_err =acpi_hest_get_payload (gdata); - -ghes_edac_report_mem_error (sev, mem_err); - -arch_apei_report_mem_error (sev, mem_err);
drivers/acpi/apei/ghes.c static void ghes_do_proc(struct ghes *ghes, · line 541
Quoted 10 lines
drivers/acpi/apei/ghes.c +1
-8-ghes_handle_memory_failure (gdata, sev); - } -else if (guid_equal (sec_type, &CPER_SEC_PCIE )) { -ghes_handle_aer (gdata); - } -else if (guid_equal (sec_type, &CPER_SEC_PROC_ARM )) { -struct cper_sec_proc_arm *err =acpi_hest_get_payload (gdata); -log_arm_hw_error (err); + handler =get_handler (sec_type);
I don't like this - it was better and more readable before because I can follow which handler gets called. This change makes is less readable.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [RFC PATCH v2 2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc
Quoted 28 lines · Borislav Petkov
Quoted 27 lines · Alexandru Gagniuc
static void ghes_do_proc(struct ghes *ghes,
drivers/acpi/apei/ghes.c +1
-13const struct acpi_hest_generic_status *estatus) {int sev, sec_sev;struct acpi_hest_generic_data *gdata; +const struct ghes_handler *handler;guid_t *sec_type;guid_t *fru_id = &NULL_UUID_LE ;char *fru_text ="" ;@@ -478,21 +537,10 @@ static void ghes_do_proc(struct ghes *ghes,if (gdata->validation_bits &CPER_SEC_VALID_FRU_TEXT ) fru_text = gdata->fru_text; -if (guid_equal (sec_type, &CPER_SEC_PLATFORM_MEM )) { -struct cper_sec_mem_err *mem_err =acpi_hest_get_payload (gdata); - -ghes_edac_report_mem_error (sev, mem_err); - -arch_apei_report_mem_error (sev, mem_err); -ghes_handle_memory_failure (gdata, sev); - } -else if (guid_equal (sec_type, &CPER_SEC_PCIE )) { -ghes_handle_aer (gdata); - } -else if (guid_equal (sec_type, &CPER_SEC_PROC_ARM )) { -struct cper_sec_proc_arm *err =acpi_hest_get_payload (gdata);
Quoted 5 lines
drivers/acpi/apei/ghes.c static void ghes_do_proc(struct ghes *ghes, · line 541
Quoted 2 lines
drivers/acpi/apei/ghes.c +1
-1-log_arm_hw_error (err); + handler =get_handler (sec_type);I don't like this - it was better and more readable before because I can follow which handler gets called. This change makes is less readable.
I agree with the readability argument in the current situation of three handlers. I guess I was thinking ahead and generalizing for an arbitrary number of handlers.
On the other side, you lose readability as soon as you get a few more handlers and the function becomes too long. And more importantly, you lose generality: it's not obvious that there's ghes_edac_report_mem_error() which too wide a context.
Alex
Borislav Petkov
maintainerRe: [RFC PATCH v2 2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc
Quoted 2 lines · Alex G.
On the other side, you lose readability as soon as you get a few more handlers and the function becomes too long.
No you don't - you split it properly.
Quoted 2 lines
And more importantly, you lose generality: it's not obvious that there's
ghes_edac_report_mem_error()which too wide a context.
I don't understand what that means.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [RFC PATCH v2 2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc
Quoted 5 lines · Borislav Petkov
Quoted 2 lines · Alex G.
On the other side, you lose readability as soon as you get a few more handlers and the function becomes too long.
No you don't - you split it properly.
And that was the motivation behind my splitting it in this patch.
Quoted 4 lines
Quoted 2 lines
And more importantly, you lose generality: it's not obvious that there's
ghes_edac_report_mem_error()which too wide a context.I don't understand what that means.
My apologies, sometimes my thought is too far ahead of my typing fingers. For the purpose of handling _one_ error, you need the CPER entry for that one error -- narrow context. You don't need the entire GHES structure -- wide context. Individual handlers should not be able to access the entire ghes.
When the handlers are restricted to a common signature --which doesn't include ghes--, it's obvious when functions try to bite more than they can chew.
Alex
Borislav Petkov
maintainerRe: [RFC PATCH v2 2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc
Quoted 1 line · Alex G.
And that was the motivation behind my splitting it in this patch.
By "split" I don't mean add a function pointer which gets selected and then called - if the function becomes too long, you simply split the function body properly.
Quoted 2 lines
You don't need the entire GHES structure -- wide context. Individual handlers should not be able to access the entire ghes.
But you remove the @ghes argument in patch 1. So what are we even talking about?
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [RFC PATCH v2 2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc
Quoted 6 lines · Borislav Petkov
Quoted 1 line · Alex G.
And that was the motivation behind my splitting it in this patch.
By "split" I don't mean add a function pointer which gets selected and then called - if the function becomes too long, you simply split the function body properly.
The bulk of the function is the if/else mapping from UUID to error handler. I don't see how that can be easily split up, hence why I originally resorted to the mapping. As you said, we'll keep it simple at first.
Quoted 5 lines
Quoted 2 lines
You don't need the entire GHES structure -- wide context. Individual handlers should not be able to access the entire ghes.
But you remove the @ghes argument in patch 1. So what are we even talking about?
You could say, by convention, handlers shouldn't access ghes directly, but that is not obvious when @ghes is in scope. The reason I bring it up is that, if [1/4] ends up being unneeded, then I will drop it from the series.
Alex
Borislav Petkov
maintainerRe: [RFC PATCH v2 2/4] acpi: apei: Split GHES handlers outside of ghes_do_proc
Quoted 4 lines · Alex G.
The bulk of the function is the if/else mapping from UUID to error handler. I don't see how that can be easily split up, hence why I originally resorted to the mapping. As you said, we'll keep it simple at first.
So that function is 43 lines now. Why are we even talking about this?!
Just add your UUID check to the if-else statement and be done with it already. No handlers no nothing.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.RFC PATCH v2 3/4 acpi: apei: Do not panic() when correctable errors are marked as fatal.
In reply to Alexandru GagniucFirmware is evil:
- ACPI was created to "try and make the 'ACPI' extensions somehow Windows specific" in order to "work well with NT and not the others even if they are open"
- EFI was created to hide "secret" registers from the OS.
- UEFI was created to allow compromising an otherwise secure OS.
Never has firmware been created to solve a problem or simplify an otherwise cumbersome process. It is of no surprise then, that firmware nowadays intentionally crashes an OS.
One simple way to do that is to mark GHES errors as fatal. Firmware knows and even expects that an OS will crash in this case. And most OSes do.
PCIe errors are notorious for having different definitions of "fatal". In ACPI, and other firmware sandards, 'fatal' means the machine is about to explode and needs to be reset. In PCIe, on the other hand, fatal means that the link to a device has died. In the hotplug world of PCIe, this is akin to a USB disconnect. From that view, the "fatal" loss of a link is a normal event. To allow a machine to crash in this case is downright idiotic.
To solve this, implement an IRQ safe handler for AER. This makes sure we have enough information to invoke the full AER handler later down the road, and tells ghes_notify_nmi that "It's all cool". ghes_notify_nmi() then gets calmed down a little, and doesn't panic().
--- drivers/acpi/apei/ghes.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-)
drivers/acpi/apei/ghes.c +42 -2
@@ -481,12 +481,26 @@ static int ghes_handle_aer(struct acpi_hest_generic_data *gdata, int sev)return ghes_severity (gdata->error_severity); } +static int ghes_handle_aer_irqsafe (struct acpi_hest_generic_data *gdata, +int sev) +{ +struct cper_sec_pcie *pcie_err =acpi_hest_get_payload (gdata); + +/* The system can always recover from AER errors. */ +if (pcie_err->validation_bits &CPER_PCIE_VALID_DEVICE_ID && + pcie_err->validation_bits &CPER_PCIE_VALID_AER_INFO ) +return CPER_SEV_RECOVERABLE ; + +return ghes_severity (gdata->error_severity); +} +/** * ghes_handler - handler for ACPI APEI errors * @error_uuid: UUID describing the error entry (See ACPI/EFI CPER for details) * @handle: Handler for the GHES entry of type 'error_uuid'. The handler * returns the severity of the error after handling. A handler is allowed * to demote errors to correctable or corrected, as appropriate. +* @handle_irqsafe: (optional) Non-blocking handler for GHES entry. */ static const struct ghes_handler { const guid_t *error_uuid;
@@ -498,6 +512,7 @@ static const struct ghes_handler { .handle = ghes_handle_mem, }, { .error_uuid = &CPER_SEC_PCIE, + .handle_irqsafe = ghes_handle_aer_irqsafe, .handle = ghes_handle_aer, }, { .error_uuid = &CPER_SEC_PROC_ARM,
@@ -551,6 +566,30 @@ static void ghes_do_proc(struct ghes *ghes, } } +/* How severe is the error if handling is deferred outside IRQ/NMI context? */ +static int ghes_deferrable_severity (struct ghes *ghes) +{ +int deferrable_sev, sev, sec_sev; +struct acpi_hest_generic_data *gdata; +const struct ghes_handler *handler; +const guid_t *section_type; +const struct acpi_hest_generic_status *estatus = ghes->estatus; + + deferrable_sev =GHES_SEV_NO ; + sev =ghes_severity (estatus->error_severity); +apei_estatus_for_each_section (estatus, gdata) { + section_type = (guid_t *)gdata->section_type; + handler =get_handler (section_type); +if (handler && handler->handle_irqsafe) + sec_sev = handler->handle_irqsafe (gdata, sev); +else + sec_sev =ghes_severity (gdata->error_severity); + deferrable_sev =max (deferrable_sev, sec_sev); + } + +return deferrable_sev; +} +static void __ghes_print_estatus (const char *pfx,const struct acpi_hest_generic *generic,const struct acpi_hest_generic_status *estatus)
@@ -980,7 +1019,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 sev, dsev, ret =NMI_DONE ;if (!atomic_add_unless (&ghes_in_nmi,1 ,1 ))return ret;
@@ -993,8 +1032,9 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) ret =NMI_HANDLED ; } + dsev =ghes_deferrable_severity (ghes); sev =ghes_severity (ghes->estatus->error_severity); -if (sev >=GHES_SEV_PANIC ) { +if ((sev >=GHES_SEV_PANIC ) && (dsev >=GHES_SEV_PANIC )) {oops_begin ();ghes_print_queued_estatus ();__ghes_panic (ghes);
Signature · 1 line
2.14.3
Borislav Petkov
maintainerRe: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
Quoted 10 lines · Alexandru Gagniuc
Firmware is evil:
- ACPI was created to "try and make the 'ACPI' extensions somehow Windows specific" in order to "work well with NT and not the others even if they are open"
- EFI was created to hide "secret" registers from the OS.
- UEFI was created to allow compromising an otherwise secure OS.Never has firmware been created to solve a problem or simplify an otherwise cumbersome process. It is of no surprise then, that firmware nowadays intentionally crashes an OS.
I don't believe I'm saying this but, get rid of that rant. Even though I agree, it doesn't belong in a commit message.
Quoted 33 lines
One simple way to do that is to mark GHES errors as fatal. Firmware knows and even expects that an OS will crash in this case. And most OSes do.
PCIe errors are notorious for having different definitions of "fatal". In ACPI, and other firmware sandards, 'fatal' means the machine is about to explode and needs to be reset. In PCIe, on the other hand, fatal means that the link to a device has died. In the hotplug world of PCIe, this is akin to a USB disconnect. From that view, the "fatal" loss of a link is a normal event. To allow a machine to crash in this case is downright idiotic.
To solve this, implement an IRQ safe handler for AER. This makes sure we have enough information to invoke the full AER handler later down the road, and tells
ghes_notify_nmithat "It's all cool".ghes_notify_nmi()then gets calmed down a little, and doesn'tpanic().Signed-off-by Alexandru Gagniuc <[email protected]>--- drivers/acpi/apei/ghes.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-)drivers/acpi/apei/ghes.c +2
-0@@ -481,12 +481,26 @@ static int ghes_handle_aer(struct acpi_hest_generic_data *gdata, int sev)return ghes_severity (gdata->error_severity); } +static int ghes_handle_aer_irqsafe (struct acpi_hest_generic_data *gdata, +int sev)
drivers/acpi/apei/ghes.c static int ghes_handle_aer(struct acpi_hest_generic_data *gdata, int sev) · line 495
Quoted 10 lines
drivers/acpi/apei/ghes.c +10
-0+{ +struct cper_sec_pcie *pcie_err =acpi_hest_get_payload (gdata); + +/* The system can always recover from AER errors. */ +if (pcie_err->validation_bits &CPER_PCIE_VALID_DEVICE_ID && + pcie_err->validation_bits &CPER_PCIE_VALID_AER_INFO ) +return CPER_SEV_RECOVERABLE ; + +return ghes_severity (gdata->error_severity); +}
Well, Tyler touched that AER error severity handling recently and we had it all nicely documented in the comment above ghes_handle_aer().
Your ghes_handle_aer_irqsafe() graft basically bypasses ghes_handle_aer() instead of incorporating in it.
If all you wanna say is, the severity computation should go through all the sections and look at each error's severity before making a decision, then add that to ghes_severity() instead of doing that "deferrable" severity dance.
And add the changes to the policy to the comment above ghes_handle_aer(). I don't want any changes from people coming and going and leaving us scratching heads why we did it this way.
And no need for those handlers and so on - make it simple first - then we can talk more complex handling.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
Quoted 8 lines · Borislav Petkov
Quoted 7 lines · Alexandru Gagniuc
Firmware is evil:
- ACPI was created to "try and make the 'ACPI' extensions somehow Windows specific" in order to "work well with NT and not the others even if they are open"
- EFI was created to hide "secret" registers from the OS.
- UEFI was created to allow compromising an otherwise secure OS.
Quoted 6 lines
Quoted 3 lines
Never has firmware been created to solve a problem or simplify an otherwise cumbersome process. It is of no surprise then, that firmware nowadays intentionally crashes an OS.
I don't believe I'm saying this but, get rid of that rant. Even though I agree, it doesn't belong in a commit message.
Of course.
(snip)> Well, Tyler touched that AER error severity handling recently and we had
Quoted 9 lines
it all nicely documented in the comment above
ghes_handle_aer().Your
ghes_handle_aer_irqsafe()graft basically bypassesghes_handle_aer()instead of incorporating in it.If all you wanna say is, the severity computation should go through all the sections and look at each error's severity before making a decision, then add that to
ghes_severity()instead of doing that "deferrable" severity dance.
ghes_severity() is a one-to-one mapping from a set of unsorted severities to monotonically increasing numbers. The "one-to-one" mapping part of the sentence is obvious from the function name. To change it to parse the entire GHES would completely destroy this, and I think it would apply policy in the wrong place.
Should I do that, I might have to call it something like ghes_parse_and_apply_policy_to_severity(). But that misses the whole point if these changes.
I would like to get to the handlers first, and then decide if things are okay or not, but the ARM guys didn't exactly like this approach. It seems there are quite some per-error-type considerations. The logical step is to associate these considerations with the specific error type they apply to, rather than hide them as a decision under an innocent ghes_severity().
Quoted 6 lines
And add the changes to the policy to the comment above
ghes_handle_aer(). I don't want any changes from people coming and going and leaving us scratching heads why we did it this way.And no need for those handlers and so on - make it simple first - then we can talk more complex handling.
I don't want to leave people scratching their heads, but I also don't want to make AER a special case without having a generic way to handle these cases. People are just as susceptible to scratch their heads wondering why AER is a special case and everything else crashes.
Maybe it's better move the AER handling to NMI/IRQ context, since ghes_handle_aer() is only scheduling the real AER andler, and is irq safe. I'm scratching my head about why we're messing with IRQ work from NMI context, instead of just scheduling a regular handler to take care of things.
Alex
Re: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
Hi Alex,
(I haven't read through all this yet, just on this one:)
Quoted 5 lines · Alex G.
Maybe it's better move the AER handling to NMI/IRQ context, since
ghes_handle_aer()is only scheduling the real AER andler, and is irq safe. I'm scratching my head about why we're messing with IRQ work from NMI context, instead of just scheduling a regular handler to take care of things.
We can't touch schedule_work_on() from NMI context as it takes spinlocks and disables interrupts. (see __queue_work()) The NMI may have interrupted IRQ-context code
that was already holding the same locks.
IRQ-work behaves differently, it uses an llist for the work and an arch code hook
to raise a self-IPI.
Thanks,
James
Re: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
Quoted 17 lines · James Morse
Hi Alex,
(I haven't read through all this yet, just on this one:)
Quoted 5 lines · Alex G.
Maybe it's better move the AER handling to NMI/IRQ context, since
ghes_handle_aer()is only scheduling the real AER andler, and is irq safe. I'm scratching my head about why we're messing with IRQ work from NMI context, instead of just scheduling a regular handler to take care of things.We can't touch
schedule_work_on()from NMI context as it takes spinlocks and
disables interrupts. (see__queue_work()) The NMI may have interrupted IRQ-context code
that was already holding the same locks.
Quoted 3 lines
IRQ-work behaves differently, it uses an llist for the work and an arch code hook
to raise a self-IPI.
That makes sense. Thank you!
Alex
Quoted 4 lines
Thanks,
James
Borislav Petkov
maintainerRe: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
In reply to Alex G.Quoted 5 lines · Alex G.
ghes_severity()is a one-to-one mapping from a set of unsorted severities to monotonically increasing numbers. The "one-to-one" mapping part of the sentence is obvious from the function name. To change it to parse the entire GHES would completely destroy this, and I think it would apply policy in the wrong place.
So do a wrapper or whatever. Do a ghes_compute_severity() or however you would wanna call it and do the iteration there.
Quoted 3 lines
Should I do that, I might have to call it something like
ghes_parse_and_apply_policy_to_severity(). But that misses the whole point if these changes.
What policy? You simply compute the severity like we do in the mce code.
Quoted 2 lines
I would like to get to the handlers first, and then decide if things are okay or not,
Why? Give me an example why you'd handle an error first and then decide whether we're ok or not?
Usually, the error handler decides that in one place. So what exactly are you trying to do differently that doesn't fit that flow?
Quoted 4 lines
I don't want to leave people scratching their heads, but I also don't want to make AER a special case without having a generic way to handle these cases. People are just as susceptible to scratch their heads wondering why AER is a special case and everything else crashes.
Not if it is properly done *and* documented why we applying the respective policy for the error type.
Quoted 5 lines
Maybe it's better move the AER handling to NMI/IRQ context, since
ghes_handle_aer()is only scheduling the real AER andler, and is irq safe. I'm scratching my head about why we're messing with IRQ work from NMI context, instead of just scheduling a regular handler to take care of things.
No, first pls explain what exactly you're trying to do and then we can talk about how to do it. Btw, a real-life example to accompany that intention goes a long way.
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
Quoted 9 lines · Borislav Petkov
Quoted 5 lines · Alex G.
ghes_severity()is a one-to-one mapping from a set of unsorted severities to monotonically increasing numbers. The "one-to-one" mapping part of the sentence is obvious from the function name. To change it to parse the entire GHES would completely destroy this, and I think it would apply policy in the wrong place.So do a wrapper or whatever. Do a
ghes_compute_severity()or however you would wanna call it and do the iteration there.
That doesn't sound right. There isn't a formula to compute. What we're doing is we're looking at individual error sources, and deciding what errors we can handle based both on the error, and our ability to handle the error.
Quoted 5 lines
Quoted 3 lines
Should I do that, I might have to call it something like
ghes_parse_and_apply_policy_to_severity(). But that misses the whole point if these changes.What policy? You simply compute the severity like we do in the mce code.
As explained above, our ability to resolve an error depends on the interaction between the error and error handler. This is very closely tied to the capabilities of each individual handler. I'll do it your way, but I don't think ignoring this tight coupling is the right thing to do.
Quoted 9 lines
Quoted 3 lines
I would like to get to the handlers first, and then decide if things are okay or not,
Why? Give me an example why you'd handle an error first and then decide whether we're ok or not?
Usually, the error handler decides that in one place. So what exactly are you trying to do differently that doesn't fit that flow?
In the NMI case you don't make it to the error handler. James and I beat this subject to the afterlife in v1.
Quoted 8 lines
Quoted 4 lines
I don't want to leave people scratching their heads, but I also don't want to make AER a special case without having a generic way to handle these cases. People are just as susceptible to scratch their heads wondering why AER is a special case and everything else crashes.
Not if it is properly done *and* documented why we applying the respective policy for the error type.
Quoted 7 lines
Quoted 5 lines
Maybe it's better move the AER handling to NMI/IRQ context, since
ghes_handle_aer()is only scheduling the real AER andler, and is irq safe. I'm scratching my head about why we're messing with IRQ work from NMI context, instead of just scheduling a regular handler to take care of things.No, first pls explain what exactly you're trying to do
I realize v1 was quite a while back, so I'll take this opportunity to restate:
At a very high level, I'm working with Dell on improving server reliability, with a focus on NVME hotplug and surprise removal. One of the features we don't support is surprise removal of NVME drives; hotplug is supported with 'prepare to remove'. This is one of the reasons NVME is not on feature parity with SAS and SATA.
My role is to solve this issue on linux, and to not worry about other OSes. This puts me in a position to have a linux-centric view of the problem, as opposed to the more common firmware-centric view.
Part of solving the surprise removal issue involves improving FFS error handling. This is required because the servers which are shipping use FFS instead of native error notifications. As part of extensive testing, I have found the NMI handler to be the most common cause of crashes, and hence this series.
Quoted 1 line
and then we can talk about how to do it.
Your move.
Quoted 1 line
Btw, a real-life example to accompany that intention goes a long way.
I'm not sure if this is the example you're looking for, but take an r740xd server, and slowly unplug an Intel NVME drives at an angle. You're likely to crash the machine.
Alex
Borislav Petkov
maintainerRe: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
Quoted 5 lines · Alex G.
At a very high level, I'm working with Dell on improving server reliability, with a focus on NVME hotplug and surprise removal. One of the features we don't support is surprise removal of NVME drives; hotplug is supported with 'prepare to remove'. This is one of the reasons NVME is not on feature parity with SAS and SATA.
Ok, first question: is surprise removal something purely mechanical or do you need firmware support for it? In the sense that you need to tell the firmware that you will be removing the drive.
I'm sceptical, though, as it has "surprise" in the name so I'm guessing the firmware doesn't know about it, the drive physically disappears and the FW starts spewing PCIe errors...
Quoted 3 lines
I'm not sure if this is the example you're looking for, but take an r740xd server, and slowly unplug an Intel NVME drives at an angle. You're likely to crash the machine.
No no, that's actually a great example!
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
SURPRISE!!!
Quoted 10 lines · Borislav Petkov
Quoted 5 lines · Alex G.
At a very high level, I'm working with Dell on improving server reliability, with a focus on NVME hotplug and surprise removal. One of the features we don't support is surprise removal of NVME drives; hotplug is supported with 'prepare to remove'. This is one of the reasons NVME is not on feature parity with SAS and SATA.
Ok, first question: is surprise removal something purely mechanical or do you need firmware support for it? In the sense that you need to tell the firmware that you will be removing the drive.
SURPRISE!!! removal only means that the system was not expecting the drive to be yanked. An example is removing a USB flash drive without first unmounting it and removing the usb device (echo 0 > /sys/bus/usb/.../authorized).
PCIe removal and hotplug is fairly well spec'd, and NVMe rides on that without issue. It's much easier and faster for an OS to just follow the spec and handle things on its own.
Interference from firmware only comes in with EFI/ACPI and FFS. From a purely technical point of view, firmware has nothing to do with this.
Quoted 1 line
From a firmware-centric view, unfortunately, firmware wants the ability
to log errors to the BMC... and hotplug events.
Does firmware need to know that a drive will be removed? I'm not aware of any such requirement. I think the main purpose of 'prepare to remove' is to shut down any traffic on the link. This way, link removal does not generate PCIe errors which may otherwise end up crashing the OS.
Quoted 3 lines
I'm sceptical, though, as it has "surprise" in the name so I'm guessing the firmware doesn't know about it, the drive physically disappears and the FW starts spewing PCIe errors...
It's not the FW that spews out errors. It's the hardware. It's very likely that a device which is actively used will have several DMA transactions already queued up and lots of traffic going through the link. When the link dies and the traffic can't be delivered, Unsupported Request errors are very common.
On the r740xd, FW just hides those errors from the OS with no further notification. On this machine BIOS sets things up such that non-posted requests report fatal (PCIe) errors. FW still tries very hard to hide this from the OS, and I think the heuristic is that if the drive physical presence is gone, don't even report the error.
There are a lot of problems with the approach, but one thing to keep in mind is that the FW was written at a time when OSes were more than happy to crash at any PCIe error reported through GHES.
Alex
Quoted 8 lines
Quoted 3 lines
I'm not sure if this is the example you're looking for, but take an r740xd server, and slowly unplug an Intel NVME drives at an angle. You're likely to crash the machine.
No no, that's actually a great example!
Thx.
Borislav Petkov
maintainerRe: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
(snip useful explanation).
Quoted 5 lines · Alex G.
On the r740xd, FW just hides those errors from the OS with no further notification. On this machine BIOS sets things up such that non-posted requests report fatal (PCIe) errors. FW still tries very hard to hide this from the OS, and I think the heuristic is that if the drive physical presence is gone, don't even report the error.
Ok, second question: can you detect from the error signatures alone that it was a surprise removal? How does such an error look like, in detail? Got error logs somewhere to dump?
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
Quoted 11 lines · Borislav Petkov
(snip useful explanation).
Quoted 5 lines · Alex G.
On the r740xd, FW just hides those errors from the OS with no further notification. On this machine BIOS sets things up such that non-posted requests report fatal (PCIe) errors. FW still tries very hard to hide this from the OS, and I think the heuristic is that if the drive physical presence is gone, don't even report the error.
Ok, second question: can you detect from the error signatures alone that it was a surprise removal?
I suppose you could make some inference, given the timing of other events going on around the the crash. It's not uncommon to see a "Card not present" event around drive removal.
Since the presence detect pin breaks last, you might not get that interrupt for a long while. In that case it's much harder to determine if you're seeing a SURPRISE!!! removal or some other fault.
I don't think you can use GHES alone to determine the nature of the event. There is not a 1:1 mapping from the set of things going wrong to the set of PCIe errors.
Quoted 1 line
How does such an error look like, in detail?
It's green on the soft side, with lots of red accents, as well as some textured white shades:
[ 51.414616] pciehp 0000:b0:06.0:pcie204: Slot(176): Link Down
[ 51.414634] pciehp 0000:b0:05.0:pcie204: Slot(179): Link Down
[ 52.703343] FIRMWARE BUG: Firmware sent fatal error that we were able
to correct
[ 52.703345] BROKEN FIRMWARE: Complain to your hardware vendor
[ 52.703347] {1}[Hardware Error]: Hardware error from APEI Generic
Hardware Error Source: 1
[ 52.703358] pciehp 0000:b0:06.0:pcie204: Slot(176): Link Up
[ 52.711616] {1}[Hardware Error]: event severity: fatal
[ 52.716754] {1}[Hardware Error]: Error 0, type: fatal
[ 52.721891] {1}[Hardware Error]: section_type: PCIe error
[ 52.727463] {1}[Hardware Error]: port_type: 6, downstream switch port
[ 52.734075] {1}[Hardware Error]: version: 3.0
[ 52.738607] {1}[Hardware Error]: command: 0x0407, status: 0x0010
[ 52.744786] {1}[Hardware Error]: device_id: 0000:b0:06.0
[ 52.750271] {1}[Hardware Error]: slot: 4
[ 52.754371] {1}[Hardware Error]: secondary_bus: 0xb3
[ 52.759509] {1}[Hardware Error]: vendor_id: 0x10b5, device_id: 0x9733
[ 52.766123] {1}[Hardware Error]: class_code: 000406
[ 52.771182] {1}[Hardware Error]: bridge: secondary_status: 0x0000,
control: 0x0003
[ 52.779038] pcieport 0000:b0:06.0: aer_status: 0x00100000, aer_mask:
0x01a10000
[ 52.782303] nvme0n1: detected capacity change from 3200631791616 to 0
[ 52.786348] pcieport 0000:b0:06.0: [20] Unsupported Request
[ 52.786349] pcieport 0000:b0:06.0: aer_layer=Transaction Layer,
aer_agent=Requester ID
[ 52.786350] pcieport 0000:b0:06.0: aer_uncor_severity: 0x004eb030
[ 52.786352] pcieport 0000:b0:06.0: TLP Header: 40000001 0000020f
e12023bc 01000000
[ 52.786357] pcieport 0000:b0:06.0: broadcast error_detected message
[ 52.883895] pci 0000:b3:00.0: device has no driver
[ 52.883976] pciehp 0000:b0:06.0:pcie204: Slot(176): Link Down
[ 52.884184] pciehp 0000:b0:06.0:pcie204: Slot(176): Link Down event
queued; currently getting powered on
[ 52.967175] pciehp 0000:b0:06.0:pcie204: Slot(176): Link UpQuoted 1 line
Got error logs somewhere to dump?
Sure [1]. They have the ANSI sequences, so you might want to wget and grep them in a color terminal.
Alex
[1] http://gtech.myftp.org/~mrnuke/nvme_logs/log-20180416-1919.log
Borislav Petkov
maintainerRe: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
Quoted 33 lines · Alex G.
Quoted 1 line
How does such an error look like, in detail?
It's green on the soft side, with lots of red accents, as well as some textured white shades:
[ 51.414616] pciehp 0000:b0:06.0:pcie204: Slot(176): Link Down [ 51.414634] pciehp 0000:b0:05.0:pcie204: Slot(179): Link Down [ 52.703343] FIRMWARE BUG: Firmware sent fatal error that we were able to correct [ 52.703345] BROKEN FIRMWARE: Complain to your hardware vendor [ 52.703347] {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 [ 52.703358] pciehp 0000:b0:06.0:pcie204: Slot(176): Link Up [ 52.711616] {1}[Hardware Error]: event severity: fatal [ 52.716754] {1}[Hardware Error]: Error 0, type: fatal [ 52.721891] {1}[Hardware Error]: section_type: PCIe error [ 52.727463] {1}[Hardware Error]: port_type: 6, downstream switch port [ 52.734075] {1}[Hardware Error]: version: 3.0 [ 52.738607] {1}[Hardware Error]: command: 0x0407, status: 0x0010 [ 52.744786] {1}[Hardware Error]: device_id: 0000:b0:06.0 [ 52.750271] {1}[Hardware Error]: slot: 4 [ 52.754371] {1}[Hardware Error]: secondary_bus: 0xb3 [ 52.759509] {1}[Hardware Error]: vendor_id: 0x10b5, device_id: 0x9733 [ 52.766123] {1}[Hardware Error]: class_code: 000406 [ 52.771182] {1}[Hardware Error]: bridge: secondary_status: 0x0000, control: 0x0003 [ 52.779038] pcieport 0000:b0:06.0: aer_status: 0x00100000, aer_mask: 0x01a10000 [ 52.782303] nvme0n1: detected capacity change from 3200631791616 to 0 [ 52.786348] pcieport 0000:b0:06.0: [20] Unsupported Request [ 52.786349] pcieport 0000:b0:06.0: aer_layer=Transaction Layer, aer_agent=Requester ID [ 52.786350] pcieport 0000:b0:06.0: aer_uncor_severity: 0x004eb030
Quoted 8 lines
[ 52.786352] pcieport 0000:b0:06.0: TLP Header: 40000001 0000020f e12023bc 01000000 [ 52.786357] pcieport 0000:b0:06.0: broadcast error_detected message [ 52.883895] pci 0000:b3:00.0: device has no driver [ 52.883976] pciehp 0000:b0:06.0:pcie204: Slot(176): Link Down [ 52.884184] pciehp 0000:b0:06.0:pcie204: Slot(176): Link Down event queued; currently getting powered on [ 52.967175] pciehp 0000:b0:06.0:pcie204: Slot(176): Link Up
Btw, from another discussion we're having with Yazen:
@Yazen, do you see how this error record is worth shit?
class_code: 000406
command: 0x0407, status: 0x0010
bridge: secondary_status: 0x0000, control: 0x0003
aer_status: 0x00100000, aer_mask: 0x01a10000
aer_uncor_severity: 0x004eb030
those above are only some of the fields which are purely useless undecoded. Makes me wonder what's worse for the user: dump the half-decoded error or not dump an error at all...
Anyway, Alex, I see this in the logs:
[ 66.581121] pciehp 0000:b0:06.0:pcie204: Slot(176): Link Down [ 66.591939] pciehp 0000:b0:05.0:pcie204: Slot(179): Card not present [ 66.592102] pciehp 0000:b0:06.0:pcie204: Slot(176): Card not present
and that comes from that pciehp_isr() interrupt handler AFAICT.
So there *is* a way to know that the card is not present anymore. So, theoretically, and ignoring the code layering for now, we can connect that error to the card not present event and then ignore the error...
Hmmm.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
Quoted 45 lines · Borislav Petkov
Quoted 33 lines · Alex G.
Quoted 1 line
How does such an error look like, in detail?
It's green on the soft side, with lots of red accents, as well as some textured white shades:
[ 51.414616] pciehp 0000:b0:06.0:pcie204: Slot(176): Link Down [ 51.414634] pciehp 0000:b0:05.0:pcie204: Slot(179): Link Down [ 52.703343] FIRMWARE BUG: Firmware sent fatal error that we were able to correct [ 52.703345] BROKEN FIRMWARE: Complain to your hardware vendor [ 52.703347] {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 [ 52.703358] pciehp 0000:b0:06.0:pcie204: Slot(176): Link Up [ 52.711616] {1}[Hardware Error]: event severity: fatal [ 52.716754] {1}[Hardware Error]: Error 0, type: fatal [ 52.721891] {1}[Hardware Error]: section_type: PCIe error [ 52.727463] {1}[Hardware Error]: port_type: 6, downstream switch port [ 52.734075] {1}[Hardware Error]: version: 3.0 [ 52.738607] {1}[Hardware Error]: command: 0x0407, status: 0x0010 [ 52.744786] {1}[Hardware Error]: device_id: 0000:b0:06.0 [ 52.750271] {1}[Hardware Error]: slot: 4 [ 52.754371] {1}[Hardware Error]: secondary_bus: 0xb3 [ 52.759509] {1}[Hardware Error]: vendor_id: 0x10b5, device_id: 0x9733 [ 52.766123] {1}[Hardware Error]: class_code: 000406 [ 52.771182] {1}[Hardware Error]: bridge: secondary_status: 0x0000, control: 0x0003 [ 52.779038] pcieport 0000:b0:06.0: aer_status: 0x00100000, aer_mask: 0x01a10000 [ 52.782303] nvme0n1: detected capacity change from 3200631791616 to 0 [ 52.786348] pcieport 0000:b0:06.0: [20] Unsupported Request [ 52.786349] pcieport 0000:b0:06.0: aer_layer=Transaction Layer, aer_agent=Requester ID [ 52.786350] pcieport 0000:b0:06.0: aer_uncor_severity: 0x004eb030Quoted 8 lines
[ 52.786352] pcieport 0000:b0:06.0: TLP Header: 40000001 0000020f e12023bc 01000000 [ 52.786357] pcieport 0000:b0:06.0: broadcast error_detected message [ 52.883895] pci 0000:b3:00.0: device has no driver [ 52.883976] pciehp 0000:b0:06.0:pcie204: Slot(176): Link Down [ 52.884184] pciehp 0000:b0:06.0:pcie204: Slot(176): Link Down event queued; currently getting powered on [ 52.967175] pciehp 0000:b0:06.0:pcie204: Slot(176): Link UpBtw, from another discussion we're having with Yazen:
Quoted 7 lines
@Yazen, do you see how this error record is worth shit?
class_code: 000406
command:0x0407, status:0x0010
bridge:secondary_status:0x0000, control:0x0003
aer_status:0x00100000,aer_mask:0x01a10000
aer_uncor_severity:0x004eb030
That tells you what FFS said about the error. Keep in mind that FFS has cleared the hardware error bits, which the AER handler would normally read from the PCI device.
Quoted 3 lines
those above are only some of the fields which are purely useless undecoded. Makes me wonder what's worse for the user: dump the half-decoded error or not dump an error at all...
It's immediately obvious if there's a glaring FFS bug and if we get bogus data. If you distrust firmware as much as I do, then you will find great value in having such info in the logs. It's probably not too useful to a casual user, but then neither is a majority of the system log.
Quoted 11 lines
Anyway, Alex, I see this in the logs:
[ 66.581121] pciehp 0000:b0:06.0:pcie204: Slot(176): Link Down [ 66.591939] pciehp 0000:b0:05.0:pcie204: Slot(179): Card not present [ 66.592102] pciehp 0000:b0:06.0:pcie204: Slot(176): Card not presentand that comes from that
pciehp_isr()interrupt handler AFAICT.So there *is* a way to know that the card is not present anymore. So, theoretically, and ignoring the code layering for now, we can connect that error to the card not present event and then ignore the error...
You're missing the timing and assuming you will get the hotplug interrupt. In this example, you have 22ms between the link down and presence detect state change. This is a fairly fast removal.
Hotplug dependencies aside (you can have the kernel run without PCIe hotplug support), I don't think you want to just linger in NMI for dozens of milliseconds waiting for presence detect confirmation.
For enterprise SFF NVMe drives, the data lanes will disconnect before the presence detect. FFS relies on presence detect, and these are two of the reasons why slow removal is such a problem. You might not get a presence detect interrupt at all.
Presence detect is optional for PCIe. PD is such a reliable heuristic, that it guarantees worse error handling than the crackmonkey firmware. I don't see how might be useful in a way which gives us better handling than firmware.
Quoted 1 line
Hmmm.
Hmmm
Anyway, heuristics about PCIe error recovery belong in the recovery handler. I don't think it's smart to apply policy before we get there
Alex
Borislav Petkov
maintainerRe: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
Quoted 1 line · Alex G.
That tells you what FFS said about the error.
I betcha those status and command values have a human-readable counterparts.
Btw, what do you abbreviate with "FFS"?
Quoted 4 lines
It's immediately obvious if there's a glaring FFS bug and if we get bogus data. If you distrust firmware as much as I do, then you will find great value in having such info in the logs. It's probably not too useful to a casual user, but then neither is a majority of the system log.
No no, you're missing the point - I *want* all data in the error log which helps debug a hardware issue. I just want it humanly readable so that I don't have to jot down the values and go scour the manuals to map what it actually means.
Quoted 7 lines
You're missing the timing and assuming you will get the hotplug interrupt. In this example, you have 22ms between the link down and presence detect state change. This is a fairly fast removal.
Hotplug dependencies aside (you can have the kernel run without PCIe hotplug support), I don't think you want to just linger in NMI for dozens of milliseconds waiting for presence detect confirmation.
No, I don't mean that. I mean something like deferred processing: you get an error, you notice it is a device which supports physical removal so you exit the NMI handler and process the error in normal, process context which allows you to query the device and say, "Hey device, are you still there?"
If it is not, you drop all the hw I/O errors reported for it.
Hmmm?
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
Quoted 6 lines · Borislav Petkov
Quoted 1 line · Alex G.
That tells you what FFS said about the error.
I betcha those status and command values have a human-readable counterparts.
Btw, what do you abbreviate with "FFS"?
Firmware-first.
Quoted 9 lines
Quoted 4 lines
It's immediately obvious if there's a glaring FFS bug and if we get bogus data. If you distrust firmware as much as I do, then you will find great value in having such info in the logs. It's probably not too useful to a casual user, but then neither is a majority of the system log.
No no, you're missing the point - I *want* all data in the error log which helps debug a hardware issue. I just want it humanly readable so that I don't have to jot down the values and go scour the manuals to map what it actually means.
We could probably use more of the native AER print functions, but that's beyond the scope of this patch. I tried something like this [1], but have given up following the PCI maintainer's radio silence. I don't care _that_ much about the log format.
[1] http://www.spinics.net/lists/linux-pci/msg71422.html
Quoted 9 lines
Quoted 7 lines
You're missing the timing and assuming you will get the hotplug interrupt. In this example, you have 22ms between the link down and presence detect state change. This is a fairly fast removal.
Hotplug dependencies aside (you can have the kernel run without PCIe hotplug support), I don't think you want to just linger in NMI for dozens of milliseconds waiting for presence detect confirmation.
No, I don't mean that. I mean something like deferred processing:
Like the exact thing that this patch series implements? :)
Quoted 5 lines
you
get an error, you notice it is a device which supports physical removal so you exit the NMI handler and process the error in normal, process context which allows you to query the device and say, "Hey device, are you still there?"
Like the exact way the AER handler works?
Quoted 1 line
If it is not, you drop all the hw I/O errors reported for it.
Like the PCI error recovery mechanisms that AER invokes?
Quoted 1 line
Hmmm?
Hmmm
Borislav Petkov
maintainerRe: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
Quoted 1 line · Alex G.
Firmware-first.
Ok, my guess was right.
Quoted 2 lines
We could probably use more of the native AER print functions, but that's beyond the scope of this patch.
No no, this does not belong in this patchset.
Quoted 1 line
Like the exact thing that this patch series implements? :)
Exact thing? I don't think so.
No, your patchset is grafting some funky and questionable side-handler which gets to see the PCIe errors first, out-of-line and then it practically downgrades their severity outside of the error processing flow.
What I've been telling you to do is to extend ghes_severity() to give the lower than PANIC severity for CPER_SEC_PCIE errors first so that the machine doesn't panic from them anymore and those PCIe errors get processed in the normal error processing path down through ghes_do_proc() and then land in ghes_handle_aer(). No adhoc ->handle_irqsafe thing - just the normal straightforward error processing path.
There, in ghes_handle_aer(), you do the check whether the device is still there - i.e., you try to apply some heuristics to detect the error type and why the system is complaining - you maybe even check whether the NVMe device is still there - and *then* you do the proper recovery action.
And you document for the future people looking at this code *why* you're doing this.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
Quoted 10 lines · Borislav Petkov
Quoted 1 line · Alex G.
Firmware-first.
Ok, my guess was right.
Quoted 2 lines
We could probably use more of the native AER print functions, but that's beyond the scope of this patch.
No no, this does not belong in this patchset.
Quoted 8 lines
Quoted 1 line
Like the exact thing that this patch series implements? :)
Exact thing? I don't think so.
No, your patchset is grafting some funky and questionable side-handler which gets to see the PCIe errors first, out-of-line and then it practically downgrades their severity outside of the error processing flow.
SURPRISE!!! This is a what vs how issue. I am keeping the what, and working on the how that you suggested.
Quoted 1 line
What I've been telling you
It's coming (eventually). I'm trying to avoid pushing more than one series per week.
(snip useful email context)
Hmmm.
Alex
Borislav Petkov
maintainerRe: [RFC PATCH v2 3/4] acpi: apei: Do not panic() when correctable errors are marked as fatal.
Quoted 1 line · Alex G.
SURPRISE!!!
What does that mean? You've had too much coffee?
Quoted 2 lines
It's coming (eventually). I'm trying to avoid pushing more than one series per week.
You better. Flooding people with patchsets won't get you very far.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.RFC PATCH v2 4/4 acpi: apei: Warn when GHES marks correctable errors as "fatal"
In reply to Alexandru GagniucThere seems to be a culture amongst BIOS teams to want to crash the OS when an error can't be handled in firmware. Marking GHES errors as "fatal" is a very common way to do this.
However, a number of errors reported by GHES may be fatal in the sense a device or link is lost, but are not fatal to the system. When there is a disagreement with firmware about the handleability of an error, print a warning message.
--- drivers/acpi/apei/ghes.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/acpi/apei/ghes.c +9 -1
@@ -535,13 +535,14 @@ static const struct ghes_handler *get_handler(const guid_t *type)static void ghes_do_proc (struct ghes *ghes,const struct acpi_hest_generic_status *estatus) { -int sev, sec_sev; +int sev, sec_sev, corrected_sev;struct acpi_hest_generic_data *gdata;const struct ghes_handler *handler;guid_t *sec_type;guid_t *fru_id = &NULL_UUID_LE ;char *fru_text ="" ; + corrected_sev =GHES_SEV_NO ; sev =ghes_severity (estatus->error_severity);apei_estatus_for_each_section (estatus, gdata) { sec_type = (guid_t *)gdata->section_type;
@@ -563,6 +564,13 @@ static void ghes_do_proc(struct ghes *ghes, sec_sev, err, gdata->error_data_length); } + + corrected_sev =max (corrected_sev, sec_sev); + } + +if ((sev >=GHES_SEV_PANIC ) && (corrected_sev < sev)) { +pr_warn ("FIRMWARE BUG: Firmware sent fatal error that we were able to correct" ); +pr_warn ("BROKEN FIRMWARE: Complain to your hardware vendor" ); } }
Signature · 1 line
2.14.3
Borislav Petkov
maintainerRe: [RFC PATCH v2 4/4] acpi: apei: Warn when GHES marks correctable errors as "fatal"
Quoted 35 lines · Alexandru Gagniuc
There seems to be a culture amongst BIOS teams to want to crash the OS when an error can't be handled in firmware. Marking GHES errors as "fatal" is a very common way to do this.
However, a number of errors reported by GHES may be fatal in the sense a device or link is lost, but are not fatal to the system. When there is a disagreement with firmware about the handleability of an error, print a warning message.
Signed-off-by Alexandru Gagniuc <[email protected]>--- drivers/acpi/apei/ghes.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)drivers/acpi/apei/ghes.c +2
-1@@ -535,13 +535,14 @@ static const struct ghes_handler *get_handler(const guid_t *type)static void ghes_do_proc (struct ghes *ghes,const struct acpi_hest_generic_status *estatus) { -int sev, sec_sev; +int sev, sec_sev, corrected_sev;struct acpi_hest_generic_data *gdata;const struct ghes_handler *handler;guid_t *sec_type;guid_t *fru_id = &NULL_UUID_LE ;char *fru_text ="" ; + corrected_sev =GHES_SEV_NO ; sev =ghes_severity (estatus->error_severity);apei_estatus_for_each_section (estatus, gdata) { sec_type = (guid_t *)gdata->section_type;
drivers/acpi/apei/ghes.c static void ghes_do_proc(struct ghes *ghes, · line 573
Quoted 10 lines
drivers/acpi/apei/ghes.c +7
-0@@ -563,6 +564,13 @@ static void ghes_do_proc(struct ghes *ghes, sec_sev, err, gdata->error_data_length); } + + corrected_sev =max (corrected_sev, sec_sev); + } + +if ((sev >=GHES_SEV_PANIC ) && (corrected_sev < sev)) { +pr_warn ("FIRMWARE BUG: Firmware sent fatal error that we were able to correct" ); +pr_warn ("BROKEN FIRMWARE: Complain to your hardware vendor" );
No, I don't want any of that crap issuing stuff in dmesg and then people opening bugs and running around and trying to replace hardware.
We either can handle the error and log a normal record somewhere or we cannot and explode. The complaining about the FW doesn't bring shit.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [RFC PATCH v2 4/4] acpi: apei: Warn when GHES marks correctable errors as "fatal"
Quoted 1 line · Borislav Petkov
On Mon, Apr 16, 2018 at 04:59:03PM -0500, Alexandru Gagniuc wrote:
(snip)
Quoted 11 lines
drivers/acpi/apei/ghes.c static void ghes_do_proc(struct ghes *ghes, · line 573
Quoted 7 lines
drivers/acpi/apei/ghes.c +7
-0+ + corrected_sev =max (corrected_sev, sec_sev); + } + +if ((sev >=GHES_SEV_PANIC ) && (corrected_sev < sev)) { +pr_warn ("FIRMWARE BUG: Firmware sent fatal error that we were able to correct" ); +pr_warn ("BROKEN FIRMWARE: Complain to your hardware vendor" );No, I don't want any of that crap issuing stuff in dmesg and then people opening bugs and running around and trying to replace hardware.
Quoted 2 lines
We either can handle the error and log a normal record somewhere or we cannot and explode.
There is value in this. From my observations, fw claims it will do everything through FFS, yet fails to fully handle the situation. It's rooted in FW's assumptions about OS behavior. Because the (old) versions of windows, esxi, and rhel used during development crash, fw assumes that _all_ OSes crash. The result in a surprising majority of cases is that FFS doesn't properly handle recurring errors, and fw is, in fact, broken.
Quoted 1 line
The complaining about the FW doesn't bring shit.
You are correct. It doesn't bring defecation. It brings a red flag that helps people get closer to the root cause of problems.
That being said, I can just drop this patch.
Alex
Borislav Petkov
maintainerRe: [RFC PATCH v2 4/4] acpi: apei: Warn when GHES marks correctable errors as "fatal"
Quoted 7 lines · Alex G.
There is value in this. From my observations, fw claims it will do everything through FFS, yet fails to fully handle the situation. It's rooted in FW's assumptions about OS behavior. Because the (old) versions of windows, esxi, and rhel used during development crash, fw assumes that _all_ OSes crash. The result in a surprising majority of cases is that FFS doesn't properly handle recurring errors, and fw is, in fact, broken.
So FW being broken is a social secret. But we don't care. We have tried, nothing happens. No one moves. The crack monkeys which program it have long moved to the next release and you hear crap like, "we don't support linux" and other bullshit.
What we do now is to try to make the best of it - we either can handle an error *without* firmware's help or we panic. If we can recover from it, let's do that without screaming about something the user can't deal with anyway.
All those FW_ERR printks cause nothing but expensive support calls, the outcome of which is nothing. Just a lot of money down the drain.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.v3 cover acpi: apei: Improve PCIe error handling with firmware-first
In reply to Alexandru GagniucOr "acpi: apei: Don't trust firmware any further than you can throw it"
This is the improved implementation following feedback from James and Borislav. This implementation is much simpler, albeit less flexible than v2.
I'm leaving this as RFC because the BIOS team is a bit scared of an OS that won't crash when it's told to. However, if people like the idea, then I have nothing against merging this.
Borislav, if you don't like the third patch in the series, feel free to leave it out. THings will work beautifully with or without it.
Changes since v2:
- Due to popular request, simple is chosen over flexible
- Removed splitting of handlers into irq safe portion.
- Change behavior only for PCIe errors
Changes since v1:
- Due to popular request, the panic() is left in the NMI handler
- GHES AER handler is split into NMI and non-NMI portions
- ghes_notify_nmi() does not panic on deferrable errors
- The handlers are put in a mapping and given a common call signature
Alexandru Gagniuc (3):
EDAC, GHES: Remove unused argument to ghes_edac_report_mem_error
acpi: apei: Do not panic() on PCIe errors reported through GHES
acpi: apei: Warn when GHES marks correctable errors as "fatal"
drivers/acpi/apei/ghes.c | 56 +++++++++++++++++++++++++++++++++++++++++++----- drivers/edac/ghes_edac.c | 3 +-- include/acpi/ghes.h | 5 ++--- 3 files changed, 54 insertions(+), 10 deletions(-)
Signature · 1 line
2.14.3
RFC PATCH v3 1/3 EDAC, GHES: Remove unused argument to ghes_edac_report_mem_error
The use of the 'ghes' argument was removed in a previous commit, but function signature was not updated to reflect this.
--- drivers/acpi/apei/ghes.c | 2 +- drivers/edac/ghes_edac.c | 3 +-- include/acpi/ghes.h | 5 ++--- 3 files changed, 4 insertions(+), 6 deletions(-)
drivers/acpi/apei/ghes.c +1 -1
@@ -481,7 +481,7 @@ static void ghes_do_proc(struct ghes *ghes,if (guid_equal (sec_type, &CPER_SEC_PLATFORM_MEM )) {struct cper_sec_mem_err *mem_err =acpi_hest_get_payload (gdata); -ghes_edac_report_mem_error (ghes, sev, mem_err); +ghes_edac_report_mem_error (sev, mem_err);arch_apei_report_mem_error (sev, mem_err);ghes_handle_memory_failure (gdata, sev);
drivers/edac/ghes_edac.c +1 -2
@@ -172,8 +172,7 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg) } } -void ghes_edac_report_mem_error (struct ghes *ghes,int sev, -struct cper_sec_mem_err *mem_err) +void ghes_edac_report_mem_error (int sev,struct cper_sec_mem_err *mem_err) {enum hw_event_mc_err_type type;struct edac_raw_error_desc *e;
include/acpi/ghes.h +2 -3
@@ -55,15 +55,14 @@ enum {/* From drivers/edac/ghes_edac.c */ #ifdef CONFIG_EDAC_GHES -void ghes_edac_report_mem_error (struct ghes *ghes,int sev, -struct cper_sec_mem_err *mem_err); +void ghes_edac_report_mem_error (int sev,struct cper_sec_mem_err *mem_err);int ghes_edac_register (struct ghes *ghes,struct device *dev);void ghes_edac_unregister (struct ghes *ghes);#else -static inline void ghes_edac_report_mem_error (struct ghes *ghes,int sev, +static inline void ghes_edac_report_mem_error (int sev,struct cper_sec_mem_err *mem_err) { }
Signature · 1 line
2.14.3
RFC PATCH v3 2/3 acpi: apei: Do not panic() on PCIe errors reported through GHES
In reply to Alexandru GagniucThe policy was to panic() when GHES said that an error is "Fatal". This logic is wrong for several reasons, as it doesn't take into account what caused the error.
PCIe fatal errors indicate that the link to a device is either unstable or unusable. They don't indicate that the machine is on fire, and they are not severe enough that we need to panic(). Instead of relying on crackmonkey firmware, evaluate the error severity based on what caused the error (GHES subsections).
--- drivers/acpi/apei/ghes.c | 48 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 4 deletions(-)
drivers/acpi/apei/ghes.c +44 -4
@@ -425,8 +425,7 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int* GHES_SEV_RECOVERABLE -> AER_NONFATAL * GHES_SEV_RECOVERABLE && CPER_SEC_RESET -> AER_FATAL * These both need to be reported and recovered from by the AER driver. -* GHES_SEV_PANIC does not make it to this handling since the kernel must -* panic. +* GHES_SEV_PANIC -> AER_FATAL */ static void ghes_handle_aer (struct acpi_hest_generic_data *gdata) {
@@ -459,6 +458,46 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)#endif } +/* PCIe errors should not cause a panic. */ +static int ghes_sec_pcie_severity (struct acpi_hest_generic_data *gdata) +{ +struct cper_sec_pcie *pcie_err =acpi_hest_get_payload (gdata); + +if (pcie_err->validation_bits &CPER_PCIE_VALID_DEVICE_ID && + pcie_err->validation_bits &CPER_PCIE_VALID_AER_INFO && +IS_ENABLED (CONFIG_ACPI_APEI_PCIEAER )) +return CPER_SEV_RECOVERABLE ; + +return ghes_severity (gdata->error_severity); +} +/* +* The severity field in the status block is oftentimes more severe than it +* needs to be. This makes it an unreliable metric for the severity. A more +* reliable way is to look at each subsection and correlate it with how well +* the error can be handled. +* - SEC_PCIE: All PCIe errors can be handled by AER. +*/ +static int ghes_actual_severity (struct ghes *ghes) +{ +int worst_sev, sec_sev; +struct acpi_hest_generic_data *gdata; +const guid_t *section_type; +const struct acpi_hest_generic_status *estatus = ghes->estatus; + + worst_sev =GHES_SEV_NO ; +apei_estatus_for_each_section (estatus, gdata) { + section_type = (guid_t *)gdata->section_type; + sec_sev =ghes_severity (gdata->error_severity); + +if (guid_equal (section_type, &CPER_SEC_PCIE )) + sec_sev =ghes_sec_pcie_severity (gdata); + + worst_sev =max (worst_sev, sec_sev); + } + +return worst_sev; +} +static void ghes_do_proc (struct ghes *ghes,const struct acpi_hest_generic_status *estatus) {
@@ -932,7 +971,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 sev, asev, ret =NMI_DONE ;if (!atomic_add_unless (&ghes_in_nmi,1 ,1 ))return ret;
@@ -945,8 +984,9 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) ret =NMI_HANDLED ; } + asev =ghes_actual_severity (ghes); sev =ghes_severity (ghes->estatus->error_severity); -if (sev >=GHES_SEV_PANIC ) { +if ((sev >=GHES_SEV_PANIC ) && (asev >=GHES_SEV_PANIC )) {oops_begin ();ghes_print_queued_estatus ();__ghes_panic (ghes);
Signature · 1 line
2.14.3
Borislav Petkov
maintainerRe: [RFC PATCH v3 2/3] acpi: apei: Do not panic() on PCIe errors reported through GHES
Quoted 10 lines · Alexandru Gagniuc
drivers/acpi/apei/ghes.c +1
-1@@ -932,7 +971,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 sev, asev, ret =NMI_DONE ;if (!atomic_add_unless (&ghes_in_nmi,1 ,1 ))return ret;
drivers/acpi/apei/ghes.c static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) · line 988
Quoted 5 lines
drivers/acpi/apei/ghes.c +1
-0@@ -945,8 +984,9 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) ret =NMI_HANDLED ; } + asev =ghes_actual_severity (ghes); sev =ghes_severity (ghes->estatus->error_severity);
So renaming ghes_deferrable_severity() to ghes_actual_severity() is not a big change. And that's not what I meant.
I'd like to see here:
sev = ghes_severity(ghes);
and inside you do all the required mapping/severity processing/etc. And you can rename the current ghes_severity() to ghes_map_cper_severity() or whatever...
drivers/acpi/apei/ghes.c static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) · line 989
Quoted 2 lines
drivers/acpi/apei/ghes.c +1
-1-if (sev >=GHES_SEV_PANIC ) { +if ((sev >=GHES_SEV_PANIC ) && (asev >=GHES_SEV_PANIC )) {
... so that this change doesn't happen and there are not two severities queried but a single one.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [RFC PATCH v3 2/3] acpi: apei: Do not panic() on PCIe errors reported through GHES
Hi Borislav,
Quoted 14 lines · Borislav Petkov
Quoted 13 lines · Alexandru Gagniuc
drivers/acpi/apei/ghes.c +1
-1@@ -932,7 +971,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 sev, asev, ret =NMI_DONE ;if (!atomic_add_unless (&ghes_in_nmi,1 ,1 ))return ret;@@ -945,8 +984,9 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) ret =NMI_HANDLED ; }
Quoted 5 lines
drivers/acpi/apei/ghes.c static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) · line 988
Quoted 2 lines
drivers/acpi/apei/ghes.c +1
-0+ asev =ghes_actual_severity (ghes); sev =ghes_severity (ghes->estatus->error_severity);So renaming
ghes_deferrable_severity()toghes_actual_severity()is not a big change. And that's not what I meant.
I'm sorry I misunderstood you.
Quoted 3 lines
I'd like to see here:
sev = ghes_severity(ghes);
sev = ghes_severity(ghes);
Quoted 3 lines
and inside you do all the required mapping/severity processing/etc. And you can rename the current
ghes_severity()toghes_map_cper_severity()or whatever...
I agree that the current ghes_severity() name is vague. I'll get it done properly in v4 (next week).
Quoted 5 lines
drivers/acpi/apei/ghes.c static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) · line 989
Quoted 2 lines
drivers/acpi/apei/ghes.c +1
-1-if (sev >=GHES_SEV_PANIC ) { +if ((sev >=GHES_SEV_PANIC ) && (asev >=GHES_SEV_PANIC )) {... so that this change doesn't happen and there are not two severities queried but a single one.
Two severities is a result of the wanky GHES data structure. Nothing says we have to use the severity field in the header... if you're okay with just ignoring it.
Alex
RFC PATCH v3 3/3 acpi: apei: Warn when GHES marks correctable errors as "fatal"
In reply to Alexandru GagniucThere seems to be a culture amongst BIOS teams to want to crash the OS when an error can't be handled in firmware. Marking GHES errors as "fatal" is a very common way to do this.
However, a number of errors reported by GHES may be fatal in the sense a device or link is lost, but are not fatal to the system. When there is a disagreement with firmware about the handleability of an error, print a warning message.
--- drivers/acpi/apei/ghes.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/acpi/apei/ghes.c +6 -0
@@ -539,6 +539,12 @@ static void ghes_do_proc(struct ghes *ghes, sec_sev, err, gdata->error_data_length); } + + } + +if ((sev >=GHES_SEV_PANIC ) && (ghes_actual_severity (ghes) < sev)) { +pr_warn ("FIRMWARE BUG: Firmware sent fatal error that we were able to correct" ); +pr_warn ("BROKEN FIRMWARE: Complain to your hardware vendor" ); } }
Signature · 1 line
2.14.3
Borislav Petkov
maintainerRe: [RFC PATCH v3 3/3] acpi: apei: Warn when GHES marks correctable errors as "fatal"
Quoted 19 lines · Alexandru Gagniuc
There seems to be a culture amongst BIOS teams to want to crash the OS when an error can't be handled in firmware. Marking GHES errors as "fatal" is a very common way to do this.
However, a number of errors reported by GHES may be fatal in the sense a device or link is lost, but are not fatal to the system. When there is a disagreement with firmware about the handleability of an error, print a warning message.
Signed-off-by Alexandru Gagniuc <[email protected]>--- drivers/acpi/apei/ghes.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/acpi/apei/ghes.c +0
-0
drivers/acpi/apei/ghes.c static void ghes_do_proc(struct ghes *ghes, · line 547
Quoted 9 lines
drivers/acpi/apei/ghes.c +6
-0@@ -539,6 +539,12 @@ static void ghes_do_proc(struct ghes *ghes, sec_sev, err, gdata->error_data_length); } + + } + +if ((sev >=GHES_SEV_PANIC ) && (ghes_actual_severity (ghes) < sev)) { +pr_warn ("FIRMWARE BUG: Firmware sent fatal error that we were able to correct" ); +pr_warn ("BROKEN FIRMWARE: Complain to your hardware vendor" );
Pasting the same comment from last time since you missed it:
"No, I don't want any of that crap issuing stuff in dmesg and then people opening bugs and running around and trying to replace hardware.
We either can handle the error and log a normal record somewhere or we cannot and explode. The complaining about the FW doesn't bring shit."
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [RFC PATCH v3 3/3] acpi: apei: Warn when GHES marks correctable errors as "fatal"
Quoted 7 lines · Borislav Petkov
Pasting the same comment from last time since you missed it:
"No, I don't want any of that crap issuing stuff in dmesg and then people opening bugs and running around and trying to replace hardware.
We either can handle the error and log a normal record somewhere or we cannot and explode. The complaining about the FW doesn't bring shit."
" Borislav, if you don't like the third patch in the series, feel free to leave it out. THings will work beautifully with or without it."
:)
Borislav Petkov
maintainerRe: [RFC PATCH v3 3/3] acpi: apei: Warn when GHES marks correctable errors as "fatal"
Quoted 2 lines · Alex G.
" Borislav, if you don't like the third patch in the series, feel free to leave it out. THings will work beautifully with or without it."
Then don't send it.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Moore
Re: [Devel] [RFC PATCH v3 3/3] acpi: apei: Warn when GHES marks correctable errors as "fatal"
In reply to Alex G.Quoted 18 lines
-----Original Message-----
From: Alex G. [mailto:mr.nuke.me(a)gmail.com]
Sent: Thursday, April 26, 2018 10:48 AM
To: Borislav Petkov <bp(a)alien8.de>
Cc: linux-acpi(a)vger.kernel.org; linux-edac(a)vger.kernel.org; Rafael J. Wysocki <rjw(a)rjwysocki.net>; Len Brown <lenb(a)kernel.org>; Luck, Tony <tony.luck(a)intel.com>; Mauro Carvalho Chehab <mchehab(a)kernel.org>; Moore, Robert <robert.moore(a)intel.com>; Schmauss, Erik <erik.schmauss(a)intel.com>; Tyler Baicar <tbaicar(a)codeaurora.org>; Will Deacon <will.deacon(a)arm.com>; James Morse <james.morse(a)arm.com>; Shiju Jose <shiju.jose(a)huawei.com>; Jonathan (Zhixiong) Zhang <zjzhang(a)codeaurora.org>; Dongjiu Geng <gengdongjiu(a)huawei.com>; linux- kernel(a)vger.kernel.org; devel(a)acpica.org
Subject: Re: [RFC PATCH v3 3/3] acpi: apei: Warn when GHES marks correctable errors as "fatal"
Quoted 7 lines
Quoted 6 lines · Borislav Petkov
Pasting the same comment from last time since you missed it:
"No, I don't want any of that crap issuing stuff in dmesg and then people opening bugs and running around and trying to replace hardware.
We either can handle the error and log a normal record somewhere
Usually nobody looks at these, it seems to me. On windows, ACPI errors just go into the bit bucket.
or we
Quoted 1 line
Quoted 1 line
cannot and explode.
This of course would be more windows behavior.
Quoted 1 line
Quoted 1 line
The complaining about the FW doesn't bring shit."
We seem to get down to more of a philosophical problem. Basically, just ignore errors or report them somewhere where they will be actually seen. Or, just crash.
Quoted 5 lines
" Borislav, if you don't like the third patch in the series, feel free to leave it out. THings will work beautifully with or without it."
:)
Re: [RFC PATCH v3 3/3] acpi: apei: Warn when GHES marks correctable errors as "fatal"
In reply to Borislav PetkovQuoted 18 lines · Borislav Petkov
Quoted 10 lines · Alexandru Gagniuc
There seems to be a culture amongst BIOS teams to want to crash the OS when an error can't be handled in firmware. Marking GHES errors as "fatal" is a very common way to do this.
However, a number of errors reported by GHES may be fatal in the sense a device or link is lost, but are not fatal to the system. When there is a disagreement with firmware about the handleability of an error, print a warning message.
drivers/acpi/apei/ghes.c static void ghes_do_proc(struct ghes *ghes, · line 547
Quoted 4 lines
drivers/acpi/apei/ghes.c +4
-0+ +if ((sev >=GHES_SEV_PANIC ) && (ghes_actual_severity (ghes) < sev)) { +pr_warn ("FIRMWARE BUG: Firmware sent fatal error that we were able to correct" ); +pr_warn ("BROKEN FIRMWARE: Complain to your hardware vendor" );Pasting the same comment from last time since you missed it:
Quoted 2 lines
"No, I don't want any of that crap issuing stuff in dmesg and then people opening bugs and running around and trying to replace hardware.
We want to see warnings. Maybe they can be toned done. We even have dedicated distros for firmware testing.
Quoted 1 line
Good mailing practices for 400: avoid top-posting and trim the reply.
Good mailing practices -- limit use of four letter words on public lists.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [RFC PATCH v3 3/3] acpi: apei: Warn when GHES marks correctable errors as "fatal"
Quoted 19 lines · Pavel Machek
Quoted 18 lines · Borislav Petkov
Quoted 10 lines · Alexandru Gagniuc
There seems to be a culture amongst BIOS teams to want to crash the OS when an error can't be handled in firmware. Marking GHES errors as "fatal" is a very common way to do this.
However, a number of errors reported by GHES may be fatal in the sense a device or link is lost, but are not fatal to the system. When there is a disagreement with firmware about the handleability of an error, print a warning message.
drivers/acpi/apei/ghes.c static void ghes_do_proc(struct ghes *ghes, · line 547
Quoted 4 lines
drivers/acpi/apei/ghes.c +4
-0+ +if ((sev >=GHES_SEV_PANIC ) && (ghes_actual_severity (ghes) < sev)) { +pr_warn ("FIRMWARE BUG: Firmware sent fatal error that we were able to correct" ); +pr_warn ("BROKEN FIRMWARE: Complain to your hardware vendor" );Pasting the same comment from last time since you missed it:
Quoted 5 lines
Quoted 2 lines
"No, I don't want any of that crap issuing stuff in dmesg and then people opening bugs and running around and trying to replace hardware.
We want to see warnings. Maybe they can be toned done. We even have dedicated distros for firmware testing.
I'm told that had we had this warning when the r740 BIOS was in development, we would have solved a lot of the issues that I'm currently working on. That would, in turn, have exposed bigger issues, and we would have had a platform to fix and test those bigger issues.
Hardware vendors who test on linux might be scratching their heads at this error, though they tend to figure out what they're doing wrong, and fix it.
One argument against was "expensive support calls", on which I call BS. The firmware resources are expensive, but those are there whether or not the customers call to complain.
Alex
Quoted 3 lines
Quoted 1 line
Good mailing practices for 400: avoid top-posting and trim the reply.
Good mailing practices -- limit use of four letter words on public lists.
Then can't show word 'four'.