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
