Re: [PATCH v3 4/4] iommu/vt-d: Introduce batched cache invalidation

[Baolu Lu <baolu.lu@linux.intel.com>]

On 8/19/24 11:40 PM, Jacob Pan wrote:
> On Sat, 17 Aug 2024 11:28:21 +0800
> Baolu Lu<baolu.lu@linux.intel.com>  wrote:
> 
>> On 2024/8/17 0:38, Jacob Pan wrote:
>>> On Thu, 15 Aug 2024 14:52:21 +0800
>>> Tina Zhang<tina.zhang@intel.com>  wrote:
>>>    
>>>> @@ -270,7 +343,8 @@ static void cache_tag_flush_iotlb(struct
>>>> dmar_domain *domain, struct cache_tag * u64 type =
>>>> DMA_TLB_PSI_FLUSH;
>>>>    	if (domain->use_first_level) {
>>>> -		qi_flush_piotlb(iommu, tag->domain_id, tag->pasid,
>>>> addr, pages, ih);
>>>> +		qi_batch_add_piotlb(iommu, tag->domain_id,
>>>> tag->pasid, addr,
>>>> +				    pages, ih, domain->qi_batch);
>>>>    		return;
>>>>    	}
>>>>    
>>>> @@ -287,7 +361,8 @@ static void cache_tag_flush_iotlb(struct
>>>> dmar_domain *domain, struct cache_tag * }
>>>>    
>>>>    	if (ecap_qis(iommu->ecap))
>>>> -		qi_flush_iotlb(iommu, tag->domain_id, addr | ih,
>>>> mask, type);
>>>> +		qi_batch_add_iotlb(iommu, tag->domain_id, addr |
>>>> ih, mask, type,
>>>> +				   domain->qi_batch);
>>>>      
>>> If I understand this correctly, IOTLB flush maybe deferred until the
>>> batch array is full, right? If so, is there a security gap where
>>> callers think the mapping is gone after the call returns?
>> No. All related caches are flushed before function return. A domain
>> can have multiple cache tags. Previously, we sent individual cache
>> invalidation requests to hardware. This change combines all necessary
>> invalidation requests into a single batch and raise them to hardware
>> together to make it more efficient.
> I was looking at the code below, if the index does not reach
> QI_MAX_BATCHED_DESC_COUNT. There will be no flush after
> cache_tag_flush_iotlb() returns, right?

No. qi_batch_flush_descs() is called explicitly before return.

@@ -341,6 +417,7 @@ static void cache_tag_flush_devtlb_all(struct 
dmar_domain *domain, struct cache_
  void cache_tag_flush_range(struct dmar_domain *domain, unsigned long 
start,
                            unsigned long end, int ih)
  {
+       struct intel_iommu *iommu = NULL;
         unsigned long pages, mask, addr;
         struct cache_tag *tag;
         unsigned long flags;
@@ -349,6 +426,10 @@ void cache_tag_flush_range(struct dmar_domain 
*domain, unsigned long start,

         spin_lock_irqsave(&domain->cache_lock, flags);
         list_for_each_entry(tag, &domain->cache_tags, node) {
+               if (iommu && iommu != tag->iommu)
+                       qi_batch_flush_descs(iommu, domain->qi_batch);
+               iommu = tag->iommu;
+
                 switch (tag->type) {
                 case CACHE_TAG_IOTLB:
                 case CACHE_TAG_NESTING_IOTLB:
@@ -372,6 +453,7 @@ void cache_tag_flush_range(struct dmar_domain 
*domain, unsigned long start,

                 trace_cache_tag_flush_range(tag, start, end, addr, 
pages, mask);
         }
+       qi_batch_flush_descs(iommu, domain->qi_batch);
         spin_unlock_irqrestore(&domain->cache_lock, flags);
  }

Thanks,
baolu