On Apr 26, 2012 6:06 PM, "Christian Limpach" <christian.limpach@gmail.com>
wrote:
>
> On Thu, Apr 26, 2012 at 5:15 PM, Aravindh Puthiyaparambil
> <aravindh@virtuata.com> 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
> is:
> - 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.

Aravindh