Re: [Xen-devel] [PATCH v2 4/6] xen/x86: disable global pages for domains with XPTI active

[Juergen Gross <jgross@xxxxxxxx>]

On 08/03/18 14:38, Jan Beulich wrote:
>>>> On 02.03.18 at 09:14, <jgross@xxxxxxxx> wrote:
>> Instead of flushing the TLB from global pages when switching address
>> spaces with XPTI being active just disable global pages via %cr4
>> completely when a domain subject to XPTI is active. This avoids the
>> need for extra TLB flushes as loading %cr3 will remove all TLB
>> entries.
> 
> Hmm, it's far from obvious that this is an improvement overall.
> Besides Xen's global pages, we also prevent guest user pages to
> be evicted from the TLB across user <-> kernel mode changes.
> And the effects of this are likely quite work load dependent.

With XPTI active we flush the TLB, including all global entries, when
switching between page tables when returning to the guest. So there are
no entries which could survive.

>> @@ -412,18 +414,22 @@ static void prepare_set(void)
>>      write_cr0(read_cr0() | X86_CR0_CD);
>>      wbinvd();
>>  
>> -    /*  TLB flushing here relies on Xen always using CR4.PGE. */
>> -    BUILD_BUG_ON(!(XEN_MINIMAL_CR4 & X86_CR4_PGE));
>> -    write_cr4(read_cr4() & ~X86_CR4_PGE);
>> +    cr4 = read_cr4();
>> +    if (cr4 & X86_CR4_PGE)
>> +            write_cr4(cr4 & ~X86_CR4_PGE);
>> +    else
>> +            asm volatile( "mov %0, %%cr3" : : "r" (read_cr3()) : "memory" );
>>  
>>      /*  Save MTRR state */
>>      rdmsrl(MSR_MTRRdefType, deftype);
>>  
>>      /*  Disable MTRRs, and set the default type to uncached  */
>>      mtrr_wrmsr(MSR_MTRRdefType, deftype & ~0xcff);
>> +
>> +    return !!(cr4 & X86_CR4_PGE);
> 
> Unnecessary !!.

Return type is bool. Isn't !! better in this case?

> 
>> --- a/xen/arch/x86/flushtlb.c
>> +++ b/xen/arch/x86/flushtlb.c
>> @@ -72,20 +72,39 @@ static void post_flush(u32 t)
>>      this_cpu(tlbflush_time) = t;
>>  }
>>  
>> +static void do_flush_tlb(unsigned long cr3)
> 
> I think this is not a good name, because for its use in write_cr3()
> the TLB flush is specifically a secondary effect. Personally I'd
> prefer the function to be named e.g. do_write_cr3().

Okay.

> 
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -622,7 +622,8 @@ unsigned long pv_guest_cr4_fixup(const struct vcpu *, 
>> unsigned long guest_cr4);
>>              X86_CR4_SMAP | X86_CR4_OSXSAVE |                \
>>              X86_CR4_FSGSBASE))                              \
>>        | ((v)->domain->arch.vtsc ? X86_CR4_TSD : 0))         \
>> -     & ~X86_CR4_DE)
>> +     & ~(X86_CR4_DE |                                       \
>> +         ((v)->domain->arch.pv_domain.xpti ? X86_CR4_PGE : 0)))
> 
> With this you manage to turn off global pages when switching to
> a PV vCPU. But I can't see how you turn global pages back on when
> switching away from it. I can see they would be turned back on e.g.
> on the first entry to a VMX guest, but how about an SVM one? And
> how about the time between switching away from the PV vCPU and
> that VM entry? Granted all flushes are global ones right now, but
> that should change with the modification here: If you look back at
> 4.2 code, you'll see that FLUSH_TLB was handled differently in that
> case, retaining Xen's global mappings. Any flush IPI not requesting
> global pages to be flushed could then leave intact Xen's own TLB
> entries, which takes as a prereq that CR4.PGE gets turned back on
> earlier.

Right, turning PGE on again is missing. I had a different solution for
switching PGE on and off in the beginning, but things got rather
complicated. So I changed my mind and turning PGE on again must have
slipped through.

> And one more change would belong into this patch, I think: In patch
> 2 you change write_ptbase(). The bare CR3 write there would
> become eligible to tick the TLB flush clock with what you do here.

Yes, I'll add that.


Juergen

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