From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH for-4.5 v11 4/9] xen/arm: Data abort
 exception (R/W) mem_events.
Date: Mon, 29 Sep 2014 13:52:43 +0100
Message-ID: <5429561B.3000803@linaro.org>
References: <1411990609-22374-1-git-send-email-tklengyel@sec.in.tum.de>	<1411990609-22374-5-git-send-email-tklengyel@sec.in.tum.de>	<54295214.6030902@linaro.org>
	<CAErYnsg-z8=tw9BTb3Mrfhv8AjA_MJyQc5M-73ZMW+zi8zyZhw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <CAErYnsg-z8=tw9BTb3Mrfhv8AjA_MJyQc5M-73ZMW+zi8zyZhw@mail.gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Tamas K Lengyel <tamas.lengyel@zentific.com>
Cc: wei.liu2@citrix.com, Ian Campbell <ian.campbell@citrix.com>, Tim Deegan <tim@xen.org>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>, Stefano Stabellini <stefano.stabellini@citrix.com>, Daniel De Graaf <dgdegra@tycho.nsa.gov>, Tamas K Lengyel <tklengyel@sec.in.tum.de>
List-Id: xen-devel@lists.xenproject.org

On 09/29/2014 01:47 PM, Tamas K Lengyel wrote:
>     > +/* Set access type for a region of pfns.
>     > + * If start_pfn == -1ul, sets the default access type */
>     > +long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
>     > +                        uint32_t start, uint32_t mask, xenmem_access_t access)
>     > +{
> 
> 
>     [..]
> 
>     > +
>     > +    rc = apply_p2m_changes(d, MEMACCESS,
>     > +                           pfn_to_paddr(pfn+start), pfn_to_paddr(pfn+nr),
>     > +                           0, MATTR_MEM, mask, 0, a);
>     > +
>     > +    flush_tlb_domain(d);
>     > +    iommu_iotlb_flush(d, pfn+start, nr-start);
> 
>     With your solution, when rc == 0 (i.e the call memaccess has been fully
>     applied), you will have one more TLB flush: one in apply_p2m_changes,
>     the other one here...
> 
> 
> No, I don't flush it in apply_p2m_changes, *flush is not set to true in
> MEMACCESS in this version.

Oh right, sorry I haven't noticed it.

>     As this code already exists in apply_p2m_changes but in the wrong place,
>     why didn't you move it later?
> 
> 
> The problem is with the preemption case that just goes to out. I found
> it cleaner to just flush the tlb here for both cases instead of having
> the preemption case going to a flush: label then to out. If that's
> preferred, I'm OK with that approach too.

What is the issue to do?

out:
   if ( flush )
   {
      TLB flush
   }

   if ( rc < 0 && ( op == INSERT ....

   ...

   return rc;

I would prefer if you have to duplicate the flush here and at the same
time you will fix other flush issue in the different path in case of error.

Regards,

-- 
Julien Grall