* [PATCH 0/7] APEI: Make APEI architecture independent.
@ 2014-04-09 15:14 Tomasz Nowicki
2014-04-09 15:14 ` [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture Tomasz Nowicki
` (7 more replies)
0 siblings, 8 replies; 33+ messages in thread
From: Tomasz Nowicki @ 2014-04-09 15:14 UTC (permalink / raw)
To: rjw, lenb, tony.luck, bp, bp, m.chehab
Cc: linux-edac, x86, linux-acpi, linux-kernel, linaro-acpi, 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. 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:
- replace arch specific calls with more generic one
- treat NMI notification as GHES feature - CONFIG_ACPI_APEI_NMI
- isolate NMI related code
- reorganize function logic
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 selected for x86 by default. Architectures that want to use this
feature should also provide NMI code infrastructure.
Tomasz Nowicki (7):
apei, mce: Call MCE-specific code only for X86 architecture.
acpi, apei, ghes: Introduce more generic mechanism to init/deinit
GHES error notifications.
ACPI, APEI, GHES: Introduce ACPI_NMI to make NMI error notification a
GHES feature.
acpi, apei, ghes: Factor out NMI error notification context.
acpi, apei, ghes: Attach NMI init/deinit functions while
CONFIG_ACPI_NMI is enabled.
acpi, apei, ghes: Make unmapping functionality independent from
architecture.
acpi, apei, ghes: Factor out ioremap virtual memory for IRQ and NMI
context.
drivers/acpi/apei/Kconfig | 10 +-
drivers/acpi/apei/ghes.c | 329 ++++++++++++++++++++++++++++-----------------
drivers/acpi/apei/hest.c | 8 +-
3 files changed, 218 insertions(+), 129 deletions(-)
--
1.7.9.5
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture.
2014-04-09 15:14 [PATCH 0/7] APEI: Make APEI architecture independent Tomasz Nowicki
@ 2014-04-09 15:14 ` Tomasz Nowicki
2014-05-05 11:44 ` Borislav Petkov
2014-04-09 15:14 ` [PATCH 2/7] acpi, apei, ghes: Introduce more generic mechanism to init/deinit GHES error notifications Tomasz Nowicki
` (6 subsequent siblings)
7 siblings, 1 reply; 33+ messages in thread
From: Tomasz Nowicki @ 2014-04-09 15:14 UTC (permalink / raw)
To: rjw, lenb, tony.luck, bp, bp, m.chehab
Cc: linux-edac, x86, linux-acpi, linux-kernel, linaro-acpi, Tomasz Nowicki
This commit is dealing with MCE code in:
- hest.c
Move acpi_disable_cmcff flag to hest_parse_cmc() and makes
that depend on CONFIG_X86_MCE so that we do not have to maintain
acpi_disable_cmcff for architectures which do not support MCE.
Also, wrap architectural MCE header inside #ifdef CONFIG_X86_MCE.
- ghes.c
Wrap architectural MCE header inside #ifdef CONFIG_X86_MCE similar to rest
of the MCE code in this file.
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
drivers/acpi/apei/ghes.c | 2 ++
drivers/acpi/apei/hest.c | 8 ++++++--
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index dab7cb7..f7edffc 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -49,7 +49,9 @@
#include <linux/aer.h>
#include <acpi/ghes.h>
+#ifdef CONFIG_X86_MCE
#include <asm/mce.h>
+#endif
#include <asm/tlbflush.h>
#include <asm/nmi.h>
diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index f5e37f3..98db702 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -36,7 +36,9 @@
#include <linux/io.h>
#include <linux/platform_device.h>
#include <acpi/apei.h>
+#ifdef CONFIG_X86_MCE
#include <asm/mce.h>
+#endif
#include "apei-internal.h"
@@ -133,6 +135,9 @@ static int __init hest_parse_cmc(struct acpi_hest_header *hest_hdr, void *data)
struct acpi_hest_ia_corrected *cmc;
struct acpi_hest_ia_error_bank *mc_bank;
+ if (acpi_disable_cmcff)
+ return 1;
+
if (hest_hdr->type != ACPI_HEST_TYPE_IA32_CORRECTED_CHECK)
return 0;
@@ -263,8 +268,7 @@ void __init acpi_hest_init(void)
goto err;
}
- if (!acpi_disable_cmcff)
- apei_hest_parse(hest_parse_cmc, NULL);
+ apei_hest_parse(hest_parse_cmc, NULL);
if (!ghes_disable) {
rc = apei_hest_parse(hest_parse_ghes_count, &ghes_count);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 2/7] acpi, apei, ghes: Introduce more generic mechanism to init/deinit GHES error notifications.
2014-04-09 15:14 [PATCH 0/7] APEI: Make APEI architecture independent Tomasz Nowicki
2014-04-09 15:14 ` [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture Tomasz Nowicki
@ 2014-04-09 15:14 ` Tomasz Nowicki
2014-05-13 18:13 ` Borislav Petkov
2014-04-09 15:14 ` [PATCH 3/7] ACPI, APEI, GHES: Introduce ACPI_APEI_NMI to make NMI error notification a GHES feature Tomasz Nowicki
` (5 subsequent siblings)
7 siblings, 1 reply; 33+ messages in thread
From: Tomasz Nowicki @ 2014-04-09 15:14 UTC (permalink / raw)
To: rjw, lenb, tony.luck, bp, bp, m.chehab
Cc: linux-edac, x86, linux-acpi, linux-kernel, linaro-acpi, 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 can avoid #ifdef usage in common code e.g. for NMI which is currently
supported by x86.
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
drivers/acpi/apei/ghes.c | 242 ++++++++++++++++++++++++++++------------------
1 file changed, 149 insertions(+), 93 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index f7edffc..ca8387e 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -148,6 +148,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,
@@ -879,10 +887,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)
{
@@ -898,33 +902,151 @@ static unsigned long ghes_esource_prealloc_size(
return prealloc_size;
}
+static int ghes_notify_init_nmi(struct ghes *ghes)
+{
+ unsigned long len;
+
+ len = ghes_esource_prealloc_size(ghes->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);
+
+ return 0;
+}
+
+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 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",
+ ghes_notify_init_nmi,
+ ghes_notify_remove_nmi},
+ [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;
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);
- goto err;
- default:
- pr_warning(FW_WARN GHES_PFX "Unknown notification type: %u for generic hardware error source: %d\n",
+ if (generic->notify.type >= ACPI_HEST_NOTIFY_RESERVED) {
+ pr_warning(FW_WARN GHES_PFX "Unknown notification type: %u for "
+ "generic hardware error source: %d\n",
generic->notify.type, generic->header.source_id);
goto err;
}
+ ghes_init_call = ghes_notify_tab[generic->notify.type].init_call;
+ ghes_notif_name = ghes_notify_tab[generic->notify.type].notif_name;
+ if (!ghes_init_call) {
+ pr_warning(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)) {
@@ -944,48 +1066,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;
@@ -1003,44 +1087,16 @@ 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);
ghes = platform_get_drvdata(ghes_dev);
+ ghes->flags |= GHES_EXITING;
+
generic = ghes->generic;
+ ghes_remove_call = ghes_notify_tab[generic->notify.type].remove_call;
- 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;
- }
+ if (ghes_remove_call)
+ (*ghes_remove_call)(ghes);
ghes_fini(ghes);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 3/7] ACPI, APEI, GHES: Introduce ACPI_APEI_NMI to make NMI error notification a GHES feature.
2014-04-09 15:14 [PATCH 0/7] APEI: Make APEI architecture independent Tomasz Nowicki
2014-04-09 15:14 ` [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture Tomasz Nowicki
2014-04-09 15:14 ` [PATCH 2/7] acpi, apei, ghes: Introduce more generic mechanism to init/deinit GHES error notifications Tomasz Nowicki
@ 2014-04-09 15:14 ` Tomasz Nowicki
2014-04-09 15:14 ` [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context Tomasz Nowicki
` (4 subsequent siblings)
7 siblings, 0 replies; 33+ messages in thread
From: Tomasz Nowicki @ 2014-04-09 15:14 UTC (permalink / raw)
To: rjw, lenb, tony.luck, bp, bp, m.chehab
Cc: linux-edac, x86, linux-acpi, linux-kernel, linaro-acpi, 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 ACPI_APEI_NMI which will be used in next patches to isolate
NMI related code in ghes.c file. Only NMI error notification feature
depends on x86 and it is selected by default for x86 arch.
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
drivers/acpi/apei/Kconfig | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
index c4dac71..3ae248a 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
@@ -16,6 +15,7 @@ config ACPI_APEI_GHES
select ACPI_HED
select IRQ_WORK
select GENERIC_ALLOCATOR
+ select ACPI_APEI_NMI if X86
help
Generic Hardware Error Source provides a way to report
platform hardware errors (such as that from chipset). It
@@ -26,6 +26,14 @@ config ACPI_APEI_GHES
by firmware to produce more valuable hardware error
information for Linux.
+config ACPI_APEI_NMI
+ bool "NMI error notification support"
+ depends on ACPI_APEI_GHES
+ 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context.
2014-04-09 15:14 [PATCH 0/7] APEI: Make APEI architecture independent Tomasz Nowicki
` (2 preceding siblings ...)
2014-04-09 15:14 ` [PATCH 3/7] ACPI, APEI, GHES: Introduce ACPI_APEI_NMI to make NMI error notification a GHES feature Tomasz Nowicki
@ 2014-04-09 15:14 ` Tomasz Nowicki
2014-05-13 19:41 ` Borislav Petkov
2014-04-09 15:14 ` [PATCH 5/7] acpi, apei, ghes: Attach NMI init/deinit functions while CONFIG_ACPI_APEI_NMI is enabled Tomasz Nowicki
` (3 subsequent siblings)
7 siblings, 1 reply; 33+ messages in thread
From: Tomasz Nowicki @ 2014-04-09 15:14 UTC (permalink / raw)
To: rjw, lenb, tony.luck, bp, bp, m.chehab
Cc: linux-edac, x86, linux-acpi, linux-kernel, linaro-acpi, Tomasz Nowicki
Use CONFIG_ACPI_APEI_NMI to isolate NMI error notification path. NMI related
data and functions are grouped so they can be wrapped inside one
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
drivers/acpi/apei/ghes.c | 54 +++++++++++++++++++++++++---------------------
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index ca8387e..7a0d66e 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -53,7 +53,9 @@
#include <asm/mce.h>
#endif
#include <asm/tlbflush.h>
+#ifdef CONFIG_ACPI_APEI_NMI
#include <asm/nmi.h>
+#endif
#include "apei-internal.h"
@@ -88,8 +90,6 @@
bool ghes_disable;
module_param_named(disable, ghes_disable, bool, 0);
-static int ghes_panic_timeout __read_mostly = 30;
-
/*
* All error sources notified with SCI shares one notifier function,
* so they need to be linked and checked one by one. This is applied
@@ -99,16 +99,9 @@ static int ghes_panic_timeout __read_mostly = 30;
* list changing, not for traversing.
*/
static LIST_HEAD(ghes_sci);
-static LIST_HEAD(ghes_nmi);
static DEFINE_MUTEX(ghes_list_mutex);
/*
- * NMI may be triggered on any CPU, so ghes_nmi_lock is used for
- * mutual exclusion.
- */
-static DEFINE_RAW_SPINLOCK(ghes_nmi_lock);
-
-/*
* Because the memory area used to transfer hardware error information
* from BIOS to Linux can be determined only in NMI, IRQ or timer
* handler, but general ioremap can not be used in atomic context, so
@@ -132,18 +125,8 @@ static struct vm_struct *ghes_ioremap_area;
static DEFINE_RAW_SPINLOCK(ghes_ioremap_lock_nmi);
static DEFINE_SPINLOCK(ghes_ioremap_lock_irq);
-/*
- * printk is not safe in NMI context. So in NMI handler, we allocate
- * required memory from lock-less memory allocator
- * (ghes_estatus_pool), save estatus into it, put them into lock-less
- * list (ghes_estatus_llist), then delay printk into IRQ context via
- * irq_work (ghes_proc_irq_work). ghes_estatus_size_request record
- * required pool size by all NMI error source.
- */
static struct gen_pool *ghes_estatus_pool;
static unsigned long ghes_estatus_pool_size_request;
-static struct llist_head ghes_estatus_llist;
-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;
@@ -259,11 +242,6 @@ static int ghes_estatus_pool_expand(unsigned long len)
return 0;
}
-static void ghes_estatus_pool_shrink(unsigned long len)
-{
- ghes_estatus_pool_size_request -= PAGE_ALIGN(len);
-}
-
static struct ghes *ghes_new(struct acpi_hest_generic *generic)
{
struct ghes *ghes;
@@ -744,6 +722,28 @@ static int ghes_notify_sci(struct notifier_block *this,
return ret;
}
+#ifdef CONFIG_ACPI_APEI_NMI
+/*
+ * printk is not safe in NMI context. So in NMI handler, we allocate
+ * required memory from lock-less memory allocator
+ * (ghes_estatus_pool), save estatus into it, put them into lock-less
+ * list (ghes_estatus_llist), then delay printk into IRQ context via
+ * irq_work (ghes_proc_irq_work). ghes_estatus_size_request record
+ * required pool size by all NMI error source.
+ */
+static struct llist_head ghes_estatus_llist;
+static struct irq_work ghes_proc_irq_work;
+
+/*
+ * NMI may be triggered on any CPU, so ghes_nmi_lock is used for
+ * mutual exclusion.
+ */
+static DEFINE_RAW_SPINLOCK(ghes_nmi_lock);
+
+static LIST_HEAD(ghes_nmi);
+
+static int ghes_panic_timeout __read_mostly = 30;
+
static struct llist_node *llist_nodes_reverse(struct llist_node *llnode)
{
struct llist_node *next, *tail = NULL;
@@ -902,6 +902,12 @@ static unsigned long ghes_esource_prealloc_size(
return prealloc_size;
}
+static void ghes_estatus_pool_shrink(unsigned long len)
+{
+ ghes_estatus_pool_size_request -= PAGE_ALIGN(len);
+}
+#endif
+
static int ghes_notify_init_nmi(struct ghes *ghes)
{
unsigned long len;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 5/7] acpi, apei, ghes: Attach NMI init/deinit functions while CONFIG_ACPI_APEI_NMI is enabled.
2014-04-09 15:14 [PATCH 0/7] APEI: Make APEI architecture independent Tomasz Nowicki
` (3 preceding siblings ...)
2014-04-09 15:14 ` [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context Tomasz Nowicki
@ 2014-04-09 15:14 ` Tomasz Nowicki
2014-05-13 19:49 ` Borislav Petkov
2014-04-09 15:14 ` [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture Tomasz Nowicki
` (2 subsequent siblings)
7 siblings, 1 reply; 33+ messages in thread
From: Tomasz Nowicki @ 2014-04-09 15:14 UTC (permalink / raw)
To: rjw, lenb, tony.luck, bp, bp, m.chehab
Cc: linux-edac, x86, linux-acpi, linux-kernel, linaro-acpi, Tomasz Nowicki
Thanks to more generic way of init/deinit error notification, we can
register NMI related calls in runtime. It happens before walking through
GHES entries, so probe function will treat NMI as supported.
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
drivers/acpi/apei/ghes.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 7a0d66e..aaf8db3 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -906,7 +906,6 @@ static void ghes_estatus_pool_shrink(unsigned long len)
{
ghes_estatus_pool_size_request -= PAGE_ALIGN(len);
}
-#endif
static int ghes_notify_init_nmi(struct ghes *ghes)
{
@@ -941,6 +940,20 @@ static void ghes_notify_remove_nmi(struct ghes *ghes)
ghes_estatus_pool_shrink(len);
}
+static void ghes_init_nmi(void)
+{
+ 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;
+
+}
+#else
+inline static void ghes_init_nmi(void)
+{
+
+}
+#endif
+
static int ghes_notify_init_polled(struct ghes *ghes)
{
ghes->timer.function = ghes_poll_func;
@@ -1018,9 +1031,7 @@ static struct ghes_notify_setup
[ACPI_HEST_NOTIFY_SCI] = {"SCI",
ghes_notify_init_sci,
ghes_notify_remove_sci},
- [ACPI_HEST_NOTIFY_NMI] = {"NMI",
- ghes_notify_init_nmi,
- ghes_notify_remove_nmi},
+ [ACPI_HEST_NOTIFY_NMI] = {"NMI", NULL, NULL},
[ACPI_HEST_NOTIFY_CMCI] = {"CMCI", NULL, NULL},
[ACPI_HEST_NOTIFY_MCE] = {"MCE", NULL, NULL},
};
@@ -1141,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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.
2014-04-09 15:14 [PATCH 0/7] APEI: Make APEI architecture independent Tomasz Nowicki
` (4 preceding siblings ...)
2014-04-09 15:14 ` [PATCH 5/7] acpi, apei, ghes: Attach NMI init/deinit functions while CONFIG_ACPI_APEI_NMI is enabled Tomasz Nowicki
@ 2014-04-09 15:14 ` Tomasz Nowicki
2014-05-13 20:11 ` Borislav Petkov
2014-04-09 15:14 ` [PATCH 7/7] acpi, apei, ghes: Factor out ioremap virtual memory for IRQ and NMI context Tomasz Nowicki
2014-05-05 9:25 ` [PATCH 0/7] APEI: Make APEI architecture independent Tomasz Nowicki
7 siblings, 1 reply; 33+ messages in thread
From: Tomasz Nowicki @ 2014-04-09 15:14 UTC (permalink / raw)
To: rjw, lenb, tony.luck, bp, bp, m.chehab
Cc: linux-edac, x86, linux-acpi, linux-kernel, linaro-acpi, Tomasz Nowicki
Till now __flush_tlb_one was used for unmapping virtual memory which
is x86 specific function. Replace it with more generic
flush_tlb_kernel_range.
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
drivers/acpi/apei/ghes.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index aaf8db3..624878b 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -185,7 +185,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);
+ flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
}
static void ghes_iounmap_irq(void __iomem *vaddr_ptr)
@@ -195,7 +195,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);
+ flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
}
static int ghes_estatus_pool_init(void)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 7/7] acpi, apei, ghes: Factor out ioremap virtual memory for IRQ and NMI context.
2014-04-09 15:14 [PATCH 0/7] APEI: Make APEI architecture independent Tomasz Nowicki
` (5 preceding siblings ...)
2014-04-09 15:14 ` [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture Tomasz Nowicki
@ 2014-04-09 15:14 ` Tomasz Nowicki
2014-05-14 17:13 ` Borislav Petkov
2014-05-05 9:25 ` [PATCH 0/7] APEI: Make APEI architecture independent Tomasz Nowicki
7 siblings, 1 reply; 33+ messages in thread
From: Tomasz Nowicki @ 2014-04-09 15:14 UTC (permalink / raw)
To: rjw, lenb, tony.luck, bp, bp, m.chehab
Cc: linux-edac, x86, linux-acpi, linux-kernel, linaro-acpi, 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 CONFIG_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>
---
drivers/acpi/apei/ghes.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 624878b..20377ac 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -109,12 +109,11 @@ static DEFINE_MUTEX(ghes_list_mutex);
*/
/*
- * 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;
@@ -141,7 +140,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(CONFIG_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");
@@ -310,6 +310,8 @@ static void ghes_copy_tofrom_phys(void *buffer, u64 paddr, u32 len,
u64 offset;
u32 trunk;
+ BUG_ON(in_nmi && !IS_ENABLED(CONFIG_ACPI_APEI_NMI));
+
while (len > 0) {
offset = paddr - (paddr & PAGE_MASK);
if (in_nmi) {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 0/7] APEI: Make APEI architecture independent.
2014-04-09 15:14 [PATCH 0/7] APEI: Make APEI architecture independent Tomasz Nowicki
` (6 preceding siblings ...)
2014-04-09 15:14 ` [PATCH 7/7] acpi, apei, ghes: Factor out ioremap virtual memory for IRQ and NMI context Tomasz Nowicki
@ 2014-05-05 9:25 ` Tomasz Nowicki
7 siblings, 0 replies; 33+ messages in thread
From: Tomasz Nowicki @ 2014-05-05 9:25 UTC (permalink / raw)
To: rjw, lenb, tony.luck, bp, bp, m.chehab
Cc: linux-edac, x86, linux-acpi, linux-kernel, linaro-acpi
Hi,
I would like to kindly remind and request for some comments on this patches.
Regards,
Tomasz
On 09.04.2014 17:14, Tomasz Nowicki wrote:
> 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. 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:
> - replace arch specific calls with more generic one
> - treat NMI notification as GHES feature - CONFIG_ACPI_APEI_NMI
> - isolate NMI related code
> - reorganize function logic
>
> 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 selected for x86 by default. Architectures that want to use this
> feature should also provide NMI code infrastructure.
>
> Tomasz Nowicki (7):
> apei, mce: Call MCE-specific code only for X86 architecture.
> acpi, apei, ghes: Introduce more generic mechanism to init/deinit
> GHES error notifications.
> ACPI, APEI, GHES: Introduce ACPI_NMI to make NMI error notification a
> GHES feature.
> acpi, apei, ghes: Factor out NMI error notification context.
> acpi, apei, ghes: Attach NMI init/deinit functions while
> CONFIG_ACPI_NMI is enabled.
> acpi, apei, ghes: Make unmapping functionality independent from
> architecture.
> acpi, apei, ghes: Factor out ioremap virtual memory for IRQ and NMI
> context.
>
> drivers/acpi/apei/Kconfig | 10 +-
> drivers/acpi/apei/ghes.c | 329 ++++++++++++++++++++++++++++-----------------
> drivers/acpi/apei/hest.c | 8 +-
> 3 files changed, 218 insertions(+), 129 deletions(-)
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture.
2014-04-09 15:14 ` [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture Tomasz Nowicki
@ 2014-05-05 11:44 ` Borislav Petkov
2014-05-05 14:34 ` Tomasz Nowicki
0 siblings, 1 reply; 33+ messages in thread
From: Borislav Petkov @ 2014-05-05 11:44 UTC (permalink / raw)
To: Tomasz Nowicki
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On Wed, Apr 09, 2014 at 05:14:29PM +0200, Tomasz Nowicki wrote:
> This commit is dealing with MCE code in:
> - hest.c
> Move acpi_disable_cmcff flag to hest_parse_cmc() and makes
> that depend on CONFIG_X86_MCE so that we do not have to maintain
> acpi_disable_cmcff for architectures which do not support MCE.
> Also, wrap architectural MCE header inside #ifdef CONFIG_X86_MCE.
>
> - ghes.c
> Wrap architectural MCE header inside #ifdef CONFIG_X86_MCE similar to rest
> of the MCE code in this file.
>
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> ---
> drivers/acpi/apei/ghes.c | 2 ++
> drivers/acpi/apei/hest.c | 8 ++++++--
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index dab7cb7..f7edffc 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -49,7 +49,9 @@
> #include <linux/aer.h>
>
> #include <acpi/ghes.h>
> +#ifdef CONFIG_X86_MCE
> #include <asm/mce.h>
> +#endif
> #include <asm/tlbflush.h>
> #include <asm/nmi.h>
>
> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
> index f5e37f3..98db702 100644
> --- a/drivers/acpi/apei/hest.c
> +++ b/drivers/acpi/apei/hest.c
> @@ -36,7 +36,9 @@
> #include <linux/io.h>
> #include <linux/platform_device.h>
> #include <acpi/apei.h>
> +#ifdef CONFIG_X86_MCE
> #include <asm/mce.h>
> +#endif
Actually, I would prefer if you wrapped all the arch-specific calls into
arch-specific functions, say, convert
apei_mce_report_mem_error -> apei_arch_report_mem_error
and have default empty functions for arches which don't use that
functionality.
This way you can save yourself the ugly ifdeffery around the place.
>
> #include "apei-internal.h"
>
> @@ -133,6 +135,9 @@ static int __init hest_parse_cmc(struct acpi_hest_header *hest_hdr, void *data)
> struct acpi_hest_ia_corrected *cmc;
> struct acpi_hest_ia_error_bank *mc_bank;
>
> + if (acpi_disable_cmcff)
> + return 1;
This could be
if (arch_disable_cmcff())
return 1;
with the default stub being
static inline bool arch_disable_cmcff(void)
{
return false;
}
and so on, like it is done in many other places in the kernel.
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture.
2014-05-05 11:44 ` Borislav Petkov
@ 2014-05-05 14:34 ` Tomasz Nowicki
2014-05-05 14:53 ` Borislav Petkov
0 siblings, 1 reply; 33+ messages in thread
From: Tomasz Nowicki @ 2014-05-05 14:34 UTC (permalink / raw)
To: Borislav Petkov
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On 05.05.2014 13:44, Borislav Petkov wrote:
> On Wed, Apr 09, 2014 at 05:14:29PM +0200, Tomasz Nowicki wrote:
>> This commit is dealing with MCE code in:
>> - hest.c
>> Move acpi_disable_cmcff flag to hest_parse_cmc() and makes
>> that depend on CONFIG_X86_MCE so that we do not have to maintain
>> acpi_disable_cmcff for architectures which do not support MCE.
>> Also, wrap architectural MCE header inside #ifdef CONFIG_X86_MCE.
>>
>> - ghes.c
>> Wrap architectural MCE header inside #ifdef CONFIG_X86_MCE similar to rest
>> of the MCE code in this file.
>>
>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>> ---
>> drivers/acpi/apei/ghes.c | 2 ++
>> drivers/acpi/apei/hest.c | 8 ++++++--
>> 2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index dab7cb7..f7edffc 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -49,7 +49,9 @@
>> #include <linux/aer.h>
>>
>> #include <acpi/ghes.h>
>> +#ifdef CONFIG_X86_MCE
>> #include <asm/mce.h>
>> +#endif
>> #include <asm/tlbflush.h>
>> #include <asm/nmi.h>
>>
>> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
>> index f5e37f3..98db702 100644
>> --- a/drivers/acpi/apei/hest.c
>> +++ b/drivers/acpi/apei/hest.c
>> @@ -36,7 +36,9 @@
>> #include <linux/io.h>
>> #include <linux/platform_device.h>
>> #include <acpi/apei.h>
>> +#ifdef CONFIG_X86_MCE
>> #include <asm/mce.h>
>> +#endif
>
> Actually, I would prefer if you wrapped all the arch-specific calls into
> arch-specific functions, say, convert
>
> apei_mce_report_mem_error -> apei_arch_report_mem_error
>
> and have default empty functions for arches which don't use that
> functionality.
>
> This way you can save yourself the ugly ifdeffery around the place.
>
True, this can be improved as you suggested.
>>
>> #include "apei-internal.h"
>>
>> @@ -133,6 +135,9 @@ static int __init hest_parse_cmc(struct acpi_hest_header *hest_hdr, void *data)
>> struct acpi_hest_ia_corrected *cmc;
>> struct acpi_hest_ia_error_bank *mc_bank;
>>
>> + if (acpi_disable_cmcff)
>> + return 1;
>
> This could be
>
> if (arch_disable_cmcff())
> return 1;
>
> with the default stub being
>
> static inline bool arch_disable_cmcff(void)
> {
> return false;
> }
>
> and so on, like it is done in many other places in the kernel.
acpi_disable_cmcff as global value can switch off/on MC entries
analysing via kernel args. This glob value resides in x86 ACPI code and
has meaning only for MCE related mechanism, that is why I have moved it
under hest_parse_cmc.
Thanks.
Tomasz
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture.
2014-05-05 14:34 ` Tomasz Nowicki
@ 2014-05-05 14:53 ` Borislav Petkov
2014-05-05 15:32 ` Tomasz Nowicki
0 siblings, 1 reply; 33+ messages in thread
From: Borislav Petkov @ 2014-05-05 14:53 UTC (permalink / raw)
To: Tomasz Nowicki
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On Mon, May 05, 2014 at 04:34:41PM +0200, Tomasz Nowicki wrote:
> acpi_disable_cmcff as global value can switch off/on MC entries
> analysing via kernel args.
No, it switches off firmware first mode for correctable errors because
of buggy BIOSes - 9ad95879cd1b2 (what else...)
> This glob value resides in x86 ACPI code and has meaning only for MCE
> related mechanism,
Of course it doesn't!
> that is why I have moved it under hest_parse_cmc.
See APEI section in the ACPI spec "18.4 Firmware First Error Handling."
Regardless of what your version of APEI does, you actually shouldn't
need to touch acpi_disable_cmcff at all as it not arch-specific.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture.
2014-05-05 14:53 ` Borislav Petkov
@ 2014-05-05 15:32 ` Tomasz Nowicki
2014-05-05 15:33 ` Borislav Petkov
0 siblings, 1 reply; 33+ messages in thread
From: Tomasz Nowicki @ 2014-05-05 15:32 UTC (permalink / raw)
To: Borislav Petkov
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On 05.05.2014 16:53, Borislav Petkov wrote:
> On Mon, May 05, 2014 at 04:34:41PM +0200, Tomasz Nowicki wrote:
>> acpi_disable_cmcff as global value can switch off/on MC entries
>> analysing via kernel args.
>
> No, it switches off firmware first mode for correctable errors because
> of buggy BIOSes - 9ad95879cd1b2 (what else...)
>
>> This glob value resides in x86 ACPI code and has meaning only for MCE
>> related mechanism,
>
> Of course it doesn't!
>
>> that is why I have moved it under hest_parse_cmc.
>
> See APEI section in the ACPI spec "18.4 Firmware First Error Handling."
>
> Regardless of what your version of APEI does, you actually shouldn't
> need to touch acpi_disable_cmcff at all as it not arch-specific.
>
You are right! I will fix that big misunderstanding from my side in next
patch version.
Thanks,
Tomasz
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture.
2014-05-05 15:32 ` Tomasz Nowicki
@ 2014-05-05 15:33 ` Borislav Petkov
2014-05-05 15:36 ` Tomasz Nowicki
0 siblings, 1 reply; 33+ messages in thread
From: Borislav Petkov @ 2014-05-05 15:33 UTC (permalink / raw)
To: Tomasz Nowicki
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On Mon, May 05, 2014 at 05:32:07PM +0200, Tomasz Nowicki wrote:
> You are right! I will fix that big misunderstanding from my side in
> next patch version.
Ok, but pls wait with resubmitting until I take a look at the rest of
your patches.
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture.
2014-05-05 15:33 ` Borislav Petkov
@ 2014-05-05 15:36 ` Tomasz Nowicki
0 siblings, 0 replies; 33+ messages in thread
From: Tomasz Nowicki @ 2014-05-05 15:36 UTC (permalink / raw)
To: Borislav Petkov
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On 05.05.2014 17:33, Borislav Petkov wrote:
> On Mon, May 05, 2014 at 05:32:07PM +0200, Tomasz Nowicki wrote:
>> You are right! I will fix that big misunderstanding from my side in
>> next patch version.
>
> Ok, but pls wait with resubmitting until I take a look at the rest of
> your patches.
Sure, I will.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/7] acpi, apei, ghes: Introduce more generic mechanism to init/deinit GHES error notifications.
2014-04-09 15:14 ` [PATCH 2/7] acpi, apei, ghes: Introduce more generic mechanism to init/deinit GHES error notifications Tomasz Nowicki
@ 2014-05-13 18:13 ` Borislav Petkov
2014-05-15 14:31 ` Tomasz Nowicki
0 siblings, 1 reply; 33+ messages in thread
From: Borislav Petkov @ 2014-05-13 18:13 UTC (permalink / raw)
To: Tomasz Nowicki
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On Wed, Apr 09, 2014 at 05:14:30PM +0200, Tomasz Nowicki wrote:
> 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 can avoid #ifdef usage in common code e.g. for NMI which is currently
> supported by x86.
>
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> ---
> drivers/acpi/apei/ghes.c | 242 ++++++++++++++++++++++++++++------------------
> 1 file changed, 149 insertions(+), 93 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index f7edffc..ca8387e 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -148,6 +148,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);
> +};
Please run through checkpatch before submitting:
WARNING: please, no space before tabs
#44: FILE: drivers/acpi/apei/ghes.c:153:
+^Iint ^I (*init_call)(struct ghes *ghes);$
WARNING: please, no space before tabs
#45: FILE: drivers/acpi/apei/ghes.c:154:
+^Ivoid ^I (*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,
> @@ -879,10 +887,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)
> {
> @@ -898,33 +902,151 @@ static unsigned long ghes_esource_prealloc_size(
> return prealloc_size;
> }
>
> +static int ghes_notify_init_nmi(struct ghes *ghes)
> +{
> + unsigned long len;
> +
> + len = ghes_esource_prealloc_size(ghes->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);
> +
> + return 0;
Unconditionally returning 0 seems kinda moot. At least
register_nmi_handler() retval could be returned...
...
> +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",
> + ghes_notify_init_nmi,
> + ghes_notify_remove_nmi},
> + [ACPI_HEST_NOTIFY_CMCI] = {"CMCI", NULL, NULL},
> + [ACPI_HEST_NOTIFY_MCE] = {"MCE", NULL, NULL},
> +};
I guess you don't need to save the ->notif_name strings but add
a function which maps ACPI_HEST_* to strings => smaller struct
ghes_notify_setup.
> @@ -944,48 +1066,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();
What happened to those BUG() catch-all stoppers?
> - }
> + rc = (*ghes_init_call)(ghes);
> + if (rc)
> + goto err_edac_unreg;
> +
> platform_set_drvdata(ghes_dev, ghes);
>
> return 0;
> @@ -1003,44 +1087,16 @@ 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);
>
> ghes = platform_get_drvdata(ghes_dev);
> + ghes->flags |= GHES_EXITING;
> +
> generic = ghes->generic;
> + ghes_remove_call = ghes_notify_tab[generic->notify.type].remove_call;
>
> - 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;
This one too.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context.
2014-04-09 15:14 ` [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context Tomasz Nowicki
@ 2014-05-13 19:41 ` Borislav Petkov
2014-05-23 12:06 ` Tomasz Nowicki
0 siblings, 1 reply; 33+ messages in thread
From: Borislav Petkov @ 2014-05-13 19:41 UTC (permalink / raw)
To: Tomasz Nowicki
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On Wed, Apr 09, 2014 at 05:14:32PM +0200, Tomasz Nowicki wrote:
> Use CONFIG_ACPI_APEI_NMI to isolate NMI error notification path. NMI related
> data and functions are grouped so they can be wrapped inside one
>
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> ---
> drivers/acpi/apei/ghes.c | 54 +++++++++++++++++++++++++---------------------
> 1 file changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index ca8387e..7a0d66e 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -53,7 +53,9 @@
> #include <asm/mce.h>
> #endif
> #include <asm/tlbflush.h>
> +#ifdef CONFIG_ACPI_APEI_NMI
> #include <asm/nmi.h>
> +#endif
This, again, can be avoided with adding arch-specific versions and empty
default stubs.
> #include "apei-internal.h"
...
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 5/7] acpi, apei, ghes: Attach NMI init/deinit functions while CONFIG_ACPI_APEI_NMI is enabled.
2014-04-09 15:14 ` [PATCH 5/7] acpi, apei, ghes: Attach NMI init/deinit functions while CONFIG_ACPI_APEI_NMI is enabled Tomasz Nowicki
@ 2014-05-13 19:49 ` Borislav Petkov
0 siblings, 0 replies; 33+ messages in thread
From: Borislav Petkov @ 2014-05-13 19:49 UTC (permalink / raw)
To: Tomasz Nowicki
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On Wed, Apr 09, 2014 at 05:14:33PM +0200, Tomasz Nowicki wrote:
> Thanks to more generic way of init/deinit error notification, we can
> register NMI related calls in runtime. It happens before walking through
> GHES entries, so probe function will treat NMI as supported.
And we do this special handling which could confuse people because... ?
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> ---
> drivers/acpi/apei/ghes.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 7a0d66e..aaf8db3 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -906,7 +906,6 @@ static void ghes_estatus_pool_shrink(unsigned long len)
> {
> ghes_estatus_pool_size_request -= PAGE_ALIGN(len);
> }
> -#endif
>
> static int ghes_notify_init_nmi(struct ghes *ghes)
> {
> @@ -941,6 +940,20 @@ static void ghes_notify_remove_nmi(struct ghes *ghes)
> ghes_estatus_pool_shrink(len);
> }
>
> +static void ghes_init_nmi(void)
> +{
> + 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;
> +
> +}
> +#else
> +inline static void ghes_init_nmi(void)
> +{
> +
> +}
> +#endif
WARNING: storage class should be at the beginning of the declaration
#51: FILE: drivers/acpi/apei/ghes.c:951:
+inline static void ghes_init_nmi(void)
ERROR: inline keyword should sit between storage class and type
#51: FILE: drivers/acpi/apei/ghes.c:951:
+inline static void ghes_init_nmi(void)
i.e..
static inline void ...
> +
> static int ghes_notify_init_polled(struct ghes *ghes)
> {
> ghes->timer.function = ghes_poll_func;
> @@ -1018,9 +1031,7 @@ static struct ghes_notify_setup
> [ACPI_HEST_NOTIFY_SCI] = {"SCI",
> ghes_notify_init_sci,
> ghes_notify_remove_sci},
> - [ACPI_HEST_NOTIFY_NMI] = {"NMI",
> - ghes_notify_init_nmi,
> - ghes_notify_remove_nmi},
> + [ACPI_HEST_NOTIFY_NMI] = {"NMI", NULL, NULL},
> [ACPI_HEST_NOTIFY_CMCI] = {"CMCI", NULL, NULL},
> [ACPI_HEST_NOTIFY_MCE] = {"MCE", NULL, NULL},
> };
> @@ -1141,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
>
>
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.
2014-04-09 15:14 ` [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture Tomasz Nowicki
@ 2014-05-13 20:11 ` Borislav Petkov
2014-05-14 12:32 ` Tomasz Nowicki
0 siblings, 1 reply; 33+ messages in thread
From: Borislav Petkov @ 2014-05-13 20:11 UTC (permalink / raw)
To: Tomasz Nowicki
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On Wed, Apr 09, 2014 at 05:14:34PM +0200, Tomasz Nowicki wrote:
> Till now __flush_tlb_one was used for unmapping virtual memory which
> is x86 specific function. Replace it with more generic
> flush_tlb_kernel_range.
>
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> ---
> drivers/acpi/apei/ghes.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index aaf8db3..624878b 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -185,7 +185,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);
> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
> }
>
> static void ghes_iounmap_irq(void __iomem *vaddr_ptr)
> @@ -195,7 +195,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);
> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
flush_tlb_kernel_range() does send an IPI to every core on x86 which is
much more expensive than what __flush_tlb_one does.
Fairer it would be if you added a __flush_tlb_one() version for arm
which does flush_tlb_kernel_range for you.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.
2014-05-13 20:11 ` Borislav Petkov
@ 2014-05-14 12:32 ` Tomasz Nowicki
2014-05-14 12:35 ` Will Deacon
0 siblings, 1 reply; 33+ messages in thread
From: Tomasz Nowicki @ 2014-05-14 12:32 UTC (permalink / raw)
To: Borislav Petkov, Catalin Marinas, Russell King - ARM Linux, Will Deacon
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On 13.05.2014 22:11, Borislav Petkov wrote:
> On Wed, Apr 09, 2014 at 05:14:34PM +0200, Tomasz Nowicki wrote:
>> Till now __flush_tlb_one was used for unmapping virtual memory which
>> is x86 specific function. Replace it with more generic
>> flush_tlb_kernel_range.
>>
>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>> ---
>> drivers/acpi/apei/ghes.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index aaf8db3..624878b 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -185,7 +185,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);
>> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
>> }
>>
>> static void ghes_iounmap_irq(void __iomem *vaddr_ptr)
>> @@ -195,7 +195,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);
>> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
>
> flush_tlb_kernel_range() does send an IPI to every core on x86 which is
> much more expensive than what __flush_tlb_one does.
>
> Fairer it would be if you added a __flush_tlb_one() version for arm
> which does flush_tlb_kernel_range for you.
>
Thanks for comment. I am not sure if maintainers will allow me to add
sth like __flush_tlb_one() for arm/arm64. Let me ask them directly.
Catalin, Russell what do you think?
Regards,
Tomasz
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.
2014-05-14 12:32 ` Tomasz Nowicki
@ 2014-05-14 12:35 ` Will Deacon
2014-05-14 12:45 ` Catalin Marinas
0 siblings, 1 reply; 33+ messages in thread
From: Will Deacon @ 2014-05-14 12:35 UTC (permalink / raw)
To: Tomasz Nowicki
Cc: Borislav Petkov, Catalin Marinas, Russell King - ARM Linux, rjw,
lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On Wed, May 14, 2014 at 01:32:27PM +0100, Tomasz Nowicki wrote:
> On 13.05.2014 22:11, Borislav Petkov wrote:
> > On Wed, Apr 09, 2014 at 05:14:34PM +0200, Tomasz Nowicki wrote:
> >> Till now __flush_tlb_one was used for unmapping virtual memory which
> >> is x86 specific function. Replace it with more generic
> >> flush_tlb_kernel_range.
> >>
> >> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> >> ---
> >> drivers/acpi/apei/ghes.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> >> index aaf8db3..624878b 100644
> >> --- a/drivers/acpi/apei/ghes.c
> >> +++ b/drivers/acpi/apei/ghes.c
> >> @@ -185,7 +185,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);
> >> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
> >> }
> >>
> >> static void ghes_iounmap_irq(void __iomem *vaddr_ptr)
> >> @@ -195,7 +195,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);
> >> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
> >
> > flush_tlb_kernel_range() does send an IPI to every core on x86 which is
> > much more expensive than what __flush_tlb_one does.
> >
> > Fairer it would be if you added a __flush_tlb_one() version for arm
> > which does flush_tlb_kernel_range for you.
> >
>
> Thanks for comment. I am not sure if maintainers will allow me to add
> sth like __flush_tlb_one() for arm/arm64. Let me ask them directly.
> Catalin, Russell what do you think?
I don't have the background for this, but if you don't need broadcasting
(if this avoids IPIs on x86, I guess you don't) then why not use
local_flush_tlb_kernel_range instead?
Will
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.
2014-05-14 12:35 ` Will Deacon
@ 2014-05-14 12:45 ` Catalin Marinas
2014-05-14 12:48 ` Will Deacon
0 siblings, 1 reply; 33+ messages in thread
From: Catalin Marinas @ 2014-05-14 12:45 UTC (permalink / raw)
To: Will Deacon
Cc: Tomasz Nowicki, Borislav Petkov, Russell King - ARM Linux, rjw,
lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On Wed, May 14, 2014 at 01:35:42PM +0100, Will Deacon wrote:
> On Wed, May 14, 2014 at 01:32:27PM +0100, Tomasz Nowicki wrote:
> > On 13.05.2014 22:11, Borislav Petkov wrote:
> > > On Wed, Apr 09, 2014 at 05:14:34PM +0200, Tomasz Nowicki wrote:
> > >> Till now __flush_tlb_one was used for unmapping virtual memory which
> > >> is x86 specific function. Replace it with more generic
> > >> flush_tlb_kernel_range.
> > >>
> > >> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> > >> ---
> > >> drivers/acpi/apei/ghes.c | 4 ++--
> > >> 1 file changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> > >> index aaf8db3..624878b 100644
> > >> --- a/drivers/acpi/apei/ghes.c
> > >> +++ b/drivers/acpi/apei/ghes.c
> > >> @@ -185,7 +185,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);
> > >> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
> > >> }
> > >>
> > >> static void ghes_iounmap_irq(void __iomem *vaddr_ptr)
> > >> @@ -195,7 +195,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);
> > >> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
> > >
> > > flush_tlb_kernel_range() does send an IPI to every core on x86 which is
> > > much more expensive than what __flush_tlb_one does.
> > >
> > > Fairer it would be if you added a __flush_tlb_one() version for arm
> > > which does flush_tlb_kernel_range for you.
> > >
> >
> > Thanks for comment. I am not sure if maintainers will allow me to add
> > sth like __flush_tlb_one() for arm/arm64. Let me ask them directly.
> > Catalin, Russell what do you think?
>
> I don't have the background for this, but if you don't need broadcasting
> (if this avoids IPIs on x86, I guess you don't) then why not use
> local_flush_tlb_kernel_range instead?
Is this generic enough (we don't have it on arm64)?
--
Catalin
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.
2014-05-14 12:45 ` Catalin Marinas
@ 2014-05-14 12:48 ` Will Deacon
2014-05-14 12:52 ` Tomasz Nowicki
0 siblings, 1 reply; 33+ messages in thread
From: Will Deacon @ 2014-05-14 12:48 UTC (permalink / raw)
To: Catalin Marinas
Cc: Tomasz Nowicki, Borislav Petkov, Russell King - ARM Linux, rjw,
lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On Wed, May 14, 2014 at 01:45:07PM +0100, Catalin Marinas wrote:
> On Wed, May 14, 2014 at 01:35:42PM +0100, Will Deacon wrote:
> > On Wed, May 14, 2014 at 01:32:27PM +0100, Tomasz Nowicki wrote:
> > > On 13.05.2014 22:11, Borislav Petkov wrote:
> > > > On Wed, Apr 09, 2014 at 05:14:34PM +0200, Tomasz Nowicki wrote:
> > > >> Till now __flush_tlb_one was used for unmapping virtual memory which
> > > >> is x86 specific function. Replace it with more generic
> > > >> flush_tlb_kernel_range.
> > > >>
> > > >> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> > > >> ---
> > > >> drivers/acpi/apei/ghes.c | 4 ++--
> > > >> 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >>
> > > >> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> > > >> index aaf8db3..624878b 100644
> > > >> --- a/drivers/acpi/apei/ghes.c
> > > >> +++ b/drivers/acpi/apei/ghes.c
> > > >> @@ -185,7 +185,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);
> > > >> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
> > > >> }
> > > >>
> > > >> static void ghes_iounmap_irq(void __iomem *vaddr_ptr)
> > > >> @@ -195,7 +195,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);
> > > >> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
> > > >
> > > > flush_tlb_kernel_range() does send an IPI to every core on x86 which is
> > > > much more expensive than what __flush_tlb_one does.
> > > >
> > > > Fairer it would be if you added a __flush_tlb_one() version for arm
> > > > which does flush_tlb_kernel_range for you.
> > > >
> > >
> > > Thanks for comment. I am not sure if maintainers will allow me to add
> > > sth like __flush_tlb_one() for arm/arm64. Let me ask them directly.
> > > Catalin, Russell what do you think?
> >
> > I don't have the background for this, but if you don't need broadcasting
> > (if this avoids IPIs on x86, I guess you don't) then why not use
> > local_flush_tlb_kernel_range instead?
>
> Is this generic enough (we don't have it on arm64)?
Well, it's more popular than __flush_tlb_one and the naming is more
descriptive imo.
Will
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.
2014-05-14 12:48 ` Will Deacon
@ 2014-05-14 12:52 ` Tomasz Nowicki
2014-05-14 13:21 ` Borislav Petkov
0 siblings, 1 reply; 33+ messages in thread
From: Tomasz Nowicki @ 2014-05-14 12:52 UTC (permalink / raw)
To: Will Deacon, Catalin Marinas
Cc: Borislav Petkov, Russell King - ARM Linux, rjw, lenb, tony.luck,
bp, m.chehab, linux-edac, x86, linux-acpi, linux-kernel,
linaro-acpi
On 14.05.2014 14:48, Will Deacon wrote:
> On Wed, May 14, 2014 at 01:45:07PM +0100, Catalin Marinas wrote:
>> On Wed, May 14, 2014 at 01:35:42PM +0100, Will Deacon wrote:
>>> On Wed, May 14, 2014 at 01:32:27PM +0100, Tomasz Nowicki wrote:
>>>> On 13.05.2014 22:11, Borislav Petkov wrote:
>>>>> On Wed, Apr 09, 2014 at 05:14:34PM +0200, Tomasz Nowicki wrote:
>>>>>> Till now __flush_tlb_one was used for unmapping virtual memory which
>>>>>> is x86 specific function. Replace it with more generic
>>>>>> flush_tlb_kernel_range.
>>>>>>
>>>>>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>>>>>> ---
>>>>>> drivers/acpi/apei/ghes.c | 4 ++--
>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>>>>>> index aaf8db3..624878b 100644
>>>>>> --- a/drivers/acpi/apei/ghes.c
>>>>>> +++ b/drivers/acpi/apei/ghes.c
>>>>>> @@ -185,7 +185,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);
>>>>>> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
>>>>>> }
>>>>>>
>>>>>> static void ghes_iounmap_irq(void __iomem *vaddr_ptr)
>>>>>> @@ -195,7 +195,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);
>>>>>> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
>>>>>
>>>>> flush_tlb_kernel_range() does send an IPI to every core on x86 which is
>>>>> much more expensive than what __flush_tlb_one does.
>>>>>
>>>>> Fairer it would be if you added a __flush_tlb_one() version for arm
>>>>> which does flush_tlb_kernel_range for you.
>>>>>
>>>>
>>>> Thanks for comment. I am not sure if maintainers will allow me to add
>>>> sth like __flush_tlb_one() for arm/arm64. Let me ask them directly.
>>>> Catalin, Russell what do you think?
>>>
>>> I don't have the background for this, but if you don't need broadcasting
>>> (if this avoids IPIs on x86, I guess you don't) then why not use
>>> local_flush_tlb_kernel_range instead?
>>
>> Is this generic enough (we don't have it on arm64)?
>
> Well, it's more popular than __flush_tlb_one and the naming is more
> descriptive imo.
I am aiming ARM64 but ideally it should work for x86, arm64 and arm.
Tomasz
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.
2014-05-14 12:52 ` Tomasz Nowicki
@ 2014-05-14 13:21 ` Borislav Petkov
0 siblings, 0 replies; 33+ messages in thread
From: Borislav Petkov @ 2014-05-14 13:21 UTC (permalink / raw)
To: Tomasz Nowicki
Cc: Will Deacon, Catalin Marinas, Russell King - ARM Linux, rjw,
lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On Wed, May 14, 2014 at 02:52:29PM +0200, Tomasz Nowicki wrote:
> I am aiming ARM64 but ideally it should work for x86, arm64 and arm.
I guess you can call it local_flush_tlb_one() which calls
__flush_tlb_one() on x86 and flush_tlb_kernel_range() on arm*
A bunch of other arches have local_flush_tlb_one so going with that
naming might be on the right track :)
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 7/7] acpi, apei, ghes: Factor out ioremap virtual memory for IRQ and NMI context.
2014-04-09 15:14 ` [PATCH 7/7] acpi, apei, ghes: Factor out ioremap virtual memory for IRQ and NMI context Tomasz Nowicki
@ 2014-05-14 17:13 ` Borislav Petkov
0 siblings, 0 replies; 33+ messages in thread
From: Borislav Petkov @ 2014-05-14 17:13 UTC (permalink / raw)
To: Tomasz Nowicki
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On Wed, Apr 09, 2014 at 05:14:35PM +0200, Tomasz Nowicki wrote:
> @@ -310,6 +310,8 @@ static void ghes_copy_tofrom_phys(void *buffer, u64 paddr, u32 len,
> u64 offset;
> u32 trunk;
>
> + BUG_ON(in_nmi && !IS_ENABLED(CONFIG_ACPI_APEI_NMI));
> +
That's a good idea but it should be in the NMI notifier
ghes_notify_nmi() instead and simpler:
BUG_ON(!IS_ENABLED(CONFIG_ACPI_APEI_NMI));
> while (len > 0) {
> offset = paddr - (paddr & PAGE_MASK);
> if (in_nmi) {
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/7] acpi, apei, ghes: Introduce more generic mechanism to init/deinit GHES error notifications.
2014-05-13 18:13 ` Borislav Petkov
@ 2014-05-15 14:31 ` Tomasz Nowicki
2014-05-21 18:11 ` Borislav Petkov
0 siblings, 1 reply; 33+ messages in thread
From: Tomasz Nowicki @ 2014-05-15 14:31 UTC (permalink / raw)
To: Borislav Petkov
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On 13.05.2014 20:13, Borislav Petkov wrote:
> On Wed, Apr 09, 2014 at 05:14:30PM +0200, Tomasz Nowicki wrote:
>> 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 can avoid #ifdef usage in common code e.g. for NMI which is currently
>> supported by x86.
>>
>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>> ---
>> drivers/acpi/apei/ghes.c | 242 ++++++++++++++++++++++++++++------------------
>> 1 file changed, 149 insertions(+), 93 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index f7edffc..ca8387e 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -148,6 +148,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);
>> +};
>
> Please run through checkpatch before submitting:
>
> WARNING: please, no space before tabs
> #44: FILE: drivers/acpi/apei/ghes.c:153:
> +^Iint ^I (*init_call)(struct ghes *ghes);$
>
> WARNING: please, no space before tabs
> #45: FILE: drivers/acpi/apei/ghes.c:154:
> +^Ivoid ^I (*remove_call)(struct ghes *ghes);$
My bad, will run it before next submission.
>
>> +
>> +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,
>> @@ -879,10 +887,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)
>> {
>> @@ -898,33 +902,151 @@ static unsigned long ghes_esource_prealloc_size(
>> return prealloc_size;
>> }
>>
>> +static int ghes_notify_init_nmi(struct ghes *ghes)
>> +{
>> + unsigned long len;
>> +
>> + len = ghes_esource_prealloc_size(ghes->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);
>> +
>> + return 0;
>
> Unconditionally returning 0 seems kinda moot. At least
> register_nmi_handler() retval could be returned...
>
> ...
Yes, it's worth returning register_nmi_handler() retval. Nice catch.
>
>> +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",
>> + ghes_notify_init_nmi,
>> + ghes_notify_remove_nmi},
>> + [ACPI_HEST_NOTIFY_CMCI] = {"CMCI", NULL, NULL},
>> + [ACPI_HEST_NOTIFY_MCE] = {"MCE", NULL, NULL},
>> +};
>
> I guess you don't need to save the ->notif_name strings but add
> a function which maps ACPI_HEST_* to strings => smaller struct
> ghes_notify_setup.
Well, we need to keep these strings somewhere :) IMO, this is simpler
solution. What would be advantage of additional map function?
>
>
>> @@ -944,48 +1066,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();
>
> What happened to those BUG() catch-all stoppers?
I am validating generic->notify.type at the begin of function and return
error in case of unknown value. I think there is no need to check that
again.
>
>> - }
>> + rc = (*ghes_init_call)(ghes);
>> + if (rc)
>> + goto err_edac_unreg;
>> +
>> platform_set_drvdata(ghes_dev, ghes);
>>
>> return 0;
>> @@ -1003,44 +1087,16 @@ 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);
>>
>> ghes = platform_get_drvdata(ghes_dev);
>> + ghes->flags |= GHES_EXITING;
>> +
>> generic = ghes->generic;
>> + ghes_remove_call = ghes_notify_tab[generic->notify.type].remove_call;
>>
>> - 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;
>
> This one too.
>
For this one I should provide generic->notify.type validation. Will
rethink and fix it.
Tomasz
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/7] acpi, apei, ghes: Introduce more generic mechanism to init/deinit GHES error notifications.
2014-05-15 14:31 ` Tomasz Nowicki
@ 2014-05-21 18:11 ` Borislav Petkov
0 siblings, 0 replies; 33+ messages in thread
From: Borislav Petkov @ 2014-05-21 18:11 UTC (permalink / raw)
To: Tomasz Nowicki
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On Thu, May 15, 2014 at 04:31:20PM +0200, Tomasz Nowicki wrote:
> Well, we need to keep these strings somewhere :) IMO, this is simpler
> solution. What would be advantage of additional map function?
Nevermind, I got confused here.
> I am validating generic->notify.type at the begin of function and
> return error in case of unknown value. I think there is no need to
> check that again.
Yes, you do :)
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context.
2014-05-13 19:41 ` Borislav Petkov
@ 2014-05-23 12:06 ` Tomasz Nowicki
2014-05-23 16:48 ` Borislav Petkov
0 siblings, 1 reply; 33+ messages in thread
From: Tomasz Nowicki @ 2014-05-23 12:06 UTC (permalink / raw)
To: Borislav Petkov
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On 13.05.2014 21:41, Borislav Petkov wrote:
> On Wed, Apr 09, 2014 at 05:14:32PM +0200, Tomasz Nowicki wrote:
>> Use CONFIG_ACPI_APEI_NMI to isolate NMI error notification path. NMI related
>> data and functions are grouped so they can be wrapped inside one
I have missed end of sentence. I should goes like that:
Use CONFIG_ACPI_APEI_NMI to isolate NMI error notification path. NMI
related data and functions are grouped so they can be wrapped inside one
#ifdefs CONFIG_ACPI_APEI_NMI.
>>
>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>> ---
>> drivers/acpi/apei/ghes.c | 54 +++++++++++++++++++++++++---------------------
>> 1 file changed, 30 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index ca8387e..7a0d66e 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -53,7 +53,9 @@
>> #include <asm/mce.h>
>> #endif
>> #include <asm/tlbflush.h>
>> +#ifdef CONFIG_ACPI_APEI_NMI
>> #include <asm/nmi.h>
>> +#endif
>
> This, again, can be avoided with adding arch-specific versions and empty
> default stubs.
>
I had that thoughts too. Looking at simple MCE calls, yes, it does make
sense to create corresponding arch-specific version and provide logic as
needed. I think that NMI is much more complicated. NMI context is
tightly coupled with the rest of GHES mechanisms like pool memory, list
locks etc. which are GHES locally available. I am not saying it is
impossible, I am just concerned if it makes sense to create e.g.
apei-nmi.c arch-specific file and export necessary GHES mechanisms just
for NMI purpose. Please let me know your opinion on this.
Regards,
Tomasz
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context.
2014-05-23 12:06 ` Tomasz Nowicki
@ 2014-05-23 16:48 ` Borislav Petkov
2014-05-26 13:26 ` Tomasz Nowicki
0 siblings, 1 reply; 33+ messages in thread
From: Borislav Petkov @ 2014-05-23 16:48 UTC (permalink / raw)
To: Tomasz Nowicki
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On Fri, May 23, 2014 at 02:06:47PM +0200, Tomasz Nowicki wrote:
> I had that thoughts too. Looking at simple MCE calls, yes, it does
> make sense to create corresponding arch-specific version and provide
> logic as needed. I think that NMI is much more complicated....
How about this ontop of your patches. It builds here but it is not
trivial for me to try it on !X86. Maybe you can test it quicker than me
finding some other arch box first... :)
Thanks.
---
Index: b/arch/x86/include/asm/nmi.h
===================================================================
--- a/arch/x86/include/asm/nmi.h 2014-05-23 17:17:08.192266007 +0200
+++ b/arch/x86/include/asm/nmi.h 2014-05-23 18:45:06.636178772 +0200
@@ -57,7 +57,7 @@ struct nmiaction {
int __register_nmi_handler(unsigned int, struct nmiaction *);
-void unregister_nmi_handler(unsigned int, const char *);
+void unregister_nmi_handler(unsigned int type, const char *name);
void stop_nmi(void);
void restart_nmi(void);
Index: b/drivers/acpi/apei/ghes.c
===================================================================
--- a/drivers/acpi/apei/ghes.c 2014-05-23 17:17:27.680265685 +0200
+++ b/drivers/acpi/apei/ghes.c 2014-05-23 17:46:32.968236841 +0200
@@ -47,15 +47,13 @@
#include <linux/genalloc.h>
#include <linux/pci.h>
#include <linux/aer.h>
+#include <linux/nmi.h>
#include <acpi/ghes.h>
#ifdef CONFIG_X86_MCE
#include <asm/mce.h>
#endif
#include <asm/tlbflush.h>
-#ifdef CONFIG_ACPI_APEI_NMI
-#include <asm/nmi.h>
-#endif
#include "apei-internal.h"
Index: b/include/linux/nmi.h
===================================================================
--- a/include/linux/nmi.h 2014-05-23 17:42:55.928240428 +0200
+++ b/include/linux/nmi.h 2014-05-23 18:44:41.244179192 +0200
@@ -53,4 +53,11 @@ extern int proc_dowatchdog(struct ctl_ta
void __user *, size_t *, loff_t *);
#endif
+#ifdef CONFIG_ACPI_APEI_NMI
+#include <asm/nmi.h>
+#else
+#define register_nmi_handler(t, fn, fg, n, init...) do { } while(0)
+void unregister_nmi_handler(unsigned int type, const char *name) {}
+#endif /* CONFIG_ACPI_APEI_NMI */
+
#endif
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context.
2014-05-23 16:48 ` Borislav Petkov
@ 2014-05-26 13:26 ` Tomasz Nowicki
2014-05-26 13:45 ` Borislav Petkov
0 siblings, 1 reply; 33+ messages in thread
From: Tomasz Nowicki @ 2014-05-26 13:26 UTC (permalink / raw)
To: Borislav Petkov
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On 23.05.2014 18:48, Borislav Petkov wrote:
> On Fri, May 23, 2014 at 02:06:47PM +0200, Tomasz Nowicki wrote:
>> I had that thoughts too. Looking at simple MCE calls, yes, it does
>> make sense to create corresponding arch-specific version and provide
>> logic as needed. I think that NMI is much more complicated....
>
> How about this ontop of your patches. It builds here but it is not
> trivial for me to try it on !X86. Maybe you can test it quicker than me
> finding some other arch box first... :)
>
Now I do follow :) Nicely done, I have applied your patch and indeed
there are more arch dependencies for !X86. I created another patch (it
can be applied on top of last patch set). Does it look reasonable?
diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
index 86f9301..0f03ab6 100644
--- a/arch/x86/include/asm/nmi.h
+++ b/arch/x86/include/asm/nmi.h
@@ -54,7 +54,18 @@ struct nmiaction {
int __register_nmi_handler(unsigned int, struct nmiaction *);
-void unregister_nmi_handler(unsigned int, const char *);
+void unregister_nmi_handler(unsigned int type, const char *name);
+
+static inline int arch_apei_register_nmi(nmi_handler_t fn,
+ const char *name)
+{
+ return register_nmi_handler(NMI_LOCAL, fn, 0, name);
+}
+
+static inline void arch_apei_unregister_nmi(const char *name)
+{
+ unregister_nmi_handler(NMI_LOCAL, name);
+}
void stop_nmi(void);
void restart_nmi(void);
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 35a44d9..84c79af 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -47,13 +47,11 @@
#include <linux/genalloc.h>
#include <linux/pci.h>
#include <linux/aer.h>
+#include <linux/nmi.h>
#include <acpi/ghes.h>
#include <asm/apei.h>
#include <asm/tlbflush.h>
-#ifdef CONFIG_ACPI_APEI_NMI
-#include <asm/nmi.h>
-#endif
#include "apei-internal.h"
@@ -718,7 +716,6 @@ static int ghes_notify_sci(struct notifier_block *this,
return ret;
}
-#ifdef CONFIG_ACPI_APEI_NMI
/*
* printk is not safe in NMI context. So in NMI handler, we allocate
* required memory from lock-less memory allocator
@@ -817,7 +814,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(CONFIG_ACPI_APEI_NMI));
@@ -832,14 +829,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);
@@ -914,7 +911,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);
@@ -928,7 +925,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
@@ -941,17 +938,14 @@ static void ghes_notify_remove_nmi(struct ghes *ghes)
static void ghes_init_nmi(void)
{
+ if (!IS_ENABLED(CONFIG_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;
}
-#else
-static inline void ghes_init_nmi(void)
-{
-
-}
-#endif
static int ghes_notify_init_polled(struct ghes *ghes)
{
diff --git a/include/linux/nmi.h b/include/linux/nmi.h
index 084b4c5..1aa351c 100644
--- a/include/linux/nmi.h
+++ b/include/linux/nmi.h
@@ -56,4 +56,19 @@ extern int proc_dowatchdog(struct ctl_table *, int ,
void __user *, size_t *, loff_t *);
#endif
+#define APEI_NMI_DONE 0
+#define APEI_NMI_HANDLED 1
+
+#ifdef CONFIG_ACPI_APEI_NMI
+#include <asm/nmi.h>
+#define arch_apei_nmi_oops_begin() oops_begin()
+#else
+#define arch_apei_register_nmi(fn, n) ({ \
+ void __attribute__((unused)) *dummy = fn; \
+ (-ENOSYS); \
+});
+#define arch_apei_unregister_nmi(n)
+#define arch_apei_nmi_oops_begin()
+#endif /* CONFIG_ACPI_APEI_NMI */
+
#endif
Regards,
Tomasz
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context.
2014-05-26 13:26 ` Tomasz Nowicki
@ 2014-05-26 13:45 ` Borislav Petkov
2014-05-26 14:02 ` Tomasz Nowicki
0 siblings, 1 reply; 33+ messages in thread
From: Borislav Petkov @ 2014-05-26 13:45 UTC (permalink / raw)
To: Tomasz Nowicki
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On Mon, May 26, 2014 at 03:26:06PM +0200, Tomasz Nowicki wrote:
> Now I do follow :) Nicely done, I have applied your patch and indeed
> there are more arch dependencies for !X86.
Not nicely enough, I guess :-)
> diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
> index 86f9301..0f03ab6 100644
> --- a/arch/x86/include/asm/nmi.h
> +++ b/arch/x86/include/asm/nmi.h
> @@ -54,7 +54,18 @@ struct nmiaction {
>
> int __register_nmi_handler(unsigned int, struct nmiaction *);
>
> -void unregister_nmi_handler(unsigned int, const char *);
> +void unregister_nmi_handler(unsigned int type, const char *name);
> +
> +static inline int arch_apei_register_nmi(nmi_handler_t fn,
> + const char *name)
> +{
> + return register_nmi_handler(NMI_LOCAL, fn, 0, name);
> +}
> +
> +static inline void arch_apei_unregister_nmi(const char *name)
> +{
> + unregister_nmi_handler(NMI_LOCAL, name);
> +}
I'm guessing you've added those wrappers so that you don't have to
export NMI_LOCAL?
> void stop_nmi(void);
> void restart_nmi(void);
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 35a44d9..84c79af 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -47,13 +47,11 @@
> #include <linux/genalloc.h>
> #include <linux/pci.h>
> #include <linux/aer.h>
> +#include <linux/nmi.h>
>
> #include <acpi/ghes.h>
> #include <asm/apei.h>
> #include <asm/tlbflush.h>
> -#ifdef CONFIG_ACPI_APEI_NMI
> -#include <asm/nmi.h>
> -#endif
>
> #include "apei-internal.h"
>
> @@ -718,7 +716,6 @@ static int ghes_notify_sci(struct notifier_block *this,
> return ret;
> }
>
> -#ifdef CONFIG_ACPI_APEI_NMI
> /*
> * printk is not safe in NMI context. So in NMI handler, we allocate
> * required memory from lock-less memory allocator
> @@ -817,7 +814,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(CONFIG_ACPI_APEI_NMI));
>
> @@ -832,14 +829,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);
> @@ -914,7 +911,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);
>
> @@ -928,7 +925,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
> @@ -941,17 +938,14 @@ static void ghes_notify_remove_nmi(struct ghes *ghes)
>
> static void ghes_init_nmi(void)
> {
> + if (!IS_ENABLED(CONFIG_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;
> }
> -#else
> -static inline void ghes_init_nmi(void)
> -{
> -
> -}
> -#endif
>
> static int ghes_notify_init_polled(struct ghes *ghes)
> {
> diff --git a/include/linux/nmi.h b/include/linux/nmi.h
> index 084b4c5..1aa351c 100644
> --- a/include/linux/nmi.h
> +++ b/include/linux/nmi.h
> @@ -56,4 +56,19 @@ extern int proc_dowatchdog(struct ctl_table *, int ,
> void __user *, size_t *, loff_t *);
> #endif
>
> +#define APEI_NMI_DONE 0
> +#define APEI_NMI_HANDLED 1
> +
> +#ifdef CONFIG_ACPI_APEI_NMI
> +#include <asm/nmi.h>
> +#define arch_apei_nmi_oops_begin() oops_begin()
> +#else
> +#define arch_apei_register_nmi(fn, n) ({ \
> + void __attribute__((unused)) *dummy = fn; \
Do we really need this dummy assignment? Wouldn't it be just fine to
simply have:
#define arch_apei_register_nmi(fn, n) ({ (-ENOSYS); })
Just a nitpick, though; otherwise, this looks nicely abstracted.
Thanks!
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context.
2014-05-26 13:45 ` Borislav Petkov
@ 2014-05-26 14:02 ` Tomasz Nowicki
0 siblings, 0 replies; 33+ messages in thread
From: Tomasz Nowicki @ 2014-05-26 14:02 UTC (permalink / raw)
To: Borislav Petkov
Cc: rjw, lenb, tony.luck, bp, m.chehab, linux-edac, x86, linux-acpi,
linux-kernel, linaro-acpi
On 26.05.2014 15:45, Borislav Petkov wrote:
> On Mon, May 26, 2014 at 03:26:06PM +0200, Tomasz Nowicki wrote:
>> Now I do follow :) Nicely done, I have applied your patch and indeed
>> there are more arch dependencies for !X86.
>
> Not nicely enough, I guess :-)
>
>> diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
>> index 86f9301..0f03ab6 100644
>> --- a/arch/x86/include/asm/nmi.h
>> +++ b/arch/x86/include/asm/nmi.h
>> @@ -54,7 +54,18 @@ struct nmiaction {
>>
>> int __register_nmi_handler(unsigned int, struct nmiaction *);
>>
>> -void unregister_nmi_handler(unsigned int, const char *);
>> +void unregister_nmi_handler(unsigned int type, const char *name);
>> +
>> +static inline int arch_apei_register_nmi(nmi_handler_t fn,
>> + const char *name)
>> +{
>> + return register_nmi_handler(NMI_LOCAL, fn, 0, name);
>> +}
>> +
>> +static inline void arch_apei_unregister_nmi(const char *name)
>> +{
>> + unregister_nmi_handler(NMI_LOCAL, name);
>> +}
>
> I'm guessing you've added those wrappers so that you don't have to
> export NMI_LOCAL?
Yep.
>
>> void stop_nmi(void);
>> void restart_nmi(void);
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 35a44d9..84c79af 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -47,13 +47,11 @@
>> #include <linux/genalloc.h>
>> #include <linux/pci.h>
>> #include <linux/aer.h>
>> +#include <linux/nmi.h>
>>
>> #include <acpi/ghes.h>
>> #include <asm/apei.h>
>> #include <asm/tlbflush.h>
>> -#ifdef CONFIG_ACPI_APEI_NMI
>> -#include <asm/nmi.h>
>> -#endif
>>
>> #include "apei-internal.h"
>>
>> @@ -718,7 +716,6 @@ static int ghes_notify_sci(struct notifier_block *this,
>> return ret;
>> }
>>
>> -#ifdef CONFIG_ACPI_APEI_NMI
>> /*
>> * printk is not safe in NMI context. So in NMI handler, we allocate
>> * required memory from lock-less memory allocator
>> @@ -817,7 +814,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(CONFIG_ACPI_APEI_NMI));
>>
>> @@ -832,14 +829,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);
>> @@ -914,7 +911,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);
>>
>> @@ -928,7 +925,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
>> @@ -941,17 +938,14 @@ static void ghes_notify_remove_nmi(struct ghes *ghes)
>>
>> static void ghes_init_nmi(void)
>> {
>> + if (!IS_ENABLED(CONFIG_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;
>> }
>> -#else
>> -static inline void ghes_init_nmi(void)
>> -{
>> -
>> -}
>> -#endif
>>
>> static int ghes_notify_init_polled(struct ghes *ghes)
>> {
>> diff --git a/include/linux/nmi.h b/include/linux/nmi.h
>> index 084b4c5..1aa351c 100644
>> --- a/include/linux/nmi.h
>> +++ b/include/linux/nmi.h
>> @@ -56,4 +56,19 @@ extern int proc_dowatchdog(struct ctl_table *, int ,
>> void __user *, size_t *, loff_t *);
>> #endif
>>
>> +#define APEI_NMI_DONE 0
>> +#define APEI_NMI_HANDLED 1
>> +
>> +#ifdef CONFIG_ACPI_APEI_NMI
>> +#include <asm/nmi.h>
>> +#define arch_apei_nmi_oops_begin() oops_begin()
>> +#else
>> +#define arch_apei_register_nmi(fn, n) ({ \
>> + void __attribute__((unused)) *dummy = fn; \
>
> Do we really need this dummy assignment? Wouldn't it be just fine to
> simply have:
>
> #define arch_apei_register_nmi(fn, n) ({ (-ENOSYS); })
For !X86 I've got compilator warning that ghes_notify_nmi is defined but
not used. I just want to make console a bit cleaner :)
>
> Just a nitpick, though; otherwise, this looks nicely abstracted.
Thanks for your help!
Tomasz
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2014-05-26 14:01 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-09 15:14 [PATCH 0/7] APEI: Make APEI architecture independent Tomasz Nowicki
2014-04-09 15:14 ` [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture Tomasz Nowicki
2014-05-05 11:44 ` Borislav Petkov
2014-05-05 14:34 ` Tomasz Nowicki
2014-05-05 14:53 ` Borislav Petkov
2014-05-05 15:32 ` Tomasz Nowicki
2014-05-05 15:33 ` Borislav Petkov
2014-05-05 15:36 ` Tomasz Nowicki
2014-04-09 15:14 ` [PATCH 2/7] acpi, apei, ghes: Introduce more generic mechanism to init/deinit GHES error notifications Tomasz Nowicki
2014-05-13 18:13 ` Borislav Petkov
2014-05-15 14:31 ` Tomasz Nowicki
2014-05-21 18:11 ` Borislav Petkov
2014-04-09 15:14 ` [PATCH 3/7] ACPI, APEI, GHES: Introduce ACPI_APEI_NMI to make NMI error notification a GHES feature Tomasz Nowicki
2014-04-09 15:14 ` [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context Tomasz Nowicki
2014-05-13 19:41 ` Borislav Petkov
2014-05-23 12:06 ` Tomasz Nowicki
2014-05-23 16:48 ` Borislav Petkov
2014-05-26 13:26 ` Tomasz Nowicki
2014-05-26 13:45 ` Borislav Petkov
2014-05-26 14:02 ` Tomasz Nowicki
2014-04-09 15:14 ` [PATCH 5/7] acpi, apei, ghes: Attach NMI init/deinit functions while CONFIG_ACPI_APEI_NMI is enabled Tomasz Nowicki
2014-05-13 19:49 ` Borislav Petkov
2014-04-09 15:14 ` [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture Tomasz Nowicki
2014-05-13 20:11 ` Borislav Petkov
2014-05-14 12:32 ` Tomasz Nowicki
2014-05-14 12:35 ` Will Deacon
2014-05-14 12:45 ` Catalin Marinas
2014-05-14 12:48 ` Will Deacon
2014-05-14 12:52 ` Tomasz Nowicki
2014-05-14 13:21 ` Borislav Petkov
2014-04-09 15:14 ` [PATCH 7/7] acpi, apei, ghes: Factor out ioremap virtual memory for IRQ and NMI context Tomasz Nowicki
2014-05-14 17:13 ` Borislav Petkov
2014-05-05 9:25 ` [PATCH 0/7] APEI: Make APEI architecture independent Tomasz Nowicki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).