Re: [PATCH v5 2/2] x86/mce: Add support for Extended Physical Address MCA changes

[Yazen Ghannam]

On Wed, Apr 13, 2022 at 06:19:11PM +0200, Borislav Petkov wrote:

...

>  static void __mcheck_cpu_init_generic(void)
>  {
> -	enum mcp_flags m_fl = 0;
> -	mce_banks_t all_banks;
>  	u64 cap;
>  
> -	if (!mca_cfg.bootlog)
> -		m_fl = MCP_DONTLOG;
> -
> -	/*
> -	 * Log the machine checks left over from the previous reset. Log them
> -	 * only, do not start processing them. That will happen in mcheck_late_init()
> -	 * when all consumers have been registered on the notifier chain.
> -	 */
> -	bitmap_fill(all_banks, MAX_NR_BANKS);
> -	machine_check_poll(MCP_UC | MCP_QUEUE_LOG | m_fl, &all_banks);
> -
>  	cr4_set_bits(X86_CR4_MCE);

I think the init logic breaks here. MCE now gets enabled before clearing old
errors. So it's possible that the old errors get overwritten by new ones.

>  
>  	rdmsrl(MSR_IA32_MCG_CAP, cap);
> @@ -1754,36 +1738,22 @@ static void __mcheck_cpu_init_generic(void)
>  		wrmsr(MSR_IA32_MCG_CTL, 0xffffffff, 0xffffffff);
>  }
>  
> -static void __mcheck_cpu_init_clear_banks(void)
> +static void __mcheck_cpu_init_prepare_banks(void)
>  {
>  	struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
> +	mce_banks_t all_banks;
> +	u64 msrval;
>  	int i;
>  
> -	for (i = 0; i < this_cpu_read(mce_num_banks); i++) {
> -		struct mce_bank *b = &mce_banks[i];
> -
> -		if (!b->init)
> -			continue;
> -		wrmsrl(mca_msr_reg(i, MCA_CTL), b->ctl);
> -		wrmsrl(mca_msr_reg(i, MCA_STATUS), 0);
> +	/*
> +	 * Log the machine checks left over from the previous reset. Log them
> +	 * only, do not start processing them. That will happen in mcheck_late_init()
> +	 * when all consumers have been registered on the notifier chain.
> +	 */
> +	if (mca_cfg.bootlog) {
> +		bitmap_fill(all_banks, MAX_NR_BANKS);
> +		machine_check_poll(MCP_UC | MCP_QUEUE_LOG, &all_banks);
>  	}
> -}
> -
> -/*
> - * Do a final check to see if there are any unused/RAZ banks.
> - *
> - * This must be done after the banks have been initialized and any quirks have
> - * been applied.
> - *
> - * Do not call this from any user-initiated flows, e.g. CPU hotplug or sysfs.
> - * Otherwise, a user who disables a bank will not be able to re-enable it
> - * without a system reboot.
> - */
> -static void __mcheck_cpu_check_banks(void)
> -{
> -	struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
> -	u64 msrval;
> -	int i;
>  
>  	for (i = 0; i < this_cpu_read(mce_num_banks); i++) {
>  		struct mce_bank *b = &mce_banks[i];
> @@ -1791,6 +1761,9 @@ static void __mcheck_cpu_check_banks(void)
>  		if (!b->init)
>  			continue;
>  
> +		wrmsrl(mca_msr_reg(i, MCA_CTL), b->ctl);
> +		wrmsrl(mca_msr_reg(i, MCA_STATUS), 0);

Same idea here. STATUS should be cleared before turning on reporting in a bank
using MCA_CTL.

> +
>  		rdmsrl(mca_msr_reg(i, MCA_CTL), msrval);
>  		b->init = !!msrval;
>  	}
> @@ -2158,8 +2131,7 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c)
>  	__mcheck_cpu_init_early(c);
>  	__mcheck_cpu_init_generic();
>  	__mcheck_cpu_init_vendor(c);
> -	__mcheck_cpu_init_clear_banks();
> -	__mcheck_cpu_check_banks();
> +	__mcheck_cpu_init_prepare_banks();

I think moving __mcheck_cpu_init_generic() after
__mcheck_cpu_init_prepare_banks() and swapping the MCA_STATUS and MCA_CTL
writes above would be correct.

In summary:
1) __mcheck_cpu_init_vendor()
2) __mcheck_cpu_init_prepare_banks()
  a) Read and clear MCA_STATUS.
  b) Initialize MCA_CTL.
3) __mcheck_cpu_init_generic()

I realize this is still different than the original flow. But it seems to
maintain the goal of this patch. And it actually matches the AMD documentation
more closely than the original flow.

One downside though is that the system goes longer with CR4.MCE cleared. So
there's greater risk of encountering a shutdown due to a machine check error.

Thanks,
Yazen