Re: [Xen-devel] [PATCH v5 7/7] xen/x86: use PCID feature

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.04.18 at 09:52, <jgross@xxxxxxxx> wrote:
> @@ -100,12 +102,35 @@ void write_cr3_cr4(unsigned long cr3, unsigned long cr4)
>      t = pre_flush();
>  
>      if ( read_cr4() & X86_CR4_PGE )
> +        /*
> +         * X86_CR4_PGE set means PCID being inactive.
> +         * We have to purge the TLB via flipping cr4.pge.
> +         */
>          write_cr4(cr4 & ~X86_CR4_PGE);
> +    else if ( use_invpcid )
> +        /*
> +         * If we are using PCID purge the TLB via INVPCID as loading cr3
> +         * will affect the new PCID only.
> +         * If INVPCID is not supported we don't use PCIDs so loading cr3
> +         * will purge the TLB (we are in the "global pages off" branch).
> +         * invpcid_flush_all_nonglobals() seems to be faster than
> +         * invpcid_flush_all().
> +         */
> +        invpcid_flush_all_nonglobals();
>  
>      asm volatile ( "mov %0, %%cr3" : : "r" (cr3) : "memory" );
>  
>      if ( read_cr4() != cr4 )
>          write_cr4(cr4);
> +    else if ( old_pcid != (cr3 & X86_CR3_PCID_MASK) )
> +        /*
> +         * Make sure no TLB entries related to the old PCID created between
> +         * flushing the TLB and writing the new %cr3 value remain in the TLB.
> +         * Writing %cr3 is documented to be a speculation barrier, OTOH the
> +         * performance impact of the additional flush is next to invisible.
> +         * So better be save than sorry.
> +         */
> +        invpcid_flush_single_context(old_pcid);

I'm not really happy about this comment. The CR3 write being a
speculation barrier is of no real interest here. Until the CPU's
speculation logic reaches that insn, all sort of things can happen.
We don't even know the exact code the compiler will generate,
much less what that code will trigger inside the CPU.

Also I think it's "safe" in the last sentence.

> --- a/xen/include/asm-x86/x86-defns.h
> +++ b/xen/include/asm-x86/x86-defns.h
> @@ -45,7 +45,9 @@
>  /*
>   * Intel CPU flags in CR3
>   */
> -#define X86_CR3_NOFLUSH (_AC(1, ULL) << 63)
> +#define X86_CR3_NOFLUSH    (_AC(1, ULL) << 63)
> +#define X86_CR3_ADDR_MASK  (PAGE_MASK & PADDR_MASK & ~X86_CR3_NOFLUSH)

Why still ~X86_CR3_NOFLUSH now that you use PADDR_MASK?

Jan


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