| Message ID | 20240404151359.47970-6-yazen.ghannam@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MCA Updates | expand |
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.
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
On Tue, Apr 23, 2024 at 03:32:00PM -0400, Yazen Ghannam wrote: > 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 don't care - this was just an example of how it should look like. Like the rest of the code around the kernel and not like an obfuscated C contest mess. > 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. To you maybe. Pls stick to how common code does masks generation and manipulation so that this remains readable. This FIELD* crap is not helping. > Okay. I was thinking to keep the names shorter since they are only used in > this file. But I'll change them. If you want to keep them shorter, then think of an overall shorter scheme of how the register *and* the fields which belong to it, should be named. But there's a point in having the same prefix for register and bits which belong to it. > Okay. I'll include the follow up patches in the next revision of this set. Pls do. Thanks.
On 4/23/2024 10:29 PM, Borislav Petkov wrote: > On Tue, Apr 23, 2024 at 03:32:00PM -0400, Yazen Ghannam wrote: >> 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 don't care - this was just an example of how it should look like. Like > the rest of the code around the kernel and not like an obfuscated > C contest mess. > >> 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. > > To you maybe. > > Pls stick to how common code does masks generation and manipulation so > that this remains readable. This FIELD* crap is not helping. > Okay, will do. I'll drop all the bitfield stuff from the entire set. >> Okay. I was thinking to keep the names shorter since they are only used in >> this file. But I'll change them. > > If you want to keep them shorter, then think of an overall shorter > scheme of how the register *and* the fields which belong to it, should > be named. But there's a point in having the same prefix for register and > bits which belong to it. > Okay, understood. Thanks, Yazen
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c index c76bc158b6b6..3093fed06194 100644 --- a/arch/x86/kernel/cpu/mce/amd.c +++ b/arch/x86/kernel/cpu/mce/amd.c @@ -50,6 +50,7 @@ #define MASK_DEF_INT_TYPE 0x00000006 #define DEF_LVT_OFF 0x2 #define DEF_INT_TYPE_APIC 0x2 +#define INTR_TYPE_APIC 0x1 /* Scalable MCA: */ #define MCI_IPID_MCATYPE GENMASK_ULL(47, 44) @@ -57,6 +58,12 @@ #define MCI_IPID_MCATYPE_OLD 0xFFFF0000 #define MCI_IPID_HWID_OLD 0xFFF +/* MCA_CONFIG register, one per MCA bank */ +#define CFG_DFR_INT_TYPE GENMASK_ULL(38, 37) +#define CFG_MCAX_EN BIT_ULL(32) +#define CFG_LSB_IN_STATUS BIT_ULL(8) +#define CFG_DFR_INT_SUPP BIT_ULL(5) + /* Threshold LVT offset is at MSR0xC0000410[15:12] */ #define SMCA_THR_LVT_OFF 0xF000 @@ -344,45 +351,51 @@ static void smca_set_misc_banks_map(unsigned int bank, unsigned int cpu) } -static void smca_configure(unsigned int bank, unsigned int cpu) +/* Set appropriate bits in MCA_CONFIG. */ +static void configure_smca(unsigned int bank) { - u8 *bank_counts = this_cpu_ptr(smca_bank_counts); - const struct smca_hwid *s_hwid; - unsigned int i, hwid_mcatype; - u32 high, low; - u32 smca_config = MSR_AMD64_SMCA_MCx_CONFIG(bank); + u64 mca_config; - /* Set appropriate bits in MCA_CONFIG */ - if (!rdmsr_safe(smca_config, &low, &high)) { - /* - * OS is required to set the MCAX 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 MCx_CONFIG register. If the bit is not set, - * uncorrectable errors will cause a system panic. - * - * MCA_CONFIG[MCAX] is bit 32 (0 in the high portion of the MSR.) - */ - high |= BIT(0); + if (!mce_flags.smca) + return; - /* - * 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] ([6:5] in the - * high portion of the MSR). OS should set this to 0x1 to enable - * APIC based interrupt. First, check that no interrupt has been - * set. - */ - if ((low & BIT(5)) && !((high >> 5) & 0x3)) - high |= BIT(5); + if (rdmsrl_safe(MSR_AMD64_SMCA_MCx_CONFIG(bank), &mca_config)) + return; + + /* + * 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); - 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); - 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) +{ + u8 *bank_counts = this_cpu_ptr(smca_bank_counts); + const struct smca_hwid *s_hwid; + unsigned int i, hwid_mcatype; + u32 high, low; smca_set_misc_banks_map(bank, cpu); @@ -758,8 +771,9 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c) for (bank = 0; bank < this_cpu_read(mce_num_banks); ++bank) { if (mce_flags.smca) - smca_configure(bank, cpu); + smca_configure_old(bank, cpu); + configure_smca(bank); disable_err_thresholding(c, bank); for (block = 0; block < NR_BLOCKS; ++block) {
The current SMCA configuration function does more than just configure SMCA features. It also detects and caches the SMCA bank types. However, the bank type caching flow will be removed during the init path clean up. Define a new function that only configures SMCA features. This will operate on the MCA_CONFIG MSR, so include updated register field definitions using bitops. Leave old code until init path is cleaned up. Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com> --- Notes: Link: https://lkml.kernel.org/r/20231118193248.1296798-10-yazen.ghannam@amd.com v1->v2: * No change. arch/x86/kernel/cpu/mce/amd.c | 84 ++++++++++++++++++++--------------- 1 file changed, 49 insertions(+), 35 deletions(-)