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(-)
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);
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);
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
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;
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)
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)
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) {
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(-)
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.
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
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.
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
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.
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.
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.
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"
...
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},}; @@ -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
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.
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
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
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)?
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
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
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 :)
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) {
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
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 :)
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
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
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
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();@@ -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");@@ -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");
- 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!
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();@@ -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");@@ -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");
- 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
-
Borislav Petkov -
Catalin Marinas -
Tomasz Nowicki -
Will Deacon