| 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 15:45:33 +0200
From: Borislav Petkov <bp@...en8.de>
To: Tomasz Nowicki <tomasz.nowicki@...aro.org>
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 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.
--
--
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