[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
- To: Arianna Avanzini <avanzini.arianna@xxxxxxxxx>, xen-devel@xxxxxxxxxxxxx
- From: Julien Grall <julien.grall@xxxxxxxxxx>
- Date: Sat, 15 Mar 2014 22:42:57 +0000
- Cc: julien.grall@xxxxxxxxxx, paolo.valente@xxxxxxxxxx, keir@xxxxxxx, stefano.stabellini@xxxxxxxxxxxxx, tim@xxxxxxx, dario.faggioli@xxxxxxxxxx, Ian.Jackson@xxxxxxxxxxxxx, Ian.Campbell@xxxxxxxxxxxxx, etrudeau@xxxxxxxxxxxx, JBeulich@xxxxxxxx, viktor.kleinik@xxxxxxxxxxxxxxx
- Delivery-date: Sat, 15 Mar 2014 22:43:21 +0000
- List-id: Xen developer discussion <xen-devel.lists.xen.org>
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
spin_lock(&p2m->lock);
if ( d != current->domain )
@@ -367,9 +382,23 @@ static int apply_p2m_changes(struct domain *d,
maddr += PAGE_SIZE;
}
break;
- 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 ==
p2m_invalid.
The important bit is the MFN. I don't think we care about "t".
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
