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

Joao Martins
2023-10-16T10:44:59Z

On 16/10/2023 01:51, Baolu Lu wrote:
> On 9/23/23 9:25 AM, Joao Martins wrote:
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -300,6 +300,7 @@ static int iommu_skip_te_disable;
>>   #define IDENTMAP_AZALIA        4
>>     const struct iommu_ops intel_iommu_ops;
>> +const struct iommu_dirty_ops intel_dirty_ops;
>>     static bool translation_pre_enabled(struct intel_iommu *iommu)
>>   {
>> @@ -4077,6 +4078,7 @@ static struct iommu_domain
>> *intel_iommu_domain_alloc(unsigned type)
>>   static struct iommu_domain *
>>   intel_iommu_domain_alloc_user(struct device *dev, u32 flags)
>>   {
>> +    bool enforce_dirty = (flags & IOMMU_HWPT_ALLOC_ENFORCE_DIRTY);
>>       struct iommu_domain *domain;
>>       struct intel_iommu *iommu;
>>   @@ -4087,9 +4089,15 @@ intel_iommu_domain_alloc_user(struct device *dev, u32
>> flags)
>>       if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !ecap_nest(iommu->ecap))
>>           return ERR_PTR(-EOPNOTSUPP);
>>   +    if (enforce_dirty &&
>> +        !device_iommu_capable(dev, IOMMU_CAP_DIRTY))
>> +        return ERR_PTR(-EOPNOTSUPP);
>> +
>>       domain = iommu_domain_alloc(dev->bus);
>>       if (!domain)
>>           domain = ERR_PTR(-ENOMEM);
>> +    if (domain && enforce_dirty)
>
> @domain can not be NULL here.
>
True, it should be:

if (!IS_ERR(domain) && enforce_dirty)

>> +        domain->dirty_ops = &intel_dirty_ops;
>>       return domain;
>>   }
>
> The VT-d driver always uses second level for a user domain translation.
> In order to avoid checks of "domain->use_first_level" in the callbacks,
> how about check it here and return failure if first level is used for
> user domain?
>

I was told by Yi Y Sun offlist to have the first_level checked, because dirty
bit in first stage page table is always enabled (and cannot be toggled on/off).
I can remove it again; initially RFC didn't have it as it was failing in similar
way to how you suggest here. Not sure how to proceed?

>
> [...]
>         domain = iommu_domain_alloc(dev->bus);
>         if (!domain)
>                 return ERR_PTR(-ENOMEM);
>
>         if (enforce_dirty) {
>                 if (to_dmar_domain(domain)->use_first_level) {
>                         iommu_domain_free(domain);
>                         return ERR_PTR(-EOPNOTSUPP);
>                 }
>                 domain->dirty_ops = &intel_dirty_ops;
>         }
>
>         return domain;
>

Should I fail on first level pagetable dirty enforcement, certainly will adopt
the above (and remove the sucessfully return on first_level set_dirty_tracking)

>>   @@ -4367,6 +4375,9 @@ static bool intel_iommu_capable(struct device *dev,
>> enum iommu_cap cap)
>>           return dmar_platform_optin();
>>       case IOMMU_CAP_ENFORCE_CACHE_COHERENCY:
>>           return ecap_sc_support(info->iommu->ecap);
>> +    case IOMMU_CAP_DIRTY:
>> +        return sm_supported(info->iommu) &&
>> +            ecap_slads(info->iommu->ecap);
>
> Above appears several times in this patch. Is it possible to define it
> as a macro?
>
Yeap, for sure much cleaner indeed.

> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index bccd44db3316..379e141bbb28 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -542,6 +542,8 @@ 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))
>
Yeap.

>>       default:
>>           return false;
>>       }
>
> Best regards,
> baolu