Re: [PATCH 2/3] x86/mce/AMD, EDAC: Simplify SMCA decoding

[Aravind Gopalakrishnan]

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@xxxxxxx>
> Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@xxxxxxx>
>
>   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.