Re: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

From: Jason Gunthorpe <jgg@nvidia.com>
To: Joao Martins <joao.m.martins@oracle.com>
Cc: "Tian, Kevin" <kevin.tian@intel.com>,
	Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>,
	"robin.murphy@arm.com" <robin.murphy@arm.com>,
	"will@kernel.org" <will@kernel.org>,
	"joro@8bytes.org" <joro@8bytes.org>,
	"ryan.roberts@arm.com" <ryan.roberts@arm.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>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>
Subject: Re: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support
Date: Fri, 24 May 2024 11:07:54 -0300	[thread overview]
Message-ID: <20240524140754.GZ20229@nvidia.com> (raw)
In-Reply-To: <2d949dfb-ea40-418b-81f2-f2711b796d4a@oracle.com>

On Fri, May 24, 2024 at 12:30:22PM +0100, Joao Martins wrote:
> On 23/05/2024 04:30, Tian, Kevin wrote:
> >> From: Jason Gunthorpe <jgg@nvidia.com>
> >> Sent: Thursday, May 23, 2024 1:51 AM
> >>
> >> diff --git a/drivers/iommu/iommufd/iommufd_private.h
> >> b/drivers/iommu/iommufd/iommufd_private.h
> >> index 991f864d1f9bc1..de3761e15cab54 100644
> >> --- a/drivers/iommu/iommufd/iommufd_private.h
> >> +++ b/drivers/iommu/iommufd/iommufd_private.h
> >> @@ -52,6 +52,7 @@ struct io_pagetable {
> >>  	/* IOVA that cannot be allocated, struct iopt_reserved */
> >>  	struct rb_root_cached reserved_itree;
> >>  	u8 disable_large_pages;
> >> +	u8 dirty_tracking_enabled;
> >>  	unsigned long iova_alignment;
> >>  };
> >>
> > 
> > should it be a hwpt flag instead?
> > 
> 
> Most of this deals with iopt locks and walking iopt areas to clear dirty. So
> this being a iopt attribute looks cleaner in implementation. But I think I see
> your point suggestion considering it represents a iommu domain property.

Yeah, the original idea of the hwpt/iopt split was to keep code that
was principly iommu_domain related away from the code which was
principally attachment and lifetime related. Since this value is
covered by iopt locks it makes sense in this struct.

Not sure the split will stand the test of time as we keep finding
reasons to muddle the boundary :\

Jason