Firmware first mode for corrected errors

The Corrected Machine Check structure (CMC) in HEST has a flag which can be set by the firmware to indicate to the OS that it prefers to process the corrected error events first. In this scenario, the OS is expected to not monitor for corrected errors (through CMCI/polling). Instead, the firmware notifies the OS on corrected error events through GHES.

Linux already has support for GHES. This patch adds support for parsing CMC structure and to disable CMCI/polling if the firmware first flag is set.

Further, the list of machine check bank structures at the end of CMC is used to determine which MCA banks function in FF mode, so that we continue to monitor error events on the other banks.

Signed-off-by Naveen N. Rao <[email protected]>

Add a boot option to disable firmware first mode for corrected errors.

Signed-off-by Naveen N. Rao <[email protected]>

If the firmware indicates in GHES error data entry that the error threshold has exceeded for a corrected error event, then we try to soft-offline the page. This could be called in interrupt context, so we queue this up similar to how we handle memory failure scenarios.

Signed-off-by Naveen N. Rao <[email protected]>

---
 drivers/acpi/apei/ghes.c |   12 ++++++++++
 include/linux/mm.h       |    1 +
 mm/memory-failure.c      |   53 ++++++++++++++++++++++++++++++----------------
 3 files changed, 48 insertions(+), 18 deletions(-)

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

index fcd7d91..5a630ed 100644
@@ -429,6 +429,18 @@ static void ghes_do_proc(struct ghes *ghes,
 						  mem_err);
 #endif
 #ifdef CONFIG_ACPI_APEI_MEMORY_FAILURE
+			if (sec_sev == GHES_SEV_CORRECTED &&
+			    (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED) &&

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

 			if (sev == GHES_SEV_RECOVERABLE &&
 			    sec_sev == GHES_SEV_RECOVERABLE &&
 			    mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS) {

include/linux/mm.h +1 -0

index e0c8528..f9907d2 100644
@@ -1787,6 +1787,7 @@ enum mf_flags {
 };
 extern int memory_failure(unsigned long pfn, int trapno, int flags);
 extern void memory_failure_queue(unsigned long pfn, int trapno, int flags);
+extern void soft_memory_failure_queue(unsigned long pfn, int trapno, int flags);
 extern int unpoison_memory(unsigned long pfn);
 extern int sysctl_memory_failure_early_kill;
 extern int sysctl_memory_failure_recovery;

mm/memory-failure.c +0 -0

index ceb0c7f..50caefd 100644

mm/memory-failure.c +23 -14

 };
 
 struct memory_failure_cpu {

@@ -1233,6 +1234,28 @@ struct memory_failure_cpu {
 
 static DEFINE_PER_CPU(struct memory_failure_cpu, memory_failure_cpu);
 
+void __memory_failure_queue(unsigned long pfn, int trapno, int flags, int soft_offline)
+{
+	struct memory_failure_cpu *mf_cpu;
+	unsigned long proc_flags;
+	struct memory_failure_entry entry = {
+		.pfn =		pfn,
+		.trapno =	trapno,
+		.flags =	flags,
+		.soft_offline = soft_offline
+	};
+
+	mf_cpu = &get_cpu_var(memory_failure_cpu);
+	spin_lock_irqsave(&mf_cpu->lock, proc_flags);
+	if (kfifo_put(&mf_cpu->fifo, &entry))
+		schedule_work_on(smp_processor_id(), &mf_cpu->work);
+	else
+		pr_err("Memory failure: buffer overflow when queuing memory failure at 0x%#lx\n",
+		       pfn);
+	spin_unlock_irqrestore(&mf_cpu->lock, proc_flags);
+	put_cpu_var(memory_failure_cpu);
+}
+
 /**
  * memory_failure_queue - Schedule handling memory failure of a page.
  * @pfn: Page Number of the corrupted page

@@ -1250,28 +1273,19 @@ static DEFINE_PER_CPU(struct memory_failure_cpu, memory_failure_cpu);
  *
  * Can run in IRQ context.
  */
+
 void memory_failure_queue(unsigned long pfn, int trapno, int flags)
 {
-	struct memory_failure_cpu *mf_cpu;
-	unsigned long proc_flags;
-	struct memory_failure_entry entry = {
-		.pfn =		pfn,
-		.trapno =	trapno,
-		.flags =	flags,
-	};
-
-	mf_cpu = &get_cpu_var(memory_failure_cpu);
-	spin_lock_irqsave(&mf_cpu->lock, proc_flags);
-	if (kfifo_put(&mf_cpu->fifo, &entry))
-		schedule_work_on(smp_processor_id(), &mf_cpu->work);
-	else
-		pr_err("Memory failure: buffer overflow when queuing memory failure at 0x%#lx\n",

mm/memory-failure.c +7 -1

+}
+EXPORT_SYMBOL_GPL(soft_memory_failure_queue);
+
 static void memory_failure_work_func(struct work_struct *work)
 {
 	struct memory_failure_cpu *mf_cpu;

@@ -1286,7 +1300,10 @@ static void memory_failure_work_func(struct work_struct *work)
 		spin_unlock_irqrestore(&mf_cpu->lock, proc_flags);
 		if (!gotten)
 			break;
-		memory_failure(entry.pfn, entry.trapno, entry.flags);
+		if (entry.soft_offline)
+			soft_offline_page(pfn_to_page(entry.pfn), entry.flags);
+		else
+			memory_failure(entry.pfn, entry.trapno, entry.flags);
 	}
 }
 

Quoted 23 lines · Naveen N. Rao

If the firmware indicates in GHES error data entry that the error threshold has exceeded for a corrected error event, then we try to soft-offline the page. This could be called in interrupt context, so we queue this up similar to how we handle memory failure scenarios.

Signed-off-by Naveen N. Rao <[email protected]>

---
  drivers/acpi/apei/ghes.c |   12 ++++++++++
  include/linux/mm.h       |    1 +
  mm/memory-failure.c      |   53 ++++++++++++++++++++++++++++++----------------
  3 files changed, 48 insertions(+), 18 deletions(-)

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

index fcd7d91..5a630ed 100644
@@ -429,6 +429,18 @@ static void ghes_do_proc(struct ghes *ghes,
  						  mem_err);
  #endif
  #ifdef CONFIG_ACPI_APEI_MEMORY_FAILURE
+			if (sec_sev == GHES_SEV_CORRECTED &&
+			    (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED) &&

drivers/acpi/apei/ghes.c static void ghes_do_proc(struct ghes *ghes, · line 443

Quoted 10 lines

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

+			    (mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS)) {
+				unsigned long pfn;
+				pfn = mem_err->physical_addr >> PAGE_SHIFT;
+				if (pfn_valid(pfn))
+					soft_memory_failure_queue(pfn, 0, 0);
+				else
+					pr_warning(FW_WARN GHES_PFX
+					"Invalid address in generic error data: %#lx\n",
+					mem_err->physical_addr);
+			}

Yuck, this looks like BIOS code.

Can we carve out this into a function and do

void function(.. )
{
#ifdef CONFIG_ACPI_APEI_MEMORY_FAILURE

<code at 1st indentation, much more readable>

#endif
}

so that we can nicely call it from ghes_do_proc()?

Quoted 19 lines

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

  			if (sev == GHES_SEV_RECOVERABLE &&
  			    sec_sev == GHES_SEV_RECOVERABLE &&
  			    mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS) {

include/linux/mm.h +1 -0

index e0c8528..f9907d2 100644
@@ -1787,6 +1787,7 @@ enum mf_flags {
  };
  extern int memory_failure(unsigned long pfn, int trapno, int flags);
  extern void memory_failure_queue(unsigned long pfn, int trapno, int flags);
+extern void soft_memory_failure_queue(unsigned long pfn, int trapno, int flags);
  extern int unpoison_memory(unsigned long pfn);
  extern int sysctl_memory_failure_early_kill;
  extern int sysctl_memory_failure_recovery;

mm/memory-failure.c +0 -1

index ceb0c7f..50caefd 100644
--- a/mm/memory-failure.c

mm/memory-failure.c struct memory_failure_entry { · line 1225

Quoted 6 lines

+++ b/mm/memory-failure.c

mm/memory-failure.c +1 -0

@@ -1222,6 +1222,7 @@ struct memory_failure_entry {
  	unsigned long pfn;
  	int trapno;
  	int flags;
+	bool soft_offline;

Why a new bool? This flags int looks nice above. :)

Here is the updated patch. I also added printk_ratelimit() in line with the rest of the GHES code.

Thanks,
Naveen

--
If the firmware indicates in GHES error data entry that the error threshold has exceeded for a corrected error event, then we try to soft-offline the page. This could be called in interrupt context, so we queue this up similar to how we handle memory failure scenarios.

Signed-off-by Naveen N. Rao <[email protected]>

---
 drivers/acpi/apei/ghes.c |   38 +++++++++++++++++++++++++++++---------
 include/linux/mm.h       |    1 +
 mm/memory-failure.c      |    5 ++++-
 3 files changed, 34 insertions(+), 10 deletions(-)

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

index fcd7d91..74ef688 100644

drivers/acpi/apei/ghes.c +14 -9

+			"Invalid address in generic error data: %#llx\n",
+			mem_err->physical_addr);
+	}
+	if (sev == GHES_SEV_RECOVERABLE &&
+	    sec_sev == GHES_SEV_RECOVERABLE &&
+	    mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS) {
+		unsigned long pfn;
+		pfn = mem_err->physical_addr >> PAGE_SHIFT;
+		memory_failure_queue(pfn, 0, 0);
+	}
+#endif
+}
+
 static void ghes_do_proc(struct ghes *ghes,
 			 const struct acpi_hest_generic_status *estatus)
 {

@@ -428,15 +456,7 @@ static void ghes_do_proc(struct ghes *ghes,
 			apei_mce_report_mem_error(sev == GHES_SEV_CORRECTED,
 						  mem_err);
 #endif
-#ifdef CONFIG_ACPI_APEI_MEMORY_FAILURE
-			if (sev == GHES_SEV_RECOVERABLE &&
-			    sec_sev == GHES_SEV_RECOVERABLE &&
-			    mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS) {
-				unsigned long pfn;
-				pfn = mem_err->physical_addr >> PAGE_SHIFT;
-				memory_failure_queue(pfn, 0, 0);
-			}
-#endif
+			ghes_handle_memory_failure(gdata, sev);
 		}
 #ifdef CONFIG_ACPI_APEI_PCIEAER
 		else if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,

include/linux/mm.h +1 -0

index e0c8528..958e9efd 100644
@@ -1784,6 +1784,7 @@ enum mf_flags {
 	MF_COUNT_INCREASED = 1 << 0,
 	MF_ACTION_REQUIRED = 1 << 1,
 	MF_MUST_KILL = 1 << 2,
+	MF_SOFT_OFFLINE = 1 << 3,
 };
 extern int memory_failure(unsigned long pfn, int trapno, int flags);
 extern void memory_failure_queue(unsigned long pfn, int trapno, int flags);

mm/memory-failure.c +0 -0

index ceb0c7f..0d6717e 100644

Quoted 28 lines · Naveen N. Rao

Here is the updated patch. I also added printk_ratelimit() in line with the rest of the GHES code.

Thanks,
Naveen

--
If the firmware indicates in GHES error data entry that the error threshold has exceeded for a corrected error event, then we try to soft-offline the page. This could be called in interrupt context, so we queue this up similar to how we handle memory failure scenarios.

Signed-off-by Naveen N. Rao <[email protected]>

---
  drivers/acpi/apei/ghes.c |   38 +++++++++++++++++++++++++++++---------
  include/linux/mm.h       |    1 +
  mm/memory-failure.c      |    5 ++++-
  3 files changed, 34 insertions(+), 10 deletions(-)

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

index fcd7d91..74ef688 100644
@@ -409,6 +409,34 @@ static void ghes_clear_estatus(struct ghes *ghes)
  	ghes->flags &= ~GHES_TO_CLEAR;
  }
 

drivers/acpi/apei/ghes.c static void ghes_clear_estatus(struct ghes *ghes) · line 417

Quoted 6 lines

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

+static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int sev)
+{
+#ifdef CONFIG_ACPI_APEI_MEMORY_FAILURE
+	int sec_sev = ghes_severity(gdata->error_severity);
+	struct cper_sec_mem_err *mem_err;
+	mem_err = (struct cper_sec_mem_err *)(gdata+1);

A newline here please. Also, spaces around '+'.

drivers/acpi/apei/ghes.c static void ghes_clear_estatus(struct ghes *ghes) · line 421

Quoted 5 lines

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

+	if (sec_sev == GHES_SEV_CORRECTED &&
+	    (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED) &&
+	    (mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS)) {
+		unsigned long pfn;

This pfn is defined twice, move it up to the beginning of the function.

drivers/acpi/apei/ghes.c static void ghes_clear_estatus(struct ghes *ghes) · line 426

Quoted 6 lines

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

+		pfn = mem_err->physical_addr >> PAGE_SHIFT;
+		if (pfn_valid(pfn))
+			memory_failure_queue(pfn, 0, MF_SOFT_OFFLINE);
+		else if (printk_ratelimit())
+			pr_warning(FW_WARN GHES_PFX

WARNING: Prefer printk_ratelimited or pr_<level>_ratelimited to printk_ratelimit #35: FILE: drivers/acpi/apei/ghes.c:425:
+ else if (printk_ratelimit())

Please run your patches through checkpatch.pl first.

This requested change will even simplify the code (ontop of your patch):

drivers/acpi/apei/ghes.c +4 -5

index 74ef6882bca9..87e11d468f6b 100644
@@ -422,10 +422,10 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int
  		pfn = mem_err->physical_addr >> PAGE_SHIFT;
  		if (pfn_valid(pfn))
  			memory_failure_queue(pfn, 0, MF_SOFT_OFFLINE);
-		else if (printk_ratelimit())
-			pr_warning(FW_WARN GHES_PFX
-			"Invalid address in generic error data: %#llx\n",
-			mem_err->physical_addr);
+		else
+			pr_warn_ratelimited(FW_WARN GHES_PFX
+					    "Invalid address in generic error data: %#llx\n",
+					    mem_err->physical_addr);
  	}
  	if (sev == GHES_SEV_RECOVERABLE &&
  	    sec_sev == GHES_SEV_RECOVERABLE &&
---

Quoted 53 lines

drivers/acpi/apei/ghes.c +14 -9

+			"Invalid address in generic error data: %#llx\n",
+			mem_err->physical_addr);
+	}
+	if (sev == GHES_SEV_RECOVERABLE &&
+	    sec_sev == GHES_SEV_RECOVERABLE &&
+	    mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS) {
+		unsigned long pfn;
+		pfn = mem_err->physical_addr >> PAGE_SHIFT;
+		memory_failure_queue(pfn, 0, 0);
+	}
+#endif
+}
+
  static void ghes_do_proc(struct ghes *ghes,
  			 const struct acpi_hest_generic_status *estatus)
  {

@@ -428,15 +456,7 @@ static void ghes_do_proc(struct ghes *ghes,
  			apei_mce_report_mem_error(sev == GHES_SEV_CORRECTED,
  						  mem_err);
  #endif
-#ifdef CONFIG_ACPI_APEI_MEMORY_FAILURE
-			if (sev == GHES_SEV_RECOVERABLE &&
-			    sec_sev == GHES_SEV_RECOVERABLE &&
-			    mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS) {
-				unsigned long pfn;
-				pfn = mem_err->physical_addr >> PAGE_SHIFT;
-				memory_failure_queue(pfn, 0, 0);
-			}
-#endif
+			ghes_handle_memory_failure(gdata, sev);
  		}
  #ifdef CONFIG_ACPI_APEI_PCIEAER
  		else if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,

include/linux/mm.h +1 -0

index e0c8528..958e9efd 100644
@@ -1784,6 +1784,7 @@ enum mf_flags {
  	MF_COUNT_INCREASED = 1 << 0,
  	MF_ACTION_REQUIRED = 1 << 1,
  	MF_MUST_KILL = 1 << 2,
+	MF_SOFT_OFFLINE = 1 << 3,
  };
  extern int memory_failure(unsigned long pfn, int trapno, int flags);
  extern void memory_failure_queue(unsigned long pfn, int trapno, int flags);

mm/memory-failure.c +0 -0

index ceb0c7f..0d6717e 100644

mm/memory-failure.c static void memory_failure_work_func(struct work_struct *work) · line 1292

Quoted 8 lines

mm/memory-failure.c +4 -1

@@ -1286,7 +1286,10 @@ static void memory_failure_work_func(struct work_struct *work)
  		spin_unlock_irqrestore(&mf_cpu->lock, proc_flags);
  		if (!gotten)
  			break;
-		memory_failure(entry.pfn, entry.trapno, entry.flags);
+		if (entry.flags & MF_SOFT_OFFLINE)
+			soft_offline_page(pfn_to_page(entry.pfn), entry.flags);
+		else
+			memory_failure(entry.pfn, entry.trapno, entry.flags);

The rest looks ok to me.

I'm guessing this has been tested by injecting errors...?

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+#ifdef CONFIG_ACPI_APEI_MEMORY_FAILURE
+       int sec_sev = ghes_severity(gdata->error_severity);
+       struct cper_sec_mem_err *mem_err;
+       mem_err = (struct cper_sec_mem_err *)(gdata+1);

A newline here please. Also, spaces around '+'.

I was off on vacation last week - looks like you got lots done without me :-)

I have parts 1 & 2 applied to an internal tree. Looks like parts 3 & 4 need a few final polishes to get an Ack from Boris.

Quoted 5 lines · Tony Luck

I was off on vacation last week - looks like you got lots done without me :-)

I have parts 1 & 2 applied to an internal tree. Looks like parts 3 & 4 need a few final polishes to get an Ack from Boris.

Cool :)
Here is the updated patch for part 3 incorporating Boris's comments.

Thanks,
Naveen
--

If the firmware indicates in GHES error data entry that the error threshold has exceeded for a corrected error event, then we try to soft-offline the page. This could be called in interrupt context, so we queue this up similar to how we handle memory failure scenarios.

Signed-off-by Naveen N. Rao <[email protected]>

Quoted 1 line

Signed-off-by Naveen N. Rao <[email protected]>

Acked-by Borislav Petkov <[email protected]>

Hi Tony,
Here is the updated patchset which addresses all previous review comments. For the third patch, I have also moved the soft-offline section in ghes.c into the #ifdef block for APEI_MEMORY_FAILURE.

If there are no further comments, can you please queue this up for 3.11?

I am adding another patch here to disable page offlining in case the firmware starts acting up.

Thanks,
Naveen

--

Add a sysctl memory_failure_soft_offline to control what is done on receipt of firmware ghes notification for a corrected error. By default, kernel tries to soft-offline the page immediately. If set to 0, no action is taken.

Quoted 8 lines · Naveen N. Rao

I am adding another patch here to disable page offlining in case the firmware starts acting up.

Thanks,
Naveen

--

Quoted 3 lines

Add a sysctl memory_failure_soft_offline to control what is done on receipt of firmware ghes notification for a corrected error. By default, kernel tries to soft-offline the page immediately. If set to 0, no action is taken.

What is the rationale for that? Are we adding it just in case, as a chicken bit or do you have a specific case?

If the second, we'd love to hear about it in the commit message. :)

Nope, this is a just-in-case thing. I think you or Tony asked to have this in a previous discussion so that we're covered if firmware starts acting up. Other than that, I'm ok if this is left out.

I'm happy with just the acpi=nocmcff to avoid a BIOS that does weird stuff. Or do you think we might still have to deal with a string of APEI messages?