[PATCH v3 0/5] APEI: Make APEI architecture independent.

[PATCH v3 0/5] APEI: Make APEI architecture independent.

[Tomasz Nowicki]

APEI is currently implemented so that it depends on x86 hardware.
The primary dependency is that GHES uses the x86 NMI for hardware
error notification and MCE for memory error handling. These patches
remove that dependency.

Other APEI features such as error reporting via external IRQ, error
serialization, or error injection, do not require changes to use them
on non-x86 architectures.

The following patch set eliminates the APEI Kconfig x86 dependency
by making these changes:
- treat NMI notification as GHES feature - ARCH_HAS_ACPI_APEI_NMI
- reorganize GHES notification type init/deinitialization
  so that we can decide in run time whether NMI is supported or not
- identify architectural boxes and abstract it accordingly (NMI and MCE)
- rework ioremap for both IRQ and NMI context

NMI code is kept in ghes.c file since NMI and IRQ context are tightly coupled.

Note, these patches introduce no functional changes for x86. The NMI notification
feature is hard selected for x86. Architectures that want to use this
feature should also provide NMI code infrastructure.

V1->V2
- address Borislav's comment
- abstract arch-specific calls instead of wrapping into the #ifdef

V2->V3
- address Robert's comment
- disable ACPI_APEI_NMI selection so that it is hard selected by arch Kconfig
- rename ACPI_APEI_NMI to ARCH_HAS_ACPI_APEI_NMI

Tomasz Nowicki (5):
  apei, mce: Factor out APEI architecture specific MCE calls.
  acpi, apei, ghes: Introduce ARCH_HAS_ACPI_APEI_NMI to make NMI error
    notification a GHES feature.
  acpi, apei, ghes: Introduce more generic mechanism to init/deinit
    GHES error notifications.
  apei, ghes, nmi: Factor out NMI arch-specific calls.
  acpi, apei, ghes: Factor out ioremap virtual memory for IRQ and NMI
    context.

 arch/x86/Kconfig              |    1 +
 arch/x86/kernel/acpi/Makefile |    1 +
 arch/x86/kernel/acpi/apei.c   |   87 ++++++++++++
 drivers/acpi/apei/Kconfig     |    8 +-
 drivers/acpi/apei/apei-base.c |   32 +++++
 drivers/acpi/apei/ghes.c      |  301 +++++++++++++++++++++++++----------------
 drivers/acpi/apei/hest.c      |   29 +---
 include/acpi/apei.h           |   11 ++
 8 files changed, 327 insertions(+), 143 deletions(-)
 create mode 100644 arch/x86/kernel/acpi/apei.c

-- 
1.7.9.5

[PATCH v3 1/5] apei, mce: Factor out APEI architecture specific MCE calls.

[Tomasz Nowicki]

This commit abstracts MCE calls and provides weak corresponding default
implementation for those architectures which do not need arch specific
actions. Each platform willing to do additional architectural actions
should provides desired function definition. It allows us to avoid wrap
code into #ifdef in generic code and prevent new platform from introducing
dummy stub function too.

Initially, there are two APEI arch-specific calls:
- apei_arch_enable_cmcff()
- apei_arch_report_mem_error()
Both interact with MCE driver for X86 architecture.

Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
 arch/x86/kernel/acpi/Makefile |    1 +
 arch/x86/kernel/acpi/apei.c   |   56 +++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/apei/apei-base.c |   13 ++++++++++
 drivers/acpi/apei/ghes.c      |    6 ++---
 drivers/acpi/apei/hest.c      |   29 +--------------------
 include/acpi/apei.h           |    3 +++
 6 files changed, 76 insertions(+), 32 deletions(-)
 create mode 100644 arch/x86/kernel/acpi/apei.c

diff --git a/arch/x86/kernel/acpi/Makefile b/arch/x86/kernel/acpi/Makefile
index 163b225..3242e59 100644
--- a/arch/x86/kernel/acpi/Makefile
+++ b/arch/x86/kernel/acpi/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_ACPI)		+= boot.o
 obj-$(CONFIG_ACPI_SLEEP)	+= sleep.o wakeup_$(BITS).o
+obj-$(CONFIG_ACPI_APEI)		+= apei.o
 
 ifneq ($(CONFIG_ACPI_PROCESSOR),)
 obj-y				+= cstate.o
diff --git a/arch/x86/kernel/acpi/apei.c b/arch/x86/kernel/acpi/apei.c
new file mode 100644
index 0000000..dca2852
--- /dev/null
+++ b/arch/x86/kernel/acpi/apei.c
@@ -0,0 +1,56 @@
+/*
+ * Arch-specific APEI-related functions.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <acpi/apei.h>
+
+#include <asm/mce.h>
+
+int apei_arch_enable_cmcff(struct acpi_hest_header *hest_hdr, void *data)
+{
+#ifdef CONFIG_X86_MCE
+	int i;
+	struct acpi_hest_ia_corrected *cmc;
+	struct acpi_hest_ia_error_bank *mc_bank;
+
+	if (hest_hdr->type != ACPI_HEST_TYPE_IA32_CORRECTED_CHECK)
+		return 0;
+
+	cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
+	if (!cmc->enabled)
+		return 0;
+
+	/*
+	 * We expect HEST to provide a list of MC banks that report errors
+	 * in firmware first mode. Otherwise, return non-zero value to
+	 * indicate that we are done parsing HEST.
+	 */
+	if (!(cmc->flags & ACPI_HEST_FIRMWARE_FIRST) ||
+	    !cmc->num_hardware_banks)
+		return 1;
+
+	pr_info("HEST: Enabling Firmware First mode for corrected errors.\n");
+
+	mc_bank = (struct acpi_hest_ia_error_bank *)(cmc + 1);
+	for (i = 0; i < cmc->num_hardware_banks; i++, mc_bank++)
+		mce_disable_bank(mc_bank->bank_number);
+#endif
+	return 1;
+}
+
+void apei_arch_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
+{
+#ifdef CONFIG_X86_MCE
+	apei_mce_report_mem_error(sev, mem_err);
+#endif
+}
diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 8678dfe..4a11c1a 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -745,6 +745,19 @@ struct dentry *apei_get_debugfs_dir(void)
 }
 EXPORT_SYMBOL_GPL(apei_get_debugfs_dir);
 
+int __weak apei_arch_enable_cmcff(struct acpi_hest_header *hest_hdr,
+				  void *data)
+{
+	return 1;
+}
+EXPORT_SYMBOL_GPL(apei_arch_enable_cmcff);
+
+void __weak apei_arch_report_mem_error(int sev,
+				       struct cper_sec_mem_err *mem_err)
+{
+}
+EXPORT_SYMBOL_GPL(apei_arch_report_mem_error);
+
 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 dab7cb7..e7dc5c6 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -49,7 +49,7 @@
 #include <linux/aer.h>
 
 #include <acpi/ghes.h>
-#include <asm/mce.h>
+#include <acpi/apei.h>
 #include <asm/tlbflush.h>
 #include <asm/nmi.h>
 
@@ -455,9 +455,7 @@ static void ghes_do_proc(struct ghes *ghes,
 			mem_err = (struct cper_sec_mem_err *)(gdata+1);
 			ghes_edac_report_mem_error(ghes, sev, mem_err);
 
-#ifdef CONFIG_X86_MCE
-			apei_mce_report_mem_error(sev, mem_err);
-#endif
+			apei_arch_report_mem_error(sev, mem_err);
 			ghes_handle_memory_failure(gdata, sev);
 		}
 #ifdef CONFIG_ACPI_APEI_PCIEAER
diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index f5e37f3..edf26bc 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -36,7 +36,6 @@
 #include <linux/io.h>
 #include <linux/platform_device.h>
 #include <acpi/apei.h>
-#include <asm/mce.h>
 
 #include "apei-internal.h"
 
@@ -128,33 +127,7 @@ EXPORT_SYMBOL_GPL(apei_hest_parse);
  */
 static int __init hest_parse_cmc(struct acpi_hest_header *hest_hdr, void *data)
 {
-#ifdef CONFIG_X86_MCE
-	int i;
-	struct acpi_hest_ia_corrected *cmc;
-	struct acpi_hest_ia_error_bank *mc_bank;
-
-	if (hest_hdr->type != ACPI_HEST_TYPE_IA32_CORRECTED_CHECK)
-		return 0;
-
-	cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
-	if (!cmc->enabled)
-		return 0;
-
-	/*
-	 * We expect HEST to provide a list of MC banks that report errors
-	 * in firmware first mode. Otherwise, return non-zero value to
-	 * indicate that we are done parsing HEST.
-	 */
-	if (!(cmc->flags & ACPI_HEST_FIRMWARE_FIRST) || !cmc->num_hardware_banks)
-		return 1;
-
-	pr_info(HEST_PFX "Enabling Firmware First mode for corrected errors.\n");
-
-	mc_bank = (struct acpi_hest_ia_error_bank *)(cmc + 1);
-	for (i = 0; i < cmc->num_hardware_banks; i++, mc_bank++)
-		mce_disable_bank(mc_bank->bank_number);
-#endif
-	return 1;
+	return apei_arch_enable_cmcff(hest_hdr, data);
 }
 
 struct ghes_arr {
diff --git a/include/acpi/apei.h b/include/acpi/apei.h
index 04f349d..62b9d1c 100644
--- a/include/acpi/apei.h
+++ b/include/acpi/apei.h
@@ -42,5 +42,8 @@ ssize_t erst_read(u64 record_id, struct cper_record_header *record,
 		  size_t buflen);
 int erst_clear(u64 record_id);
 
+int apei_arch_enable_cmcff(struct acpi_hest_header *hest_hdr, void *data);
+void apei_arch_report_mem_error(int sev, struct cper_sec_mem_err *mem_err);
+
 #endif
 #endif
-- 
1.7.9.5

[PATCH v3 2/5] acpi, apei, ghes: Introduce ARCH_HAS_ACPI_APEI_NMI to make NMI error notification a GHES feature.

[Tomasz Nowicki]

Currently APEI depends on x86 architecture. It is because of NMI hardware
error notification of GHES which is currently supported by x86 only.
However, many other APEI features can be still used perfectly by other
architectures.

This commit adds ARCH_HAS_ACPI_APEI_NMI which will be used in next patches
for NMI related code isolation in ghes.c file. Only NMI error notification
feature depends on x86 so let it be hard selected for x86 arch.

Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
 arch/x86/Kconfig          |    1 +
 drivers/acpi/apei/Kconfig |    8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 3fc9b12..e1dc819 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -24,6 +24,7 @@ config X86
 	select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
 	select ARCH_MIGHT_HAVE_PC_PARPORT
 	select ARCH_MIGHT_HAVE_PC_SERIO
+	select ARCH_HAS_ACPI_APEI_NMI
 	select HAVE_AOUT if X86_32
 	select HAVE_UNSTABLE_SCHED_CLOCK
 	select ARCH_SUPPORTS_NUMA_BALANCING if X86_64
diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
index c4dac71..9f6c3ec 100644
--- a/drivers/acpi/apei/Kconfig
+++ b/drivers/acpi/apei/Kconfig
@@ -3,7 +3,6 @@ config ACPI_APEI
 	select MISC_FILESYSTEMS
 	select PSTORE
 	select UEFI_CPER
-	depends on X86
 	help
 	  APEI allows to report errors (for example from the chipset)
 	  to the operating system. This improves NMI handling
@@ -26,6 +25,13 @@ config ACPI_APEI_GHES
 	  by firmware to produce more valuable hardware error
 	  information for Linux.
 
+config ARCH_HAS_ACPI_APEI_NMI
+	bool
+	help
+	  Firmware first mode can use NMI notification mechanism to report errors
+	  to operating system. This feature is currently supported by X86
+	  architecture only.
+
 config ACPI_APEI_PCIEAER
 	bool "APEI PCIe AER logging/recovering support"
 	depends on ACPI_APEI && PCIEAER
-- 
1.7.9.5

[PATCH v3 3/5] acpi, apei, ghes: Introduce more generic mechanism to init/deinit GHES error notifications.

[Tomasz Nowicki]

Init/deinit of GHES error notifications are moved to corresponding
functions e.g. for SCI ghes_notify_init_sci{sci} and ghes_notify_remove_{sci}
which in turn are gathered to ghes_notify_tab table.
ghes_probe and ghes_remove check notification support based on
function reference pointer in ghes_notify_tab and call it with ghes argument.

This approach allows us to change address of a given error notification
init/deinit function call in ghes_notify_tab in runtime. In turn,
we will avoid #ifdef usage in common code for NMI which is currently
supported by x86.

Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
 drivers/acpi/apei/ghes.c |  271 +++++++++++++++++++++++++++++-----------------
 1 file changed, 174 insertions(+), 97 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index e7dc5c6..6db5110 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -146,6 +146,14 @@ static struct irq_work ghes_proc_irq_work;
 struct ghes_estatus_cache *ghes_estatus_caches[GHES_ESTATUS_CACHES_SIZE];
 static atomic_t ghes_estatus_cache_alloced;
 
+struct ghes_notify_setup {
+	const char *notif_name;
+	int (*init_call)(struct ghes *ghes);
+	void (*remove_call)(struct ghes *ghes);
+};
+
+static struct ghes_notify_setup	ghes_notify_tab[];
+
 static int ghes_ioremap_init(void)
 {
 	ghes_ioremap_area = __get_vm_area(PAGE_SIZE * GHES_IOREMAP_PAGES,
@@ -811,6 +819,8 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
 	int sev, sev_global = -1;
 	int ret = NMI_DONE;
 
+	BUG_ON(!IS_ENABLED(ARCH_HAS_ACPI_APEI_NMI));
+
 	raw_spin_lock(&ghes_nmi_lock);
 	list_for_each_entry_rcu(ghes, &ghes_nmi, list) {
 		if (ghes_read_estatus(ghes, 1)) {
@@ -875,10 +885,6 @@ out:
 	return ret;
 }
 
-static struct notifier_block ghes_notifier_sci = {
-	.notifier_call = ghes_notify_sci,
-};
-
 static unsigned long ghes_esource_prealloc_size(
 	const struct acpi_hest_generic *generic)
 {
@@ -894,39 +900,169 @@ static unsigned long ghes_esource_prealloc_size(
 	return prealloc_size;
 }
 
+static int ghes_notify_init_nmi(struct ghes *ghes)
+{
+	unsigned long len;
+	int status = 0;
+
+	len = ghes_esource_prealloc_size(ghes->generic);
+	ghes_estatus_pool_expand(len);
+	mutex_lock(&ghes_list_mutex);
+	if (list_empty(&ghes_nmi))
+		status = register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0,
+					      "ghes");
+	list_add_rcu(&ghes->list, &ghes_nmi);
+	mutex_unlock(&ghes_list_mutex);
+
+	return status;
+}
+
+static void ghes_notify_remove_nmi(struct ghes *ghes)
+{
+	unsigned long len;
+
+	mutex_lock(&ghes_list_mutex);
+	list_del_rcu(&ghes->list);
+	if (list_empty(&ghes_nmi))
+		unregister_nmi_handler(NMI_LOCAL, "ghes");
+	mutex_unlock(&ghes_list_mutex);
+	/*
+	 * To synchronize with NMI handler, ghes can only be
+	 * freed after NMI handler finishes.
+	 */
+	synchronize_rcu();
+	len = ghes_esource_prealloc_size(ghes->generic);
+	ghes_estatus_pool_shrink(len);
+}
+
+static void ghes_init_nmi(void)
+{
+	if (!IS_ENABLED(ARCH_HAS_ACPI_APEI_NMI))
+		return;
+
+	init_irq_work(&ghes_proc_irq_work, ghes_proc_in_irq);
+	ghes_notify_tab[ACPI_HEST_NOTIFY_NMI].init_call = ghes_notify_init_nmi;
+	ghes_notify_tab[ACPI_HEST_NOTIFY_NMI].remove_call =
+							ghes_notify_remove_nmi;
+}
+
+static int ghes_notify_init_polled(struct ghes *ghes)
+{
+	ghes->timer.function = ghes_poll_func;
+	ghes->timer.data = (unsigned long)ghes;
+	init_timer_deferrable(&ghes->timer);
+	ghes_add_timer(ghes);
+
+	return 0;
+}
+
+static void ghes_notify_remove_polled(struct ghes *ghes)
+{
+	del_timer_sync(&ghes->timer);
+}
+
+static int ghes_notify_init_external(struct ghes *ghes)
+{
+	int rc;
+
+	/* External interrupt vector is GSI */
+	rc = acpi_gsi_to_irq(ghes->generic->notify.vector, &ghes->irq);
+	if (rc) {
+		pr_err(GHES_PFX "Failed to map GSI to IRQ for generic hardware error source: %d\n",
+		       ghes->generic->header.source_id);
+		goto out;
+	}
+
+	rc = request_irq(ghes->irq, ghes_irq_func, 0, "GHES IRQ", ghes);
+	if (rc)
+		pr_err(GHES_PFX "Failed to register IRQ for generic hardware error source: %d\n",
+		       ghes->generic->header.source_id);
+
+out:
+	return rc;
+}
+
+static void ghes_notify_remove_external(struct ghes *ghes)
+{
+	free_irq(ghes->irq, ghes);
+}
+
+static struct notifier_block ghes_notifier_sci = {
+	.notifier_call = ghes_notify_sci,
+};
+
+static int ghes_notify_init_sci(struct ghes *ghes)
+{
+	mutex_lock(&ghes_list_mutex);
+	if (list_empty(&ghes_sci))
+		register_acpi_hed_notifier(&ghes_notifier_sci);
+	list_add_rcu(&ghes->list, &ghes_sci);
+	mutex_unlock(&ghes_list_mutex);
+
+	return 0;
+}
+
+static void ghes_notify_remove_sci(struct ghes *ghes)
+{
+	mutex_lock(&ghes_list_mutex);
+	list_del_rcu(&ghes->list);
+	if (list_empty(&ghes_sci))
+		unregister_acpi_hed_notifier(&ghes_notifier_sci);
+	mutex_unlock(&ghes_list_mutex);
+}
+
+static struct ghes_notify_setup
+	ghes_notify_tab[ACPI_HEST_NOTIFY_RESERVED] = {
+		[ACPI_HEST_NOTIFY_POLLED]   = {"POLLED",
+					       ghes_notify_init_polled,
+					       ghes_notify_remove_polled},
+		[ACPI_HEST_NOTIFY_EXTERNAL] = {"EXT_IRQ",
+					       ghes_notify_init_external,
+					       ghes_notify_remove_external},
+		[ACPI_HEST_NOTIFY_LOCAL]    = {"LOCAL_IRQ", NULL, NULL},
+		[ACPI_HEST_NOTIFY_SCI]      = {"SCI",
+					       ghes_notify_init_sci,
+					       ghes_notify_remove_sci},
+		[ACPI_HEST_NOTIFY_NMI]      = {"NMI", NULL, NULL},
+		[ACPI_HEST_NOTIFY_CMCI]     = {"CMCI", NULL, NULL},
+		[ACPI_HEST_NOTIFY_MCE]      = {"MCE", NULL, NULL},
+};
+
 static int ghes_probe(struct platform_device *ghes_dev)
 {
 	struct acpi_hest_generic *generic;
 	struct ghes *ghes = NULL;
-	unsigned long len;
 	int rc = -EINVAL;
+	int (*ghes_init_call)(struct ghes *ghes);
+	const char *ghes_notif_name;
+	u8 notify_type;
+
 
 	generic = *(struct acpi_hest_generic **)ghes_dev->dev.platform_data;
 	if (!generic->enabled)
 		return -ENODEV;
 
-	switch (generic->notify.type) {
-	case ACPI_HEST_NOTIFY_POLLED:
-	case ACPI_HEST_NOTIFY_EXTERNAL:
-	case ACPI_HEST_NOTIFY_SCI:
-	case ACPI_HEST_NOTIFY_NMI:
-		break;
-	case ACPI_HEST_NOTIFY_LOCAL:
-		pr_warning(GHES_PFX "Generic hardware error source: %d notified via local interrupt is not supported!\n",
-			   generic->header.source_id);
+	notify_type = generic->notify.type;
+	if (notify_type >= ACPI_HEST_NOTIFY_RESERVED) {
+		pr_warn(FW_WARN GHES_PFX "Unknown notification type: %u for generic hardware error source: %d\n",
+			notify_type, generic->header.source_id);
 		goto err;
-	default:
-		pr_warning(FW_WARN GHES_PFX "Unknown notification type: %u for generic hardware error source: %d\n",
-			   generic->notify.type, generic->header.source_id);
+	}
+
+	ghes_init_call = ghes_notify_tab[notify_type].init_call;
+	ghes_notif_name = ghes_notify_tab[notify_type].notif_name;
+	if (!ghes_init_call) {
+		pr_warn(GHES_PFX "Generic hardware error source: %d notified via %s is not supported!\n",
+			generic->header.source_id, ghes_notif_name);
 		goto err;
 	}
 
 	rc = -EIO;
 	if (generic->error_block_length <
 	    sizeof(struct acpi_generic_status)) {
-		pr_warning(FW_BUG GHES_PFX "Invalid error block length: %u for generic hardware error source: %d\n",
-			   generic->error_block_length,
-			   generic->header.source_id);
+		pr_warn(FW_BUG GHES_PFX "Invalid error block length: %u for generic hardware error source: %d\n",
+			generic->error_block_length,
+			generic->header.source_id);
 		goto err;
 	}
 	ghes = ghes_new(generic);
@@ -940,48 +1076,10 @@ static int ghes_probe(struct platform_device *ghes_dev)
 	if (rc < 0)
 		goto err;
 
-	switch (generic->notify.type) {
-	case ACPI_HEST_NOTIFY_POLLED:
-		ghes->timer.function = ghes_poll_func;
-		ghes->timer.data = (unsigned long)ghes;
-		init_timer_deferrable(&ghes->timer);
-		ghes_add_timer(ghes);
-		break;
-	case ACPI_HEST_NOTIFY_EXTERNAL:
-		/* External interrupt vector is GSI */
-		rc = acpi_gsi_to_irq(generic->notify.vector, &ghes->irq);
-		if (rc) {
-			pr_err(GHES_PFX "Failed to map GSI to IRQ for generic hardware error source: %d\n",
-			       generic->header.source_id);
-			goto err_edac_unreg;
-		}
-		rc = request_irq(ghes->irq, ghes_irq_func, 0, "GHES IRQ", ghes);
-		if (rc) {
-			pr_err(GHES_PFX "Failed to register IRQ for generic hardware error source: %d\n",
-			       generic->header.source_id);
-			goto err_edac_unreg;
-		}
-		break;
-	case ACPI_HEST_NOTIFY_SCI:
-		mutex_lock(&ghes_list_mutex);
-		if (list_empty(&ghes_sci))
-			register_acpi_hed_notifier(&ghes_notifier_sci);
-		list_add_rcu(&ghes->list, &ghes_sci);
-		mutex_unlock(&ghes_list_mutex);
-		break;
-	case ACPI_HEST_NOTIFY_NMI:
-		len = ghes_esource_prealloc_size(generic);
-		ghes_estatus_pool_expand(len);
-		mutex_lock(&ghes_list_mutex);
-		if (list_empty(&ghes_nmi))
-			register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0,
-						"ghes");
-		list_add_rcu(&ghes->list, &ghes_nmi);
-		mutex_unlock(&ghes_list_mutex);
-		break;
-	default:
-		BUG();
-	}
+	rc = (*ghes_init_call)(ghes);
+	if (rc)
+		goto err_edac_unreg;
+
 	platform_set_drvdata(ghes_dev, ghes);
 
 	return 0;
@@ -999,45 +1097,24 @@ static int ghes_remove(struct platform_device *ghes_dev)
 {
 	struct ghes *ghes;
 	struct acpi_hest_generic *generic;
-	unsigned long len;
+	void (*ghes_remove_call)(struct ghes *ghes);
+	u8 notify_type;
 
 	ghes = platform_get_drvdata(ghes_dev);
-	generic = ghes->generic;
-
 	ghes->flags |= GHES_EXITING;
-	switch (generic->notify.type) {
-	case ACPI_HEST_NOTIFY_POLLED:
-		del_timer_sync(&ghes->timer);
-		break;
-	case ACPI_HEST_NOTIFY_EXTERNAL:
-		free_irq(ghes->irq, ghes);
-		break;
-	case ACPI_HEST_NOTIFY_SCI:
-		mutex_lock(&ghes_list_mutex);
-		list_del_rcu(&ghes->list);
-		if (list_empty(&ghes_sci))
-			unregister_acpi_hed_notifier(&ghes_notifier_sci);
-		mutex_unlock(&ghes_list_mutex);
-		break;
-	case ACPI_HEST_NOTIFY_NMI:
-		mutex_lock(&ghes_list_mutex);
-		list_del_rcu(&ghes->list);
-		if (list_empty(&ghes_nmi))
-			unregister_nmi_handler(NMI_LOCAL, "ghes");
-		mutex_unlock(&ghes_list_mutex);
-		/*
-		 * To synchronize with NMI handler, ghes can only be
-		 * freed after NMI handler finishes.
-		 */
-		synchronize_rcu();
-		len = ghes_esource_prealloc_size(generic);
-		ghes_estatus_pool_shrink(len);
-		break;
-	default:
-		BUG();
-		break;
+
+	generic = ghes->generic;
+	notify_type = generic->notify.type;
+	if (notify_type >= ACPI_HEST_NOTIFY_RESERVED) {
+		pr_warn(FW_WARN GHES_PFX "Unknown notification type: %u for generic hardware error source: %d\n",
+			notify_type, generic->header.source_id);
+		return -EINVAL;
 	}
 
+	ghes_remove_call = ghes_notify_tab[notify_type].remove_call;
+	if (ghes_remove_call)
+		(*ghes_remove_call)(ghes);
+
 	ghes_fini(ghes);
 
 	ghes_edac_unregister(ghes);
@@ -1075,7 +1152,7 @@ static int __init ghes_init(void)
 		return -EINVAL;
 	}
 
-	init_irq_work(&ghes_proc_irq_work, ghes_proc_in_irq);
+	ghes_init_nmi();
 
 	rc = ghes_ioremap_init();
 	if (rc)
-- 
1.7.9.5

[PATCH v3 4/5] apei, ghes, nmi: Factor out NMI arch-specific calls.

[Tomasz Nowicki]

Similar to MCE related patch, all NMI architectural calls are abstracted.
Also, we are providing corresponding X86 functions' content.

Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
 arch/x86/kernel/acpi/apei.c   |   25 +++++++++++++++++++++++++
 drivers/acpi/apei/apei-base.c |   19 +++++++++++++++++++
 drivers/acpi/apei/ghes.c      |   14 ++++++--------
 include/acpi/apei.h           |    7 +++++++
 4 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/acpi/apei.c b/arch/x86/kernel/acpi/apei.c
index dca2852..221a3a6 100644
--- a/arch/x86/kernel/acpi/apei.c
+++ b/arch/x86/kernel/acpi/apei.c
@@ -12,9 +12,12 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/kdebug.h>
 #include <acpi/apei.h>
 
 #include <asm/mce.h>
+#include <asm/nmi.h>
+#include <asm/tlbflush.h>
 
 int apei_arch_enable_cmcff(struct acpi_hest_header *hest_hdr, void *data)
 {
@@ -54,3 +57,25 @@ void apei_arch_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
 	apei_mce_report_mem_error(sev, mem_err);
 #endif
 }
+
+int arch_apei_register_nmi(int (*nmi_handler)(unsigned int, struct pt_regs *),
+			   const char *name)
+{
+	struct nmiaction apei_nmi_action = {
+		.handler = nmi_handler,
+		.name = name,
+		.flags = 0,
+	};
+
+	return __register_nmi_handler(NMI_LOCAL, &apei_nmi_action);
+}
+
+void arch_apei_unregister_nmi(const char *name)
+{
+	unregister_nmi_handler(NMI_LOCAL, name);
+}
+
+void arch_apei_nmi_oops_begin(void)
+{
+	oops_begin();
+}
diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 4a11c1a..78c2a26 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -758,6 +758,25 @@ void __weak apei_arch_report_mem_error(int sev,
 }
 EXPORT_SYMBOL_GPL(apei_arch_report_mem_error);
 
+int __weak arch_apei_register_nmi(
+	__attribute__((unused))	int (*nmi_handler)(unsigned int,
+						   struct pt_regs *),
+	const char *name)
+{
+	return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(arch_apei_register_nmi);
+
+void __weak arch_apei_unregister_nmi(const char *name)
+{
+}
+EXPORT_SYMBOL_GPL(arch_apei_unregister_nmi);
+
+void __weak arch_apei_nmi_oops_begin(void)
+{
+}
+EXPORT_SYMBOL_GPL(arch_apei_nmi_oops_begin);
+
 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 6db5110..21aeac5 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -51,7 +51,6 @@
 #include <acpi/ghes.h>
 #include <acpi/apei.h>
 #include <asm/tlbflush.h>
-#include <asm/nmi.h>
 
 #include "apei-internal.h"
 
@@ -817,7 +816,7 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
 {
 	struct ghes *ghes, *ghes_global = NULL;
 	int sev, sev_global = -1;
-	int ret = NMI_DONE;
+	int ret = APEI_NMI_DONE;
 
 	BUG_ON(!IS_ENABLED(ARCH_HAS_ACPI_APEI_NMI));
 
@@ -832,14 +831,14 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
 			sev_global = sev;
 			ghes_global = ghes;
 		}
-		ret = NMI_HANDLED;
+		ret = APEI_NMI_HANDLED;
 	}
 
-	if (ret == NMI_DONE)
+	if (ret == APEI_NMI_DONE)
 		goto out;
 
 	if (sev_global >= GHES_SEV_PANIC) {
-		oops_begin();
+		arch_apei_nmi_oops_begin();
 		ghes_print_queued_estatus();
 		__ghes_print_estatus(KERN_EMERG, ghes_global->generic,
 				     ghes_global->estatus);
@@ -909,8 +908,7 @@ static int ghes_notify_init_nmi(struct ghes *ghes)
 	ghes_estatus_pool_expand(len);
 	mutex_lock(&ghes_list_mutex);
 	if (list_empty(&ghes_nmi))
-		status = register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0,
-					      "ghes");
+		status = arch_apei_register_nmi(ghes_notify_nmi, "ghes");
 	list_add_rcu(&ghes->list, &ghes_nmi);
 	mutex_unlock(&ghes_list_mutex);
 
@@ -924,7 +922,7 @@ static void ghes_notify_remove_nmi(struct ghes *ghes)
 	mutex_lock(&ghes_list_mutex);
 	list_del_rcu(&ghes->list);
 	if (list_empty(&ghes_nmi))
-		unregister_nmi_handler(NMI_LOCAL, "ghes");
+		arch_apei_unregister_nmi("ghes");
 	mutex_unlock(&ghes_list_mutex);
 	/*
 	 * To synchronize with NMI handler, ghes can only be
diff --git a/include/acpi/apei.h b/include/acpi/apei.h
index 62b9d1c..348e1ea 100644
--- a/include/acpi/apei.h
+++ b/include/acpi/apei.h
@@ -14,6 +14,9 @@
 #define APEI_ERST_CLEAR_RECORD		_IOW('E', 1, u64)
 #define APEI_ERST_GET_RECORD_COUNT	_IOR('E', 2, u32)
 
+#define APEI_NMI_DONE		0
+#define APEI_NMI_HANDLED	1
+
 #ifdef __KERNEL__
 
 extern bool hest_disable;
@@ -44,6 +47,10 @@ int erst_clear(u64 record_id);
 
 int apei_arch_enable_cmcff(struct acpi_hest_header *hest_hdr, void *data);
 void apei_arch_report_mem_error(int sev, struct cper_sec_mem_err *mem_err);
+int arch_apei_register_nmi(int (*nmi_handler)(unsigned int, struct pt_regs *),
+			   const char *name);
+void arch_apei_unregister_nmi(const char *name);
+void arch_apei_nmi_oops_begin(void);
 
 #endif
 #endif
-- 
1.7.9.5

[PATCH v3 5/5] acpi, apei, ghes: Factor out ioremap virtual memory for IRQ and NMI context.

[Tomasz Nowicki]

GHES currently maps two pages with atomic_ioremap.  From now
on, NMI is optional so there is no need to allocate an NMI page
for platforms without NMI support.

To make it possible to not use a second page, swap the existing
page order so that the IRQ context page is first, and the optional
NMI context page is second.  Then, use ARCH_HAS_ACPI_APEI_NMI to decide
at runtime how many pages are to be allocated.  Finally, put in
sanity checks to avoid accessing unallocated memory.

Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
 arch/x86/kernel/acpi/apei.c |    6 ++++++
 drivers/acpi/apei/ghes.c    |   16 ++++++++--------
 include/acpi/apei.h         |    1 +
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/acpi/apei.c b/arch/x86/kernel/acpi/apei.c
index 221a3a6..842d158 100644
--- a/arch/x86/kernel/acpi/apei.c
+++ b/arch/x86/kernel/acpi/apei.c
@@ -79,3 +79,9 @@ void arch_apei_nmi_oops_begin(void)
 {
 	oops_begin();
 }
+
+void arch_apei_flush_tlb_one(unsigned long addr)
+{
+	__flush_tlb_one(addr);
+}
+EXPORT_SYMBOL_GPL(arch_apei_flush_tlb_one);
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 21aeac5..93a4d0b 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -113,12 +113,11 @@ static DEFINE_RAW_SPINLOCK(ghes_nmi_lock);
  */
 
 /*
- * Two virtual pages are used, one for NMI context, the other for
- * IRQ/PROCESS context
+ * Two virtual pages are used, one for IRQ/PROCESS context, the other for
+ * NMI context (optionally).
  */
-#define GHES_IOREMAP_PAGES		2
-#define GHES_IOREMAP_NMI_PAGE(base)	(base)
-#define GHES_IOREMAP_IRQ_PAGE(base)	((base) + PAGE_SIZE)
+#define GHES_IOREMAP_IRQ_PAGE(base)	(base)
+#define GHES_IOREMAP_NMI_PAGE(base)	((base) + PAGE_SIZE)
 
 /* virtual memory area for atomic ioremap */
 static struct vm_struct *ghes_ioremap_area;
@@ -155,7 +154,8 @@ static struct ghes_notify_setup	ghes_notify_tab[];
 
 static int ghes_ioremap_init(void)
 {
-	ghes_ioremap_area = __get_vm_area(PAGE_SIZE * GHES_IOREMAP_PAGES,
+	ghes_ioremap_area = __get_vm_area(
+		PAGE_SIZE * (IS_ENABLED(ARCH_HAS_ACPI_APEI_NMI) ? 2 : 1),
 		VM_IOREMAP, VMALLOC_START, VMALLOC_END);
 	if (!ghes_ioremap_area) {
 		pr_err(GHES_PFX "Failed to allocate virtual memory area for atomic ioremap.\n");
@@ -199,7 +199,7 @@ static void ghes_iounmap_nmi(void __iomem *vaddr_ptr)
 
 	BUG_ON(vaddr != (unsigned long)GHES_IOREMAP_NMI_PAGE(base));
 	unmap_kernel_range_noflush(vaddr, PAGE_SIZE);
-	__flush_tlb_one(vaddr);
+	arch_apei_flush_tlb_one(vaddr);
 }
 
 static void ghes_iounmap_irq(void __iomem *vaddr_ptr)
@@ -209,7 +209,7 @@ static void ghes_iounmap_irq(void __iomem *vaddr_ptr)
 
 	BUG_ON(vaddr != (unsigned long)GHES_IOREMAP_IRQ_PAGE(base));
 	unmap_kernel_range_noflush(vaddr, PAGE_SIZE);
-	__flush_tlb_one(vaddr);
+	arch_apei_flush_tlb_one(vaddr);
 }
 
 static int ghes_estatus_pool_init(void)
diff --git a/include/acpi/apei.h b/include/acpi/apei.h
index 348e1ea..ff2bb7e 100644
--- a/include/acpi/apei.h
+++ b/include/acpi/apei.h
@@ -51,6 +51,7 @@ int arch_apei_register_nmi(int (*nmi_handler)(unsigned int, struct pt_regs *),
 			   const char *name);
 void arch_apei_unregister_nmi(const char *name);
 void arch_apei_nmi_oops_begin(void);
+void arch_apei_flush_tlb_one(unsigned long addr);
 
 #endif
 #endif
-- 
1.7.9.5

Re: [PATCH v3 3/5] acpi, apei, ghes: Introduce more generic mechanism to init/deinit GHES error notifications.

[Robert Richter]

On 13.06.14 13:02:58, Tomasz Nowicki wrote:

> @@ -811,6 +819,8 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
>  	int sev, sev_global = -1;
>  	int ret = NMI_DONE;
>  
> +	BUG_ON(!IS_ENABLED(ARCH_HAS_ACPI_APEI_NMI));
> +

Now that we have the ARCH_HAS_ACPI_APEI_NMI option, group nmi code,
put it in an #ifdef ... and make function stubs for the !nmi case
where necessary. That code should moved to patch #2. If an arch does
not support nmi code, we don't want to compile it into the kernel.

Also this patch is quit a bit large and should further split into
moving functional code into separate functions and the introduction of
the notifier setup. This makes review much easier.

I did not yet took a deep look into your notifier framework, but I
don't really see a reason for the dynamic collection of function
pointers in ghes_notify_tab. See below.

>  	raw_spin_lock(&ghes_nmi_lock);
>  	list_for_each_entry_rcu(ghes, &ghes_nmi, list) {
>  		if (ghes_read_estatus(ghes, 1)) {
> @@ -875,10 +885,6 @@ out:
>  	return ret;
>  }

> +static int ghes_notify_init_nmi(struct ghes *ghes)
> +{
> +	unsigned long len;
> +	int status = 0;
> +
> +	len = ghes_esource_prealloc_size(ghes->generic);
> +	ghes_estatus_pool_expand(len);
> +	mutex_lock(&ghes_list_mutex);
> +	if (list_empty(&ghes_nmi))
> +		status = register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0,
> +					      "ghes");
> +	list_add_rcu(&ghes->list, &ghes_nmi);
> +	mutex_unlock(&ghes_list_mutex);
> +
> +	return status;
> +}
> +
> +static void ghes_notify_remove_nmi(struct ghes *ghes)
> +{
> +	unsigned long len;
> +
> +	mutex_lock(&ghes_list_mutex);
> +	list_del_rcu(&ghes->list);
> +	if (list_empty(&ghes_nmi))
> +		unregister_nmi_handler(NMI_LOCAL, "ghes");
> +	mutex_unlock(&ghes_list_mutex);
> +	/*
> +	 * To synchronize with NMI handler, ghes can only be
> +	 * freed after NMI handler finishes.
> +	 */
> +	synchronize_rcu();
> +	len = ghes_esource_prealloc_size(ghes->generic);
> +	ghes_estatus_pool_shrink(len);
> +}
> +
> +static void ghes_init_nmi(void)
> +{
> +	if (!IS_ENABLED(ARCH_HAS_ACPI_APEI_NMI))
> +		return;
> +
> +	init_irq_work(&ghes_proc_irq_work, ghes_proc_in_irq);
> +	ghes_notify_tab[ACPI_HEST_NOTIFY_NMI].init_call = ghes_notify_init_nmi;
> +	ghes_notify_tab[ACPI_HEST_NOTIFY_NMI].remove_call =
> +							ghes_notify_remove_nmi;
> +}
> +

So this is the only code of your whole patch set that actually changes
an entry, and just one time only during nmi init. Thus, there is no
need at all for ghes_notify_tab. Just create function stubs for
ghes_notify_{init,remove}_nmi for the !nmi case with the error message
in it and call the functions directly in the switch/cases.

> +static struct ghes_notify_setup
> +	ghes_notify_tab[ACPI_HEST_NOTIFY_RESERVED] = {
> +		[ACPI_HEST_NOTIFY_POLLED]   = {"POLLED",
> +					       ghes_notify_init_polled,
> +					       ghes_notify_remove_polled},
> +		[ACPI_HEST_NOTIFY_EXTERNAL] = {"EXT_IRQ",
> +					       ghes_notify_init_external,
> +					       ghes_notify_remove_external},
> +		[ACPI_HEST_NOTIFY_LOCAL]    = {"LOCAL_IRQ", NULL, NULL},
> +		[ACPI_HEST_NOTIFY_SCI]      = {"SCI",
> +					       ghes_notify_init_sci,
> +					       ghes_notify_remove_sci},
> +		[ACPI_HEST_NOTIFY_NMI]      = {"NMI", NULL, NULL},
> +		[ACPI_HEST_NOTIFY_CMCI]     = {"CMCI", NULL, NULL},
> +		[ACPI_HEST_NOTIFY_MCE]      = {"MCE", NULL, NULL},
> +};

Again, just keep the switch/case statements in the probe and removal
function and call the init/remove functions directly in them. This is
much easier.

If we need dynamic registration of handlers (which I don't see yet)
for the error sources above we could do this with an acpi notify
handler or so.

-Robert

Re: [PATCH v3 0/5] APEI: Make APEI architecture independent.

[Robert Richter]

On 13.06.14 13:02:55, Tomasz Nowicki wrote:
> V2->V3
> - address Robert's comment
> - disable ACPI_APEI_NMI selection so that it is hard selected by arch Kconfig
> - rename ACPI_APEI_NMI to ARCH_HAS_ACPI_APEI_NMI

Btw, it would be easier to wait a bit with reposting a new patch set
until there are no further comments expected for *all* patches (I
didn't have the time to look through all patches yet).

Thanks,

-Robert

Re: [PATCH v3 0/5] APEI: Make APEI architecture independent.

[Borislav Petkov]

On Fri, Jun 13, 2014 at 03:14:23PM +0200, Robert Richter wrote:
> Btw, it would be easier to wait a bit with reposting a new patch set
> until there are no further comments expected for *all* patches (I
> didn't have the time to look through all patches yet).

Yes, a patchset version a week roughly should be fine.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v3 4/5] apei, ghes, nmi: Factor out NMI arch-specific calls.

[Robert Richter]

On 13.06.14 13:02:59, Tomasz Nowicki wrote:
> Similar to MCE related patch, all NMI architectural calls are abstracted.
> Also, we are providing corresponding X86 functions' content.

We don't need to abstract nmi calls for archs that don't support
nmis. Just disable the code with #ifdefs depending on
ARCH_HAS_ACPI_APEI_NMI. Most changes in this patch, esp. all __weak
functions are obsolete then.

The remainings of this patch could probably merged with earlier
patches.

-Robert

> 
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> ---
>  arch/x86/kernel/acpi/apei.c   |   25 +++++++++++++++++++++++++
>  drivers/acpi/apei/apei-base.c |   19 +++++++++++++++++++
>  drivers/acpi/apei/ghes.c      |   14 ++++++--------
>  include/acpi/apei.h           |    7 +++++++
>  4 files changed, 57 insertions(+), 8 deletions(-)

Re: [PATCH v3 5/5] acpi, apei, ghes: Factor out ioremap virtual memory for IRQ and NMI context.

[Robert Richter]

On 13.06.14 13:03:00, Tomasz Nowicki wrote:
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 21aeac5..93a4d0b 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -113,12 +113,11 @@ static DEFINE_RAW_SPINLOCK(ghes_nmi_lock);
>   */
>  
>  /*
> - * Two virtual pages are used, one for NMI context, the other for
> - * IRQ/PROCESS context
> + * Two virtual pages are used, one for IRQ/PROCESS context, the other for
> + * NMI context (optionally).
>   */
> -#define GHES_IOREMAP_PAGES		2
> -#define GHES_IOREMAP_NMI_PAGE(base)	(base)
> -#define GHES_IOREMAP_IRQ_PAGE(base)	((base) + PAGE_SIZE)
> +#define GHES_IOREMAP_IRQ_PAGE(base)	(base)
> +#define GHES_IOREMAP_NMI_PAGE(base)	((base) + PAGE_SIZE)
>  
>  /* virtual memory area for atomic ioremap */
>  static struct vm_struct *ghes_ioremap_area;
> @@ -155,7 +154,8 @@ static struct ghes_notify_setup	ghes_notify_tab[];
>  
>  static int ghes_ioremap_init(void)
>  {
> -	ghes_ioremap_area = __get_vm_area(PAGE_SIZE * GHES_IOREMAP_PAGES,
> +	ghes_ioremap_area = __get_vm_area(
> +		PAGE_SIZE * (IS_ENABLED(ARCH_HAS_ACPI_APEI_NMI) ? 2 : 1),

Use this instead above:

#ifdef ARCH_HAS_ACPI_APEI_NMI
#define GHES_IOREMAP_PAGES           2
#else
#define GHES_IOREMAP_PAGES           1
#endif

Otherwise this patch looks fine to me.

-Robert

Re: [PATCH v3 1/5] apei, mce: Factor out APEI architecture specific MCE calls.

[Robert Richter]

On 13.06.14 13:02:56, Tomasz Nowicki wrote:
> This commit abstracts MCE calls and provides weak corresponding default
> implementation for those architectures which do not need arch specific
> actions. Each platform willing to do additional architectural actions
> should provides desired function definition. It allows us to avoid wrap
> code into #ifdef in generic code and prevent new platform from introducing
> dummy stub function too.
> 
> Initially, there are two APEI arch-specific calls:
> - apei_arch_enable_cmcff()
> - apei_arch_report_mem_error()
> Both interact with MCE driver for X86 architecture.
> 
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> ---
>  arch/x86/kernel/acpi/Makefile |    1 +
>  arch/x86/kernel/acpi/apei.c   |   56 +++++++++++++++++++++++++++++++++++++++++
>  drivers/acpi/apei/apei-base.c |   13 ++++++++++
>  drivers/acpi/apei/ghes.c      |    6 ++---
>  drivers/acpi/apei/hest.c      |   29 +--------------------
>  include/acpi/apei.h           |    3 +++
>  6 files changed, 76 insertions(+), 32 deletions(-)
>  create mode 100644 arch/x86/kernel/acpi/apei.c

Looks fine to me.

-Robert

Re: [PATCH v3 2/5] acpi, apei, ghes: Introduce ARCH_HAS_ACPI_APEI_NMI to make NMI error notification a GHES feature.

[Robert Richter]

On 13.06.14 13:02:57, Tomasz Nowicki wrote:
> Currently APEI depends on x86 architecture. It is because of NMI hardware
> error notification of GHES which is currently supported by x86 only.
> However, many other APEI features can be still used perfectly by other
> architectures.
> 
> This commit adds ARCH_HAS_ACPI_APEI_NMI which will be used in next patches
> for NMI related code isolation in ghes.c file. Only NMI error notification
> feature depends on x86 so let it be hard selected for x86 arch.
> 
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> ---
>  arch/x86/Kconfig          |    1 +
>  drivers/acpi/apei/Kconfig |    8 +++++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)

Apart from merging the patch with later ones this change looks fine to
me

> +config ARCH_HAS_ACPI_APEI_NMI
> +	bool
> +	help
> +	  Firmware first mode can use NMI notification mechanism to report errors
> +	  to operating system. This feature is currently supported by X86
> +	  architecture only.
> +

Maybe the help description could be removed, it is not used anywhere
in a dialog. At least the reference to x86 should be removed (if the
option is used for another arch the text will be wrong and need to be
changed again).

-Robert

Re: [PATCH v3 1/5] apei, mce: Factor out APEI architecture specific MCE calls.

[Borislav Petkov]

On Fri, Jun 13, 2014 at 01:02:56PM +0200, Tomasz Nowicki wrote:
> This commit abstracts MCE calls and provides weak corresponding default
> implementation for those architectures which do not need arch specific
> actions. Each platform willing to do additional architectural actions
> should provides desired function definition. It allows us to avoid wrap
> code into #ifdef in generic code and prevent new platform from introducing
> dummy stub function too.
> 
> Initially, there are two APEI arch-specific calls:
> - apei_arch_enable_cmcff()
> - apei_arch_report_mem_error()
> Both interact with MCE driver for X86 architecture.
> 
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>

...

> diff --git a/arch/x86/kernel/acpi/apei.c b/arch/x86/kernel/acpi/apei.c
> new file mode 100644
> index 0000000..dca2852
> --- /dev/null
> +++ b/arch/x86/kernel/acpi/apei.c
> @@ -0,0 +1,56 @@
> +/*
> + * Arch-specific APEI-related functions.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <acpi/apei.h>
> +
> +#include <asm/mce.h>
> +
> +int apei_arch_enable_cmcff(struct acpi_hest_header *hest_hdr, void *data)

Arch-specific function names usually use the "arch_" prefix. Otherwise
it looks ok.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v3 2/5] acpi, apei, ghes: Introduce ARCH_HAS_ACPI_APEI_NMI to make NMI error notification a GHES feature.

[Borislav Petkov]

On Fri, Jun 13, 2014 at 01:02:57PM +0200, Tomasz Nowicki wrote:
> Currently APEI depends on x86 architecture. It is because of NMI hardware
> error notification of GHES which is currently supported by x86 only.
> However, many other APEI features can be still used perfectly by other
> architectures.
> 
> This commit adds ARCH_HAS_ACPI_APEI_NMI which will be used in next patches
> for NMI related code isolation in ghes.c file. Only NMI error notification
> feature depends on x86 so let it be hard selected for x86 arch.
> 
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> ---
>  arch/x86/Kconfig          |    1 +
>  drivers/acpi/apei/Kconfig |    8 +++++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 3fc9b12..e1dc819 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -24,6 +24,7 @@ config X86
>  	select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
>  	select ARCH_MIGHT_HAVE_PC_PARPORT
>  	select ARCH_MIGHT_HAVE_PC_SERIO
> +	select ARCH_HAS_ACPI_APEI_NMI
>  	select HAVE_AOUT if X86_32
>  	select HAVE_UNSTABLE_SCHED_CLOCK
>  	select ARCH_SUPPORTS_NUMA_BALANCING if X86_64
> diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
> index c4dac71..9f6c3ec 100644
> --- a/drivers/acpi/apei/Kconfig
> +++ b/drivers/acpi/apei/Kconfig
> @@ -3,7 +3,6 @@ config ACPI_APEI
>  	select MISC_FILESYSTEMS
>  	select PSTORE
>  	select UEFI_CPER
> -	depends on X86

Now this can practically be enabled on any architecture, AFAICT. Which
is wrong.

I think a better solution would be to have another HAVE_ symbol which
each arch which sports APEI selects. Like in the diff below ontop of
this patch, also incorporating Robert's comments.

You'll have to do select HAVE_ACPI_APEI on arm too.

Hmm?

--
Index: b/arch/x86/Kconfig
===================================================================
--- a/arch/x86/Kconfig	2014-06-19 16:25:48.118452980 +0200
+++ b/arch/x86/Kconfig	2014-06-19 16:24:57.270453451 +0200
@@ -24,7 +24,6 @@ config X86
 	select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
 	select ARCH_MIGHT_HAVE_PC_PARPORT
 	select ARCH_MIGHT_HAVE_PC_SERIO
-	select ARCH_HAS_ACPI_APEI_NMI
 	select HAVE_AOUT if X86_32
 	select HAVE_UNSTABLE_SCHED_CLOCK
 	select ARCH_SUPPORTS_NUMA_BALANCING if X86_64
@@ -132,6 +131,8 @@ config X86
 	select HAVE_CC_STACKPROTECTOR
 	select GENERIC_CPU_AUTOPROBE
 	select HAVE_ARCH_AUDITSYSCALL
+	select HAVE_ACPI_APEI
+	select HAVE_ACPI_APEI_NMI
 
 config INSTRUCTION_DECODER
 	def_bool y
Index: b/drivers/acpi/apei/Kconfig
===================================================================
--- a/drivers/acpi/apei/Kconfig	2014-06-19 16:25:48.118452980 +0200
+++ b/drivers/acpi/apei/Kconfig	2014-06-19 16:24:32.710453679 +0200
@@ -1,8 +1,15 @@
+config HAVE_ACPI_APEI
+	bool
+
+config HAVE_ACPI_APEI_NMI
+	bool
+
 config ACPI_APEI
 	bool "ACPI Platform Error Interface (APEI)"
 	select MISC_FILESYSTEMS
 	select PSTORE
 	select UEFI_CPER
+	depends on HAVE_ACPI_APEI
 	help
 	  APEI allows to report errors (for example from the chipset)
 	  to the operating system. This improves NMI handling
@@ -25,12 +32,6 @@ config ACPI_APEI_GHES
 	  by firmware to produce more valuable hardware error
 	  information for Linux.
 
-config ARCH_HAS_ACPI_APEI_NMI
-	bool
-	help
-	  Firmware first mode can use NMI notification mechanism to report errors
-	  to operating system. This feature is currently supported by X86
-	  architecture only.
 
 config ACPI_APEI_PCIEAER
 	bool "APEI PCIe AER logging/recovering support"


-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v3 3/5] acpi, apei, ghes: Introduce more generic mechanism to init/deinit GHES error notifications.

[Borislav Petkov]

On Fri, Jun 13, 2014 at 03:10:51PM +0200, Robert Richter wrote:
> On 13.06.14 13:02:58, Tomasz Nowicki wrote:
> 
> > @@ -811,6 +819,8 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
> >  	int sev, sev_global = -1;
> >  	int ret = NMI_DONE;
> >  
> > +	BUG_ON(!IS_ENABLED(ARCH_HAS_ACPI_APEI_NMI));
> > +
> 
> Now that we have the ARCH_HAS_ACPI_APEI_NMI option, group nmi code,
> put it in an #ifdef ... and make function stubs for the !nmi case
> where necessary. That code should moved to patch #2. If an arch does
> not support nmi code, we don't want to compile it into the kernel.
> 
> Also this patch is quit a bit large and should further split into
> moving functional code into separate functions and the introduction of
> the notifier setup. This makes review much easier.
> 
> I did not yet took a deep look into your notifier framework, but I
> don't really see a reason for the dynamic collection of function
> pointers in ghes_notify_tab. See below.

Ok, I'll wait out with further review after you've integrated Robert's
comments.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v3 2/5] acpi, apei, ghes: Introduce ARCH_HAS_ACPI_APEI_NMI to make NMI error notification a GHES feature.

[Tomasz Nowicki]

On 19.06.2014 16:27, Borislav Petkov wrote:
> On Fri, Jun 13, 2014 at 01:02:57PM +0200, Tomasz Nowicki wrote:
>> Currently APEI depends on x86 architecture. It is because of NMI hardware
>> error notification of GHES which is currently supported by x86 only.
>> However, many other APEI features can be still used perfectly by other
>> architectures.
>>
>> This commit adds ARCH_HAS_ACPI_APEI_NMI which will be used in next patches
>> for NMI related code isolation in ghes.c file. Only NMI error notification
>> feature depends on x86 so let it be hard selected for x86 arch.
>>
>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>> ---
>>   arch/x86/Kconfig          |    1 +
>>   drivers/acpi/apei/Kconfig |    8 +++++++-
>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index 3fc9b12..e1dc819 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -24,6 +24,7 @@ config X86
>>   	select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS
>>   	select ARCH_MIGHT_HAVE_PC_PARPORT
>>   	select ARCH_MIGHT_HAVE_PC_SERIO
>> +	select ARCH_HAS_ACPI_APEI_NMI
>>   	select HAVE_AOUT if X86_32
>>   	select HAVE_UNSTABLE_SCHED_CLOCK
>>   	select ARCH_SUPPORTS_NUMA_BALANCING if X86_64
>> diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
>> index c4dac71..9f6c3ec 100644
>> --- a/drivers/acpi/apei/Kconfig
>> +++ b/drivers/acpi/apei/Kconfig
>> @@ -3,7 +3,6 @@ config ACPI_APEI
>>   	select MISC_FILESYSTEMS
>>   	select PSTORE
>>   	select UEFI_CPER
>> -	depends on X86
>
> Now this can practically be enabled on any architecture, AFAICT. Which
> is wrong.
>
> I think a better solution would be to have another HAVE_ symbol which
> each arch which sports APEI selects. Like in the diff below ontop of
> this patch, also incorporating Robert's comments.
>
> You'll have to do select HAVE_ACPI_APEI on arm too.
>
> Hmm?
>

Now that it turns out we have to provide at least tlb_flush_... arch 
function, APEI can not be selected by any architecture. So you are right 
I will introduce HAVE_ACPI_APEI in next version of patches. Thanks.

Tomasz

Re: [PATCH v3 1/5] apei, mce: Factor out APEI architecture specific MCE calls.

[Tomasz Nowicki]

On 19.06.2014 16:17, Borislav Petkov wrote:
> On Fri, Jun 13, 2014 at 01:02:56PM +0200, Tomasz Nowicki wrote:
>> This commit abstracts MCE calls and provides weak corresponding default
>> implementation for those architectures which do not need arch specific
>> actions. Each platform willing to do additional architectural actions
>> should provides desired function definition. It allows us to avoid wrap
>> code into #ifdef in generic code and prevent new platform from introducing
>> dummy stub function too.
>>
>> Initially, there are two APEI arch-specific calls:
>> - apei_arch_enable_cmcff()
>> - apei_arch_report_mem_error()
>> Both interact with MCE driver for X86 architecture.
>>
>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>
> ...
>
>> diff --git a/arch/x86/kernel/acpi/apei.c b/arch/x86/kernel/acpi/apei.c
>> new file mode 100644
>> index 0000000..dca2852
>> --- /dev/null
>> +++ b/arch/x86/kernel/acpi/apei.c
>> @@ -0,0 +1,56 @@
>> +/*
>> + * Arch-specific APEI-related functions.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <acpi/apei.h>
>> +
>> +#include <asm/mce.h>
>> +
>> +int apei_arch_enable_cmcff(struct acpi_hest_header *hest_hdr, void *data)
>
> Arch-specific function names usually use the "arch_" prefix. Otherwise
> it looks ok.

True, will fix that.

Tomasz

Re: [PATCH v3 3/5] acpi, apei, ghes: Introduce more generic mechanism to init/deinit GHES error notifications.

[Tomasz Nowicki]

On 13.06.2014 15:10, Robert Richter wrote:
> On 13.06.14 13:02:58, Tomasz Nowicki wrote:
>
>> @@ -811,6 +819,8 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
>>   	int sev, sev_global = -1;
>>   	int ret = NMI_DONE;
>>
>> +	BUG_ON(!IS_ENABLED(ARCH_HAS_ACPI_APEI_NMI));
>> +
>
> Now that we have the ARCH_HAS_ACPI_APEI_NMI option, group nmi code,
> put it in an #ifdef ... and make function stubs for the !nmi case
> where necessary. That code should moved to patch #2. If an arch does
> not support nmi code, we don't want to compile it into the kernel.
>
> Also this patch is quit a bit large and should further split into
> moving functional code into separate functions and the introduction of
> the notifier setup. This makes review much easier.
>
> I did not yet took a deep look into your notifier framework, but I
> don't really see a reason for the dynamic collection of function
> pointers in ghes_notify_tab. See below.
>
>>   	raw_spin_lock(&ghes_nmi_lock);
>>   	list_for_each_entry_rcu(ghes, &ghes_nmi, list) {
>>   		if (ghes_read_estatus(ghes, 1)) {
>> @@ -875,10 +885,6 @@ out:
>>   	return ret;
>>   }
>
>> +static int ghes_notify_init_nmi(struct ghes *ghes)
>> +{
>> +	unsigned long len;
>> +	int status = 0;
>> +
>> +	len = ghes_esource_prealloc_size(ghes->generic);
>> +	ghes_estatus_pool_expand(len);
>> +	mutex_lock(&ghes_list_mutex);
>> +	if (list_empty(&ghes_nmi))
>> +		status = register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0,
>> +					      "ghes");
>> +	list_add_rcu(&ghes->list, &ghes_nmi);
>> +	mutex_unlock(&ghes_list_mutex);
>> +
>> +	return status;
>> +}
>> +
>> +static void ghes_notify_remove_nmi(struct ghes *ghes)
>> +{
>> +	unsigned long len;
>> +
>> +	mutex_lock(&ghes_list_mutex);
>> +	list_del_rcu(&ghes->list);
>> +	if (list_empty(&ghes_nmi))
>> +		unregister_nmi_handler(NMI_LOCAL, "ghes");
>> +	mutex_unlock(&ghes_list_mutex);
>> +	/*
>> +	 * To synchronize with NMI handler, ghes can only be
>> +	 * freed after NMI handler finishes.
>> +	 */
>> +	synchronize_rcu();
>> +	len = ghes_esource_prealloc_size(ghes->generic);
>> +	ghes_estatus_pool_shrink(len);
>> +}
>> +
>> +static void ghes_init_nmi(void)
>> +{
>> +	if (!IS_ENABLED(ARCH_HAS_ACPI_APEI_NMI))
>> +		return;
>> +
>> +	init_irq_work(&ghes_proc_irq_work, ghes_proc_in_irq);
>> +	ghes_notify_tab[ACPI_HEST_NOTIFY_NMI].init_call = ghes_notify_init_nmi;
>> +	ghes_notify_tab[ACPI_HEST_NOTIFY_NMI].remove_call =
>> +							ghes_notify_remove_nmi;
>> +}
>> +
>
> So this is the only code of your whole patch set that actually changes
> an entry, and just one time only during nmi init. Thus, there is no
> need at all for ghes_notify_tab. Just create function stubs for
> ghes_notify_{init,remove}_nmi for the !nmi case with the error message
> in it and call the functions directly in the switch/cases.
>
>> +static struct ghes_notify_setup
>> +	ghes_notify_tab[ACPI_HEST_NOTIFY_RESERVED] = {
>> +		[ACPI_HEST_NOTIFY_POLLED]   = {"POLLED",
>> +					       ghes_notify_init_polled,
>> +					       ghes_notify_remove_polled},
>> +		[ACPI_HEST_NOTIFY_EXTERNAL] = {"EXT_IRQ",
>> +					       ghes_notify_init_external,
>> +					       ghes_notify_remove_external},
>> +		[ACPI_HEST_NOTIFY_LOCAL]    = {"LOCAL_IRQ", NULL, NULL},
>> +		[ACPI_HEST_NOTIFY_SCI]      = {"SCI",
>> +					       ghes_notify_init_sci,
>> +					       ghes_notify_remove_sci},
>> +		[ACPI_HEST_NOTIFY_NMI]      = {"NMI", NULL, NULL},
>> +		[ACPI_HEST_NOTIFY_CMCI]     = {"CMCI", NULL, NULL},
>> +		[ACPI_HEST_NOTIFY_MCE]      = {"MCE", NULL, NULL},
>> +};
>
> Again, just keep the switch/case statements in the probe and removal
> function and call the init/remove functions directly in them. This is
> much easier.
>
> If we need dynamic registration of handlers (which I don't see yet)
> for the error sources above we could do this with an acpi notify
> handler or so.
>

Without abstraction, notify handler registration seems to be overhead. I 
will modify code as you suggested. Thanks.

Tomasz