On Mon, Nov 14, 2022 at 09:39:50PM +0000, Andrew Cooper wrote:
> On 03/11/2022 17:02, Roger Pau Monne wrote:
> > The current logic for AMD SSBD context switches it on every
> > vm{entry,exit} if the Xen and guest selections don't match. This is
> > expensive when not using SPEC_CTRL, and hence should be avoided as
> > much as possible.
> >
> > When SSBD is not being set from SPEC_CTRL on AMD don't context switch
> > at vm{entry,exit} and instead only context switch SSBD when switching
> > vCPUs. This has the side effect of running Xen code with the guest
> > selection of SSBD, the documentation is updated to note this behavior.
> > Also note that then when `ssbd` is selected on the command line guest
> > SSBD selection will not have an effect, and the hypervisor will run
> > with SSBD unconditionally enabled when not using SPEC_CTRL itself.
> >
> > This fixes an issue with running C code in a GIF=0 region, that's
> > problematic when using UBSAN or other instrumentation techniques.
>
> This paragraph needs to be at the top, because it's the reason why this
> is a blocker bug for 4.17. Everything else is discussing why we take
> the approach we take.
Done.
> (and to be clear, it's slow even with MSR_SPEC_CTRL. It's just that its
> a whole lot less slow than with the LS_CFG MSR.)
>
> >
> > As a result of no longer running the code to set SSBD in a GIF=0
> > region the locking of amd_set_legacy_ssbd() can be done using normal
> > spinlocks, and some more checks can be added to assure it works as
> > intended.
> >
> > Finally it's also worth noticing that since the guest SSBD selection
> > is no longer set on vmentry the VIRT_SPEC_MSR handling needs to
> > propagate the value to the hardware as part of handling the wrmsr.
> >
> > Signed-off-by: Roger Pau Monné <roger....@citrix.com>
> > ---
> > Changes since v2:
> > - Fix calling set_reg unconditionally.
> > - Fix comment.
> > - Call amd_set_ssbd() from guest_wrmsr().
> >
> > Changes since v1:
> > - Just check virt_spec_ctrl value != 0 on context switch.
> > - Remove stray asm newline.
> > - Use val in svm_set_reg().
> > - Fix style in amd.c.
> > - Do not clear ssbd
> > ---
> > docs/misc/xen-command-line.pandoc | 10 +++---
> > xen/arch/x86/cpu/amd.c | 55 +++++++++++++++++--------------
> > xen/arch/x86/hvm/svm/entry.S | 6 ----
> > xen/arch/x86/hvm/svm/svm.c | 45 ++++++++++---------------
> > xen/arch/x86/include/asm/amd.h | 2 +-
> > xen/arch/x86/msr.c | 9 +++++
>
> Need to patch msr.h now that the semantics of virt_spec_ctrl have changed.
Sure, will adjust the comment there.
>
> > diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> > index 98c52d0686..05d72c6501 100644
> > --- a/xen/arch/x86/cpu/amd.c
> > +++ b/xen/arch/x86/cpu/amd.c
> > <snip>
> > +void amd_set_ssbd(bool enable)
> > +{
> > + if (opt_ssbd)
> > + /*
> > + * Ignore attempts to turn off SSBD, it's hardcoded on the
> > + * command line.
> > + */
> > + return;
> > +
> > + if (cpu_has_virt_ssbd)
> > + wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0, 0);
> > + else if (amd_legacy_ssbd)
> > + core_set_legacy_ssbd(enable);
> > + else
> > + ASSERT_UNREACHABLE();
>
> This assert is reachable on Fam14 and older, I think.
Hm, I'm unsure how. Calls to this function are gated on the vCPU
having virt_ssbd set in the CPUID policy, and that can only happen if
there's a way to set SSBD.
Can you elaborate on the path that you think can trigger this?
As I would think that's the path that needs fixing, rather than
removing the assert here.
> > diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> > index 1aeaabcb13..8b101d4f27 100644
> > --- 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);
> > + }
> > }
> >
> > static void cf_check svm_ctxt_switch_to(struct vcpu *v)
> > @@ -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);
> > + }
>
> While this functions, it's still a perf problem. You now flip the bit
> twice when switching between vcpus with legacy SSBD.
>
> This wouldn't be so bad if you'd also fixed the inner function to not do
> a read/modify/write on the very slow MSR, because then we'd only be
> touching it twice, not 4 times.
>
> This isn't critical to fix for 4.17, but will need addressing in due course.
Indeed. I know about this, but didn't consider it critical enough to
fix in the current release status.
> However, as the patch does need a respin, amd_set_ssbd() is too
> generic. It's previous name, amd_set_legacy_ssbd(), was more
> appropriate, as it clearly highlights the fact that it's the
> non-MSR_SPEC_CTRL path.
Can adjust the function name, not a problem.
Thanks, Roger.
