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

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

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.

> +		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?


[...]
         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;

>   
> @@ -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?

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

>   	default:
>   		return false;
>   	}

Best regards,
baolu