Re: [PATCH v4 3/4] x86: limit issuing of IBPB during context switch

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

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