| Message ID | 8a8dd03a-5447-bc45-1554-50fb5b6c075c@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | (mainly) IOMMU hook adjustments | expand |
On 01/12/2021 09:41, Jan Beulich wrote: > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -591,7 +591,8 @@ static int __must_check iommu_flush_all( > bool_t flush_dev_iotlb; > int rc = 0; > > - flush_all_cache(); > + flush_local(FLUSH_CACHE); While I agree that the conversion is an improvement, the logic still looks totally bogus. I can believe that it might have been a stopgap to fix problems before we identified the need for sync_cache() for non-coherent IOMMUs, but there's no need I can spot for any WBINVDs on any of these paths. I'm fairly sure this should just be dropped, and Xen will get faster as a result. ~Andrew
On 01.12.2021 14:02, Andrew Cooper wrote: > On 01/12/2021 09:41, Jan Beulich wrote: >> --- a/xen/drivers/passthrough/vtd/iommu.c >> +++ b/xen/drivers/passthrough/vtd/iommu.c >> @@ -591,7 +591,8 @@ static int __must_check iommu_flush_all( >> bool_t flush_dev_iotlb; >> int rc = 0; >> >> - flush_all_cache(); >> + flush_local(FLUSH_CACHE); > > While I agree that the conversion is an improvement, the logic still > looks totally bogus. > > I can believe that it might have been a stopgap to fix problems before > we identified the need for sync_cache() for non-coherent IOMMUs, but > there's no need I can spot for any WBINVDs on any of these paths. > > I'm fairly sure this should just be dropped, and Xen will get faster as > a result. Kevin, thoughts? I have to admit I'm hesitant to remove such code, when there's no clear indication why it's there. I'm also not sure how much of a win the dropping would be, considering the places where this function gets called from. Jan
> From: Jan Beulich <jbeulich@suse.com> > Sent: Thursday, December 2, 2021 4:48 PM > > On 01.12.2021 14:02, Andrew Cooper wrote: > > On 01/12/2021 09:41, Jan Beulich wrote: > >> --- a/xen/drivers/passthrough/vtd/iommu.c > >> +++ b/xen/drivers/passthrough/vtd/iommu.c > >> @@ -591,7 +591,8 @@ static int __must_check iommu_flush_all( > >> bool_t flush_dev_iotlb; > >> int rc = 0; > >> > >> - flush_all_cache(); > >> + flush_local(FLUSH_CACHE); > > > > While I agree that the conversion is an improvement, the logic still > > looks totally bogus. > > > > I can believe that it might have been a stopgap to fix problems before > > we identified the need for sync_cache() for non-coherent IOMMUs, but > > there's no need I can spot for any WBINVDs on any of these paths. > > > > I'm fairly sure this should just be dropped, and Xen will get faster as > > a result. > > Kevin, thoughts? I have to admit I'm hesitant to remove such code, when > there's no clear indication why it's there. I'm also not sure how much > of a win the dropping would be, considering the places where this > function gets called from. > me too. Could Andrew elaborate further on "fairly sure" part? Thanks Kevin
--- a/xen/drivers/passthrough/vtd/extern.h +++ b/xen/drivers/passthrough/vtd/extern.h @@ -76,8 +76,6 @@ int __must_check qinval_device_iotlb_syn struct pci_dev *pdev, u16 did, u16 size, u64 addr); -void flush_all_cache(void); - uint64_t alloc_pgtable_maddr(unsigned long npages, nodeid_t node); void free_pgtable_maddr(u64 maddr); void *map_vtd_domain_page(u64 maddr); --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -591,7 +591,8 @@ static int __must_check iommu_flush_all( bool_t flush_dev_iotlb; int rc = 0; - flush_all_cache(); + flush_local(FLUSH_CACHE); + for_each_drhd_unit ( drhd ) { int context_rc, iotlb_rc; --- a/xen/drivers/passthrough/vtd/x86/vtd.c +++ b/xen/drivers/passthrough/vtd/x86/vtd.c @@ -46,8 +46,3 @@ void unmap_vtd_domain_page(const void *v { unmap_domain_page(va); } - -void flush_all_cache() -{ - wbinvd(); -}
Let's use infrastructure we have available instead of an open-coded wbinvd() invocation. Signed-off-by: Jan Beulich <jbeulich@suse.com>