| Message ID | 20180921221705.6478-6-james.morse@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | APEI in_nmi() rework | expand |
On Fri, Sep 21, 2018 at 11:16:52PM +0100, James Morse wrote: > Now that there are two users of the estatus queue, and likely to be more, > make it a Kconfig symbol selected by the appropriate notification. We > can move the ARCH_HAVE_NMI_SAFE_CMPXCHG checks in here too. Ok, question: why do we need to complicate things at all? I mean, why do we even need a Kconfig symbol? This code is being used by two arches now so why not simply build it in unconditionally and be done with it. The couple of KB saved are simply not worth the effort, especially if it is going to end up being enabled on 99% of the setups... Or?
Hi Boris, On 01/10/18 18:59, Borislav Petkov wrote: > On Fri, Sep 21, 2018 at 11:16:52PM +0100, James Morse wrote: >> Now that there are two users of the estatus queue, and likely to be more, >> make it a Kconfig symbol selected by the appropriate notification. We >> can move the ARCH_HAVE_NMI_SAFE_CMPXCHG checks in here too. > > Ok, question: why do we need to complicate things at all? I mean, why do > we even need a Kconfig symbol? Before patch 4, this was behind CONFIG_HAVE_ACPI_APEI_NMI, (so it made use of an existing kconfig symbol), and there was only one user x86:NMI. The ACPI spec has four ~NMI notifications, so far the support for these in Linux has been selectable separately. If you build the kernel without any of them then this code would be unused, and generate warnings because all those users are behind #ifdef too. > This code is being used by two arches now so why not simply build it in > unconditionally and be done with it. The couple of KB saved are simply > not worth the effort, especially if it is going to end up being enabled > on 99% of the setups... I'm all in favour of letting the compiler work it out, but the existing ghes code has #ifdef/#else all over the place. This is 'keeping the style'. I assumed it was done this way to support an older compiler on x86, (I see that jumped from 3.2 to 4.6 with commit cafa0010cd51) We could strip the lot away to a few IS_ENABLED() in ghes_probe() and the memory_failure()/AER calls if you'd prefer. Thanks, James
On Wed, Oct 03, 2018 at 06:50:36PM +0100, James Morse wrote: > I'm all in favour of letting the compiler work it out, but the existing ghes > code has #ifdef/#else all over the place. This is 'keeping the style'. Yeah, but this "style" is not the optimal one and we should simplify/clean up and fix this thing. Swapping the order of your statements here: > The ACPI spec has four ~NMI notifications, so far the support for > these in Linux has been selectable separately. Yes, but: distro kernels end up enabling all those options anyway and distro kernels are 90-ish% of the setups. Which means, this will get enabled anyway and this additional Kconfig symbol is simply going to be one automatic reply "Yes". So let's build it in by default and if someone complains about it, we can always carve it out. But right now I don't see the need for the unnecessary separation... Thx.
Hi Boris, On 04/10/2018 18:34, Borislav Petkov wrote: > On Wed, Oct 03, 2018 at 06:50:36PM +0100, James Morse wrote: >> I'm all in favour of letting the compiler work it out, but the existing ghes >> code has #ifdef/#else all over the place. This is 'keeping the style'. > > Yeah, but this "style" is not the optimal one and we should > simplify/clean up and fix this thing. > > Swapping the order of your statements here: > >> The ACPI spec has four ~NMI notifications, so far the support for >> these in Linux has been selectable separately. > > Yes, but: distro kernels end up enabling all those options anyway and > distro kernels are 90-ish% of the setups. Which means, this will get > enabled anyway and this additional Kconfig symbol is simply going to be > one automatic reply "Yes". > > So let's build it in by default and if someone complains about it, we > can always carve it out. But right now I don't see the need for the > unnecessary separation... Ripping out the existing #ifdefs and replacing them with IS_ENABLED() would let the compiler work out the estatus stuff is unused, and saves us describing the what-uses-it logic in Kconfig. But this does expose the x86 nmi stuff on arm64, which doesn't build today. Dragging NMI_HANDLED and friends up to the 'linux' header causes a fair amount of noise under arch/x86 (include the new header in 22 files). Adding dummy declarations to arm64 fixes this, and doesn't affect the other architectures that have an asm/nmi.h Alternatively we could leave {un,}register_nmi_handler() under CONFIG_HAVE_ACPI_APEI_NMI. I think we need to keep the NOTIFY_NMI kconfig symbol around, as its one of the two I can't work out how to fix without the TLBI-IPI. Thanks, James
On Fri, Oct 12, 2018 at 06:17:48PM +0100, James Morse wrote: > Ripping out the existing #ifdefs and replacing them with IS_ENABLED() would let > the compiler work out the estatus stuff is unused, and saves us describing the > what-uses-it logic in Kconfig. > > But this does expose the x86 nmi stuff on arm64, which doesn't build today. Gah, that ifdeffery is one big mess. ;-\ One fine day... > Dragging NMI_HANDLED and friends up to the 'linux' header causes a fair amount > of noise under arch/x86 (include the new header in 22 files). Adding dummy > declarations to arm64 fixes this, and doesn't affect the other architectures > that have an asm/nmi.h > > Alternatively we could leave {un,}register_nmi_handler() under > CONFIG_HAVE_ACPI_APEI_NMI. I think we need to keep the NOTIFY_NMI kconfig symbol > around, as its one of the two I can't work out how to fix without the TLBI-IPI. Hmm, so I just tried the diff below with my arm64 cross compiler and a defconfig with CONFIG_ACPI_APEI_GHES=y CONFIG_EDAC_GHES=y and it did build fine. What am I missing? --- diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig index 2b191e09b647..52ae5438edeb 100644 --- a/drivers/acpi/apei/Kconfig +++ b/drivers/acpi/apei/Kconfig @@ -4,7 +4,6 @@ config HAVE_ACPI_APEI config HAVE_ACPI_APEI_NMI bool - select ACPI_APEI_GHES_ESTATUS_QUEUE config ACPI_APEI bool "ACPI Platform Error Interface (APEI)" @@ -34,10 +33,6 @@ config ACPI_APEI_GHES by firmware to produce more valuable hardware error information for Linux. -config ACPI_APEI_GHES_ESTATUS_QUEUE - bool - depends on ACPI_APEI_GHES && ARCH_HAVE_NMI_SAFE_CMPXCHG - config ACPI_APEI_PCIEAER bool "APEI PCIe AER logging/recovering support" depends on ACPI_APEI && PCIEAER @@ -48,7 +43,6 @@ config ACPI_APEI_PCIEAER config ACPI_APEI_SEA bool "APEI Synchronous External Abort logging/recovering support" depends on ARM64 && ACPI_APEI_GHES - select ACPI_APEI_GHES_ESTATUS_QUEUE default y help This option should be enabled if the system supports diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index 463c8e6d1bb5..8191d711564b 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -683,7 +683,6 @@ static void ghes_estatus_cache_add( rcu_read_unlock(); } -#ifdef CONFIG_ACPI_APEI_GHES_ESTATUS_QUEUE /* * Handlers for CPER records may not be NMI safe. For example, * memory_failure_queue() takes spinlocks and calls schedule_work_on(). @@ -862,10 +861,6 @@ static void ghes_nmi_init_cxt(void) init_irq_work(&ghes_proc_irq_work, ghes_proc_in_irq); } -#else -static inline void ghes_nmi_init_cxt(void) { } -#endif /* CONFIG_ACPI_APEI_GHES_ESTATUS_QUEUE */ - static int ghes_ack_error(struct acpi_hest_generic_v2 *gv2) { int rc;
diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig index 52ae5438edeb..2b191e09b647 100644 --- a/drivers/acpi/apei/Kconfig +++ b/drivers/acpi/apei/Kconfig @@ -4,6 +4,7 @@ config HAVE_ACPI_APEI config HAVE_ACPI_APEI_NMI bool + select ACPI_APEI_GHES_ESTATUS_QUEUE config ACPI_APEI bool "ACPI Platform Error Interface (APEI)" @@ -33,6 +34,10 @@ config ACPI_APEI_GHES by firmware to produce more valuable hardware error information for Linux. +config ACPI_APEI_GHES_ESTATUS_QUEUE + bool + depends on ACPI_APEI_GHES && ARCH_HAVE_NMI_SAFE_CMPXCHG + config ACPI_APEI_PCIEAER bool "APEI PCIe AER logging/recovering support" depends on ACPI_APEI && PCIEAER @@ -43,6 +48,7 @@ config ACPI_APEI_PCIEAER config ACPI_APEI_SEA bool "APEI Synchronous External Abort logging/recovering support" depends on ARM64 && ACPI_APEI_GHES + select ACPI_APEI_GHES_ESTATUS_QUEUE default y help This option should be enabled if the system supports diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index 150fb184c7cb..2880547e13b8 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -58,10 +58,6 @@ #define GHES_PFX "GHES: " -#if defined(CONFIG_HAVE_ACPI_APEI_NMI) || defined(CONFIG_ACPI_APEI_SEA) -#define WANT_NMI_ESTATUS_QUEUE 1 -#endif - #define GHES_ESTATUS_MAX_SIZE 65536 #define GHES_ESOURCE_PREALLOC_MAX_SIZE 65536 @@ -685,7 +681,7 @@ static void ghes_estatus_cache_add( rcu_read_unlock(); } -#ifdef WANT_NMI_ESTATUS_QUEUE +#ifdef CONFIG_ACPI_APEI_GHES_ESTATUS_QUEUE /* * Handlers for CPER records may not be NMI safe. For example, * memory_failure_queue() takes spinlocks and calls schedule_work_on(). @@ -727,7 +723,6 @@ static void ghes_print_queued_estatus(void) /* Save estatus for further processing in IRQ context */ static void __process_error(struct ghes *ghes) { -#ifdef CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG u32 len, node_len; struct ghes_estatus_node *estatus_node; struct acpi_hest_generic_status *estatus; @@ -747,7 +742,6 @@ static void __process_error(struct ghes *ghes) estatus = GHES_ESTATUS_FROM_NODE(estatus_node); memcpy(estatus, ghes->estatus, len); llist_add(&estatus_node->llnode, &ghes_estatus_llist); -#endif } static int _in_nmi_notify_one(struct ghes *ghes) @@ -786,7 +780,7 @@ static int ghes_estatus_queue_notified(struct list_head *rcu_list) } rcu_read_unlock(); - if (IS_ENABLED(CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG) && !ret) + if (!ret) irq_work_queue(&ghes_proc_irq_work); return ret; @@ -865,7 +859,7 @@ static void ghes_nmi_init_cxt(void) #else static inline void ghes_nmi_init_cxt(void) { } -#endif /* WANT_NMI_ESTATUS_QUEUE */ +#endif /* CONFIG_ACPI_APEI_GHES_ESTATUS_QUEUE */ static int ghes_ack_error(struct acpi_hest_generic_v2 *gv2) {
Now that there are two users of the estatus queue, and likely to be more, make it a Kconfig symbol selected by the appropriate notification. We can move the ARCH_HAVE_NMI_SAFE_CMPXCHG checks in here too. Signed-off-by: James Morse <james.morse@arm.com> --- drivers/acpi/apei/Kconfig | 6 ++++++ drivers/acpi/apei/ghes.c | 12 +++--------- 2 files changed, 9 insertions(+), 9 deletions(-)