RE: [PATCH v2 14/16] x86/mce/amd: Enable interrupt vectors once per-CPU on SMCA systems

[Zhuo, Qiuxu]

> From: Yazen Ghannam <yazen.ghannam@xxxxxxx>
> [...]
> behavior. The deferred error interrupt is technically advertised by the SUCCOR
> feature. However, this was first made available on SMCA systems.
> Therefore, only set up the deferred error interrupt on SMCA systems and
> simplify the code.

Does this description imply that:

    if mce_flags.succor = true, then mce_flags.smca must also be true.

> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@xxxxxxx>
> [...]
> -static void deferred_error_interrupt_enable(struct cpuinfo_x86 *c) -{
> -	u32 low = 0, high = 0;
> -	int def_offset = -1, def_new;
> -
> -	if (rdmsr_safe(MSR_CU_DEF_ERR, &low, &high))
> -		return;
> -
> -	def_new = (low & MASK_DEF_LVTOFF) >> 4;
> -	if (!(low & MASK_DEF_LVTOFF)) {
> -		pr_err(FW_BUG "Your BIOS is not setting up LVT offset 0x2 for
> deferred error IRQs correctly.\n");
> -		def_new = DEF_LVT_OFF;
> -		low = (low & ~MASK_DEF_LVTOFF) | (DEF_LVT_OFF << 4);
> -	}

This code is missing from this patch.
Expected?

> -
> -	def_offset = setup_APIC_deferred_error(def_offset, def_new);
> -	if ((def_offset == def_new) &&
> -	    (deferred_error_int_vector != amd_deferred_error_interrupt))
> -		deferred_error_int_vector = amd_deferred_error_interrupt;
> -
> -	if (!mce_flags.smca)
> -		low = (low & ~MASK_DEF_INT_TYPE) | DEF_INT_TYPE_APIC;
> -
> -	wrmsr(MSR_CU_DEF_ERR, low, high);
> -}
> -

This code is missing from this patch.
Expected?

>  static u32 smca_get_block_address(unsigned int bank, unsigned int block,
>  				  unsigned int cpu)
>  {
> @@ -551,7 +516,6 @@ prepare_threshold_block(unsigned int bank, unsigned
> int block, u32 addr,
>  			int offset, u32 misc_high)
>  {
>  	unsigned int cpu = smp_processor_id();
> -	u32 smca_low, smca_high;
>  	struct threshold_block b;
>  	int new;
> 
> @@ -571,18 +535,10 @@ prepare_threshold_block(unsigned int bank,
> unsigned int block, u32 addr,
>  	__set_bit(bank, this_cpu_ptr(&mce_amd_data)->thr_intr_banks);
>  	b.interrupt_enable = 1;
> 
> -	if (!mce_flags.smca) {
> -		new = (misc_high & MASK_LVTOFF_HI) >> 20;
> -		goto set_offset;
> -	}
> -
> -	/* Gather LVT offset for thresholding: */
> -	if (rdmsr_safe(MSR_CU_DEF_ERR, &smca_low, &smca_high))
> -		goto out;
> -
> -	new = (smca_low & SMCA_THR_LVT_OFF) >> 12;
> +	if (mce_flags.smca)
> +		goto done;
> 
> -set_offset:
> +	new = (misc_high & MASK_LVTOFF_HI) >> 20;
>  	offset = setup_APIC_mce_threshold(offset, new);
>  	if (offset == new)
>  		thresholding_irq_en = true;
> @@ -590,7 +546,6 @@ prepare_threshold_block(unsigned int bank, unsigned
> int block, u32 addr,
>  done:
>  	mce_threshold_block_init(&b, offset);
> 
> -out:
>  	return offset;
>  }
> 
> @@ -659,6 +614,32 @@ static void disable_err_thresholding(struct
> cpuinfo_x86 *c, unsigned int bank)
>  		wrmsrl(MSR_K7_HWCR, hwcr);
>  }
> 
> +/*
> + * Enable the APIC LVT interrupt vectors once per-CPU. This should be
> +done before hardware is
> + * ready to send interrupts.
> + *
> + * Individual error sources are enabled later during per-bank init.
> + */
> +static void smca_enable_interrupt_vectors(void)
> +{
> +	struct mce_amd_cpu_data *data = this_cpu_ptr(&mce_amd_data);
> +	u64 mca_intr_cfg, offset;
> +
> +	if (!mce_flags.smca || !mce_flags.succor)
> +		return;
> +

In the old code, the deferred IRQ setup just depends on mce_flags.succor,
But now it depends on: mce_flags.smca && mce_flags.succor.
Is this expected?

> +	if (rdmsrl_safe(MSR_CU_DEF_ERR, &mca_intr_cfg))
> +		return;
> +
> +	offset = (mca_intr_cfg & SMCA_THR_LVT_OFF) >> 12;
> +	if (!setup_APIC_eilvt(offset, THRESHOLD_APIC_VECTOR,
> APIC_EILVT_MSG_FIX, 0))
> +		data->thr_intr_en = true;
> +
> +	offset = (mca_intr_cfg & MASK_DEF_LVTOFF) >> 4;
> +	if (!setup_APIC_eilvt(offset, DEFERRED_ERROR_VECTOR,
> APIC_EILVT_MSG_FIX, 0))
> +		data->dfr_intr_en = true;
> +}
> +
>  static void amd_apply_quirks(struct cpuinfo_x86 *c)  {
>  	struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array); @@
> -690,11 +671,16 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
> 
>  	amd_apply_quirks(c);
>  	mce_flags.amd_threshold	 = 1;
> +	smca_enable_interrupt_vectors();
> 
>  	for (bank = 0; bank < this_cpu_read(mce_num_banks); ++bank) {
> -		if (mce_flags.smca)
> +		if (mce_flags.smca) {
>  			smca_configure(bank, cpu);
> 
> +			if (!this_cpu_ptr(&mce_amd_data)->thr_intr_en)
> +				continue;
> +		}
> +
>  		disable_err_thresholding(c, bank);
> 
>  		for (block = 0; block < NR_BLOCKS; ++block) { @@ -715,9
> +701,6 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
>  			offset = prepare_threshold_block(bank, block,
> address, offset, high);
>  		}
>  	}
> -
> -	if (mce_flags.succor)
> -		deferred_error_interrupt_enable(c);

The old code to set up deferred error IRQ just depends on mce_flags.succor,
> [...]