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
