Re: [PATCH for-4.17 v3 1/2] amd/virt_ssbd: set SSBD at vCPU context switch

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 03/11/2022 17:02, Roger Pau Monne wrote:
> The current logic for AMD SSBD context switches it on every
> vm{entry,exit} if the Xen and guest selections don't match.  This is
> expensive when not using SPEC_CTRL, and hence should be avoided as
> much as possible.
>
> When SSBD is not being set from SPEC_CTRL on AMD don't context switch
> at vm{entry,exit} and instead only context switch SSBD when switching
> vCPUs.  This has the side effect of running Xen code with the guest
> selection of SSBD, the documentation is updated to note this behavior.
> Also note that then when `ssbd` is selected on the command line guest
> SSBD selection will not have an effect, and the hypervisor will run
> with SSBD unconditionally enabled when not using SPEC_CTRL itself.
>
> This fixes an issue with running C code in a GIF=0 region, that's
> problematic when using UBSAN or other instrumentation techniques.

This paragraph needs to be at the top, because it's the reason why this
is a blocker bug for 4.17.  Everything else is discussing why we take
the approach we take.

(and to be clear, it's slow even with MSR_SPEC_CTRL.  It's just that its
a whole lot less slow than with the LS_CFG MSR.)

>
> As a result of no longer running the code to set SSBD in a GIF=0
> region the locking of amd_set_legacy_ssbd() can be done using normal
> spinlocks, and some more checks can be added to assure it works as
> intended.
>
> Finally it's also worth noticing that since the guest SSBD selection
> is no longer set on vmentry the VIRT_SPEC_MSR handling needs to
> propagate the value to the hardware as part of handling the wrmsr.
>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Changes since v2:
>  - Fix calling set_reg unconditionally.
>  - Fix comment.
>  - Call amd_set_ssbd() from guest_wrmsr().
>
> Changes since v1:
>  - Just check virt_spec_ctrl value != 0 on context switch.
>  - Remove stray asm newline.
>  - Use val in svm_set_reg().
>  - Fix style in amd.c.
>  - Do not clear ssbd
> ---
>  docs/misc/xen-command-line.pandoc | 10 +++---
>  xen/arch/x86/cpu/amd.c            | 55 +++++++++++++++++--------------
>  xen/arch/x86/hvm/svm/entry.S      |  6 ----
>  xen/arch/x86/hvm/svm/svm.c        | 45 ++++++++++---------------
>  xen/arch/x86/include/asm/amd.h    |  2 +-
>  xen/arch/x86/msr.c                |  9 +++++

Need to patch msr.h now that the semantics of virt_spec_ctrl have changed.


> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> index 98c52d0686..05d72c6501 100644
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> <snip>
> +void amd_set_ssbd(bool enable)
> +{
> +     if (opt_ssbd)
> +             /*
> +              * Ignore attempts to turn off SSBD, it's hardcoded on the
> +              * command line.
> +              */
> +             return;
> +
> +     if (cpu_has_virt_ssbd)
> +             wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0, 0);
> +     else if (amd_legacy_ssbd)
> +             core_set_legacy_ssbd(enable);
> +     else
> +             ASSERT_UNREACHABLE();

This assert is reachable on Fam14 and older, I think.

> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 1aeaabcb13..8b101d4f27 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -973,6 +973,16 @@ static void cf_check svm_ctxt_switch_from(struct vcpu *v)
>  
>      /* Resume use of ISTs now that the host TR is reinstated. */
>      enable_each_ist(idt_tables[cpu]);
> +
> +    /*
> +     * Clear previous guest selection of SSBD if set.  Note that 
> SPEC_CTRL.SSBD
> +     * is already cleared by svm_vmexit_spec_ctrl.
> +     */
> +    if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
> +    {
> +        ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
> +        amd_set_ssbd(false);
> +    }
>  }
>  
>  static void cf_check svm_ctxt_switch_to(struct vcpu *v)
> @@ -1000,6 +1010,13 @@ static void cf_check svm_ctxt_switch_to(struct vcpu *v)
>  
>      if ( cpu_has_msr_tsc_aux )
>          wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
> +
> +    /* Load SSBD if set by the guest. */
> +    if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
> +    {
> +        ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
> +        amd_set_ssbd(true);
> +    }

While this functions, it's still a perf problem.  You now flip the bit
twice when switching between vcpus with legacy SSBD.

This wouldn't be so bad if you'd also fixed the inner function to not do
a read/modify/write on the very slow MSR, because then we'd only be
touching it twice, not 4 times.

This isn't critical to fix for 4.17, but will need addressing in due course.

However, as the patch does need a respin, amd_set_ssbd() is too
generic.  It's previous name, amd_set_legacy_ssbd(), was more
appropriate, as it clearly highlights the fact that it's the
non-MSR_SPEC_CTRL path.

~Andrew