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:19:36 +0000 Message-ID: <5324D1F8.1080302@linaro.org> References: <1394914286-29713-1-git-send-email-avanzini.arianna@gmail.com> <1394914286-29713-3-git-send-email-avanzini.arianna@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1394914286-29713-3-git-send-email-avanzini.arianna@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 Hello Arianna, Thanks for the patch. On 15/03/14 20:11, Arianna Avanzini wrote: > --- > xen/arch/arm/p2m.c | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index d00c882..47bf154 100644 > --- a/xen/arcah/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -243,7 +243,8 @@ static int apply_p2m_changes(struct domain *d, > int rc; > struct p2m_domain *p2m = &d->arch.p2m; > lpae_t *first = NULL, *second = NULL, *third = NULL; > - paddr_t addr; > + p2m_type_t _t; > + paddr_t addr, _maddr = INVALID_PADDR; > unsigned long cur_first_page = ~0, > cur_first_offset = ~0, > cur_second_offset = ~0; > @@ -252,6 +253,20 @@ static int apply_p2m_changes(struct domain *d, > bool_t populate = (op == INSERT || op == ALLOCATE); > lpae_t pte; > > + /* > + * As of now, the lookup is needed only in in case > + * of REMOVE operation, as a consistency check on > + * the existence of a mapping between the machine > + * address and the start guest address given as > + * parameters. > + */ > + if (op == REMOVE) > + /* > + * Be sure to lookup before grabbing the p2m_lock, > + * as the p2m_lookup() function holds it too. > + */ > + _maddr = p2m_lookup(d, start_gpaddr, &_t); > + 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. 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). > + maddr != _maddr ) maddr is not incremented during where the page is removed. The next iteration will likely fail. You need to increment it in various place. > + { > + count++; > + break; IHMO, skipping the page is totally wrong. You should return an error here. Regards, -- Julien Grall