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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@xxxxxxx]
> Sent: 23 January 2019 11:34
> To: Andrii Anisov <andrii.anisov@xxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Andrii Anisov
> <andrii_anisov@xxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> 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@xxxxxxxx>
> >
> > 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@xxxxxxxx>
> >
> > ---
> >
> > 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel