> -----Original Message-----
> From: Julien Grall [mailto:[email protected]]
> Sent: 23 January 2019 11:34
> To: Andrii Anisov <[email protected]>; xen-
> [email protected]
> Cc: Stefano Stabellini <[email protected]>; Andrii Anisov
> <[email protected]>; Paul Durrant <[email protected]>
> 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 <[email protected]>
> >
> > 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 <[email protected]>
> >
> > ---
> >
> > 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
