lkml.org 
[lkml]   [2016]   [Feb]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 1/4] EDAC, MCE, AMD: Enable error decoding of Scalable MCA errors
Date


On 2/23/16 6:37 AM, Borislav Petkov wrote:
> On Tue, Feb 16, 2016 at 03:45:08PM -0600, Aravind Gopalakrishnan wrote:
>> /* AMD-specific bits */
>> #define MCI_STATUS_DEFERRED (1ULL<<44) /* declare an uncorrected error */
>> #define MCI_STATUS_POISON (1ULL<<43) /* access poisonous data */
>> +#define MCI_STATUS_TCC (1ULL<<55) /* Task context corrupt */
> \n
>

Ack.

>> +/*
>> + * McaX field if set indicates a given bank supports MCA extensions:
>> + * - Deferred error interrupt type is specifiable by bank
>> + * - BlkPtr field indicates prescence of extended MISC registers.
> ^^^^^^^^^
>
> Btw, that's MCi_MISC[BlkPtr] ?

MCi_MISC0[BlkPtr] specifically. Will update the comments about this.

> Also, please integrate a spell checker into your patch creation
> workflow.

Sorry about that. Looks like this pair is not defined in spelling.txt.
So, might be worth adding there as well?

>> + * But should not be used to determine MSR numbers
>> + * - TCC bit is present in MCx_STATUS
> All sentences end with a "."

Will fix.

>
>> +/*
>> + * Enumerating new IP types and HWID values
> Please stop using gerund, i.e., -ing, in comments and commit messages.
>
> "Enumerate new IP ..." is just fine.

Ack.

>
>> + * in ScalableMCA enabled AMD processors
>> + */
>> +#ifdef CONFIG_X86_MCE_AMD
>> +enum ip_types {
> AMD-specific so "amd_ip_types"

Ok, will fix.

>
>> + F17H_CORE = 0, /* Core errors */
>> + DF, /* Data Fabric */
>> + UMC, /* Unified Memory Controller */
>> + FUSE, /* FUSE subsystem */
> What's FUSE subsystem?

It's the block for programming FUSE registers.

>
> In any case, this could use a different name in order not to confuse
> with Linux's filesystem in userspace.

Ok, will ask internally as well as to what name suits here.

>> +
>> +struct hwid {
> amd_hwid and so on. All below should have the "amd_" prefix so that it
> is obvious.

Will fix.

>
>> + const char *ipname;
>> + unsigned int hwid_value;
>> +};
>> +
>> +extern struct hwid hwid_mappings[N_IP_TYPES];
>> +
>> +enum core_mcatypes {
>> + LS = 0, /* Load Store */
>> + IF, /* Instruction Fetch */
>> + L2_CACHE, /* L2 cache */
>> + DE, /* Decoder unit */
>> + RES, /* Reserved */
>> + EX, /* Execution unit */
>> + FP, /* Floating Point */
>> + L3_CACHE /* L3 cache */
>> +};
>> +
>> +enum df_mcatypes {
>> + CS = 0, /* Coherent Slave */
>> + PIE /* Power management, Interrupts, etc */
>> +};
>> +#endif
> So all those are defined here but we have a header for exactly that
> drivers/edac/mce_amd.h. And then you define and export hwid_mappings in
> arch/x86/kernel/cpu/mcheck/mce_amd.c to not use it there.
>
> Why isn't all this in drivers/edac/mce_amd.[ch] ?
>
> And if it is there, then you obviously don't need the "amd_" prefix.

I have a patch that uses these enums here. But I didn't send it out
along with this patchset as I was testing the patch.
So yes, I need it here and in the EDAC driver.

>
>> +
>> #endif /* _ASM_X86_MCE_H */
>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> index 5523465..93bccbc 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -266,7 +266,9 @@
>>
>> /* 'SMCA': AMD64 Scalable MCA */
>> #define MSR_AMD64_SMCA_MC0_CONFIG 0xc0002004
>> +#define MSR_AMD64_SMCA_MC0_IPID 0xc0002005
>> #define MSR_AMD64_SMCA_MCx_CONFIG(x) (MSR_AMD64_SMCA_MC0_CONFIG + 0x10*(x))
>> +#define MSR_AMD64_SMCA_MCx_IPID(x) (MSR_AMD64_SMCA_MC0_IPID + 0x10*(x))
> Are those MSRs used in multiple files? If not, -> mce.h.

Yes, I'll need them in arch/x86/.../mce_amd.c as well.
A later patch will be using it there.

>>
>>
>> +/* Defining HWID to IP type mappings for Scalable MCA */
> " Define ..."

Ack

>
>> + case L3_CACHE:
>> + if (xec > (ARRAY_SIZE(f17h_l3_mce_desc) - 1))
>> + goto wrong_f17hcore_error;
>> +
>> + pr_cont("%s.\n", f17h_l3_mce_desc[xec]);
>> + break;
>> +
>> + default:
>> + goto wrong_f17hcore_error;
> That's a lot of repeated code. You can assign the desc array to a temp
> variable depending on mca_type and do the if and pr_cont only once using
> that temp variable.

Ok, will simplify.

>
>> +
>> + case PIE:
>> + if (xec > (ARRAY_SIZE(f17h_pie_mce_desc) - 1))
>> + goto wrong_df_error;
>> +
>> + pr_cont("%s.\n", f17h_pie_mce_desc[xec]);
>> + break;
> Ditto.
>

Will fix.

>> +
>> +/* Decode errors according to Scalable MCA specification */
>> +static void decode_smca_errors(struct mce *m)
>> +{
>> + u32 low, high;
>> + u32 addr = MSR_AMD64_SMCA_MCx_IPID(m->bank);
>> + unsigned int hwid, mca_type, i;
>> + u8 xec = XEC(m->status, xec_mask);
>> +
>> + if (rdmsr_safe(addr, &low, &high)) {
>> + pr_emerg("Unable to decode errors from banks\n");
> That statement is not very useful in its current form.

How about "Unable to gather IP block that threw the error. Therefore
cannot decode errors further.\n"

Or should I just remove the pr_emerg()?

>
>> + for (i = 0; i < ARRAY_SIZE(hwid_mappings); i++)
> So you use those hwid_mappings here. Why aren't they defined here too?

Same reason as the enums-
In a subsequent patch, I'll need it in arch/x86/../mce_amd.c
(I should have probably indicated this in the cover letter so you were
aware of it. Sorry about that)

>
>> + case SMU:
>> + pr_emerg(HW_ERR "SMU Error: ");
>> + if (xec > (ARRAY_SIZE(f17h_smu_mce_desc) - 1)) {
>> + pr_cont("Unrecognized error code from SMU MCA bank\n");
>> + return;
>> + }
>> + pr_cont("%s.\n", f17h_smu_mce_desc[xec]);
>> + break;
> Also a lot of repeated code which could be simplified.

Hmm, will try to simplify it in V2.

>
>>
>> + if (mce_flags.smca) {
> ERROR: "mce_flags" [drivers/edac/edac_mce_amd.ko] undefined!
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
> make: *** Waiting for unfinished jobs....
>
> Just read CPUID again here instead of exporting mce_flags.

Right. Will fix this.

>
>> + u32 smca_low, smca_high;
>> + u32 smca_addr = MSR_AMD64_SMCA_MCx_CONFIG(m->bank);
> s/smca_//

Will fix.

>
> Also, all those MSRs should be defined in drivers/edac/mce_amd.h if not
> used outside.

It will be used in arch/x86/../mce_amd.c

>
>> +
>> + if (!rdmsr_safe(smca_addr, &smca_low, &smca_high) &&
>> + (smca_low & MCI_CONFIG_MCAX))
>> + pr_cont("|%s",
> No need for that break here.

Ok, Will fix.

>
>>
>> + case 0x17:
>> + xec_mask = 0x3f;
>> + if (!mce_flags.smca) {
>> + printk(KERN_WARNING "Decoding supported only on Scalable MCA enabled processors\n");
> What is that supposed to do? I thought all new ones will have SMCA...
>
>
If for some reason the CPUID bit is not set, then we should not assume
the processor supports the features right?

Thanks,
-Aravind.

\
 
 \ /
  Last update: 2016-02-24 00:41    [W:0.156 / U:1.124 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site