Message-ID: <dfb77246-3c13-9e37-d3d5-0975fc7c6567@codeaurora.org>
Date:   Tue, 10 Jan 2017 10:50:53 -0700
From:   "Baicar, Tyler" <tbaicar@...eaurora.org>
To:     James Morse <james.morse@....com>
Cc:     christoffer.dall@...aro.org, marc.zyngier@....com,
        pbonzini@...hat.com, rkrcmar@...hat.com, linux@...linux.org.uk,
        catalin.marinas@....com, will.deacon@....com, rjw@...ysocki.net,
        lenb@...nel.org, matt@...eblueprint.co.uk, robert.moore@...el.com,
        lv.zheng@...el.com, nkaje@...eaurora.org, zjzhang@...eaurora.org,
        mark.rutland@....com, akpm@...ux-foundation.org,
        eun.taik.lee@...sung.com, sandeepa.s.prabhu@...il.com,
        labbott@...hat.com, shijie.huang@....com, rruigrok@...eaurora.org,
        paul.gortmaker@...driver.com, tn@...ihalf.com, fu.wei@...aro.org,
        rostedt@...dmis.org, bristot@...hat.com,
        linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-acpi@...r.kernel.org, linux-efi@...r.kernel.org,
        devel@...ica.org, Suzuki.Poulose@....com, punit.agrawal@....com,
        astone@...hat.com, harba@...eaurora.org, hanjun.guo@...aro.org,
        john.garry@...wei.com, shiju.jose@...wei.com
Subject: Re: [PATCH V6 05/10] acpi: apei: handle SEA notification type for
 ARMv8

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.

Powered by blists - more mailing lists