On Wed, Nov 02, 2022 at 12:49:17PM +0100, Jan Beulich wrote:
> On 29.10.2022 15:12, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -973,6 +973,16 @@ static void cf_check svm_ctxt_switch_from(struct vcpu
> > *v)
> >
> > /* Resume use of ISTs now that the host TR is reinstated. */
> > enable_each_ist(idt_tables[cpu]);
> > +
> > + /*
> > + * Clear previous guest selection of SSBD if set. Note that
> > SPEC_CTRL.SSBD
> > + * is already cleared by svm_vmexit_spec_ctrl.
> > + */
> > + if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
> > + {
> > + ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
> > + amd_set_ssbd(false);
> > + }
> > }
>
> Aren't you potentially turning off SSBD here just to ...
>
> > @@ -1000,6 +1010,13 @@ static void cf_check svm_ctxt_switch_to(struct vcpu
> > *v)
> >
> > if ( cpu_has_msr_tsc_aux )
> > wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
> > +
> > + /* Load SSBD if set by the guest. */
> > + if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
> > + {
> > + ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
> > + amd_set_ssbd(true);
> > + }
> > }
>
> ... turn it on here again? IOW wouldn't switching better be isolated to
> just svm_ctxt_switch_to(), doing nothing if already in the intended mode?
What if we switch from a HVM vCPU into a PV one? AFAICT then
svm_ctxt_switch_to() won't get called and we would be running the PV
guest with the previous HVM domain SSBD selection.
> > --- a/xen/arch/x86/msr.c
> > +++ b/xen/arch/x86/msr.c
> > @@ -697,7 +697,15 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t
> > val)
> > msrs->spec_ctrl.raw &= ~SPEC_CTRL_SSBD;
> > }
> > else
> > + {
> > msrs->virt_spec_ctrl.raw = val & SPEC_CTRL_SSBD;
> > + if ( v == curr )
> > + /*
> > + * Propagate the value to hardware, as it won't be context
> > + * switched on vmentry.
> > + */
>
> I have to admit that I find "on vmentry" in the comment misleading: Reading
> it I first thought you're still alluding to the old model. Plus I also find
> the combination of "context switched" and "on vmentry" problematic, as we
> generally mean something else when we say "context switch".
I had a hard time wording this, because of the Xen/guest vs vCPU
context switches.
What about:
"Propagate the value to hardware, as it won't we set on guest resume
path."
> > + goto set_reg;
>
> It's not clear why you want to use hvm_set_reg() in the first place - the
> comment says "propagate to hardware", which would mean wrmsrl() in the
> usual case. Here it would mean a direct call to amd_set_ssbd() imo. That
> would then also be in line with all other "v == curr" conditionals, none
> of which apply to any "goto set_reg". ..._set_reg(), aiui, is meant only
> for use in cases where vCPU state needs updating such that proper state
> would be loaded later (e.g. during VM entry).
I thought it was better to hide those vendor specific calls in the
already existing vendor hooks (set_reg). I don't mind calling
amd_set_ssbd() directly here if that's preferred, it seemed kind of a
layering violation when we have vendor specific hooks in place.
Thanks, Roger.
