>>> On 09.04.18 at 16:33, <jgr...@suse.com> wrote: > On 09/04/18 16:15, Jan Beulich wrote: >>>>> On 06.04.18 at 09:52, <jgr...@suse.com> 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. > > In case you are feeling strong regarding this comment I can > remove it.
The first sentence is fine (and useful), so please don't drop it wholesale. >>> --- 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? > > I just want to make clear that NOFLUSH is not included. Would you > like a comment better? No - the involved symbol names together make clear that the NOFLUSH bit can't be included. The same bit can't possibly be part of the address _and_ be the signal to not flush. Jan _______________________________________________ Xen-devel mailing list Xenfirstname.lastname@example.org https://lists.xenproject.org/mailman/listinfo/xen-devel