[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

On 15/03/14 22:36, Arianna Avanzini wrote:
On 03/15/2014 11:19 PM, Julien Grall wrote:
Did you try remove path? apply_p2m_changes is taking p2m->lock which is also
taken by p2m_lookup. With this solution it will end up to a deadlock.

The lookup is performed before grabbing p2m->lock, as stated in the comment.
I'll certainly remove it as it is useless, thank you for the feedback and for
the many suggestions.

Oh right, didn't pay attention about it. In any case, you need to keep in my mind that p2m_lookup is "slow" (it will map and unmap several page). If we can avoid it, it's better.

Anyway, you don't need to use p2m_lookup because you already have all the data
in pte (if pte.p2m.valid == 1):
    - pte.p2m.type  = p2m type
    - pte.p2m.base  = MFN


       if ( d != current->domain )
@@ -367,9 +382,23 @@ static int apply_p2m_changes(struct domain *d,
                       maddr += PAGE_SIZE;
-            case RELINQUISH:
               case REMOVE:
+                    /*
+                     * Ensure that, if we are trying to unmap I/O memory
+                     * ranges, the given gfn is p2m_mmio_direct.
+                     */

+                    if ( t == p2m_mmio_direct ? _t != p2m_mmio_direct : 0 ||
+                         paddr_to_pfn(_maddr) == INVALID_MFN ||

Testing pte.p2m.valid instead of paddr_to(_maddr)... is right answer.

Moreover, why do you need to check t? Every call to guest_physmap_remove_page is
done with a valid mfn (I guess it can be enhanced by a BUG_ON(mfn !=
INVALID_MFN) in this function).

I might be wrong, but it seems to me that apply_p2m_changes() is called with op
== REMOVE also from guest_physmap_remove_page(), and in that case t == 

The important bit is the MFN. I don't think we care about "t".

Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.