Re: [PATCH 0/3] Fix MCE handling for AMD multi-node processors

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

On 12/23/2014 1:41 PM, Aravind Gopalakrishnan wrote:
> On 12/22/2014 5:19 PM, Borislav Petkov wrote:
>> On Mon, Dec 22, 2014 at 02:56:47PM -0600, Aravind Gopalakrishnan wrote:
>>> On 12/22/2014 2:15 PM, Borislav Petkov wrote:
>>>> On Mon, Dec 22, 2014 at 02:10:09PM -0600, Aravind Gopalakrishnan 
>>>> wrote:
>>>>> When a MCE happens that is to be logged onto bank 4 of AMD multi-node
>>>>> processors, they are reported only to corresponding node base core of
>>>>> the cpu on which the error occurred.
>>>>>
>>>>> Refer D18F3x44[NbMcaToMstCpuEn] on BKDGs of Fam10h and later for
>>>> Let me try to understand this correctly:
>>>>
>>>> Does that mean that we could fix this by simply doing:
>>>>
>>>> D18F3x44[NbMcaToMstCpuEn]=0b
>>>>
>>>> on each NB?
>>>>
>>> Not quite..
>>> When this field is 0, BKDG says the error may be reported to the 
>>> core that
>>> originated the request *if applicable and known*
>>> Looking at the error signatures table for MC4 (Part 2),
>>> we can see only some errors have 'ErrCoreId' column as valid
>>>
>>> Besides, if IO originated the request, then it is reported only to NBC.
>>>
>>> So, to take care of all these cases, I am just following one 
>>> approach here:
>>> and that is to look at NBC MSRs for any bank 4 errors.
>>> (It seems to be what the BKDG recommends anyway as BIOS by default 
>>> should
>>> set D18F3x44[NbMcaToMstCpuEn])
>> Then in that case you have to check the case where
>> D18F3x44[NbMcaToMstCpuEn] is 0 for whatever reason (some BIOS forgot to
>> set it or whatever) and to set it again.
>

Hi Boris,
It seems my earlier understanding of hardware behavior was not 
completely right.
Here are some clarifications I have received after some internal discussion-
When D18F3x44[NBMstToMstCpuEn] is set, the interrupt is also routed to 
the NBC.

This was not immediately clear to me from the description for the field 
in the BKDG.
The BKDG states that errors are reported to the NBC and also that 
status, addr, ctl
MSRs for MC4 are only accessible from the NBC.
I took this to understand that the error info is written to the NBC MSRs 
while
the #MC could be generated from the non-NBC.

Now, given that setting NBMstToMstCpuEn ensures #MC is generated only on 
NBC for MC4 errors,
we don't have a problem to solve in the #MC handler code.
So, we can discard patch2 of the series,

But we still need to change the error injection interfaces in mce_amd_inj:
mce_amd_inj triggers a #MC on the cpu number that the user specifies on 
debugfs.
For any error other than MC4 errors, this is fine.
But we should really be triggering #MC only on NBC for MC4 errors.
and also make sure we use NBC to write to the status/addr MSRs as only 
NBC has access to
these MSRs.

And for this, we will still need some the changes introduced in patch 1 
and patch 3 entirely.

And since calculating a corresponding NBC for a given core will be 
needed only in mce_amd_inj,
I can move those calculations to mce_amd_inj.
(Or if you prefer it be in a common location then I can leave it as it 
as too)


>>
>> Also, the math in amd_get_nbc_for_node() is too fragile and will break
>> the moment some BIOS renumbers cores to accomodate some other OS.
>>
>

Could you please clarify this?

For cores_per_node I am using c->x86_max_cores which is a value obtained 
from cpuid_ecx(0x80000008);
and NodesPerProcessor is obtained from cpuid_ecx(0x8000001e); (or 
MSR_FAM10H_NODE_ID)

These values should still be consistent for other OS too right?

Also, these are the values that we have currently in amd_get_topology(). 
I simply refactored the code..

Thanks,
-Aravind.