[PATCH 0/2] AMD Zen-based IF Unit Poison Quirk

[PATCH 0/2] AMD Zen-based IF Unit Poison Quirk

[Yazen Ghannam]

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

The Instruction Fetch unit on AMD Zen-based systems has a
microarchitectural quirk in which RIPV is not set on poison consumption
errors. However, the error is guaranteed to be delivered before a
context switch. Therefore, the CS register can be considered valid.

Patch 1 handles this behavior. Patch 2 addresses fallout from the change
in behavior in Patch 1.

Both patches Cc: stable because there have been bug reports that seem to
exhibit this behavior. There are no Fixes tags, because I don't think we
can point to specific commits that introduced this issue.

Thanks,
Yazen

Yazen Ghannam (2):
  x86/MCE: Always save CS register on AMD Zen IF errors
  x86/MCE: Don't call kill_me_now() directly

 arch/x86/kernel/cpu/mce/amd.c      | 17 +++++++++++++++++
 arch/x86/kernel/cpu/mce/core.c     | 12 ++++++++----
 arch/x86/kernel/cpu/mce/internal.h |  2 ++
 3 files changed, 27 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/2] x86/MCE: Always save CS register on AMD Zen IF errors

[Yazen Ghannam]

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

The Instruction Fetch (IF) units on AMD Zen-based systems do not
guarantee a synchronous #MC is delivered. Therefore, MCG_STATUS[EIPV|RIPV]
will not be set. However, the microarchitecture does guarantee that the
exception is delivered within the same context. In other words, the
exact rIP is not known, but the context is known to not have changed.

There is no architecturally-defined method to determine this behavior.

The Code Segment (CS) register is always valid on AMD Zen-based IF units
regardless of the value of MCG_STATUS[EIPV|RIPV].

Add a quirk for all current Zen-based systems to save the CS register
for the IF banks.

This is needed to properly determine the context of the error.
Otherwise, the severity grading function will assume the context is
IN_KERNEL due to the m->cs value being 0 (the initialized value). This
leads to unnecessary kernel panics on data poison errors due to the
kernel believing the poison consumption occurred in kernel context.

Cc: <stable@vger.kernel.org>
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/cpu/mce/amd.c      | 17 +++++++++++++++++
 arch/x86/kernel/cpu/mce/core.c     |  7 +++++++
 arch/x86/kernel/cpu/mce/internal.h |  2 ++
 3 files changed, 26 insertions(+)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index e486f96b3cb3..141dcdd857b5 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -180,6 +180,23 @@ static struct smca_hwid smca_hwid_mcatypes[] = {
 struct smca_bank smca_banks[MAX_NR_BANKS];
 EXPORT_SYMBOL_GPL(smca_banks);
 
+/*
+ * Zen-based Instruction Fetch Units set EIPV=RIPV=0 on poison consumption
+ * errors (XEC = 12). However, the context is still valid, so save the CS
+ * register for later use.
+ */
+void quirk_zen_ifu(int bank, struct mce *m, struct pt_regs *regs)
+{
+	if (smca_get_bank_type(bank) != SMCA_IF)
+		return;
+	if ((m->mcgstatus & (MCG_STATUS_EIPV|MCG_STATUS_RIPV)) != 0)
+		return;
+	if (((m->status >> 16) & 0x1F) != 12)
+		return;
+
+	m->cs = regs->cs;
+}
+
 /*
  * In SMCA enabled processors, we can have multiple banks for a given IP type.
  * So to define a unique name for each bank, we use a temp c-string to append
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index bf7fe87a7e88..308fb644b94a 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1754,6 +1754,13 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
 		if (c->x86 == 0x15 && c->x86_model <= 0xf)
 			mce_flags.overflow_recov = 1;
 
+		if (c->x86 == 0x17 || c->x86 == 0x19)
+			quirk_no_way_out = quirk_zen_ifu;
+	}
+
+	if (c->x86_vendor == X86_VENDOR_HYGON) {
+		if (c->x86 == 0x18)
+			quirk_no_way_out = quirk_zen_ifu;
 	}
 
 	if (c->x86_vendor == X86_VENDOR_INTEL) {
diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
index 88dcc79cfb07..656d5d6c9783 100644
--- a/arch/x86/kernel/cpu/mce/internal.h
+++ b/arch/x86/kernel/cpu/mce/internal.h
@@ -181,8 +181,10 @@ extern struct mca_msr_regs msr_ops;
 extern bool filter_mce(struct mce *m);
 
 #ifdef CONFIG_X86_MCE_AMD
+extern void quirk_zen_ifu(int bank, struct mce *m, struct pt_regs *regs);
 extern bool amd_filter_mce(struct mce *m);
 #else
+#define quirk_zen_ifu							NULL
 static inline bool amd_filter_mce(struct mce *m)			{ return false; };
 #endif
 
-- 
2.25.1

[PATCH 2/2] x86/MCE: Don't call kill_me_now() directly

[Yazen Ghannam]

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

Always call kill_me_maybe() in order to attempt memory recovery. This
ensures that any memory associated with the error is properly marked as
poison.

This is needed for errors that occur on memory, but that do not have
MCG_STATUS[RIPV] set. One example is data poison consumption through the
instruction fetch units on AMD Zen-based systems.

The MF_MUST_KILL flag is passed to memory_failure() when
MCG_STATUS[RIPV] is not set. So the associated process will still be
killed.

Cc: <stable@vger.kernel.org>
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/cpu/mce/core.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 308fb644b94a..9040d45ed997 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1285,10 +1285,7 @@ static void queue_task_work(struct mce *m, int kill_current_task)
 	current->mce_ripv = !!(m->mcgstatus & MCG_STATUS_RIPV);
 	current->mce_whole_page = whole_page(m);
 
-	if (kill_current_task)
-		current->mce_kill_me.func = kill_me_now;
-	else
-		current->mce_kill_me.func = kill_me_maybe;
+	current->mce_kill_me.func = kill_me_maybe;
 
 	task_work_add(current, &current->mce_kill_me, TWA_RESUME);
 }
-- 
2.25.1

Re: [PATCH 2/2] x86/MCE: Don't call kill_me_now() directly

[Luck, Tony]

On Tue, May 04, 2021 at 05:47:12PM +0000, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> Always call kill_me_maybe() in order to attempt memory recovery. This
> ensures that any memory associated with the error is properly marked as
> poison.
> 
> This is needed for errors that occur on memory, but that do not have
> MCG_STATUS[RIPV] set. One example is data poison consumption through the
> instruction fetch units on AMD Zen-based systems.
> 
> The MF_MUST_KILL flag is passed to memory_failure() when
> MCG_STATUS[RIPV] is not set. So the associated process will still be
> killed.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  arch/x86/kernel/cpu/mce/core.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 308fb644b94a..9040d45ed997 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1285,10 +1285,7 @@ static void queue_task_work(struct mce *m, int kill_current_task)
>  	current->mce_ripv = !!(m->mcgstatus & MCG_STATUS_RIPV);
>  	current->mce_whole_page = whole_page(m);
>  
> -	if (kill_current_task)
> -		current->mce_kill_me.func = kill_me_now;
> -	else
> -		current->mce_kill_me.func = kill_me_maybe;
> +	current->mce_kill_me.func = kill_me_maybe;
>  
>  	task_work_add(current, &current->mce_kill_me, TWA_RESUME);
>  }

Could we just get rid of kill_me_now() at the same time? It's only
one line, and with this change only called in one place (from
kill_me_maybe()) ... just put the force_sig(SIGBUS); inline?

-Tony

Re: [PATCH 2/2] x86/MCE: Don't call kill_me_now() directly

[Yazen Ghannam]

On Tue, May 04, 2021 at 11:07:34AM -0700, Luck, Tony wrote:
> On Tue, May 04, 2021 at 05:47:12PM +0000, Yazen Ghannam wrote:
...
> > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> > index 308fb644b94a..9040d45ed997 100644
> > --- a/arch/x86/kernel/cpu/mce/core.c
> > +++ b/arch/x86/kernel/cpu/mce/core.c
> > @@ -1285,10 +1285,7 @@ static void queue_task_work(struct mce *m, int kill_current_task)
> >  	current->mce_ripv = !!(m->mcgstatus & MCG_STATUS_RIPV);
> >  	current->mce_whole_page = whole_page(m);
> >  
> > -	if (kill_current_task)
> > -		current->mce_kill_me.func = kill_me_now;
> > -	else
> > -		current->mce_kill_me.func = kill_me_maybe;
> > +	current->mce_kill_me.func = kill_me_maybe;
> >  
> >  	task_work_add(current, &current->mce_kill_me, TWA_RESUME);
> >  }
> 
> Could we just get rid of kill_me_now() at the same time? It's only
> one line, and with this change only called in one place (from
> kill_me_maybe()) ... just put the force_sig(SIGBUS); inline?
>

Okay, will do.

Thanks,
Yazen