| 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