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 <jbeul...@suse.com>
Thanks, will wait for confirmation that the proposed comment is fine.
Roger.
