Re: [PATCH v4 4/4] x86/PV: issue branch prediction barrier when switching 64-bit guest to kernel mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.12.2023 18:24, Roger Pau Monné wrote:
> On Tue, Feb 14, 2023 at 05:12:08PM +0100, Jan Beulich wrote:
>> Since both kernel and user mode run in ring 3, they run in the same
>> "predictor mode".
> 
> That only true when IBRS is enabled, otherwise all CPU modes share the
> same predictor mode?

But here we only care about ring 3 anyway?

>> @@ -753,7 +755,9 @@ static inline void pv_inject_sw_interrup
>>   * but we can't make such requests fail all of the sudden.
>>   */
>>  #define PV64_VM_ASSIST_MASK (PV32_VM_ASSIST_MASK                      | \
>> -                             (1UL << VMASST_TYPE_m2p_strict))
>> +                             (1UL << VMASST_TYPE_m2p_strict)          | \
>> +                             ((opt_ibpb_mode_switch + 0UL) <<           \
>> +                              VMASST_TYPE_mode_switch_no_ibpb))
> 
> I'm wondering that it's kind of weird to offer the option to PV domUs
> if opt_ibpb_entry_pv is set, as then the guest mode switch will always
> (implicitly) do a IBPB as requiring an hypercall and thus take an
> entry point into Xen.
> 
> I guess it's worth having it just as a way to signal to Xen that the
> hypervisor does perform an IBPB, even if the guest cannot disable it.

I'm afraid I'm confused by your reply. Not only, but also because the
latter sentence looks partly backwards / non-logical to me.

>> --- a/xen/arch/x86/pv/domain.c
>> +++ b/xen/arch/x86/pv/domain.c
>> @@ -455,6 +455,7 @@ static void _toggle_guest_pt(struct vcpu
>>  void toggle_guest_mode(struct vcpu *v)
>>  {
>>      const struct domain *d = v->domain;
>> +    struct cpu_info *cpu_info = get_cpu_info();
>>      unsigned long gs_base;
>>  
>>      ASSERT(!is_pv_32bit_vcpu(v));
>> @@ -467,15 +468,21 @@ void toggle_guest_mode(struct vcpu *v)
>>      if ( v->arch.flags & TF_kernel_mode )
>>          v->arch.pv.gs_base_kernel = gs_base;
>>      else
>> +    {
>>          v->arch.pv.gs_base_user = gs_base;
>> +
>> +        if ( opt_ibpb_mode_switch &&
>> +             !(d->arch.spec_ctrl_flags & SCF_entry_ibpb) &&
>> +             !VM_ASSIST(d, mode_switch_no_ibpb) )
>> +            cpu_info->spec_ctrl_flags |= SCF_new_pred_ctxt;
> 
> Likewise similar to the remarks I've made before, if doing an IBPB on
> entry is enough to cover for the case here, it must also be fine to
> issue the IBPB right here, instead of deferring to return to guest
> context?
> 
> The only concern would be (as you mentioned before) to avoid clearing
> valid Xen predictions, but I would rather see some figures about what
> effect the delaying to return to guest has vs issuing it right here.

Part of the reason (aiui) to do things on the exit path was to
consolidate the context switch induced one and the user->kernel switch
one into the same place and mechanism.

>> --- a/xen/include/public/xen.h
>> +++ b/xen/include/public/xen.h
>> @@ -554,6 +554,16 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
>>   */
>>  #define VMASST_TYPE_m2p_strict           32
>>  
>> +/*
>> + * x86-64 guests: Suppress IBPB on guest-user to guest-kernel mode switch.
> 
> I think this needs to be more vague, as it's not true that the IBPB
> will be suppressed if Xen is unconditionally issuing one on all guest
> entry points.
> 
> Maybe adding:
> 
> "Setting the assist signals Xen that the IBPB can be avoided from a
> guest perspective, however Xen might still issue one for other
> reasons."

I've done s/Suppress/Permit skipping/. I wouldn't want to go further,
as that then becomes related to implementation details imo. IOW of
course Xen may issue IBPB whenever it thinks there's a possible need.

>> + *
>> + * By default (on affected and capable hardware) as a safety measure Xen,
>> + * to cover for the fact that guest-kernel and guest-user modes are both
>> + * running in ring 3 (and hence share prediction context), would issue a
>> + * barrier for user->kernel mode switches of PV guests.
>> + */
>> +#define VMASST_TYPE_mode_switch_no_ibpb  33
> 
> Would it be possible to define the assist as
> VMASST_TYPE_mode_switch_ibpb and have it on when enabled?  So that the
> guest would disable it if unneeded?  IMO negated options are in
> general harder to understand.

Negative options aren't nice, yes, but VM assists start out as all
clear. The guest needs to change a "false" to a "true", and thus it
cannot be a positive option here, as we want the default (off) to be
safe/secure.

Jan