| Message ID | e54f4fbb-92e2-9785-8648-596c615213a2@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IOMMU: avoid double flushing in shared page table case | expand |
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,
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
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,
> 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; >
--- 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;
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.