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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.11.2022 18: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.
> 
> 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>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one further remark:

> --- 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);
> +    }
>  }

Is "cleared" in the comment correct when "spec-ctrl=ssbd"? I think "suitably
set" or "cleared/set" or some such would be wanted. This could certainly be
adjusted while committing (if you agree), but I will want to give Andrew some
time anyway before putting it in, to avoid there again being objections after
a change in this area has gone in.

Jan