From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v8 02/14] arch/arm: add consistency check to REMOVE p2m changes Date: Sun, 25 May 2014 16:50:03 +0100 Message-ID: <5382112B.9030406@linaro.org> References: <1401015115-7610-1-git-send-email-avanzini.arianna@gmail.com> <1401015115-7610-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: <1401015115-7610-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, andrew.cooper3@citrix.com, viktor.kleinik@globallogic.com List-Id: xen-devel@lists.xenproject.org Hi Arianna, On 25/05/14 11:51, Arianna Avanzini wrote: > + unsigned long mfn = pte.p2m.base; > + > + /* > + * Ensure that the guest address given as argument to > + * this function is actually mapped to the specified > + * machine address. maddr here is the machine address > + * given to the function, while mfn is the machine > + * frame number actually mapped to the guest address: > + * check if the two correspond. > + */ > + if ( !pte.p2m.valid || maddr != pfn_to_paddr(mfn) ) > + { > + printk("p2m_remove: nonexistent mapping: " > + "%"PRIx64" and %"PRIx64"\n", > + pfn_to_paddr(mfn), maddr); > + /* > + * If we have successfully removed other mappings, > + * overload flush local variable to store if we need > + * to flush TLBs. > + */ > + if (count) flush = 1; else flush = 0; Hrrm, why do you need this line? Flush is already correctly set previously (see flush |= ... few lines above). > + rc = -EINVAL; > + goto out_flush; As I said on a previous mail, you should continue to unmap even if we fail to remove one mapping. Otherwise, we may let the guest access to some MMIO region by mistake. > + } > + } > + /* fall through */ > + case RELINQUISH: > + { > if ( !pte.p2m.valid ) > { > count++; > @@ -462,28 +490,30 @@ static int apply_p2m_changes(struct domain *d, > > /* Got the next page */ > addr += PAGE_SIZE; > + maddr += PAGE_SIZE; > } > > - if ( flush ) > + if ( op == ALLOCATE || op == INSERT ) > { > unsigned long sgfn = paddr_to_pfn(start_gpaddr); > unsigned long egfn = paddr_to_pfn(end_gpaddr); > > - flush_tlb_domain(d); > - iommu_iotlb_flush(d, sgfn, egfn - sgfn); > + p2m->max_mapped_gfn = MAX(p2m->max_mapped_gfn, egfn); > + p2m->lowest_mapped_gfn = MIN(p2m->lowest_mapped_gfn, sgfn); > } > > - if ( op == ALLOCATE || op == INSERT ) > + rc = 0; > + > +out_flush: There is no need to create a new label. You can move the label "out" here and avoid to invert the position of the 2 if. It doesn't harm to update p2m->max_mapped_gfn and p2m->lowest_mapped_gfn and/or flush TLBs if we fail. It could also be safeguard for later :). Regards, -- Julien Grall