Re: [PATCH RFCv2 04/24] iommu: Add iommu_domain ops for dirty tracking

From: Jason Gunthorpe <jgg@nvidia.com>
To: Joao Martins <joao.m.martins@oracle.com>
Cc: Robin Murphy <robin.murphy@arm.com>,
	iommu@lists.linux.dev, Kevin Tian <kevin.tian@intel.com>,
	Shameerali Kolothum Thodi  <shameerali.kolothum.thodi@huawei.com>,
	Lu Baolu <baolu.lu@linux.intel.com>, Yi Liu <yi.l.liu@intel.com>,
	Yi Y Sun <yi.y.sun@intel.com>, Eric Auger <eric.auger@redhat.com>,
	Nicolin Chen <nicolinc@nvidia.com>,
	Joerg Roedel <joro@8bytes.org>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Will Deacon <will@kernel.org>,
	Alex Williamson <alex.williamson@redhat.com>,
	kvm@vger.kernel.org
Subject: Re: [PATCH RFCv2 04/24] iommu: Add iommu_domain ops for dirty tracking
Date: Thu, 10 Aug 2023 22:09:56 -0300	[thread overview]
Message-ID: <ZNWKZLJUMnVOEjef@nvidia.com> (raw)
In-Reply-To: <113513fa-ac9c-31c4-6d97-ca2055ceafed@oracle.com>

On Thu, Aug 10, 2023 at 09:36:37PM +0100, Joao Martins wrote:

> > Yes, the "domain_default_ops" is basically a transitional hack to
> > help migrate to narrowly defined per-usage domain ops.
> > 
> > eg things like blocking and identity should not have mapping ops.
> > 
> My earlier point was more about not what 'domain_default_ops' represents
> but that it's a pointer. Shared by everyone (devices and domains alike). But you
> sort of made it clear that it's OK to duplicate it to not have dirty tracking.
> The duplication is what I felt a little odd.

Well, it is one path, we could also add a dirty_ops to the
domain. Hard to say which is better.

> (...) I wasn't quite bodging, just trying to parallelize what was bus cleanup
> could be tackling domain/device-independent ops without them being global. Maybe
> I read too much into it hence my previous question.

domain_alloc_user bypasses the bus cleanup

> > Return the IOMMU_CAP_DIRTY as generic data in the new GET_INFO
> 
> I have this one here:
> 
> https://lore.kernel.org/linux-iommu/20230518204650.14541-14-joao.m.martins@oracle.com/
> 
> I can rework to GET_HW_INFO but it really needs to be generic bits of data and
> not iommu hw specific e.g. that translates into device_iommu_capable() cap
> checking. 

Yes, HW_INFO seems the right way. Just add a

   __aligned_u64 out_capabilities;

To that struct iommu_hw_info and fill it with generic code.

> > Accept some generic flag in the alloc_hwpt requesting dirty
> > Pass generic flags down to the driver.
> > Reject set flags and drivers that don't implement alloc_domain_user.
> > Driver refuses to attach the dirty enabled domain to places that do
> > dirty tracking.
> 
> This is already done, and so far I have an unsigned long flags to
> domain_alloc_user() and probably be kept defining it as
> iommu-domain-feature bit

Yes a flag in some way is the best choice

> (unless it's better to follow similar direction as hwpt_type like in
> domain_alloc_user). And gets stored as iommu_domain::flags, like this series
> had. Though if majority of driver rejection flows via alloc_domain_user only
> (which has a struct device), perhaps it's not even needed to store as a new
> iommu_domain::flags

Right, we don't need to reflect it back if the dirty ops are NULL.

Jason