Re: [PATCH v4] x86: mce: kexec: switch MCE handler for kexec/kdump

[Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>]

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