As specified on Errata 1474:

"A core will fail to exit CC6 after about 1044 days after the last
system reset. The time of failure may vary depending on the spread
spectrum and REFCLK frequency."

Detect when running on AMD Zen2 (family 17h models 30-3fh, 60-6fh or
70-7fh) and setup a timer to prevent entering C6 after 1000 days of
uptime.  Take into account the TSC value at boot in order to account
for any time elapsed before Xen has been booted.  Worst case we end
up disabling C6 before strictly necessary, but that would still be
safe, and it's better than not taking the TSC value into account and
hanging.

Disable C6 by updating the MSR listed in the revision guide, this
avoids applying workarounds in the CPU idle drivers, as the processor
won't be allowed to enter C6 by the hardware itself.

Print a message once C6 is disabled in order to let the user know.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
The current Revision Guide for Fam17h model 60-6Fh (Lucienne and
Renoir) hasn't been updated to reflect the MSR workaround, but the PPR
for those models lists the MSR and the bits as having the expected
meaning, so I assume it's safe to apply the same workaround there.

For all accounts this seems to affect all Zen2 models, and hence the
workaround should be the same.  Might also affect Hygon, albeit I
think Hygon is strictly limited to Zen1.
---
Changes since v1:
 - Apply the workaround listed by AMD: toggle some MSR bits.
 - Do not apply the workaround if virtualized.
 - Check for STIBP feature instead of listing specific models.
 - Implement the DAYS macro based on SECONDS.
---
 xen/arch/x86/cpu/amd.c               | 70 ++++++++++++++++++++++++++++
 xen/arch/x86/include/asm/msr-index.h |  5 ++
 xen/include/xen/time.h               |  1 +
 3 files changed, 76 insertions(+)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 0eaef82e5145..bdf45f8387e8 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -51,6 +51,8 @@ bool __read_mostly amd_acpi_c1e_quirk;
 bool __ro_after_init amd_legacy_ssbd;
 bool __initdata amd_virt_spec_ctrl;
 
+static bool __read_mostly c6_disabled;
+
 static inline int rdmsr_amd_safe(unsigned int msr, unsigned int *lo,
 				 unsigned int *hi)
 {
@@ -905,6 +907,31 @@ void __init detect_zen2_null_seg_behaviour(void)
 
 }
 
+static void cf_check disable_c6(void *arg)
+{
+	uint64_t val;
+
+	if (!c6_disabled) {
+		printk(XENLOG_WARNING
+    "Disabling C6 after 1000 days apparent uptime due to AMD errata 1474\n");
+		c6_disabled = true;
+		smp_call_function(disable_c6, NULL, 0);
+	}
+
+	/* Update the MSR to disable C6, done on all threads. */
+	if (rdmsr_safe(MSR_AMD_CSTATE_CFG, val)) {
+error:
+		printk_once(XENLOG_ERR
+		"Unable to disable C6 on AMD system affected by errata 1474\n"
+		"Reboot recommended, otherwise system might hang\n");
+		return;
+	}
+	val &= ~(CSTATE_CFG_CCR0_CC6EN | CSTATE_CFG_CCR1_CC6EN |
+	         CSTATE_CFG_CCR2_CC6EN);
+	if (wrmsr_safe(MSR_AMD_CSTATE_CFG, val))
+		goto error;
+}
+
 static void cf_check init_amd(struct cpuinfo_x86 *c)
 {
 	u32 l, h;
@@ -1171,6 +1198,9 @@ static void cf_check init_amd(struct cpuinfo_x86 *c)
 	if ((smp_processor_id() == 1) && !cpu_has(c, X86_FEATURE_ITSC))
 		disable_c1_ramping();
 
+	if (c6_disabled)
+		disable_c6(NULL);
+
 	check_syscfg_dram_mod_en();
 
 	amd_log_freq(c);
@@ -1180,3 +1210,43 @@ const struct cpu_dev amd_cpu_dev = {
 	.c_early_init	= early_init_amd,
 	.c_init		= init_amd,
 };
+
+static int __init cf_check c6_errata_check(void)
+{
+	/*
+	 * Errata #1474: A Core May Hang After About 1044 Days
+	 * Set up a timer to disable C6 after 1000 days uptime.
+	 */
+	s_time_t delta;
+
+	/*
+	 * Apply to all Zen2 models.  According to AMD revision guides at least
+	 * Rome, Castle Peak, Renoir, Lucienne and Matisse are affected.
+	 */
+	if (cpu_has_hypervisor || !boot_cpu_has(X86_FEATURE_AMD_STIBP))
+		return 0;
+
+	/*
+	 * Deduct current TSC value, this would be relevant if kexec'ed for
+	 * example.  Might not be accurate, but worst case we end up disabling
+	 * C6 before strictly required, which would still be safe.
+	 *
+	 * NB: all affected models (Zen2) have invariant TSC and TSC adjust
+	 * MSR, so early_time_init() will have already cleared any TSC offset.
+	 */
+	delta = DAYS(1000) - tsc_ticks2ns(rdtsc());
+	if (delta > 0) {
+		static struct timer errata_c6;
+
+		init_timer(&errata_c6, disable_c6, NULL, 0);
+		set_timer(&errata_c6, NOW() + delta);
+	} else
+		disable_c6(NULL);
+
+	return 0;
+}
+/*
+ * Must be executed after early_time_init() for tsc_ticks2ns() to have been
+ * calibrated.  That prevents us doing the check in init_amd().
+ */
+presmp_initcall(c6_errata_check);
diff --git a/xen/arch/x86/include/asm/msr-index.h b/xen/arch/x86/include/asm/msr-index.h
index 2749e433d2a7..5df090fba791 100644
--- a/xen/arch/x86/include/asm/msr-index.h
+++ b/xen/arch/x86/include/asm/msr-index.h
@@ -211,6 +211,11 @@
 
 #define MSR_VIRT_SPEC_CTRL                  0xc001011f /* Layout matches MSR_SPEC_CTRL */
 
+#define MSR_AMD_CSTATE_CFG                  0xc0010296
+#define  CSTATE_CFG_CCR0_CC6EN              (_AC(1, ULL) <<  6)
+#define  CSTATE_CFG_CCR1_CC6EN              (_AC(1, ULL) << 14)
+#define  CSTATE_CFG_CCR2_CC6EN              (_AC(1, ULL) << 22)
+
 /*
  * Legacy MSR constants in need of cleanup.  No new MSRs below this comment.
  */
diff --git a/xen/include/xen/time.h b/xen/include/xen/time.h
index b7427460dd13..9ceaec541f4d 100644
--- a/xen/include/xen/time.h
+++ b/xen/include/xen/time.h
@@ -53,6 +53,7 @@ struct tm wallclock_time(uint64_t *ns);
 
 #define SYSTEM_TIME_HZ  1000000000ULL
 #define NOW()           ((s_time_t)get_s_time())
+#define DAYS(_d)        SECONDS((_d) * 86400ULL)
 #define SECONDS(_s)     ((s_time_t)((_s)  * 1000000000ULL))
 #define MILLISECS(_ms)  ((s_time_t)((_ms) * 1000000ULL))
 #define MICROSECS(_us)  ((s_time_t)((_us) * 1000ULL))
-- 
2.41.0

On Fri, Jun 30, 2023 at 03:18:20PM +0200, Roger Pau Monne wrote:
> As specified on Errata 1474:
> 
> "A core will fail to exit CC6 after about 1044 days after the last
> system reset. The time of failure may vary depending on the spread
> spectrum and REFCLK frequency."
> 
> Detect when running on AMD Zen2 (family 17h models 30-3fh, 60-6fh or
> 70-7fh) and setup a timer to prevent entering C6 after 1000 days of
> uptime.  Take into account the TSC value at boot in order to account
> for any time elapsed before Xen has been booted.  Worst case we end
> up disabling C6 before strictly necessary, but that would still be
> safe, and it's better than not taking the TSC value into account and
> hanging.
> 
> Disable C6 by updating the MSR listed in the revision guide, this
> avoids applying workarounds in the CPU idle drivers, as the processor
> won't be allowed to enter C6 by the hardware itself.
> 
> Print a message once C6 is disabled in order to let the user know.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> The current Revision Guide for Fam17h model 60-6Fh (Lucienne and
> Renoir) hasn't been updated to reflect the MSR workaround, but the PPR
> for those models lists the MSR and the bits as having the expected
> meaning, so I assume it's safe to apply the same workaround there.
> 
> For all accounts this seems to affect all Zen2 models, and hence the
> workaround should be the same.  Might also affect Hygon, albeit I
> think Hygon is strictly limited to Zen1.
> ---
> Changes since v1:
>  - Apply the workaround listed by AMD: toggle some MSR bits.
>  - Do not apply the workaround if virtualized.
>  - Check for STIBP feature instead of listing specific models.
>  - Implement the DAYS macro based on SECONDS.
> ---
>  xen/arch/x86/cpu/amd.c               | 70 ++++++++++++++++++++++++++++
>  xen/arch/x86/include/asm/msr-index.h |  5 ++
>  xen/include/xen/time.h               |  1 +
>  3 files changed, 76 insertions(+)
> 
> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> index 0eaef82e5145..bdf45f8387e8 100644
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -51,6 +51,8 @@ bool __read_mostly amd_acpi_c1e_quirk;
>  bool __ro_after_init amd_legacy_ssbd;
>  bool __initdata amd_virt_spec_ctrl;
>  
> +static bool __read_mostly c6_disabled;
> +
>  static inline int rdmsr_amd_safe(unsigned int msr, unsigned int *lo,
>  				 unsigned int *hi)
>  {
> @@ -905,6 +907,31 @@ void __init detect_zen2_null_seg_behaviour(void)
>  
>  }
>  
> +static void cf_check disable_c6(void *arg)
> +{
> +	uint64_t val;
> +
> +	if (!c6_disabled) {
> +		printk(XENLOG_WARNING
> +    "Disabling C6 after 1000 days apparent uptime due to AMD errata 1474\n");
> +		c6_disabled = true;
> +		smp_call_function(disable_c6, NULL, 0);

I've realized this is racy with CPU hotplug, so I will need to inhibit
CPU hotplug around the call to smp_call_function() in order to prevent
CPUs being hotplugged and not seeing c6_disabled while set and also
not being set in cpu_online_map when the call to smp_call_function
happens.

Thanks, Roger.

On 30/06/2023 2:18 pm, Roger Pau Monne wrote:
> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> index 0eaef82e5145..bdf45f8387e8 100644
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -51,6 +51,8 @@ bool __read_mostly amd_acpi_c1e_quirk;
>  bool __ro_after_init amd_legacy_ssbd;
>  bool __initdata amd_virt_spec_ctrl;
>  
> +static bool __read_mostly c6_disabled;
> +
>  static inline int rdmsr_amd_safe(unsigned int msr, unsigned int *lo,
>  				 unsigned int *hi)
>  {
> @@ -905,6 +907,31 @@ void __init detect_zen2_null_seg_behaviour(void)
>  
>  }
>  
> +static void cf_check disable_c6(void *arg)
> +{
> +	uint64_t val;
> +
> +	if (!c6_disabled) {
> +		printk(XENLOG_WARNING
> +    "Disabling C6 after 1000 days apparent uptime due to AMD errata 1474\n");
> +		c6_disabled = true;
> +		smp_call_function(disable_c6, NULL, 0);
> +	}
> +
> +	/* Update the MSR to disable C6, done on all threads. */
> +	if (rdmsr_safe(MSR_AMD_CSTATE_CFG, val)) {
> +error:
> +		printk_once(XENLOG_ERR
> +		"Unable to disable C6 on AMD system affected by errata 1474\n"
> +		"Reboot recommended, otherwise system might hang\n");
> +		return;
> +	}
> +	val &= ~(CSTATE_CFG_CCR0_CC6EN | CSTATE_CFG_CCR1_CC6EN |
> +	         CSTATE_CFG_CCR2_CC6EN);
> +	if (wrmsr_safe(MSR_AMD_CSTATE_CFG, val))
> +		goto error;

These MSRs don't fault, and we already excluded hypervisors previously,
so the safe() doesn't really help.  The only way you'd spot a failure is
by reading back and noticing that the update didn't take effect.

Independently, this really really makes me want to dust off my
msr_{set,clear}_bits() helpers to reduce the boilerplate required for
logic like this.

> +}
> +
>  static void cf_check init_amd(struct cpuinfo_x86 *c)
>  {
>  	u32 l, h;
> @@ -1171,6 +1198,9 @@ static void cf_check init_amd(struct cpuinfo_x86 *c)
>  	if ((smp_processor_id() == 1) && !cpu_has(c, X86_FEATURE_ITSC))
>  		disable_c1_ramping();
>  
> +	if (c6_disabled)
> +		disable_c6(NULL);
> +
>  	check_syscfg_dram_mod_en();
>  
>  	amd_log_freq(c);
> @@ -1180,3 +1210,43 @@ const struct cpu_dev amd_cpu_dev = {
>  	.c_early_init	= early_init_amd,
>  	.c_init		= init_amd,
>  };
> +
> +static int __init cf_check c6_errata_check(void)
> +{
> +	/*
> +	 * Errata #1474: A Core May Hang After About 1044 Days
> +	 * Set up a timer to disable C6 after 1000 days uptime.
> +	 */
> +	s_time_t delta;
> +
> +	/*
> +	 * Apply to all Zen2 models.  According to AMD revision guides at least
> +	 * Rome, Castle Peak, Renoir, Lucienne and Matisse are affected.

You probably want to replicate the spectral chicken comment about how
identifying Zen2 is actually very hard.

That said, I don't see anything which limits this logic to Fam17h.

> +	 */
> +	if (cpu_has_hypervisor || !boot_cpu_has(X86_FEATURE_AMD_STIBP))
> +		return 0;
> +
> +	/*
> +	 * Deduct current TSC value, this would be relevant if kexec'ed for
> +	 * example.  Might not be accurate, but worst case we end up disabling
> +	 * C6 before strictly required, which would still be safe.
> +	 *
> +	 * NB: all affected models (Zen2) have invariant TSC and TSC adjust
> +	 * MSR, so early_time_init() will have already cleared any TSC offset.
> +	 */
> +	delta = DAYS(1000) - tsc_ticks2ns(rdtsc());
> +	if (delta > 0) {
> +		static struct timer errata_c6;
> +
> +		init_timer(&errata_c6, disable_c6, NULL, 0);
> +		set_timer(&errata_c6, NOW() + delta);
> +	} else
> +		disable_c6(NULL);

I doubt the smp_call_function() is going to be happy at presmp time.

Do we really care if we're already beyond the timeout?

> +
> +	return 0;
> +}
> +/*
> + * Must be executed after early_time_init() for tsc_ticks2ns() to have been
> + * calibrated.  That prevents us doing the check in init_amd().
> + */
> +presmp_initcall(c6_errata_check);
> diff --git a/xen/arch/x86/include/asm/msr-index.h b/xen/arch/x86/include/asm/msr-index.h
> index 2749e433d2a7..5df090fba791 100644
> --- a/xen/arch/x86/include/asm/msr-index.h
> +++ b/xen/arch/x86/include/asm/msr-index.h
> @@ -211,6 +211,11 @@
>  
>  #define MSR_VIRT_SPEC_CTRL                  0xc001011f /* Layout matches MSR_SPEC_CTRL */
>  
> +#define MSR_AMD_CSTATE_CFG                  0xc0010296
> +#define  CSTATE_CFG_CCR0_CC6EN              (_AC(1, ULL) <<  6)
> +#define  CSTATE_CFG_CCR1_CC6EN              (_AC(1, ULL) << 14)
> +#define  CSTATE_CFG_CCR2_CC6EN              (_AC(1, ULL) << 22)

While MSR_AMD_CSTATE_CFG is liable to stay stable, the CC6EN bits are
uarch specific and not applicable to others.

I'd suggest keeping them local to c6_errata_check(), which probably
ought to gain a zen2 somewhere in the name.

They definitely can't say in a global header with names that don't tie
them to Zen2 specifically.

~Andrew

On Fri, Jun 30, 2023 at 06:10:10PM +0100, Andrew Cooper wrote:
> On 30/06/2023 2:18 pm, Roger Pau Monne wrote:
> > diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> > index 0eaef82e5145..bdf45f8387e8 100644
> > --- a/xen/arch/x86/cpu/amd.c
> > +++ b/xen/arch/x86/cpu/amd.c
> > @@ -51,6 +51,8 @@ bool __read_mostly amd_acpi_c1e_quirk;
> >  bool __ro_after_init amd_legacy_ssbd;
> >  bool __initdata amd_virt_spec_ctrl;
> >  
> > +static bool __read_mostly c6_disabled;
> > +
> >  static inline int rdmsr_amd_safe(unsigned int msr, unsigned int *lo,
> >  				 unsigned int *hi)
> >  {
> > @@ -905,6 +907,31 @@ void __init detect_zen2_null_seg_behaviour(void)
> >  
> >  }
> >  
> > +static void cf_check disable_c6(void *arg)
> > +{
> > +	uint64_t val;
> > +
> > +	if (!c6_disabled) {
> > +		printk(XENLOG_WARNING
> > +    "Disabling C6 after 1000 days apparent uptime due to AMD errata 1474\n");
> > +		c6_disabled = true;
> > +		smp_call_function(disable_c6, NULL, 0);
> > +	}
> > +
> > +	/* Update the MSR to disable C6, done on all threads. */
> > +	if (rdmsr_safe(MSR_AMD_CSTATE_CFG, val)) {
> > +error:
> > +		printk_once(XENLOG_ERR
> > +		"Unable to disable C6 on AMD system affected by errata 1474\n"
> > +		"Reboot recommended, otherwise system might hang\n");
> > +		return;
> > +	}
> > +	val &= ~(CSTATE_CFG_CCR0_CC6EN | CSTATE_CFG_CCR1_CC6EN |
> > +	         CSTATE_CFG_CCR2_CC6EN);
> > +	if (wrmsr_safe(MSR_AMD_CSTATE_CFG, val))
> > +		goto error;
> 
> These MSRs don't fault, and we already excluded hypervisors previously,
> so the safe() doesn't really help.  The only way you'd spot a failure is
> by reading back and noticing that the update didn't take effect.

I was worried about people explicitly clearing the hypervisor flag in
CPUID and then Xen crashing on boot, so better safe than sorry.  I
don't see much value in crashing here if the MSR is somehow
unavailable.

> Independently, this really really makes me want to dust off my
> msr_{set,clear}_bits() helpers to reduce the boilerplate required for
> logic like this.
> 
> > +}
> > +
> >  static void cf_check init_amd(struct cpuinfo_x86 *c)
> >  {
> >  	u32 l, h;
> > @@ -1171,6 +1198,9 @@ static void cf_check init_amd(struct cpuinfo_x86 *c)
> >  	if ((smp_processor_id() == 1) && !cpu_has(c, X86_FEATURE_ITSC))
> >  		disable_c1_ramping();
> >  
> > +	if (c6_disabled)
> > +		disable_c6(NULL);
> > +
> >  	check_syscfg_dram_mod_en();
> >  
> >  	amd_log_freq(c);
> > @@ -1180,3 +1210,43 @@ const struct cpu_dev amd_cpu_dev = {
> >  	.c_early_init	= early_init_amd,
> >  	.c_init		= init_amd,
> >  };
> > +
> > +static int __init cf_check c6_errata_check(void)
> > +{
> > +	/*
> > +	 * Errata #1474: A Core May Hang After About 1044 Days
> > +	 * Set up a timer to disable C6 after 1000 days uptime.
> > +	 */
> > +	s_time_t delta;
> > +
> > +	/*
> > +	 * Apply to all Zen2 models.  According to AMD revision guides at least
> > +	 * Rome, Castle Peak, Renoir, Lucienne and Matisse are affected.
> 
> You probably want to replicate the spectral chicken comment about how
> identifying Zen2 is actually very hard.
> 
> That said, I don't see anything which limits this logic to Fam17h.

Oh, right, will add the missing fam17h check.

> > +	 */
> > +	if (cpu_has_hypervisor || !boot_cpu_has(X86_FEATURE_AMD_STIBP))
> > +		return 0;
> > +
> > +	/*
> > +	 * Deduct current TSC value, this would be relevant if kexec'ed for
> > +	 * example.  Might not be accurate, but worst case we end up disabling
> > +	 * C6 before strictly required, which would still be safe.
> > +	 *
> > +	 * NB: all affected models (Zen2) have invariant TSC and TSC adjust
> > +	 * MSR, so early_time_init() will have already cleared any TSC offset.
> > +	 */
> > +	delta = DAYS(1000) - tsc_ticks2ns(rdtsc());
> > +	if (delta > 0) {
> > +		static struct timer errata_c6;
> > +
> > +		init_timer(&errata_c6, disable_c6, NULL, 0);
> > +		set_timer(&errata_c6, NOW() + delta);
> > +	} else
> > +		disable_c6(NULL);
> 
> I doubt the smp_call_function() is going to be happy at presmp time.

I did take a look and forced it by setting delta = 0,  AFAICT it works
fine, cpu_online_map will be zeroed, so on_selected_cpus() is just a
noop.  Interrupts are enabled by that point, so I don't see a problem
with this approach.

> Do we really care if we're already beyond the timeout?

I would expect that C6 will already be disabled in that case, but I
don't see an issue with Xen also setting those bits.

> > +
> > +	return 0;
> > +}
> > +/*
> > + * Must be executed after early_time_init() for tsc_ticks2ns() to have been
> > + * calibrated.  That prevents us doing the check in init_amd().
> > + */
> > +presmp_initcall(c6_errata_check);
> > diff --git a/xen/arch/x86/include/asm/msr-index.h b/xen/arch/x86/include/asm/msr-index.h
> > index 2749e433d2a7..5df090fba791 100644
> > --- a/xen/arch/x86/include/asm/msr-index.h
> > +++ b/xen/arch/x86/include/asm/msr-index.h
> > @@ -211,6 +211,11 @@
> >  
> >  #define MSR_VIRT_SPEC_CTRL                  0xc001011f /* Layout matches MSR_SPEC_CTRL */
> >  
> > +#define MSR_AMD_CSTATE_CFG                  0xc0010296
> > +#define  CSTATE_CFG_CCR0_CC6EN              (_AC(1, ULL) <<  6)
> > +#define  CSTATE_CFG_CCR1_CC6EN              (_AC(1, ULL) << 14)
> > +#define  CSTATE_CFG_CCR2_CC6EN              (_AC(1, ULL) << 22)
> 
> While MSR_AMD_CSTATE_CFG is liable to stay stable, the CC6EN bits are
> uarch specific and not applicable to others.
> 
> I'd suggest keeping them local to c6_errata_check(), which probably
> ought to gain a zen2 somewhere in the name.
> 
> They definitely can't say in a global header with names that don't tie
> them to Zen2 specifically.

That MSR and bit definitions are also available on family 19h model
11h and model 70h (with even more bits added to the MSR), so it feels
weird to name them Zen2 specifically (as it seems AMD is attempting to
keep the current bits stable across families).

I can move the defines to inside the function, but I'm not convinced
those are uarch specific.

Thanks, Roger.