[PATCH] x86/MCE, EDAC/mce_amd: Save all aux registers on SMCA systems

[PATCH] x86/MCE, EDAC/mce_amd: Save all aux registers on SMCA systems

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

The Intel SDM and AMD APM both state that the auxiliary MCA registers
should be read if their respective valid bits are set in MCA_STATUS.

The Processor Programming Reference for AMD Fam17h systems has a new
recommendation that the auxiliary registers should be saved
unconditionally. This recommendation can be retroactively applied to
older AMD systems. However, we only need to apply this to SMCA systems
to avoid modifying behavior on older systems.

Define a separate function to save all auxiliary registers on SMCA
systems. Call this function from both the MCE handlers and the AMD LVT
interrupt handlers so that we don't duplicate code.

Print all auxiliary registers in EDAC/mce_amd. Don't restrict this to
SMCA systems in order to save a conditional and keep the format similar
between SMCA and non-SMCA systems.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
Links:
https://lkml.kernel.org/r/20180326191526.64314-1-Yazen.Ghannam@amd.com
https://lkml.kernel.org/r/20180326191526.64314-2-Yazen.Ghannam@amd.com

 arch/x86/kernel/cpu/mcheck/mce-internal.h |  6 +++
 arch/x86/kernel/cpu/mcheck/mce.c          | 20 ++--------
 arch/x86/kernel/cpu/mcheck/mce_amd.c      | 65 +++++++++++++++++++++----------
 drivers/edac/mce_amd.c                    | 12 ++----
 4 files changed, 57 insertions(+), 46 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h
index 374d1aa66952..67a2c7c095ca 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-internal.h
+++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h
@@ -59,6 +59,12 @@ static inline void mce_intel_hcpu_update(unsigned long cpu) { }
 static inline void cmci_disable_bank(int bank) { }
 #endif
 
+#ifdef CONFIG_X86_MCE_AMD
+bool smca_read_aux(struct mce *m, int bank);
+#else
+static inline bool smca_read_aux(struct mce *m, int bank) { return false; }
+#endif
+
 void mce_timer_kick(unsigned long interval);
 
 #ifdef CONFIG_ACPI_APEI
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 42cf2880d0ed..6be63e9e067d 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -639,6 +639,9 @@ static struct notifier_block mce_default_nb = {
  */
 static void mce_read_aux(struct mce *m, int i)
 {
+	if (smca_read_aux(m, i))
+		return;
+
 	if (m->status & MCI_STATUS_MISCV)
 		m->misc = mce_rdmsrl(msr_ops.misc(i));
 
@@ -653,23 +656,6 @@ static void mce_read_aux(struct mce *m, int i)
 			m->addr >>= shift;
 			m->addr <<= shift;
 		}
-
-		/*
-		 * Extract [55:<lsb>] where lsb is the least significant
-		 * *valid* bit of the address bits.
-		 */
-		if (mce_flags.smca) {
-			u8 lsb = (m->addr >> 56) & 0x3f;
-
-			m->addr &= GENMASK_ULL(55, lsb);
-		}
-	}
-
-	if (mce_flags.smca) {
-		m->ipid = mce_rdmsrl(MSR_AMD64_SMCA_MCx_IPID(i));
-
-		if (m->status & MCI_STATUS_SYNDV)
-			m->synd = mce_rdmsrl(MSR_AMD64_SMCA_MCx_SYND(i));
 	}
 }
 
diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index f7666eef4a87..b00d5fff1848 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -244,6 +244,47 @@ static void smca_configure(unsigned int bank, unsigned int cpu)
 	}
 }
 
+
+static bool _smca_read_aux(struct mce *m, int bank, bool read_addr)
+{
+	if (!mce_flags.smca)
+		return false;
+
+	rdmsrl(MSR_AMD64_SMCA_MCx_IPID(bank), m->ipid);
+	rdmsrl(MSR_AMD64_SMCA_MCx_SYND(bank), m->synd);
+
+	/*
+	 * We should already have a value if we're coming from the Threshold LVT
+	 * interrupt handler. Otherwise, read it now.
+	 */
+	if (!m->misc)
+		rdmsrl(msr_ops.misc(bank), m->misc);
+
+	/*
+	 * Read MCA_ADDR if we don't have it already. We should already have it
+	 * if we're coming from the interrupt handlers.
+	 */
+	if (read_addr)
+		rdmsrl(msr_ops.addr(bank), m->addr);
+
+	/*
+	 * Extract [55:<lsb>] where lsb is the least significant
+	 * *valid* bit of the address bits.
+	 */
+	if (m->addr) {
+		u8 lsb = (m->addr >> 56) & 0x3f;
+
+		m->addr &= GENMASK_ULL(55, lsb);
+	}
+
+	return true;
+}
+
+bool smca_read_aux(struct mce *m, int bank)
+{
+	return _smca_read_aux(m, bank, true);
+}
+
 struct thresh_restart {
 	struct threshold_block	*b;
 	int			reset;
@@ -799,30 +840,12 @@ static void __log_error(unsigned int bank, u64 status, u64 addr, u64 misc)
 	mce_setup(&m);
 
 	m.status = status;
+	m.addr	 = addr;
 	m.misc   = misc;
 	m.bank   = bank;
 	m.tsc	 = rdtsc();
 
-	if (m.status & MCI_STATUS_ADDRV) {
-		m.addr = addr;
-
-		/*
-		 * Extract [55:<lsb>] where lsb is the least significant
-		 * *valid* bit of the address bits.
-		 */
-		if (mce_flags.smca) {
-			u8 lsb = (m.addr >> 56) & 0x3f;
-
-			m.addr &= GENMASK_ULL(55, lsb);
-		}
-	}
-
-	if (mce_flags.smca) {
-		rdmsrl(MSR_AMD64_SMCA_MCx_IPID(bank), m.ipid);
-
-		if (m.status & MCI_STATUS_SYNDV)
-			rdmsrl(MSR_AMD64_SMCA_MCx_SYND(bank), m.synd);
-	}
+	_smca_read_aux(&m, bank, false);
 
 	mce_log(&m);
 }
@@ -849,7 +872,7 @@ _log_error_bank(unsigned int bank, u32 msr_stat, u32 msr_addr, u64 misc)
 	if (!(status & MCI_STATUS_VAL))
 		return false;
 
-	if (status & MCI_STATUS_ADDRV)
+	if ((status & MCI_STATUS_ADDRV) || mce_flags.smca)
 		rdmsrl(msr_addr, addr);
 
 	__log_error(bank, status, addr, misc);
diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index 2ab4d61ee47e..d7badf80f13f 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -990,16 +990,12 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 
 	pr_cont("]: 0x%016llx\n", m->status);
 
-	if (m->status & MCI_STATUS_ADDRV)
-		pr_emerg(HW_ERR "Error Addr: 0x%016llx\n", m->addr);
+	pr_emerg(HW_ERR "Error Addr: 0x%016llx, Misc: 0x%016llx\n",
+		 m->addr, m->misc);
 
 	if (boot_cpu_has(X86_FEATURE_SMCA)) {
-		pr_emerg(HW_ERR "IPID: 0x%016llx", m->ipid);
-
-		if (m->status & MCI_STATUS_SYNDV)
-			pr_cont(", Syndrome: 0x%016llx", m->synd);
-
-		pr_cont("\n");
+		pr_emerg(HW_ERR "IPID: 0x%016llx, Syndrome: 0x%016llx\n",
+			 m->ipid, m->synd);
 
 		decode_smca_error(m);
 		goto err_code;
-- 
2.14.1

Re: [PATCH] x86/MCE, EDAC/mce_amd: Save all aux registers on SMCA systems

[Borislav Petkov]

On Mon, Apr 02, 2018 at 02:57:07PM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> The Intel SDM and AMD APM both state that the auxiliary MCA registers
> should be read if their respective valid bits are set in MCA_STATUS.
> 
> The Processor Programming Reference for AMD Fam17h systems has a new
> recommendation that the auxiliary registers should be saved
> unconditionally. This recommendation can be retroactively applied to
> older AMD systems. However, we only need to apply this to SMCA systems
> to avoid modifying behavior on older systems.

Applying the logic of that recommendation on older systems: wouldn't it
be prudent to save them there too, if it helps debugging an MCE?

> Define a separate function to save all auxiliary registers on SMCA
> systems. Call this function from both the MCE handlers and the AMD LVT
> interrupt handlers so that we don't duplicate code.
> 
> Print all auxiliary registers in EDAC/mce_amd. Don't restrict this to
> SMCA systems in order to save a conditional and keep the format similar
> between SMCA and non-SMCA systems.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>

...

> diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> index f7666eef4a87..b00d5fff1848 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> @@ -244,6 +244,47 @@ static void smca_configure(unsigned int bank, unsigned int cpu)
>  	}
>  }
>  
> +
> +static bool _smca_read_aux(struct mce *m, int bank, bool read_addr)
> +{
> +	if (!mce_flags.smca)
> +		return false;
> +
> +	rdmsrl(MSR_AMD64_SMCA_MCx_IPID(bank), m->ipid);
> +	rdmsrl(MSR_AMD64_SMCA_MCx_SYND(bank), m->synd);
> +
> +	/*
> +	 * We should already have a value if we're coming from the Threshold LVT
> +	 * interrupt handler. Otherwise, read it now.
> +	 */
> +	if (!m->misc)
> +		rdmsrl(msr_ops.misc(bank), m->misc);
> +
> +	/*
> +	 * Read MCA_ADDR if we don't have it already. We should already have it
> +	 * if we're coming from the interrupt handlers.
> +	 */
> +	if (read_addr)

Why not

	if (!m->addr)

?

And yeah, if it has been read to 0 already, reading it again won't
change anything.

And thinking about it more, you don't really need those if-tests, I'd
say. So what, you'll read one or two MSRs once more. It is not such a
hot path that we can't stomach the perf penalty of reading the MSRs.

> +		rdmsrl(msr_ops.addr(bank), m->addr);
> +
> +	/*
> +	 * Extract [55:<lsb>] where lsb is the least significant
> +	 * *valid* bit of the address bits.
> +	 */
> +	if (m->addr) {

And that test is probably not needed either: if m->addr is 0, the
below would be 0 anyway.

> +		u8 lsb = (m->addr >> 56) & 0x3f;
> +
> +		m->addr &= GENMASK_ULL(55, lsb);
> +	}
> +
> +	return true;
> +}

IOW, those tests are probably ok but getting rid of them would make the
code more readable and I think we can afford that here.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

RE: [PATCH] x86/MCE, EDAC/mce_amd: Save all aux registers on SMCA systems

[Ghannam, Yazen]

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Tuesday, April 17, 2018 1:21 PM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org;
> tony.luck@intel.com; x86@kernel.org
> Subject: Re: [PATCH] x86/MCE, EDAC/mce_amd: Save all aux registers on
> SMCA systems
> 
> On Mon, Apr 02, 2018 at 02:57:07PM -0500, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> >
> > The Intel SDM and AMD APM both state that the auxiliary MCA registers
> > should be read if their respective valid bits are set in MCA_STATUS.
> >
> > The Processor Programming Reference for AMD Fam17h systems has a new
> > recommendation that the auxiliary registers should be saved
> > unconditionally. This recommendation can be retroactively applied to
> > older AMD systems. However, we only need to apply this to SMCA systems
> > to avoid modifying behavior on older systems.
> 
> Applying the logic of that recommendation on older systems: wouldn't it
> be prudent to save them there too, if it helps debugging an MCE?
> 

We could but it's an issue of documentation and testing the older systems.

My first pass at this was to unconditionally read the registers because my
understanding was that registers that aren't accessible would be read-as-zero.
I thought this was a common MCA implementation. But Tony pointed out that
this isn't the case on Intel systems. This is the case on recent AMD systems. But
I don't know if it's the case on older systems which may or may not have
followed the Intel implementation more closely.

So to be safe, HW folks said we can restrict this to only SMCA systems because
1) The recommendation first shows up in the Fam17h PPR.
2) We know it's safe from Fam17h onwards.

> > Define a separate function to save all auxiliary registers on SMCA
> > systems. Call this function from both the MCE handlers and the AMD LVT
> > interrupt handlers so that we don't duplicate code.
> >
> > Print all auxiliary registers in EDAC/mce_amd. Don't restrict this to
> > SMCA systems in order to save a conditional and keep the format similar
> > between SMCA and non-SMCA systems.
> >
> > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> ...
> 
> > diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c
> b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> > index f7666eef4a87..b00d5fff1848 100644
> > --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
> > +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> > @@ -244,6 +244,47 @@ static void smca_configure(unsigned int bank,
> unsigned int cpu)
> >  	}
> >  }
> >
> > +
> > +static bool _smca_read_aux(struct mce *m, int bank, bool read_addr)
> > +{
> > +	if (!mce_flags.smca)
> > +		return false;
> > +
> > +	rdmsrl(MSR_AMD64_SMCA_MCx_IPID(bank), m->ipid);
> > +	rdmsrl(MSR_AMD64_SMCA_MCx_SYND(bank), m->synd);
> > +
> > +	/*
> > +	 * We should already have a value if we're coming from the
> Threshold LVT
> > +	 * interrupt handler. Otherwise, read it now.
> > +	 */
> > +	if (!m->misc)
> > +		rdmsrl(msr_ops.misc(bank), m->misc);
> > +
> > +	/*
> > +	 * Read MCA_ADDR if we don't have it already. We should already
> have it
> > +	 * if we're coming from the interrupt handlers.
> > +	 */
> > +	if (read_addr)
> 
> Why not
> 
> 	if (!m->addr)
> 
> ?
> 
> And yeah, if it has been read to 0 already, reading it again won't
> change anything.
> 
> And thinking about it more, you don't really need those if-tests, I'd
> say. So what, you'll read one or two MSRs once more. It is not such a
> hot path that we can't stomach the perf penalty of reading the MSRs.
> 

The issue here is because we share this path with the interrupt handlers,
specifically the Deferred error interrupt handler. The DFR handler will
read from MCA_ADDR or MCA_DEADDR so we should just use what it
got. Otherwise, we may read MCA_ADDR and assume it's correct for
the error.

For example,

Deferred error occurs:
- MCA_{STATUS,ADDR,DESTAT,DEADDR} all have valid data.

MCE occurs
- MCA_{STATUS,ADDR} are overwritten with non-zero data.
- MCE handler clears MCA_STATUS. MCA_ADDR is non-zero.

DFR handler finds MCA_STATUS[Deferred] is clear, so it saves
MCA_DESTAT and MCA_DEADDR which is 0.

If !m->addr (which has MCA_DEADDR), then we read MCA_STATUS
which has the address from the MCE.


> > +		rdmsrl(msr_ops.addr(bank), m->addr);
> > +
> > +	/*
> > +	 * Extract [55:<lsb>] where lsb is the least significant
> > +	 * *valid* bit of the address bits.
> > +	 */
> > +	if (m->addr) {
> 
> And that test is probably not needed either: if m->addr is 0, the
> below would be 0 anyway.
> 
> > +		u8 lsb = (m->addr >> 56) & 0x3f;
> > +
> > +		m->addr &= GENMASK_ULL(55, lsb);
> > +	}
> > +
> > +	return true;
> > +}
> 
> IOW, those tests are probably ok but getting rid of them would make the
> code more readable and I think we can afford that here.
> 

Okay, I'll get rid the last test. But the first one is necessary with our current
code flow.

Thanks,
Yazen

Re: [PATCH] x86/MCE, EDAC/mce_amd: Save all aux registers on SMCA systems

[Borislav Petkov]

On Tue, Apr 17, 2018 at 06:30:34PM +0000, Ghannam, Yazen wrote:
> We could but it's an issue of documentation and testing the older systems.
> 
> My first pass at this was to unconditionally read the registers because my
> understanding was that registers that aren't accessible would be read-as-zero.
> I thought this was a common MCA implementation. But Tony pointed out that
> this isn't the case on Intel systems. This is the case on recent AMD systems. But
> I don't know if it's the case on older systems which may or may not have
> followed the Intel implementation more closely.

So if our worry is the #GPs, we can always use the rdmsr*_safe()
variants and look at the return value. And dump a invalid value like
0xdeadbeef or so, if the read failed.

But if any bit of info we've gotten this way, helps us debug an MCE,
we're already golden!

> For example,
> 
> Deferred error occurs:
> - MCA_{STATUS,ADDR,DESTAT,DEADDR} all have valid data.
> 
> MCE occurs
> - MCA_{STATUS,ADDR} are overwritten with non-zero data.
> - MCE handler clears MCA_STATUS. MCA_ADDR is non-zero.
> 
> DFR handler finds MCA_STATUS[Deferred] is clear, so it saves
> MCA_DESTAT and MCA_DEADDR which is 0.
> 
> If !m->addr (which has MCA_DEADDR), then we read MCA_STATUS
> which has the address from the MCE.

The code could use a shorter version of this as a comment to state why
we're doing it. Because it is not obvious.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

RE: [PATCH] x86/MCE, EDAC/mce_amd: Save all aux registers on SMCA systems

[Ghannam, Yazen]

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Wednesday, April 18, 2018 1:14 PM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org;
> tony.luck@intel.com; x86@kernel.org
> Subject: Re: [PATCH] x86/MCE, EDAC/mce_amd: Save all aux registers on
> SMCA systems
> 
> On Tue, Apr 17, 2018 at 06:30:34PM +0000, Ghannam, Yazen wrote:
> > We could but it's an issue of documentation and testing the older systems.
> >
> > My first pass at this was to unconditionally read the registers because my
> > understanding was that registers that aren't accessible would be read-as-
> zero.
> > I thought this was a common MCA implementation. But Tony pointed out
> that
> > this isn't the case on Intel systems. This is the case on recent AMD systems.
> But
> > I don't know if it's the case on older systems which may or may not have
> > followed the Intel implementation more closely.
> 
> So if our worry is the #GPs, we can always use the rdmsr*_safe()
> variants and look at the return value. And dump a invalid value like
> 0xdeadbeef or so, if the read failed.
> 
> But if any bit of info we've gotten this way, helps us debug an MCE,
> we're already golden!
> 

Okay, I can do that. What about using mce_rdmsrl()? The value gets set to
0 and a user gets a single warning. This may be more clear to the user. Also,
it shouldn't affect code that checks for non-zero values, like in __print_mce().

> > For example,
> >
> > Deferred error occurs:
> > - MCA_{STATUS,ADDR,DESTAT,DEADDR} all have valid data.
> >
> > MCE occurs
> > - MCA_{STATUS,ADDR} are overwritten with non-zero data.
> > - MCE handler clears MCA_STATUS. MCA_ADDR is non-zero.
> >
> > DFR handler finds MCA_STATUS[Deferred] is clear, so it saves
> > MCA_DESTAT and MCA_DEADDR which is 0.
> >
> > If !m->addr (which has MCA_DEADDR), then we read MCA_STATUS
> > which has the address from the MCE.
> 
> The code could use a shorter version of this as a comment to state why
> we're doing it. Because it is not obvious.
> 

Yes, will do.

Thanks,
Yazen

Re: [PATCH] x86/MCE, EDAC/mce_amd: Save all aux registers on SMCA systems

[Borislav Petkov]

On Fri, Apr 20, 2018 at 01:05:17PM +0000, Ghannam, Yazen wrote:
> Okay, I can do that. What about using mce_rdmsrl()? The value gets set to
> 0 and a user gets a single warning. This may be more clear to the user. Also,
> it shouldn't affect code that checks for non-zero values, like in __print_mce().

Hmm, good point. From the looks of it, it shouldn't be a problem and a
MSR value of 0 is of no interest anyway. I *think*.

But pls make it a separate patch so that we can revert it in case
something doesn't work as expected and we've missed a case.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.