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

[PATCH v2 0/5] 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 cleans up the code by
replacing MCx_{STATUS, ADDR, MISC} macros with mca_msr_reg() 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 fourth patch checks for writes ignored from
MCA_STATUS and MCA_DESTAT.

The second patch warns the user about MCA handlers missing signatures if
valid bit is not set in MCA_STATUS register before doing error injection.

The third patch does some cleanup by replacing MCx_{STATUS, ADDR, MISC}
macros with mca_msr_reg().

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

Smita Koralahalli (5):
  x86/mce/inject: Check if a bank is unpopulated before error injection
  x86/mce/inject: Warn the user on a not set valid bit in MCA_STATUS
  x86/mce: Use mca_msr_reg() in prepare_msrs()
  x86/mce/inject: Check for writes ignored in status registers
  x86/mce/mce-inject: Return error code to userspace from mce-inject
    module

 arch/x86/kernel/cpu/mce/core.c   |   1 +
 arch/x86/kernel/cpu/mce/inject.c | 103 ++++++++++++++++++++++++-------
 2 files changed, 82 insertions(+), 22 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] 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" error injection check the value of this register before trying
to inject the error.

Do not impose any limitation 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.

Signed-off-by: Smita Koralahalli <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().
---
 arch/x86/kernel/cpu/mce/inject.c | 39 +++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 0bfc14041bbb..601efd104bb4 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)			\
@@ -89,13 +88,11 @@ MCE_INJECT_GET(status);
 MCE_INJECT_GET(misc);
 MCE_INJECT_GET(addr);
 MCE_INJECT_GET(synd);
-MCE_INJECT_GET(ipid);
 
 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");
-DEFINE_SIMPLE_ATTRIBUTE(ipid_fops, inj_ipid_get, inj_ipid_set, "%llx\n");
 
 static void setup_inj_struct(struct mce *m)
 {
@@ -577,6 +574,25 @@ static int inj_bank_set(void *data, u64 val)
 	}
 
 	m->bank = val;
+
+	/*
+	 * Read IPID value to determine if a bank is unpopulated on the target
+	 * CPU.
+	 */
+	if (cpu_feature_enabled(X86_FEATURE_SMCA)) {
+
+		/* Check for user provided IPID value on a sw injection. */
+		if (!m->ipid) {
+			rdmsrl_on_cpu(m->extcpu, MSR_AMD64_SMCA_MCx_IPID(val),
+				      &m->ipid);
+			if (!m->ipid) {
+				pr_err("Cannot set IPID - bank %llu unpopulated\n",
+					val);
+				return -ENODEV;
+			}
+		}
+	}
+
 	do_inject();
 
 	/* Reset injection struct */
@@ -589,6 +605,23 @@ MCE_INJECT_GET(bank);
 
 DEFINE_SIMPLE_ATTRIBUTE(bank_fops, inj_bank_get, inj_bank_set, "%llu\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 (val && inj_type == SW_INJ)
+			m->ipid = val;
+	}
+
+	return 0;
+}
+
+MCE_INJECT_GET(ipid);
+
+DEFINE_SIMPLE_ATTRIBUTE(ipid_fops, inj_ipid_get, inj_ipid_set, "%llx\n");
+
 static const char readme_msg[] =
 "Description of the files and their usages:\n"
 "\n"
-- 
2.17.1

[PATCH v2 2/5] x86/mce/inject: Warn the user on a not set valid bit in MCA_STATUS

[Smita Koralahalli]

MCA handlers check the valid bit in each status register (MCA_STATUS[Val])
and will likely ignore signatures if the valid bit is not set.

Warn the user if the valid bit is not set before doing error injection.

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
v2:
	Added a warning statement instead of setting the valid bit.
---
 arch/x86/kernel/cpu/mce/inject.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 601efd104bb4..a993dc3d0333 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -487,6 +487,9 @@ static void do_inject(void)
 
 	i_mce.tsc = rdtsc_ordered();
 
+	if (!(i_mce.status & MCI_STATUS_VAL))
+		pr_warn("Handlers might ignore signatures with Val=0 in MCA_STATUS\n");
+
 	if (i_mce.misc)
 		i_mce.status |= MCI_STATUS_MISCV;
 
-- 
2.17.1

[PATCH v2 3/5] x86/mce: Use mca_msr_reg() in prepare_msrs()

[Smita Koralahalli]

Replace MCx_{STATUS, ADDR, MISC} macros with mca_msr_reg().

Also, restructure the code to avoid multiple initializations for MCA
registers. SMCA machines define a different set of MSRs for MCA registers
and mca_msr_reg() returns the proper MSR address for SMCA and legacy
processors.

Initialize MCA_MISC and MCA_SYND registers at the end after initializing
MCx_{STATUS, DESTAT} which is further explained in the next patch.

Make mca_msr_reg() exportable in order to be accessible from mce-inject
module.

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
v2:
	Replaced msr_ops -> mca_msr_reg().
---
 arch/x86/kernel/cpu/mce/core.c   |  1 +
 arch/x86/kernel/cpu/mce/inject.c | 27 +++++++++++++--------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 6ed365337a3b..fb4d8ac1cb4f 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -194,6 +194,7 @@ u32 mca_msr_reg(int bank, enum mca_msr reg)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mca_msr_reg);
 
 static void __print_mce(struct mce *m)
 {
diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index a993dc3d0333..40d0bebe0cd2 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -461,22 +461,21 @@ static void prepare_msrs(void *info)
 
 	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);
-			wrmsrl(MSR_AMD64_SMCA_MCx_DEADDR(b), m.addr);
-		} else {
-			wrmsrl(MSR_AMD64_SMCA_MCx_STATUS(b), m.status);
-			wrmsrl(MSR_AMD64_SMCA_MCx_ADDR(b), m.addr);
-		}
+	if (boot_cpu_has(X86_FEATURE_SMCA) &&
+	    m.inject_flags == DFR_INT_INJ) {
+		wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(b), m.status);
+		wrmsrl(MSR_AMD64_SMCA_MCx_DEADDR(b), m.addr);
+		goto out;
+	}
+
+	wrmsrl(mca_msr_reg(b, MCA_STATUS), m.status);
+	wrmsrl(mca_msr_reg(b, MCA_ADDR), m.addr);
 
-		wrmsrl(MSR_AMD64_SMCA_MCx_MISC(b), m.misc);
+out:
+	wrmsrl(mca_msr_reg(b, MCA_MISC), m.misc);
+
+	if (boot_cpu_has(X86_FEATURE_SMCA))
 		wrmsrl(MSR_AMD64_SMCA_MCx_SYND(b), m.synd);
-	} else {
-		wrmsrl(MSR_IA32_MCx_STATUS(b), m.status);
-		wrmsrl(MSR_IA32_MCx_ADDR(b), m.addr);
-		wrmsrl(MSR_IA32_MCx_MISC(b), m.misc);
-	}
 }
 
 static void do_inject(void)
-- 
2.17.1

[PATCH v2 4/5] 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.

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

Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
v2:
	msr_ops -> mca_msr_reg().
	simulation -> injection.
	pr_info() -> pr_err().
	Aligned on "=".
---
 arch/x86/kernel/cpu/mce/inject.c | 39 ++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 40d0bebe0cd2..72d29d26e033 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -454,24 +454,39 @@ 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);
+	u32 status_reg = mca_msr_reg(b, MCA_STATUS);
+	u32 addr_reg   = mca_msr_reg(b, MCA_ADDR);
 
 	if (boot_cpu_has(X86_FEATURE_SMCA) &&
 	    m.inject_flags == DFR_INT_INJ) {
-		wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(b), m.status);
-		wrmsrl(MSR_AMD64_SMCA_MCx_DEADDR(b), m.addr);
-		goto out;
+		status_reg = MSR_AMD64_SMCA_MCx_DESTAT(b);
+		addr_reg   = MSR_AMD64_SMCA_MCx_DEADDR(b);
 	}
 
-	wrmsrl(mca_msr_reg(b, MCA_STATUS), m.status);
-	wrmsrl(mca_msr_reg(b, MCA_ADDR), m.addr);
+	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;
+	}
 
-out:
+	wrmsrl(MSR_IA32_MCG_STATUS, m.mcgstatus);
+	wrmsrl(addr_reg, m.addr);
 	wrmsrl(mca_msr_reg(b, MCA_MISC), m.misc);
 
 	if (boot_cpu_has(X86_FEATURE_SMCA))
@@ -484,6 +499,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();
 
 	if (!(i_mce.status & MCI_STATUS_VAL))
@@ -536,10 +554,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 v2 5/5] 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>
---
v2:
	Added pr_err() along with error code.
---
 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 72d29d26e033..a1c3c1e0425f 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -547,8 +547,11 @@ static void do_inject(void)
 	}
 
 	cpus_read_lock();
-	if (!cpu_online(cpu))
+	if (!cpu_online(cpu)) {
+		pr_err("No online CPUs available for error injection\n");
+		mce_err.err = -ENODEV;
 		goto err;
+	}
 
 	toggle_hw_mce_inject(cpu, true);
 
@@ -621,7 +624,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 v2 2/5] x86/mce/inject: Warn the user on a not set valid bit in MCA_STATUS

[Luck, Tony]

+	if (!(i_mce.status & MCI_STATUS_VAL))
+		pr_warn("Handlers might ignore signatures with Val=0 in MCA_STATUS\n");

I don't think there is any "might" about this. All code paths start by checking for MCI_STATUS_VAL
and skipping if it isn't set.

s/might/will/

-Tony

RE: [PATCH v2 5/5] x86/mce/mce-inject: Return error code to userspace from mce-inject module

[Luck, Tony]

+	if (!cpu_online(cpu)) {
+		pr_err("No online CPUs available for error injection\n");
+		mce_err.err = -ENODEV;

Maybe "Chosen CPU is not online\n"?

-Tony

Re: [PATCH v2 1/5] x86/mce/inject: Check if a bank is unpopulated before error injection

[Borislav Petkov]

On Tue, Oct 19, 2021 at 06:36:37PM -0500, Smita Koralahalli wrote:
> 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" error injection check the value of this register before trying
> to inject the error.
> 
> Do not impose any limitation 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.
> 
> Signed-off-by: Smita Koralahalli <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().
> ---
>  arch/x86/kernel/cpu/mce/inject.c | 39 +++++++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 3 deletions(-)

So I gave it a critical look and did some modifications, see below.
Looking at those IPID MSRs - they're all read-only, which means for !sw
injection, all the module can do is check whether they're 0 - and fail
injection in that case - and do the injection otherwise.

Ok?

---
From: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
Date: Tue, 19 Oct 2021 18:36:37 -0500
Subject: [PATCH] x86/mce/inject: Check if a bank is populated before error
 injection

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 limitation on a sw-type injection (software-only
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
---
 arch/x86/kernel/cpu/mce/inject.c | 42 +++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 0bfc14041bbb..3333ae7886bd 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,33 @@ 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.29.2

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 1/5] x86/mce/inject: Check if a bank is unpopulated before error injection

[Koralahalli Channabasappa, Smita]

On 10/25/21 8:56 AM, Borislav Petkov wrote:
> On Tue, Oct 19, 2021 at 06:36:37PM -0500, Smita Koralahalli wrote:
>> 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" error injection check the value of this register before trying
>> to inject the error.
>>
>> Do not impose any limitation 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.
>>
>> Signed-off-by: Smita Koralahalli <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().
>> ---
>>   arch/x86/kernel/cpu/mce/inject.c | 39 +++++++++++++++++++++++++++++---
>>   1 file changed, 36 insertions(+), 3 deletions(-)
> So I gave it a critical look and did some modifications, see below.
> Looking at those IPID MSRs - they're all read-only, which means for !sw
> injection, all the module can do is check whether they're 0 - and fail
> injection in that case - and do the injection otherwise.
>
> Ok?

Yes, this looks more clearer. Thank you.

>
> ---
> From: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> Date: Tue, 19 Oct 2021 18:36:37 -0500
> Subject: [PATCH] x86/mce/inject: Check if a bank is populated before error
>   injection
>
> 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 limitation on a sw-type injection (software-only
> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.kernel.org%2Fr%2F20211019233641.140275-2-Smita.KoralahalliChannabasappa%40amd.com&amp;data=04%7C01%7CSmita.KoralahalliChannabasappa%40amd.com%7C6da4cbaab660413d27a208d997bf48f1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637707670099990329%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=Jpecn9lg2XxdqhbLpMIxuSh%2BRA3eafMReXmdLI3SkfM%3D&amp;reserved=0
> ---
>   arch/x86/kernel/cpu/mce/inject.c | 42 +++++++++++++++++++++++++++++++-
>   1 file changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
> index 0bfc14041bbb..3333ae7886bd 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,33 @@ 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 */

Re: [PATCH v2 2/5] x86/mce/inject: Warn the user on a not set valid bit in MCA_STATUS

[Borislav Petkov]

On Tue, Oct 19, 2021 at 06:36:38PM -0500, Smita Koralahalli wrote:
> MCA handlers check the valid bit in each status register (MCA_STATUS[Val])
> and will likely ignore signatures if the valid bit is not set.
> 
> Warn the user if the valid bit is not set before doing error injection.
> 
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> ---
> v2:
> 	Added a warning statement instead of setting the valid bit.
> ---
>  arch/x86/kernel/cpu/mce/inject.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
> index 601efd104bb4..a993dc3d0333 100644
> --- a/arch/x86/kernel/cpu/mce/inject.c
> +++ b/arch/x86/kernel/cpu/mce/inject.c
> @@ -487,6 +487,9 @@ static void do_inject(void)
>  
>  	i_mce.tsc = rdtsc_ordered();
>  
> +	if (!(i_mce.status & MCI_STATUS_VAL))
> +		pr_warn("Handlers might ignore signatures with Val=0 in MCA_STATUS\n");
> +
>  	if (i_mce.misc)
>  		i_mce.status |= MCI_STATUS_MISCV;
>  
> -- 

So what's the real reason for this?

You've injected and you didn't get any feedback and were wondering why?

If handlers ignore !VAL MCEs, why don't you simply set it
unconditionally on entry to do_inject()?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 2/5] x86/mce/inject: Warn the user on a not set valid bit in MCA_STATUS

[Koralahalli Channabasappa, Smita]

Hi Boris,

On 10/26/21 5:02 AM, Borislav Petkov wrote:

> On Tue, Oct 19, 2021 at 06:36:38PM -0500, Smita Koralahalli wrote:
>> MCA handlers check the valid bit in each status register (MCA_STATUS[Val])
>> and will likely ignore signatures if the valid bit is not set.
>>
>> Warn the user if the valid bit is not set before doing error injection.
>>
>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
>> ---
>> v2:
>> 	Added a warning statement instead of setting the valid bit.
>> ---
>>   arch/x86/kernel/cpu/mce/inject.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
>> index 601efd104bb4..a993dc3d0333 100644
>> --- a/arch/x86/kernel/cpu/mce/inject.c
>> +++ b/arch/x86/kernel/cpu/mce/inject.c
>> @@ -487,6 +487,9 @@ static void do_inject(void)
>>   
>>   	i_mce.tsc = rdtsc_ordered();
>>   
>> +	if (!(i_mce.status & MCI_STATUS_VAL))
>> +		pr_warn("Handlers might ignore signatures with Val=0 in MCA_STATUS\n");
>> +
>>   	if (i_mce.misc)
>>   		i_mce.status |= MCI_STATUS_MISCV;
>>   
>> -- 
> So what's the real reason for this?
>
> You've injected and you didn't get any feedback and were wondering why?
>
> If handlers ignore !VAL MCEs, why don't you simply set it
> unconditionally on entry to do_inject()?

Like how it was done here?
https://lkml.kernel.org/r/20210915232739.6367-3-Smita.KoralahalliChannabasappa@amd.com

Thanks,

Re: [PATCH v2 2/5] x86/mce/inject: Warn the user on a not set valid bit in MCA_STATUS

[Borislav Petkov]

On Tue, Oct 26, 2021 at 11:58:58AM -0500, Koralahalli Channabasappa, Smita wrote:
> Like how it was done here?
> https://lkml.kernel.org/r/20210915232739.6367-3-Smita.KoralahalliChannabasappa@amd.com

Whoops, sorry about that.

So let's analyze this properly - there are two cases:

1. warn if VAL=0: what does that bring us? The person doing the injection
will simply have to set the valid bit and repeat the injection.

I guess "maybe the user wants to inject with Val not set" doesn't make a
whole lot of sense because nothing will happen - error will get ignored.

So we can do all the warning we want - it will be useless and in some
cases the user might not even see it.

So it sounds to me like setting the valid bit directly makes a lot more
sense.

2. Automatically set VAL=1 to correct any VAL=0 injections.

Yes, we force the VAL bit to 1 and that is not what the user injected
but the user injecting with VAL=0 will get ignored, i.e., it will be
pointless.

So we "help" here and set the valid bit.

Anything else I'm missing?

Sorry again for being back'n'forth on this.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 2/5] x86/mce/inject: Warn the user on a not set valid bit in MCA_STATUS

[Koralahalli Channabasappa, Smita]

On 10/26/21 12:15 PM, Borislav Petkov wrote:

> On Tue, Oct 26, 2021 at 11:58:58AM -0500, Koralahalli Channabasappa, Smita wrote:
> Whoops, sorry about that.
>
> So let's analyze this properly - there are two cases:
>
> 1. warn if VAL=0: what does that bring us? The person doing the injection
> will simply have to set the valid bit and repeat the injection.
>
> I guess "maybe the user wants to inject with Val not set" doesn't make a
> whole lot of sense because nothing will happen - error will get ignored.
>
> So we can do all the warning we want - it will be useless and in some
> cases the user might not even see it.
>
> So it sounds to me like setting the valid bit directly makes a lot more
> sense.
>
> 2. Automatically set VAL=1 to correct any VAL=0 injections.
>
> Yes, we force the VAL bit to 1 and that is not what the user injected
> but the user injecting with VAL=0 will get ignored, i.e., it will be
> pointless.
>
> So we "help" here and set the valid bit.
>
> Anything else I'm missing?
>
> Sorry again for being back'n'forth on this.

Right. We are correcting VAL=0 injections made by the user by setting
valid bit unconditionally.

Not a problem. I could have broken this down in the comments before
coming up with this patch.

I will make the necessary changes and set the valid bit in the next
revision of the patch series.

Thanks,
Smita.

Re: [PATCH v2 2/5] x86/mce/inject: Warn the user on a not set valid bit in MCA_STATUS

[Borislav Petkov]

On Tue, Oct 26, 2021 at 01:53:32PM -0500, Koralahalli Channabasappa, Smita wrote:
> Not a problem. I could have broken this down in the comments before
> coming up with this patch.

No worries - sometimes proposing the wrong thing even helps in deciding
faster what makes sense and what not. :-)

> I will make the necessary changes and set the valid bit in the next
> revision of the patch series.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 3/5] x86/mce: Use mca_msr_reg() in prepare_msrs()

[Borislav Petkov]

On Tue, Oct 19, 2021 at 06:36:39PM -0500, Smita Koralahalli wrote:
> Replace MCx_{STATUS, ADDR, MISC} macros with mca_msr_reg().

And this is where your commit message and patch should end. It is a bad
idea to do textual replacements *and* functional changes in a single
patch: it is hard to review and debug if there are possible issues. So
you do the textual replacements in the first one and then the functional
changes in subsequent patches.

> Also, restructure the code to avoid multiple initializations for MCA
> registers.

What multiple initializations?

> SMCA machines define a different set of MSRs for MCA registers
> and mca_msr_reg() returns the proper MSR address for SMCA and legacy
> processors.
> 
> Initialize MCA_MISC and MCA_SYND registers at the end after initializing
> MCx_{STATUS, DESTAT} which is further explained in the next patch.

And this should be *in* the next patch.

Also, there's no concept of "next patch" when you do git log on the
upstream tree and use different sorting etc. So a patch should be
self-contained and do one change only.

There's very good documentation in Documentation/process/, expecially
Documentation/process/submitting-patches.rst, which explains how a patch
should look like.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 3/5] x86/mce: Use mca_msr_reg() in prepare_msrs()

[Koralahalli Channabasappa, Smita]

On 10/27/21 6:41 AM, Borislav Petkov wrote:
> On Tue, Oct 19, 2021 at 06:36:39PM -0500, Smita Koralahalli wrote:
>> Replace MCx_{STATUS, ADDR, MISC} macros with mca_msr_reg().
> And this is where your commit message and patch should end. It is a bad
> idea to do textual replacements *and* functional changes in a single
> patch: it is hard to review and debug if there are possible issues. So
> you do the textual replacements in the first one and then the functional
> changes in subsequent patches.

Okay I will break this down and send v3 as suggested.

>> Also, restructure the code to avoid multiple initializations for MCA
>> registers.
> What multiple initializations?

Multiple initialization here I mean: Initializing the MCA registers twice.
Prior to mca_msr_reg() replacement, the MCA registers were initialized
separately for SMCA and legacy processors. However, this is not required
after replacing with mca_msr_reg() as it does the job of returning the
proper MSR addresses.

Probably, my wording is more confusing here. Does this seem better?
"Do not initialize MCx_{STATUS, ADDR, MISC} separately for SMCA and
legacy processors as mca_msr_reg() returns the appropriate MSR addresses
for both."

I will split this into second patch.

>> SMCA machines define a different set of MSRs for MCA registers
>> and mca_msr_reg() returns the proper MSR address for SMCA and legacy
>> processors.
>>
>> Initialize MCA_MISC and MCA_SYND registers at the end after initializing
>> MCx_{STATUS, DESTAT} which is further explained in the next patch.
> And this should be *in* the next patch.

Okay, basically break it down into three. One for replacing, one for
cleaning up the multiple initialization of MCA registers and the last for
moving MCA_MISC and MCA_SYND to the end.

Will do it as suggested..

>
> Also, there's no concept of "next patch" when you do git log on the
> upstream tree and use different sorting etc. So a patch should be
> self-contained and do one change only.
>
> There's very good documentation in Documentation/process/, expecially
> Documentation/process/submitting-patches.rst, which explains how a patch
> should look like.
>
> Thx.

Ok will take a look at this again to correct my mistakes. Thanks for the
inputs.

Thanks,
Smita

Re: [PATCH v2 3/5] x86/mce: Use mca_msr_reg() in prepare_msrs()

[Borislav Petkov]

On Wed, Oct 27, 2021 at 03:19:51PM -0500, Koralahalli Channabasappa, Smita wrote:
> Multiple initialization here I mean: Initializing the MCA registers twice.
> Prior to mca_msr_reg() replacement, the MCA registers were initialized
> separately for SMCA and legacy processors. However, this is not required
> after replacing with mca_msr_reg() as it does the job of returning the
> proper MSR addresses.

You mean, there was a simple if-else statement

	if (SMCA)

		prepare MSRs

	else

		prepare MSRs for !SMCA

which did the init for each type of system in one go.

But frankly, your change doesn't make it more readable but less - you
have a goto label now and another SMCA feature check at the end. Vs a
simple if-else which is trivial to read.

So I don't see any advantage in this change.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 3/5] x86/mce: Use mca_msr_reg() in prepare_msrs()

[Koralahalli Channabasappa, Smita]

On 10/28/21 3:53 AM, Borislav Petkov wrote:

> On Wed, Oct 27, 2021 at 03:19:51PM -0500, Koralahalli Channabasappa, Smita wrote:
>> Multiple initialization here I mean: Initializing the MCA registers twice.
>> Prior to mca_msr_reg() replacement, the MCA registers were initialized
>> separately for SMCA and legacy processors. However, this is not required
>> after replacing with mca_msr_reg() as it does the job of returning the
>> proper MSR addresses.
> You mean, there was a simple if-else statement
>
> 	if (SMCA)
>
> 		prepare MSRs
>
> 	else
>
> 		prepare MSRs for !SMCA
>
> which did the init for each type of system in one go.
>
> But frankly, your change doesn't make it more readable but less - you
> have a goto label now and another SMCA feature check at the end. Vs a
> simple if-else which is trivial to read.
>
> So I don't see any advantage in this change.

Okay. I will rework and remove this patch in my next series.

Thanks,

>