| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 26 May 2014 16:02:41 +0200
From: Tomasz Nowicki <tomasz.nowicki@...aro.org>
To: Borislav Petkov <bp@...en8.de>
CC: rjw@...ysocki.net, lenb@...nel.org, tony.luck@...el.com,
bp@...e.de, m.chehab@...sung.com, linux-edac@...r.kernel.org,
x86@...nel.org, linux-acpi@...r.kernel.org,
linux-kernel@...r.kernel.org, linaro-acpi@...ts.linaro.org
Subject: Re: [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification
context.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists