Re: [PATCH v2 07/16] x86/mce/amd: Simplify DFR handler setup

From: Yazen Ghannam <yazen.ghannam@amd.com>
To: Borislav Petkov <bp@alien8.de>
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
Date: Mon, 29 Apr 2024 09:56:56 -0400	[thread overview]
Message-ID: <d1e329da-6a04-47f7-bdab-ea6c4f584802@amd.com> (raw)
In-Reply-To: <20240429125956.GNZi-ZzN1Izxps8ztT@fat_crate.local>

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