From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Limpach Subject: Re: [PATCH 0 of 2] Add libxc API that sets mem_access type for an array of gfns Date: Wed, 2 May 2012 20:28:15 -0700 Message-ID: References: Reply-To: Christian.Limpach@gmail.com Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Aravindh Puthiyaparambil Cc: tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Fri, Apr 27, 2012 at 9:22 PM, Aravindh Puthiyaparambil wrote: >>> Let me re-iterate: >>> - if it's a leaf entry, then there's no need to call ept_free_entry >>> - if you don't need to call ept_free_entry, then you can defer >>> ept_sync_domain (subject to the iommu case) >>> - you could pass in a pointer to a bool to indicate to the caller that >>> ept_sync_domain was deferred and that the caller needs to do it, with >>> a NULL pointer indicating that the caller doesn't support defer > > How does this look? It's missing the "leaf entry" part of my description of how I think things should work. Without that, you're effectively ignoring the following comment at the end of ept_set_entry: /* Release the old intermediate tables, if any. This has to be the last thing we do, after the ept_sync_domain() and removal from the iommu tables, so as to avoid a potential use-after-free. */ See inline comments in your patch below for how I'd change this, untested... christian > @@ -293,6 +296,7 @@ ept_set_entry(struct p2m_domain *p2m, un > =A0 =A0 int needs_sync =3D 1; > =A0 =A0 struct domain *d =3D p2m->domain; > =A0 =A0 ept_entry_t old_entry =3D { .epte =3D 0 }; > + =A0 =A0bool_t _sync_deferred =3D 0; don't need that > =A0 =A0 /* > =A0 =A0 =A0* the caller must make sure: > @@ -309,6 +313,9 @@ ept_set_entry(struct p2m_domain *p2m, un > =A0 =A0 =A0 =A0 =A0 =A0(target =3D=3D 1 && hvm_hap_has_2mb(d)) || > =A0 =A0 =A0 =A0 =A0 =A0(target =3D=3D 0)); > > + =A0 =A0if (sync_deferred) > + =A0 =A0 =A0 =A0*sync_deferred =3D 1; > + > =A0 =A0 table =3D map_domain_page(ept_get_asr(d)); > > =A0 =A0 for ( i =3D ept_get_wl(d); i > target; i-- ) > @@ -346,7 +353,11 @@ ept_set_entry(struct p2m_domain *p2m, un > > =A0 =A0 =A0 =A0 /* No need to flush if the old entry wasn't valid */ > =A0 =A0 =A0 =A0 if ( !is_epte_present(ept_entry) ) > needs_sync =3D 0; This should be: if ( !is_epte_present(ept_entry) || (!target && sync_deferred) ) { needs_sync =3D 0; if (sync_deferred) *sync_deferred =3D 0; } This expresses the notion that we're going to skip the call to ept_free_entry since it's a leaf entry (target =3D=3D 0) and that the caller will make the ept_sync_domain call (sync_deferred !=3D NULL). > > =A0 =A0 =A0 =A0 /* If we're replacing a non-leaf entry with a leaf entry > (1GiB or 2MiB), > =A0 =A0 =A0 =A0 =A0* the intermediate tables will be freed below after th= e ept flush > @@ -385,6 +396,9 @@ ept_set_entry(struct p2m_domain *p2m, un > > =A0 =A0 =A0 =A0 ASSERT(is_epte_superpage(ept_entry)); > > + =A0 =A0 =A0 =A0if ( sync_deferred ) > + =A0 =A0 =A0 =A0 =A0 =A0_sync_deferred =3D 1; > + don't need that > =A0 =A0 =A0 =A0 split_ept_entry =3D atomic_read_ept_entry(ept_entry); > > =A0 =A0 =A0 =A0 if ( !ept_split_super_page(p2m, &split_ept_entry, i, targ= et) ) > @@ -438,7 +452,7 @@ ept_set_entry(struct p2m_domain *p2m, un > =A0out: > =A0 =A0 unmap_domain_page(table); > > - =A0 =A0if ( needs_sync ) > + =A0 =A0if ( needs_sync && !_sync_deferred ) > =A0 =A0 =A0 =A0 ept_sync_domain(p2m->domain); don't need that change, test on needs_sync is sufficient > > =A0 =A0 if ( rv && iommu_enabled && need_iommu(p2m->domain) && > need_modify_vtd_table ) Then at the end of ept_set_pte add the test for non-leaf, which is more like an optimization/clarification since ept_free_entry has the same test already. if ( target && is_epte_present(&old_entry) ) ept_free_entry(p2m, &old_entry, target);