| Message ID | 29e2b527-16b8-e72d-f625-781aedf21bc4@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/spec-ctrl: IBPB improvements | expand |
On Tue, Feb 14, 2023 at 05:11:40PM +0100, Jan Beulich wrote: > When the outgoing vCPU had IBPB issued and RSB overwritten upon entering > Xen, then there's no need for a 2nd barrier during context switch. > > Note that SCF_entry_ibpb is always clear for the idle domain, so no > explicit idle domain check is needed to augment the feature check > (which is simply inapplicable to "idle"). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Roger Pau Monné <roger.pau@citrix.com> > --- > v4: Tighten the condition. > v3: Fold into series. > --- > I think in principle we could limit the impact from finding the idle > domain as "prevd", by having __context_switch() tell us what kind > domain's vCPU was switched out (it could still be "idle", but in fewer > cases). > > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -2005,17 +2005,26 @@ void context_switch(struct vcpu *prev, s > } > else > { > + unsigned int feat_sc_rsb = X86_FEATURE_SC_RSB_HVM; > + > __context_switch(); > > /* Re-enable interrupts before restoring state which may fault. */ > local_irq_enable(); > > if ( is_pv_domain(nextd) ) > + { > load_segments(next); > > + feat_sc_rsb = X86_FEATURE_SC_RSB_PV; > + } > + > ctxt_switch_levelling(next); > > - if ( opt_ibpb_ctxt_switch && !is_idle_domain(nextd) ) > + if ( opt_ibpb_ctxt_switch && !is_idle_domain(nextd) && > + (!(prevd->arch.spec_ctrl_flags & SCF_entry_ibpb) || > + /* is_idle_domain(prevd) || */ I would rather add a comment to note that the idle domain always has SCF_entry_ibpb clear, rather than leaving this commented check in the condition. > + !boot_cpu_has(feat_sc_rsb)) ) I do wonder if it would be more fail safe (and easier to expand going forward) if we introduce a new cpu_info field to track the CPU state: relevant here would be whether RSB has been overwritten and IBPB executed. Such state would be cleared on each return from guest path. Thanks, Roger.
On 18.12.2023 16:19, Roger Pau Monné wrote: > On Tue, Feb 14, 2023 at 05:11:40PM +0100, Jan Beulich wrote: >> When the outgoing vCPU had IBPB issued and RSB overwritten upon entering >> Xen, then there's no need for a 2nd barrier during context switch. >> >> Note that SCF_entry_ibpb is always clear for the idle domain, so no >> explicit idle domain check is needed to augment the feature check >> (which is simply inapplicable to "idle"). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. However, aiui the plan still is for Andrew to pick up this series and integrate it with other work he has in progress (or he is planning to do). >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -2005,17 +2005,26 @@ void context_switch(struct vcpu *prev, s >> } >> else >> { >> + unsigned int feat_sc_rsb = X86_FEATURE_SC_RSB_HVM; >> + >> __context_switch(); >> >> /* Re-enable interrupts before restoring state which may fault. */ >> local_irq_enable(); >> >> if ( is_pv_domain(nextd) ) >> + { >> load_segments(next); >> >> + feat_sc_rsb = X86_FEATURE_SC_RSB_PV; >> + } >> + >> ctxt_switch_levelling(next); >> >> - if ( opt_ibpb_ctxt_switch && !is_idle_domain(nextd) ) >> + if ( opt_ibpb_ctxt_switch && !is_idle_domain(nextd) && >> + (!(prevd->arch.spec_ctrl_flags & SCF_entry_ibpb) || >> + /* is_idle_domain(prevd) || */ > > I would rather add a comment to note that the idle domain always has > SCF_entry_ibpb clear, rather than leaving this commented check in the > condition. While I think I can see your point, I like it this way to match the other !is_idle_domain() that's here. >> + !boot_cpu_has(feat_sc_rsb)) ) > > I do wonder if it would be more fail safe (and easier to expand going > forward) if we introduce a new cpu_info field to track the CPU state: > relevant here would be whether RSB has been overwritten and IBPB > executed. Such state would be cleared on each return from guest path. To be honest - I'm not sure whether that would help or make things more fragile. More state also means more things which can become incorrect / inconsistent. Jan
On 18.12.2023 16:19, Roger Pau Monné wrote: > On Tue, Feb 14, 2023 at 05:11:40PM +0100, Jan Beulich wrote: >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -2005,17 +2005,26 @@ void context_switch(struct vcpu *prev, s >> } >> else >> { >> + unsigned int feat_sc_rsb = X86_FEATURE_SC_RSB_HVM; >> + >> __context_switch(); >> >> /* Re-enable interrupts before restoring state which may fault. */ >> local_irq_enable(); >> >> if ( is_pv_domain(nextd) ) >> + { >> load_segments(next); >> >> + feat_sc_rsb = X86_FEATURE_SC_RSB_PV; >> + } >> + >> ctxt_switch_levelling(next); >> >> - if ( opt_ibpb_ctxt_switch && !is_idle_domain(nextd) ) >> + if ( opt_ibpb_ctxt_switch && !is_idle_domain(nextd) && >> + (!(prevd->arch.spec_ctrl_flags & SCF_entry_ibpb) || >> + /* is_idle_domain(prevd) || */ > > I would rather add a comment to note that the idle domain always has > SCF_entry_ibpb clear, rather than leaving this commented check in the > condition. > >> + !boot_cpu_has(feat_sc_rsb)) ) Oh, for completeness: For v5 I have this @@ -2092,17 +2092,26 @@ void context_switch(struct vcpu *prev, s } else { + unsigned int feat_sc_rsb = X86_FEATURE_SC_RSB_HVM; + __context_switch(); /* Re-enable interrupts before restoring state which may fault. */ local_irq_enable(); if ( is_pv_domain(nextd) ) + { load_segments(next); + feat_sc_rsb = X86_FEATURE_SC_RSB_PV; + } + ctxt_switch_levelling(next); - if ( opt_ibpb_ctxt_switch && !is_idle_domain(nextd) ) + if ( opt_ibpb_ctxt_switch && !is_idle_domain(nextd) && + (!(prevd->arch.spec_ctrl_flags & SCF_entry_ibpb) || + /* is_idle_domain(prevd) || */ + (!cpu_has_auto_ibrs && !boot_cpu_has(feat_sc_rsb))) ) { static DEFINE_PER_CPU(unsigned int, last); unsigned int *last_id = &this_cpu(last); i.e. with the cpu_has_auto_ibrs check added. Jan
--- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -2005,17 +2005,26 @@ void context_switch(struct vcpu *prev, s } else { + unsigned int feat_sc_rsb = X86_FEATURE_SC_RSB_HVM; + __context_switch(); /* Re-enable interrupts before restoring state which may fault. */ local_irq_enable(); if ( is_pv_domain(nextd) ) + { load_segments(next); + feat_sc_rsb = X86_FEATURE_SC_RSB_PV; + } + ctxt_switch_levelling(next); - if ( opt_ibpb_ctxt_switch && !is_idle_domain(nextd) ) + if ( opt_ibpb_ctxt_switch && !is_idle_domain(nextd) && + (!(prevd->arch.spec_ctrl_flags & SCF_entry_ibpb) || + /* is_idle_domain(prevd) || */ + !boot_cpu_has(feat_sc_rsb)) ) { static DEFINE_PER_CPU(unsigned int, last); unsigned int *last_id = &this_cpu(last);
When the outgoing vCPU had IBPB issued and RSB overwritten upon entering Xen, then there's no need for a 2nd barrier during context switch. Note that SCF_entry_ibpb is always clear for the idle domain, so no explicit idle domain check is needed to augment the feature check (which is simply inapplicable to "idle"). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v4: Tighten the condition. v3: Fold into series. --- I think in principle we could limit the impact from finding the idle domain as "prevd", by having __context_switch() tell us what kind domain's vCPU was switched out (it could still be "idle", but in fewer cases).