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

Borislav Petkov
2024-04-23T19:07:03Z

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.