Re: [Xen-devel] [PATCH 4/4] x86: avoid double CR3 reload when switching to guest user mode

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.02.18 at 18:37, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 31/01/18 10:12, Jan Beulich wrote:
>>
>>>> --- a/xen/arch/x86/pv/domain.c
>>>> +++ b/xen/arch/x86/pv/domain.c
>>>> @@ -220,10 +220,20 @@ int pv_domain_initialise(struct domain *
>>>>      return rc;
>>>>  }
>>>>  
>>>> -static void _toggle_guest_pt(struct vcpu *v)
>>>> +static void _toggle_guest_pt(struct vcpu *v, bool force_cr3)
>>>>  {
>>>>      v->arch.flags ^= TF_kernel_mode;
>>>>      update_cr3(v);
>>>> +
>>>> +    /*
>>>> +     * There's no need to load CR3 here when it is going to be loaded on 
> the
>>>> +     * way out to guest mode again anyway, and when the page tables we're
>>>> +     * currently on are the kernel ones (whereas when switching to kernel
>>>> +     * mode we need to be able to write a bounce frame onto the kernel 
> stack).
>>>> +     */
>>>> +    if ( !force_cr3 && !(v->arch.flags & TF_kernel_mode) )
>>>> +        return;
>>>> +
>>>>      /* Don't flush user global mappings from the TLB. Don't tick TLB 
>>>> clock. 
> */
>>>>      asm volatile ( "mov %0, %%cr3" : : "r" (v->arch.cr3) : "memory" );
>>>>  
>>>> @@ -253,13 +263,13 @@ void toggle_guest_mode(struct vcpu *v)
>>>>      }
>>>>      asm volatile ( "swapgs" );
>>>>  
>>>> -    _toggle_guest_pt(v);
>>>> +    _toggle_guest_pt(v, cpu_has_no_xpti);
>>>>  }
>>>>  
>>>>  void toggle_guest_pt(struct vcpu *v)
>>>>  {
>>>>      if ( !is_pv_32bit_vcpu(v) )
>>>> -        _toggle_guest_pt(v);
>>>> +        _toggle_guest_pt(v, true);
>>> This can be converted as well.  The only callers are the LDT-fault and
>>> I/O perm check, both when we are currently on user pagetables, needing
>>> to switch to kernel briefly, then back to user.
>> Converted to what?
> 
> _toggle_guest_pt(v, cpu_has_no_xpti);

I don't understand: We can't avoid the page table switch when
!cpu_has_no_xpti.

>> Those code paths certainly need CR3 to be
>> written, or else the actual memory accesses will fail.
> 
> These are called in pairs, the first of which switches from user to the
> kernel which pagetables, which fails your TF_kernel_mode fastpath
> (because of the xor at the top),

That TF_kernel_mode check happens only when force_cr3 is false.

> and the second call to move from the
> kernel back to user, which can in principle use the same fastpath.

Just like for the first of the paired calls, the second one also
mustn't skip the CR3 write. Just to summarize: When we come
here for one of the two cases where the paired calls are used,
we're running on user mode page tables but want to access a
kernel mode data structure. Hence we need to switch to the
kernel page tables (including the CR3 load), do the access, and
switch back to the user mode page tables.

Jan

>>> However, it would be better to drop the parameter and feed
>>> cpu_has_no_xpti into the conditional above which explains why it is safe
>>> to do.
>> As a result, I also don't understand this part.
> 
> The force_cr3 parameter is unnecessary, at which point it would be far
> clearer to explicitly check against cpu_has_no_xpti for the fastpath.
> 
> ~Andrew




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel