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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

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.

Talking of VMX: I wonder whether it wouldn't be better (perhaps both
cheaper and long term possibly more correct) if vmx_ctxt_switch_to()
didn't write CR4, but instead sync-ed HOST_CR4 to what read_cr4()
returns. Jun, Kevin, do you have any thoughts on this? For the patch
here, this would get the behavior on par with SVM, as described above.

Jan


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