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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.12.2023 18:07, Roger Pau Monné wrote:
> On Tue, Dec 19, 2023 at 04:11:09PM +0100, Roger Pau Monné wrote:
>> On Tue, Dec 19, 2023 at 03:06:50PM +0100, Jan Beulich wrote:
>>> On 19.12.2023 12:48, Roger Pau Monné wrote:
>>>> On Tue, Dec 19, 2023 at 10:56:16AM +0100, Jan Beulich wrote:
>>>>> On 18.12.2023 18:24, Roger Pau Monné wrote:
>>>>>> On Tue, Feb 14, 2023 at 05:12:08PM +0100, Jan Beulich wrote:
>>>>>>> --- 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.
>>>>
>>>> Isn't it kind of a very specific case that we end up doing a
>>>> user->kernel switch as part of a context switch?  IOW: would require
>>>> the vCPU to be scheduled out at that very specific point.
>>>
>>> No, there's no user->kernel switch at the same time as context switch.
>>> What I was trying to explain is that with the actual IBPB being issued
>>> on exit to guest, both the context switch path and the user->kernel
>>> mode switch path set the same indicator, for the exit path to consume.
>>
>> Deferring to exit to guest path could be OK, but unless strictly
>> needed, which I don't think it's the case, I would request for IBPB to
>> be executed in C context rather than assembly one.
>>
>>>>>>> + *
>>>>>>> + * 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.
>>>>
>>>> Are you sure?  I see VMASST_TYPE_pae_extended_cr3 getting set in
>>>> dom0_construct_pv() and that makes me wonder whether other bits
>>>> couldn't start set also.
>>>>
>>>> Maybe there's some restriction I'm missing, but I don't see any
>>>> wording in the description of the interface that states that all
>>>> assists are supposed to start disabled.
>>>
>>> Well, that setting of pae_extended_cr3 is in response to the kernel's
>>> notes section having a respective indicator. So we still only set the
>>> bit in response to what the kernel's asking us to do, just that here
>>> we carry out the request ahead of launching the kernel.
>>>
>>> Also consider what would happen during migration if there was a
>>> default-on assist: At the destination we can't know whether the
>>> source simply didn't know of the bit, or whether the guest elected to
>>> clear it.
>>
>> Hm, I see, so I was indeed missing that aspect.  VM assist is passed
>> as a plain bitmap, and there's no signal on which assists the VM had
>> available on the source side if not enabled.
> 
> Sorry, please bear with me, as I've been further thinking about this.
> 
> Why does the assist needs to be default-on?  It's my understanding
> that the guest can execute the IBPB itself by writing to the MSR, but
> that's suboptimal in the user -> kernel context switch as it then
> involves two traps into Xen, but the guest is not left insecure, it
> just needs to write the MSR itself like on native.
> 
> In fact, if we add an IBPB by default as part of amd64 PV user ->
> kernel guest context switch, we are likely doing a double IBPB on
> guests not aware of the assist.
> 
> IOW: I don't know why doing the assist as guest opt-in would be
> insecure, in fact I think it's the best approach (again I might be
> missing something).

By issuing IBPB by default we can make guests safe (in this regard)
irrespective of their awareness of IBPB, and in particular their
awareness of IBPB being needed explicitly on the user->kernel mode
transition (where on native, with IBRS enabled, sufficient separation
exists iirc). IOW we're trying to cater for a 64-bit-PV special aspect
by default. (Andrew, please correct me if there's anything wrong in
here.)

Jan