Messages in this thread | | | Date | Tue, 23 Apr 2024 15:32:00 -0400 | Subject | Re: [PATCH v2 05/16] x86/mce/amd: Clean up SMCA configuration | From | Yazen Ghannam <> |
| |
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
| |