Re: [PATCH 4/5] iommu/arm-smmu-v3: Add set_dirty_tracking() support

From: Joao Martins <joao.m.martins@oracle.com>
To: Jason Gunthorpe <jgg@nvidia.com>,
	Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Cc: "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"robin.murphy@arm.com" <robin.murphy@arm.com>,
	"will@kernel.org" <will@kernel.org>,
	"joro@8bytes.org" <joro@8bytes.org>,
	"kevin.tian@intel.com" <kevin.tian@intel.com>,
	"nicolinc@nvidia.com" <nicolinc@nvidia.com>,
	"mshavit@google.com" <mshavit@google.com>,
	"eric.auger@redhat.com" <eric.auger@redhat.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
Date: Thu, 14 Dec 2023 16:23:33 +0000	[thread overview]
Message-ID: <c9354a08-6aa5-42db-b8b5-44496eefb76f@oracle.com> (raw)
In-Reply-To: <20231130125412.GH1389974@nvidia.com>

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/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel