| Message ID | 20231128094940.1344-6-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/smmuv3: Add IOMMUFD dirty tracking support for SMMUv3 | expand |
On Tue, Nov 28, 2023 at 09:49:40AM +0000, Shameer Kolothum wrote: > @@ -2701,6 +2703,9 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > master = dev_iommu_priv_get(dev); > smmu = master->smmu; > > + if (domain->dirty_ops && !arm_smmu_dbm_capable(smmu)) > + return -EINVAL; > + This is not necessary, a domain can be attached to a single smmu and finalize was run on that smmu already. So dirty ops should only be set if this is a S1 domain finalized ona smmu that was dbm capable. Otherwise none of this makes any sense. > @@ -3104,6 +3115,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags, > > smmu_domain->domain.type = IOMMU_DOMAIN_UNMANAGED; > smmu_domain->domain.ops = arm_smmu_ops.default_domain_ops; > + if (enforce_dirty) > + smmu_domain->domain.dirty_ops = &arm_smmu_dirty_ops; Ah, this seems in the wrong place, perhaps that is the confusion everywhere? If the finalize actually enables dirty tracking in the pgtbl_ops then it should set the diryty_ops, they should not be set in alloc_user. Specifically, a S2 domain should never have dirty_ops set. IOW if domain.dirty_ops != NULL then pgtbl_ops != NULL && pgtbl_ops->read_and_clear_dirty Thus no need to have all the other prints/etc then. So I'd move this into finalize. > @@ -4152,11 +4166,13 @@ static void arm_smmu_get_httu(struct arm_smmu_device *smmu, u32 reg) > > if (smmu->dev->of_node) > smmu->features |= features; > - else if (features != fw_features) > + else if (features != fw_features) { > /* ACPI IORT sets the HTTU bits */ > dev_warn(smmu->dev, > - "IDR0.HTTU overridden by FW configuration (0x%x)\n", > + "IDR0.HTTU not overridden by FW configuration (0x%x)\n", > fw_features); > + smmu->features |= features; > + } > } Is this hunk misplaced? Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Wednesday, November 29, 2023 7:49 PM > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org; > robin.murphy@arm.com; will@kernel.org; joro@8bytes.org; > kevin.tian@intel.com; nicolinc@nvidia.com; mshavit@google.com; > eric.auger@redhat.com; joao.m.martins@oracle.com; jiangkunkun > <jiangkunkun@huawei.com>; zhukeqian <zhukeqian1@huawei.com>; Linuxarm > <linuxarm@huawei.com> > Subject: Re: [PATCH 5/5] iommu/arm-smmu-v3: Enforce dirty tracking in domain > attach/alloc > > On Tue, Nov 28, 2023 at 09:49:40AM +0000, Shameer Kolothum wrote: > > @@ -2701,6 +2703,9 @@ static int arm_smmu_attach_dev(struct > iommu_domain *domain, struct device *dev) > > master = dev_iommu_priv_get(dev); > > smmu = master->smmu; > > > > + if (domain->dirty_ops && !arm_smmu_dbm_capable(smmu)) > > + return -EINVAL; > > + > > This is not necessary, a domain can be attached to a single smmu and > finalize was run on that smmu already. So dirty ops should only be set > if this is a S1 domain finalized ona smmu that was dbm capable. > > Otherwise none of this makes any sense. > > > @@ -3104,6 +3115,9 @@ arm_smmu_domain_alloc_user(struct device *dev, > u32 flags, > > > > smmu_domain->domain.type = IOMMU_DOMAIN_UNMANAGED; > > smmu_domain->domain.ops = arm_smmu_ops.default_domain_ops; > > + if (enforce_dirty) > > + smmu_domain->domain.dirty_ops = &arm_smmu_dirty_ops; > > Ah, this seems in the wrong place, perhaps that is the confusion > everywhere? > > If the finalize actually enables dirty tracking in the pgtbl_ops then > it should set the diryty_ops, they should not be set in alloc_user. > > Specifically, a S2 domain should never have dirty_ops set. > > IOW if domain.dirty_ops != NULL then pgtbl_ops != NULL && pgtbl_ops- > >read_and_clear_dirty > > Thus no need to have all the other prints/etc then. > > So I'd move this into finalize. Ok. Make sense. I will move it. > > > @@ -4152,11 +4166,13 @@ static void arm_smmu_get_httu(struct > arm_smmu_device *smmu, u32 reg) > > > > if (smmu->dev->of_node) > > smmu->features |= features; > > - else if (features != fw_features) > > + else if (features != fw_features) { > > /* ACPI IORT sets the HTTU bits */ > > dev_warn(smmu->dev, > > - "IDR0.HTTU overridden by FW configuration (0x%x)\n", > > + "IDR0.HTTU not overridden by FW configuration > (0x%x)\n", > > fw_features); > > + smmu->features |= features; > > + } > > } > > Is this hunk misplaced? Oops... My bad. Added for testing. Thanks, Shameer
On 28/11/2023 09:49, Shameer Kolothum wrote: > From: Joao Martins <joao.m.martins@oracle.com> > > SMMUv3 implements all requirements of revoking device > attachment if smmu does not support dirty tracking. > > Finally handle the IOMMU_CAP_DIRTY in iommu_capable for > IOMMUFD_DEVICE_GET_HW_INFO. > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 22 ++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > index c5eabdc29ba5..2c100f2136bc 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2244,6 +2244,8 @@ static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap) > case IOMMU_CAP_NOEXEC: > case IOMMU_CAP_DEFERRED_FLUSH: > return true; > + case IOMMU_CAP_DIRTY_TRACKING: > + return arm_smmu_dbm_capable(master->smmu); > default: > return false; > } > @@ -2701,6 +2703,9 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > master = dev_iommu_priv_get(dev); > smmu = master->smmu; > > + if (domain->dirty_ops && !arm_smmu_dbm_capable(smmu)) > + return -EINVAL; > + > mutex_lock(&smmu_domain->init_mutex); > > if (!smmu_domain->smmu) { > @@ -3077,7 +3082,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags, > const struct iommu_user_data *user_data) > { > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > - const u32 paging_flags = IOMMU_HWPT_ALLOC_NEST_PARENT; > + const u32 paging_flags = IOMMU_HWPT_ALLOC_NEST_PARENT | > + IOMMU_HWPT_ALLOC_DIRTY_TRACKING; > + bool enforce_dirty = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING; > struct arm_smmu_domain *smmu_domain; > int ret; > > @@ -3090,6 +3097,10 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags, > if (user_data) > return ERR_PTR(-EINVAL); > > + if (enforce_dirty && > + !device_iommu_capable(dev, IOMMU_CAP_DIRTY_TRACKING)) > + return ERR_PTR(-EOPNOTSUPP); > + > smmu_domain = arm_smmu_domain_alloc(); > if (!smmu_domain) > return ERR_PTR(-ENOMEM); > @@ -3104,6 +3115,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags, > > smmu_domain->domain.type = IOMMU_DOMAIN_UNMANAGED; > smmu_domain->domain.ops = arm_smmu_ops.default_domain_ops; > + if (enforce_dirty) > + smmu_domain->domain.dirty_ops = &arm_smmu_dirty_ops; > + > ret = arm_smmu_domain_finalise(smmu_domain, master->smmu); > if (ret) > goto err_free; Perhaps it would be better to limit the blast radius of the always-on mode, to just enable it in the context descriptor depending on passing the HWPT_ALLOC_DIRTY_TRACKING flag instead of enabling for everybody. Something like this in arm_smmu_domain_finalise(): if (arm_smmu_dbm_capable(smmu) && smmu_domain->domain.dirty_ops && smmu_domain->stage == ARM_SMMU_DOMAIN_S1) pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_ARM_HD; For bisectability perhaps the second patch of this series should be the last, while bringing the implementation of arm_smmu_dbm_capable() to this patch.
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index c5eabdc29ba5..2c100f2136bc 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2244,6 +2244,8 @@ static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap) case IOMMU_CAP_NOEXEC: case IOMMU_CAP_DEFERRED_FLUSH: return true; + case IOMMU_CAP_DIRTY_TRACKING: + return arm_smmu_dbm_capable(master->smmu); default: return false; } @@ -2701,6 +2703,9 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) master = dev_iommu_priv_get(dev); smmu = master->smmu; + if (domain->dirty_ops && !arm_smmu_dbm_capable(smmu)) + return -EINVAL; + mutex_lock(&smmu_domain->init_mutex); if (!smmu_domain->smmu) { @@ -3077,7 +3082,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags, const struct iommu_user_data *user_data) { struct arm_smmu_master *master = dev_iommu_priv_get(dev); - const u32 paging_flags = IOMMU_HWPT_ALLOC_NEST_PARENT; + const u32 paging_flags = IOMMU_HWPT_ALLOC_NEST_PARENT | + IOMMU_HWPT_ALLOC_DIRTY_TRACKING; + bool enforce_dirty = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING; struct arm_smmu_domain *smmu_domain; int ret; @@ -3090,6 +3097,10 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags, if (user_data) return ERR_PTR(-EINVAL); + if (enforce_dirty && + !device_iommu_capable(dev, IOMMU_CAP_DIRTY_TRACKING)) + return ERR_PTR(-EOPNOTSUPP); + smmu_domain = arm_smmu_domain_alloc(); if (!smmu_domain) return ERR_PTR(-ENOMEM); @@ -3104,6 +3115,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags, smmu_domain->domain.type = IOMMU_DOMAIN_UNMANAGED; smmu_domain->domain.ops = arm_smmu_ops.default_domain_ops; + if (enforce_dirty) + smmu_domain->domain.dirty_ops = &arm_smmu_dirty_ops; + ret = arm_smmu_domain_finalise(smmu_domain, master->smmu); if (ret) goto err_free; @@ -4152,11 +4166,13 @@ static void arm_smmu_get_httu(struct arm_smmu_device *smmu, u32 reg) if (smmu->dev->of_node) smmu->features |= features; - else if (features != fw_features) + else if (features != fw_features) { /* ACPI IORT sets the HTTU bits */ dev_warn(smmu->dev, - "IDR0.HTTU overridden by FW configuration (0x%x)\n", + "IDR0.HTTU not overridden by FW configuration (0x%x)\n", fw_features); + smmu->features |= features; + } } static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)