Message-ID: <6ae4df67-ba0b-4b50-8c1d-a5d382105ad2@amd.com>
Date:   Tue, 19 Sep 2023 13:59:11 -0400
From:   Yazen Ghannam <yazen.ghannam@....com>
To:     Tony Luck <tony.luck@...el.com>, Borislav Petkov <bp@...en8.de>
Cc:     yazen.ghannam@....com, Smita.KoralahalliChannabasappa@....com,
        dave.hansen@...ux.intel.com, x86@...nel.org,
        linux-edac@...r.kernel.org, linux-kernel@...r.kernel.org,
        patches@...ts.linux.dev
Subject: Re: [PATCH v7 3/3] x86/mce: Handle Intel threshold interrupt storms

On 7/18/23 5:08 PM, Tony Luck wrote:
> Add an Intel specific hook into machine_check_poll() to keep track
> of per-CPU, per-bank corrected error logs (with a stub for the
> CONFIG_MCE_INTEL=n case).
> 
> When a storm is observed the Rate of interrupts is reduced by setting

Rate -> rate

> a large threshold value for this bank in IA32_MCi_CTL2. This bank is
> added to the bitmap of banks for this CPU to poll. The polling rate
> is increased to once per second.
> 
> When a storm ends reset the

Spurious newline?

> threshold in IA32_MCi_CTL2 back to 1, removes the bank from the bitmap

removes -> remove

> for polling, and changes the polling rate back to the default.

changes -> change

> 
> If a CPU with banks in storm mode is taken offline, the new CPU
> that inherits ownership of those banks takes over management of
> storm(s) in the inherited bank(s).
> 
> The cmci_discover() function was already very large. These changes
> pushed it well over the top. Refactor with three helper functions
> to braing it back under control.

braing -> bring

> 
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
>  arch/x86/kernel/cpu/mce/internal.h |   2 +
>  arch/x86/kernel/cpu/mce/core.c     |   3 +
>  arch/x86/kernel/cpu/mce/intel.c    | 202 +++++++++++++++++++++--------
>  3 files changed, 156 insertions(+), 51 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
> index da790d13d010..e641c991beb1 100644
> --- a/arch/x86/kernel/cpu/mce/internal.h
> +++ b/arch/x86/kernel/cpu/mce/internal.h
> @@ -41,12 +41,14 @@ struct dentry *mce_get_debugfs_dir(void);
>  extern mce_banks_t mce_banks_ce_disabled;
>  
>  #ifdef CONFIG_X86_MCE_INTEL
> +void mce_intel_handle_storm(int bank, bool on);
>  void cmci_disable_bank(int bank);
>  void intel_init_cmci(void);
>  void intel_init_lmce(void);
>  void intel_clear_lmce(void);
>  bool intel_filter_mce(struct mce *m);
>  #else
> +static inline void mce_intel_handle_storm(int bank, bool on) { }
>  static inline void cmci_disable_bank(int bank) { }
>  static inline void intel_init_cmci(void) { }
>  static inline void intel_init_lmce(void) { }
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 6a44e15d74fe..0a287998e62f 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -2054,6 +2054,9 @@ static void mce_zhaoxin_feature_clear(struct cpuinfo_x86 *c)
>  void mce_handle_storm(int bank, bool on)
>  {
>  	switch (boot_cpu_data.x86_vendor) {
> +	case X86_VENDOR_INTEL:
> +		mce_intel_handle_storm(bank, on);
> +		break;
>  	}
>  }
>  
> diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
> index 052bf2708391..55643c5944e1 100644
> --- a/arch/x86/kernel/cpu/mce/intel.c
> +++ b/arch/x86/kernel/cpu/mce/intel.c
> @@ -47,8 +47,27 @@ static DEFINE_PER_CPU(mce_banks_t, mce_banks_owned);
>   */
>  static DEFINE_RAW_SPINLOCK(cmci_discover_lock);
>  
> +/* Linux non-storm CMCI threshold (may be overridden by BIOS) */
>  #define CMCI_THRESHOLD		1

Just curious, but why use '1' for the default? We have a lot of code to
hide corrected errors. So why not just use the maximum limit? This would
effectively hide the corrected errors. And if not the maximum, maybe some
other intermediate value?

>  
> +/*
> + * MCi_CTL2 threshold for each bank when there is no storm.
> + * Default value for each bank may have been set by BIOS.
> + */
> +static int cmci_threshold[MAX_NR_BANKS];

Can this be a 'u16', since the max threshold for Intel is 0x7FFF?

> +
> +/*
> + * High threshold to limit CMCI rate during storms. Max supported is
> + * 0x7FFF. Use this slightly smaller value so it has a distinctive
> + * signature when some asks "Why am I not seeing all corrected errors?"

Maybe this answers my question above.

> + * A high threshold is used instead of just disabling CMCI for a
> + * bank because both corrected and uncorrected errors may be logged
> + * in the same bank and signalled with CMCI. The threshold only applies
> + * to corrected errors, so keeping CMCI enabled means that uncorrected
> + * errors will still be processed in a timely fashion.
> + */
> +#define CMCI_STORM_THRESHOLD	32749
> +
>  static int cmci_supported(int *banks)
>  {
>  	u64 cap;
> @@ -103,6 +122,31 @@ static bool lmce_supported(void)
>  	return tmp & FEAT_CTL_LMCE_ENABLED;
>  }
>  
> +/*
> + * Set a new CMCI threshold value. Preserve the state of the
> + * MCI_CTL2_CMCI_EN bit in case this happens during a
> + * cmci_rediscover() operation.
> + */
> +static void cmci_set_threshold(int bank, int thresh)
> +{
> +	unsigned long flags;
> +	u64 val;
> +
> +	raw_spin_lock_irqsave(&cmci_discover_lock, flags);
> +	rdmsrl(MSR_IA32_MCx_CTL2(bank), val);
> +	val &= ~MCI_CTL2_CMCI_THRESHOLD_MASK;
> +	wrmsrl(MSR_IA32_MCx_CTL2(bank), val | thresh);
> +	raw_spin_unlock_irqrestore(&cmci_discover_lock, flags);
> +}
> +
> +void mce_intel_handle_storm(int bank, bool on)
> +{
> +	if (on)
> +		cmci_set_threshold(bank, CMCI_STORM_THRESHOLD);
> +	else
> +		cmci_set_threshold(bank, cmci_threshold[bank]);
> +}
> +
>  /*
>   * The interrupt handler. This is called on every event.
>   * Just call the poller directly to log any events.
> @@ -114,72 +158,126 @@ static void intel_threshold_interrupt(void)
>  	machine_check_poll(MCP_TIMESTAMP, this_cpu_ptr(&mce_banks_owned));
>  }
>  
> +/*
> + * Check all the reasons why current CPU cannot claim
> + * ownership of a bank.
> + * 1: CPU already owns this bank
> + * 2: BIOS owns this bank
> + * 3: Some other CPU owns this bank
> + */
> +static bool cmci_skip_bank(int bank, u64 *val)
> +{
> +	unsigned long *owned = (void *)this_cpu_ptr(&mce_banks_owned);
> +
> +	if (test_bit(bank, owned))
> +		return true;
> +
> +	/* Skip banks in firmware first mode */
> +	if (test_bit(bank, mce_banks_ce_disabled))
> +		return true;
> +
> +	rdmsrl(MSR_IA32_MCx_CTL2(bank), *val);
> +
> +	/* Already owned by someone else? */
> +	if (*val & MCI_CTL2_CMCI_EN) {
> +		clear_bit(bank, owned);
> +		__clear_bit(bank, this_cpu_ptr(mce_poll_banks));
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +/*
> + * Decide which CMCI interrupt threshold to use:
> + * 1: If this bank is in storm mode from whichever CPU was
> + *    the previous owner, stay in storm mode.
> + * 2: If ignoring any threshold set by BIOS, set Linux default
> + * 3: Try to honor BIOS threshold (unless buggy BIOS set it at zero).
> + */
> +static u64 cmci_pick_threshold(u64 val, int *bios_zero_thresh)
> +{
> +	if ((val & MCI_CTL2_CMCI_THRESHOLD_MASK) == CMCI_STORM_THRESHOLD)
> +		return val;
> +
> +	if (!mca_cfg.bios_cmci_threshold) {

Are there many users of this option? Maybe this is something we should
also include in the AMD threshold code. But I don't think anyone has
asked me about it yet.

> +		val &= ~MCI_CTL2_CMCI_THRESHOLD_MASK;
> +		val |= CMCI_THRESHOLD;
> +	} else if (!(val & MCI_CTL2_CMCI_THRESHOLD_MASK)) {
> +		/*
> +		 * If bios_cmci_threshold boot option was specified
> +		 * but the threshold is zero, we'll try to initialize
> +		 * it to 1.
> +		 */
> +		*bios_zero_thresh = 1;
> +		val |= CMCI_THRESHOLD;
> +	}
> +
> +	return val;
> +}
> +
> +/*
> + * Try to claim ownership of a bank.
> + */
> +static void cmci_claim_bank(int bank, u64 val, int bios_zero_thresh, int *bios_wrong_thresh)
> +{
> +	struct mca_storm_desc *storm = this_cpu_ptr(&storm_desc);
> +
> +	val |= MCI_CTL2_CMCI_EN;
> +	wrmsrl(MSR_IA32_MCx_CTL2(bank), val);
> +	rdmsrl(MSR_IA32_MCx_CTL2(bank), val);
> +
> +	/* Did the enable bit stick? -- the bank supports CMCI */
> +	if (val & MCI_CTL2_CMCI_EN) {
> +		set_bit(bank, (void *)this_cpu_ptr(&mce_banks_owned));

Newline here, please.

> +		if ((val & MCI_CTL2_CMCI_THRESHOLD_MASK) == CMCI_STORM_THRESHOLD) {
> +			pr_notice("CPU%d BANK%d CMCI inherited storm\n", smp_processor_id(), bank);
> +			storm->banks[bank].history = ~0ull;
> +			storm->banks[bank].timestamp = jiffies;
> +			cmci_storm_begin(bank);
> +		} else {
> +			__clear_bit(bank, this_cpu_ptr(mce_poll_banks));
> +		}

Newline here, please.

> +		/*
> +		 * We are able to set thresholds for some banks that
> +		 * had a threshold of 0. This means the BIOS has not
> +		 * set the thresholds properly or does not work with
> +		 * this boot option. Note down now and report later.
> +		 */
> +		if (mca_cfg.bios_cmci_threshold && bios_zero_thresh &&
> +		    (val & MCI_CTL2_CMCI_THRESHOLD_MASK))
> +			*bios_wrong_thresh = 1;
> +
> +		/* Save default threshold for each bank */
> +		if (cmci_threshold[bank] == 0)
> +			cmci_threshold[bank] = val & MCI_CTL2_CMCI_THRESHOLD_MASK;
> +	} else {
> +		WARN_ON(!test_bit(bank, this_cpu_ptr(mce_poll_banks)));

Could you invert the "MCI_CTL2_CMCI_EN" check and WARN/return early?
This could save an indentation level.

> +	}
> +}
> +
>  /*
>   * Enable CMCI (Corrected Machine Check Interrupt) for available MCE banks
>   * on this CPU. Use the algorithm recommended in the SDM to discover shared
> - * banks.
> + * banks. Called during initial bootstrap, and also for hotplug CPU operations
> + * to rediscover/reassign machine check banks.
>   */
>  static void cmci_discover(int banks)
>  {
> -	unsigned long *owned = (void *)this_cpu_ptr(&mce_banks_owned);
> -	unsigned long flags;
> -	int i;
>  	int bios_wrong_thresh = 0;
> +	unsigned long flags;
> +	int i;
>  
>  	raw_spin_lock_irqsave(&cmci_discover_lock, flags);
>  	for (i = 0; i < banks; i++) {
>  		u64 val;
>  		int bios_zero_thresh = 0;
>  
> -		if (test_bit(i, owned))
> +		if (cmci_skip_bank(i, &val))
>  			continue;
>  
> -		/* Skip banks in firmware first mode */
> -		if (test_bit(i, mce_banks_ce_disabled))
> -			continue;
> -
> -		rdmsrl(MSR_IA32_MCx_CTL2(i), val);
> -
> -		/* Already owned by someone else? */
> -		if (val & MCI_CTL2_CMCI_EN) {
> -			clear_bit(i, owned);
> -			__clear_bit(i, this_cpu_ptr(mce_poll_banks));
> -			continue;
> -		}
> -
> -		if (!mca_cfg.bios_cmci_threshold) {
> -			val &= ~MCI_CTL2_CMCI_THRESHOLD_MASK;
> -			val |= CMCI_THRESHOLD;
> -		} else if (!(val & MCI_CTL2_CMCI_THRESHOLD_MASK)) {
> -			/*
> -			 * If bios_cmci_threshold boot option was specified
> -			 * but the threshold is zero, we'll try to initialize
> -			 * it to 1.
> -			 */
> -			bios_zero_thresh = 1;
> -			val |= CMCI_THRESHOLD;
> -		}
> -
> -		val |= MCI_CTL2_CMCI_EN;
> -		wrmsrl(MSR_IA32_MCx_CTL2(i), val);
> -		rdmsrl(MSR_IA32_MCx_CTL2(i), val);
> -
> -		/* Did the enable bit stick? -- the bank supports CMCI */
> -		if (val & MCI_CTL2_CMCI_EN) {
> -			set_bit(i, owned);
> -			__clear_bit(i, this_cpu_ptr(mce_poll_banks));
> -			/*
> -			 * We are able to set thresholds for some banks that
> -			 * had a threshold of 0. This means the BIOS has not
> -			 * set the thresholds properly or does not work with
> -			 * this boot option. Note down now and report later.
> -			 */
> -			if (mca_cfg.bios_cmci_threshold && bios_zero_thresh &&
> -					(val & MCI_CTL2_CMCI_THRESHOLD_MASK))
> -				bios_wrong_thresh = 1;
> -		} else {
> -			WARN_ON(!test_bit(i, this_cpu_ptr(mce_poll_banks)));
> -		}
> +		val = cmci_pick_threshold(val, &bios_zero_thresh);
> +		cmci_claim_bank(i, val, bios_zero_thresh, &bios_wrong_thresh);
>  	}
>  	raw_spin_unlock_irqrestore(&cmci_discover_lock, flags);
>  	if (mca_cfg.bios_cmci_threshold && bios_wrong_thresh) {
> @@ -218,6 +316,8 @@ static void __cmci_disable_bank(int bank)
>  	val &= ~MCI_CTL2_CMCI_EN;
>  	wrmsrl(MSR_IA32_MCx_CTL2(bank), val);
>  	__clear_bit(bank, this_cpu_ptr(mce_banks_owned));

Newline here, please.

> +	if ((val & MCI_CTL2_CMCI_THRESHOLD_MASK) == CMCI_STORM_THRESHOLD)
> +		cmci_storm_end(bank);
>  }
>  
>  /*

Thanks,
Yazen

Powered by blists - more mailing lists