Re: [PATCH 06/12] iommu/vt-d: Use cache_tag_flush_cm_range() in iotlb_sync_map

[Baolu Lu]

On 4/8/24 10:51 AM, Tian, Kevin wrote:
> > From: Baolu Lu<baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Sunday, April 7, 2024 2:41 PM
> >
> > On 3/28/24 3:48 PM, Tian, Kevin wrote:
> > > > From: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> > > > Sent: Monday, March 25, 2024 10:17 AM
> > > >
> > > > The iotlb_sync_map callback is called by the iommu core after non-
> > present
> > > > to present mappings are created. The iommu driver uses this callback to
> > > > invalidate caches if IOMMU is working in caching mode and second-only
> > > > translation is used for the domain. Use cache_tag_flush_cm_range() in
> > this
> > > > callback.
> > > >
> > > > Signed-off-by: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> > > > ---
> > > >    drivers/iommu/intel/iommu.c | 22 +---------------------
> > > >    1 file changed, 1 insertion(+), 21 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > > > index 1c03d2dafb9d..2dcab1e5cd4d 100644
> > > > --- a/drivers/iommu/intel/iommu.c
> > > > +++ b/drivers/iommu/intel/iommu.c
> > > > @@ -1504,20 +1504,6 @@ static void iommu_flush_iotlb_psi(struct
> > > > intel_iommu *iommu,
> > > >    		iommu_flush_dev_iotlb(domain, addr, mask);
> > > >    }
> > > >
> > > > -/* Notification for newly created mappings */
> > > > -static void __mapping_notify_one(struct intel_iommu *iommu, struct
> > > > dmar_domain *domain,
> > > > -				 unsigned long pfn, unsigned int pages)
> > > > -{
> > > > -	/*
> > > > -	 * It's a non-present to present mapping. Only flush if caching mode
> > > > -	 * and second level.
> > > > -	 */
> > > > -	if (cap_caching_mode(iommu->cap) && !domain->use_first_level)
> > > > -		iommu_flush_iotlb_psi(iommu, domain, pfn, pages, 0, 1);
> > > > -	else
> > > > -		iommu_flush_write_buffer(iommu);
> > > > -}
> > > iommu_flush_write_buffer is for a quite different issue. it's clearer to
> > > keep it separated from the iotlb helpers.
> > The VT-d spec describes the write buffer flushing in section 6.8. It
> > states,
> >
> > 1. Write buffer flushing is required only for earlier VT-d hardware
> >      implementations. Newer hardware implementations are expected to NOT
> >      require this.
> > 2. Write buffer flushing is related to cache invalidation. The hardware
> >      performs an implicit write-buffer-flushing as a pre-condition to
> >      the invalidation operations.
> >
> > According to this, write buffer flushing is only required in the map
> > path when IOMMU is not in caching mode and the RWBF iommu capability is
> > set, which perfectly matches the case of cache_tag_flush_cm_range().
> >
> > By doing it in cache_tag_flush_cm_range(), the code will be simpler and
> > easier for maintenance.
> but above applies actually when caching mode is false or when 1st
> stage is being used. this is the opposite of what flush_cm_range()
> implies.
>
> if we do it through a general flush_range() helper which accepts
> a parameter to indicate the map path, then it's reasonable to move
> this special handling to that helper as that is another trick which
> the caller doesn't need to know.

This could be an option. How about my proposal in another reply?

Best regards,
baolu