[PATCH 0/7] APEI: Make APEI architecture independent.

[PATCH 0/7] APEI: Make APEI architecture independent.

[Tomasz Nowicki]

APEI is currently implemented so that it depends on x86 hardware.
The primary dependency is that GHES uses the x86 NMI for hardware
error notification. 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

[PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture.

[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

Re: [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture.

[Borislav Petkov]

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.
--

Re: [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture.

[Tomasz Nowicki]

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

Re: [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture.

[Borislav Petkov]

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.
--

Re: [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture.

[Tomasz Nowicki]

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

Re: [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture.

[Borislav Petkov]

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.
--

Re: [PATCH 1/7] apei, mce: Call MCE-specific code only for X86 architecture.

[Tomasz Nowicki]

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.

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

[Tomasz Nowicki]

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

This approach allows us to change address of a given error notification
init/deinit function call in ghes_notify_tab in runtime. In turn,
we 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

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

[Borislav Petkov]

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.
--

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

[Tomasz Nowicki]

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

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

[Borislav Petkov]

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.
--

[PATCH 3/7] ACPI, APEI, GHES: Introduce ACPI_APEI_NMI to make NMI error notification a GHES feature.

[Tomasz Nowicki]

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

This commit adds 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

[PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context.

[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

Re: [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context.

[Borislav Petkov]

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.
--

Re: [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context.

[Tomasz Nowicki]

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

Re: [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context.

[Borislav Petkov]

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.
--

Re: [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context.

[Tomasz Nowicki]

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

Re: [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context.

[Borislav Petkov]

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.
--

Re: [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context.

[Tomasz Nowicki]

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

[PATCH 5/7] acpi, apei, ghes: Attach NMI init/deinit functions while CONFIG_ACPI_APEI_NMI is enabled.

[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

Re: [PATCH 5/7] acpi, apei, ghes: Attach NMI init/deinit functions while CONFIG_ACPI_APEI_NMI is enabled.

[Borislav Petkov]

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.
--

[PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.

[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

Re: [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.

[Borislav Petkov]

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.
--

Re: [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.

[Tomasz Nowicki]

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

Re: [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.

[Will Deacon]

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

Re: [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.

[Catalin Marinas]

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

Re: [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.

[Will Deacon]

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

Re: [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.

[Tomasz Nowicki]

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

Re: [PATCH 6/7] acpi, apei, ghes: Make unmapping functionality independent from architecture.

[Borislav Petkov]

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.
--

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

[Tomasz Nowicki]

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

To make it possible to not use a second page, swap the existing
page order so that the IRQ context page is first, and the optional
NMI context page is second.  Then, use 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

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

[Borislav Petkov]

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.
--

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

[Tomasz Nowicki]

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(-)
>