Re: [RFC] arm/p2m: call iommu iotlb flush if iommu exists and enabled

From: Paul Durrant <Paul.Durrant@citrix.com>
To: 'Julien Grall' <julien.grall@arm.com>,
	Andrii Anisov <andrii.anisov@gmail.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Andrii Anisov <andrii_anisov@epam.com>
Subject: Re: [RFC] arm/p2m: call iommu iotlb flush if iommu exists and enabled
Date: Wed, 23 Jan 2019 11:45:18 +0000	[thread overview]
Message-ID: <68e806f1d1b7496fb3e948576508bd86@AMSPEX02CL03.citrite.net> (raw)
In-Reply-To: <c861a2f4-25a4-e5a4-f112-42853cba84e3@arm.com>

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 23 January 2019 11:34
> To: Andrii Anisov <andrii.anisov@gmail.com>; xen-
> devel@lists.xenproject.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Andrii Anisov
> <andrii_anisov@epam.com>; Paul Durrant <Paul.Durrant@citrix.com>
> Subject: Re: [RFC] arm/p2m: call iommu iotlb flush if iommu exists and
> enabled
> 
> (+ Paul)
> 
> Hello,
> 
> On 23/01/2019 10:12, Andrii Anisov wrote:
> > From: Andrii Anisov <andrii_anisov@epam.com>
> >
> > Taking decission by `need_iommu_pt_sync()` make us never kicking
> 
> s/decission/decision/
> 
> > `iommu_iotlb_flush()` for IOMMUs which do share TLB with CPU.
> 
> I am not aware of platform where we share the TLB with the CPU. Do you
> mean
> sharing the P2M?
> 
> > So check `has_iommu_pt()` instead.
> >
> > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> >
> > ---
> >
> > Julien,
> >
> > Could you please look at this, IMO there is a mistake here.
> > x86 uses `need_iommu_pt_sync()` to make decission if iommu's map/unmap
> should be additionally called.
> > But ARM has no non-shared pt support in the mainline, so using
> `need_iommu_pt_sync()` seems to be odd.
> >
> >   xen/arch/arm/p2m.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 2394f97..059a391 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -1019,7 +1019,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
> >            !mfn_eq(lpae_get_mfn(*entry), lpae_get_mfn(orig_pte)) )
> >           p2m_free_entry(p2m, orig_pte, level);
> >
> > -    if ( need_iommu_pt_sync(p2m->domain) &&
> > +    if ( has_iommu_pt(p2m->domain) &&
> 
> I think this makes sense because we want to flush the TLB when the P2M is
> shared. Although, I would like to hear Paul's opinion here.

Yes, this was a mistake when moving from the old macros and need_iommu. Andrii is correct that need_iommu_pt_sync() is supposed to gate whether an explicit map/unmap is needed. The need for flush should only depend on has_iommu_pt(). There is also iommu_use_hap_pt() which is actually defined as has_iommu_pt(), but I wonder whether that should just go away for ARM since the page tables are always shared.

  Paul

> 
> >            (lpae_is_valid(orig_pte) || lpae_is_valid(*entry)) )
> >       {
> >           unsigned int flush_flags = 0; >
> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel