lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 2 Mar 2016 09:52:23 -0600 From: Aravind Gopalakrishnan <aravind.gopalakrishnan@....com> To: Borislav Petkov <bp@...en8.de> CC: <tony.luck@...el.com>, <hpa@...or.com>, <mingo@...hat.com>, <tglx@...utronix.de>, <dougthompson@...ssion.com>, <mchehab@....samsung.com>, <x86@...nel.org>, <linux-edac@...r.kernel.org>, <linux-kernel@...r.kernel.org>, <ashok.raj@...el.com>, <gong.chen@...ux.intel.com>, <len.brown@...el.com>, <peterz@...radead.org>, <ak@...ux.intel.com>, <alexander.shishkin@...ux.intel.com> Subject: Re: [PATCH 2/3] x86/mce/AMD, EDAC: Simplify SMCA decoding On 3/2/2016 4:53 AM, Borislav Petkov wrote: > Merge all IP blocks into a single enum. This allows for easier block > name use later. Drop superfluous "_BLOCK" from the enum names. > > Signed-off-by: Borislav Petkov <bp@...e.de> > Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@....com> > > enum amd_ip_types { > - SMCA_F17H_CORE_BLOCK = 0, /* Core errors */ > - SMCA_DF_BLOCK, /* Data Fabric */ > - SMCA_UMC_BLOCK, /* Unified Memory Controller */ > - SMCA_PB_BLOCK, /* Parameter Block */ > - SMCA_PSP_BLOCK, /* Platform Security Processor */ > - SMCA_SMU_BLOCK, /* System Management Unit */ > + SMCA_F17H_CORE = 0, /* Core errors */ > + SMCA_LS, /* - Load Store */ > + SMCA_IF, /* - Instruction Fetch */ > + SMCA_L2_CACHE, /* - L2 cache */ > + SMCA_DE, /* - Decoder unit */ > + RES, /* - Reserved */ > + SMCA_EX, /* - Execution unit */ > + SMCA_FP, /* - Floating Point */ > + SMCA_L3_CACHE, /* - L3 cache */ > + > + SMCA_DF, /* Data Fabric */ > + SMCA_CS, /* - Coherent Slave */ > + SMCA_PIE, /* - Power management, Interrupts, etc */ > + > + SMCA_UMC, /* Unified Memory Controller */ > + SMCA_PB, /* Parameter Block */ > + SMCA_PSP, /* Platform Security Processor */ > + SMCA_SMU, /* System Management Unit */ > N_AMD_IP_TYPES > }; > No, this would break the logic in EDAC. The main reason I placed it in separate enums is because the "mca_type" values map to the enum. These blocks- + SMCA_LS, /* - Load Store */ + SMCA_IF, /* - Instruction Fetch */ + SMCA_L2_CACHE, /* - L2 cache */ + SMCA_DE, /* - Decoder unit */ + RES, /* - Reserved */ + SMCA_EX, /* - Execution unit */ + SMCA_FP, /* - Floating Point */ + SMCA_L3_CACHE, /* - L3 cache */ have the same hwid value (0xb0). But they differ in mca_type values. And in exactly that order. (LS is mca_type 0, IF is mca_type 1 and so on..) So, after we get mca_type value from the MSR (mca_type = (high & MCI_IPID_MCATYPE) >> 16), We check for LS (=0) or IF (=1) ... With this change, LS block is assigned 1 due to the ordering in enum. And this logic applies to "DF" block as well. (whose component blocks are "coherent slave" and "pie" which have mca_type values of 0 and 1 respectively) "DF" and "F17h_core" are essentially parent blocks and CS, PIE, LS, IF etc are children. hwid indicates the parent, mca_type indicates the child.. > > > /* Define HWID to IP type mappings for Scalable MCA */ > -struct amd_hwid amd_hwid_mappings[] = > -{ > - [SMCA_F17H_CORE_BLOCK] = { "f17h_core", 0xB0 }, > - [SMCA_DF_BLOCK] = { "data fabric", 0x2E }, > - [SMCA_UMC_BLOCK] = { "UMC", 0x96 }, > - [SMCA_PB_BLOCK] = { "param block", 0x5 }, > - [SMCA_PSP_BLOCK] = { "PSP", 0xFF }, > - [SMCA_SMU_BLOCK] = { "SMU", 0x1 }, > +struct amd_hwid amd_hwids[] = > +{ > + [SMCA_F17H_CORE] = { "F17h core", 0xB0 }, > + [SMCA_LS] = { "Load-Store", 0x0 }, > + [SMCA_IF] = { "IFetch", 0x0 }, > + [SMCA_L2_CACHE] = { "L2 Cache", 0x0 }, > + [SMCA_DE] = { "Decoder", 0x0 }, > + [SMCA_EX] = { "Execution", 0x0 }, > + [SMCA_FP] = { "Floating Point", 0x0 }, > + [SMCA_L3_CACHE] = { "L3 Cache", 0x0 }, > + [SMCA_DF] = { "Data Fabric", 0x2E }, > + [SMCA_CS] = { "Coherent Slave", 0x0 }, > + [SMCA_PIE] = { "PwrMan/Intr", 0x0 }, > + [SMCA_UMC] = { "UMC", 0x96 }, > + [SMCA_PB] = { "Param Block", 0x5 }, > + [SMCA_PSP] = { "PSP", 0xFF }, > + [SMCA_SMU] = { "SMU", 0x1 }, > }; > -EXPORT_SYMBOL_GPL(amd_hwid_mappings); > +EXPORT_SYMBOL_GPL(amd_hwids); > These strings are what I intend to use for the sysfs interface. So, I am not sure if "PwrMan/Intr" would work when I need to create the kobj.. Also, the legacy names use snake_case- static const char * const th_names[] = { "load_store", "insn_fetch", "combined_unit", "", "northbridge", "execution_unit", }; So, I think we should continue this approach and have something like this- static const char * const amd_core_mcablock_names[] = { [SMCA_LS] = "load_store", [SMCA_IF] = "insn_fetch", [SMCA_L2_CACHE] = "l2_cache", [SMCA_DE] = "decode_unit", [RES] = "", [SMCA_EX] = "execution_unit", [SMCA_FP] = "floating_point", [SMCA_L3_CACHE] = "l3_cache", }; static const char * const amd_df_mcablock_names[] = { [SMCA_CS] = "coherent_slave", [SMCA_PIE] = "pie", }; (Split arrays again because I feel it'd be better to have arrays ordered according to mca_type values) Expanding "df" to "data_fabric" and "pb" to "param_block" is fine with me. > > > if (xec > len) { > - pr_cont("Unrecognized error code from %s MCA bank\n", ip_name); > + pr_cont("Unrecognized error code from %s MCA bank\n", amd_hwids[mca_type].name); This wouldn't work because of the mca_type ordering as mentioned above. (Or it should be amd_core_mcablock_names[mca_type]) > > if (xec > len) { > - pr_cont("Unrecognized error code from %s MCA bank\n", ip_name); > + pr_cont("Unrecognized error code from %s MCA bank\n", amd_hwids[mca_type].name); > return; > } > Ditto. > > > - pr_emerg(HW_ERR "%s Error: ", ip_name); > + ip_name = amd_hwids[mca_type].name; This should be amd_hwids[i].name We shouldn't be using mca_type value as index.. Thanks, -Aravind.
Powered by blists - more mailing lists