After the commit 8fcc4ae6faf8 ("arm64: acpi: Make apei_claim_sea()
synchronise with APEI's irq work") applied, do_sea() return directly
for user-mode if apei_claim_sea() handled any error record. Therefore,
each error record reported by the user-mode SEA must be effectively
processed in APEI GHES driver.
Currently, GHES driver only processes Memory Error Section.(Ignore PCIe
Error Section, as it has nothing to do with SEA). It is not enough.
Because ARM Processor Error could also be used for SEA in some hardware
platforms, such as Kunpeng9xx series. We can't ask them to switch to
use Memory Error Section for two reasons:
1)The server was delivered to customers, and it will introduce
compatibility issue.
2)It make sense to use ARM Processor Error Section. Because either
cache or memory errors could generate SEA when consumed by a processor.
Do memory failure handling for ARM Processor Error Section just like
for Memory Error Section.
Signed-off-by: Xiaofei Tan <[email protected]>
---
Changes since v4:
- 1. Change the patch name from " ACPI / APEI: do memory failure on the
physical address reported by ARM processor error section" to this
more proper one.
- 2. Add a comment in the code to tell why not filter out corrected
error in an uncorrected section.
Changes since v3:
- Print unhandled error following James Morse's advice.
Changes since v2:
- Updated commit log
---
drivers/acpi/apei/ghes.c | 76 +++++++++++++++++++++++++++++++++++++-----------
1 file changed, 59 insertions(+), 17 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index fce7ade..0893968 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -441,28 +441,35 @@ static void ghes_kick_task_work(struct callback_head *head)
gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node, node_len);
}
-static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
- int sev)
+static bool ghes_do_memory_failure(u64 physical_addr, int flags)
{
unsigned long pfn;
- int flags = -1;
- int sec_sev = ghes_severity(gdata->error_severity);
- struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
return false;
- if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
- return false;
-
- pfn = mem_err->physical_addr >> PAGE_SHIFT;
+ pfn = PHYS_PFN(physical_addr);
if (!pfn_valid(pfn)) {
pr_warn_ratelimited(FW_WARN GHES_PFX
"Invalid address in generic error data: %#llx\n",
- mem_err->physical_addr);
+ physical_addr);
return false;
}
+ memory_failure_queue(pfn, flags);
+ return true;
+}
+
+static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
+ int sev)
+{
+ int flags = -1;
+ int sec_sev = ghes_severity(gdata->error_severity);
+ struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
+
+ if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
+ return false;
+
/* iff following two events can be handled properly by now */
if (sec_sev == GHES_SEV_CORRECTED &&
(gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED))
@@ -470,14 +477,51 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
if (sev == GHES_SEV_RECOVERABLE && sec_sev == GHES_SEV_RECOVERABLE)
flags = 0;
- if (flags != -1) {
- memory_failure_queue(pfn, flags);
- return true;
- }
+ if (flags != -1)
+ return ghes_do_memory_failure(mem_err->physical_addr, flags);
return false;
}
+static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev)
+{
+ struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
+ struct cper_arm_err_info *err_info;
+ bool queued = false;
+ int sec_sev, i;
+
+ log_arm_hw_error(err);
+
+ sec_sev = ghes_severity(gdata->error_severity);
+ if (sev != GHES_SEV_RECOVERABLE || sec_sev != GHES_SEV_RECOVERABLE)
+ return false;
+
+ err_info = (struct cper_arm_err_info *) (err + 1);
+ for (i = 0; i < err->err_info_num; i++, err_info++) {
+ bool is_cache = (err_info->type == CPER_ARM_CACHE_ERROR);
+ bool has_pa = (err_info->validation_bits & CPER_ARM_INFO_VALID_PHYSICAL_ADDR);
+
+ /*
+ * The field (err_info->error_info & BIT(26)) is fixed to set to
+ * 1 in some old firmware of HiSilicon Kunpeng920. We assume that
+ * firmware won't mix corrected errors in an uncorrected section,
+ * and don't filter out 'corrected' error here.
+ */
+ if (!is_cache || !has_pa) {
+ pr_warn_ratelimited(FW_WARN GHES_PFX
+ "Unhandled processor error type %s\n",
+ err_info->type < ARRAY_SIZE(cper_proc_error_type_strs) ?
+ cper_proc_error_type_strs[err_info->type] : "unknown error");
+ continue;
+ }
+
+ if (ghes_do_memory_failure(err_info->physical_fault_addr, 0))
+ queued = true;
+ }
+
+ return queued;
+}
+
/*
* 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
@@ -605,9 +649,7 @@ static bool ghes_do_proc(struct ghes *ghes,
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);
+ queued = ghes_handle_arm_hw_error(gdata, sev);
} else {
void *err = acpi_hest_get_payload(gdata);
--
2.8.1
@James
Hi James, please help to review this patch. Thank you very much. :)
On 2020/12/10 20:09, Xiaofei Tan wrote:
> After the commit 8fcc4ae6faf8 ("arm64: acpi: Make apei_claim_sea()
> synchronise with APEI's irq work") applied, do_sea() return directly
> for user-mode if apei_claim_sea() handled any error record. Therefore,
> each error record reported by the user-mode SEA must be effectively
> processed in APEI GHES driver.
>
> Currently, GHES driver only processes Memory Error Section.(Ignore PCIe
> Error Section, as it has nothing to do with SEA). It is not enough.
> Because ARM Processor Error could also be used for SEA in some hardware
> platforms, such as Kunpeng9xx series. We can't ask them to switch to
> use Memory Error Section for two reasons:
> 1)The server was delivered to customers, and it will introduce
> compatibility issue.
> 2)It make sense to use ARM Processor Error Section. Because either
> cache or memory errors could generate SEA when consumed by a processor.
>
> Do memory failure handling for ARM Processor Error Section just like
> for Memory Error Section.
>
> Signed-off-by: Xiaofei Tan <[email protected]>
> ---
> Changes since v4:
> - 1. Change the patch name from " ACPI / APEI: do memory failure on the
> physical address reported by ARM processor error section" to this
> more proper one.
> - 2. Add a comment in the code to tell why not filter out corrected
> error in an uncorrected section.
>
> Changes since v3:
> - Print unhandled error following James Morse's advice.
>
> Changes since v2:
> - Updated commit log
> ---
> drivers/acpi/apei/ghes.c | 76 +++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 59 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index fce7ade..0893968 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -441,28 +441,35 @@ static void ghes_kick_task_work(struct callback_head *head)
> gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node, node_len);
> }
>
> -static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
> - int sev)
> +static bool ghes_do_memory_failure(u64 physical_addr, int flags)
> {
> unsigned long pfn;
> - int flags = -1;
> - int sec_sev = ghes_severity(gdata->error_severity);
> - struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
>
> if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
> return false;
>
> - if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
> - return false;
> -
> - pfn = mem_err->physical_addr >> PAGE_SHIFT;
> + pfn = PHYS_PFN(physical_addr);
> if (!pfn_valid(pfn)) {
> pr_warn_ratelimited(FW_WARN GHES_PFX
> "Invalid address in generic error data: %#llx\n",
> - mem_err->physical_addr);
> + physical_addr);
> return false;
> }
>
> + memory_failure_queue(pfn, flags);
> + return true;
> +}
> +
> +static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
> + int sev)
> +{
> + int flags = -1;
> + int sec_sev = ghes_severity(gdata->error_severity);
> + struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
> +
> + if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
> + return false;
> +
> /* iff following two events can be handled properly by now */
> if (sec_sev == GHES_SEV_CORRECTED &&
> (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED))
> @@ -470,14 +477,51 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
> if (sev == GHES_SEV_RECOVERABLE && sec_sev == GHES_SEV_RECOVERABLE)
> flags = 0;
>
> - if (flags != -1) {
> - memory_failure_queue(pfn, flags);
> - return true;
> - }
> + if (flags != -1)
> + return ghes_do_memory_failure(mem_err->physical_addr, flags);
>
> return false;
> }
>
> +static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev)
> +{
> + struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
> + struct cper_arm_err_info *err_info;
> + bool queued = false;
> + int sec_sev, i;
> +
> + log_arm_hw_error(err);
> +
> + sec_sev = ghes_severity(gdata->error_severity);
> + if (sev != GHES_SEV_RECOVERABLE || sec_sev != GHES_SEV_RECOVERABLE)
> + return false;
> +
> + err_info = (struct cper_arm_err_info *) (err + 1);
> + for (i = 0; i < err->err_info_num; i++, err_info++) {
> + bool is_cache = (err_info->type == CPER_ARM_CACHE_ERROR);
> + bool has_pa = (err_info->validation_bits & CPER_ARM_INFO_VALID_PHYSICAL_ADDR);
> +
> + /*
> + * The field (err_info->error_info & BIT(26)) is fixed to set to
> + * 1 in some old firmware of HiSilicon Kunpeng920. We assume that
> + * firmware won't mix corrected errors in an uncorrected section,
> + * and don't filter out 'corrected' error here.
> + */
> + if (!is_cache || !has_pa) {
> + pr_warn_ratelimited(FW_WARN GHES_PFX
> + "Unhandled processor error type %s\n",
> + err_info->type < ARRAY_SIZE(cper_proc_error_type_strs) ?
> + cper_proc_error_type_strs[err_info->type] : "unknown error");
> + continue;
> + }
> +
> + if (ghes_do_memory_failure(err_info->physical_fault_addr, 0))
> + queued = true;
> + }
> +
> + return queued;
> +}
> +
> /*
> * 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
> @@ -605,9 +649,7 @@ static bool ghes_do_proc(struct ghes *ghes,
> 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);
> + queued = ghes_handle_arm_hw_error(gdata, sev);
> } else {
> void *err = acpi_hest_get_payload(gdata);
>
>
On Tue, Jan 26, 2021 at 2:32 PM tanxiaofei <[email protected]> wrote:
>
> @James
> Hi James, please help to review this patch. Thank you very much. :)
James, Boris, any comments?
> On 2020/12/10 20:09, Xiaofei Tan wrote:
> > After the commit 8fcc4ae6faf8 ("arm64: acpi: Make apei_claim_sea()
> > synchronise with APEI's irq work") applied, do_sea() return directly
> > for user-mode if apei_claim_sea() handled any error record. Therefore,
> > each error record reported by the user-mode SEA must be effectively
> > processed in APEI GHES driver.
> >
> > Currently, GHES driver only processes Memory Error Section.(Ignore PCIe
> > Error Section, as it has nothing to do with SEA). It is not enough.
> > Because ARM Processor Error could also be used for SEA in some hardware
> > platforms, such as Kunpeng9xx series. We can't ask them to switch to
> > use Memory Error Section for two reasons:
> > 1)The server was delivered to customers, and it will introduce
> > compatibility issue.
> > 2)It make sense to use ARM Processor Error Section. Because either
> > cache or memory errors could generate SEA when consumed by a processor.
> >
> > Do memory failure handling for ARM Processor Error Section just like
> > for Memory Error Section.
> >
> > Signed-off-by: Xiaofei Tan <[email protected]>
> > ---
> > Changes since v4:
> > - 1. Change the patch name from " ACPI / APEI: do memory failure on the
> > physical address reported by ARM processor error section" to this
> > more proper one.
> > - 2. Add a comment in the code to tell why not filter out corrected
> > error in an uncorrected section.
> >
> > Changes since v3:
> > - Print unhandled error following James Morse's advice.
> >
> > Changes since v2:
> > - Updated commit log
> > ---
> > drivers/acpi/apei/ghes.c | 76 +++++++++++++++++++++++++++++++++++++-----------
> > 1 file changed, 59 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> > index fce7ade..0893968 100644
> > --- a/drivers/acpi/apei/ghes.c
> > +++ b/drivers/acpi/apei/ghes.c
> > @@ -441,28 +441,35 @@ static void ghes_kick_task_work(struct callback_head *head)
> > gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node, node_len);
> > }
> >
> > -static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
> > - int sev)
> > +static bool ghes_do_memory_failure(u64 physical_addr, int flags)
> > {
> > unsigned long pfn;
> > - int flags = -1;
> > - int sec_sev = ghes_severity(gdata->error_severity);
> > - struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
> >
> > if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
> > return false;
> >
> > - if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
> > - return false;
> > -
> > - pfn = mem_err->physical_addr >> PAGE_SHIFT;
> > + pfn = PHYS_PFN(physical_addr);
> > if (!pfn_valid(pfn)) {
> > pr_warn_ratelimited(FW_WARN GHES_PFX
> > "Invalid address in generic error data: %#llx\n",
> > - mem_err->physical_addr);
> > + physical_addr);
> > return false;
> > }
> >
> > + memory_failure_queue(pfn, flags);
> > + return true;
> > +}
> > +
> > +static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
> > + int sev)
> > +{
> > + int flags = -1;
> > + int sec_sev = ghes_severity(gdata->error_severity);
> > + struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
> > +
> > + if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
> > + return false;
> > +
> > /* iff following two events can be handled properly by now */
> > if (sec_sev == GHES_SEV_CORRECTED &&
> > (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED))
> > @@ -470,14 +477,51 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
> > if (sev == GHES_SEV_RECOVERABLE && sec_sev == GHES_SEV_RECOVERABLE)
> > flags = 0;
> >
> > - if (flags != -1) {
> > - memory_failure_queue(pfn, flags);
> > - return true;
> > - }
> > + if (flags != -1)
> > + return ghes_do_memory_failure(mem_err->physical_addr, flags);
> >
> > return false;
> > }
> >
> > +static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev)
> > +{
> > + struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
> > + struct cper_arm_err_info *err_info;
> > + bool queued = false;
> > + int sec_sev, i;
> > +
> > + log_arm_hw_error(err);
> > +
> > + sec_sev = ghes_severity(gdata->error_severity);
> > + if (sev != GHES_SEV_RECOVERABLE || sec_sev != GHES_SEV_RECOVERABLE)
> > + return false;
> > +
> > + err_info = (struct cper_arm_err_info *) (err + 1);
> > + for (i = 0; i < err->err_info_num; i++, err_info++) {
> > + bool is_cache = (err_info->type == CPER_ARM_CACHE_ERROR);
> > + bool has_pa = (err_info->validation_bits & CPER_ARM_INFO_VALID_PHYSICAL_ADDR);
> > +
> > + /*
> > + * The field (err_info->error_info & BIT(26)) is fixed to set to
> > + * 1 in some old firmware of HiSilicon Kunpeng920. We assume that
> > + * firmware won't mix corrected errors in an uncorrected section,
> > + * and don't filter out 'corrected' error here.
> > + */
> > + if (!is_cache || !has_pa) {
> > + pr_warn_ratelimited(FW_WARN GHES_PFX
> > + "Unhandled processor error type %s\n",
> > + err_info->type < ARRAY_SIZE(cper_proc_error_type_strs) ?
> > + cper_proc_error_type_strs[err_info->type] : "unknown error");
> > + continue;
> > + }
> > +
> > + if (ghes_do_memory_failure(err_info->physical_fault_addr, 0))
> > + queued = true;
> > + }
> > +
> > + return queued;
> > +}
> > +
> > /*
> > * 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
> > @@ -605,9 +649,7 @@ static bool ghes_do_proc(struct ghes *ghes,
> > 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);
> > + queued = ghes_handle_arm_hw_error(gdata, sev);
> > } else {
> > void *err = acpi_hest_get_payload(gdata);
> >
> >
>
Hi James, Boris
a friendly ping..
any comments for this ?
On 2021/2/5 20:55, Rafael J. Wysocki wrote:
> On Tue, Jan 26, 2021 at 2:32 PM tanxiaofei <[email protected]> wrote:
>>
>> @James
>> Hi James, please help to review this patch. Thank you very much. :)
>
> James, Boris, any comments?
>
>> On 2020/12/10 20:09, Xiaofei Tan wrote:
>>> After the commit 8fcc4ae6faf8 ("arm64: acpi: Make apei_claim_sea()
>>> synchronise with APEI's irq work") applied, do_sea() return directly
>>> for user-mode if apei_claim_sea() handled any error record. Therefore,
>>> each error record reported by the user-mode SEA must be effectively
>>> processed in APEI GHES driver.
>>>
>>> Currently, GHES driver only processes Memory Error Section.(Ignore PCIe
>>> Error Section, as it has nothing to do with SEA). It is not enough.
>>> Because ARM Processor Error could also be used for SEA in some hardware
>>> platforms, such as Kunpeng9xx series. We can't ask them to switch to
>>> use Memory Error Section for two reasons:
>>> 1)The server was delivered to customers, and it will introduce
>>> compatibility issue.
>>> 2)It make sense to use ARM Processor Error Section. Because either
>>> cache or memory errors could generate SEA when consumed by a processor.
>>>
>>> Do memory failure handling for ARM Processor Error Section just like
>>> for Memory Error Section.
>>>
>>> Signed-off-by: Xiaofei Tan <[email protected]>
>>> ---
>>> Changes since v4:
>>> - 1. Change the patch name from " ACPI / APEI: do memory failure on the
>>> physical address reported by ARM processor error section" to this
>>> more proper one.
>>> - 2. Add a comment in the code to tell why not filter out corrected
>>> error in an uncorrected section.
>>>
>>> Changes since v3:
>>> - Print unhandled error following James Morse's advice.
>>>
>>> Changes since v2:
>>> - Updated commit log
>>> ---
>>> drivers/acpi/apei/ghes.c | 76 +++++++++++++++++++++++++++++++++++++-----------
>>> 1 file changed, 59 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>>> index fce7ade..0893968 100644
>>> --- a/drivers/acpi/apei/ghes.c
>>> +++ b/drivers/acpi/apei/ghes.c
>>> @@ -441,28 +441,35 @@ static void ghes_kick_task_work(struct callback_head *head)
>>> gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node, node_len);
>>> }
>>>
>>> -static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>>> - int sev)
>>> +static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>>> {
>>> unsigned long pfn;
>>> - int flags = -1;
>>> - int sec_sev = ghes_severity(gdata->error_severity);
>>> - struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
>>>
>>> if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
>>> return false;
>>>
>>> - if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
>>> - return false;
>>> -
>>> - pfn = mem_err->physical_addr >> PAGE_SHIFT;
>>> + pfn = PHYS_PFN(physical_addr);
>>> if (!pfn_valid(pfn)) {
>>> pr_warn_ratelimited(FW_WARN GHES_PFX
>>> "Invalid address in generic error data: %#llx\n",
>>> - mem_err->physical_addr);
>>> + physical_addr);
>>> return false;
>>> }
>>>
>>> + memory_failure_queue(pfn, flags);
>>> + return true;
>>> +}
>>> +
>>> +static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>>> + int sev)
>>> +{
>>> + int flags = -1;
>>> + int sec_sev = ghes_severity(gdata->error_severity);
>>> + struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
>>> +
>>> + if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
>>> + return false;
>>> +
>>> /* iff following two events can be handled properly by now */
>>> if (sec_sev == GHES_SEV_CORRECTED &&
>>> (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED))
>>> @@ -470,14 +477,51 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>>> if (sev == GHES_SEV_RECOVERABLE && sec_sev == GHES_SEV_RECOVERABLE)
>>> flags = 0;
>>>
>>> - if (flags != -1) {
>>> - memory_failure_queue(pfn, flags);
>>> - return true;
>>> - }
>>> + if (flags != -1)
>>> + return ghes_do_memory_failure(mem_err->physical_addr, flags);
>>>
>>> return false;
>>> }
>>>
>>> +static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev)
>>> +{
>>> + struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
>>> + struct cper_arm_err_info *err_info;
>>> + bool queued = false;
>>> + int sec_sev, i;
>>> +
>>> + log_arm_hw_error(err);
>>> +
>>> + sec_sev = ghes_severity(gdata->error_severity);
>>> + if (sev != GHES_SEV_RECOVERABLE || sec_sev != GHES_SEV_RECOVERABLE)
>>> + return false;
>>> +
>>> + err_info = (struct cper_arm_err_info *) (err + 1);
>>> + for (i = 0; i < err->err_info_num; i++, err_info++) {
>>> + bool is_cache = (err_info->type == CPER_ARM_CACHE_ERROR);
>>> + bool has_pa = (err_info->validation_bits & CPER_ARM_INFO_VALID_PHYSICAL_ADDR);
>>> +
>>> + /*
>>> + * The field (err_info->error_info & BIT(26)) is fixed to set to
>>> + * 1 in some old firmware of HiSilicon Kunpeng920. We assume that
>>> + * firmware won't mix corrected errors in an uncorrected section,
>>> + * and don't filter out 'corrected' error here.
>>> + */
>>> + if (!is_cache || !has_pa) {
>>> + pr_warn_ratelimited(FW_WARN GHES_PFX
>>> + "Unhandled processor error type %s\n",
>>> + err_info->type < ARRAY_SIZE(cper_proc_error_type_strs) ?
>>> + cper_proc_error_type_strs[err_info->type] : "unknown error");
>>> + continue;
>>> + }
>>> +
>>> + if (ghes_do_memory_failure(err_info->physical_fault_addr, 0))
>>> + queued = true;
>>> + }
>>> +
>>> + return queued;
>>> +}
>>> +
>>> /*
>>> * 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
>>> @@ -605,9 +649,7 @@ static bool ghes_do_proc(struct ghes *ghes,
>>> 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);
>>> + queued = ghes_handle_arm_hw_error(gdata, sev);
>>> } else {
>>> void *err = acpi_hest_get_payload(gdata);
>>>
>>>
>>
>
> .
>
ping..
On 2020/12/10 20:09, Xiaofei Tan wrote:
> After the commit 8fcc4ae6faf8 ("arm64: acpi: Make apei_claim_sea()
> synchronise with APEI's irq work") applied, do_sea() return directly
> for user-mode if apei_claim_sea() handled any error record. Therefore,
> each error record reported by the user-mode SEA must be effectively
> processed in APEI GHES driver.
>
> Currently, GHES driver only processes Memory Error Section.(Ignore PCIe
> Error Section, as it has nothing to do with SEA). It is not enough.
> Because ARM Processor Error could also be used for SEA in some hardware
> platforms, such as Kunpeng9xx series. We can't ask them to switch to
> use Memory Error Section for two reasons:
> 1)The server was delivered to customers, and it will introduce
> compatibility issue.
> 2)It make sense to use ARM Processor Error Section. Because either
> cache or memory errors could generate SEA when consumed by a processor.
>
> Do memory failure handling for ARM Processor Error Section just like
> for Memory Error Section.
>
> Signed-off-by: Xiaofei Tan <[email protected]>
> ---
> Changes since v4:
> - 1. Change the patch name from " ACPI / APEI: do memory failure on the
> physical address reported by ARM processor error section" to this
> more proper one.
> - 2. Add a comment in the code to tell why not filter out corrected
> error in an uncorrected section.
>
> Changes since v3:
> - Print unhandled error following James Morse's advice.
>
> Changes since v2:
> - Updated commit log
> ---
> drivers/acpi/apei/ghes.c | 76 +++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 59 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index fce7ade..0893968 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -441,28 +441,35 @@ static void ghes_kick_task_work(struct callback_head *head)
> gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node, node_len);
> }
>
> -static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
> - int sev)
> +static bool ghes_do_memory_failure(u64 physical_addr, int flags)
> {
> unsigned long pfn;
> - int flags = -1;
> - int sec_sev = ghes_severity(gdata->error_severity);
> - struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
>
> if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
> return false;
>
> - if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
> - return false;
> -
> - pfn = mem_err->physical_addr >> PAGE_SHIFT;
> + pfn = PHYS_PFN(physical_addr);
> if (!pfn_valid(pfn)) {
> pr_warn_ratelimited(FW_WARN GHES_PFX
> "Invalid address in generic error data: %#llx\n",
> - mem_err->physical_addr);
> + physical_addr);
> return false;
> }
>
> + memory_failure_queue(pfn, flags);
> + return true;
> +}
> +
> +static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
> + int sev)
> +{
> + int flags = -1;
> + int sec_sev = ghes_severity(gdata->error_severity);
> + struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
> +
> + if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
> + return false;
> +
> /* iff following two events can be handled properly by now */
> if (sec_sev == GHES_SEV_CORRECTED &&
> (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED))
> @@ -470,14 +477,51 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
> if (sev == GHES_SEV_RECOVERABLE && sec_sev == GHES_SEV_RECOVERABLE)
> flags = 0;
>
> - if (flags != -1) {
> - memory_failure_queue(pfn, flags);
> - return true;
> - }
> + if (flags != -1)
> + return ghes_do_memory_failure(mem_err->physical_addr, flags);
>
> return false;
> }
>
> +static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev)
> +{
> + struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
> + struct cper_arm_err_info *err_info;
> + bool queued = false;
> + int sec_sev, i;
> +
> + log_arm_hw_error(err);
> +
> + sec_sev = ghes_severity(gdata->error_severity);
> + if (sev != GHES_SEV_RECOVERABLE || sec_sev != GHES_SEV_RECOVERABLE)
> + return false;
> +
> + err_info = (struct cper_arm_err_info *) (err + 1);
> + for (i = 0; i < err->err_info_num; i++, err_info++) {
> + bool is_cache = (err_info->type == CPER_ARM_CACHE_ERROR);
> + bool has_pa = (err_info->validation_bits & CPER_ARM_INFO_VALID_PHYSICAL_ADDR);
> +
> + /*
> + * The field (err_info->error_info & BIT(26)) is fixed to set to
> + * 1 in some old firmware of HiSilicon Kunpeng920. We assume that
> + * firmware won't mix corrected errors in an uncorrected section,
> + * and don't filter out 'corrected' error here.
> + */
> + if (!is_cache || !has_pa) {
> + pr_warn_ratelimited(FW_WARN GHES_PFX
> + "Unhandled processor error type %s\n",
> + err_info->type < ARRAY_SIZE(cper_proc_error_type_strs) ?
> + cper_proc_error_type_strs[err_info->type] : "unknown error");
> + continue;
> + }
> +
> + if (ghes_do_memory_failure(err_info->physical_fault_addr, 0))
> + queued = true;
> + }
> +
> + return queued;
> +}
> +
> /*
> * 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
> @@ -605,9 +649,7 @@ static bool ghes_do_proc(struct ghes *ghes,
> 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);
> + queued = ghes_handle_arm_hw_error(gdata, sev);
> } else {
> void *err = acpi_hest_get_payload(gdata);
>
>
ping..
On 2020/12/10 20:09, Xiaofei Tan wrote:
> After the commit 8fcc4ae6faf8 ("arm64: acpi: Make apei_claim_sea()
> synchronise with APEI's irq work") applied, do_sea() return directly
> for user-mode if apei_claim_sea() handled any error record. Therefore,
> each error record reported by the user-mode SEA must be effectively
> processed in APEI GHES driver.
>
> Currently, GHES driver only processes Memory Error Section.(Ignore PCIe
> Error Section, as it has nothing to do with SEA). It is not enough.
> Because ARM Processor Error could also be used for SEA in some hardware
> platforms, such as Kunpeng9xx series. We can't ask them to switch to
> use Memory Error Section for two reasons:
> 1)The server was delivered to customers, and it will introduce
> compatibility issue.
> 2)It make sense to use ARM Processor Error Section. Because either
> cache or memory errors could generate SEA when consumed by a processor.
>
> Do memory failure handling for ARM Processor Error Section just like
> for Memory Error Section.
>
> Signed-off-by: Xiaofei Tan <[email protected]>
> ---
> Changes since v4:
> - 1. Change the patch name from " ACPI / APEI: do memory failure on the
> physical address reported by ARM processor error section" to this
> more proper one.
> - 2. Add a comment in the code to tell why not filter out corrected
> error in an uncorrected section.
>
> Changes since v3:
> - Print unhandled error following James Morse's advice.
>
> Changes since v2:
> - Updated commit log
> ---
> drivers/acpi/apei/ghes.c | 76 +++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 59 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index fce7ade..0893968 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -441,28 +441,35 @@ static void ghes_kick_task_work(struct callback_head *head)
> gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node, node_len);
> }
>
> -static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
> - int sev)
> +static bool ghes_do_memory_failure(u64 physical_addr, int flags)
> {
> unsigned long pfn;
> - int flags = -1;
> - int sec_sev = ghes_severity(gdata->error_severity);
> - struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
>
> if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
> return false;
>
> - if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
> - return false;
> -
> - pfn = mem_err->physical_addr >> PAGE_SHIFT;
> + pfn = PHYS_PFN(physical_addr);
> if (!pfn_valid(pfn)) {
> pr_warn_ratelimited(FW_WARN GHES_PFX
> "Invalid address in generic error data: %#llx\n",
> - mem_err->physical_addr);
> + physical_addr);
> return false;
> }
>
> + memory_failure_queue(pfn, flags);
> + return true;
> +}
> +
> +static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
> + int sev)
> +{
> + int flags = -1;
> + int sec_sev = ghes_severity(gdata->error_severity);
> + struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
> +
> + if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
> + return false;
> +
> /* iff following two events can be handled properly by now */
> if (sec_sev == GHES_SEV_CORRECTED &&
> (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED))
> @@ -470,14 +477,51 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
> if (sev == GHES_SEV_RECOVERABLE && sec_sev == GHES_SEV_RECOVERABLE)
> flags = 0;
>
> - if (flags != -1) {
> - memory_failure_queue(pfn, flags);
> - return true;
> - }
> + if (flags != -1)
> + return ghes_do_memory_failure(mem_err->physical_addr, flags);
>
> return false;
> }
>
> +static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev)
> +{
> + struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
> + struct cper_arm_err_info *err_info;
> + bool queued = false;
> + int sec_sev, i;
> +
> + log_arm_hw_error(err);
> +
> + sec_sev = ghes_severity(gdata->error_severity);
> + if (sev != GHES_SEV_RECOVERABLE || sec_sev != GHES_SEV_RECOVERABLE)
> + return false;
> +
> + err_info = (struct cper_arm_err_info *) (err + 1);
> + for (i = 0; i < err->err_info_num; i++, err_info++) {
> + bool is_cache = (err_info->type == CPER_ARM_CACHE_ERROR);
> + bool has_pa = (err_info->validation_bits & CPER_ARM_INFO_VALID_PHYSICAL_ADDR);
> +
> + /*
> + * The field (err_info->error_info & BIT(26)) is fixed to set to
> + * 1 in some old firmware of HiSilicon Kunpeng920. We assume that
> + * firmware won't mix corrected errors in an uncorrected section,
> + * and don't filter out 'corrected' error here.
> + */
> + if (!is_cache || !has_pa) {
> + pr_warn_ratelimited(FW_WARN GHES_PFX
> + "Unhandled processor error type %s\n",
> + err_info->type < ARRAY_SIZE(cper_proc_error_type_strs) ?
> + cper_proc_error_type_strs[err_info->type] : "unknown error");
> + continue;
> + }
> +
> + if (ghes_do_memory_failure(err_info->physical_fault_addr, 0))
> + queued = true;
> + }
> +
> + return queued;
> +}
> +
> /*
> * 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
> @@ -605,9 +649,7 @@ static bool ghes_do_proc(struct ghes *ghes,
> 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);
> + queued = ghes_handle_arm_hw_error(gdata, sev);
> } else {
> void *err = acpi_hest_get_payload(gdata);
>
>
Hi Xiaofei Tan,
Sorry for the delayed response,
this still applies and builds to v5.13-rc4.
On 10/12/2020 12:09, Xiaofei Tan wrote:
> After the commit 8fcc4ae6faf8 ("arm64: acpi: Make apei_claim_sea()
> synchronise with APEI's irq work") applied, do_sea() return directly
> for user-mode if apei_claim_sea() handled any error record. Therefore,
> each error record reported by the user-mode SEA must be effectively
> processed in APEI GHES driver.
If you describe it the other way round, it would be clearer what the problem here is.
Something like:
| Before commit 8fcc4ae6faf8 ("arm64: acpi: Make apei_claim_sea() synchronise
| with APEI's irq work"), do_sea() would unconditionally signal the affected task
| from the arch code. Since that change, the GHES driver sends the signals,.
| This exposes a problem as errors the GHES driver doesn't understand are silently
| ignored.
> Currently, GHES driver only processes Memory Error Section.(Ignore PCIe
> Error Section, as it has nothing to do with SEA).
(you're starting to confuse me! - I went and checked before I realised you were talking to
me, not describing the code...)
> It is not enough. > Because ARM Processor Error could also be used for SEA in some hardware
> platforms, such as Kunpeng9xx series. We can't ask them to switch to
> use Memory Error Section for two reasons:
> 1)The server was delivered to customers, and it will introduce
> compatibility issue.
> 2)It make sense to use ARM Processor Error Section. Because either
> cache or memory errors could generate SEA when consumed by a processor.
I think you just need to say:
| Existing firmware on Kunpeng9xx systems reports cache errors with the 'ARM Processor
| Error' CPER records.
Could you add something about why the silent-ignore is a problem? Do the errors get taken
again? Does user-space get stuck in this loop?
> Do memory failure handling for ARM Processor Error Section just like
> for Memory Error Section.
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index fce7ade..0893968 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> +static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev)
> +{
> + struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
> + struct cper_arm_err_info *err_info;
> + bool queued = false;
> + int sec_sev, i;
> +
> + log_arm_hw_error(err);
> +
> + sec_sev = ghes_severity(gdata->error_severity);
> + if (sev != GHES_SEV_RECOVERABLE || sec_sev != GHES_SEV_RECOVERABLE)
> + return false;
> +
> + err_info = (struct cper_arm_err_info *) (err + 1);
> + for (i = 0; i < err->err_info_num; i++, err_info++) {
err_info has a version and a length, so its expected to be made bigger at some point.
It would be better to use the length instead of 'err_info++', or at least to break out of
the loop if a length > sizeof(*err_info) is seen.
With that:
Reviewed-by: James Morse <[email protected]>
The following nits would make this easier to read:
> + bool is_cache = (err_info->type == CPER_ARM_CACHE_ERROR);
> + bool has_pa = (err_info->validation_bits & CPER_ARM_INFO_VALID_PHYSICAL_ADDR);
> + /*
> + * The field (err_info->error_info & BIT(26)) is fixed to set to
> + * 1 in some old firmware of HiSilicon Kunpeng920. We assume that
> + * firmware won't mix corrected errors in an uncorrected section,
> + * and don't filter out 'corrected' error here.
> + */
(Nothing reads err_info->error_info, I guess this is a warning to the next person to touch
this)
> + if (!is_cache || !has_pa) {
> + pr_warn_ratelimited(FW_WARN GHES_PFX
> + "Unhandled processor error type %s\n",
> + err_info->type < ARRAY_SIZE(cper_proc_error_type_strs) ?
> + cper_proc_error_type_strs[err_info->type] : "unknown error");
> + continue;
This is hard to read. The convention is to indent the extra lines to the relevant '('.
e.g.:
| pr_warn_ratelimited(FW_WARN GHES_PFX
| "Unhandled processor error type %s\n",
You could make it shorter by working out the error_type string earlier
e.g.:
| char *error_type = "unknown_error";
|
| if (err_info->type < ARRAY_SIZE(cper_proc_error_type_strs)
| error_type = cper_proc_error_type_strs[err_info->type];
> + }
> + if (ghes_do_memory_failure(err_info->physical_fault_addr, 0))
> + queued = true;
| if (it_returned_true())
| queued = true;
Looks funny, and if you moved this earlier, your pr_warn_ratelimted() would have an extra
level of indentation to play with.
i.e.:
| if (is_cache && has_pa) {
| queued = ghes_do_memory_failure(err_info->physical_fault_addr, 0);
| continue;
| }
Thanks,
James
Hi James,
On 2021/6/4 22:19, James Morse wrote:
> Hi Xiaofei Tan,
>
> Sorry for the delayed response,
It's okay.
> this still applies and builds to v5.13-rc4.
Yes, that's why i hadn't resend it.
>
> On 10/12/2020 12:09, Xiaofei Tan wrote:
>> After the commit 8fcc4ae6faf8 ("arm64: acpi: Make apei_claim_sea()
>> synchronise with APEI's irq work") applied, do_sea() return directly
>> for user-mode if apei_claim_sea() handled any error record. Therefore,
>> each error record reported by the user-mode SEA must be effectively
>> processed in APEI GHES driver.
>
> If you describe it the other way round, it would be clearer what the problem here is.
> Something like:
> | Before commit 8fcc4ae6faf8 ("arm64: acpi: Make apei_claim_sea() synchronise
> | with APEI's irq work"), do_sea() would unconditionally signal the affected task
> | from the arch code. Since that change, the GHES driver sends the signals,.
> | This exposes a problem as errors the GHES driver doesn't understand are silently
> | ignored.
>
Yes, it's clearer. Only one little change to the last sentence.
Change to:
This exposes a problem as errors the GHES driver doesn't understand
or doesn't handle effectively are silently ignored.
>
>> Currently, GHES driver only processes Memory Error Section.(Ignore PCIe
>> Error Section, as it has nothing to do with SEA).
>
> (you're starting to confuse me! - I went and checked before I realised you were talking to
> me, not describing the code...)
>
>> It is not enough. > Because ARM Processor Error could also be used for SEA in some hardware
>> platforms, such as Kunpeng9xx series. We can't ask them to switch to
>> use Memory Error Section for two reasons:
>> 1)The server was delivered to customers, and it will introduce
>> compatibility issue.
>> 2)It make sense to use ARM Processor Error Section. Because either
>> cache or memory errors could generate SEA when consumed by a processor.
>
> I think you just need to say:
> | Existing firmware on Kunpeng9xx systems reports cache errors with the 'ARM Processor
> | Error' CPER records.
>
OK
>
> Could you add something about why the silent-ignore is a problem?
OK
> Do the errors get taken again?Does user-space get stuck in this loop?
Yes, the process will repeat endlessly.
>
>
>> Do memory failure handling for ARM Processor Error Section just like
>> for Memory Error Section.
>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index fce7ade..0893968 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>
>> +static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev)
>> +{
>> + struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
>> + struct cper_arm_err_info *err_info;
>> + bool queued = false;
>> + int sec_sev, i;
>> +
>> + log_arm_hw_error(err);
>> +
>> + sec_sev = ghes_severity(gdata->error_severity);
>> + if (sev != GHES_SEV_RECOVERABLE || sec_sev != GHES_SEV_RECOVERABLE)
>> + return false;
>> +
>> + err_info = (struct cper_arm_err_info *) (err + 1);
>> + for (i = 0; i < err->err_info_num; i++, err_info++) {
>
> err_info has a version and a length, so its expected to be made bigger at some point.
Yes, but the table "ARM Processor Error Section" fixed the length of
processor error information structure to fixed 32 bytes. Then the
description of the UEFI spec need to update.
> It would be better to use the length instead of 'err_info++', or at least to break out of
> the loop if a length > sizeof(*err_info) is seen.
>
OK. Maybe check length != sizeof(*err_info) is better. I will add this.
if (err_info->length != sizeof(struct cper_arm_err_info)) {
pr_warn_ratelimited(FW_WARN GHES_PFX
"Error info length %d is invalid\n",
err_info->length);
break;
}
> With that:
> Reviewed-by: James Morse <[email protected]>
>
>
> The following nits would make this easier to read:
>
>> + bool is_cache = (err_info->type == CPER_ARM_CACHE_ERROR);
>> + bool has_pa = (err_info->validation_bits & CPER_ARM_INFO_VALID_PHYSICAL_ADDR);
>
>> + /*
>> + * The field (err_info->error_info & BIT(26)) is fixed to set to
>> + * 1 in some old firmware of HiSilicon Kunpeng920. We assume that
>> + * firmware won't mix corrected errors in an uncorrected section,
>> + * and don't filter out 'corrected' error here.
>> + */
> (Nothing reads err_info->error_info, I guess this is a warning to the next person to touch
> this)
>
Yes
>
>> + if (!is_cache || !has_pa) {
>> + pr_warn_ratelimited(FW_WARN GHES_PFX
>> + "Unhandled processor error type %s\n",
>> + err_info->type < ARRAY_SIZE(cper_proc_error_type_strs) ?
>> + cper_proc_error_type_strs[err_info->type] : "unknown error");
>> + continue;
>
> This is hard to read. The convention is to indent the extra lines to the relevant '('.
> e.g.:
> | pr_warn_ratelimited(FW_WARN GHES_PFX
> | "Unhandled processor error type %s\n",
>
> You could make it shorter by working out the error_type string earlier
> e.g.:
> | char *error_type = "unknown_error";
> |
> | if (err_info->type < ARRAY_SIZE(cper_proc_error_type_strs)
> | error_type = cper_proc_error_type_strs[err_info->type];
>
OK, I will do this change.
>
>> + }
>
>> + if (ghes_do_memory_failure(err_info->physical_fault_addr, 0))
>> + queued = true;
>
> | if (it_returned_true())
> | queued = true;
>
> Looks funny, and if you moved this earlier, your pr_warn_ratelimted() would have an extra
> level of indentation to play with.
> i.e.:
> | if (is_cache && has_pa) {
> | queued = ghes_do_memory_failure(err_info->physical_fault_addr, 0);
> | continue;
> | }
>
Right.
>
> Thanks,
>
> James
>
> .
>
Hi James,
On 2021/6/5 16:59, Xiaofei Tan wrote:
> Hi James,
>
> On 2021/6/4 22:19, James Morse wrote:
...
>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>>> index fce7ade..0893968 100644
>>> --- a/drivers/acpi/apei/ghes.c
>>> +++ b/drivers/acpi/apei/ghes.c
>>
>>> +static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev)
>>> +{
>>> + struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
>>> + struct cper_arm_err_info *err_info;
>>> + bool queued = false;
>>> + int sec_sev, i;
>>> +
>>> + log_arm_hw_error(err);
>>> +
>>> + sec_sev = ghes_severity(gdata->error_severity);
>>> + if (sev != GHES_SEV_RECOVERABLE || sec_sev != GHES_SEV_RECOVERABLE)
>>> + return false;
>>> +
>>> + err_info = (struct cper_arm_err_info *) (err + 1);
>>> + for (i = 0; i < err->err_info_num; i++, err_info++) {
>>
>> err_info has a version and a length, so its expected to be made bigger at some point.
>
>
> Yes, but the table "ARM Processor Error Section" fixed the length of
> processor error information structure to fixed 32 bytes. Then the
> description of the UEFI spec need to update.
>
>> It would be better to use the length instead of 'err_info++', or at least to break out of
>> the loop if a length > sizeof(*err_info) is seen.
>>
>
> OK. Maybe check length != sizeof(*err_info) is better. I will add this.
>
> if (err_info->length != sizeof(struct cper_arm_err_info)) {
> pr_warn_ratelimited(FW_WARN GHES_PFX
> "Error info length %d is invalid\n",
> err_info->length);
> break;
> }
>
I considered this more carefully. It should be better to use the length instead of 'err_info', than
just break out. Because if we want to expand "ARM Processor Error Infomation Structure", the proper way
is just add some new member at the end of the structure. Then we have a chance to work well for the situation
of new firmware + old kernel.
>> With that:
>> Reviewed-by: James Morse <[email protected]>
>>
>>
>> The following nits would make this easier to read:
>>
>>
>
> Right.
>
>>
>> Thanks,
>>
>> James
>>
>> .
>>
>
>
> .
>