diff mbox series

[8/9] x86/mce/amd: Enable interrupt vectors once per-CPU on SMCA systems

Message ID 20240523155641.2805411-9-yazen.ghannam@amd.com (mailing list archive)
State New, archived
Headers show
Series AMD MCA interrupts rework | expand

Commit Message

Yazen Ghannam May 23, 2024, 3:56 p.m. UTC
Scalable MCA systems have a per-CPU register that gives the APIC LVT
offset for the thresholding and deferred error interrupts.

Currently, this register is read once to set up the deferred error
interrupt and then read again for each thresholding block. Furthermore,
the APIC LVT registers are configured each time, but they only need to
be configured once per-CPU.

Move the APIC LVT setup to the early part of CPU init, so that the
registers are set up once. Also, this ensures that the kernel is ready
to service the interrupts before the individual error sources (each MCA
bank) are enabled.

Apply this change only to SMCA systems to avoid breaking any legacy
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.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/cpu/mce/amd.c | 116 +++++++++++++++-------------------
 1 file changed, 52 insertions(+), 64 deletions(-)

Comments

Borislav Petkov June 4, 2024, 3:46 p.m. UTC | #1
On Thu, May 23, 2024 at 10:56:40AM -0500, Yazen Ghannam wrote:
>  static bool thresholding_irq_en;
>  static DEFINE_PER_CPU_READ_MOSTLY(mce_banks_t, mce_thr_intr_banks);
>  static DEFINE_PER_CPU_READ_MOSTLY(mce_banks_t, mce_dfr_intr_banks);
> +static DEFINE_PER_CPU_READ_MOSTLY(bool, smca_thr_intr_enabled);
> +static DEFINE_PER_CPU_READ_MOSTLY(bool, smca_dfr_intr_enabled);

So before you add those, we already have:

static DEFINE_PER_CPU_READ_MOSTLY(struct smca_bank[MAX_NR_BANKS], smca_banks);
static DEFINE_PER_CPU_READ_MOSTLY(u8[N_SMCA_BANK_TYPES], smca_bank_counts);
static DEFINE_PER_CPU(struct threshold_bank **, threshold_banks);
static DEFINE_PER_CPU(u64, bank_map);
static DEFINE_PER_CPU(u64, smca_misc_banks_map);

Please think of a proper struct which collects all that info in the
smallest possible format and unify everything.

It is a mess currently.

> +/*
> + * 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(struct cpuinfo_x86 *c)
> +{
> +	u8 thr_offset, dfr_offset;
> +	u64 mca_intr_cfg;
> +
> +	if (!mce_flags.smca || !mce_flags.succor)
> +		return;
> +
> +	if (c == &boot_cpu_data) {
> +		mce_threshold_vector		= amd_threshold_interrupt;
> +		deferred_error_int_vector	= amd_deferred_error_interrupt;
> +	}

Nah, this should be done differently: you define a function
cpu_mca_init() which you call from early_identify_cpu(). In it, you do
the proper checks and assign those two vectors above. That in
a pre-patch.

Then, the rest becomes per-CPU code which you simply run in
mce_amd_feature_init(), dilligently, one thing after the other.

And then you don't need smca_{dfr,thr}_intr_enabled anymore because you
know that after having run setup_APIC_eilvt().

IOW, mce_amd_feature_init() does *all* per-CPU MCA init on AMD and it is
all concentrated in one place and not spread around.

I think this should be a much better cleanup.

Thx.
Yazen Ghannam Aug. 16, 2024, 2:17 p.m. UTC | #2
On Tue, Jun 04, 2024 at 05:46:35PM +0200, Borislav Petkov wrote:
> On Thu, May 23, 2024 at 10:56:40AM -0500, Yazen Ghannam wrote:
> >  static bool thresholding_irq_en;
> >  static DEFINE_PER_CPU_READ_MOSTLY(mce_banks_t, mce_thr_intr_banks);
> >  static DEFINE_PER_CPU_READ_MOSTLY(mce_banks_t, mce_dfr_intr_banks);
> > +static DEFINE_PER_CPU_READ_MOSTLY(bool, smca_thr_intr_enabled);
> > +static DEFINE_PER_CPU_READ_MOSTLY(bool, smca_dfr_intr_enabled);
> 
> So before you add those, we already have:
> 
> static DEFINE_PER_CPU_READ_MOSTLY(struct smca_bank[MAX_NR_BANKS], smca_banks);
> static DEFINE_PER_CPU_READ_MOSTLY(u8[N_SMCA_BANK_TYPES], smca_bank_counts);
> static DEFINE_PER_CPU(struct threshold_bank **, threshold_banks);
> static DEFINE_PER_CPU(u64, bank_map);
> static DEFINE_PER_CPU(u64, smca_misc_banks_map);
> 
> Please think of a proper struct which collects all that info in the
> smallest possible format and unify everything.
> 
> It is a mess currently.
> 

Agreed. I actually want to remove almost all of those. You can see the
goal here: https://github.com/AMDESE/linux/tree/wip-mca

Of course, this is out-of-date. I'll collect any new variables into a
struct so we (hopefully) don't repeat history. :P

> > +/*
> > + * 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(struct cpuinfo_x86 *c)
> > +{
> > +	u8 thr_offset, dfr_offset;
> > +	u64 mca_intr_cfg;
> > +
> > +	if (!mce_flags.smca || !mce_flags.succor)
> > +		return;
> > +
> > +	if (c == &boot_cpu_data) {
> > +		mce_threshold_vector		= amd_threshold_interrupt;
> > +		deferred_error_int_vector	= amd_deferred_error_interrupt;
> > +	}
> 
> Nah, this should be done differently: you define a function
> cpu_mca_init() which you call from early_identify_cpu(). In it, you do
> the proper checks and assign those two vectors above. That in
> a pre-patch.
> 
> Then, the rest becomes per-CPU code which you simply run in
> mce_amd_feature_init(), dilligently, one thing after the other.
> 
> And then you don't need smca_{dfr,thr}_intr_enabled anymore because you
> know that after having run setup_APIC_eilvt().
> 
> IOW, mce_amd_feature_init() does *all* per-CPU MCA init on AMD and it is
> all concentrated in one place and not spread around.
> 
> I think this should be a much better cleanup.
>

Okay, will work on it.

I have a couple of other "init cleanup" patches from a previous
discussion. I'll fold those into this set.

Thanks,
Yazen
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index c6594da95340..7acaa21e11e1 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -46,9 +46,6 @@ 
 /* Deferred error settings */
 #define MSR_CU_DEF_ERR		0xC0000410
 #define MASK_DEF_LVTOFF		0x000000F0
-#define MASK_DEF_INT_TYPE	0x00000006
-#define DEF_LVT_OFF		0x2
-#define DEF_INT_TYPE_APIC	0x2
 
 /* Scalable MCA: */
 
@@ -58,6 +55,8 @@ 
 static bool thresholding_irq_en;
 static DEFINE_PER_CPU_READ_MOSTLY(mce_banks_t, mce_thr_intr_banks);
 static DEFINE_PER_CPU_READ_MOSTLY(mce_banks_t, mce_dfr_intr_banks);
+static DEFINE_PER_CPU_READ_MOSTLY(bool, smca_thr_intr_enabled);
+static DEFINE_PER_CPU_READ_MOSTLY(bool, smca_dfr_intr_enabled);
 
 static const char * const th_names[] = {
 	"load_store",
@@ -297,7 +296,8 @@  static void smca_configure(unsigned int bank, unsigned int cpu)
 		 * APIC based interrupt. First, check that no interrupt has been
 		 * set.
 		 */
-		if ((low & BIT(5)) && !((high >> 5) & 0x3)) {
+		if ((low & BIT(5)) && !((high >> 5) & 0x3) &&
+		    this_cpu_read(smca_dfr_intr_enabled)) {
 			__set_bit(bank, this_cpu_ptr(mce_dfr_intr_banks));
 			high |= BIT(5);
 		}
@@ -389,6 +389,14 @@  static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
 {
 	int msr = (hi & MASK_LVTOFF_HI) >> 20;
 
+	/*
+	 * On SMCA CPUs, LVT offset is programmed at a different MSR, and
+	 * the BIOS provides the value. The original field where LVT offset
+	 * was set is reserved. Return early here:
+	 */
+	if (mce_flags.smca)
+		return 0;
+
 	if (apic < 0) {
 		pr_err(FW_BUG "cpu %d, failed to setup threshold interrupt "
 		       "for bank %d, block %d (MSR%08X=0x%x%08x)\n", b->cpu,
@@ -397,14 +405,6 @@  static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 hi)
 	}
 
 	if (apic != msr) {
-		/*
-		 * On SMCA CPUs, LVT offset is programmed at a different MSR, and
-		 * the BIOS provides the value. The original field where LVT offset
-		 * was set is reserved. Return early here:
-		 */
-		if (mce_flags.smca)
-			return 0;
-
 		pr_err(FW_BUG "cpu %d, invalid threshold interrupt offset %d "
 		       "for bank %d, block %d (MSR%08X=0x%x%08x)\n",
 		       b->cpu, apic, b->bank, b->block, b->address, hi, lo);
@@ -485,41 +485,6 @@  static int setup_APIC_mce_threshold(int reserved, int new)
 	return reserved;
 }
 
-static int setup_APIC_deferred_error(int reserved, int new)
-{
-	if (reserved < 0 && !setup_APIC_eilvt(new, DEFERRED_ERROR_VECTOR,
-					      APIC_EILVT_MSG_FIX, 0))
-		return new;
-
-	return reserved;
-}
-
-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);
-	}
-
-	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);
-}
-
 static u32 smca_get_block_address(unsigned int bank, unsigned int block,
 				  unsigned int cpu)
 {
@@ -565,7 +530,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;
 
@@ -585,18 +549,10 @@  prepare_threshold_block(unsigned int bank, unsigned int block, u32 addr,
 	__set_bit(bank, this_cpu_ptr(mce_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;
@@ -604,7 +560,6 @@  prepare_threshold_block(unsigned int bank, unsigned int block, u32 addr,
 done:
 	mce_threshold_block_init(&b, offset);
 
-out:
 	return offset;
 }
 
@@ -673,6 +628,37 @@  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(struct cpuinfo_x86 *c)
+{
+	u8 thr_offset, dfr_offset;
+	u64 mca_intr_cfg;
+
+	if (!mce_flags.smca || !mce_flags.succor)
+		return;
+
+	if (c == &boot_cpu_data) {
+		mce_threshold_vector		= amd_threshold_interrupt;
+		deferred_error_int_vector	= amd_deferred_error_interrupt;
+	}
+
+	if (rdmsrl_safe(MSR_CU_DEF_ERR, &mca_intr_cfg))
+		return;
+
+	thr_offset = (mca_intr_cfg & SMCA_THR_LVT_OFF) >> 12;
+	if (!setup_APIC_eilvt(thr_offset, THRESHOLD_APIC_VECTOR, APIC_EILVT_MSG_FIX, 0))
+		this_cpu_write(smca_thr_intr_enabled, true);
+
+	dfr_offset = (mca_intr_cfg & MASK_DEF_LVTOFF) >> 4;
+	if (!setup_APIC_eilvt(dfr_offset, DEFERRED_ERROR_VECTOR, APIC_EILVT_MSG_FIX, 0))
+		this_cpu_write(smca_dfr_intr_enabled, true);
+}
+
 /* cpu init entry point, called from mce.c with preempt off */
 void mce_amd_feature_init(struct cpuinfo_x86 *c)
 {
@@ -680,11 +666,16 @@  void mce_amd_feature_init(struct cpuinfo_x86 *c)
 	u32 low = 0, high = 0, address = 0;
 	int offset = -1;
 
+	smca_enable_interrupt_vectors(c);
 
 	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_read(smca_thr_intr_enabled))
+				continue;
+		}
+
 		disable_err_thresholding(c, bank);
 
 		for (block = 0; block < NR_BLOCKS; ++block) {
@@ -705,9 +696,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);
 }
 
 /*