ACPI / APEI: Boot Error Record Table processing was needlessly complicated
Subsystems (3)
Luck, Tony
reviewerPATCH 1/1 ACPI / APEI: Boot Error Record Table processing was needlessly complicated
From: Tony Luck <[email protected]>
Quoting version 6.1 of the ACPI specification. Section 18.3.1 "Boot Error Source" says:
The Boot Error Region is a range of addressable memory OSPM can access during initialization to determine if an unhandled error condition occurred. System firmware must report this memory range as firmware reserved. The format of the Boot Error Region follow that of an Error Status Block, this is defined in Section 18.3.2.7. The format of the error status block is described by Table 18-342.
This clarifies some points that were obfuscated in earlier versions. E.g. there is no longer a separate table to describe the format of the "Boot Error Region" (which was identical to the "Error Status Block"). Also saying "follow that of *an* Error Status Block" makes it clear that there is just one block (which can still contain multiple "Generic Error Data Entry structures").
The loop inside bert_print_all() is unnecessary (but probably harmless as the "while (remain > sizeof(struct acpi_bert_region))" loop should terminate after we skipped over the first entry.
We can drop the "bert_print_all()" function and just move the four relevant lines inline in "bert_init()". Since there are no remaining users of "struct acpi_bert_region" we delete it from <acpi/actbl1.h>
--- drivers/acpi/apei/bert.c | 54 +++++++----------------------------------------- include/acpi/actbl1.h | 11 +--------- 2 files changed, 8 insertions(+), 57 deletions(-)
drivers/acpi/apei/bert.c +7 -47
@@ -34,50 +34,6 @@static int bert_disable; -static void __init bert_print_all (struct acpi_bert_region *region, -unsigned int region_len) -{ -struct acpi_hest_generic_status *estatus = - (struct acpi_hest_generic_status *)region; -int remain = region_len; -u32 estatus_len; - -if (!estatus->block_status) -return ; - -while (remain >sizeof (struct acpi_bert_region )) { -if (cper_estatus_check (estatus)) { -pr_err (FW_BUG "Invalid error record.\n" ); -return ; - } - - estatus_len =cper_estatus_len (estatus); -if (remain < estatus_len) { -pr_err (FW_BUG "Truncated status block (length: %u).\n" , - estatus_len); -return ; - } - -pr_info_once ("Error records from previous boot:\n" ); - - cper_estatus_print(KERN_INFO HW_ERR, estatus); - -/* -* Because the boot error source is "one-time polled" type, -* clear Block Status of current Generic Error Status Block, -* once it's printed. -*/ - estatus->block_status =0 ; - - estatus = (void *)estatus + estatus_len; -/* No more error records. */ -if (!estatus->block_status) -return ; - - remain -= estatus_len; - } -} -static int __init setup_bert_disable (char *str) { bert_disable =1 ;
@@ -89,7 +45,7 @@ __setup("bert_disable", setup_bert_disable);static int __init bert_check_table (struct acpi_table_bert *bert_tab) {if (bert_tab->header.length <sizeof (struct acpi_table_bert ) || - bert_tab->region_length <sizeof (struct acpi_bert_region )) + bert_tab->region_length <sizeof (struct acpi_hest_generic_status ))return -EINVAL ;return 0 ;
@@ -98,7 +54,7 @@ static int __init bert_check_table(struct acpi_table_bert *bert_tab)static int __init bert_init (void ) {struct apei_resources bert_resources; -struct acpi_bert_region *boot_error_region; +struct acpi_hest_generic_status *boot_error_region;struct acpi_table_bert *bert_tab;unsigned int region_len;acpi_status status;
@@ -138,7 +94,11 @@ static int __init bert_init(void) goto out_fini; boot_error_region =ioremap_cache (bert_tab->address, region_len);if (boot_error_region) { -bert_print_all (boot_error_region, region_len); +if (boot_error_region->block_status) { +pr_info ("Error records from previous boot:\n" ); + cper_estatus_print(KERN_INFO HW_ERR, boot_error_region); + boot_error_region->block_status =0 ; + }iounmap (boot_error_region); }else { rc = -ENOMEM ;
include/acpi/actbl1.h +1 -10
@@ -127,16 +127,7 @@ struct acpi_table_bert {struct acpi_table_header header;/* Common ACPI table header */ u32 region_length;/* Length of the boot error region */ u64 address;/* Physical address of the error region */ -}; - -/* Boot Error Region (not a subtable, pointed to by Address field above) */ - -struct acpi_bert_region { -u32 block_status;/* Type of error information */ -u32 raw_data_offset;/* Offset to raw error data */ -u32 raw_data_length;/* Length of raw error data */ -u32 data_length;/* Length of generic error data */ -u32 error_severity;/* Severity code */ +/* which is a struct acpi_hest_generic_status */ };/* Values for block_status flags above */
Signature · 1 line
2.11.0
Luck, Tony
reviewerN.B. this really needs some testing by someone who has a system that generates populated BERT records. All I know at the moment is that it works with en empty BERT log
-Tony
Quoted 139 lines · Tony
From: Tony Luck <[email protected]>
Quoting version 6.1 of the ACPI specification. Section 18.3.1 "Boot Error Source" says:
The Boot Error Region is a range of addressable memory OSPM can access during initialization to determine if an unhandled error condition occurred. System firmware must report this memory range as firmware reserved. The format of the Boot Error Region follow that of an Error Status Block, this is defined in Section 18.3.2.7. The format of the error status block is described by Table 18-342.
This clarifies some points that were obfuscated in earlier versions. E.g. there is no longer a separate table to describe the format of the "Boot Error Region" (which was identical to the "Error Status Block"). Also saying "follow that of *an* Error Status Block" makes it clear that there is just one block (which can still contain multiple "Generic Error Data Entry structures").
The loop inside
bert_print_all()is unnecessary (but probably harmless as the "while (remain >sizeof(struct acpi_bert_region))" loop should terminate after we skipped over the first entry.We can drop the "
bert_print_all()" function and just move the four relevant lines inline in "bert_init()". Since there are no remaining users of "structacpi_bert_region" we delete it from <acpi/actbl1.h>Cc Len Brown <[email protected]>Cc Huang Ying <[email protected]>Cc Borislav Petkov <[email protected]>Cc Tomasz Nowicki <[email protected]>Cc Jonathan (Zhixiong) Zhang <[email protected]>Cc Tyler Baicar <[email protected]>Signed-off-by Tony Luck <[email protected]>--- drivers/acpi/apei/bert.c | 54 +++++++----------------------------------------- include/acpi/actbl1.h | 11 +--------- 2 files changed, 8 insertions(+), 57 deletions(-)drivers/acpi/apei/bert.c +7
-47@@ -34,50 +34,6 @@static int bert_disable; -static void __init bert_print_all (struct acpi_bert_region *region, -unsigned int region_len) -{ -struct acpi_hest_generic_status *estatus = - (struct acpi_hest_generic_status *)region; -int remain = region_len; -u32 estatus_len; - -if (!estatus->block_status) -return ; - -while (remain >sizeof (struct acpi_bert_region )) { -if (cper_estatus_check (estatus)) { -pr_err (FW_BUG "Invalid error record.\n" ); -return ; - } - - estatus_len =cper_estatus_len (estatus); -if (remain < estatus_len) { -pr_err (FW_BUG "Truncated status block (length: %u).\n" , - estatus_len); -return ; - } - -pr_info_once ("Error records from previous boot:\n" ); - - cper_estatus_print(KERN_INFO HW_ERR, estatus); - -/* -* Because the boot error source is "one-time polled" type, -* clear Block Status of current Generic Error Status Block, -* once it's printed. -*/ - estatus->block_status =0 ; - - estatus = (void *)estatus + estatus_len; -/* No more error records. */ -if (!estatus->block_status) -return ; - - remain -= estatus_len; - } -} -static int __init setup_bert_disable (char *str) { bert_disable =1 ;@@ -89,7 +45,7 @@ __setup("bert_disable", setup_bert_disable);static int __init bert_check_table (struct acpi_table_bert *bert_tab) {if (bert_tab->header.length <sizeof (struct acpi_table_bert ) || - bert_tab->region_length <sizeof (struct acpi_bert_region )) + bert_tab->region_length <sizeof (struct acpi_hest_generic_status ))return -EINVAL ;return 0 ;@@ -98,7 +54,7 @@ static int __init bert_check_table(struct acpi_table_bert *bert_tab)static int __init bert_init (void ) {struct apei_resources bert_resources; -struct acpi_bert_region *boot_error_region; +struct acpi_hest_generic_status *boot_error_region;struct acpi_table_bert *bert_tab;unsigned int region_len;acpi_status status;@@ -138,7 +94,11 @@ static int __init bert_init(void) goto out_fini; boot_error_region =ioremap_cache (bert_tab->address, region_len);if (boot_error_region) { -bert_print_all (boot_error_region, region_len); +if (boot_error_region->block_status) { +pr_info ("Error records from previous boot:\n" ); + cper_estatus_print(KERN_INFO HW_ERR, boot_error_region); + boot_error_region->block_status =0 ; + }iounmap (boot_error_region); }else { rc = -ENOMEM ;include/acpi/actbl1.h +0
-4@@ -127,16 +127,7 @@ struct acpi_table_bert {struct acpi_table_header header;/* Common ACPI table header */ u32 region_length;/* Length of the boot error region */ u64 address;/* Physical address of the error region */ -}; - -/* Boot Error Region (not a subtable, pointed to by Address field above) */ -
include/acpi/actbl1.h struct acpi_table_bert { · line 133
Quoted 10 lines
include/acpi/actbl1.h +1
-6-struct acpi_bert_region { -u32 block_status;/* Type of error information */ -u32 raw_data_offset;/* Offset to raw error data */ -u32 raw_data_length;/* Length of raw error data */ -u32 data_length;/* Length of generic error data */ -u32 error_severity;/* Severity code */ +/* which is a struct acpi_hest_generic_status */ };/* Values for block_status flags above */
The actbl1.h change should be routed through the upstream ACPICA I think after we've dropped all things depending on it from the kernel.
Thanks,
Rafael
Quoted 39 lines · Tony
From: Tony Luck <[email protected]>
Quoting version 6.1 of the ACPI specification. Section 18.3.1 "Boot Error Source" says:
The Boot Error Region is a range of addressable memory OSPM can access during initialization to determine if an unhandled error condition occurred. System firmware must report this memory range as firmware reserved. The format of the Boot Error Region follow that of an Error Status Block, this is defined in Section 18.3.2.7. The format of the error status block is described by Table 18-342.
This clarifies some points that were obfuscated in earlier versions. E.g. there is no longer a separate table to describe the format of the "Boot Error Region" (which was identical to the "Error Status Block"). Also saying "follow that of *an* Error Status Block" makes it clear that there is just one block (which can still contain multiple "Generic Error Data Entry structures").
The loop inside
bert_print_all()is unnecessary (but probably harmless as the "while (remain >sizeof(struct acpi_bert_region))" loop should terminate after we skipped over the first entry.We can drop the "
bert_print_all()" function and just move the four relevant lines inline in "bert_init()". Since there are no remaining users of "structacpi_bert_region" we delete it from <acpi/actbl1.h>Cc Len Brown <[email protected]>Cc Huang Ying <[email protected]>Cc Borislav Petkov <[email protected]>Cc Tomasz Nowicki <[email protected]>Cc Jonathan (Zhixiong) Zhang <[email protected]>Cc Tyler Baicar <[email protected]>Signed-off-by Tony Luck <[email protected]>--- drivers/acpi/apei/bert.c | 54 +++++++----------------------------------------- include/acpi/actbl1.h | 11 +--------- 2 files changed, 8 insertions(+), 57 deletions(-)
...
include/acpi/actbl1.h struct acpi_table_bert { · line 130
Quoted 7 lines
include/acpi/actbl1.h +1
-6-struct acpi_bert_region { -u32 block_status;/* Type of error information */ -u32 raw_data_offset;/* Offset to raw error data */ -u32 raw_data_length;/* Length of raw error data */ -u32 data_length;/* Length of generic error data */ -u32 error_severity;/* Severity code */ +/* which is a struct acpi_hest_generic_status */
Just this nitpick: please merge this comment with the one above it into a single:
u64 address; /*
* Physical address of the error region which is a Generic
* Error Status Block (struct acpi_hest_generic_status). */
I would like to advocate to use the actual names from the spec too because those are so many and so dumbly named, so that one's head starts spinning pretty quickly, trying to keep 'em apart.
Other than that,
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Luck, Tony
reviewerv2 cover Clean up ACPI Boot Error Record Table code
From: Tony Luck <[email protected]>
ACPI 6.1 spec clarified the structure of the Boot Error Record Table. Linux code was overly complex, and included a redundant structure definition of "acpi_bert_region".
v2 changes:
Rafael: Split the change to <acpi/actbl1.h> into a separate patch so
it can be fed through ACPICA process.
Boris: Better wording for the comment about the "address" field in
the acpi_table_bert structure.
Tony Luck (2):
ACPI / APEI: Boot Error Record Table processing was needlessly
complicated
ACPI / APEI: No remaining users of struct acpi_bert_region
drivers/acpi/apei/bert.c | 54 +++++++----------------------------------------- include/acpi/actbl1.h | 16 +++++--------- 2 files changed, 12 insertions(+), 58 deletions(-)
Signature · 1 line
2.11.0
Luck, Tony
reviewerPATCH v2 1/2 ACPI / APEI: Boot Error Record Table processing was needlessly complicated
From: Tony Luck <[email protected]>
Quoting version 6.1 of the ACPI specification. Section 18.3.1 "Boot Error Source" says:
The Boot Error Region is a range of addressable memory OSPM can access during initialization to determine if an unhandled error condition occurred. System firmware must report this memory range as firmware reserved. The format of the Boot Error Region follow that of an Error Status Block, this is defined in Section 18.3.2.7. The format of the error status block is described by Table 18-342.
This clarifies some points that were obfuscated in earlier versions. E.g. there is no longer a separate table to describe the format of the "Boot Error Region" (which was identical to the "Error Status Block"). Also saying "follow that of *an* Error Status Block" makes it clear that there is just one block (which can still contain multiple "Generic Error Data Entry structures").
The loop inside bert_print_all() is unnecessary (but probably harmless as the "while (remain > sizeof(struct acpi_bert_region))" loop should terminate after we skipped over the first entry.
We can drop the "bert_print_all()" function and just move the four relevant lines inline in "bert_init()".
--- drivers/acpi/apei/bert.c | 54 +++++++----------------------------------------- 1 file changed, 7 insertions(+), 47 deletions(-)
drivers/acpi/apei/bert.c +7 -47
@@ -34,50 +34,6 @@static int bert_disable; -static void __init bert_print_all (struct acpi_bert_region *region, -unsigned int region_len) -{ -struct acpi_hest_generic_status *estatus = - (struct acpi_hest_generic_status *)region; -int remain = region_len; -u32 estatus_len; - -if (!estatus->block_status) -return ; - -while (remain >sizeof (struct acpi_bert_region )) { -if (cper_estatus_check (estatus)) { -pr_err (FW_BUG "Invalid error record.\n" ); -return ; - } - - estatus_len =cper_estatus_len (estatus); -if (remain < estatus_len) { -pr_err (FW_BUG "Truncated status block (length: %u).\n" , - estatus_len); -return ; - } - -pr_info_once ("Error records from previous boot:\n" ); - - cper_estatus_print(KERN_INFO HW_ERR, estatus); - -/* -* Because the boot error source is "one-time polled" type, -* clear Block Status of current Generic Error Status Block, -* once it's printed. -*/ - estatus->block_status =0 ; - - estatus = (void *)estatus + estatus_len; -/* No more error records. */ -if (!estatus->block_status) -return ; - - remain -= estatus_len; - } -} -static int __init setup_bert_disable (char *str) { bert_disable =1 ;
@@ -89,7 +45,7 @@ __setup("bert_disable", setup_bert_disable);static int __init bert_check_table (struct acpi_table_bert *bert_tab) {if (bert_tab->header.length <sizeof (struct acpi_table_bert ) || - bert_tab->region_length <sizeof (struct acpi_bert_region )) + bert_tab->region_length <sizeof (struct acpi_hest_generic_status ))return -EINVAL ;return 0 ;
@@ -98,7 +54,7 @@ static int __init bert_check_table(struct acpi_table_bert *bert_tab)static int __init bert_init (void ) {struct apei_resources bert_resources; -struct acpi_bert_region *boot_error_region; +struct acpi_hest_generic_status *boot_error_region;struct acpi_table_bert *bert_tab;unsigned int region_len;acpi_status status;
@@ -138,7 +94,11 @@ static int __init bert_init(void) goto out_fini; boot_error_region =ioremap_cache (bert_tab->address, region_len);if (boot_error_region) { -bert_print_all (boot_error_region, region_len); +if (boot_error_region->block_status) { +pr_info ("Error records from previous boot:\n" ); + cper_estatus_print(KERN_INFO HW_ERR, boot_error_region); + boot_error_region->block_status =0 ; + }iounmap (boot_error_region); }else { rc = -ENOMEM ;
Signature · 1 line
2.11.0
Luck, Tony
reviewerPATCH v2 2/2 ACPI / APEI: No remaining users of struct acpi_bert_region
In reply to Luck, TonyFrom: Tony Luck <[email protected]>
Version 6.1 of the ACPI specification. Section 18.3.1 "Boot Error Source" clarified that the format of the Boot Error Region is a "Generic Error Status Block". So we don't need separate (but identical) "struct acpi_bert_region" and "struct acpi_hest_generic_status".
Code using acpi_bert_region has been changed to use acpi_hest_generic_status instead. So we can delete the definition now.
--- include/acpi/actbl1.h | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
include/acpi/actbl1.h +5 -11
@@ -126,17 +126,11 @@ struct acpi_whea_header {struct acpi_table_bert {struct acpi_table_header header;/* Common ACPI table header */ u32 region_length;/* Length of the boot error region */ -u64 address;/* Physical address of the error region */ -}; - -/* Boot Error Region (not a subtable, pointed to by Address field above) */ - -struct acpi_bert_region { -u32 block_status;/* Type of error information */ -u32 raw_data_offset;/* Offset to raw error data */ -u32 raw_data_length;/* Length of raw error data */ -u32 data_length;/* Length of generic error data */ -u32 error_severity;/* Severity code */ +u64 address;/* +* Physical address of the error region +* which is a Generic Error Status Block +* (struct acpi_hest_generic_status) +*/ };/* Values for block_status flags above */
Signature · 1 line
2.11.0
Re: [PATCH v2 2/2] ACPI / APEI: No remaining users of struct acpi_bert_region
Quoted 41 lines · Tony
From: Tony Luck <[email protected]>
Version 6.1 of the ACPI specification. Section 18.3.1 "Boot Error Source" clarified that the format of the Boot Error Region is a "Generic Error Status Block". So we don't need separate (but identical) "struct
acpi_bert_region" and "structacpi_hest_generic_status".Code using
acpi_bert_regionhas been changed to useacpi_hest_generic_statusinstead. So we can delete the definition now.Cc Len Brown <[email protected]>Cc Huang Ying <[email protected]>Cc Borislav Petkov <[email protected]>Cc Tomasz Nowicki <[email protected]>Cc Jonathan (Zhixiong) Zhang <[email protected]>Cc Tyler Baicar <[email protected]>Reviewed-by Borislav Petkov <[email protected]>Signed-off-by Tony Luck <[email protected]>--- include/acpi/actbl1.h | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)include/acpi/actbl1.h +0
-9@@ -126,17 +126,11 @@ struct acpi_whea_header {struct acpi_table_bert {struct acpi_table_header header;/* Common ACPI table header */ u32 region_length;/* Length of the boot error region */ -u64 address;/* Physical address of the error region */ -}; - -/* Boot Error Region (not a subtable, pointed to by Address field above) */ - -struct acpi_bert_region { -u32 block_status;/* Type of error information */ -u32 raw_data_offset;/* Offset to raw error data */ -u32 raw_data_length;/* Length of raw error data */
include/acpi/actbl1.h struct acpi_whea_header { · line 136
Quoted 10 lines
include/acpi/actbl1.h +5
-2-u32 data_length;/* Length of generic error data */ -u32 error_severity;/* Severity code */ +u64 address;/* +* Physical address of the error region +* which is a Generic Error Status Block +* (struct acpi_hest_generic_status) +*/ };/* Values for block_status flags above */
Well, again, this is an ACPICA header and by changing it in the kernel alone we make it out of sync with the upstream, which may be a problem for the maintainers thereof when they port changes from the upstream ACPICA to the kernel.
So, (a) do we have to change it and (b) can that be routed through the upstream?
Thanks,
Rafael
Luck, Tony
reviewerRe: [PATCH v2 2/2] ACPI / APEI: No remaining users of struct acpi_bert_region
Quoted 7 lines · Rafael J. Wysocki
Well, again, this is an ACPICA header and by changing it in the kernel alone we make it out of sync with the upstream, which may be a problem for the maintainers thereof when they port changes from the upstream ACPICA to the kernel.
So, (a) do we have to change it and (b) can that be routed through the upstream?
a) No ... we don't have to
b) Yes. What is the process / mailing list to make an ACPICA change?
-Tony
Re: [PATCH v2 2/2] ACPI / APEI: No remaining users of struct acpi_bert_region
Quoted 10 lines · Tony
Quoted 7 lines · Rafael J. Wysocki
Well, again, this is an ACPICA header and by changing it in the kernel alone we make it out of sync with the upstream, which may be a problem for the maintainers thereof when they port changes from the upstream ACPICA to the kernel.
So, (a) do we have to change it and (b) can that be routed through the upstream?
a) No ... we don't have to
OK
Quoted 1 line
b) Yes. What is the process / mailing list to make an ACPICA change?
The relevant information is there in MAINTAINERS.
The process is to post changes to the devel mailing list, as per MAINTAINERS, but it should be sufficient to CC the ACPICA maintainers, which I did, and talk to them. :-)
Thanks,
Rafael
Re: [PATCH v2 0/2] Clean up ACPI Boot Error Record Table code
In reply to Luck, TonyQuoted 21 lines · Tony
From: Tony Luck <[email protected]>
ACPI 6.1 spec clarified the structure of the Boot Error Record Table. Linux code was overly complex, and included a redundant structure definition of "
acpi_bert_region".v2 changes:
Rafael: Split the change to <acpi/actbl1.h> into a separate patch so
it can be fed through ACPICA process.
Boris: Better wording for the comment about the "address" field in
theacpi_table_bertstructure.Tony Luck (2):
ACPI / APEI: Boot Error Record Table processing was needlessly
complicated
ACPI / APEI: No remaining users of structacpi_bert_regiondrivers/acpi/apei/bert.c | 54 +++++++----------------------------------------- include/acpi/actbl1.h | 16 +++++--------- 2 files changed, 12 insertions(+), 58 deletions(-)
Quoted 9 lines
Cc Len Brown <[email protected]>Cc Huang Ying <[email protected]>Cc Borislav Petkov <[email protected]>Cc Tomasz Nowicki <[email protected]>Cc Jonathan (Zhixiong) Zhang <[email protected]>Cc Tyler Baicar <[email protected]>Reviewed-by Borislav Petkov <[email protected]>Signed-off-by Tony Luck <[email protected]>
Hello Tony,
I tested this out and can see everything works properly as long as the BERT record is valid.
Previously, if I made the CPER section size too small then the kernel would just print the Firmware Bug: Invalid error record. Doing the same thing with this patch series I can see an unlimited stream of unknown error sections being printed out (I tested this with my ACPI 6.1/UEFI 2.6 patches which include the non-standard error record support).
[ 11.482831] [Hardware Error]: Error 1, type: recoverable [ 11.482832] [Hardware Error]: section type: unknown, 00000000-0000-0000-0000-000000000000 [ 11.482833] [Hardware Error]: section length: 0x0 [ 11.482834] [Hardware Error]: Error 2, type: recoverable [ 11.482835] [Hardware Error]: section type: unknown, 00000000-0000-0000-0000-000000000000 [ 11.482836] [Hardware Error]: section length: 0x0 [ 11.482837] [Hardware Error]: Error 3, type: recoverable [ 11.482839] [Hardware Error]: section type: unknown, 00000000-0000-0000-0000-000000000000 [ 11.482839] [Hardware Error]: section length: 0x0
...
For that reason, I'd suggest leaving this check in:
if (cper_estatus_check(estatus)) {
pr_err(FW_BUG "Invalid error record.\n");
return;
}Thanks,
Tyler
Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Luck, Tony
reviewerPATCH v3 1/1 ACPI / APEI: Boot Error Record Table processing was needlessly complicated
From: Tony Luck <[email protected]>
Quoting version 6.1 of the ACPI specification. Section 18.3.1 "Boot Error Source" says:
The Boot Error Region is a range of addressable memory OSPM can access during initialization to determine if an unhandled error condition occurred. System firmware must report this memory range as firmware reserved. The format of the Boot Error Region follow that of an Error Status Block, this is defined in Section 18.3.2.7. The format of the error status block is described by Table 18-342.
This clarifies some points that were obfuscated in earlier versions. E.g. there is no longer a separate table to describe the format of the "Boot Error Region" (which was identical to the "Error Status Block"). Also saying "follow that of *an* Error Status Block" makes it clear that there is just one block (which can still contain multiple "Generic Error Data Entry structures").
The loop inside bert_print_all() is unnecessary (but probably harmless as the "while (remain > sizeof(struct acpi_bert_region))" loop should terminate after we skipped over the first entry.
We can drop the "bert_print_all()" function and just move the four relevant lines inline in "bert_init()".
---
v3:
1) Back to just one patch (second part to delete the unused structure definition can fly later via ACPICA tree).
2) Tyler Baicur ran some tests that included non-standard error support and found that with an invalid BERT record the kernel loops forever printing "section type: unknown" messages. He recommended putting back the cper_estatus_check() test that I'd dropped. Here's the net diff since v2:
if (boot_error_region->block_status) {
+ rc = cper_estatus_check(boot_error_region);
+ if (rc) {
+ pr_err(FW_BUG "Invalid error record.\n");
+ iounmap(boot_error_region);
+ return rc;
+ }
pr_info("Error records from previous boot:\n");drivers/acpi/apei/bert.c | 60 +++++++++++------------------------------------- 1 file changed, 13 insertions(+), 47 deletions(-)
drivers/acpi/apei/bert.c +13 -47
@@ -34,50 +34,6 @@static int bert_disable; -static void __init bert_print_all (struct acpi_bert_region *region, -unsigned int region_len) -{ -struct acpi_hest_generic_status *estatus = - (struct acpi_hest_generic_status *)region; -int remain = region_len; -u32 estatus_len; - -if (!estatus->block_status) -return ; - -while (remain >sizeof (struct acpi_bert_region )) { -if (cper_estatus_check (estatus)) { -pr_err (FW_BUG "Invalid error record.\n" ); -return ; - } - - estatus_len =cper_estatus_len (estatus); -if (remain < estatus_len) { -pr_err (FW_BUG "Truncated status block (length: %u).\n" , - estatus_len); -return ; - } - -pr_info_once ("Error records from previous boot:\n" ); - - cper_estatus_print(KERN_INFO HW_ERR, estatus); - -/* -* Because the boot error source is "one-time polled" type, -* clear Block Status of current Generic Error Status Block, -* once it's printed. -*/ - estatus->block_status =0 ; - - estatus = (void *)estatus + estatus_len; -/* No more error records. */ -if (!estatus->block_status) -return ; - - remain -= estatus_len; - } -} -static int __init setup_bert_disable (char *str) { bert_disable =1 ;
@@ -89,7 +45,7 @@ __setup("bert_disable", setup_bert_disable);static int __init bert_check_table (struct acpi_table_bert *bert_tab) {if (bert_tab->header.length <sizeof (struct acpi_table_bert ) || - bert_tab->region_length <sizeof (struct acpi_bert_region )) + bert_tab->region_length <sizeof (struct acpi_hest_generic_status ))return -EINVAL ;return 0 ;
@@ -98,7 +54,7 @@ static int __init bert_check_table(struct acpi_table_bert *bert_tab)static int __init bert_init (void ) {struct apei_resources bert_resources; -struct acpi_bert_region *boot_error_region; +struct acpi_hest_generic_status *boot_error_region;struct acpi_table_bert *bert_tab;unsigned int region_len;acpi_status status;
@@ -138,7 +94,17 @@ static int __init bert_init(void) goto out_fini; boot_error_region =ioremap_cache (bert_tab->address, region_len);if (boot_error_region) { -bert_print_all (boot_error_region, region_len); +if (boot_error_region->block_status) { + rc =cper_estatus_check (boot_error_region); +if (rc) { +pr_err (FW_BUG "Invalid error record.\n" ); +iounmap (boot_error_region); +return rc; + } +pr_info ("Error records from previous boot:\n" ); + cper_estatus_print(KERN_INFO HW_ERR, boot_error_region); + boot_error_region->block_status =0 ; + }iounmap (boot_error_region); }else { rc = -ENOMEM ;
Signature · 1 line
2.11.0
Quoted 26 lines · Tony
From: Tony Luck <[email protected]>
Quoting version 6.1 of the ACPI specification. Section 18.3.1 "Boot Error Source" says:
The Boot Error Region is a range of addressable memory OSPM can access during initialization to determine if an unhandled error condition occurred. System firmware must report this memory range as firmware reserved. The format of the Boot Error Region follow that of an Error Status Block, this is defined in Section 18.3.2.7. The format of the error status block is described by Table 18-342.
This clarifies some points that were obfuscated in earlier versions. E.g. there is no longer a separate table to describe the format of the "Boot Error Region" (which was identical to the "Error Status Block"). Also saying "follow that of *an* Error Status Block" makes it clear that there is just one block (which can still contain multiple "Generic Error Data Entry structures").
The loop inside
bert_print_all()is unnecessary (but probably harmless as the "while (remain >sizeof(struct acpi_bert_region))" loop should terminate after we skipped over the first entry.We can drop the "
bert_print_all()" function and just move the four relevant lines inline in "bert_init()".
Quoted 9 lines
Cc Len Brown <[email protected]>Cc Huang Ying <[email protected]>Cc Borislav Petkov <[email protected]>Cc Tomasz Nowicki <[email protected]>Cc Jonathan (Zhixiong) Zhang <[email protected]>Cc Tyler Baicar <[email protected]>Reviewed-by Borislav Petkov <[email protected]>Signed-off-by Tony Luck <[email protected]>
Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.