Received-SPF: pass (google.com: domain of linux-kernel+bounces-162405-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <d1e329da-6a04-47f7-bdab-ea6c4f584802@amd.com>
Date: Mon, 29 Apr 2024 09:56:56 -0400
User-Agent: Mozilla Thunderbird
Cc: yazen.ghannam@amd.com, robert.richter@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>
References: <20240404151359.47970-1-yazen.ghannam@amd.com>
 <20240404151359.47970-8-yazen.ghannam@amd.com>
 <20240424190658.GHZilYUvw1KfSfVd_e@fat_crate.local>
 <e0d10606-4472-4cde-b55d-34180efad42b@amd.com>
 <20240429125956.GNZi-ZzN1Izxps8ztT@fat_crate.local>
From: Yazen Ghannam <yazen.ghannam@amd.com>
In-Reply-To: <20240429125956.GNZi-ZzN1Izxps8ztT@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?eGNhVUlUVUw5Z0JmSDhRNU5RdlB0Uzhac1dndWVxS2tyR0J4b2N1dCs4SUZk?=
 =?utf-8?B?RnNSWmZMcys1MkFVRFpuYmJ4cjM0UUZVMWVINmQxOERTYk5peFVaRlgwWVZr?=
 =?utf-8?B?NExFc3JGdHgvWUFnRExsQ2M1cmM5aFdRa3JXYkVXY2xIMVRYenYzbGRQakU3?=
 =?utf-8?B?SnZSU2xpa01SamZPeGlVMlgrZGdtMTNnRFJuSXcwZTByeUtBdUZpak9waW1U?=
 =?utf-8?B?NlhNRFR1bHl0RkhFNG50U2NINko4Z21CbkhvcldWOFl1YmUraEhKSmkyUFNy?=
 =?utf-8?B?NklFa2JpQ2QxTE15Y2pja04xb3VZNG5BN3dYZ1hRYUdsazFXWjlMV3pUcG9Y?=
 =?utf-8?B?UkxGcEFkSmM0dEZsUUp2U0cyOGc4Rk9nTHFUWDhXTklXK3BiMU1nNDNvOVIr?=
 =?utf-8?B?UTNValBjOU9yMk0xUmtlVlBvVnQreWU5WEc1R1VkMlZaeDhOaXlnbnNIVHF1?=
 =?utf-8?B?QVV2ODdiWFIwTjdKdDh5YXF5ZEQzOTU3S2Z0V1ozQVUzY2czR0ZqVlQ2WSsz?=
 =?utf-8?B?ZysreUNxM05hWHRhSWVFSzNlalo4VU1NNldydWhtMk5KdVBDV29vTE5YOXRk?=
 =?utf-8?B?UmxSTS9XbllYRkUxbFZIRnk0SURyZ1lWa2c4U1ROQld1VHRLQnoyUkUwVVdU?=
 =?utf-8?B?YXdxbXlZWnhRSWVmWnFMbXdzRHZyOHE1SjJoaHRPaDA3V2V6UmpYelhUY0pO?=
 =?utf-8?B?NEFoTGU1SnJwbmt4SVBzV0FxaHdhYUFHcm1Db3VnTkpwbk5VRTdEZkFLdGZQ?=
 =?utf-8?B?ZDNrbUl5QzMwR1psTCs4R0lSdm5xVjhDUUhFSTBWV0hMK0hCOFVBZWsrKzQ5?=
 =?utf-8?B?OGxtaVBIbmlDWGplcDgyUVVkNjM2VjNMazVGczZ0c294SDJlRld0SkdZUDF4?=
 =?utf-8?B?Q1Z1S2R6OEQ4TW9LYXJ1UGpPdTJlVkI4TGdaK200Z2k0bWYxUjZ3TGpBNk90?=
 =?utf-8?B?WnJnUEtOUmxVUG03S0RGNWFLWVVJY3FEUGdsTmRZRmRlbW4zUCtRNll3SXZk?=
 =?utf-8?B?ellML00wSjM4Y0VKelYxcU56SkNCUFhESTByd0ZEYjRiMkYyKzdDV2pkMWJ3?=
 =?utf-8?B?M0VzRkZJdG9FaEFJbkxyNDNTVDV0U3BxZ3FDeG9SVzRvcTlqQ1h2emQrdXN1?=
 =?utf-8?B?d2tFRVhLL1J0RDZYb3VrMmJqVmxtYnNabTdiZFVBeEswemFHSno4czZ5ZjhR?=
 =?utf-8?B?V2U0Nk5yMXRFUE5WOVZST3AvNXNwcC8yZ3UwZUZaVGVIUWNRWVRqKzl0clpa?=
 =?utf-8?B?U2Q1RnRac1piSjA4U3hEUFlQVy84ZS91U040U29jRVpIVmpOaEFwa3V5MHBw?=
 =?utf-8?B?L2Y3YllIcFRVOUV6bUVyMVlpQlVsTHMvYk9qNVA4dmFrZkJWVm1EMmhpSjMy?=
 =?utf-8?B?NkJHNDNLanF3TUVpbWQ2VXRsT3ZBYk5TWlozTCsvKzE5QU1QcXN4YmNmK1RX?=
 =?utf-8?B?WFk5dDcwWmQzNTQzazJndSttOHBzb0o3alZoSndwYTA1aVlNaXdmSzcvdWl0?=
 =?utf-8?B?b2p3SkdkaGhMM0phN05qNVpMeEFuMkpYU1Z3MlUyeU1USnhPQUJxdkVJVGVn?=
 =?utf-8?B?OGdmOGhxQSs4RWhoQVJxZDg4Ulg5TEpwbGNXMkRsSmxJaXVqME5VUzVxWkg1?=
 =?utf-8?B?R1pOeWpkU2VnTXR0Q083allKN3o3WjYvUUgybVBhOTZkdjY4azhUNzcxcitv?=
 =?utf-8?B?K3I0Vk5wdlVlbmtRS3RrQjNRT1BSaUdLMzB0a01rcEtPTzJtb3hRVTVoRFlu?=
 =?utf-8?B?TGxtRDl3WmZpU1V1ZFFzU3RJVU9qZ2FrVFk2TG5iWm41Q1R3MTMrUkI4bmxM?=
 =?utf-8?B?c2U1NElPeDJSa0RKRUI3RXBybTJGeFN3dHBHQ0lEb01YLzZGeWtIRmhtZDND?=
 =?utf-8?B?c2liSU1lektzZUliN0tZRjlWdlRZVlN0SlYwK0ZhSE1mMHZKSy9mVnJORTFB?=
 =?utf-8?B?WmVYamFsbDkyTGdKdXhIYWxPYkRVbGtncmZkZDNjbzdvQUI4WGhrNzZTSzdn?=
 =?utf-8?B?MWo1YkxtQWtFWUJqWWs3bFNnSHFkOFY0VjJkNDdjUExjR1hKR1h5QWpTUDU5?=
 =?utf-8?B?MFFheVR3TmVEcUFYMEplTko0R0RQcHl0SkVsYkN2UW90NEptNG5lK2R1dXR1?=
 =?utf-8?Q?CT0pnJ3XqBU3prU5eUVV1TAlA?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4186c59f-78df-48ba-8c65-08dc68543cc9
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3108.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Apr 2024 13:56:58.6988
 (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: wGLCUuCFlnkfs78wguJk8G5RWK5Hrup5XQlSfjJSLftu7QCDIlHcRyNJRAJnk1qiPZEjRfwVx3O0xFogqzEMCA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW6PR12MB8960

On 4/29/2024 8:59 AM, Borislav Petkov wrote:
> On Thu, Apr 25, 2024 at 10:12:44AM -0400, Yazen Ghannam wrote:
>> I trust the value from hardware.
>>
>> The intention here is to simplify the code for general maintenance and to make
>> later patches easier.
> 
> There's this BIOS thing which programs those and throws a wrench in all
> our trusting in the hw.
> 
>> 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.
> 
> Yeah, already solved in the previous mail.
> 
>> 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.
> 
> Yap, and it was pretty blatant AFAIR.
> 
>> Is this the only case of a real issue?
> 
> I don't remember anything else but I'm not excluding there not being
> others.
> 
>> If so, then why apply this method to the THR and DFR interrupts?
> 
> Meaning what exactly? You want to trust the hw for THR and DFR and let
> the others use this offset reservation we're doing?
> 

Right, I mean we should do things the simpler way unless there's a real issue
to address.

>> 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"?
> 
> Why unconditionally do the assignment if it is already assigned?
> 
> I don't think x86 does store tearing so that we get deferred interrupt
> on some core while some other core writes the same function pointer in
> there but why even risk it if it can be avoided with a simple test?
> 

I'm not opposed to this, but I don't understand what is at risk.

Is it that the function pointer may not be written atomically? So even if we
write it again with the same value, a concurrent interrupt on another core may
see a partially updated (corrupt) pointer?

intel_init_cmci() does not do this check. So is it more at risk, or is the AMD
code just more cautious?

Again I'm not against the current code. I just think we should simplify it, if
possible.

Thanks,
Yazen