Re: [PATCH v3] amd: disable C6 after 1000 days on Zen2

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.07.2023 16:47, 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 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@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with two remarks:

> @@ -1249,3 +1282,44 @@ const struct cpu_dev amd_cpu_dev = {
>       .c_early_init   = early_init_amd,
>       .c_init         = init_amd,
>  };
> +
> +static int __init cf_check zen2_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;
> +
> +     /*
> +      * Zen1 vs Zen2 isn't a simple model number comparison, so use STIBP as
> +      * a heuristic to separate the two uarches in Fam17h.
> +      */
> +     if (cpu_has_hypervisor || boot_cpu_data.x86 != 0x17 ||
> +         !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.

I'm not really convinced of this being the worst case. TSC can be
written, and hence it could also have been altered in a way making
things look as if system wasn't running for a long time when really
it has been. But I'm okay to leave that special case aside right
now.

> +      * 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, zen2_disable_c6, NULL, 0);
> +             set_timer(&errata_c6, NOW() + delta);
> +     } else
> +             zen2_disable_c6(NULL);

Strictly speaking you don't need the if/else here, since timers set
in the past will simply have their handlers executed right away (and
if that wasn't the case, there would be a race here).

Jan