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

Baolu Lu
2023-10-16T00:54:54Z

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