RE: [PATCH 02/12] iommu/vt-d: Add cache tag invalidation helpers

["Tian, Kevin" <kevin.tian@intel.com>]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Sunday, April 7, 2024 1:33 PM
> 
> On 3/28/24 3:39 PM, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@linux.intel.com>
> >> Sent: Monday, March 25, 2024 10:17 AM
> >>
> >> +/*
> >> + * Invalidate a range of IOVA when IOMMU is in caching mode and new
> >> mappings
> >> + * are added to the target domain.
> >> + */
> >> +void cache_tag_flush_cm_range(struct dmar_domain *domain, unsigned
> >> long start,
> >> +			      unsigned long end)
> >> +{
> >
> > I'm also not sure why this is worth a separate helper. why couldn't it
> > be managed by previous flush_range()?
> This is only my preference. I'd like to separate things belonging to
> different paths, so that it's easier for maintenance. For example, if,
> in the future, we need to add or enhance something for a specific case,
> we don't need to care about other cases.

IMHO caching mode is an attribute in low level iommu which can be
handled perfectly well within the helper by checking that attribute.

it sounds a bit weird for the caller to know that detail and call different
helpers when all paths just want to request to flush a specific range.

> >> +
> >> +		if (tag->type == CACHE_TAG_TYPE_IOTLB ||
> >> +		    tag->type == CACHE_TAG_TYPE_PARENT_IOTLB) {
> >> +			/*
> >> +			 * Fallback to domain selective flush if no
> >> +			 * PSI support or the size is too big.
> >> +			 */
> >> +			if (!cap_pgsel_inv(iommu->cap) ||
> >> +			    mask > cap_max_amask_val(iommu->cap))
> >> +				iommu->flush.flush_iotlb(iommu, tag-
> >>> domain_id,
> >> +							 0, 0,
> >> DMA_TLB_DSI_FLUSH);
> >> +			else
> >> +				iommu->flush.flush_iotlb(iommu, tag-
> >>> domain_id,
> >> +							 addr, mask,
> >> +
> >> DMA_TLB_PSI_FLUSH);
> >> +		}
> >> +	}
> >
> > You skipped devtlb invalidation. yes it's not necessary based on current
> > nested translation design and this part is inconsistent in different paths.
> >
> > but as a semantics change you may want to first make removing devtlb
> > invalidation a separate patch and then do this cleanup, so bisect is easier.
> 
> This helper is designed for map paths when the IOMMU is in caching mode.
> Caching mode doesn't impact the device TLB, so we should not invalidate
> the device TLB here.
> 
> I guess the confusing thing is about the code below.
> 
> 
>          /*
>           * In caching mode, changes of pages from non-present to
> present require
>           * flush. However, device IOTLB doesn't need to be flushed in
> this case.
>           */
>          if (!cap_caching_mode(iommu->cap) || !map)
>                  iommu_flush_dev_iotlb(domain, addr, mask);
> 
> The real code doesn't match the comments, and I think the comments
> describe the right thing. So perhaps the code should be changed to match
> the comments.
> 
> 	if (!map)
> 		iommu_flush_dev_iotlb(domain, addr, mask);
> 
> ??
> 

yes. that should be fixed separately.