* [RFC] arm/p2m: call iommu iotlb flush if iommu exists and enabled
@ 2019-01-23 10:12 Andrii Anisov
2019-01-23 11:33 ` Julien Grall
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Anisov @ 2019-01-23 10:12 UTC (permalink / raw)
To: xen-devel, Julien Grall; +Cc: Stefano Stabellini, Andrii Anisov
From: Andrii Anisov <andrii_anisov@epam.com>
Taking decission by `need_iommu_pt_sync()` make us never kicking
`iommu_iotlb_flush()` for IOMMUs which do share TLB with CPU.
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) &&
(lpae_is_valid(orig_pte) || lpae_is_valid(*entry)) )
{
unsigned int flush_flags = 0;
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [RFC] arm/p2m: call iommu iotlb flush if iommu exists and enabled
2019-01-23 10:12 [RFC] arm/p2m: call iommu iotlb flush if iommu exists and enabled Andrii Anisov
@ 2019-01-23 11:33 ` Julien Grall
2019-01-23 11:45 ` Paul Durrant
2019-01-23 12:35 ` Andrii Anisov
0 siblings, 2 replies; 6+ messages in thread
From: Julien Grall @ 2019-01-23 11:33 UTC (permalink / raw)
To: Andrii Anisov, xen-devel; +Cc: Stefano Stabellini, Andrii Anisov, Paul Durrant
(+ 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.
> (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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC] arm/p2m: call iommu iotlb flush if iommu exists and enabled
2019-01-23 11:33 ` Julien Grall
@ 2019-01-23 11:45 ` Paul Durrant
2019-01-23 12:40 ` Andrii Anisov
2019-01-23 12:35 ` Andrii Anisov
1 sibling, 1 reply; 6+ messages in thread
From: Paul Durrant @ 2019-01-23 11:45 UTC (permalink / raw)
To: 'Julien Grall', Andrii Anisov, xen-devel
Cc: Stefano Stabellini, Andrii Anisov
> -----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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC] arm/p2m: call iommu iotlb flush if iommu exists and enabled
2019-01-23 11:45 ` Paul Durrant
@ 2019-01-23 12:40 ` Andrii Anisov
2019-01-23 12:41 ` Paul Durrant
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Anisov @ 2019-01-23 12:40 UTC (permalink / raw)
To: Paul Durrant, 'Julien Grall', xen-devel
Cc: Stefano Stabellini, Andrii Anisov
Hello Paul,
On 23.01.19 13:45, Paul Durrant wrote:
> 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().
So I fix the commit message and send V2 for XEN 4.12.
> 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.
Actually, the mainline currently supports only SMMU, which does share the page table.
But it is not always true, we are using HW, where PT is not shared. And going to upstream that support one day.
--
Sincerely,
Andrii Anisov.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] arm/p2m: call iommu iotlb flush if iommu exists and enabled
2019-01-23 12:40 ` Andrii Anisov
@ 2019-01-23 12:41 ` Paul Durrant
0 siblings, 0 replies; 6+ messages in thread
From: Paul Durrant @ 2019-01-23 12:41 UTC (permalink / raw)
To: 'Andrii Anisov', 'Julien Grall', xen-devel
Cc: Stefano Stabellini, Andrii Anisov
> -----Original Message-----
> From: Andrii Anisov [mailto:andrii.anisov@gmail.com]
> Sent: 23 January 2019 12:40
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Julien Grall'
> <julien.grall@arm.com>; 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
>
> Hello Paul,
>
> On 23.01.19 13:45, Paul Durrant wrote:
> > 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().
> So I fix the commit message and send V2 for XEN 4.12.
Cool.
>
> > 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.
> Actually, the mainline currently supports only SMMU, which does share the
> page table.
> But it is not always true, we are using HW, where PT is not shared. And
> going to upstream that support one day.
Oh, ok. Leave it in then :-)
Paul
>
> --
> Sincerely,
> Andrii Anisov.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] arm/p2m: call iommu iotlb flush if iommu exists and enabled
2019-01-23 11:33 ` Julien Grall
2019-01-23 11:45 ` Paul Durrant
@ 2019-01-23 12:35 ` Andrii Anisov
1 sibling, 0 replies; 6+ messages in thread
From: Andrii Anisov @ 2019-01-23 12:35 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Stefano Stabellini, Andrii Anisov, Paul Durrant
On 23.01.19 13:33, Julien Grall wrote:
> Do you mean sharing the P2M?
For sure!
--
Sincerely,
Andrii Anisov.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-23 12:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-23 10:12 [RFC] arm/p2m: call iommu iotlb flush if iommu exists and enabled Andrii Anisov
2019-01-23 11:33 ` Julien Grall
2019-01-23 11:45 ` Paul Durrant
2019-01-23 12:40 ` Andrii Anisov
2019-01-23 12:41 ` Paul Durrant
2019-01-23 12:35 ` Andrii Anisov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.