Re: [PATCH v5 3/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.05.2022 10:26, Roger Pau Monne wrote:
> Expose VIRT_SSBD to guests if the hardware supports setting SSBD in
> the LS_CFG MSR (a.k.a. non-architectural way). Different AMD CPU
> families use different bits in LS_CFG, so exposing VIRT_SPEC_CTRL.SSBD
> allows for an unified way of exposing SSBD support to guests on AMD
> hardware that's compatible migration wise, regardless of what
> underlying mechanism is used to set SSBD.
> 
> Note that on AMD Family 17h and Hygon Family 18h processors the value
> of SSBD in LS_CFG is shared between threads on the same core, so
> there's extra logic in order to synchronize the value and have SSBD
> set as long as one of the threads in the core requires it to be set.
> Such logic also requires extra storage for each thread state, which is
> allocated at initialization time.
> 
> Do the context switching of the SSBD selection in LS_CFG between
> hypervisor and guest in the same handler that's already used to switch
> the value of VIRT_SPEC_CTRL.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

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

> +void amd_set_legacy_ssbd(bool enable)
> +{
> +     const struct cpuinfo_x86 *c = &current_cpu_data;
> +     struct ssbd_ls_cfg *status;
> +
> +     if ((c->x86 != 0x17 && c->x86 != 0x18) || c->x86_num_siblings <= 1) {
> +             set_legacy_ssbd(c, enable);
> +             return;
> +     }
> +
> +     status = &ssbd_ls_cfg[c->phys_proc_id * ssbd_max_cores +
> +                           c->cpu_core_id];
> +
> +     /*
> +      * Open code a very simple spinlock: this function is used with GIF==0
> +      * and different IF values, so would trigger the checklock detector.
> +      * Instead of trying to workaround the detector, use a very simple lock
> +      * implementation: it's better to reduce the amount of code executed
> +      * with GIF==0.
> +      */
> +     while ( test_and_set_bool(status->locked) )

Nit: A bit of Xen style slipped into here.

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -3126,6 +3126,8 @@ void vmexit_virt_spec_ctrl(void)
>  
>      if ( cpu_has_virt_ssbd )
>          wrmsr(MSR_VIRT_SPEC_CTRL, val, 0);
> +    else
> +        amd_set_legacy_ssbd(val);
>  }
>  
>  /* Called with GIF=0. */
> @@ -3138,6 +3140,8 @@ void vmentry_virt_spec_ctrl(void)
>  
>      if ( cpu_has_virt_ssbd )
>          wrmsr(MSR_VIRT_SPEC_CTRL, current->arch.msrs->virt_spec_ctrl.raw, 0);
> +    else
> +        amd_set_legacy_ssbd(!val);
>  }

Aiui the adjustment suggested for patch 2 will not really get in the way
of this, by only requiring to drop the ! .

Jan