From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCHv3 1/2] x86/ept: invalidate guest physical mappings on VMENTER Date: Fri, 4 Dec 2015 13:39:10 +0000 Message-ID: <5661977E.6080409@citrix.com> References: <1449160950-31722-1-git-send-email-david.vrabel@citrix.com> <1449160950-31722-2-git-send-email-david.vrabel@citrix.com> <56617254.1070904@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1a4qZy-0003nn-L7 for xen-devel@lists.xenproject.org; Fri, 04 Dec 2015 13:39:18 +0000 In-Reply-To: <56617254.1070904@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap , David Vrabel , xen-devel@lists.xenproject.org Cc: Kevin Tian , Jun Nakajima , George Dunlap , Andrew Cooper , Tim Deegan , Jan Beulich List-Id: xen-devel@lists.xenproject.org On 04/12/15 11:00, George Dunlap wrote: > On 03/12/15 16:42, David Vrabel wrote: >> If a guest allocates a page and the tlbflush_timestamp on the page >> indicates that a TLB flush of the previous owner is required, only the >> linear and combined mappings are invalidated. The guest-physical >> mappings are not invalidated. >> >> This is currently safe because the EPT code ensures that the >> guest-physical and combined mappings are invalidated /before/ the page >> is freed. However, this prevents us from deferring the EPT invalidate >> until after the page is freed (e.g., to defer the invalidate until the >> p2m locks are released). >> >> The TLB flush that may be done after allocating page already causes >> the original guest to VMEXIT, thus on VMENTER we can do an INVEPT if >> one is still pending. >> >> ept_sync_domain() now marks all PCPUs as needing to be invalidated, >> including PCPUs that the domain has not run on. We still only IPI >> those PCPUs that are active so this does not result in any more[1] >> INVEPT calls. >> >> We do not attempt to track when PCPUs may have cached translations >> because the only safe way to clear this per-CPU state if immediately >> after an invalidate the PCPU is not active (i.e., the PCPU is not in >> d->domain_dirty_cpumask). Since we only invalidate on VMENTER or by >> IPIing active PCPUs this can never happen. >> >> [1] There is one unnecessary INVEPT when the domain runs on a PCPU for >> the very first time. But this is: a) only 1 additional invalidate >> per PCPU for the lifetime of the domain; and b) we can avoid it >> with a subsequent change. >> >> Signed-off-by: David Vrabel > > This looks like a definite improvement. > > One thing you missed is that in ept_p2m_init(), it calls > __ept_sync_domain() on all cpus; but because the "invalidate" mask is > clear at that point, nothing will actually happen. Good point. I'd missed this because I'd planned to replace this initial invalidate by initializing ept->invalidate to all ones (as I alluded to in the [1] footnote). > Part of this I think comes as a result from inverting what the mask > means. Before this patch, "not synced" is the default, and therefore > you need to invalidate unless someone has set the bit saying you don't > have to. After this, "don't invalidate" is the default and you do > nothing unless someone has set a bit saying you do have to. > > I'd think prefer it if you left the mask as "synced_mask"; then you can > actually take that on_each_cpu() out of the ept_p2m_init entirely. > (Probably wants a comment pointing that out.) I changed its name because it's old use as synced_mask (i.e., the set of CPUs needing to be notified of required invalidation) did not match its name. I rather not keep the name and invert its use. David