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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.01.18 at 15:29, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 23/01/18 10:38, Jan Beulich wrote:
>> When XPTI is active, the CR3 load in restore_all_guest is sufficient
>> when switching to user mode, improving in particular system call and
>> page fault exit paths for the guest.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> While I can see the utility of this, we are starting to get into
> complicated territory as to which cr3 is loaded.  Also, the name
> "toggle" is no longer strictly accurate.

Yes, the "which CR3 is loaded" is not very nice a situation. I
wouldn't mind this being dropped unless it can be proven by
someone else to have more than just a pretty marginal effect.
The one patch from this series I certainly want to see go in is
patch 2 (with whatever adjustments are necessary); all the
others are more or less optional, but I still wanted to post
them.

As to "toggle" in the name - the function still toggles page
tables in all cases (in the sense of what's stored in v->arch.cr3);
it just doesn't necessarily also load that value into CR3.

> That being said, all of these helpers are only used in synchronous
> contexts as far as I can tell, so some ASSERT(!in_irq()) would probably
> go a long way.

I can certainly do that.

>> --- 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? Those code paths certainly need CR3 to be
written, or else the actual memory accesses will fail.

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

Jan


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