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

From: Joao Martins <joao.m.martins@oracle.com>
To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>,
	"Tian, Kevin" <kevin.tian@intel.com>
Cc: "robin.murphy@arm.com" <robin.murphy@arm.com>,
	"will@kernel.org" <will@kernel.org>,
	"joro@8bytes.org" <joro@8bytes.org>,
	"jgg@nvidia.com" <jgg@nvidia.com>,
	"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: Wed, 22 May 2024 15:37:57 +0100	[thread overview]
Message-ID: <4618c24f-aaa8-453f-aa9d-67a864e1c93b@oracle.com> (raw)
In-Reply-To: <13e93238f21b42539097d7e347be684e@huawei.com>

On 22/05/2024 15:03, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Tian, Kevin <kevin.tian@intel.com>
>> Sent: Wednesday, May 22, 2024 8:12 AM
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org
>> Cc: robin.murphy@arm.com; will@kernel.org; joro@8bytes.org;
>> jgg@nvidia.com; ryan.roberts@arm.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 v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty()
>> support
>>
>>> From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>>> Sent: Tuesday, April 30, 2024 9:43 PM
>>> +
>>> +static int arm_lpae_read_and_clear_dirty(struct io_pgtable_ops *ops,
>>> +					 unsigned long iova, size_t size,
>>> +					 unsigned long flags,
>>> +					 struct iommu_dirty_bitmap *dirty)
>>> +{
>>> +	struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
>>> +	struct io_pgtable_cfg *cfg = &data->iop.cfg;
>>> +	struct io_pgtable_walk_data walk_data = {
>>> +		.dirty = dirty,
>>> +		.flags = flags,
>>> +		.addr = iova,
>>> +		.end = iova + size,
>>> +	};
>>> +	arm_lpae_iopte *ptep = data->pgd;
>>> +	int lvl = data->start_level;
>>> +
>>> +	if (WARN_ON(!size))
>>> +		return -EINVAL;
>>> +	if (WARN_ON((iova + size - 1) & ~(BIT(cfg->ias) - 1)))
>>> +		return -EINVAL;
>>> +	if (data->iop.fmt != ARM_64_LPAE_S1)
>>> +		return -EINVAL;
>>> +
>>> +	return __arm_lpae_iopte_walk_dirty(data, &walk_data, ptep, lvl);
>>
>> Intel/AMD drivers also checks:
>>
>>         if (!dmar_domain->dirty_tracking && dirty->bitmap)
>>                 return -EINVAL;
> 
> Is that really required? Is the concern here is user may issue
> GET_DIRTY_BITMAP IOCTL without any validation and that may
> result in unnecessary scanning of page tables? May be we should
> handle it in core code then I think.

This is just to catch the case where IOMMUFD can call into read_and_clear()
without dirty tracking enabled and without a bitmap structure to clear dirty
bits -- in order to ensure a clean PTE data snapshot after start(). And thus it
errors out on others.

Right now we don't track in iommufd that dirty tracking is active in the domain,
vendor does so in its own way. I mean we could move that to core, but we end up
duplicating what the iommu driver is already doing.

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