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

From: "Tian, Kevin" <kevin.tian@intel.com>
To: Jan Beulich <JBeulich@suse.com>, Juergen Gross <jgross@suse.com>
Cc: "andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
	Dario Faggioli <dfaggioli@suse.com>,
	"wei.liu2@citrix.com" <wei.liu2@citrix.com>,
	"Nakajima, Jun" <jun.nakajima@intel.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 4/6] xen/x86: disable global pages for domains with XPTI active
Date: Fri, 9 Mar 2018 03:01:11 +0000	[thread overview]
Message-ID: <AADFC41AFE54684AB9EE6CBC0274A5D19103E46E@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <5AA14AF302000078001AFD30@prv-mh.provo.novell.com>

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, March 8, 2018 9:39 PM
> 
> > --- 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.
> 

I didn't see a reason why we want HOST_CR4 different from 
latest cr4 value, so yes, I think it's a reasonable cleanup.

p.s. current vmx logic looks problematic. It actually does 
reverse sync from HOST_CR4 to cr4 before vmentry which 
doesn't make any sense.

Thanks
Kevin 

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