[PATCH v3 0/4] arm64: improve handle synchronous External Data Abort

[PATCH v3 0/4] arm64: improve handle synchronous External Data Abort

[Xie XiuQi]

This series fix some issue for arm64 synchronous External Data Abort.

1. fix unhandled processor error
According to the RAS documentation, if we cannot determine the impact
of the error based on the details of the error when an SEA occurs, the
process cannot safely continue to run. Therefore, for unhandled error,
we should signal the system and terminate the process immediately.

2. improve for handling memory errors

If error happened in current execution context, we need pass
MF_ACTION_REQUIRED flag to memory_failure(), and if memory_failure()
recovery failed, we must handle this case, other than ignore it.

---
v3: add improve for handing memory errors
v2: fix compile warning reported by kernel test robot.

Xie XiuQi (4):
  ACPI: APEI: include missing acpi/apei.h
  arm64: ghes: fix error unhandling in synchronous External Data Abort
  arm64: ghes: handle the case when memory_failure recovery failed
  arm64: ghes: pass MF_ACTION_REQUIRED to memory_failure when sea

 arch/arm64/kernel/acpi.c      |  6 ++++++
 drivers/acpi/apei/apei-base.c |  5 +++++
 drivers/acpi/apei/ghes.c      | 31 ++++++++++++++++++++++++-------
 include/acpi/apei.h           |  1 +
 include/linux/mm.h            |  2 +-
 mm/memory-failure.c           | 24 +++++++++++++++++-------
 6 files changed, 54 insertions(+), 15 deletions(-)

-- 
2.20.1

[PATCH v3 1/4] ACPI: APEI: include missing acpi/apei.h

[Xie XiuQi]

kernel test robot reported this warning with 'make W=1':

drivers/acpi/apei/apei-base.c:763:12: warning: no previous prototype for 'arch_apei_enable_cmcff' [-Wmissing-prototypes]
  763 | int __weak arch_apei_enable_cmcff(struct acpi_hest_header *hest_hdr,
      |            ^~~~~~~~~~~~~~~~~~~~~~
drivers/acpi/apei/apei-base.c:770:13: warning: no previous prototype for 'arch_apei_report_mem_error' [-Wmissing-prototypes]
  770 | void __weak arch_apei_report_mem_error(int sev,
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~

Include missing acpi/apei.h to avoid this warning.

Reported-by: kernel test robot <lkp@intel.com>
Fixes: 9dae3d0d9e64 ("apei, mce: Factor out APEI architecture specific MCE calls")
Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
---
 drivers/acpi/apei/apei-base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 9b52482b4ed5..02196a312dc5 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -28,6 +28,7 @@
 #include <linux/rculist.h>
 #include <linux/interrupt.h>
 #include <linux/debugfs.h>
+#include <acpi/apei.h>
 #include <asm/unaligned.h>
 
 #include "apei-internal.h"
-- 
2.20.1

[PATCH v3 2/4] arm64: ghes: fix error unhandling in synchronous External Data Abort

[Xie XiuQi]

According to the RAS documentation, if we cannot determine the impact
of the error based on the details of the error when an SEA occurs, the
process cannot safely continue to run. Therefore, for unhandled error,
we should signal the system and terminate the process immediately.

2.2 Generating error exceptions:
  "An error exception is generated when a detected error is signaled
  to the PE as an in-band error response to an architecturally-executed
  memory access or cache maintenance operation. This includes any explicit
  data access, instruction fetch, translation table walk, or hardware
  update to the translation tables made by an architecturally-executed
  instruction." [1]

2.3 Taking error exceptions:
  Software is only able to successfully recover execution and make progress
  from a restart address for the exception by executing an Exception Return
  instruction to branch to the instruction at this restart address if all
  of the following are true: [2]
    - The error has not been silently propagated by the PE.
    - At the point when the Exception Return instruction is executed, the
      PE state and memory system state are consistent with the PE having
      executed all of the instructions up to but not including the
      instruction at the restart address, and none afterwards. That is,
      at least one of the following restart conditions is true:
        - The error has been not architecturally consumed by the PE
          andinfected the PE state.
        - Executing the instruction at the restart address will not consume
          the error and will correct any corrupt state by overwriting it
          with the correct value or values

After commit 8fcc4ae6faf8 ("arm64: acpi: Make apei_claim_sea() synchronise
with APEI's irq work"), we deferred de SEA process to irq_work.
For example, an memory reading error without valid pa, the process isn't
been terminated. It is not safe.

commit ccb5ecdc2dd ("ACPI: APEI: fix synchronous external aborts in user-mode")
fix the cache errors, but the tlb or uarch errors also have
problems.

In this patch, a SIGBUS is force signaled to fix this case.

Note:
RAS documentation: https://developer.arm.com/documentation/ddi0587/latest

Fixes: 8fcc4ae6faf8 ("arm64: acpi: Make apei_claim_sea() synchronise with APEI's irq work")
Fixes: ccb5ecdc2dde ("ACPI: APEI: fix synchronous external aborts in user-mode")
Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
---
 arch/arm64/kernel/acpi.c      |  6 ++++++
 drivers/acpi/apei/apei-base.c |  4 ++++
 drivers/acpi/apei/ghes.c      | 14 +++++++++++---
 include/acpi/apei.h           |  1 +
 4 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index a5a256e3f9fe..75fc16a68dc3 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -32,6 +32,7 @@
 #include <asm/cpu_ops.h>
 #include <asm/daifflags.h>
 #include <asm/smp_plat.h>
+#include <asm/traps.h>
 
 int acpi_noirq = 1;		/* skip ACPI IRQ initialization */
 int acpi_disabled = 1;
@@ -407,6 +408,11 @@ int apei_claim_sea(struct pt_regs *regs)
 	return err;
 }
 
+void arch_apei_do_recovery_failed(void)
+{
+	arm64_force_sig_mceerr(BUS_MCEERR_AR, 0, 0, "Unhandled processor error");
+}
+
 void arch_reserve_mem_area(acpi_physical_address addr, size_t size)
 {
 	memblock_mark_nomap(addr, size);
diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 02196a312dc5..784fe75258d9 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -774,6 +774,10 @@ void __weak arch_apei_report_mem_error(int sev,
 }
 EXPORT_SYMBOL_GPL(arch_apei_report_mem_error);
 
+void __weak arch_apei_do_recovery_failed(void)
+{
+}
+
 int apei_osc_setup(void)
 {
 	static u8 whea_uuid_str[] = "ed855e0c-6c90-47bf-a62a-26de0fc5ad5c";
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 9952f3a792ba..ba0631c54c52 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -48,6 +48,7 @@
 #include <asm/fixmap.h>
 #include <asm/tlbflush.h>
 #include <ras/ras_event.h>
+#include <asm/traps.h>
 
 #include "apei-internal.h"
 
@@ -483,11 +484,12 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
 	return false;
 }
 
-static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev)
+static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
+				     int sev, int notify_type)
 {
 	struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
 	bool queued = false;
-	int sec_sev, i;
+	int sec_sev, i, unhandled_errs = 0;
 	char *p;
 
 	log_arm_hw_error(err);
@@ -521,9 +523,14 @@ static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int s
 		pr_warn_ratelimited(FW_WARN GHES_PFX
 				    "Unhandled processor error type: %s\n",
 				    error_type);
+		unhandled_errs++;
+
 		p += err_info->length;
 	}
 
+	if (unhandled_errs && notify_type == ACPI_HEST_NOTIFY_SEA)
+		arch_apei_do_recovery_failed();
+
 	return queued;
 }
 
@@ -631,6 +638,7 @@ static bool ghes_do_proc(struct ghes *ghes,
 	const guid_t *fru_id = &guid_null;
 	char *fru_text = "";
 	bool queued = false;
+	int notify_type = ghes->generic->notify.type;
 
 	sev = ghes_severity(estatus->error_severity);
 	apei_estatus_for_each_section(estatus, gdata) {
@@ -654,7 +662,7 @@ static bool ghes_do_proc(struct ghes *ghes,
 			ghes_handle_aer(gdata);
 		}
 		else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
-			queued = ghes_handle_arm_hw_error(gdata, sev);
+			queued = ghes_handle_arm_hw_error(gdata, sev, notify_type);
 		} else {
 			void *err = acpi_hest_get_payload(gdata);
 
diff --git a/include/acpi/apei.h b/include/acpi/apei.h
index dc60f7db5524..136be5534581 100644
--- a/include/acpi/apei.h
+++ b/include/acpi/apei.h
@@ -52,6 +52,7 @@ int erst_clear(u64 record_id);
 
 int arch_apei_enable_cmcff(struct acpi_hest_header *hest_hdr, void *data);
 void arch_apei_report_mem_error(int sev, struct cper_sec_mem_err *mem_err);
+void arch_apei_do_recovery_failed(void);
 
 #endif
 #endif
-- 
2.20.1

[PATCH v3 3/4] arm64: ghes: handle the case when memory_failure recovery failed

[Xie XiuQi]

memory_failure() may not always recovery successfully. In synchronous 
external data abort case, if memory_failure() recovery failed, we must handle it.

In this case, if the recovery fails, the common helper function
arch_apei_do_recovery_failed() is invoked. For arm64 platform, we just
send a SIGBUS.

Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
---
 drivers/acpi/apei/ghes.c |  3 ++-
 include/linux/mm.h       |  2 +-
 mm/memory-failure.c      | 24 +++++++++++++++++-------
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index ba0631c54c52..ddc4da603215 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -435,7 +435,8 @@ static void ghes_kick_task_work(struct callback_head *head)
 
 	estatus_node = container_of(head, struct ghes_estatus_node, task_work);
 	if (IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
-		memory_failure_queue_kick(estatus_node->task_work_cpu);
+		if (memory_failure_queue_kick(estatus_node->task_work_cpu))
+			arch_apei_do_recovery_failed();
 
 	estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
 	node_len = GHES_ESTATUS_NODE_LEN(cper_estatus_len(estatus));
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 974ccca609d2..126d1395c208 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3290,7 +3290,7 @@ int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
 		      unsigned long count, int mf_flags);
 extern int memory_failure(unsigned long pfn, int flags);
 extern void memory_failure_queue(unsigned long pfn, int flags);
-extern void memory_failure_queue_kick(int cpu);
+extern int memory_failure_queue_kick(int cpu);
 extern int unpoison_memory(unsigned long pfn);
 extern int sysctl_memory_failure_early_kill;
 extern int sysctl_memory_failure_recovery;
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index bead6bccc7f2..b9398f67264a 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2240,12 +2240,12 @@ void memory_failure_queue(unsigned long pfn, int flags)
 }
 EXPORT_SYMBOL_GPL(memory_failure_queue);
 
-static void memory_failure_work_func(struct work_struct *work)
+static int __memory_failure_work_func(struct work_struct *work)
 {
 	struct memory_failure_cpu *mf_cpu;
 	struct memory_failure_entry entry = { 0, };
 	unsigned long proc_flags;
-	int gotten;
+	int gotten, ret = 0, result;
 
 	mf_cpu = container_of(work, struct memory_failure_cpu, work);
 	for (;;) {
@@ -2254,24 +2254,34 @@ static void memory_failure_work_func(struct work_struct *work)
 		spin_unlock_irqrestore(&mf_cpu->lock, proc_flags);
 		if (!gotten)
 			break;
-		if (entry.flags & MF_SOFT_OFFLINE)
+		if (entry.flags & MF_SOFT_OFFLINE) {
 			soft_offline_page(entry.pfn, entry.flags);
-		else
-			memory_failure(entry.pfn, entry.flags);
+		} else {
+			result = memory_failure(entry.pfn, entry.flags);
+			if (ret == 0 && result != 0)
+				ret = result;
+		}
 	}
+
+	return ret;
+}
+
+static void memory_failure_work_func(struct work_struct *work)
+{
+	__memory_failure_work_func(work);
 }
 
 /*
  * Process memory_failure work queued on the specified CPU.
  * Used to avoid return-to-userspace racing with the memory_failure workqueue.
  */
-void memory_failure_queue_kick(int cpu)
+int memory_failure_queue_kick(int cpu)
 {
 	struct memory_failure_cpu *mf_cpu;
 
 	mf_cpu = &per_cpu(memory_failure_cpu, cpu);
 	cancel_work_sync(&mf_cpu->work);
-	memory_failure_work_func(&mf_cpu->work);
+	return __memory_failure_work_func(&mf_cpu->work);
 }
 
 static int __init memory_failure_init(void)
-- 
2.20.1

[PATCH v3 4/4] arm64: ghes: pass MF_ACTION_REQUIRED to memory_failure when sea

[Xie XiuQi]

For synchronous external data abort case, pass MF_ACTION_REQUIRED to
memory_failure, ensure that error recovery is performed before
return to the user space.

Synchronous external data abort happened in current execution context,
so as the description for 'action required', MF_ACTION_REQUIRED flag
is needed.

  ``action optional'' if they are not immediately affected by the error
  ``action required'' if error happened in current execution context

Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
---
 drivers/acpi/apei/ghes.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index ddc4da603215..043a91a7dd17 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -463,7 +463,7 @@ static bool ghes_do_memory_failure(u64 physical_addr, int flags)
 }
 
 static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
-				       int sev)
+				       int sev, int notify_type)
 {
 	int flags = -1;
 	int sec_sev = ghes_severity(gdata->error_severity);
@@ -472,6 +472,9 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
 	if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
 		return false;
 
+	if (notify_type == ACPI_HEST_NOTIFY_SEA)
+		flags |= MF_ACTION_REQUIRED;
+
 	/* iff following two events can be handled properly by now */
 	if (sec_sev == GHES_SEV_CORRECTED &&
 	    (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED))
@@ -513,7 +516,12 @@ static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
 		 * and don't filter out 'corrected' error here.
 		 */
 		if (is_cache && has_pa) {
-			queued = ghes_do_memory_failure(err_info->physical_fault_addr, 0);
+			int flags = 0;
+
+			if (notify_type == ACPI_HEST_NOTIFY_SEA)
+				flags |= MF_ACTION_REQUIRED;
+
+			queued = ghes_do_memory_failure(err_info->physical_fault_addr, flags);
 			p += err_info->length;
 			continue;
 		}
@@ -657,7 +665,7 @@ static bool ghes_do_proc(struct ghes *ghes,
 			ghes_edac_report_mem_error(sev, mem_err);
 
 			arch_apei_report_mem_error(sev, mem_err);
-			queued = ghes_handle_memory_failure(gdata, sev);
+			queued = ghes_handle_memory_failure(gdata, sev, notify_type);
 		}
 		else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
 			ghes_handle_aer(gdata);
-- 
2.20.1

Re: [PATCH v3 0/4] arm64: improve handle synchronous External Data Abort

[Shuai Xue]

On 2022/12/6 AM12:00, Xie XiuQi wrote:
> This series fix some issue for arm64 synchronous External Data Abort.
> 
> 1. fix unhandled processor error
> According to the RAS documentation, if we cannot determine the impact
> of the error based on the details of the error when an SEA occurs, the
> process cannot safely continue to run. Therefore, for unhandled error,
> we should signal the system and terminate the process immediately.
> 
> 2. improve for handling memory errors
> 
> If error happened in current execution context, we need pass
> MF_ACTION_REQUIRED flag to memory_failure(), and if memory_failure()
> recovery failed, we must handle this case, other than ignore it.
> 
> ---
> v3: add improve for handing memory errors
> v2: fix compile warning reported by kernel test robot.
> 
> Xie XiuQi (4):
>   ACPI: APEI: include missing acpi/apei.h
>   arm64: ghes: fix error unhandling in synchronous External Data Abort
>   arm64: ghes: handle the case when memory_failure recovery failed
>   arm64: ghes: pass MF_ACTION_REQUIRED to memory_failure when sea
> 
>  arch/arm64/kernel/acpi.c      |  6 ++++++
>  drivers/acpi/apei/apei-base.c |  5 +++++
>  drivers/acpi/apei/ghes.c      | 31 ++++++++++++++++++++++++-------
>  include/acpi/apei.h           |  1 +
>  include/linux/mm.h            |  2 +-
>  mm/memory-failure.c           | 24 +++++++++++++++++-------
>  6 files changed, 54 insertions(+), 15 deletions(-)
> 

Hi, XiuQi,

As we discussed, if you want to fix this problem before the new UEFI version comes out,
you need a another patch separated synchronous error handling into task work when SEA
notification is used. Be careful that do not break error handling of other notification
type.

A reference code is pasted bellow.

Thank you.

Best Regards,
Shuai

----

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 57cae48ebc1f..1982a5e3fd8c 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -445,15 +445,71 @@ static void ghes_kick_task_work(struct callback_head *head)
 	gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node, node_len);
 }

+/**
+ * struct mce_task_work - for synchronous RAS event
+ *
+ * @twork:                callback_head for task work
+ * @pfn:                  page frame number of corrupted page
+ * @flags:                fine tune action taken
+ *
+ * Structure to pass task work to be handled before
+ * ret_to_user via task_work_add().
+ */
+struct mce_task_work {
+	struct callback_head twork;
+	u64 pfn;
+	int flags;
+};
+
+static void memory_failure_cb(struct callback_head *twork)
+{
+	int rc;
+	struct mce_task_work *twcb =
+		container_of(twork, struct mce_task_work, twork);
+
+	rc = memory_failure(twcb->pfn, twcb->flags);
+	kfree(twcb);
+
+	if (!rc)
+		return;
+	/*
+	 * -EHWPOISON from memory_failure() means that it already sent SIGBUS
+	 * to the current process with the proper error info,
+	 * -EOPNOTSUPP means hwpoison_filter() filtered the error event,
+	 *
+	 * In both cases, no further processing is required.
+	 */
+	if (ret == -EHWPOISON || ret == -EOPNOTSUPP)
+		return;
+
+	pr_err("Memory error not recovered");
+	force_sig(SIGBUS);
+}
+
 static bool ghes_do_memory_failure(u64 physical_addr, int flags)
 {
 	unsigned long pfn;
+	struct mce_task_work *twcb;

 	if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
 		return false;

 	pfn = PHYS_PFN(physical_addr);
-	memory_failure_queue(pfn, flags);
+
+	if (flags == MF_ACTION_REQUIRED && task->mm) {
+		twcb = kmalloc(sizeof(*twcb), GFP_ATOMIC);
+		if (!twcb)
+			return false;
+
+		twcb->pfn = pfn;
+		twcb->flags = flags;
+		init_task_work(&twcb->twork, memory_failure_cb);
+		task_work_add(current, &twcb->twork, TWA_RESUME);
+		return false;
+	} else {
+		memory_failure_queue(pfn, flags);
+	}
+
 	return true;
 }

-- 
2.20.1.12.g72788fdb