| Message ID | 1481147303-7979-6-git-send-email-tbaicar@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tyler, On 07/12/16 21:48, Tyler Baicar wrote: > ARM APEI extension proposal added SEA (Synchrounous External > Abort) notification type for ARMv8. > Add a new GHES error source handling function for SEA. If an error > source's notification type is SEA, then this function can be registered > into the SEA exception handler. That way GHES will parse and report > SEA exceptions when they occur. > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index 2acbc60..66ab3fd 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -767,6 +771,62 @@ static struct notifier_block ghes_notifier_sci = { > .notifier_call = ghes_notify_sci, > }; > > +#ifdef CONFIG_HAVE_ACPI_APEI_SEA > +static LIST_HEAD(ghes_sea); > + > +static int ghes_notify_sea(struct notifier_block *this, > + unsigned long event, void *data) > +{ > + struct ghes *ghes; > + int ret = NOTIFY_DONE; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(ghes, &ghes_sea, list) { > + if (!ghes_proc(ghes)) > + ret = NOTIFY_OK; > + } > + rcu_read_unlock(); > + > + return ret; > +} What stops this from being re-entrant? ghes_copy_tofrom_phs() takes the ghes_ioremap_lock_irq spinlock, but there is nothing to stop a subsequent instruction fetch or memory access causing another (maybe different) Synchronous External Abort which deadlocks trying to take the same lock. ghes_notify_sea() looks to be based on ghes_notify_sci(), which (if I've found the right part of the ACPI spec) is a level-low interrupt. spin_lock_irqsave() would mask interrupts so there is no risk of a different notification firing on the same CPU, (it looks like they are almost all ultimately an irq). NMI is the odd one out because its not maskable like this, but ghes_notify_nmi() has: > if (!atomic_add_unless(&ghes_in_nmi, 1, 1)) > return ret; To ensure there is only ever one thread poking around in this code. What happens if a system describes two GHES sources, one using an irq the other SEA? The SEA error can interrupt the irq error while its holding the above lock. I guess this is also why all the NMI code in that file is separate. Thanks, James -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/20/2016 8:29 AM, James Morse wrote: > Hi Tyler, > > On 07/12/16 21:48, Tyler Baicar wrote: >> ARM APEI extension proposal added SEA (Synchrounous External >> Abort) notification type for ARMv8. >> Add a new GHES error source handling function for SEA. If an error >> source's notification type is SEA, then this function can be registered >> into the SEA exception handler. That way GHES will parse and report >> SEA exceptions when they occur. >> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c >> index 2acbc60..66ab3fd 100644 >> --- a/drivers/acpi/apei/ghes.c >> +++ b/drivers/acpi/apei/ghes.c >> @@ -767,6 +771,62 @@ static struct notifier_block ghes_notifier_sci = { >> .notifier_call = ghes_notify_sci, >> }; >> >> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA >> +static LIST_HEAD(ghes_sea); >> + >> +static int ghes_notify_sea(struct notifier_block *this, >> + unsigned long event, void *data) >> +{ >> + struct ghes *ghes; >> + int ret = NOTIFY_DONE; >> + >> + rcu_read_lock(); >> + list_for_each_entry_rcu(ghes, &ghes_sea, list) { >> + if (!ghes_proc(ghes)) >> + ret = NOTIFY_OK; >> + } >> + rcu_read_unlock(); >> + >> + return ret; >> +} > What stops this from being re-entrant? > > ghes_copy_tofrom_phs() takes the ghes_ioremap_lock_irq spinlock, but there is > nothing to stop a subsequent instruction fetch or memory access causing another > (maybe different) Synchronous External Abort which deadlocks trying to take the > same lock. > > ghes_notify_sea() looks to be based on ghes_notify_sci(), which (if I've found > the right part of the ACPI spec) is a level-low interrupt. spin_lock_irqsave() > would mask interrupts so there is no risk of a different notification firing on > the same CPU, (it looks like they are almost all ultimately an irq). > > NMI is the odd one out because its not maskable like this, but ghes_notify_nmi() > has: >> if (!atomic_add_unless(&ghes_in_nmi, 1, 1)) >> return ret; > To ensure there is only ever one thread poking around in this code. > > What happens if a system describes two GHES sources, one using an irq the other > SEA? The SEA error can interrupt the irq error while its holding the above lock. > I guess this is also why all the NMI code in that file is separate. > > > Thanks, > > James Hi James, Let me see if I'm following you right :) I should use spin_lock_irqsave() in ghes_notify_sea() to avoid ghes_notify_sci() from interrupting this process and potentially causing the deadlock? This race condition does seem valid. We are using the same acknowledgment for all our HEST table entries, so our firmware will not populate more than one entry at a time. That gets us around this race condition. Thanks, Tyler
Hi Tyler, On 05/01/17 22:31, Baicar, Tyler wrote: > On 12/20/2016 8:29 AM, James Morse wrote: >> On 07/12/16 21:48, Tyler Baicar wrote: >>> ARM APEI extension proposal added SEA (Synchrounous External >>> Abort) notification type for ARMv8. >>> Add a new GHES error source handling function for SEA. If an error >>> source's notification type is SEA, then this function can be registered >>> into the SEA exception handler. That way GHES will parse and report >>> SEA exceptions when they occur. >>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c >>> index 2acbc60..66ab3fd 100644 >>> --- a/drivers/acpi/apei/ghes.c >>> +++ b/drivers/acpi/apei/ghes.c >>> @@ -767,6 +771,62 @@ static struct notifier_block ghes_notifier_sci = { >>> .notifier_call = ghes_notify_sci, >>> }; >>> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA >>> +static LIST_HEAD(ghes_sea); >>> + >>> +static int ghes_notify_sea(struct notifier_block *this, >>> + unsigned long event, void *data) >>> +{ >>> + struct ghes *ghes; >>> + int ret = NOTIFY_DONE; >>> + >>> + rcu_read_lock(); >>> + list_for_each_entry_rcu(ghes, &ghes_sea, list) { >>> + if (!ghes_proc(ghes)) >>> + ret = NOTIFY_OK; >>> + } >>> + rcu_read_unlock(); >>> + >>> + return ret; >>> +} >> What stops this from being re-entrant? >> >> ghes_copy_tofrom_phs() takes the ghes_ioremap_lock_irq spinlock, but there is >> nothing to stop a subsequent instruction fetch or memory access causing another >> (maybe different) Synchronous External Abort which deadlocks trying to take the >> same lock. >> >> ghes_notify_sea() looks to be based on ghes_notify_sci(), which (if I've found >> the right part of the ACPI spec) is a level-low interrupt. spin_lock_irqsave() >> would mask interrupts so there is no risk of a different notification firing on >> the same CPU, (it looks like they are almost all ultimately an irq). >> >> NMI is the odd one out because its not maskable like this, but ghes_notify_nmi() >> has: >>> if (!atomic_add_unless(&ghes_in_nmi, 1, 1)) >>> return ret; >> To ensure there is only ever one thread poking around in this code. >> >> What happens if a system describes two GHES sources, one using an irq the other >> SEA? The SEA error can interrupt the irq error while its holding the above lock. >> I guess this is also why all the NMI code in that file is separate. > Let me see if I'm following you right :) > I should use spin_lock_irqsave() in ghes_notify_sea() to avoid ghes_notify_sci() > from > interrupting this process and potentially causing the deadlock? This way round you are already safe: The CPU masks interrupts when it takes the exception, they should still be masked by the time we get in here... The other way round is a lot more fun! What happens if APEI is processing some error record that was notified via an interrupt, and then takes the Synchronous External Abort, and ends up back in this code? Masking interrupts doesn't stop the external-abort, and trying to take the ghes_ioremap_lock_irq will deadlock. What happens if we interrupt printk() holding all its locks is another thing I haven't worked out yet. > This race condition does seem valid. We are using the same acknowledgment for > all our > HEST table entries, so our firmware will not populate more than one entry at a > time. That > gets us around this race condition. Ah, so your firmware will wait for the interrupt-signalled error to be finished before it triggers the Synchronous External Abort. I think this would still be a linux bug if the firmware didn't do this. x86 could have done the same with NMI notifications, but we have all this 'if (in_nmi)' to allow interrupts-masked GHES handling to be interrupted. What do you think to re-using the 'if (in_nmi)' code for SEA? We can argue that SEA is NMI-like in that it can't be masked, and it interrupts code that had interrupts masked. It 'should' be as simple as putting 'HAVE_NMI' in arm64's Kconfig, and wrapping the atomic notifier call with nmi_enter()/nmi_exit() from linux/hardirq.h. (...famous last words...) This probably answers my printk() questions too, but I need to look into it some more. Thanks, James -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello James, On 1/6/2017 3:43 AM, James Morse wrote: > On 05/01/17 22:31, Baicar, Tyler wrote: >> On 12/20/2016 8:29 AM, James Morse wrote: >>> On 07/12/16 21:48, Tyler Baicar wrote: >>>> ARM APEI extension proposal added SEA (Synchrounous External >>>> Abort) notification type for ARMv8. >>>> Add a new GHES error source handling function for SEA. If an error >>>> source's notification type is SEA, then this function can be registered >>>> into the SEA exception handler. That way GHES will parse and report >>>> SEA exceptions when they occur. >>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c >>>> index 2acbc60..66ab3fd 100644 >>>> --- a/drivers/acpi/apei/ghes.c >>>> +++ b/drivers/acpi/apei/ghes.c >>>> @@ -767,6 +771,62 @@ static struct notifier_block ghes_notifier_sci = { >>>> .notifier_call = ghes_notify_sci, >>>> }; >>>> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA >>>> +static LIST_HEAD(ghes_sea); >>>> + >>>> +static int ghes_notify_sea(struct notifier_block *this, >>>> + unsigned long event, void *data) >>>> +{ >>>> + struct ghes *ghes; >>>> + int ret = NOTIFY_DONE; >>>> + >>>> + rcu_read_lock(); >>>> + list_for_each_entry_rcu(ghes, &ghes_sea, list) { >>>> + if (!ghes_proc(ghes)) >>>> + ret = NOTIFY_OK; >>>> + } >>>> + rcu_read_unlock(); >>>> + >>>> + return ret; >>>> +} >>> What stops this from being re-entrant? >>> >>> ghes_copy_tofrom_phs() takes the ghes_ioremap_lock_irq spinlock, but there is >>> nothing to stop a subsequent instruction fetch or memory access causing another >>> (maybe different) Synchronous External Abort which deadlocks trying to take the >>> same lock. >>> >>> ghes_notify_sea() looks to be based on ghes_notify_sci(), which (if I've found >>> the right part of the ACPI spec) is a level-low interrupt. spin_lock_irqsave() >>> would mask interrupts so there is no risk of a different notification firing on >>> the same CPU, (it looks like they are almost all ultimately an irq). >>> >>> NMI is the odd one out because its not maskable like this, but ghes_notify_nmi() >>> has: >>>> if (!atomic_add_unless(&ghes_in_nmi, 1, 1)) >>>> return ret; >>> To ensure there is only ever one thread poking around in this code. >>> >>> What happens if a system describes two GHES sources, one using an irq the other >>> SEA? The SEA error can interrupt the irq error while its holding the above lock. >>> I guess this is also why all the NMI code in that file is separate. > >> Let me see if I'm following you right :) >> I should use spin_lock_irqsave() in ghes_notify_sea() to avoid ghes_notify_sci() >> from >> interrupting this process and potentially causing the deadlock? > This way round you are already safe: The CPU masks interrupts when it takes the > exception, they should still be masked by the time we get in here... > > The other way round is a lot more fun! > > What happens if APEI is processing some error record that was notified via an > interrupt, and then takes the Synchronous External Abort, and ends up back in > this code? Masking interrupts doesn't stop the external-abort, and trying to > take the ghes_ioremap_lock_irq will deadlock. > > What happens if we interrupt printk() holding all its locks is another thing I > haven't worked out yet. > > >> This race condition does seem valid. We are using the same acknowledgment for >> all our >> HEST table entries, so our firmware will not populate more than one entry at a >> time. That >> gets us around this race condition. > Ah, so your firmware will wait for the interrupt-signalled error to be finished > before it triggers the Synchronous External Abort. I think this would still be a > linux bug if the firmware didn't do this. > > x86 could have done the same with NMI notifications, but we have all this 'if > (in_nmi)' to allow interrupts-masked GHES handling to be interrupted. > > What do you think to re-using the 'if (in_nmi)' code for SEA? We can argue that > SEA is NMI-like in that it can't be masked, and it interrupts code that had > interrupts masked. It 'should' be as simple as putting 'HAVE_NMI' in arm64's > Kconfig, and wrapping the atomic notifier call with nmi_enter()/nmi_exit() from > linux/hardirq.h. (...famous last words...) > > This probably answers my printk() questions too, but I need to look into it some > more. Thanks for the detailed description! I looked through this and it seems like re-using the NMI code should work. I'll add the use of the in_nmi code in the next patchset. Thanks, Tyler
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index b380c87..ae34349 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -53,6 +53,7 @@ config ARM64 select HANDLE_DOMAIN_IRQ select HARDIRQS_SW_RESEND select HAVE_ACPI_APEI if (ACPI && EFI) + select HAVE_ACPI_APEI_SEA if (ACPI && EFI) select HAVE_ALIGNED_STRUCT_PAGE if SLUB select HAVE_ARCH_AUDITSYSCALL select HAVE_ARCH_BITREVERSE diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig index b0140c8..3786ff1 100644 --- a/drivers/acpi/apei/Kconfig +++ b/drivers/acpi/apei/Kconfig @@ -4,6 +4,20 @@ config HAVE_ACPI_APEI config HAVE_ACPI_APEI_NMI bool +config HAVE_ACPI_APEI_SEA + bool "APEI Synchronous External Abort logging/recovering support" + depends on ARM64 + help + This option should be enabled if the system supports + firmware first handling of SEA (Synchronous External Abort). + SEA happens with certain faults of data abort or instruction + abort synchronous exceptions on ARMv8 systems. If a system + supports firmware first handling of SEA, the platform analyzes + and handles hardware error notifications with SEA, and it may then + form a HW error record for the OS to parse and handle. This + option allows the OS to look for such HW error record, and + take appropriate action. + config ACPI_APEI bool "ACPI Platform Error Interface (APEI)" select MISC_FILESYSTEMS diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index 2acbc60..66ab3fd 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -50,6 +50,10 @@ #include <acpi/apei.h> #include <asm/tlbflush.h> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA +#include <asm/system_misc.h> +#endif + #include "apei-internal.h" #define GHES_PFX "GHES: " @@ -767,6 +771,62 @@ static struct notifier_block ghes_notifier_sci = { .notifier_call = ghes_notify_sci, }; +#ifdef CONFIG_HAVE_ACPI_APEI_SEA +static LIST_HEAD(ghes_sea); + +static int ghes_notify_sea(struct notifier_block *this, + unsigned long event, void *data) +{ + struct ghes *ghes; + int ret = NOTIFY_DONE; + + rcu_read_lock(); + list_for_each_entry_rcu(ghes, &ghes_sea, list) { + if (!ghes_proc(ghes)) + ret = NOTIFY_OK; + } + rcu_read_unlock(); + + return ret; +} + +static struct notifier_block ghes_notifier_sea = { + .notifier_call = ghes_notify_sea, +}; + +static int ghes_sea_add(struct ghes *ghes) +{ + mutex_lock(&ghes_list_mutex); + if (list_empty(&ghes_sea)) + register_synchronous_ext_abort_notifier(&ghes_notifier_sea); + list_add_rcu(&ghes->list, &ghes_sea); + mutex_unlock(&ghes_list_mutex); + return 0; +} + +static void ghes_sea_remove(struct ghes *ghes) +{ + mutex_lock(&ghes_list_mutex); + list_del_rcu(&ghes->list); + if (list_empty(&ghes_sea)) + unregister_synchronous_ext_abort_notifier(&ghes_notifier_sea); + mutex_unlock(&ghes_list_mutex); +} +#else /* CONFIG_HAVE_ACPI_APEI_SEA */ +static inline int ghes_sea_add(struct ghes *ghes) +{ + pr_err(GHES_PFX "ID: %d, trying to add SEA notification which is not supported\n", + ghes->generic->header.source_id); + return -ENOTSUPP; +} + +static inline void ghes_sea_remove(struct ghes *ghes) +{ + pr_err(GHES_PFX "ID: %d, trying to remove SEA notification which is not supported\n", + ghes->generic->header.source_id); +} +#endif /* CONFIG_HAVE_ACPI_APEI_SEA */ + #ifdef CONFIG_HAVE_ACPI_APEI_NMI /* * printk is not safe in NMI context. So in NMI handler, we allocate @@ -1011,6 +1071,14 @@ static int ghes_probe(struct platform_device *ghes_dev) case ACPI_HEST_NOTIFY_EXTERNAL: case ACPI_HEST_NOTIFY_SCI: break; + case ACPI_HEST_NOTIFY_SEA: + if (!IS_ENABLED(CONFIG_HAVE_ACPI_APEI_SEA)) { + pr_warn(GHES_PFX "Generic hardware error source: %d notified via SEA is not supported\n", + generic->header.source_id); + rc = -ENOTSUPP; + goto err; + } + break; case ACPI_HEST_NOTIFY_NMI: if (!IS_ENABLED(CONFIG_HAVE_ACPI_APEI_NMI)) { pr_warn(GHES_PFX "Generic hardware error source: %d notified via NMI interrupt is not supported!\n", @@ -1022,6 +1090,13 @@ static int ghes_probe(struct platform_device *ghes_dev) pr_warning(GHES_PFX "Generic hardware error source: %d notified via local interrupt is not supported!\n", generic->header.source_id); goto err; + case ACPI_HEST_NOTIFY_GPIO: + case ACPI_HEST_NOTIFY_SEI: + case ACPI_HEST_NOTIFY_GSIV: + pr_warn(GHES_PFX "Generic hardware error source: %d notified via notification type %u is not supported\n", + generic->header.source_id, generic->header.source_id); + rc = -ENOTSUPP; + goto err; default: pr_warning(FW_WARN GHES_PFX "Unknown notification type: %u for generic hardware error source: %d\n", generic->notify.type, generic->header.source_id); @@ -1076,6 +1151,11 @@ static int ghes_probe(struct platform_device *ghes_dev) list_add_rcu(&ghes->list, &ghes_sci); mutex_unlock(&ghes_list_mutex); break; + case ACPI_HEST_NOTIFY_SEA: + rc = ghes_sea_add(ghes); + if (rc) + goto err_edac_unreg; + break; case ACPI_HEST_NOTIFY_NMI: ghes_nmi_add(ghes); break; @@ -1118,6 +1198,9 @@ static int ghes_remove(struct platform_device *ghes_dev) unregister_acpi_hed_notifier(&ghes_notifier_sci); mutex_unlock(&ghes_list_mutex); break; + case ACPI_HEST_NOTIFY_SEA: + ghes_sea_remove(ghes); + break; case ACPI_HEST_NOTIFY_NMI: ghes_nmi_remove(ghes); break;