Definitely a step in the right direction.
Now...
On Wed, Mar 09, 2022 at 11:41:07AM -0600, Carlos Bilbao wrote:
> AMD's severity grading covers very few machine errors. In the graded cases
> there are no user-readable messages, complicating debugging of critical
> hardware errors.
That's too generic. What is the actual use case here you're spending all
this time for?
> Fix the above issues extending the current grading logic for AMD with cases
> not previously considered and their corresponding messages.
>
> Signed-off-by: Carlos Bilbao <[email protected]>
> ---
> arch/x86/include/asm/mce.h | 6 +
> arch/x86/kernel/cpu/mce/severity.c | 232 +++++++++++++++++++++++++----
> 2 files changed, 205 insertions(+), 33 deletions(-)
Now, looking at the whole thing, AFAICT all you're interested in is
getting some strings out from those error types. But but, we already
have something like that. That's even mentioned in the patch:
> + * Default return values. The poll handler catches these and passes
> + * responsibility of decoding them to EDAC
So there's a big fat module mce_amd.c which does convert MCEs to
strings. So why can't that be used and extended instead of adding more
strings to more places in the kernel?
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 3/9/2022 12:37 PM, Borislav Petkov wrote:
> Definitely a step in the right direction.
>
> Now...
>
> On Wed, Mar 09, 2022 at 11:41:07AM -0600, Carlos Bilbao wrote:
>> AMD's severity grading covers very few machine errors. In the graded cases
>> there are no user-readable messages, complicating debugging of critical
>> hardware errors.
>
> That's too generic. What is the actual use case here you're spending all
> this time for?
>
We will cover grading of MCEs like deferred memory scrub errors, attempts
to access poisonous data, etc. I could list all new covered cases in the
commit message if you think that'd be positive.
>> Fix the above issues extending the current grading logic for AMD with cases
>> not previously considered and their corresponding messages.
>>
>> Signed-off-by: Carlos Bilbao <[email protected]>
>> ---
>> arch/x86/include/asm/mce.h | 6 +
>> arch/x86/kernel/cpu/mce/severity.c | 232 +++++++++++++++++++++++++----
>> 2 files changed, 205 insertions(+), 33 deletions(-)
>
> Now, looking at the whole thing, AFAICT all you're interested in is
> getting some strings out from those error types. But but, we already
> have something like that. That's even mentioned in the patch:
>
>> + * Default return values. The poll handler catches these and passes
>> + * responsibility of decoding them to EDAC
>
> So there's a big fat module mce_amd.c which does convert MCEs to
> strings. So why can't that be used and extended instead of adding more
> strings to more places in the kernel?
>
> Thx.
>
The severity grading logic has a double functionality: (1) Grade the
machine errors and (2) Assign them a severity message. The first task is
arguably more important because depending on the severity we assign, the
MCE handler do_machine_check() will do things differently.
The part of this patch that grades new errors is needed for (1). But -as
you mention- why do we need these new strings if we have the EDAC driver?
If we grade the error as a PANIC, the handler will not notify EDAC, and the
kernel will use our new severity messages when displaying the error with
mce_panic().
If the machine error is not graded as PANIC, the notification chain will
come into play, and EDAC will eventually decode the machine errors without
using the severity message. Hence, you have a good point, these new strings
will be unnecessary -We should send a new version of the patch that only
includes msgs for PANIC cases. We probably won't need helper functions by
then. I'll also take some time to think of any other error cases we might
not be covering yet.
Hope that helps clarify,
Carlos
On Thu, Mar 10, 2022 at 12:24:08PM -0600, Carlos Bilbao wrote:
> We will cover grading of MCEs like deferred memory scrub errors, attempts
> to access poisonous data, etc. I could list all new covered cases in the
> commit message if you think that'd be positive.
So no actual use case - you want to grade error severity for all types
of MCEs.
> Hope that helps clarify,
Yes, it does a bit.
It sounds to me like you want to do at least two patches:
1. Extend the severity grading function with the new types of errors
2. Add string descriptions of the error types mce_severity_amd() looks
at, so that mce_panic() issues them.
I.e., you want to decode the fatal MCEs which panic the machine.
In general, what would help is if you think about what you're trying to
achieve and write it down first. How to achieve that we can figure out
later.
What happens now is you send me a patch and I'm trying to decipher from
the code why you're doing what you're doing. Which is kinda backwards if
you think about it...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette