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

[Baolu Lu]

On 8/19/24 11:40 PM, Jacob Pan wrote:
> On Sat, 17 Aug 2024 11:28:21 +0800
> Baolu Lu<baolu.lu@xxxxxxxxxxxxxxx>  wrote:
>
> > On 2024/8/17 0:38, Jacob Pan wrote:
> > > On Thu, 15 Aug 2024 14:52:21 +0800
> > > Tina Zhang<tina.zhang@xxxxxxxxx>  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