[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 0 of 2] Add libxc API that sets mem_access type for an array of gfns
On Apr 26, 2012 6:06 PM, "Christian Limpach" <christian.limpach@xxxxxxxxx> wrote:
> On Thu, Apr 26, 2012 at 5:15 PM, Aravindh Puthiyaparambil
> <aravindh@xxxxxxxxxxxx> 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?
Ok, I will move the ASSERT back to where it was.
> - this is wrong:
> > - old_entry = *ept_entry;
> > + old_entry->epte = 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.
I will fix that.
> - are you sure you can move the ept_sync_domain call past the iommu code?
I was hoping Tim would give me feedback about that.
> 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
> - clear needs_sync when setting the type to logdirty for a leaf entry
> if ( !is_epte_present(ept_entry) ||
> (!target && p2mt == p2m_ram_logdirty) )
> needs_sync = 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.
I thought about doing that initially. But then in the bulk case I would always have to call ept_sync_domain() to be on the safe side. But if the iommu case forces me down that path, then I guess I have no choice.
Xen-devel mailing list