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: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>,
	"Tian, Kevin" <kevin.tian@intel.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: Wed, 22 May 2024 13:56:40 -0300	[thread overview]
Message-ID: <20240522165640.GD20229@nvidia.com> (raw)
In-Reply-To: <4618c24f-aaa8-453f-aa9d-67a864e1c93b@oracle.com>

On Wed, May 22, 2024 at 03:37:57PM +0100, Joao Martins wrote:

> 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(). 

Is that broken then?

iopt_clear_dirty_data() sets the NULL:

	iommu_dirty_bitmap_init(&dirty, NULL, &gather);
		ret = ops->read_and_clear_dirty(domain, iopt_area_iova(area),
						iopt_area_length(area), 0,
						&dirty);

But AMD, for instance, does nothing:

	spin_lock_irqsave(&pdomain->lock, lflags);
	if (!pdomain->dirty_tracking && dirty->bitmap) {
		spin_unlock_irqrestore(&pdomain->lock, lflags);
		return -EINVAL;
	}
	spin_unlock_irqrestore(&pdomain->lock, lflags);

	return ops->read_and_clear_dirty(ops, iova, size, flags, dirty);

It certainly didn't clear the IOPTEs.

AFAIK the NULL should be ignored here:

static inline void iommu_dirty_bitmap_record(struct iommu_dirty_bitmap *dirty,
					     unsigned long iova,
					     unsigned long length)
{
	if (dirty->bitmap)
		iova_bitmap_set(dirty->bitmap, iova, length);

Not above. That looks like a bug. Yes?

Thanks,
Jason

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