From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events. Date: Thu, 12 Mar 2015 16:35:47 +0000 Message-ID: <5501C063.9080101@linaro.org> References: <1425677073-13729-1-git-send-email-tklengyel@sec.in.tum.de> <1425677073-13729-5-git-send-email-tklengyel@sec.in.tum.de> <1426167305.21353.423.camel@citrix.com> <1426174554.32572.16.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1426174554.32572.16.camel@citrix.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: Ian Campbell , Tamas K Lengyel Cc: wei.liu2@citrix.com, Stefano Stabellini , Tim Deegan , Ian Jackson , xen-devel@lists.xen.org, stefano.stabellini@citrix.com, Jan Beulich , Keir Fraser List-Id: xen-devel@lists.xenproject.org Hi Ian, On 12/03/15 15:35, Ian Campbell wrote: > On Thu, 2015-03-12 at 16:19 +0100, Tamas K Lengyel wrote: >> >>> out: >> > + if ( flush ) >> > + { >> > + flush_tlb_domain(d); >> > + iommu_iotlb_flush(d, sgfn, egfn - sgfn); >> > + } >> >> Is moving the flush out of the loop an independent bug >> fix? If so please >> do in a separate commit with a rationale in the commit >> log. If it is >> somehow related to the changes here then please >> mention it in this >> commit log, since it's a bit subtle. >> >> >> Right, it's not a bugfix and not required to be outside the >> loop, I think I just moved it because it made sense to me to >> flush it only once instead at every iteration. I'll place it >> back. > >> >> Sorry, the flush wasn't actually part of the loop to begin with. I >> just moved it under the label out so that the TLB gets flushed when >> the memaccess setting hypercall gets preempted. I will just set a >> separate label for it before out so that the existing behavior is >> preserved but the tlb is still flushed when memaccess is preempted. > > I wonder why this isn't needed for the other uses of goto out, i.e. on > the relinquish check. relinquish is only done when the domain is destroyed. So the flush is not strictly necessary :). > I'm not convinced this isn't just a straight up bug. Anyone remember any > reasoning why we don't flush on exit if any work has been done? Actually the flush is necessary is 3 cases: - ALLOCATE - INSERT - REMOVE I don't count RELINQUISH because the guest is not scheduled anymore when it happens. REMOVE can never fail. In the case of ALLOCATE/INSERT, we redo the mapping (REMOVE) which will issue a flush. So for now we are safe. But I think, in general, the flush would be better after the out label. It would avoid missing flushing if one day we decide to implement preemption on more use-case (such as INSERT/ALLOCATE). Regards, -- Julien Grall