[PATCH v3 0/6] x86/mce: Handle error injection failures in mce-inject module

[PATCH v3 0/6] x86/mce: Handle error injection failures in mce-inject module

[Smita Koralahalli]

This series of patches handles the scenarios where error injection
fails silently on mce-inject module. It also sets the valid bit in
MCA_STATUS register unconditionally to correct Val=0 injection made by the
user and finally returns error code to userspace on failures injecting the
module.

Error injection fails if the bank is unpopulated (MCA_IPID register reads
zero) or if the platform enforces write ignored behavior on status
registers.

The first patch checks for an unpopulated bank by reading the value out
from MCA_IPID register and the third patch checks for writes ignored from
MCA_STATUS and MCA_DESTAT.

The second patch sets valid bit before doing error injection.

The fourth and fifth patch does some cleanup in prepare_msrs(). No
functional changes in these two patches.

The final patch returns error code to userspace from mce-inject module.

Smita Koralahalli (6):
  x86/mce/inject: Check if a bank is unpopulated before error injection
  x86/mce/inject: Set the valid bit in MCA_STATUS before error injection
  x86/mce/inject: Check for writes ignored in status registers
  x86/mce/inject: Simplify evaluation of writes ignored in status
    registers
  x86/mce/inject: Restructure prepare_msrs()
  x86/mce/mce-inject: Return error code to userspace from mce-inject
    module

 arch/x86/kernel/cpu/mce/inject.c | 106 ++++++++++++++++++++++++++-----
 1 file changed, 90 insertions(+), 16 deletions(-)

-- 
2.17.1

[PATCH v3 1/6] x86/mce/inject: Check if a bank is unpopulated before error injection

[Smita Koralahalli]

The MCA_IPID register uniquely identifies a bank's type on Scalable MCA
(SMCA) systems. When an MCA bank is not populated, the MCA_IPID register
will read as zero and writes to it will be ignored.

On a hw-type error injection (injection which writes the actual MCA
registers in an attempt to cause a real MCE) check the value of this
register before trying to inject the error.

Do not impose any limitations on a sw injection and allow the user to
test out all the decoding paths without relying on the available hardware,
as its purpose is to just test the code.

 [ bp: Heavily massage. ]

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lkml.kernel.org/r/20211019233641.140275-2-Smita.KoralahalliChannabasappa@amd.com
---
v2:
	simulate -> inject.
	Corrected according to kernel commenting style.
	boot_cpu_has() -> cpu_feature_enabled().
	Error simulation not possible: Bank %llu unpopulated ->
	Cannot set IPID - bank %llu unpopulated.
	Used user provided IPID value on sw injection without checking
	underlying hardware and defined it under inj_ipid_set().
v3:
	Restructure and simplify the code. For !sw injection fail the
	injection if IPID is zero and do otherwise.
---
 arch/x86/kernel/cpu/mce/inject.c | 44 +++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 0bfc14041bbb..fcec99e6c5d1 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -74,7 +74,6 @@ MCE_INJECT_SET(status);
 MCE_INJECT_SET(misc);
 MCE_INJECT_SET(addr);
 MCE_INJECT_SET(synd);
-MCE_INJECT_SET(ipid);
 
 #define MCE_INJECT_GET(reg)						\
 static int inj_##reg##_get(void *data, u64 *val)			\
@@ -95,6 +94,20 @@ DEFINE_SIMPLE_ATTRIBUTE(status_fops, inj_status_get, inj_status_set, "%llx\n");
 DEFINE_SIMPLE_ATTRIBUTE(misc_fops, inj_misc_get, inj_misc_set, "%llx\n");
 DEFINE_SIMPLE_ATTRIBUTE(addr_fops, inj_addr_get, inj_addr_set, "%llx\n");
 DEFINE_SIMPLE_ATTRIBUTE(synd_fops, inj_synd_get, inj_synd_set, "%llx\n");
+
+/* Use the user provided IPID value on a sw injection. */
+static int inj_ipid_set(void *data, u64 val)
+{
+	struct mce *m = (struct mce *)data;
+
+	if (cpu_feature_enabled(X86_FEATURE_SMCA)) {
+		if (inj_type == SW_INJ)
+			m->ipid = val;
+	}
+
+	return 0;
+}
+
 DEFINE_SIMPLE_ATTRIBUTE(ipid_fops, inj_ipid_get, inj_ipid_set, "%llx\n");
 
 static void setup_inj_struct(struct mce *m)
@@ -577,6 +590,35 @@ static int inj_bank_set(void *data, u64 val)
 	}
 
 	m->bank = val;
+
+	/*
+	 * sw-only injection allows to write arbitrary values into the MCA
+	 * registers because it tests only the decoding paths.
+	 */
+	if (inj_type == SW_INJ)
+		goto inject;
+
+	/*
+	 * Read IPID value to determine if a bank is populated on the target
+	 * CPU.
+	 */
+	if (cpu_feature_enabled(X86_FEATURE_SMCA)) {
+		u64 ipid;
+
+		if (rdmsrl_on_cpu(m->extcpu, MSR_AMD64_SMCA_MCx_IPID(val),
+				  &ipid)) {
+			pr_err("Error reading IPID on CPU%d\n", m->extcpu);
+			return -EINVAL;
+		}
+
+		if (!ipid) {
+			pr_err("Cannot inject into bank %llu - it is unpopulated\n",
+				val);
+			return -ENODEV;
+		}
+	}
+
+inject:
 	do_inject();
 
 	/* Reset injection struct */
-- 
2.17.1

[PATCH v3 2/6] x86/mce/inject: Set the valid bit in MCA_STATUS before error injection

[Smita Koralahalli]

MCA handlers check the valid bit in each status register (MCA_STATUS[Val])
and examine the remainder of the status register only if the valid bit is
set.

Set the valid bit unconditionally in the corresponding MCA_STATUS register
and correct any Val=0 injections made by the user.

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
Link: https://lkml.kernel.org/r/20211019233641.140275-3-Smita.KoralahalliChannabasappa@amd.com
---
v2:
	Added a warning statement instead of setting the valid bit.
v3:
	Reverted the changes and set the valid bit conditionally.
---
 arch/x86/kernel/cpu/mce/inject.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index fcec99e6c5d1..5e83a1ce7ac8 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -503,6 +503,8 @@ static void do_inject(void)
 
 	i_mce.tsc = rdtsc_ordered();
 
+	i_mce.status |= MCI_STATUS_VAL;
+
 	if (i_mce.misc)
 		i_mce.status |= MCI_STATUS_MISCV;
 
-- 
2.17.1

[PATCH v3 3/6] x86/mce/inject: Check for writes ignored in status registers

[Smita Koralahalli]

According to Section 2.1.16.3 under HWCR[McStatusWrEn] in "PPR for AMD
Family 19h, Model 01h, Revision B1 Processors - 55898 Rev 0.35 - Feb 5,
2021", the status register may sometimes enforce write ignored behavior
independent of the value of HWCR[McStatusWrEn] depending on the platform
settings.

Hence, evaluate for writes ignored for MCA_STATUS and MCA_DESTAT
separately, before doing error injection. If true, return with an error
code.

Deferred errors on an SMCA platform use different MSR for MCA_DESTAT.
Hence, evaluate MCA_DESTAT instead of MCA_STATUS on deferred errors, and
do not modify the existing value in MCA_STATUS by writing and reading from
it.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
Link: https://lkml.kernel.org/r/20211019233641.140275-5-Smita.KoralahalliChannabasappa@amd.com
---
v2:
	msr_ops -> mca_msr_reg().
	simulation -> injection.
	pr_info() -> pr_err().
	Aligned on ",".
v3:
	Removed "x86/mce: Use mca_msr_reg() in prepare_msrs()" patch
	and made changes on the existing MCx_{STATUS, ADDR, MISC} macros.
---
 arch/x86/kernel/cpu/mce/inject.c | 35 ++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 5e83a1ce7ac8..4d5689342384 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -470,9 +470,17 @@ static void toggle_nb_mca_mst_cpu(u16 nid)
 		       __func__, PCI_FUNC(F3->devfn), NBCFG);
 }
 
+struct mce_err_handler {
+	struct mce *mce;
+	int err;
+};
+
+static struct mce_err_handler mce_err;
+
 static void prepare_msrs(void *info)
 {
-	struct mce m = *(struct mce *)info;
+	struct mce_err_handler *i_mce_err = ((struct mce_err_handler *)info);
+	struct mce m = *i_mce_err->mce;
 	u8 b = m.bank;
 
 	wrmsrl(MSR_IA32_MCG_STATUS, m.mcgstatus);
@@ -480,9 +488,17 @@ static void prepare_msrs(void *info)
 	if (boot_cpu_has(X86_FEATURE_SMCA)) {
 		if (m.inject_flags == DFR_INT_INJ) {
 			wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(b), m.status);
+			rdmsrl(MSR_AMD64_SMCA_MCx_DESTAT(b), m.status);
+			if (!m.status)
+				goto out;
+
 			wrmsrl(MSR_AMD64_SMCA_MCx_DEADDR(b), m.addr);
 		} else {
 			wrmsrl(MSR_AMD64_SMCA_MCx_STATUS(b), m.status);
+			rdmsrl(MSR_AMD64_SMCA_MCx_STATUS(b), m.status);
+			if (!m.status)
+				goto out;
+
 			wrmsrl(MSR_AMD64_SMCA_MCx_ADDR(b), m.addr);
 		}
 
@@ -490,9 +506,18 @@ static void prepare_msrs(void *info)
 		wrmsrl(MSR_AMD64_SMCA_MCx_SYND(b), m.synd);
 	} else {
 		wrmsrl(MSR_IA32_MCx_STATUS(b), m.status);
+		rdmsrl(MSR_IA32_MCx_STATUS(b), m.status);
+		if (!m.status)
+			goto out;
+
 		wrmsrl(MSR_IA32_MCx_ADDR(b), m.addr);
 		wrmsrl(MSR_IA32_MCx_MISC(b), m.misc);
 	}
+
+out:
+	pr_err("Error injection is not available\n");
+	i_mce_err->err = -EINVAL;
+	return;
 }
 
 static void do_inject(void)
@@ -501,6 +526,9 @@ static void do_inject(void)
 	unsigned int cpu = i_mce.extcpu;
 	u8 b = i_mce.bank;
 
+	mce_err.mce = &i_mce;
+	mce_err.err = 0;
+
 	i_mce.tsc = rdtsc_ordered();
 
 	i_mce.status |= MCI_STATUS_VAL;
@@ -552,10 +580,13 @@ static void do_inject(void)
 
 	i_mce.mcgstatus = mcg_status;
 	i_mce.inject_flags = inj_type;
-	smp_call_function_single(cpu, prepare_msrs, &i_mce, 0);
+	smp_call_function_single(cpu, prepare_msrs, &mce_err, 0);
 
 	toggle_hw_mce_inject(cpu, false);
 
+	if (mce_err.err)
+		goto err;
+
 	switch (inj_type) {
 	case DFR_INT_INJ:
 		smp_call_function_single(cpu, trigger_dfr_int, NULL, 0);
-- 
2.17.1

[PATCH v3 4/6] x86/mce/inject: Simplify evaluation of writes ignored in status registers

[Smita Koralahalli]

Clean up code in prepare_msrs() and avoid reading the status variable
and checking for writes ignored each time for SMCA, legacy and deferred
case.

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
 arch/x86/kernel/cpu/mce/inject.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 4d5689342384..8772d8820994 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -483,41 +483,34 @@ static void prepare_msrs(void *info)
 	struct mce m = *i_mce_err->mce;
 	u8 b = m.bank;
 
+	u32 status_reg = MSR_IA32_MCx_STATUS(b);
+
 	wrmsrl(MSR_IA32_MCG_STATUS, m.mcgstatus);
 
 	if (boot_cpu_has(X86_FEATURE_SMCA)) {
 		if (m.inject_flags == DFR_INT_INJ) {
-			wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(b), m.status);
-			rdmsrl(MSR_AMD64_SMCA_MCx_DESTAT(b), m.status);
-			if (!m.status)
-				goto out;
-
+			status_reg = MSR_AMD64_SMCA_MCx_DESTAT(b);
 			wrmsrl(MSR_AMD64_SMCA_MCx_DEADDR(b), m.addr);
 		} else {
-			wrmsrl(MSR_AMD64_SMCA_MCx_STATUS(b), m.status);
-			rdmsrl(MSR_AMD64_SMCA_MCx_STATUS(b), m.status);
-			if (!m.status)
-				goto out;
-
+			status_reg = MSR_AMD64_SMCA_MCx_STATUS(b);
 			wrmsrl(MSR_AMD64_SMCA_MCx_ADDR(b), m.addr);
 		}
 
 		wrmsrl(MSR_AMD64_SMCA_MCx_MISC(b), m.misc);
 		wrmsrl(MSR_AMD64_SMCA_MCx_SYND(b), m.synd);
 	} else {
-		wrmsrl(MSR_IA32_MCx_STATUS(b), m.status);
-		rdmsrl(MSR_IA32_MCx_STATUS(b), m.status);
-		if (!m.status)
-			goto out;
-
 		wrmsrl(MSR_IA32_MCx_ADDR(b), m.addr);
 		wrmsrl(MSR_IA32_MCx_MISC(b), m.misc);
 	}
 
-out:
-	pr_err("Error injection is not available\n");
-	i_mce_err->err = -EINVAL;
-	return;
+	wrmsrl(status_reg, m.status);
+	rdmsrl(status_reg, m.status);
+
+	if (!m.status) {
+		pr_err("Error injection is not available\n");
+		i_mce_err->err = -EINVAL;
+		return;
+	}
 }
 
 static void do_inject(void)
-- 
2.17.1

[PATCH v3 5/6] x86/mce/inject: Restructure prepare_msrs()

[Smita Koralahalli]

Rearrange the calls and write to registers MCx_{ADDR, MISC, SYND} and
MCG_STATUS so that they are only done if error injection is available.

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
 arch/x86/kernel/cpu/mce/inject.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 8772d8820994..d4e6d753018f 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -484,23 +484,19 @@ static void prepare_msrs(void *info)
 	u8 b = m.bank;
 
 	u32 status_reg = MSR_IA32_MCx_STATUS(b);
-
-	wrmsrl(MSR_IA32_MCG_STATUS, m.mcgstatus);
+	u32 addr_reg   = MSR_IA32_MCx_ADDR(b);
+	u32 misc_reg   = MSR_IA32_MCx_MISC(b);
 
 	if (boot_cpu_has(X86_FEATURE_SMCA)) {
 		if (m.inject_flags == DFR_INT_INJ) {
 			status_reg = MSR_AMD64_SMCA_MCx_DESTAT(b);
-			wrmsrl(MSR_AMD64_SMCA_MCx_DEADDR(b), m.addr);
+			addr_reg   = MSR_AMD64_SMCA_MCx_DEADDR(b);
 		} else {
 			status_reg = MSR_AMD64_SMCA_MCx_STATUS(b);
-			wrmsrl(MSR_AMD64_SMCA_MCx_ADDR(b), m.addr);
+			addr_reg   = MSR_AMD64_SMCA_MCx_ADDR(b);
 		}
 
-		wrmsrl(MSR_AMD64_SMCA_MCx_MISC(b), m.misc);
-		wrmsrl(MSR_AMD64_SMCA_MCx_SYND(b), m.synd);
-	} else {
-		wrmsrl(MSR_IA32_MCx_ADDR(b), m.addr);
-		wrmsrl(MSR_IA32_MCx_MISC(b), m.misc);
+		misc_reg = MSR_AMD64_SMCA_MCx_MISC(b);
 	}
 
 	wrmsrl(status_reg, m.status);
@@ -511,6 +507,13 @@ static void prepare_msrs(void *info)
 		i_mce_err->err = -EINVAL;
 		return;
 	}
+
+	wrmsrl(MSR_IA32_MCG_STATUS, m.mcgstatus);
+	wrmsrl(addr_reg, m.addr);
+	wrmsrl(misc_reg, m.misc);
+
+	if (boot_cpu_has(X86_FEATURE_SMCA))
+		wrmsrl(MSR_AMD64_SMCA_MCx_SYND(b), m.synd);
 }
 
 static void do_inject(void)
-- 
2.17.1

[PATCH v3 6/6] x86/mce/mce-inject: Return error code to userspace from mce-inject module

[Smita Koralahalli]

Currently, the mce-inject module fails silently and user must look for
kernel logs to determine if the injection has succeeded.

Save time for the user and return error code from the module with
appropriate error statements if error injection fails.

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
Link: https://lkml.kernel.org/r/20211019233641.140275-6-Smita.KoralahalliChannabasappa@amd.com
---
v2:
	Added pr_err() along with error code.
v3:
	Rephrased the statement: No online CPUs available for error
	injection -> Chosen CPU is not online.
---
 arch/x86/kernel/cpu/mce/inject.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index d4e6d753018f..09f46d213cf5 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -569,8 +569,11 @@ static void do_inject(void)
 	}
 
 	cpus_read_lock();
-	if (!cpu_online(cpu))
+	if (!cpu_online(cpu)) {
+		pr_err("Chosen CPU is not online\n");
+		mce_err.err = -ENODEV;
 		goto err;
+	}
 
 	toggle_hw_mce_inject(cpu, true);
 
@@ -653,7 +656,7 @@ static int inj_bank_set(void *data, u64 val)
 	/* Reset injection struct */
 	setup_inj_struct(&i_mce);
 
-	return 0;
+	return mce_err.err;
 }
 
 MCE_INJECT_GET(bank);
-- 
2.17.1

Re: [PATCH v3 3/6] x86/mce/inject: Check for writes ignored in status registers

[Borislav Petkov]

On Thu, Nov 04, 2021 at 04:58:43PM -0500, Smita Koralahalli wrote:
> According to Section 2.1.16.3 under HWCR[McStatusWrEn] in "PPR for AMD
> Family 19h, Model 01h, Revision B1 Processors - 55898 Rev 0.35 - Feb 5,
> 2021", the status register may sometimes enforce write ignored behavior
> independent of the value of HWCR[McStatusWrEn] depending on the platform
> settings.

How and when can it enforce that?

Can we detect whether that enforcement is active and if so, fail the
injection directly instead of checking whether the writes have stuck in
the MSRs?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette