Messages in this thread | | | From | Naoya Horiguchi <> | Subject | Re: [PATCH v4] x86: mce: kexec: switch MCE handler for kexec/kdump | Date | Thu, 5 Mar 2015 01:24:47 +0000 |
| |
On Wed, Mar 04, 2015 at 11:12:33PM +0000, Luck, Tony wrote: > > - fixed AR and UC order in enum severity_level because UC is severer than AR > > by definition. Current code is not affected by this wrong order by chance. > > AR and AO are both UC errors - that happen also to be recoverable.
Maybe just saying "UC" might be confusing, meaning "UC bit is set" or "type of error (defined in Table SDM's Table 15-6 in vol.3B) is 'Uncorrected Error' (clearly separate from SRAR/SRAO)". You seem to mean the former, and I meant the latter. So I should write the description more accurately like "UC=1,PCC=0" error and "UC=1,PCC=1" error.
> Are you really sure > about this re-order not affecting existing code?
Sorry, I thought I checked that but I missed one, so let me check again now. I checked all referencing site of MCE_*_SEVERITY. Most of them are using '==' to compare the severity, where the re-order doesn't affect them. Some are using inequalities:
- around ./arch/x86/kernel/cpu/mcheck/mce.c:720,
if (mce_severity(m, mca_cfg.tolerant, msg, true) >= MCE_PANIC_SEVERITY) , the re-order doesn't affect.
- ./arch/x86/kernel/cpu/mcheck/mce.c:819:
if (m && global_worst >= MCE_PANIC_SEVERITY && mca_cfg.tolerant < 3)
, the re-order doesn't affect.
- ./arch/x86/kernel/cpu/mcheck/mce.c:832:
if (global_worst <= MCE_KEEP_SEVERITY && mca_cfg.tolerant < 3)
, ditto.
- ./arch/x86/kernel/cpu/mcheck/mce.c:1196:
no_way_out = worst >= MCE_PANIC_SEVERITY;
, ditto.
- ./arch/x86/kernel/cpu/mcheck/mce-severity.c:211:
if (s->sev >= MCE_UC_SEVERITY && ctx == IN_KERNEL) {
, the re-order should change to s->sev >= MCE_AR_SEVERITY to keep the kernel behavior.
So I fixed the last part to be included in the re-order patch.
> You might well be right, but as every one > else has pointed out mce_severity() is full of odd subtleties that catch people out.
I agree that this one big table is hard to maintain and bug-prone. One problem is that it has too many fields to check so the parameter space is huge. I think some field are checked only once, so separating it out makes table more simple and readable.
> Is the "UC" entry at the end of the severities[] table just a catch-all for things that made it > past all the other entries? Does it ever really get used?
I read through the severity check table and it seems that all UC=1 case are already considered by the above entries, so it seems not used.
> What was the test case that made you promote UC above AR?
I thought of "Action required but unaffected thread is continuable" case on kexec kernel, but I'm not sure that such a case really happens. My motivation on the promotion was mainly from what SDM says rather than the real testcase.
> This absolutely should not be buried in the middle of your other patch - it needs to > be separate with a much more than two lines of commit description.
OK, I might not include this part in this series in later post, but if I do, I'll separete it out.
Thanks, Naoya Horiguchi
| |