Received-SPF: pass (google.com: domain of linux-kernel+bounces-158669-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <e0d10606-4472-4cde-b55d-34180efad42b@amd.com>
Date: Thu, 25 Apr 2024 10:12:44 -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 07/16] x86/mce/amd: Simplify DFR handler setup
Content-Language: en-US
To: Borislav Petkov <bp@alien8.de>, robert.richter@amd.com
References: <20240404151359.47970-1-yazen.ghannam@amd.com>
 <20240404151359.47970-8-yazen.ghannam@amd.com>
 <20240424190658.GHZilYUvw1KfSfVd_e@fat_crate.local>
From: Yazen Ghannam <yazen.ghannam@amd.com>
In-Reply-To: <20240424190658.GHZilYUvw1KfSfVd_e@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?M3grVVdpeVFRWDk1T3kxVEQ4bGJlU2J5dXNzL3dXajliUE1ndjVzM3JaMm9T?=
 =?utf-8?B?VE90UG5ONWVYMDFLY2lsaGxUc1dOZzhCYW1TTDM2SFg5UktsZFluUy9odjZ5?=
 =?utf-8?B?MEJEVE9Fc0p6aHR0MEd2M1pvMkJJeVg1aGMzWGFzVlEzOFhUSmVFVUdQQnZG?=
 =?utf-8?B?S1FHRll6cnMrNmpDYXEwTEtwNmx3NHR4VEhpaDRJQWNnMlBrc3hIbk8ySm45?=
 =?utf-8?B?SktGZHZuTC9CTVNRMy8zVGgzdDFFZHUrZHZxQnZhblNRVE1Od2didzJubTZt?=
 =?utf-8?B?c2N1eUlJQW5xbEZ1OHZNckxVRzh4SXdnVXRLRFQxcEI5R3RqcVRYVlhnVldm?=
 =?utf-8?B?UFBVWE85MjFYbkVZVTZLakNEdFJRTndNeFdlZTVwSTRXUUpDY0ZHQnBvQURv?=
 =?utf-8?B?bWxyUUpVUC90NHlFVUpiUnA3MXBzNDUwdkNvZzRKV3FUUitkcEcvQjZVazN2?=
 =?utf-8?B?M3h1bUtQT1VCaXdkdG1nU3o3SFNKS0Q2bExOdmJLdXoybG5uM3drR2tDd0xq?=
 =?utf-8?B?SUFMTTlQaHZhczJlMmV4NVVscWlqUFNvemZwNS9LMUJBTXEwNk02REExTEM2?=
 =?utf-8?B?Z0gvbk4zMEtqby9idXJMOHlPZnZieWJwMWJWeHJsUC8zQWNWd0M4Z0lETWZ4?=
 =?utf-8?B?T215ZnBOUFR4VHFUZlBkS0cyTXFudWx5T2JWU25iRVJNZTZwS3hOMUVoV0Nh?=
 =?utf-8?B?emdYRFdxemhMSGVMNDJteGtzVGxubkNMa2Zlb0xpZW9TWmpRcnNPeWFsT2hi?=
 =?utf-8?B?dlViR2ttUGNVb0FwZHpVY3JUK0FvYXQ0YzdmcUdRd0thRlFhaEM1dHliRTBM?=
 =?utf-8?B?eEFZU2p5VTNaLytDM003WVFOUXV0SGozZ3NoZnNzSGIwSWJCTWhNT0xCSzU1?=
 =?utf-8?B?WjFpS1dsMithWitPcXdLK29tamlRTVB3QSs1a2NYVGp6R0tSSnlrdkFkSGRS?=
 =?utf-8?B?aU1JNmxiUTN2Yk45Z0EyYjlhUDR1T1RqK1Q3c0dzaTIzRHR3bkRlL1lLcWlN?=
 =?utf-8?B?blFDZ0VrTmJWcGxoeDYzd3RWUTRMUVJqRitZazhRcWg0RmViMGxvZjZPbGNK?=
 =?utf-8?B?elF3dkFtaG5kY1dDM2J2Rm1YT1NDMkV1eU5TelRMUlFycTJDdU1lblI4OWFl?=
 =?utf-8?B?a3RzcitiVEVHcHRHODdUdDJPdnFLdUtGU3RwV2VXZU5OK2IxbDdtK1NMaS9P?=
 =?utf-8?B?dDcyV09RQlJYOXF4NG13UGc4SjNCOEVRWW9qdFNaUWVDNk5aY3htREZFdk5T?=
 =?utf-8?B?ODVwZkttR0RvUVd2clR2Y1YzSWhhMVAyOUtqcWlPNkRlS2E2VUpjK3ZyZmxP?=
 =?utf-8?B?OFdKN1pNazZET201dkFhTGtlMXJsVDZnSkJVL2FwK0NUQU5xN29qT3JuQ01N?=
 =?utf-8?B?K0k4MS9IZ2orcENUZnA5UlY0eXBBMVlJcVdqZWN0SHhUNnNTQUxMVXFyczF4?=
 =?utf-8?B?WC84MWZhNlRMd1NIZUhUR3MvNkJsZVVWVk1BdlpUTlN5NVJ4ZHdQOWxNYVFG?=
 =?utf-8?B?b0dtSG16TUxmOTFDQmh3M0NQTEs1RDh6OTNSOU5kRFNOUkQ0c2hTZFVJNWln?=
 =?utf-8?B?dUtnYWJVekR0SXp2QzMwOGRodlFGbVZSTGdHdk5BUFhtTWgvQVJKVE11Zk5P?=
 =?utf-8?B?dVc0OHlLM1BXMWhmZEJIUzk5WmZCcXIzNEhVVG5ESlFsNkRwZm5oVVlISzVB?=
 =?utf-8?B?SmJPUXhBUkprUmZWa2lkWXVHZ1BYUWxqTmhYc3BYamJNTE5kNXc2SW12MGU2?=
 =?utf-8?B?ejRpMUJkWFltS3Q1QkpQeXhpczZGZFRMbW9nNEpOUjJkSUFUL3IrNmhNZmNY?=
 =?utf-8?B?RWhHN0RQT0NnaHIxT1pKT3dSWTJCazh3UE1xOGlVbWZIK3o5RXgyZnZ5M0FV?=
 =?utf-8?B?NXFQbUc4RC9xOTNEdWxXZjM4bkJSMVI5VEFabE5CV0dvam9RZThVbWtkajZl?=
 =?utf-8?B?NHRGWlBSMVZUWlRlVTRabng2b09YekpzSkQ1cWlleHAxREcrZ0RMa2s1RU1W?=
 =?utf-8?B?ZGcyTkt2R01lb1c2NjlSMVJ0OWtqV2FjVUsrcnFzcmVFVUVadGFMOURSTUpz?=
 =?utf-8?B?MlhSY3lJRVpjTTMzSmhTUGp6dkRlc1JiMFR3U3pKRStiZ01TdkYrMFUyR1JY?=
 =?utf-8?Q?b1NMAmJpjD3d6FfZWT8p2O6on?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c4adc1f8-3aa9-4383-abd2-08dc6531c8fa
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3108.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Apr 2024 14:12:48.0367
 (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: Lr5/UofYqw160LAHKVPfQmL03MYc4klaNxqGxmlCINlxgfVNp5xSqQJ34ArZ7HuXdG3XAnWTZoQRtop9lGKd3w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB8612

On 4/24/2024 3:06 PM, Borislav Petkov wrote:
> On Thu, Apr 04, 2024 at 10:13:50AM -0500, Yazen Ghannam wrote:
>> AMD systems with the SUCCOR feature can send an APIC LVT interrupt for
>> deferred errors. The LVT offset is 0x2 by convention, i.e. this is the
>> default as listed in hardware documentation.
>>
>> However, the MCA registers may list a different LVT offset for this
>> interrupt. The kernel should honor the value from the hardware.
> 
> There's this "may" thing again.
> 

Right, I should say "the microarchitecture allows it". :)

> Is this enablement for some future hw too or do you really trust the
> value in MSR_CU_DEF_ERR is programmed correctly in all cases?
> 

I trust the value from hardware.

The intention here is to simplify the code for general maintenance and to make
later patches easier.

>> Simplify the enable flow by using the hardware-provided value. Any
>> conflicts will be caught by setup_APIC_eilvt(). Conflicts on production
>> systems can be handled as quirks, if needed.
> 
> Well, which systems support succor?
> 
> I'd like to test this on them before we face all the quirkery. :)
> 

All Zen/SMCA systems. I don't recall any issues in this area.

Some later Family 15h systems (Carrizo?) had it. But I don't know if it was
used in production. It was slightly before my time.

> That area has been plagued by hw snafus if you look at
> setup_APIC_eilvt() and talk to uncle Robert. :-P
>

Right, I found this:
27afdf2008da ("apic, x86: Use BIOS settings for IBS and MCE threshold
interrupt LVT offsets")

Which is basically the same idea: use what is in the register.

But it looks there was an issue with IBS on Family 10h.

Is this the only case of a real issue? If so, then why apply this method to
the THR and DFR interrupts?

Robert, were there any other issues?

>> @@ -595,17 +584,15 @@ static void deferred_error_interrupt_enable(struct cpuinfo_x86 *c)
>>  	if (rdmsr_safe(MSR_CU_DEF_ERR, &low, &high))
>>  		return;
>>  
>> +	/*
>> +	 * Trust the value from hardware.
>> +	 * If there's a conflict, then setup_APIC_eilvt() will throw an error.
>> +	 */
>>  	def_new = (low & MASK_DEF_LVTOFF) >> 4;
>> -	if (!(low & MASK_DEF_LVTOFF)) {
>> -		pr_err(FW_BUG "Your BIOS is not setting up LVT offset 0x2 for deferred error IRQs correctly.\n");
>> -		def_new = DEF_LVT_OFF;
>> -		low = (low & ~MASK_DEF_LVTOFF) | (DEF_LVT_OFF << 4);
>> -	}
>> +	if (setup_APIC_eilvt(def_new, DEFERRED_ERROR_VECTOR, APIC_EILVT_MSG_FIX, 0))
>> +		return;
>>  
>> -	def_offset = setup_APIC_deferred_error(def_offset, def_new);
>> -	if ((def_offset == def_new) &&
>> -	    (deferred_error_int_vector != amd_deferred_error_interrupt))
>> -		deferred_error_int_vector = amd_deferred_error_interrupt;
> 
> There was a reason for that - deferred_error_int_vector is a global var
> and you're calling enable_deferred_error_interrupt() on each CPU.
>

Right, and all CPUs should use the same APIC LVT offset. If they differ, then
setup_APIC_eilvt() will fail above and return.

Why check "if X != Y, then X = Y"? Why not just unconditionally do "X = Y"?

>> +	deferred_error_int_vector = amd_deferred_error_interrupt;
>>  
>>  	if (!mce_flags.smca)
>>  		low = (low & ~MASK_DEF_INT_TYPE) | DEF_INT_TYPE_APIC;
> 
> Thx.
> 

Thanks,
Yazen