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).

--- 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);

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:
>> --- 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

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