Re: [PATCH v3 19/19] iommu/intel: Access/Dirty bit support for SL domains

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

On 2023/10/17 19:22, Joao Martins wrote:
> On 17/10/2023 03:08, Baolu Lu wrote:
>> On 10/17/23 12:00 AM, Joao Martins wrote:
>>>>> The iommu_dirty_bitmap is defined in iommu core. The iommu driver has no
>>>>> need to understand it and check its member anyway.
>>>>>
>>>> (...) The iommu driver has no need to understand it. iommu_dirty_bitmap_record()
>>>> already makes those checks in case there's no iova_bitmap to set bits to.
>>>>
>>> This is all true but the reason I am checking iommu_dirty_bitmap::bitmap is to
>>> essentially not record anything in the iova bitmap and just clear the dirty bits
>>> from the IOPTEs, all when dirty tracking is technically disabled. This is done
>>> internally only when starting dirty tracking, and thus to ensure that we cleanup
>>> all dirty bits before we enable dirty tracking to have a consistent snapshot as
>>> opposed to inheriting dirties from the past.
>>
>> It's okay since it serves a functional purpose. Can you please add some
>> comments around the code to explain the rationale.
>>
> 
> I added this comment below:
> 
> +       /*
> +        * IOMMUFD core calls into a dirty tracking disabled domain without an
> +        * IOVA bitmap set in order to clean dirty bits in all PTEs that might
> +        * have occured when we stopped dirty tracking. This ensures that we
> +        * never inherit dirtied bits from a previous cycle.
> +        */
> 

Yes. It's clear. Thank you!

> Also fixed an issue where I could theoretically clear the bit with
> IOMMU_NO_CLEAR. Essentially passed the read_and_clear_dirty flags and let
> dma_sl_pte_test_and_clear_dirty() to test and test-and-clear, similar to AMD:
> 
> @@ -781,6 +788,16 @@ static inline bool dma_pte_present(struct dma_pte *pte)
>          return (pte->val & 3) != 0;
>   }
> 
> +static inline bool dma_sl_pte_test_and_clear_dirty(struct dma_pte *pte,
> +                                                  unsigned long flags)
> +{
> +       if (flags & IOMMU_DIRTY_NO_CLEAR)
> +               return (pte->val & DMA_SL_PTE_DIRTY) != 0;
> +
> +       return test_and_clear_bit(DMA_SL_PTE_DIRTY_BIT,
> +                                 (unsigned long *)&pte->val);
> +}
> +

Yes. Sure.

> Anyhow, see below the full diff compared to this patch. Some things are in tree
> that is different to submitted from this patch.

[...]

> @@ -4113,7 +4123,7 @@ static void intel_iommu_domain_free(struct iommu_domain
> *domain)
>   }
> 
>   static int prepare_domain_attach_device(struct iommu_domain *domain,
> -					struct device *dev)
> +					struct device *dev, ioasid_t pasid)

How about blocking pasid attaching in intel_iommu_set_dev_pasid().

>   {
>   	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>   	struct intel_iommu *iommu;
> @@ -4126,7 +4136,8 @@ static int prepare_domain_attach_device(struct
> iommu_domain *domain,
>   	if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap))
>   		return -EINVAL;
> 
> -	if (domain->dirty_ops && !intel_iommu_slads_supported(iommu))
> +	if (domain->dirty_ops &&
> +	    (!slads_supported(iommu) || pasid != IOMMU_NO_PASID))
>   		return -EINVAL;
> 
>   	/* check if this iommu agaw is sufficient for max mapped address */

[...]

> 
> @@ -4886,14 +4897,16 @@ static int intel_iommu_read_and_clear_dirty(struct
> iommu_domain *domain,
>   	unsigned long pgsize;
>   	bool ad_enabled;
> 
> -	spin_lock(&dmar_domain->lock);
> +	/*
> +	 * IOMMUFD core calls into a dirty tracking disabled domain without an
> +	 * IOVA bitmap set in order to clean dirty bits in all PTEs that might
> +	 * have occured when we stopped dirty tracking. This ensures that we
> +	 * never inherit dirtied bits from a previous cycle.
> +	 */
>   	ad_enabled = dmar_domain->dirty_tracking;
> -	spin_unlock(&dmar_domain->lock);
> -
>   	if (!ad_enabled && dirty->bitmap)

How about
	if (!dmar_domain->dirty_tracking && dirty->bitmap)
		return -EINVAL;
?

>   		return -EINVAL;
> 
> -	rcu_read_lock();
>   	do {
>   		struct dma_pte *pte;
>   		int lvl = 0;
> @@ -4906,14 +4919,10 @@ static int intel_iommu_read_and_clear_dirty(struct
> iommu_domain *domain,
>   			continue;
>   		}
> 
> -		/* It is writable, set the bitmap */
> -		if (((flags & IOMMU_DIRTY_NO_CLEAR) &&
> -				dma_sl_pte_dirty(pte)) ||
> -		    dma_sl_pte_test_and_clear_dirty(pte))
> +		if (dma_sl_pte_test_and_clear_dirty(pte, flags))
>   			iommu_dirty_bitmap_record(dirty, iova, pgsize);
>   		iova += pgsize;
>   	} while (iova < end);
> -	rcu_read_unlock();
> 
>   	return 0;
>   }
> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index bccd44db3316..0b390d9e669b 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -542,6 +542,9 @@ enum {
>   #define sm_supported(iommu)	(intel_iommu_sm && ecap_smts((iommu)->ecap))
>   #define pasid_supported(iommu)	(sm_supported(iommu) &&			\
>   				 ecap_pasid((iommu)->ecap))
> +#define slads_supported(iommu) (sm_supported(iommu) &&                 \
> +                                ecap_slads((iommu)->ecap))
> +
> 
>   struct pasid_entry;
>   struct pasid_state_entry;
> @@ -785,13 +788,12 @@ static inline bool dma_pte_present(struct dma_pte *pte)
>   	return (pte->val & 3) != 0;
>   }
> 
> -static inline bool dma_sl_pte_dirty(struct dma_pte *pte)
> +static inline bool dma_sl_pte_test_and_clear_dirty(struct dma_pte *pte,
> +						   unsigned long flags)
>   {
> -	return (pte->val & DMA_SL_PTE_DIRTY) != 0;
> -}
> +	if (flags & IOMMU_DIRTY_NO_CLEAR)
> +		return (pte->val & DMA_SL_PTE_DIRTY) != 0;
> 
> -static inline bool dma_sl_pte_test_and_clear_dirty(struct dma_pte *pte)
> -{
>   	return test_and_clear_bit(DMA_SL_PTE_DIRTY_BIT,
>   				  (unsigned long *)&pte->val);
>   }
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index 03814942d59c..785384a59d55 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -686,15 +686,29 @@ int intel_pasid_setup_dirty_tracking(struct intel_iommu
> *iommu,
> 
>   	spin_lock(&iommu->lock);
> 
> -	did = domain_id_iommu(domain, iommu);
>   	pte = intel_pasid_get_entry(dev, pasid);
>   	if (!pte) {
>   		spin_unlock(&iommu->lock);
> -		dev_err(dev, "Failed to get pasid entry of PASID %d\n", pasid);
> +		dev_err_ratelimited(dev,
> +				    "Failed to get pasid entry of PASID %d\n",
> +				    pasid);
>   		return -ENODEV;
>   	}
> 
> +	did = domain_id_iommu(domain, iommu);
>   	pgtt = pasid_pte_get_pgtt(pte);
> +	if (pgtt != PASID_ENTRY_PGTT_SL_ONLY && pgtt != PASID_ENTRY_PGTT_NESTED) {
> +		spin_unlock(&iommu->lock);
> +		dev_err_ratelimited(dev,
> +				    "Dirty tracking not supported on translation type %d\n",
> +				    pgtt);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (pasid_get_ssade(pte) == enabled) {
> +		spin_unlock(&iommu->lock);
> +		return 0;
> +	}
> 
>   	if (enabled)
>   		pasid_set_ssade(pte);
> @@ -702,6 +716,9 @@ int intel_pasid_setup_dirty_tracking(struct intel_iommu *iommu,
>   		pasid_clear_ssade(pte);
>   	spin_unlock(&iommu->lock);
> 
> +	if (!ecap_coherent(iommu->ecap))
> +		clflush_cache_range(pte, sizeof(*pte));
> +
>   	/*
>   	 * From VT-d spec table 25 "Guidance to Software for Invalidations":
>   	 *
> @@ -720,8 +737,6 @@ int intel_pasid_setup_dirty_tracking(struct intel_iommu *iommu,
> 
>   	if (pgtt == PASID_ENTRY_PGTT_SL_ONLY || pgtt == PASID_ENTRY_PGTT_NESTED)
>   		iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
> -	else
> -		qi_flush_piotlb(iommu, did, pasid, 0, -1, 0);
> 
>   	/* Device IOTLB doesn't need to be flushed in caching mode. */
>   	if (!cap_caching_mode(iommu->cap))

Others look good to me. Thanks a lot.

Best regards,
baolu