| Message ID | 20231128094940.1344-5-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:39AM +0000, Shameer Kolothum wrote: > From: Joao Martins <joao.m.martins@oracle.com> > > Dirty tracking will always be enabled with DBM=1 modifier enabled > by default when HD is supported. Is this trying to say that ARM doesn't have a per-table global enable for dirty tracking but instead pre-sets the DBM bit to avoid the cost? So on smmuv3 to enable we have to clear everything and disable continues to pay a penalty since we don't go and mark all things as dirty again? Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Wednesday, November 29, 2023 7:43 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 4/5] iommu/arm-smmu-v3: Add set_dirty_tracking() > support > > On Tue, Nov 28, 2023 at 09:49:39AM +0000, Shameer Kolothum wrote: > > From: Joao Martins <joao.m.martins@oracle.com> > > > > Dirty tracking will always be enabled with DBM=1 modifier enabled > > by default when HD is supported. > > Is this trying to say that ARM doesn't have a per-table global enable > for dirty tracking but instead pre-sets the DBM bit to avoid the cost? > Yes. SMMUv3 has per-PTE DBM control and I think the initial RFC had it walking the PTEs and setting the DBM on set_dirty_tracking(). But it complicates things if we have to incorporates updating entries for any new mappings. And it was suggested to keep it enabled here, https://lore.kernel.org/kvm/2d369e58-8ac0-f263-7b94-fe73917782e1@linux.intel.com/T/#m3be8a185668810fb24a030617f5eaf79a9a32748 > So on smmuv3 to enable we have to clear everything and disable > continues to pay a penalty since we don't go and mark all things as > dirty again? Yes we clear everything on enable. Sorry I didn't get the second part. We don't mark dirty on disable. How is that different from Intel/AMD? Thanks, Shameer
On Thu, Nov 30, 2023 at 08:56:32AM +0000, Shameerali Kolothum Thodi wrote: > > On Tue, Nov 28, 2023 at 09:49:39AM +0000, Shameer Kolothum wrote: > > > From: Joao Martins <joao.m.martins@oracle.com> > > > > > > Dirty tracking will always be enabled with DBM=1 modifier enabled > > > by default when HD is supported. > > > > Is this trying to say that ARM doesn't have a per-table global enable > > for dirty tracking but instead pre-sets the DBM bit to avoid the cost? > > > > Yes. SMMUv3 has per-PTE DBM control and I think the initial RFC had > it walking the PTEs and setting the DBM on set_dirty_tracking(). set_dirty_tracking doesn't have access to the necessary locking to touch the PTEs. > > So on smmuv3 to enable we have to clear everything and disable > > continues to pay a penalty since we don't go and mark all things as > > dirty again? > > Yes we clear everything on enable. Sorry I didn't get the second part. > We don't mark dirty on disable. How is that different from Intel/AMD? Intel/AMD have a global switch so they just turn off the tracking and stop paying the cost. This approach on ARM means once the tracking is logically turned off the HW will continue to generate memory traffic to set dirty bits on DMAs. There is no way to back to the at-start state where their is 0 memory traffic on DMAs. Not sure that it totally matters, but it is worth noting someplace. If we do want to solve this then ARM would need iommufd to make a pass over the page table to set for disable similar to how we have to clear for enable. Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Thursday, November 30, 2023 12:54 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 4/5] iommu/arm-smmu-v3: Add set_dirty_tracking() > support > > On Thu, Nov 30, 2023 at 08:56:32AM +0000, Shameerali Kolothum Thodi > wrote: > > > On Tue, Nov 28, 2023 at 09:49:39AM +0000, Shameer Kolothum wrote: > > > > From: Joao Martins <joao.m.martins@oracle.com> > > > > > > > > Dirty tracking will always be enabled with DBM=1 modifier enabled > > > > by default when HD is supported. > > > > > > Is this trying to say that ARM doesn't have a per-table global enable > > > for dirty tracking but instead pre-sets the DBM bit to avoid the cost? > > > > > > > Yes. SMMUv3 has per-PTE DBM control and I think the initial RFC had > > it walking the PTEs and setting the DBM on set_dirty_tracking(). > > set_dirty_tracking doesn't have access to the necessary locking to > touch the PTEs. > > > > So on smmuv3 to enable we have to clear everything and disable > > > continues to pay a penalty since we don't go and mark all things as > > > dirty again? > > > > Yes we clear everything on enable. Sorry I didn't get the second part. > > We don't mark dirty on disable. How is that different from Intel/AMD? > > Intel/AMD have a global switch so they just turn off the tracking and > stop paying the cost. > > This approach on ARM means once the tracking is logically turned off > the HW will continue to generate memory traffic to set dirty bits on > DMAs. There is no way to back to the at-start state where their is 0 > memory traffic on DMAs. Not sure that it totally matters, but it is > worth noting someplace. Ah..you meant the HW penalty of keeping it on always. > If we do want to solve this then ARM would need iommufd to make a pass > over the page table to set for disable similar to how we have to clear > for enable. I will try to see if there is any overhead in keeping it on. Also we could limit turning it on for DOMAIN_NESTED ? Thanks, Shameer
On 30/11/2023 12:54, Jason Gunthorpe wrote: > On Thu, Nov 30, 2023 at 08:56:32AM +0000, Shameerali Kolothum Thodi wrote: >>> On Tue, Nov 28, 2023 at 09:49:39AM +0000, Shameer Kolothum wrote: >>>> From: Joao Martins <joao.m.martins@oracle.com> >>>> >>>> Dirty tracking will always be enabled with DBM=1 modifier enabled >>>> by default when HD is supported. >>> >>> Is this trying to say that ARM doesn't have a per-table global enable >>> for dirty tracking but instead pre-sets the DBM bit to avoid the cost? >>> >> >> Yes. SMMUv3 has per-PTE DBM control and I think the initial RFC had >> it walking the PTEs and setting the DBM on set_dirty_tracking(). > > set_dirty_tracking doesn't have access to the necessary locking to > touch the PTEs. > This is done for free by iopt_clear_dirty_data(). set_dirty_tracking() is mostly a nop, under the assumption that dirty tracking is always enabled. >>> So on smmuv3 to enable we have to clear everything and disable >>> continues to pay a penalty since we don't go and mark all things as >>> dirty again? >> >> Yes we clear everything on enable. Sorry I didn't get the second part. >> We don't mark dirty on disable. How is that different from Intel/AMD? > > Intel/AMD have a global switch so they just turn off the tracking and > stop paying the cost. > > This approach on ARM means once the tracking is logically turned off > the HW will continue to generate memory traffic to set dirty bits on > DMAs. There is no way to back to the at-start state where their is 0 > memory traffic on DMAs. Not sure that it totally matters, but it is > worth noting someplace. > > If we do want to solve this then ARM would need iommufd to make a pass > over the page table to set for disable similar to how we have to clear > for enable. The firsts attempt at this had to be dynamic[0]. Like set_dirty_tracking_range would pass over the io pagetable and set the DBM bit to enable dirty tracking. It was suggested that we switch to an always-on mode to simplify initial bringup of the feature, and if the always-on was somehow affecting DMA performance, we would re-attempt at this dynamic mode post-mortem. With the current code structure, perhaps having set_dirty_tracking() do the DBM-enable pass we would need to move iorw_sem section to include the call to set_dirty_tracking() and smmu op set_dirty_tracking() op would walk the whole pt, without relying on iopt areas. Or we go back to iterating areas similar to [0] with a new op. [0] https://lore.kernel.org/kvm/20220428210933.3583-16-joao.m.martins@oracle.com/
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 8331a2c70a0c..c5eabdc29ba5 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -3402,6 +3402,27 @@ static int arm_smmu_read_and_clear_dirty(struct iommu_domain *domain, return ret; } +static int arm_smmu_set_dirty_tracking(struct iommu_domain *domain, + bool enabled) +{ + struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); + struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops; + + if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1) + return -EINVAL; + + if (!ops) { + pr_err_once("io-pgtable don't support dirty tracking\n"); + return -ENODEV; + } + + /* + * Always enabled and the dirty bitmap is cleared prior to + * set_dirty_tracking(). + */ + return 0; +} + static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args) { return iommu_fwspec_add_ids(dev, args->args, 1); @@ -3529,6 +3550,7 @@ static struct iommu_ops arm_smmu_ops = { static struct iommu_dirty_ops arm_smmu_dirty_ops = { .read_and_clear_dirty = arm_smmu_read_and_clear_dirty, + .set_dirty_tracking = arm_smmu_set_dirty_tracking, }; /* Probing and initialisation functions */