| Message ID | 23ea08db-3b64-5d1a-6743-19abb7bd6529@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/spec-ctrl: IPBP improvements | expand |
On 25/01/2023 3:26 pm, Jan Beulich wrote: > In order to avoid clobbering Xen's own predictions, defer the barrier as > much as possible. I can't actually think of a case where this matters. We've done a reasonable amount of work to get rid of indirect branches, and rets were already immaterial because of the reset_stack_and_jump(). What I'm trying to figure out is whether this ends up hurting us. If there was an indirect call we used reliably pre and post context switch, that changed target based on the guest type, then we'd now retain and use the bad prediction. > Merely mark the CPU as needing a barrier issued the > next time we're exiting to guest context. > > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > I couldn't find any sensible (central/unique) place where to move the > comment which is being deleted alongside spec_ctrl_new_guest_context(). Given this, I'm wondering whether in patch 1, it might be better to name the new bit SCF_new_guest_context. Or new_pred_context context (which on consideration, I think is better than the current name)? This would have a knock-on effect on the feature names, which I think is important because I think you've got a subtle bug in patch 3. The comment really wants to stay, and it could simply move into spec_ctrl_asm.h at that point. ~Andrew
On 26.01.2023 21:43, Andrew Cooper wrote: > On 25/01/2023 3:26 pm, Jan Beulich wrote: >> In order to avoid clobbering Xen's own predictions, defer the barrier as >> much as possible. > > I can't actually think of a case where this matters. We've done a > reasonable amount of work to get rid of indirect branches, and rets were > already immaterial because of the reset_stack_and_jump(). > > What I'm trying to figure out is whether this ends up hurting us. If > there was an indirect call we used reliably pre and post context switch, > that changed target based on the guest type, then we'd now retain and > use the bad prediction. Hmm, so far I was understanding that the context switch operation is solely for guests' purposes; this looks to be supported by the comments there. If we were worried about Xen itself here (which of course is a valid concern), then I think that together with the issue you've spotted in patch 3 all I can do is drop the two middle patches (and the HVM part of patch 1). At which point ... >> Merely mark the CPU as needing a barrier issued the >> next time we're exiting to guest context. >> >> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> I couldn't find any sensible (central/unique) place where to move the >> comment which is being deleted alongside spec_ctrl_new_guest_context(). > > Given this, I'm wondering whether in patch 1, it might be better to name > the new bit SCF_new_guest_context. Or new_pred_context context (which > on consideration, I think is better than the current name)? > > This would have a knock-on effect on the feature names, which I think is > important because I think you've got a subtle bug in patch 3. ... this renaming, which I've done already, may have been in vein. Jan
--- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -2038,7 +2038,7 @@ void context_switch(struct vcpu *prev, s */ if ( *last_id != next_id ) { - spec_ctrl_new_guest_context(); + info->spec_ctrl_flags |= SCF_exit_ibpb; *last_id = next_id; } } --- a/xen/arch/x86/include/asm/spec_ctrl.h +++ b/xen/arch/x86/include/asm/spec_ctrl.h @@ -67,28 +67,6 @@ void init_speculation_mitigations(void); void spec_ctrl_init_domain(struct domain *d); -/* - * Switch to a new guest prediction context. - * - * This flushes all indirect branch predictors (BTB, RSB/RAS), so guest code - * which has previously run on this CPU can't attack subsequent guest code. - * - * As this flushes the RSB/RAS, it destroys the predictions of the calling - * context. For best performace, arrange for this to be used when we're going - * to jump out of the current context, e.g. with reset_stack_and_jump(). - * - * For hardware which mis-implements IBPB, fix up by flushing the RSB/RAS - * manually. - */ -static always_inline void spec_ctrl_new_guest_context(void) -{ - wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB); - - /* (ab)use alternative_input() to specify clobbers. */ - alternative_input("", "DO_OVERWRITE_RSB", X86_BUG_IBPB_NO_RET, - : "rax", "rcx"); -} - extern int8_t opt_ibpb_ctxt_switch; extern bool opt_ssbd; extern int8_t opt_eager_fpu; --- a/xen/arch/x86/spec_ctrl.c +++ b/xen/arch/x86/spec_ctrl.c @@ -854,6 +854,11 @@ static void __init ibpb_calculations(voi */ if ( opt_ibpb_ctxt_switch == -1 ) opt_ibpb_ctxt_switch = !(opt_ibpb_entry_hvm && opt_ibpb_entry_pv); + if ( opt_ibpb_ctxt_switch ) + { + setup_force_cpu_cap(X86_FEATURE_IBPB_EXIT_PV); + setup_force_cpu_cap(X86_FEATURE_IBPB_EXIT_HVM); + } } /* Calculate whether this CPU is vulnerable to L1TF. */
In order to avoid clobbering Xen's own predictions, defer the barrier as much as possible. Merely mark the CPU as needing a barrier issued the next time we're exiting to guest context. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I couldn't find any sensible (central/unique) place where to move the comment which is being deleted alongside spec_ctrl_new_guest_context(). --- v3: New.