Re: [PATCH v2 05/16] x86/mce/amd: Clean up SMCA configuration

From: Yazen Ghannam <yazen.ghannam@amd.com>
To: Borislav Petkov <bp@alien8.de>
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
Date: Tue, 23 Apr 2024 15:32:00 -0400	[thread overview]
Message-ID: <eb9c5d9b-07d2-4b56-98dd-c2616ef73a0a@amd.com> (raw)
In-Reply-To: <20240423190641.GDZigGwXXEPeDnfOsr@fat_crate.local>

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