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: Thu, 26 Apr 2012 18:06:13 -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 Thu, Apr 26, 2012 at 5:15 PM, Aravindh Puthiyaparambil wrote: > Does this look correct now? It addresses the issues I've pointed out, but: - you should leave the ASSERT where it is, or is there a reason to move it? - this is wrong: > - =A0 =A0 =A0 =A0old_entry =3D *ept_entry; > + =A0 =A0 =A0 =A0old_entry->epte =3D ept_entry->epte; You should follow the code and see what uses old_entry and you'll see that within the function old_entry->mfn is used (your diff changes the line that uses it) and ept_free_entry also accesses mfn. - are you sure you can move the ept_sync_domain call past the iommu code? I made a similar change a while ago, though it is for a more specific case, updating the ept table to "clean" the vram tracking. My change is: - clear needs_sync when setting the type to logdirty for a leaf entry if ( !is_epte_present(ept_entry) || (!target && p2mt =3D=3D p2m_ram_logdirty) ) needs_sync =3D 0; - only call ept_free_entry in the non-leaf case if ( target && is_epte_present(&old_entry) ) ept_free_entry(p2m, &old_entry, target); - call ept_sync_domain from hap_clean_vram_tracking Maybe you can do something similar, for example passing in a hint whether ept_sync_domain needs to be done or not. In my case, the reasoning is that all the entries changed from hap_clean_vram_tracking are leaf entries, so ept_free_entry will never be called and thus ept_sync_domain can be deferred. I didn't think through/consider the iommu case since that code is commented out in my tree. christian