linux-kernel - Re: [PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <874m4fs1zt.fsf@e105922-lin.cambridge.arm.com>
Date:   Fri, 14 Oct 2016 10:39:50 +0100
From:   Punit Agrawal <punit.agrawal@....com>
To:     "Baicar\, Tyler" <tbaicar@...eaurora.org>
Cc:     linux-efi@...r.kernel.org, matt@...eblueprint.co.uk,
        catalin.marinas@....com, will.deacon@....com,
        robert.moore@...el.com, paul.gortmaker@...driver.com,
        lv.zheng@...el.com, kvmarm@...ts.cs.columbia.edu,
        fu.wei@...aro.org,
        "Jonathan \(Zhixiong\) Zhang" <zjzhang@...eaurora.org>,
        linux@...linux.org.uk, linux-acpi@...r.kernel.org,
        shijie.huang@....com, lenb@...nel.org, marc.zyngier@....com,
        tomasz.nowicki@...aro.org, Naveen Kaje <nkaje@...eaurora.org>,
        rostedt@...dmis.org, sandeepa.s.prabhu@...il.com,
        Dkvm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        devel@...ica.org, rjw@...ysocki.net, linux-kernel@...r.kernel.org,
        pbonzini@...hat.com, akpm@...ux-foundation.org, bristot@...hat.com
Subject: Re: [PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8

"Baicar, Tyler" <tbaicar@...eaurora.org> writes:

> Hello Punit,
>
>
> On 10/12/2016 12:00 PM, Punit Agrawal wrote:
>> Tyler Baicar <tbaicar@...eaurora.org> writes:
>>
>>> 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.
>>>
>>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@...eaurora.org>
>>> Signed-off-by: Tyler Baicar <tbaicar@...eaurora.org>
>>> Signed-off-by: Naveen Kaje <nkaje@...eaurora.org>
>> This patch fails to apply for me on v4.8. Is there a different tree this
>> is based on?
> This patch was giving me some grief as well. I'm not sure why that is
> because this patchset was based on the 4.8 kernel with the dependent
> patch for initial APEI support.

That explains it!. I've missed out the dependency called out in the
cover letter.


>> One comment below.
>>
>> [...]
>>
>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>>> index c8488f1..28d5a09 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: "
>>> @@ -779,6 +783,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;
>> Not something you've introduced but it looks like ghes_proc erroneously
>> never returns anything other than 0. I plan to post the below fix to
>> address it.
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 60746ef..caea575 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -662,7 +662,7 @@ static int ghes_proc(struct ghes *ghes)
>>          ghes_do_proc(ghes, ghes->estatus);
>>   out:
>>          ghes_clear_estatus(ghes);
>> -   return 0;
>> + return rc;
>>   }
> Yes, this definitely should be fixed :)
>
> Thanks,
> Tyler
>>> +	}
>>> +	rcu_read_unlock();
>>> +
>>> +	return ret;
>>> +}
>>> +
>> [...]
>>