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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.05.2022 10:26, Roger Pau Monne wrote:
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -541,6 +541,9 @@ static void __init calculate_hvm_max_policy(void)
>           raw_cpuid_policy.basic.sep )
>          __set_bit(X86_FEATURE_SEP, hvm_featureset);
>  
> +    if ( boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) )
> +        __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
> +
>      /*
>       * If Xen isn't virtualising MSR_SPEC_CTRL for HVM guests (functional
>       * availability, or admin choice), hide the feature.

Especially with the setting of VIRT_SSBD below here (from patch 1) I
don't think this can go without comment. The more that the other
instance ...

> @@ -597,6 +600,13 @@ static void __init calculate_hvm_def_policy(void)
>      guest_common_feature_adjustments(hvm_featureset);
>      guest_common_default_feature_adjustments(hvm_featureset);
>  
> +    /*
> +     * Only expose VIRT_SSBD if AMD_SSBD is not available, and thus
> +     * VIRT_SC_MSR_HVM is set.
> +     */
> +    if ( boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) )
> +        __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
> +
>      sanitise_featureset(hvm_featureset);
>      cpuid_featureset_to_policy(hvm_featureset, p);
>      recalculate_xstate(p);

... here is about default exposure, so cannot really be extended to
the condition under which this is put in "max" (except that of course
"max" needs to include everything "def" has).

> @@ -3105,6 +3116,30 @@ 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 ( val == current->arch.msrs->virt_spec_ctrl.raw )
> +        return;
> +
> +    if ( cpu_has_virt_ssbd )
> +        wrmsr(MSR_VIRT_SPEC_CTRL, val, 0);
> +}
> +
> +/* Called with GIF=0. */
> +void vmentry_virt_spec_ctrl(void)
> +{
> +    unsigned int val = opt_ssbd ? SPEC_CTRL_SSBD : 0;
> +
> +    if ( val == current->arch.msrs->virt_spec_ctrl.raw )
> +        return;
> +
> +    if ( cpu_has_virt_ssbd )
> +        wrmsr(MSR_VIRT_SPEC_CTRL, current->arch.msrs->virt_spec_ctrl.raw, 0);
> +}

I guess the double use of current makes it difficult for the compiler
to CSE both uses. Furthermore for symmetry with the other function
how about

void vmentry_virt_spec_ctrl(void)
{
    unsigned int val = current->arch.msrs->virt_spec_ctrl.raw;

    if ( val == (opt_ssbd ? SPEC_CTRL_SSBD : 0) )
        return;

    if ( cpu_has_virt_ssbd )
        wrmsr(MSR_VIRT_SPEC_CTRL, val, 0);
}

i.e. "val" always representing the value we want to write?

With at least a comment added above, and preferably with the change
to the function (unless that gets in the way of the 3rd patch)
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan