* [PATCH] IOMMU: avoid double flushing in shared page table case
@ 2020-10-20 13:52 Jan Beulich
2020-10-20 15:00 ` Julien Grall
2020-10-23 6:28 ` Tian, Kevin
0 siblings, 2 replies; 5+ messages in thread
From: Jan Beulich @ 2020-10-20 13:52 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Wei Liu,
Stefano Stabellini, Kevin Tian, Jun Nakajima, Paul Durrant
While the flush coalescing optimization has been helping the non-shared
case, it has actually lead to double flushes in the shared case (which
ought to be the more common one nowadays at least): Once from
*_set_entry() and a second time up the call tree from wherever the
overriding flag gets played with. In alignment with XSA-346 suppress
flushing in this case.
Similarly avoid excessive setting of IOMMU_FLUSHF_added on the batched
flushes: "idx" hasn't been added a new mapping for.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: The Arm part really is just for completeness (and hence could also
be dropped) - the affected mapping spaces aren't currently
supported there.
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1045,7 +1045,7 @@ static int __p2m_set_entry(struct p2m_do
p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, sgfn);
}
- if ( is_iommu_enabled(p2m->domain) &&
+ if ( is_iommu_enabled(p2m->domain) && !this_cpu(iommu_dont_flush_iotlb) &&
(lpae_is_valid(orig_pte) || lpae_is_valid(*entry)) )
{
unsigned int flush_flags = 0;
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -842,7 +842,7 @@ out:
if ( rc == 0 && p2m_is_hostp2m(p2m) &&
need_modify_vtd_table )
{
- if ( iommu_use_hap_pt(d) )
+ if ( iommu_use_hap_pt(d) && !this_cpu(iommu_dont_flush_iotlb) )
rc = iommu_iotlb_flush(d, _dfn(gfn), 1ul << order,
(iommu_flags ? IOMMU_FLUSHF_added : 0) |
(vtd_pte_present ? IOMMU_FLUSHF_modified
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -870,7 +870,7 @@ int xenmem_add_to_physmap(struct domain
this_cpu(iommu_dont_flush_iotlb) = 0;
ret = iommu_iotlb_flush(d, _dfn(xatp->idx - done), done,
- IOMMU_FLUSHF_added | IOMMU_FLUSHF_modified);
+ IOMMU_FLUSHF_modified);
if ( unlikely(ret) && rc >= 0 )
rc = ret;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] IOMMU: avoid double flushing in shared page table case
2020-10-20 13:52 [PATCH] IOMMU: avoid double flushing in shared page table case Jan Beulich
@ 2020-10-20 15:00 ` Julien Grall
2020-10-20 15:05 ` Jan Beulich
2020-10-23 6:28 ` Tian, Kevin
1 sibling, 1 reply; 5+ messages in thread
From: Julien Grall @ 2020-10-20 15:00 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu,
Stefano Stabellini, Kevin Tian, Jun Nakajima, Paul Durrant
Hi Jan,
On 20/10/2020 14:52, Jan Beulich wrote:
> While the flush coalescing optimization has been helping the non-shared
> case, it has actually lead to double flushes in the shared case (which
> ought to be the more common one nowadays at least): Once from
> *_set_entry() and a second time up the call tree from wherever the
> overriding flag gets played with. In alignment with XSA-346 suppress
> flushing in this case.
>
> Similarly avoid excessive setting of IOMMU_FLUSHF_added on the batched
> flushes: "idx" hasn't been added a new mapping for.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: The Arm part really is just for completeness (and hence could also
> be dropped) - the affected mapping spaces aren't currently
> supported there.
As I may I have pointed out in the past, there are many ways to screw
things up when using iommu_dont_flush_iotlb.
So I would rather not introduce any usage on Arm until we see a use-case.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] IOMMU: avoid double flushing in shared page table case
2020-10-20 15:00 ` Julien Grall
@ 2020-10-20 15:05 ` Jan Beulich
2020-10-20 15:26 ` Julien Grall
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2020-10-20 15:05 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu,
Stefano Stabellini, Kevin Tian, Jun Nakajima, Paul Durrant
On 20.10.2020 17:00, Julien Grall wrote:
> On 20/10/2020 14:52, Jan Beulich wrote:
>> While the flush coalescing optimization has been helping the non-shared
>> case, it has actually lead to double flushes in the shared case (which
>> ought to be the more common one nowadays at least): Once from
>> *_set_entry() and a second time up the call tree from wherever the
>> overriding flag gets played with. In alignment with XSA-346 suppress
>> flushing in this case.
>>
>> Similarly avoid excessive setting of IOMMU_FLUSHF_added on the batched
>> flushes: "idx" hasn't been added a new mapping for.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> TBD: The Arm part really is just for completeness (and hence could also
>> be dropped) - the affected mapping spaces aren't currently
>> supported there.
>
> As I may I have pointed out in the past, there are many ways to screw
> things up when using iommu_dont_flush_iotlb.
>
> So I would rather not introduce any usage on Arm until we see a use-case.
"Usage" to me would mean a path actually setting the flag.
What I'm adding here, basically as a precautionary measure,
is a check of the flag. Does your use of "usage" imply you
don't want that either? Just to be sure; I'm okay to drop
the Arm part.
Jan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] IOMMU: avoid double flushing in shared page table case
2020-10-20 15:05 ` Jan Beulich
@ 2020-10-20 15:26 ` Julien Grall
0 siblings, 0 replies; 5+ messages in thread
From: Julien Grall @ 2020-10-20 15:26 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu,
Stefano Stabellini, Kevin Tian, Jun Nakajima, Paul Durrant
Hi Jan,
On 20/10/2020 16:05, Jan Beulich wrote:
> On 20.10.2020 17:00, Julien Grall wrote:
>> On 20/10/2020 14:52, Jan Beulich wrote:
>>> While the flush coalescing optimization has been helping the non-shared
>>> case, it has actually lead to double flushes in the shared case (which
>>> ought to be the more common one nowadays at least): Once from
>>> *_set_entry() and a second time up the call tree from wherever the
>>> overriding flag gets played with. In alignment with XSA-346 suppress
>>> flushing in this case.
>>>
>>> Similarly avoid excessive setting of IOMMU_FLUSHF_added on the batched
>>> flushes: "idx" hasn't been added a new mapping for.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> TBD: The Arm part really is just for completeness (and hence could also
>>> be dropped) - the affected mapping spaces aren't currently
>>> supported there.
>>
>> As I may I have pointed out in the past, there are many ways to screw
>> things up when using iommu_dont_flush_iotlb.
>>
>> So I would rather not introduce any usage on Arm until we see a use-case.
>
> "Usage" to me would mean a path actually setting the flag.
> What I'm adding here, basically as a precautionary measure,
> is a check of the flag.
The code would always be safe without checking the safe (albeit doing
pointless flush). I wouldn't say the same if we check the flag because
it is not correct to set it everywhere.
> Does your use of "usage" imply you
> don't want that either? Just to be sure; I'm okay to drop
> the Arm part.
That's correct, I don't want this check to be present until there is a
user. Only then, we can assess whether this is the right approach for Arm.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] IOMMU: avoid double flushing in shared page table case
2020-10-20 13:52 [PATCH] IOMMU: avoid double flushing in shared page table case Jan Beulich
2020-10-20 15:00 ` Julien Grall
@ 2020-10-23 6:28 ` Tian, Kevin
1 sibling, 0 replies; 5+ messages in thread
From: Tian, Kevin @ 2020-10-23 6:28 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Cooper, Andrew, George Dunlap, Ian Jackson, Julien Grall,
Wei Liu, Stefano Stabellini, Nakajima, Jun, Paul Durrant
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, October 20, 2020 9:53 PM
>
> While the flush coalescing optimization has been helping the non-shared
> case, it has actually lead to double flushes in the shared case (which
> ought to be the more common one nowadays at least): Once from
> *_set_entry() and a second time up the call tree from wherever the
> overriding flag gets played with. In alignment with XSA-346 suppress
> flushing in this case.
>
> Similarly avoid excessive setting of IOMMU_FLUSHF_added on the batched
> flushes: "idx" hasn't been added a new mapping for.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
> TBD: The Arm part really is just for completeness (and hence could also
> be dropped) - the affected mapping spaces aren't currently
> supported there.
>
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1045,7 +1045,7 @@ static int __p2m_set_entry(struct p2m_do
> p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, sgfn);
> }
>
> - if ( is_iommu_enabled(p2m->domain) &&
> + if ( is_iommu_enabled(p2m->domain)
> && !this_cpu(iommu_dont_flush_iotlb) &&
> (lpae_is_valid(orig_pte) || lpae_is_valid(*entry)) )
> {
> unsigned int flush_flags = 0;
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -842,7 +842,7 @@ out:
> if ( rc == 0 && p2m_is_hostp2m(p2m) &&
> need_modify_vtd_table )
> {
> - if ( iommu_use_hap_pt(d) )
> + if ( iommu_use_hap_pt(d) && !this_cpu(iommu_dont_flush_iotlb) )
> rc = iommu_iotlb_flush(d, _dfn(gfn), 1ul << order,
> (iommu_flags ? IOMMU_FLUSHF_added : 0) |
> (vtd_pte_present ? IOMMU_FLUSHF_modified
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -870,7 +870,7 @@ int xenmem_add_to_physmap(struct domain
> this_cpu(iommu_dont_flush_iotlb) = 0;
>
> ret = iommu_iotlb_flush(d, _dfn(xatp->idx - done), done,
> - IOMMU_FLUSHF_added | IOMMU_FLUSHF_modified);
> + IOMMU_FLUSHF_modified);
> if ( unlikely(ret) && rc >= 0 )
> rc = ret;
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-23 6:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-20 13:52 [PATCH] IOMMU: avoid double flushing in shared page table case Jan Beulich
2020-10-20 15:00 ` Julien Grall
2020-10-20 15:05 ` Jan Beulich
2020-10-20 15:26 ` Julien Grall
2020-10-23 6:28 ` Tian, Kevin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).