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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 23 Apr 2024 21:06:41 +0200
From: Borislav Petkov <bp@...en8.de>
To: Yazen Ghannam <yazen.ghannam@....com>
Cc: linux-edac@...r.kernel.org, linux-kernel@...r.kernel.org,
	tony.luck@...el.com, x86@...nel.org, Avadhut.Naik@....com,
	John.Allen@....com
Subject: Re: [PATCH v2 05/16] x86/mce/amd: Clean up SMCA configuration

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.

> -		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.

And use proper prefixes with the register name in them:

SMCA_MCI_CONFIG_

or so, for example.

>  
> -		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.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette