Re: [PATCH 3/3] x86/amd: Use newer SSBD mechanisms if they exist

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.08.2021 16:30, Andrew Cooper wrote:
> The opencoded legacy Memory Disambiguation logic in init_amd() neglected
> Fam19h for the Zen3 microarchitecture.
> 
> In practice, all Zen2 based system (AMD Fam17h Model >= 0x30 and Hygon Fam18h
> Model >= 0x4) have the architectural MSR_SPEC_CTRL and the SSBD bit within it.
> 
> Implement the algorithm given in AMD's SSBD whitepaper, and leave a
> printk_once() behind in the case that no controls can be found.
> 
> This now means that a user choosing `spec-ctrl=no-ssb` will actually turn off
> Memory Disambiguation on Fam19h/Zen3 systems.

Aiui you mean `spec-ctrl=no-ssbd` here? And the effect would then be
to turn _on_ Memory Disambiguation, unless the original comment was
the wrong way round? I'm also concerned by this behavioral change:
I think opt_ssbd would want to become a tristate, such that not
specifying the option at all will not also result in turning the bit
off even if it was on for some reason (firmware?). Similarly
"spec-ctrl=no" and "spec-ctrl=no-xen" imo shouldn't have this effect.

> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -681,6 +681,56 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
>                         c->x86_capability);
>  }
>  
> +/*
> + * Refer to the AMD Speculative Store Bypass whitepaper:
> + * 
> https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf
> + */
> +void amd_init_ssbd(const struct cpuinfo_x86 *c)
> +{
> +     int bit = -1;
> +
> +     if (cpu_has_ssb_no)
> +             return;
> +
> +     if (cpu_has_amd_ssbd) {
> +             wrmsrl(MSR_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
> +             return;
> +     }
> +
> +     if (cpu_has_virt_ssbd) {
> +             wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
> +             return;
> +     }
> +
> +     switch (c->x86) {
> +     case 0x15: bit = 54; break;
> +     case 0x16: bit = 33; break;
> +     case 0x17:
> +     case 0x18: bit = 10; break;
> +     }
> +
> +     if (bit >= 0) {
> +             uint64_t val, mask = 1ull << bit;
> +
> +             if (rdmsr_safe(MSR_AMD64_LS_CFG, val) ||
> +                 ({
> +                         val &= ~mask;
> +                         if ( opt_ssbd )

Nit: No spaces inside the parentheses here.

Jan