Re: [PATCH v2 2/3] amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.03.2022 15:18, Roger Pau Monne wrote:
> Allow HVM guests untrapped access to MSR_VIRT_SPEC_CTRL if the
> hardware has support for it. This requires adding logic in the
> vm{entry,exit} paths for SVM in order to context switch between the
> hypervisor value and the guest one. The added handlers for context
> switch will also be used for the legacy SSBD support.
> 
> Introduce a new synthetic feature leaf (X86_FEATURE_VIRT_SC_MSR_HVM)
> to signal whether VIRT_SPEC_CTRL needs to be handled on guest
> vm{entry,exit}.
> 
> Note the change in the handling of VIRT_SSBD in the featureset
> description. The change from 's' to 'S' is due to the fact that now if
> VIRT_SSBD is exposed by the hardware it can be passed through to HVM
> guests.

But lower vs upper case mean "(do not) expose by default", not whether
underlying hardware exposes the feature. In patch 1 you actually used
absence in underlying hardware to justify !, not s.

> @@ -610,6 +611,14 @@ static void cf_check svm_cpuid_policy_changed(struct 
> vcpu *v)
>      svm_intercept_msr(v, MSR_SPEC_CTRL,
>                        cp->extd.ibrs ? MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW);
>  
> +    /*
> +     * Give access to MSR_VIRT_SPEC_CTRL if the guest has been told about it
> +     * and the hardware implements it.
> +     */
> +    svm_intercept_msr(v, MSR_VIRT_SPEC_CTRL,
> +                      cp->extd.virt_ssbd && cpu_has_virt_ssbd ?

Despite giving the guest direct access guest_{rd,wr}msr() can be hit
for such guests. Don't you need to update what patch 1 added there?

Also, is there a reason the qualifier here is not in sync with ...

> @@ -3105,6 +3114,36 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>      vmcb_set_vintr(vmcb, intr);
>  }
>  
> +/* Called with GIF=0. */
> +void vmexit_virt_spec_ctrl(void)
> +{
> +    unsigned int val = opt_ssbd ? SPEC_CTRL_SSBD : 0;
> +
> +    if ( cpu_has_virt_ssbd )

... this one? Since the patching is keyed to VIRT_SC_MSR_HVM, which in
turn is enabled only when cpu_has_virt_ssbd, it would seem to me that
if any asymmetry was okay here, then using cp->extd.virt_ssbd without
cpu_has_virt_ssbd.

> @@ -1069,6 +1072,10 @@ void __init init_speculation_mitigations(void)
>              setup_force_cpu_cap(X86_FEATURE_SC_MSR_HVM);
>      }
>  
> +    /* Support VIRT_SPEC_CTRL.SSBD if AMD_SSBD is not available. */
> +    if ( opt_msr_sc_hvm && !cpu_has_amd_ssbd && cpu_has_virt_ssbd )
> +        setup_force_cpu_cap(X86_FEATURE_VIRT_SC_MSR_HVM);

In cpuid.c the comment (matching the code there) talks about exposing
by default. I can't bring this in line with the use of !cpu_has_amd_ssbd
here.

Jan