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

Baolu Lu
2023-10-17T12:50:10Z

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