Re: [PATCH V6 05/10] acpi: apei: handle SEA notification type for ARMv8

[Baicar, Tyler]

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

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.