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

Joao Martins
2023-10-17T14:19:58Z

On 17/10/2023 13:49, Baolu Lu wrote:
> 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().
>
OK

>>   {
>>       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;
> ?
>
OK

>>           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.
>

Joao