Received-SPF: pass (google.com: domain of linux-kernel+bounces-155806-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <eb9c5d9b-07d2-4b56-98dd-c2616ef73a0a@amd.com>
Date: Tue, 23 Apr 2024 15:32:00 -0400
User-Agent: Mozilla Thunderbird
Cc: yazen.ghannam@amd.com, linux-edac@vger.kernel.org,
 linux-kernel@vger.kernel.org, tony.luck@intel.com, x86@kernel.org,
 Avadhut.Naik@amd.com, John.Allen@amd.com
Subject: Re: [PATCH v2 05/16] x86/mce/amd: Clean up SMCA configuration
Content-Language: en-US
To: Borislav Petkov <bp@alien8.de>
References: <20240404151359.47970-1-yazen.ghannam@amd.com>
 <20240404151359.47970-6-yazen.ghannam@amd.com>
 <20240423190641.GDZigGwXXEPeDnfOsr@fat_crate.local>
From: Yazen Ghannam <yazen.ghannam@amd.com>
In-Reply-To: <20240423190641.GDZigGwXXEPeDnfOsr@fat_crate.local>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?RkhKMUdscVI3clRyeVdzZDBRVGZXdkVUdlFJRk84OG5uaG14RFRES3VzT2Ja?=
 =?utf-8?B?SnNubERLV2RIc0NnaHVVa0ZWZ1RMSFRzdkVTQkZrVGtjYnFSRUMwbGxETkpj?=
 =?utf-8?B?eGlacnRzSWk0dzd2L3BKMmJ5RVk3d1NZcEQzVGoyODhrZUJiS2hMb1EraXdm?=
 =?utf-8?B?QXdaSFdSQTdBcEdRT25Yb0thc2xDNVNhdnhhWVZ3SmVIUWt1NkpGWmp4bHFM?=
 =?utf-8?B?a3J3Qkl5WkxjbUtYMXJBUEsxZXZCTFhCWVpkVzU4aDVjQ2c2UytnTFZvNklM?=
 =?utf-8?B?SWk0OFcwYWpheUUyTjV3VGZwd2kwa0tKTWZIWkdKM0FqLzV6UlNEYkFEUDh1?=
 =?utf-8?B?bkZMRHJ3bHZjK0NjMlZqK2QycnZESUdVMlUraGp0cmlzOG95MHZUbURRYndQ?=
 =?utf-8?B?N2RNWnduUjRCTkZiZFpaVUpYOXM1Q1NhV1dFR1g5bW94ZHppR0NSbjhyTm9D?=
 =?utf-8?B?QnJGR1l1eC9kaEIwS3YzUkdGSEpxdlB1eVdEV1I1U29BemUvWlFTaXNRNVgr?=
 =?utf-8?B?eHVSeGI5NzlJdFlmSlZ4TVFEV3FzVEZIRDEzclRYYnMxQnliV3VoMTJBTXQr?=
 =?utf-8?B?STlFWjZXRWE5dWhzeUpabmY0S1cveGJrWUcvWXB4VlpIUjNiM2lrWEZiYTVZ?=
 =?utf-8?B?ZnQzNjhjb29XWjFFQXZiSFowYURoN3dCV3JONVplaGp3d0FoQWFvbGJudWZt?=
 =?utf-8?B?WjRCZGl6L21wdW5hN29QMXZSd0xPY1drSE1jeXQ3OGQ3QjcxMnQrSzhRVVBD?=
 =?utf-8?B?MytZZENidWZPdTRTc2FqbnErRkZkVDJRbHdVbXRPaVhIU0s5eEtlcDZjR04w?=
 =?utf-8?B?MFo1VWZtTkxWRGR1Qm91b2x6NWphSzRmSDJlQU45S1JRUUt1bU8xSmpMVlh2?=
 =?utf-8?B?MkNsVW9xVGlrMU96ZmhHQVY3aHJNMnlhTVM5bmt1TXVpcGUrU1FRNVJuMEdz?=
 =?utf-8?B?ZFQvb0hFRVRDR1dIOFdmQmlOclplWTRFUlVBWW5ySVhabmNGQ0M2OXJiZUxY?=
 =?utf-8?B?ZWx2NHpUc05ETFJVZVlnaDc4RXVrTnVzQ2dwejdnOXdpZmMwSWVQamYvVzRm?=
 =?utf-8?B?OHZDaXBaS0V1OCtYSUVnTy9SQWg1QmIrSVJSeU84TlZwU2MyRWhGUnVudGN0?=
 =?utf-8?B?eEVieGNEUjBQaDdWV3lUeW5Vb0hxWFJoV0VFYzBLMHpZeHM4KzBVU0RWL1pK?=
 =?utf-8?B?cDhiZEhRblRjcGNVOVhMZDBvcjRDMFgvMXg2eWlQUUpnV3BLZ3VXSm1lTDdh?=
 =?utf-8?B?d3hyQWg2ZlNJdlViSDRrSnNGcFB4N2kxdkVQb3UxdGNZcnBIYXQ4NGkwVmNM?=
 =?utf-8?B?SXNzbFhnR3Z1QzRUU0lQbmNWd1BESURNa2laQVoyUVMySXBQL1JMazEvcXJj?=
 =?utf-8?B?VlFQTzk4aVM3S0Vld3NURktudUtyV1FjcDV0SGtYSk4zdkdpanc3cWFHMHdQ?=
 =?utf-8?B?Y3F3S2RITUhNK2FrdVMxbVdmYkRvNlJkNDhJaTBOQlBYREN2MTgyTmtQQ25i?=
 =?utf-8?B?VUtpdVlMOWRBc1h1L1hVeDd5TFpsZmlBeDBucUxIV2RnV21FUUY3cCtVd20w?=
 =?utf-8?B?KyttVE5WNGVvNTBsdk9zNUtpckdRaTlNNXNSSGc5bVRuK2pIaktiNEdKOHEv?=
 =?utf-8?B?UWpMYmRPeFpENkpLUEdsb3E5WnJTL2lXODZ6cVJ3NERBWkk2c0V2Y1VPUkdE?=
 =?utf-8?B?enMyMTFMK0g5ZjBrQkdTdWxxRUtXVGZNMzBsa2FXcUhvaER2Z1hBSktkbEo0?=
 =?utf-8?B?eStIWDgzNHQ2c2pqaEFzd09pL1p2UUhWdklPQ2JWZyt0TWhBRDJEc2laSks3?=
 =?utf-8?B?dGR0MG5VUU01M1FSR05rOCs0NUFtemhJazFJajUxRE16QWwxbEFrUjZ6Q2Vy?=
 =?utf-8?B?UHN6UTUyTFZPSisvenIvYUhxVkRpMElEK2Z5cVJMR1JQNFE1SjU4V2JWeGZ5?=
 =?utf-8?B?VG0wci8vbU1iYTRtWVd0cmIvUVZ3RFVoaWdZbDg2WWRYaXVBVkZ3TFpTOER2?=
 =?utf-8?B?WWkyN1R5NTVxeVVuQVByajB6dzVJUUllWm9qdkFVNm1iNHEyU2daZlJPQmNP?=
 =?utf-8?B?TFl1K0ZlVXY3WjhldHBIY25wL1llaWxqY1dVSFIrQUVXQlBoYU5EYWFkK2Vt?=
 =?utf-8?Q?mObSAUg/FYRZIcIhnRY+vx1GD?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 25df4363-7637-4cda-b022-08dc63cc0d05
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3108.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Apr 2024 19:32:02.3498
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Zn29l8SxusTXxNGybwtm9A+7+VpjbeufqlL6r09neNha/7KejhGWArLG3fnYvhpazb3FNh7PAo5+otB7GAnmTw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR12MB7963

On 4/23/2024 3:06 PM, Borislav Petkov wrote:
> On Thu, Apr 04, 2024 at 10:13:48AM -0500, Yazen Ghannam wrote:
>> +	/*
>> +	 * OS is required to set the MCAX enable bit to acknowledge that it is
>> +	 * now using the new MSR ranges and new registers under each
>> +	 * bank. It also means that the OS will configure deferred
>> +	 * errors in the new MCA_CONFIG register. If the bit is not set,
>> +	 * uncorrectable errors will cause a system panic.
>> +	 */
>> +	mca_config |= FIELD_PREP(CFG_MCAX_EN, 0x1);
> 
> Can we please drop this cryptic crap?
> 
> 	mca_config |= SMCA_MCI_CONFIG_MCAX_ENABLE;
> 
> if I trust the PPR.
> 

Okay.

>> -		this_cpu_ptr(mce_banks_array)[bank].lsb_in_status = !!(low & BIT(8));
>> +	/*
>> +	 * SMCA sets the Deferred Error Interrupt type per bank.
>> +	 *
>> +	 * MCA_CONFIG[DeferredIntTypeSupported] is bit 5, and tells us
>> +	 * if the DeferredIntType bit field is available.
>> +	 *
>> +	 * MCA_CONFIG[DeferredIntType] is bits [38:37]. OS should set
>> +	 * this to 0x1 to enable APIC based interrupt. First, check that
>> +	 * no interrupt has been set.
>> +	 */
>> +	if (FIELD_GET(CFG_DFR_INT_SUPP, mca_config) && !FIELD_GET(CFG_DFR_INT_TYPE, mca_config))
>> +		mca_config |= FIELD_PREP(CFG_DFR_INT_TYPE, INTR_TYPE_APIC);
> 
> Ditto:
> 
> 	if (mca_config & CFG_DFR_INT_SUPP &&
> 	  !(mca_config & CFG_DFR_INT_TYPE))
> 		mca_config |= CFG_DFR_INT_TYPE | CFG_INTR_TYPE_APIC;
> 
> Plain and simple. Not this unreadable mess.
> 

This is not the same.

"CFG_DFR_INT_TYPE" is a register field.

"INTR_TYPE_APIC" is a value. And this same value can be used in other register
fields.

I think it's fair to just use logical AND for single bit checks instead of the
FIELD_GET() macro.

But the FIELD_PREP() macro does help for setting bitfields. I think it's
clearer than manually doing the proper shifts and masks.

> And use proper prefixes with the register name in them:
> 
> SMCA_MCI_CONFIG_
> 
> or so, for example.
> 

Okay. I was thinking to keep the names shorter since they are only used in
this file. But I'll change them.

>>  
>> -		wrmsr(smca_config, low, high);
>> -	}
>> +	if (FIELD_GET(CFG_LSB_IN_STATUS, mca_config))
>> +		this_cpu_ptr(mce_banks_array)[bank].lsb_in_status = true;
>> +
>> +	wrmsrl(MSR_AMD64_SMCA_MCx_CONFIG(bank), mca_config);
>> +}
>> +
>> +static void smca_configure_old(unsigned int bank, unsigned int cpu)
> 
> Yah, at the end of the patchset there should be patches which remove all
> the _old things. Not in a different patchset. You can split things
> accordingly.
> 

Okay. I'll include the follow up patches in the next revision of this set.

Thanks,
Yazen