Re: [Xen-devel] [PATCH 0 of 2] Add libxc API that sets mem_access type for an array of gfns

[Christian Limpach <christian.limpach@xxxxxxxxx>]

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?
- 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.
- 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 == 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.

    christian

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel