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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, May 06, 2022 at 02:15:47PM +0200, Jan Beulich wrote:
> 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).

Would you be OK with adding:

    /*
     * VIRT_SC_MSR_HVM ensures the selection of SSBD is context
     * switched between the hypervisor and guest selected values for
     * HVM when the platform doesn't expose AMD_SSBD support.
     */

> > @@ -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?

Yes, that's fine.  I've adjusted the function.

> 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>

Thanks, will wait for confirmation that the proposed comment is fine.

Roger.