Re: [PATCH 2/8] iommu/vt-d: Add entry_sync support for PASID entry updates

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

On 3/9/26 21:41, Jason Gunthorpe wrote:
>> +static void intel_pasid_sync(struct entry_sync_writer128 *writer)
>> +{
>> +	struct intel_pasid_writer *p_writer = container_of(writer,
>> +			struct intel_pasid_writer, writer);
>> +	struct intel_iommu *iommu = p_writer->iommu;
>> +	struct device *dev = p_writer->dev;
>> +	bool was_present, is_present;
>> +	u32 pasid = p_writer->pasid;
>> +	struct pasid_entry *pte;
>> +	u16 old_did, old_pgtt;
>> +
>> +	pte = intel_pasid_get_entry(dev, pasid);
>> +	was_present = p_writer->was_present;
>> +	is_present = pasid_pte_is_present(pte);
>> +	old_did = pasid_get_domain_id(&p_writer->orig_pte);
>> +	old_pgtt = pasid_pte_get_pgtt(&p_writer->orig_pte);
>> +
>> +	/* Update the last present state: */
>> +	p_writer->was_present = is_present;
>> +
>> +	if (!ecap_coherent(iommu->ecap))
>> +		clflush_cache_range(pte, sizeof(*pte));
>> +
>> +	/* Sync for "P=0" to "P=1": */
>> +	if (!was_present) {
>> +		if (is_present)
>> +			pasid_flush_caches(iommu, pte, pasid,
>> +					   pasid_get_domain_id(pte));
>> +
>> +		return;
>> +	}
>> +
>> +	/* Sync for "P=1" to "P=1": */
>> +	if (is_present) {
>> +		intel_pasid_flush_present(iommu, dev, pasid, old_did, pte);
>> +		return;
>> +	}
>> +
>> +	/* Sync for "P=1" to "P=0": */
>> +	pasid_cache_invalidation_with_pasid(iommu, old_did, pasid);
> Why all this logic? All this different stuff does is meddle with the
> IOTLB and it should not seen below.
> 
> If the sync is called it should just always call
> pasid_cache_invalidation_with_pasid(), that's it.
> 
> Writer has already eliminated all cases where sync isn't needed.

You're right. The library should simplify things. I will remove the
state tracking. The callback will only ensure that memory is flushed
(for non-coherent mode) and the relevant PASID cache is invalidated.

> 
>> +	if (old_pgtt == PASID_ENTRY_PGTT_PT || old_pgtt == PASID_ENTRY_PGTT_FL_ONLY)
>> +		qi_flush_piotlb(iommu, old_did, pasid, 0, -1, 0);
>> +	else
>> +		iommu->flush.flush_iotlb(iommu, old_did, 0, 0, DMA_TLB_DSI_FLUSH);
>> +	devtlb_invalidation_with_pasid(iommu, dev, pasid);
> The IOTLB should already be clean'd before the new entry using the
> cache tag is programmed. Cleaning it after the entry is live is buggy.
 > > The writer logic ensures it never sees a corrupted entry, so the clean
> cache tag cannot be mangled during the writing process.
> 
> The way ARM is structured has the cache tags clean if they are in the
> allocator bitmap, so when the driver fetches a new tag and starts
> using it is clean and non cleaning is needed
> 
> When it frees a tag it cleans it and then returns it to the allocator.

If I understand your remark correctly, the driver should only need the
following in the sync callback:

- clflush (if non-coherent) to ensure the entry is in physical memory.
- PASID cache invalidation to force the hardware to re-read the entry.
- Device-TLB invalidation to drop local device caches.

Does that sound right? I can move the general IOTLB/PIOTLB invalidation
logic to the domain detach/free paths.

> ATC invalidations should always be done after the PASID entry is
> written. During a hitless update both translations are unpredictably
> combined, this is unavoidable and OK.

The VT-d spec (Sections 6.5.2.5 and 6.5.2.6) explicitly mandates that an
IOTLB invalidation must precede the Device-TLB invalidation. If we only
do the device-TLB invalidation in the sync callback, we risk the device
re-fetching a stale translation from the IOMMU's internal IOTLB.

Thanks,
baolu