Subject: Re: [PATCH 2/3] x86/mce/AMD, EDAC: Simplify SMCA decoding
To: Borislav Petkov <bp@alien8.de>
References: <1456785179-14378-1-git-send-email-Aravind.Gopalakrishnan@amd.com>
 <1456785179-14378-3-git-send-email-Aravind.Gopalakrishnan@amd.com>
 <20160302105032.GC16954@pd.tnic> <20160302105353.GE16954@pd.tnic>
CC: <tony.luck@intel.com>, <hpa@zytor.com>, <mingo@redhat.com>,
        <tglx@linutronix.de>, <dougthompson@xmission.com>,
        <mchehab@osg.samsung.com>, <x86@kernel.org>,
        <linux-edac@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <ashok.raj@intel.com>, <gong.chen@linux.intel.com>,
        <len.brown@intel.com>, <peterz@infradead.org>, <ak@linux.intel.com>,
        <alexander.shishkin@linux.intel.com>
From: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
Message-ID: <56D70C37.1090902@amd.com>
Date: Wed, 2 Mar 2016 09:52:23 -0600
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <20160302105353.GE16954@pd.tnic>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:23
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Mar 2016 15:52:27.6539
 (UTC)
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.221];Helo=[atltwp01.amd.com]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR12MB0861
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 5337
Lines: 166

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@suse.de>
> Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.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.