Re: [PATCH v2 1/3] x86/mce/inject: Remove call to mce_notify_irq()

[Nikolay Borisov <nik.borisov@suse.com>]

On 17.02.25 г. 12:01 ч., Nikolay Borisov wrote:
> 
> 
> On 16.02.25 г. 18:10 ч., Borislav Petkov wrote:
>> On Mon, Feb 10, 2025 at 05:47:04PM +0200, Nikolay Borisov wrote:
>>> The call is actually a noop because when the MCE is raised the early
>>> notifier is the only call site that correctly calls mce_notify_irq()
>>> because it also sets mce_need_notify. Remove this call and as a result
>>> make mce_notify_irq() static
>>>
>>> Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
>>> ---
>>>   arch/x86/include/asm/mce.h       |  2 --
>>>   arch/x86/kernel/cpu/mce/core.c   | 44 ++++++++++++++++----------------
>>>   arch/x86/kernel/cpu/mce/inject.c |  1 -
>>>   3 files changed, 22 insertions(+), 25 deletions(-)
>>
>> So what you're looking at are the remnants of the old soft-inject of MCE
>> errors. And it seems that we lost some of that functionality along the 
>> way and
>> no one has noticed because, well, it seems no one uses it anymore.
>>
>> In order to understand how this thing was supposed to work, checkout
>>
>> ea149b36c7f5 ("x86, mce: add basic error injection infrastructure")
> 
> 
> The original code in  ea149b36c7f5 was setting the notify_user bit via
> 
> raise_mce()->machine_check_poll()->mce_log(),
>   mce_notify_user() - consumes notify_user set in mce_log above.
> 
> 
>   subsequently in 011d82611172 ("RAS: Add a Corrected Errors Collector") 
> you factored out the code from mce_log() mce_first_notifier, where the 
> bit setting remains to this day, but since it's been removed from 
> mce_log() it made the call in raise_local() defunct.
> 
> 
> Considering that no one complained about this after all these years and 
> that the dev-mcelog is deprecated I think it still makes sense to make 
> mce_notify_irq() private


Actually there is no loss of functionality since after an MCE is injected
the early notifier will correctly call the usermode helper. So I
think the following change log better reflects the situation:


     x86/mce/inject: Remove call to mce_notify_irq()
     
     The call to mce_notify_irq() has been there since the initial version of
     the soft inject mce machinery, introduced in ea149b36c7f5 ("x86,
     mce: add basic error injection infrastructure"). At that time it was
     functional since injecting an MCE resulted in the following call chain:
     
     raise_mce()
       ->machine_check_poll()
           ->mce_log() - sets notfiy_user_bit
     
     ->mce_notify_user() (current mce_notify_irq) consumed the bit and called the
     usermode helper.
     
     However, with the introduction of 011d82611172 ("RAS: Add a Corrected Errors Collector")
     the code got moved around and the user mode helper began to be called
     via the early notifier (mce_first_notifier()) rendering the remaining call
     in raise_local() defunct as the mce_need_notify bit (ex notify_user) is
     only being set from the early notifier.
     
     Remove the noop call and make mce_notify_irq static. No functional
     changes.

> 
>>
>> and follow what raise_mce() does and pay attention to notify_user 
>> which is
>> what mce_need_notify was called back then.
>>
>> Remember to have fun :-P
>>
>