Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v5] ACPI / APEI: fix the regression of synchronous
 external aborts occur in user-mode
To:     James Morse <james.morse@arm.com>, <rafael@kernel.org>,
        <rjw@rjwysocki.net>, <lenb@kernel.org>, <tony.luck@intel.com>,
        <bp@alien8.de>, <akpm@linux-foundation.org>, <jroedel@suse.de>,
        <peterz@infradead.org>
References: <1607602177-1507-1-git-send-email-tanxiaofei@huawei.com>
 <d57d786c-f9cb-46ba-78d0-3675666272f2@arm.com>
 <5b444714-aa45-517f-9595-fd5889d3c342@huawei.com>
CC:     <linux-acpi@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linuxarm@huawei.com>
From:   Xiaofei Tan <tanxiaofei@huawei.com>
Message-ID: <4e1416f4-7556-8885-6fa5-bdf8336ae9f6@huawei.com>
Date:   Tue, 8 Jun 2021 17:15:05 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.7.1
MIME-Version: 1.0
In-Reply-To: <5b444714-aa45-517f-9595-fd5889d3c342@huawei.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


Hi James,

On 2021/6/5 16:59, Xiaofei Tan wrote:
> Hi James,
>
> On 2021/6/4 22:19, James Morse wrote:

...

>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>>> index fce7ade..0893968 100644
>>> --- a/drivers/acpi/apei/ghes.c
>>> +++ b/drivers/acpi/apei/ghes.c
>>
>>> +static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev)
>>> +{
>>> +    struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
>>> +    struct cper_arm_err_info *err_info;
>>> +    bool queued = false;
>>> +    int sec_sev, i;
>>> +
>>> +    log_arm_hw_error(err);
>>> +
>>> +    sec_sev = ghes_severity(gdata->error_severity);
>>> +    if (sev != GHES_SEV_RECOVERABLE || sec_sev != GHES_SEV_RECOVERABLE)
>>> +        return false;
>>> +
>>> +    err_info = (struct cper_arm_err_info *) (err + 1);
>>> +    for (i = 0; i < err->err_info_num; i++, err_info++) {
>>
>> err_info has a version and a length, so its expected to be made bigger at some point.
>
>
> Yes, but the table "ARM Processor Error Section" fixed the length of
> processor error information structure to fixed 32 bytes. Then the
> description of the UEFI spec need to update.
>
>> It would be better to use the length instead of 'err_info++', or at least to break out of
>> the loop if a length > sizeof(*err_info) is seen.
>>
>
> OK. Maybe check length != sizeof(*err_info) is better. I will add this.
>
>     if (err_info->length != sizeof(struct cper_arm_err_info)) {
>         pr_warn_ratelimited(FW_WARN GHES_PFX
>                     "Error info length %d is invalid\n",
>                     err_info->length);
>         break;
>     }
>

I considered this more carefully. It should be better to use the length instead of 'err_info', than
just break out. Because if we want to expand "ARM Processor Error Infomation Structure", the proper way
is just add some new member at the end of the structure. Then we have a chance to work well for the situation
of new firmware + old kernel.

>> With that:
>> Reviewed-by: James Morse <james.morse@arm.com>
>>
>>
>> The following nits would make this easier to read:
>>


>>
>
> Right.
>
>>
>> Thanks,
>>
>> James
>>
>> .
>>
>
>
> .
>