From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes Date: Sat, 15 Mar 2014 22:42:57 +0000 Message-ID: <5324D771.1010802@linaro.org> References: <1394914286-29713-1-git-send-email-avanzini.arianna@gmail.com> <1394914286-29713-3-git-send-email-avanzini.arianna@gmail.com> <5324D1F8.1080302@linaro.org> <5324D609.5080709@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5324D609.5080709@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Arianna Avanzini , xen-devel@lists.xen.org Cc: julien.grall@citrix.com, paolo.valente@unimore.it, keir@xen.org, stefano.stabellini@eu.citrix.com, tim@xen.org, dario.faggioli@citrix.com, Ian.Jackson@eu.citrix.com, Ian.Campbell@eu.citrix.com, etrudeau@broadcom.com, JBeulich@suse.com, viktor.kleinik@globallogic.com List-Id: xen-devel@lists.xenproject.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