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

[Joao Martins]

On 22/05/2024 15:03, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Tian, Kevin <kevin.tian at intel.com>
>> Sent: Wednesday, May 22, 2024 8:12 AM
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi at huawei.com>;
>> iommu at lists.linux.dev; linux-arm-kernel at lists.infradead.org
>> Cc: robin.murphy at arm.com; will at kernel.org; joro at 8bytes.org;
>> jgg at nvidia.com; ryan.roberts at arm.com; nicolinc at nvidia.com;
>> mshavit at google.com; eric.auger at redhat.com; joao.m.martins at oracle.com;
>> jiangkunkun <jiangkunkun at huawei.com>; zhukeqian
>> <zhukeqian1 at huawei.com>; Linuxarm <linuxarm at huawei.com>
>> Subject: RE: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty()
>> support
>>
>>> From: Shameer Kolothum <shameerali.kolothum.thodi at 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.