Re: [RFC 1/2] ACPI: APEI: Make memory_failure() triggered by synchronization errors execute in the current context

[Lv Ying <lvying6@huawei.com>]

On 2022/12/8 11:25, Shuai Xue wrote:
> + Xie XiuQi
> 
> On 2022/12/8 AM10:20, Lv Ying wrote:
>>
>>>
>>> We also encountered this problem in production environment, and tried to
>>> solve it by dividing synchronous and asynchronous error handling into different
>>> paths: task work for synchronous error and workqueue for asynchronous error.
>>>
>>> The main challenge is how to distinguish synchronous errors in kernel first
>>> mode through APEI, a related discussion is here.[1]
>>>
>> Hi Shuai:
>>
>> I'm very happy to have received your response, we encountered this problem in our production environment too. I spent a lot of time researching the rationale for synchronous errors and asynchronous errors to share the same process. I mention in my patch commit message: memory_failure() triggered by synchronous error is
>> executed in the kworker context, the early_kill mode of memory_failure()
>> will send wrong si_code by SIGBUS signal because of wrong current process.
>>
>> The challenge for my patch is to prove the rationality of distinguishing synchronous errors. I do not have a good idea of distinguishing synchronous error by looking through ACPI/UEFI spec, so I sent this patchset for more input. And I resent RFC PATCH v1 [1]add this as TODO.
>>
>>>> @@ -978,14 +979,14 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
>>>>            estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
>>>>            len = cper_estatus_len(estatus);
>>>>            node_len = GHES_ESTATUS_NODE_LEN(len);
>>>> -        task_work_pending = ghes_do_proc(estatus_node->ghes, estatus);
>>>> +        corruption_page_pending = ghes_do_proc(estatus_node->ghes, estatus, true);
>>>>            if (!ghes_estatus_cached(estatus)) {
>>>>                generic = estatus_node->generic;
>>>>                if (ghes_print_estatus(NULL, generic, estatus))
>>>>                    ghes_estatus_cache_add(generic, estatus);
>>>>            }
>>>
>>> In the case of your patch, it is inappropriate to assume that ghes_proc_in_irq() is only
>>> called to handle synchronous error. Firmware could notify all synchronous and asynchronous
>>> error signals to kernel through NMI notification, e.g. SDEI. In this case, asynchronous
>>> error will be treated as synchronous error.
>>>
>>
>> Yes, as I mentioned above, I do not have a good idea of distinguishing synchronous error. I agree with you that ghes_proc_in_irq() is called in SDEI, SEA, NMI notify type, they are NMI-like notify, this function run some job which may not be NMI safe in IRQ context. And NMI may be asynchronous error.
>>
>> However, cureent kernel use ghes_kick_task_work in ghes_proc_in_irq(), there is an assumption here that ghes_proc_in_irq() are currently in the context of a synchronous exception, although this is not appropriate.
>>
>>> Our colleague Yingwen has submitted a proposal to extend acpi_hest_generic_data::flag (bit 8)
>>> to indicate that the error is a synchronous[2]. Personally speaking, it is a more general
>>> solution and completely solves the problem.
>>>
>>>
>>>> Background:
>>>>
>>>> In ARM world, two type events (Sync/Async) from hardware IP need OS/VMM take different actions.
>>>> Current CPER memory error record is not able to distinguish sync/async type event right now.
>>>> Current OS/VMM need to take extra actions beyond CPER which is heavy burden to identify the
>>>> two type events
>>>>
>>>> Sync event (e.g. CPU consume poisoned data) --> Firmware  -> CPER error log  --> OS/VMM take recovery action.
>>>> Async event (e.g. Memory controller detect UE event)  --> Firmware  --> CPER error log  --> OS take page action.
>>>>
>>>>
>>>> Proposal:
>>>>
>>>> - In section description Flags field(UEFI spec section N.2, add sync flag as below. OS/VMM
>>>>    could depend on this flag to distinguish sync/async events.
>>>> - Bit8 – sync flag; if set this flag indicates that this event record is synchronous(e.g.
>>>>    cpu core consumes poison data, then cause instruction/data abort); if not set, this event record is asynchronous.
>>>>
>>>> Best regards,
>>>> Yingwen Chen
>>>
>>> A RFC patch set based on above proposal is here[3].
>>>
>>
>> I'm glad your team is working on advancing this thing in the UEFI community. This will make kernel identify synchronous as an easy job. Based on your proposal, I think your work is suitable.
>>
>> However, there are real problems here in the running production environment of the LTS kernel:
>> 1. this new proposal has not yet been accepted by the UEFI spec
>> 2. it will require firmware update in production environment which may be more difficult than kernel livepatch upgrade.
>>
>> If there are more ways to distinguish synchronous error in kernel APEI,
>> I can try it. For example, in APEI mode, is it suitable to use notify type to distinguish synchronous error?
>> synchronous error:
>> SDEI SEA
>> asynchronous error:
>> NMI
> 
> Sorry, I'm afraid it's not suitable. It is no doubt that SEA notification is synchronous,
> but we should not assume that platform use NMI notification for asynchronous errors,
> and SDEI notification for synchronous errors.
> 
> If we want to address the problem now, I prefer to just consider SEA notification is synchronous
> and add MF_ACTION_REQUIRED when SEA is adopt. Please refer to XiuQi's work.[1]
> 
> [1]https://lore.kernel.org/linux-arm-kernel/20221205160043.57465-4-xiexiuqi@huawei.com/T/
> 
> Best Regards,
> Shuai
> 
Thanks for your advice, it may be currently more practical to solve the 
problem that synchronous and asynchronous errors
share the same processing flow only in the SEA scenario. I will refer to 
XiuQi's work and update RFC PATCH v2.


-- 
Thanks!
Lv Ying