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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

> 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), and the second call to move from the
kernel back to user, which can in principle use the same fastpath.

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