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

[Jacob Pan <jacob.pan@linux.microsoft.com>]

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?

+static void qi_batch_increment_index(struct
intel_iommu *iommu, struct qi_batch *batch) +{
+	if (++batch->index == QI_MAX_BATCHED_DESC_COUNT)
+		qi_batch_flush_descs(iommu, batch);
+}
+
+static void qi_batch_add_iotlb(struct intel_iommu *iommu, u16 did, u64
addr,
+			       unsigned int size_order, u64 type,
+			       struct qi_batch *batch)
+{
+	qi_desc_iotlb(iommu, did, addr, size_order, type,
&(batch->descs[batch->index]));
+	qi_batch_increment_index(iommu, batch);
+}

> Thanks,
> baolu